From cd28a4cc2ca5627a105cb751fad53efc11232880 Mon Sep 17 00:00:00 2001 From: David Veszelovszki Date: Tue, 29 Nov 2022 12:08:54 +0100 Subject: [PATCH] Log external requests (#44286) * Added Redis access * Added RedisLogItem with request+response info and stack traces * Added RedisLoggerMiddleware, with sensitive header removal * Added GraphQL API endpoint, for site admins only, matching the Cursor Connections spec * Added admin UI with five-sec polling and "copy curl" feature * Added back-end tests * Added log limit setting with default=50, range: 0..500 * Added changelog item * Deduplicated WebhookLogHeader with HTTPHeader * Unrelated: Fixed two typos * Use for the API * Made the CSS work well with both light and dark themes and made sure it's accessible. Tested with a screen reader. Co-authored-by: Robert Lin --- CHANGELOG.md | 1 + client/web/src/jscontext.ts | 2 + .../SiteAdminOutboundRequestsPage.module.scss | 82 ++++ .../SiteAdminOutboundRequestsPage.tsx | 354 ++++++++++++++++++ client/web/src/site-admin/backend.ts | 29 ++ client/web/src/site-admin/routes.tsx | 5 + client/web/src/site-admin/sidebaritems.ts | 5 + cmd/frontend/graphqlbackend/graphqlbackend.go | 3 + .../graphqlbackend/outbound_requests.go | 145 +++++++ cmd/frontend/graphqlbackend/schema.graphql | 112 +++++- .../graphqlbackend/settings_cascade.go | 2 +- cmd/frontend/graphqlbackend/webhook_logs.go | 25 +- .../internal/app/jscontext/jscontext.go | 4 + internal/conf/init.go | 14 +- internal/httpcli/client.go | 48 ++- internal/httpcli/client_test.go | 62 +++ internal/httpcli/external.go | 14 + internal/httpcli/redis_logger_middleware.go | 216 +++++++++++ .../httpcli/redis_logger_middleware_test.go | 95 +++++ internal/httptestutil/recorder.go | 34 +- internal/rcache/rcache.go | 44 ++- internal/rcache/rcache_test.go | 13 +- internal/types/types.go | 15 + schema/schema.go | 2 + schema/site.schema.json | 7 + 25 files changed, 1241 insertions(+), 92 deletions(-) create mode 100644 client/web/src/site-admin/SiteAdminOutboundRequestsPage.module.scss create mode 100644 client/web/src/site-admin/SiteAdminOutboundRequestsPage.tsx create mode 100644 cmd/frontend/graphqlbackend/outbound_requests.go create mode 100644 internal/httpcli/redis_logger_middleware.go create mode 100644 internal/httpcli/redis_logger_middleware_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e30ba86fb41..2a3161d9900 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ All notable changes to Sourcegraph are documented in this file. - Unindexed search now use the index for files that have not changed between the unindexed commit and the indexed commit. The result is faster unindexed search in general. If you are noticing issues you can disable by setting the feature flag `search-hybrid` to false. [#37112](https://github.com/sourcegraph/sourcegraph/issues/37112) - The number of commits listed in the History tab can now be customized for all users by site admins under Configuration -> Global Settings from the site admin page by using the config `history.defaultPageSize`. Individual users may also set `history.defaultPagesize` from their user settings page to override the value set under the Global Settings. [#44651](https://github.com/sourcegraph/sourcegraph/pull/44651) - Batch Changes: Mounted files can be accessed via the UI on the executions page. [#43180](https://github.com/sourcegraph/sourcegraph/pull/43180) +- Added "Outbound request log" feature for site admins [#44286](https://github.com/sourcegraph/sourcegraph/pull/44286) ### Changed diff --git a/client/web/src/jscontext.ts b/client/web/src/jscontext.ts index 5a050d201c7..6a7512bd08e 100644 --- a/client/web/src/jscontext.ts +++ b/client/web/src/jscontext.ts @@ -169,6 +169,8 @@ export interface SourcegraphContext extends Pick, 'e /** Prompt users with browsers that would crash to download a modern browser. */ RedirectUnsupportedBrowser?: boolean + + outboundRequestLogLimit?: number } export interface BrandAssets { diff --git a/client/web/src/site-admin/SiteAdminOutboundRequestsPage.module.scss b/client/web/src/site-admin/SiteAdminOutboundRequestsPage.module.scss new file mode 100644 index 00000000000..3d2975dcb0c --- /dev/null +++ b/client/web/src/site-admin/SiteAdminOutboundRequestsPage.module.scss @@ -0,0 +1,82 @@ +@import 'wildcard/src/global-styles/breakpoints'; + +.requests-grid { + display: grid; + grid-template-columns: max-content max-content max-content minmax(0, auto) max-content max-content; + row-gap: 0.2rem; + column-gap: 0.5rem; + align-items: center; + @media (--sm-breakpoint-down) { + row-gap: 0.5rem; + column-gap: 0.5rem; + } +} + +.url-column { + overflow: hidden; + text-overflow: ellipsis; +} + +.successful { + color: var(--success); +} + +.failed { + color: var(--danger); +} + +.separator { + /* Make it full width in the current row. */ + grid-column: 1 / -1; + border-top: 1px solid var(--border-color-2); + @media (--xs-breakpoint-down) { + margin-top: 1rem; + padding-bottom: 1rem; + } +} + +.more-info { + overflow-wrap: break-word; + + strong { + color: var(--primary); + } +} + +.method { + &.get { + color: var(--blue); + } + + &.post { + color: var(--indigo); + } + + &.put { + color: var(--purple); + } + + &.patch { + color: var(--pink); + } + + &.delete { + color: var(--red); + } + + &.head { + color: var(--orange); + } + + &.options { + color: var(--yellow); + } + + &.connect { + color: var(--green); + } + + &.trace { + color: var(--teal); + } +} diff --git a/client/web/src/site-admin/SiteAdminOutboundRequestsPage.tsx b/client/web/src/site-admin/SiteAdminOutboundRequestsPage.tsx new file mode 100644 index 00000000000..b2b4eadbb7f --- /dev/null +++ b/client/web/src/site-admin/SiteAdminOutboundRequestsPage.tsx @@ -0,0 +1,354 @@ +import React, { ReactNode, useCallback, useEffect, useState } from 'react' + +import { mdiChevronDown } from '@mdi/js' +import VisuallyHidden from '@reach/visually-hidden' +import classNames from 'classnames' +import copy from 'copy-to-clipboard' +import { RouteComponentProps } from 'react-router' +import { of } from 'rxjs' +import { delay, map } from 'rxjs/operators' + +import { ErrorAlert } from '@sourcegraph/branded/src/components/alerts' +import { useQuery } from '@sourcegraph/http-client/src' +import { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService' +import { + Button, + Code, + Container, + Icon, + Link, + LoadingSpinner, + PageHeader, + Popover, + PopoverContent, + PopoverTrigger, + Position, + Text, + Tooltip, +} from '@sourcegraph/wildcard' + +import { + FilteredConnection, + FilteredConnectionFilter, + FilteredConnectionQueryArguments, +} from '../components/FilteredConnection' +import { PageTitle } from '../components/PageTitle' +import { Timestamp } from '../components/time/Timestamp' +import { OutboundRequestsResult, OutboundRequestsVariables } from '../graphql-operations' + +import { OUTBOUND_REQUESTS, OUTBOUND_REQUESTS_PAGE_POLL_INTERVAL } from './backend' + +import styles from './SiteAdminOutboundRequestsPage.module.scss' + +export interface SiteAdminOutboundRequestsPageProps extends RouteComponentProps, TelemetryProps { + now?: () => Date +} + +export type OutboundRequest = OutboundRequestsResult['outboundRequests']['nodes'][0] + +const filters: FilteredConnectionFilter[] = [ + { + id: 'filters', + label: 'Filter', + type: 'select', + values: [ + { + label: 'All', + value: 'all', + tooltip: 'Show all requests', + args: {}, + }, + { + label: 'Failed', + value: 'failed', + tooltip: 'Show only failed requests', + args: { failed: true }, + }, + { + label: 'Successful', + value: 'successful', + tooltip: 'Show only successful requests', + args: { failed: false }, + }, + ], + }, +] + +export const SiteAdminOutboundRequestsPage: React.FunctionComponent< + React.PropsWithChildren +> = ({ history, telemetryService }) => { + const [items, setItems] = useState([]) + + useEffect(() => { + telemetryService.logPageView('SiteAdminOutboundRequests') + }, [telemetryService]) + + const lastId = items[items.length - 1]?.id ?? null + const { data, loading, error, stopPolling, refetch, startPolling } = useQuery< + OutboundRequestsResult, + OutboundRequestsVariables + >(OUTBOUND_REQUESTS, { + variables: { after: lastId }, + pollInterval: OUTBOUND_REQUESTS_PAGE_POLL_INTERVAL, + }) + + useEffect(() => { + if (data?.outboundRequests?.nodes?.length && (!lastId || data?.outboundRequests.nodes[0].id > lastId)) { + const newItems = items + .concat(...data.outboundRequests.nodes) + .slice( + Math.max( + items.length + + data.outboundRequests.nodes.length - + (window.context.outboundRequestLogLimit ?? 0), + 0 + ) + ) + // Workaround for https://github.com/apollographql/apollo-client/issues/3053 to update the variables. + // Weirdly enough, we don't need to wait for refetch() to complete before restarting polling. + // See http://www.petecorey.com/blog/2019/09/23/apollo-quirks-polling-after-refetching-with-new-variables/ + stopPolling() + setItems(newItems) + refetch({ after: newItems[newItems.length - 1]?.id ?? null }) + .then(() => {}) + .catch(() => {}) + startPolling(OUTBOUND_REQUESTS_PAGE_POLL_INTERVAL) + } + }, [data, lastId, items, refetch, startPolling, stopPolling]) + + const queryOutboundRequests = useCallback( + (args: FilteredConnectionQueryArguments & { failed?: boolean }) => + of([...items].reverse()).pipe( + delay(200), // Without this, FilteredConnection will get into an infinite loop. :facepalm: + map(items => { + const filtered = items?.filter( + request => + (!args.query || matchesString(request, args.query)) && + (args.failed !== false || isSuccessful(request)) && + (args.failed !== true || !isSuccessful(request)) + ) + return { + nodes: filtered ?? [], + totalCount: (filtered ?? []).length, + } + }) + ), + [items] + ) + + return ( +
+ + + This is the log of recent external requests sent by the Sourcegraph instance. Handy for seeing + what's happening between Sourcegraph and other services.{' '} + The list updates every five seconds. + + } + className="mb-3" + /> + + + {error && !loading && } + {loading && !error && } + {window.context.outboundRequestLogLimit ? ( + + className="mb-0" + listComponent="div" + listClassName={classNames('list-group mb-3', styles.requestsGrid)} + noun="request" + pluralNoun="requests" + queryConnection={queryOutboundRequests} + nodeComponent={MigrationNode} + filters={filters} + history={history} + location={history.location} + /> + ) : ( + <> + Outbound request logging is currently disabled. + + Set `outboundRequestLogLimit` to a non-zero value in your{' '} + site config to enable it. + + + )} + +
+ ) +} + +const MigrationNode: React.FunctionComponent<{ node: React.PropsWithChildren }> = ({ node }) => { + const roundedSecond = Math.round((node.duration + Number.EPSILON) * 100) / 100 + const [copied, setCopied] = useState(false) + + const copyToClipboard = (text: string): void => { + copy(text) + setCopied(true) + setTimeout(() => setCopied(false), 2000) + } + + return ( + + +
+ +
+
+ + + Request method + + ● + {' '} + {node.method} + + +
+
+ + + Status code + {node.statusCode} + + +
+
{node.url}
+
+ + + + URL: + {node.url} + + + Status: + {node.statusCode} + + + Date/time started: + + + + Duration: + {roundedSecond.toFixed(2)} second{roundedSecond === 1 ? '' : 's'} + + + Client created at: + {node.creationStackFrame} + + + Request made at: + {node.callStackFrame} + + + Error: + {node.errorMessage ? node.errorMessage : 'No error'} + + {node.requestHeaders.length ? ( + <> + + Request headers:{' '} + +
    + {node.requestHeaders.map(header => ( +
  • + {header.name}: {header.values.join(', ')} +
    • + ))} +
+ + ) : ( + 'No request headers' + )} + {node.responseHeaders.length ? ( + <> + + Response headers:{' '} + +
    + {node.responseHeaders.map(header => ( +
  • + {header.name}: {header.values.join(', ')} +
    • + ))} +
+ + ) : ( + 'No request headers' + )} + + Request body: {node.requestBody ? node.requestBody : 'Empty body'} + + + +
+
+ + + +
+ + ) +} + +const SimplePopover: React.FunctionComponent<{ label: string; children: ReactNode }> = ({ label, children }) => { + const [isOpen, setIsOpen] = useState(false) + const handleOpenChange = useCallback(({ isOpen }: { isOpen: boolean }) => setIsOpen(isOpen), [setIsOpen]) + return ( + + + {label} + + + +
{children}
+ + + ) +} + +function isSuccessful(request: OutboundRequest): boolean { + return request.statusCode < 400 +} + +function matchesString(request: OutboundRequest, query: string): boolean { + const lQuery = query.toLowerCase() + return ( + request.url.toLowerCase().includes(lQuery) || + request.method.toLowerCase().includes(lQuery) || + request.requestBody.toLowerCase().includes(lQuery) || + request.statusCode.toString().includes(lQuery) || + request.errorMessage.toLowerCase().includes(lQuery) || + request.creationStackFrame.toLowerCase().includes(lQuery) || + request.callStackFrame.toLowerCase().includes(lQuery) || + request.requestHeaders?.some( + header => + header.name.toLowerCase().includes(lQuery) || + header.values.some(value => value.toLowerCase().includes(lQuery)) + ) || + request.responseHeaders?.some( + header => + header.name.toLowerCase().includes(lQuery) || + header.values.some(value => value.toLowerCase().includes(lQuery)) + ) + ) +} + +function buildCurlCommand(request: OutboundRequest): string { + const headers = request.requestHeaders?.map(header => `-H '${header.name}: ${header.values.join(', ')}'`).join(' ') + const body = request.requestBody ? `-d '${request.requestBody}'` : '' + return `curl -X ${request.method} ${headers} ${body} '${request.url}'` +} diff --git a/client/web/src/site-admin/backend.ts b/client/web/src/site-admin/backend.ts index 40fccfaf9bc..f58474c7f98 100644 --- a/client/web/src/site-admin/backend.ts +++ b/client/web/src/site-admin/backend.ts @@ -284,6 +284,35 @@ export function fetchAllRepositoriesAndPollIfEmptyOrAnyCloning( ) } +export const OUTBOUND_REQUESTS = gql` + query OutboundRequests($after: ID) { + outboundRequests(after: $after) { + nodes { + id + startedAt + method + url + requestHeaders { + name + values + } + requestBody + statusCode + responseHeaders { + name + values + } + duration + errorMessage + creationStackFrame + callStackFrame + } + } + } +` + +export const OUTBOUND_REQUESTS_PAGE_POLL_INTERVAL = 5000 + export const UPDATE_MIRROR_REPOSITORY = gql` mutation UpdateMirrorRepository($repository: ID!) { updateMirrorRepository(repository: $repository) { diff --git a/client/web/src/site-admin/routes.tsx b/client/web/src/site-admin/routes.tsx index 2cfd5b8719b..362b550ba98 100644 --- a/client/web/src/site-admin/routes.tsx +++ b/client/web/src/site-admin/routes.tsx @@ -81,6 +81,11 @@ export const siteAdminAreaRoutes: readonly SiteAdminAreaRoute[] = [ exact: true, render: lazyComponent(() => import('./SiteAdminMigrationsPage'), 'SiteAdminMigrationsPage'), }, + { + path: '/outbound-requests', + exact: true, + render: lazyComponent(() => import('./SiteAdminOutboundRequestsPage'), 'SiteAdminOutboundRequestsPage'), + }, { path: '/feature-flags', exact: true, diff --git a/client/web/src/site-admin/sidebaritems.ts b/client/web/src/site-admin/sidebaritems.ts index a884d7d811a..e04faa24fb7 100644 --- a/client/web/src/site-admin/sidebaritems.ts +++ b/client/web/src/site-admin/sidebaritems.ts @@ -133,6 +133,11 @@ export const maintenanceGroup: SiteAdminSideBarGroup = { to: '/-/debug/jaeger', source: 'server', }, + { + label: 'Outbound requests', + to: '/site-admin/outbound-requests', + source: 'server', + }, ], } diff --git a/cmd/frontend/graphqlbackend/graphqlbackend.go b/cmd/frontend/graphqlbackend/graphqlbackend.go index 8c1c3209090..946da583bfa 100644 --- a/cmd/frontend/graphqlbackend/graphqlbackend.go +++ b/cmd/frontend/graphqlbackend/graphqlbackend.go @@ -607,6 +607,9 @@ func newSchemaResolver(db database.DB, gitserverClient gitserver.Client) *schema "WebhookLog": func(ctx context.Context, id graphql.ID) (Node, error) { return webhookLogByID(ctx, db, id) }, + "OutboundRequest": func(ctx context.Context, id graphql.ID) (Node, error) { + return r.outboundRequestByID(ctx, id) + }, "Executor": func(ctx context.Context, id graphql.ID) (Node, error) { return executorByID(ctx, db, id) }, diff --git a/cmd/frontend/graphqlbackend/outbound_requests.go b/cmd/frontend/graphqlbackend/outbound_requests.go new file mode 100644 index 00000000000..e4ed6ecb075 --- /dev/null +++ b/cmd/frontend/graphqlbackend/outbound_requests.go @@ -0,0 +1,145 @@ +package graphqlbackend + +import ( + "context" + + "github.com/graph-gophers/graphql-go" + "github.com/graph-gophers/graphql-go/relay" + "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil" + "github.com/sourcegraph/sourcegraph/internal/auth" + "github.com/sourcegraph/sourcegraph/internal/gqlutil" + "github.com/sourcegraph/sourcegraph/internal/httpcli" + "github.com/sourcegraph/sourcegraph/internal/types" +) + +type outboundRequestsArgs struct { + After *graphql.ID +} + +type outboundRequestResolver struct { + req *types.OutboundRequestLogItem +} + +type HttpHeaders struct { + name string + values []string +} + +// accessTokenConnectionResolver resolves a list of access tokens. +// +// 🚨 SECURITY: When instantiating an outboundRequestConnectionResolver value, the caller MUST check +// permissions. +type outboundRequestConnectionResolver struct { + after string +} + +func (r *schemaResolver) OutboundRequests(ctx context.Context, args *outboundRequestsArgs) (*outboundRequestConnectionResolver, error) { + // 🚨 SECURITY: Only site admins may list outbound requests. + if err := auth.CheckCurrentUserIsSiteAdmin(ctx, r.db); err != nil { + return nil, err + } + + var after string + if args.After != nil { + err := relay.UnmarshalSpec(*args.After, &after) + if err != nil { + return nil, err + } + } else { + after = "" + } + + return &outboundRequestConnectionResolver{ + after: after, + }, nil +} + +func (r *schemaResolver) outboundRequestByID(ctx context.Context, id graphql.ID) (*outboundRequestResolver, error) { + // 🚨 SECURITY: Only site admins may view outbound requests. + if err := auth.CheckCurrentUserIsSiteAdmin(ctx, r.db); err != nil { + return nil, err + } + + var key string + err := relay.UnmarshalSpec(id, key) + if err != nil { + return nil, err + } + item, _ := httpcli.GetOutboundRequestLogItem(key) + return &outboundRequestResolver{req: item}, nil +} + +func (r *outboundRequestConnectionResolver) Nodes(ctx context.Context) ([]*outboundRequestResolver, error) { + requests, err := httpcli.GetAllOutboundRequestLogItemsAfter(ctx, r.after) + if err != nil { + return nil, err + } + + resolvers := make([]*outboundRequestResolver, 0, len(requests)) + for _, item := range requests { + resolvers = append(resolvers, &outboundRequestResolver{req: item}) + } + + return resolvers, nil +} + +func (r *outboundRequestConnectionResolver) TotalCount(ctx context.Context) (int32, error) { + requests, err := httpcli.GetAllOutboundRequestLogItemsAfter(ctx, r.after) + if err != nil { + return 0, err + } + return int32(len(requests)), nil +} + +func (r *outboundRequestConnectionResolver) PageInfo() (*graphqlutil.PageInfo, error) { + return graphqlutil.HasNextPage(false), nil +} + +func (r *outboundRequestResolver) ID() graphql.ID { + return relay.MarshalID("OutboundRequest", r.req.ID) +} + +func (r *outboundRequestResolver) StartedAt() gqlutil.DateTime { + return gqlutil.DateTime{Time: r.req.StartedAt} +} + +func (r *outboundRequestResolver) Method() string { return r.req.Method } + +func (r *outboundRequestResolver) URL() string { return r.req.URL } + +func (r *outboundRequestResolver) RequestHeaders() ([]*HttpHeaders, error) { + return newHttpHeaders(r.req.RequestHeaders) +} + +func (r *outboundRequestResolver) RequestBody() string { return r.req.RequestBody } + +func (r *outboundRequestResolver) StatusCode() int32 { return r.req.StatusCode } + +func (r *outboundRequestResolver) ResponseHeaders() ([]*HttpHeaders, error) { + return newHttpHeaders(r.req.ResponseHeaders) +} + +func (r *outboundRequestResolver) Duration() float64 { return r.req.Duration } + +func (r *outboundRequestResolver) ErrorMessage() string { return r.req.ErrorMessage } + +func (r *outboundRequestResolver) CreationStackFrame() string { return r.req.CreationStackFrame } + +func (r *outboundRequestResolver) CallStackFrame() string { return r.req.CallStackFrame } + +func newHttpHeaders(headers map[string][]string) ([]*HttpHeaders, error) { + result := make([]*HttpHeaders, 0, len(headers)) + for key, values := range headers { + result = append(result, &HttpHeaders{name: key, values: values}) + } + + return result, nil +} + +func (h HttpHeaders) Name() string { + return h.name +} + +func (h HttpHeaders) Values() []string { + return h.values +} diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql index fc9008ba041..f1ad86199ab 100755 --- a/cmd/frontend/graphqlbackend/schema.graphql +++ b/cmd/frontend/graphqlbackend/schema.graphql @@ -238,7 +238,7 @@ type Mutation { createPassword(newPassword: String!): EmptyResponse """ Sets the user to accept the site's Terms of Service and Privacy Policy. - If the ID is ommitted, the current user is assumed. + If the ID is omitted, the current user is assumed. Only the user or site admins may perform this mutation. """ @@ -1575,6 +1575,16 @@ type Query { webhookID: ID ): WebhookLogConnection! + """ + Get a log of the latest outbound external requests. Only available to site admins. + """ + outboundRequests( + """ + An opaque cursor that is used for pagination. + """ + after: ID + ): OutboundRequestConnection! + """ (experimental) Get invitation based on the JWT in the invitation URL @@ -5069,7 +5079,7 @@ type GitBlob implements TreeEntry & File2 { query: String ): SymbolConnection! """ - (Experimental) Symbol defined in this blob at the specfic line number and character offset. + (Experimental) Symbol defined in this blob at the specific line number and character offset. """ symbol( """ @@ -8001,7 +8011,7 @@ interface WebhookLogMessage { """ The headers in the HTTP message. """ - headers: [WebhookLogHeader!]! + headers: [HTTPHeader!]! """ The body content of the HTTP message. @@ -8016,7 +8026,7 @@ type WebhookLogRequest implements WebhookLogMessage { """ The headers in the HTTP message. """ - headers: [WebhookLogHeader!]! + headers: [HTTPHeader!]! """ The body content of the HTTP message. @@ -8046,7 +8056,7 @@ type WebhookLogResponse implements WebhookLogMessage { """ The headers in the HTTP message. """ - headers: [WebhookLogHeader!]! + headers: [HTTPHeader!]! """ The body content of the HTTP message. @@ -8055,16 +8065,100 @@ type WebhookLogResponse implements WebhookLogMessage { } """ -A single HTTP header within a webhook log. +A list of outbound requests. """ -type WebhookLogHeader { +type OutboundRequestConnection { """ - The header name. + A list of webhook logs. + """ + nodes: [OutboundRequest!]! + + """ + The total number of webhook logs in the connection. + """ + totalCount: Int! + + """ + Pagination information. + """ + pageInfo: PageInfo! +} +""" +A single logged webhook delivery. +""" +type OutboundRequest { + """ + The request log item ID. Looks like "2021-01-01T00_00_00.000000" – Same as the Redis keys. + """ + id: ID! + + """ + The time the request was sent at. + """ + startedAt: DateTime! + + """ + The method used in the HTTP request. E.g. GET, POST, etc. + """ + method: String! + + """ + The full URL the request was sent to. + """ + url: String! + + """ + The headers sent with the HTTP request. + """ + requestHeaders: [HTTPHeader!]! + + """ + The body content of the HTTP message. + """ + requestBody: String! + + """ + The HTTP status code received. + """ + statusCode: Int! + + """ + The headers received with the HTTP response. + """ + responseHeaders: [HTTPHeader!]! + + """ + The total time the request took to complete. + """ + duration: Float! + + """ + Any error message got from the request Doer in case of an error, otherwise an empty string. + """ + errorMessage: String! + + """ + Stack information to figure out where the ExternalClientFactory was created. + """ + creationStackFrame: String! + + """ + Stack information to figure out where in the code base the request was initiated. + """ + callStackFrame: String! +} + +""" +A key-value pair +""" +type HTTPHeader { + """ + The header name """ name: String! """ - The header values. + Can be multiple values """ values: [String!]! } diff --git a/cmd/frontend/graphqlbackend/settings_cascade.go b/cmd/frontend/graphqlbackend/settings_cascade.go index 6b975ecea0c..7ed84a5ae7e 100644 --- a/cmd/frontend/graphqlbackend/settings_cascade.go +++ b/cmd/frontend/graphqlbackend/settings_cascade.go @@ -215,7 +215,7 @@ func mergeLeft(left, right reflect.Value, depth int) reflect.Value { return right } -func (r schemaResolver) ViewerSettings(ctx context.Context) (*settingsCascade, error) { +func (r *schemaResolver) ViewerSettings(ctx context.Context) (*settingsCascade, error) { user, err := CurrentUser(ctx, r.db) if err != nil { return nil, err diff --git a/cmd/frontend/graphqlbackend/webhook_logs.go b/cmd/frontend/graphqlbackend/webhook_logs.go index 55c90e725af..36a802e5165 100644 --- a/cmd/frontend/graphqlbackend/webhook_logs.go +++ b/cmd/frontend/graphqlbackend/webhook_logs.go @@ -271,16 +271,8 @@ type webhookLogMessageResolver struct { message *types.WebhookLogMessage } -func (r *webhookLogMessageResolver) Headers() []*webhookLogHeaderResolver { - headers := make([]*webhookLogHeaderResolver, 0, len(r.message.Header)) - for k, v := range r.message.Header { - headers = append(headers, &webhookLogHeaderResolver{ - name: k, - values: v, - }) - } - - return headers +func (r *webhookLogMessageResolver) Headers() ([]*HttpHeaders, error) { + return newHttpHeaders(r.message.Header) } func (r *webhookLogMessageResolver) Body() string { @@ -303,19 +295,6 @@ func (r *webhookLogRequestResolver) Version() string { return r.message.Version } -type webhookLogHeaderResolver struct { - name string - values []string -} - -func (r *webhookLogHeaderResolver) Name() string { - return r.name -} - -func (r *webhookLogHeaderResolver) Values() []string { - return r.values -} - func marshalWebhookID(id int32) graphql.ID { return relay.MarshalID("Webhook", id) } diff --git a/cmd/frontend/internal/app/jscontext/jscontext.go b/cmd/frontend/internal/app/jscontext/jscontext.go index 4b3f77d1661..82b55b15b34 100644 --- a/cmd/frontend/internal/app/jscontext/jscontext.go +++ b/cmd/frontend/internal/app/jscontext/jscontext.go @@ -116,6 +116,8 @@ type JSContext struct { EnableLegacyExtensions bool `json:"enableLegacyExtensions"` LicenseInfo *hooks.LicenseInfo `json:"licenseInfo"` + + OutboundRequestLogLimit int `json:"outboundRequestLogLimit"` } // NewJSContextFromRequest populates a JSContext struct from the HTTP @@ -250,6 +252,8 @@ func NewJSContextFromRequest(req *http.Request, db database.DB) JSContext { EnableLegacyExtensions: conf.ExperimentalFeatures().EnableLegacyExtensions, LicenseInfo: licenseInfo, + + OutboundRequestLogLimit: conf.Get().OutboundRequestLogLimit, } } diff --git a/internal/conf/init.go b/internal/conf/init.go index 2de856360dc..1c3b6b95a27 100644 --- a/internal/conf/init.go +++ b/internal/conf/init.go @@ -22,10 +22,16 @@ func Init() { // package dependency cycles, since conf itself uses httpcli's internal // client. This is gross, and the whole conf package is gross. go Watch(func() { - before := httpcli.TLSExternalConfig() - after := Get().ExperimentalFeatures.TlsExternal - if !reflect.DeepEqual(before, after) { - httpcli.SetTLSExternalConfig(after) + tlsBefore := httpcli.TLSExternalConfig() + tlsAfter := Get().ExperimentalFeatures.TlsExternal + if !reflect.DeepEqual(tlsBefore, tlsAfter) { + httpcli.SetTLSExternalConfig(tlsAfter) + } + + outboundRequestLogLimitBefore := httpcli.OutboundRequestLogLimit() + outboundRequestLogLimitAfter := Get().OutboundRequestLogLimit + if outboundRequestLogLimitBefore != outboundRequestLogLimitAfter { + httpcli.SetOutboundRequestLogLimit(outboundRequestLogLimitAfter) } }) } diff --git a/internal/httpcli/client.go b/internal/httpcli/client.go index 604f3180704..6b50ec16dc2 100644 --- a/internal/httpcli/client.go +++ b/internal/httpcli/client.go @@ -78,7 +78,7 @@ type Factory struct { common []Opt } -// redisCache is a HTTP cache backed by Redis. The TTL of a week is a balance +// redisCache is an HTTP cache backed by Redis. The TTL of a week is a balance // between caching values for a useful amount of time versus growing the cache // too large. var redisCache = rcache.NewWithTTL("http", 604800) @@ -109,6 +109,7 @@ func NewExternalClientFactory(middleware ...Middleware) *Factory { mw := []Middleware{ ContextErrorMiddleware, HeadersMiddleware("User-Agent", "Sourcegraph-Bot"), + redisLoggerMiddleware(), } mw = append(mw, middleware...) @@ -243,8 +244,8 @@ func HeadersMiddleware(headers ...string) Middleware { } // ContextErrorMiddleware wraps a Doer with context.Context error -// handling. It checks if the request context is done, and if so, -// returns its error. Otherwise it returns the error from the inner +// handling. It checks if the request context is done, and if so, +// returns its error. Otherwise, it returns the error from the inner // Doer call. func ContextErrorMiddleware(cli Doer) Doer { return DoerFunc(func(req *http.Request) (*http.Response, error) { @@ -290,6 +291,10 @@ const ( // requestRetryAttemptKey is the key to the rehttp.Attempt attached to a request, if // a request undergoes retries via NewRetryPolicy requestRetryAttemptKey requestContextKey = iota + + // redisLoggingMiddlewareErrorKey is the key to any errors that occurred when logging + // a request to Redis via redisLoggerMiddleware + redisLoggingMiddlewareErrorKey ) // NewLoggingMiddleware logs basic diagnostics about requests made through this client at @@ -329,6 +334,10 @@ func NewLoggingMiddleware(logger log.Logger) Middleware { log.Int("attempts", attempt.Index), log.Error(attempt.Error))) } + if redisErr, ok := ctx.Value(redisLoggingMiddlewareErrorKey).(error); ok { + // Get fields from redisLoggerMiddleware + fields = append(fields, log.NamedError("redisLoggerErr", redisErr)) + } // Log results with link to trace if present trace.Logger(ctx, logger). @@ -356,7 +365,7 @@ func ExternalTransportOpt(cli *http.Client) error { return nil } -// NewCertPoolOpt returns a Opt that sets the RootCAs pool of an http.Client's +// NewCertPoolOpt returns an Opt that sets the RootCAs pool of an http.Client's // transport. func NewCertPoolOpt(certs ...string) Opt { return func(cli *http.Client) error { @@ -719,3 +728,34 @@ func RequestClientTransportOpt(cli *http.Client) error { return nil } + +// IsRiskyHeader returns true if the request or response header is likely to contain private data. +func IsRiskyHeader(name string, values []string) bool { + return isRiskyHeaderName(name) || containsRiskyHeaderValue(values) +} + +// isRiskyHeaderName returns true if the request or response header is likely to contain private data +// based on its name. +func isRiskyHeaderName(name string) bool { + riskyHeaderKeys := []string{"auth", "cookie", "token"} + for _, riskyKey := range riskyHeaderKeys { + if strings.Contains(strings.ToLower(name), riskyKey) { + return true + } + } + return false +} + +// ContainsRiskyHeaderValue returns true if the values array of a request or response header +// looks like it's likely to contain private data. +func containsRiskyHeaderValue(values []string) bool { + riskyHeaderValues := []string{"bearer", "ghp_", "glpat-"} + for _, value := range values { + for _, riskyValue := range riskyHeaderValues { + if strings.Contains(strings.ToLower(value), riskyValue) { + return true + } + } + } + return false +} diff --git a/internal/httpcli/client_test.go b/internal/httpcli/client_test.go index e4844eb4f79..cdb17c0f467 100644 --- a/internal/httpcli/client_test.go +++ b/internal/httpcli/client_test.go @@ -530,6 +530,68 @@ func TestLoggingMiddleware(t *testing.T) { } assert.NotZero(t, attemptsLogged) }) + + t.Run("log redisLoggerMiddleware error", func(t *testing.T) { + const wantErrMessage = "redisLoggingError" + redisErrorMiddleware := func(next Doer) Doer { + return DoerFunc(func(req *http.Request) (*http.Response, error) { + // simplified version of what we do in redisLoggerMiddleware, since + // we just test that adding and reading the context key/value works + var middlewareErrors error + defer func() { + if middlewareErrors != nil { + *req = *req.WithContext(context.WithValue(req.Context(), + redisLoggingMiddlewareErrorKey, middlewareErrors)) + } + }() + + middlewareErrors = errors.New(wantErrMessage) + + return next.Do(req) + }) + } + + logger, exportLogs := logtest.Captured(t) + + cli, _ := NewFactory( + NewMiddleware( + ContextErrorMiddleware, + redisErrorMiddleware, + NewLoggingMiddleware(logger), + ), + ).Doer() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + t.Cleanup(srv.Close) + + req, _ := http.NewRequest("GET", srv.URL, nil) + _, err := cli.Do(req) + if err != nil { + t.Fatal(err) + } + + // Check log entries for logged fields about retries + logEntries := exportLogs() + assert.Greater(t, len(logEntries), 0) + var found bool + for _, entry := range logEntries { + // Check for appropriate scope + if !strings.Contains(entry.Scope, "httpcli") { + continue + } + + // Check for redisLoggerErr + errField, ok := entry.Fields["redisLoggerErr"] + if !ok { + continue + } + if assert.Contains(t, errField, wantErrMessage) { + found = true + break + } + } + assert.True(t, found) + }) } type notFoundTransport struct{} diff --git a/internal/httpcli/external.go b/internal/httpcli/external.go index d247abc0ba6..817f90ca7ed 100644 --- a/internal/httpcli/external.go +++ b/internal/httpcli/external.go @@ -7,6 +7,7 @@ import ( "net/http" "reflect" "sync" + "sync/atomic" "go.opentelemetry.io/otel/attribute" @@ -26,6 +27,8 @@ var tlsExternalConfig struct { *schema.TlsExternal } +var outboundRequestLogLimit atomic.Int32 + // SetTLSExternalConfig is called by the conf package whenever TLSExternalConfig changes. // This is needed to avoid circular imports. func SetTLSExternalConfig(c *schema.TlsExternal) { @@ -41,6 +44,17 @@ func TLSExternalConfig() *schema.TlsExternal { return tlsExternalConfig.TlsExternal } +// SetOutboundRequestLogLimit is called by the conf package whenever OutboundRequestLogLimit changes. +// This is needed to avoid circular imports. +func SetOutboundRequestLogLimit(i int) { + outboundRequestLogLimit.Store(int32(i)) +} + +// OutboundRequestLogLimit returns the current value of the global OutboundRequestLogLimit value. +func OutboundRequestLogLimit() int { + return int(outboundRequestLogLimit.Load()) +} + func (t *externalTransport) RoundTrip(r *http.Request) (*http.Response, error) { t.mu.RLock() config, effective := t.config, t.effective diff --git a/internal/httpcli/redis_logger_middleware.go b/internal/httpcli/redis_logger_middleware.go new file mode 100644 index 00000000000..815d6985088 --- /dev/null +++ b/internal/httpcli/redis_logger_middleware.go @@ -0,0 +1,216 @@ +package httpcli + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "path/filepath" + "runtime" + "sort" + "strings" + "time" + + "github.com/sourcegraph/sourcegraph/internal/rcache" + "github.com/sourcegraph/sourcegraph/internal/types" + "github.com/sourcegraph/sourcegraph/lib/errors" +) + +var outboundRequestsRedisCache = rcache.NewWithTTL("outbound-requests", 604800) + +func redisLoggerMiddleware() Middleware { + creatorStackFrame, _ := getFrames(4).Next() + return func(cli Doer) Doer { + return DoerFunc(func(req *http.Request) (*http.Response, error) { + start := time.Now() + resp, err := cli.Do(req) + duration := time.Since(start) + limit := OutboundRequestLogLimit() + + // Feature is turned off, do not log + if limit == 0 { + return resp, err + } + + // middlewareErrors will be set later if there is an error + var middlewareErrors error + defer func() { + if middlewareErrors != nil { + *req = *req.WithContext(context.WithValue(req.Context(), + redisLoggingMiddlewareErrorKey, middlewareErrors)) + } + }() + + // Read body + var requestBody []byte + if req != nil { + body, _ := req.GetBody() + if body != nil { + var readErr error + requestBody, readErr = io.ReadAll(body) + if err != nil { + middlewareErrors = errors.Append(middlewareErrors, + errors.Wrap(readErr, "read body")) + } + } + } + + // Create log item + var errorMessage string + if err != nil { + errorMessage = err.Error() + } + key := time.Now().UTC().Format("2006-01-02T15_04_05.999999999") + callerStackFrame, _ := getFrames(4).Next() // Caller of the caller of redisLoggerMiddleware + logItem := types.OutboundRequestLogItem{ + ID: key, + StartedAt: start, + Method: req.Method, + URL: req.URL.String(), + RequestHeaders: removeSensitiveHeaders(req.Header), + RequestBody: string(requestBody), + StatusCode: int32(resp.StatusCode), + ResponseHeaders: removeSensitiveHeaders(resp.Header), + Duration: duration.Seconds(), + ErrorMessage: errorMessage, + CreationStackFrame: formatStackFrame(creatorStackFrame), + CallStackFrame: formatStackFrame(callerStackFrame), + } + + // Serialize log item + logItemJson, jsonErr := json.Marshal(logItem) + if jsonErr != nil { + middlewareErrors = errors.Append(middlewareErrors, + errors.Wrap(jsonErr, "marshal log item")) + } + + // Save new item + outboundRequestsRedisCache.Set(key, logItemJson) + + // Delete excess items + if deleteErr := deleteExcessItems(outboundRequestsRedisCache, limit); deleteErr != nil { + middlewareErrors = errors.Append(middlewareErrors, + errors.Wrap(deleteErr, "delete excess items")) + } + + return resp, err + }) + } +} + +func deleteExcessItems(c *rcache.Cache, limit int) error { + keys, err := c.ListKeys(context.Background()) + if err != nil { + return errors.Wrap(err, "list keys") + } + + // Delete all but the last N keys + if len(keys) > limit { + sort.Strings(keys) + c.DeleteMulti(keys[:len(keys)-limit]...) + } + + return nil +} + +// GetAllOutboundRequestLogItemsAfter returns all outbound request log items after the given key, +// in ascending order, trimmed to maximum {limit} items. Example for `after`: "2021-01-01T00_00_00.000000". +func GetAllOutboundRequestLogItemsAfter(ctx context.Context, after string) ([]*types.OutboundRequestLogItem, error) { + var limit = OutboundRequestLogLimit() + + if limit == 0 { + return []*types.OutboundRequestLogItem{}, nil + } + + rawItems, err := getAllValuesAfter(ctx, outboundRequestsRedisCache, after, limit) + if err != nil { + return nil, err + } + + items := make([]*types.OutboundRequestLogItem, 0, len(rawItems)) + for _, rawItem := range rawItems { + var item types.OutboundRequestLogItem + err = json.Unmarshal(rawItem, &item) + if err != nil { + return nil, err + } + items = append(items, &item) + } + return items, nil +} + +func GetOutboundRequestLogItem(key string) (*types.OutboundRequestLogItem, error) { + rawItem, ok := outboundRequestsRedisCache.Get(key) + if !ok { + return nil, errors.New("item not found") + } + + var item types.OutboundRequestLogItem + err := json.Unmarshal(rawItem, &item) + if err != nil { + return nil, err + } + return &item, nil +} + +// getAllValuesAfter returns all items after the given key, in ascending order, trimmed to maximum {limit} items. +func getAllValuesAfter(ctx context.Context, c *rcache.Cache, after string, limit int) ([][]byte, error) { + all, err := c.ListKeys(ctx) + if err != nil { + return nil, err + } + + var keys []string + if after != "" { + for _, key := range all { + if key > after { + keys = append(keys, key) + } + } + } else { + keys = all + } + + // Sort ascending + sort.Strings(keys) + + // Limit to N + if len(keys) > limit { + keys = keys[len(keys)-limit:] + } + + return c.GetMulti(keys...), nil +} + +func removeSensitiveHeaders(headers http.Header) http.Header { + var cleanHeaders = make(http.Header) + for name, values := range headers { + if IsRiskyHeader(name, values) { + cleanHeaders[name] = []string{"REDACTED"} + } else { + cleanHeaders[name] = values + } + } + return cleanHeaders +} + +func formatStackFrame(frame runtime.Frame) string { + packageTreeAndFunctionName := strings.Join(strings.Split(frame.Function, "/")[3:], "/") + dotPieces := strings.Split(packageTreeAndFunctionName, ".") + packageTree := dotPieces[0] + functionName := dotPieces[len(dotPieces)-1] + + // Reconstruct the frame file path so that we don't include the local path on the machine that built this instance + fileName := filepath.Join(packageTree, filepath.Base(frame.File)) + + return fmt.Sprintf("%s:%d (Function: %s)", fileName, frame.Line, functionName) +} + +const pcLen = 1024 + +func getFrames(skip int) *runtime.Frames { + pc := make([]uintptr, pcLen) + n := runtime.Callers(skip, pc) + return runtime.CallersFrames(pc[:n]) +} diff --git a/internal/httpcli/redis_logger_middleware_test.go b/internal/httpcli/redis_logger_middleware_test.go new file mode 100644 index 00000000000..55c8d5a2550 --- /dev/null +++ b/internal/httpcli/redis_logger_middleware_test.go @@ -0,0 +1,95 @@ +package httpcli + +import ( + "context" + "net/http" + "strconv" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/sourcegraph/internal/rcache" + "github.com/stretchr/testify/assert" + "k8s.io/utils/strings/slices" +) + +func TestRedisLoggerMiddleware_getAllValuesAfter(t *testing.T) { + rcache.SetupForTest(t) + c := rcache.NewWithTTL("some_prefix", 1) + ctx := context.Background() + + var pairs = make([][2]string, 10) + for i := 0; i < 10; i++ { + pairs[i] = [2]string{"key" + strconv.Itoa(i), "value" + strconv.Itoa(i)} + } + c.SetMulti(pairs...) + + key := "key5" + got, err := getAllValuesAfter(ctx, c, key, 10) + + assert.Nil(t, err) + assert.Len(t, got, 4) + + got, err = getAllValuesAfter(ctx, c, key, 2) + assert.Nil(t, err) + assert.Len(t, got, 2) +} + +func TestRedisLoggerMiddleware_removeSensitiveHeaders(t *testing.T) { + input := http.Header{ + "Authorization": []string{"all values", "should be", "removed"}, + "Bearer": []string{"this should be kept as the risky value is only in the name"}, + "GHP_XXXX": []string{"this should be kept"}, + "GLPAT-XXXX": []string{"this should also be kept"}, + "GitHub-PAT": []string{"this should be removed: ghp_XXXX"}, + "GitLab-PAT": []string{"this should be removed", "glpat-XXXX"}, + "Innocent-Header": []string{"this should be removed as it includes", "the word bearer"}, + "Set-Cookie": []string{"this is verboten"}, + "Token": []string{"a token should be removed"}, + "X-Powered-By": []string{"PHP"}, + "X-Token": []string{"something that smells like a token should also be removed"}, + } + + // Build the expected output. + want := make(http.Header) + riskyKeys := []string{"Bearer", "GHP_XXXX", "GLPAT-XXXX", "X-Powered-By"} + for key, value := range input { + if slices.Contains(riskyKeys, key) { + want[key] = value + } else { + want[key] = []string{"REDACTED"} + } + } + + cleanHeaders := removeSensitiveHeaders(input) + + if diff := cmp.Diff(cleanHeaders, want); diff != "" { + t.Errorf("unexpected request headers (-have +want):\n%s", diff) + } +} + +func TestCache_DeleteFirstN(t *testing.T) { + rcache.SetupForTest(t) + c := rcache.NewWithTTL("some_prefix", 1) + + // Add 10 key-value pairs + var pairs = make([][2]string, 10) + for i := 0; i < 10; i++ { + pairs[i] = [2]string{"key" + strconv.Itoa(i), "value" + strconv.Itoa(i)} + } + c.SetMulti(pairs...) + + // Delete the first 4 key-value pairs + deleteExcessItems(c, 4) + + got, listErr := c.ListKeys(context.Background()) + + assert.Nil(t, listErr) + + assert.Len(t, got, 4) + + assert.NotContains(t, got, "key0") // 0 through 5 should be deleted + assert.NotContains(t, got, "key5") + + assert.Contains(t, got, "key6") // 6 through 9 (4 items) should be kept + assert.Contains(t, got, "key9") +} diff --git a/internal/httptestutil/recorder.go b/internal/httptestutil/recorder.go index e7c91693e82..1eef7b00688 100644 --- a/internal/httptestutil/recorder.go +++ b/internal/httptestutil/recorder.go @@ -110,38 +110,10 @@ func NewRecorderFactory(t testing.TB, update bool, name string) (*httpcli.Factor // riskyHeaderFilter deletes anything that looks risky in request and response // headers. func riskyHeaderFilter(i *cassette.Interaction) error { - riskyHeaderKeys := []string{ - "auth", "cookie", "token", - } - riskyHeaderValues := []string{ - "bearer", "ghp_", "glpat-", - } - - isRiskyKey := func(key string) bool { - lowerKey := strings.ToLower(key) - for _, riskyKey := range riskyHeaderKeys { - if strings.Contains(lowerKey, riskyKey) { - return true - } - } - return false - } - hasRiskyValue := func(values []string) bool { - for _, value := range values { - lowerValue := strings.ToLower(value) - for _, riskyValue := range riskyHeaderValues { - if strings.Contains(lowerValue, riskyValue) { - return true - } - } - } - return false - } - for _, headers := range []http.Header{i.Request.Headers, i.Response.Headers} { - for k, values := range headers { - if isRiskyKey(k) || hasRiskyValue(values) { - delete(headers, k) + for name, values := range headers { + if httpcli.IsRiskyHeader(name, values) { + delete(headers, name) } } } diff --git a/internal/rcache/rcache.go b/internal/rcache/rcache.go index 6452555d805..71979482cb5 100644 --- a/internal/rcache/rcache.go +++ b/internal/rcache/rcache.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "strings" "time" "unicode/utf8" @@ -26,7 +25,7 @@ const dataVersionToDelete = "v1" // DeleteOldCacheData deletes the rcache data in the given Redis instance // that's prefixed with dataVersionToDelete func DeleteOldCacheData(c redis.Conn) error { - return deleteKeysWithPrefix(c, dataVersionToDelete) + return deleteAllKeysWithPrefix(c, dataVersionToDelete) } // Cache implements httpcache.Cache @@ -172,10 +171,19 @@ func (r *Cache) Increase(key string) { } } +// DeleteMulti deletes the given keys. +func (r *Cache) DeleteMulti(keys ...string) { + for _, key := range keys { + r.Delete(key) + } +} + // Delete implements httpcache.Cache.Delete func (r *Cache) Delete(key string) { c := pool.Get() - defer c.Close() + defer func(c redis.Conn) { + _ = c.Close() + }(c) _, err := c.Do("DEL", r.rkeyPrefix()+key) if err != nil { @@ -183,21 +191,25 @@ func (r *Cache) Delete(key string) { } } -// ListKeys lists all keys associated with this cache. Use with care if you have long -// TTLs or no TTL configured. -func (r *Cache) ListKeys(ctx context.Context) ([]string, error) { - c, err := pool.GetContext(ctx) +// ListKeys lists all keys associated with this cache. +// Use with care if you have long TTLs or no TTL configured. +func (r *Cache) ListKeys(ctx context.Context) (results []string, err error) { + var c redis.Conn + c, err = pool.GetContext(ctx) if err != nil { return nil, errors.Wrap(err, "get redis conn") } - defer c.Close() + defer func(c redis.Conn) { + if tempErr := c.Close(); err == nil { + err = tempErr + } + }(c) - var allKeys []string cursor := 0 for { select { case <-ctx.Done(): - return allKeys, ctx.Err() + return results, ctx.Err() default: } @@ -207,22 +219,22 @@ func (r *Cache) ListKeys(ctx context.Context) ([]string, error) { "COUNT", 100), ) if err != nil { - return allKeys, errors.Wrap(err, "redis scan") + return results, errors.Wrap(err, "redis scan") } cursor, _ = redis.Int(res[0], nil) keys, _ := redis.Strings(res[1], nil) for i, k := range keys { - keys[i] = strings.TrimPrefix(k, r.rkeyPrefix()) + keys[i] = k[len(r.rkeyPrefix()):] } - allKeys = append(allKeys, keys...) + results = append(results, keys...) if cursor == 0 { break } } - return allKeys, nil + return } // rkeyPrefix generates the actual key prefix we use on redis. @@ -266,7 +278,7 @@ func SetupForTest(t TB) { } } - err := deleteKeysWithPrefix(c, globalPrefix) + err := deleteAllKeysWithPrefix(c, globalPrefix) if err != nil { log15.Error("Could not clear test prefix", "name", t.Name(), "globalPrefix", globalPrefix, "error", err) } @@ -279,7 +291,7 @@ func SetupForTest(t TB) { // See https://www.lua.org/source/5.1/luaconf.h.html var deleteBatchSize = 5000 -func deleteKeysWithPrefix(c redis.Conn, prefix string) error { +func deleteAllKeysWithPrefix(c redis.Conn, prefix string) error { const script = ` redis.replicate_commands() local cursor = '0' diff --git a/internal/rcache/rcache_test.go b/internal/rcache/rcache_test.go index 55712a0b00f..b3a46818caa 100644 --- a/internal/rcache/rcache_test.go +++ b/internal/rcache/rcache_test.go @@ -140,9 +140,14 @@ func TestCache_multi(t *testing.T) { if got, exp := c.GetMulti("k0", "k1", "k2"), [][]byte{nil, []byte("y"), []byte("z")}; !reflect.DeepEqual(exp, got) { t.Errorf("Expected %v, but got %v", exp, got) } + + c.DeleteMulti("k1", "k2") + if got, exp := c.GetMulti("k0", "k1", "k2"), [][]byte{nil, nil, nil}; !reflect.DeepEqual(exp, got) { + t.Errorf("Expected %v, but got %v", exp, got) + } } -func TestCache_deleteKeysWithPrefix(t *testing.T) { +func TestCache_deleteAllKeysWithPrefix(t *testing.T) { SetupForTest(t) // decrease the deleteBatchSize @@ -168,7 +173,7 @@ func TestCache_deleteKeysWithPrefix(t *testing.T) { conn := pool.Get() defer conn.Close() - err := deleteKeysWithPrefix(conn, c.rkeyPrefix()+"a") + err := deleteAllKeysWithPrefix(conn, c.rkeyPrefix()+"a") if err != nil { t.Error(err) } @@ -187,7 +192,7 @@ func TestCache_deleteKeysWithPrefix(t *testing.T) { func TestCache_Increase(t *testing.T) { SetupForTest(t) - c := NewWithTTL("some_prefix:", 1) + c := NewWithTTL("some_prefix", 1) c.Increase("a") got, ok := c.Get("a") @@ -206,7 +211,7 @@ func TestCache_Increase(t *testing.T) { func TestCache_ListKeys(t *testing.T) { SetupForTest(t) - c := NewWithTTL("some_prefix:", 1) + c := NewWithTTL("some_prefix", 1) c.SetMulti( [2]string{"foobar", "123"}, [2]string{"bazbar", "456"}, diff --git a/internal/types/types.go b/internal/types/types.go index b05f4e1a6ec..e557d5c7fff 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -1709,3 +1709,18 @@ type Webhook struct { CreatedByUserID int32 UpdatedByUserID int32 } + +type OutboundRequestLogItem struct { + ID string `json:"id"` + StartedAt time.Time `json:"startedAt"` + Method string `json:"method"` // The request method (GET, POST, etc.) + URL string `json:"url"` + RequestHeaders map[string][]string `json:"requestHeaders"` + RequestBody string `json:"requestBody"` + StatusCode int32 `json:"statusCode"` // The response status code + ResponseHeaders map[string][]string `json:"responseHeaders"` + Duration float64 `json:"duration"` + ErrorMessage string `json:"errorMessage"` + CreationStackFrame string `json:"creationStackFrame"` + CallStackFrame string `json:"callStackFrame"` +} diff --git a/schema/schema.go b/schema/schema.go index 4755a8cbbd3..d9c5f410357 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -2210,6 +2210,8 @@ type SiteConfiguration struct { ObservabilityTracing *ObservabilityTracing `json:"observability.tracing,omitempty"` // OrganizationInvitations description: Configuration for organization invitations. OrganizationInvitations *OrganizationInvitations `json:"organizationInvitations,omitempty"` + // OutboundRequestLogLimit description: The maximum number of outbound requests to retain. This is a global limit across all outbound requests. If the limit is exceeded, older items will be deleted. If the limit is 0, no outbound requests are logged. + OutboundRequestLogLimit int `json:"outboundRequestLogLimit,omitempty"` // ParentSourcegraph description: URL to fetch unreachable repository details from. Defaults to "https://sourcegraph.com" ParentSourcegraph *ParentSourcegraph `json:"parentSourcegraph,omitempty"` // PermissionsSyncOldestRepos description: Number of repo permissions to schedule for syncing in single scheduler iteration. diff --git a/schema/site.schema.json b/schema/site.schema.json index 7c7f65273ca..30aa664fd2d 100644 --- a/schema/site.schema.json +++ b/schema/site.schema.json @@ -1885,6 +1885,13 @@ } } }, + "outboundRequestLogLimit": { + "description": "The maximum number of outbound requests to retain. This is a global limit across all outbound requests. If the limit is exceeded, older items will be deleted. If the limit is 0, no outbound requests are logged.", + "type": "integer", + "minimum": 0, + "default": 50, + "maximum": 500 + }, "organizationInvitations": { "description": "Configuration for organization invitations.", "type": "object",