From fdbe81783f3d8b97e8e8602d6fd5ae997e7456f6 Mon Sep 17 00:00:00 2001 From: Kamil Kisiela Date: Thu, 29 Aug 2024 12:16:57 +0200 Subject: [PATCH] Add breadcrumbs to CDN Worker (#5552) --- .../cdn-worker/src/artifact-handler.ts | 8 +++ .../cdn-worker/src/artifact-storage-reader.ts | 28 ++++++-- .../services/cdn-worker/src/breadcrumbs.ts | 7 ++ packages/services/cdn-worker/src/dev.ts | 10 ++- packages/services/cdn-worker/src/handler.ts | 12 +++- packages/services/cdn-worker/src/index.ts | 68 +++++++++++++------ .../services/cdn-worker/src/key-validation.ts | 18 ++++- .../services/cdn-worker/tests/cdn.spec.ts | 39 +++++++++++ packages/services/server/src/index.ts | 12 +++- 9 files changed, 173 insertions(+), 29 deletions(-) create mode 100644 packages/services/cdn-worker/src/breadcrumbs.ts diff --git a/packages/services/cdn-worker/src/artifact-handler.ts b/packages/services/cdn-worker/src/artifact-handler.ts index 4250a8be6..09b134823 100644 --- a/packages/services/cdn-worker/src/artifact-handler.ts +++ b/packages/services/cdn-worker/src/artifact-handler.ts @@ -2,6 +2,7 @@ import * as itty from 'itty-router'; import zod from 'zod'; import { createAnalytics, type Analytics } from './analytics'; import { type ArtifactStorageReader, type ArtifactsType } from './artifact-storage-reader'; +import { createBreadcrumb, type Breadcrumb } from './breadcrumbs'; import { InvalidAuthKeyResponse, MissingAuthKeyResponse } from './errors'; import { IsAppDeploymentActive } from './is-app-deployment-active'; import type { KeyValidator } from './key-validation'; @@ -21,6 +22,7 @@ type ArtifactRequestHandler = { isKeyValid: KeyValidator; isAppDeploymentActive: IsAppDeploymentActive; analytics?: Analytics; + breadcrumb?: Breadcrumb; fallback?: ( request: Request, params: { targetId: string; artifactType: string }, @@ -60,6 +62,7 @@ const authHeaderName = 'x-hive-cdn-key' as const; export const createArtifactRequestHandler = (deps: ArtifactRequestHandler) => { const router = itty.Router(); const analytics = deps.analytics ?? createAnalytics(); + const breadcrumb = deps.breadcrumb ?? createBreadcrumb(); const authenticate = async ( request: itty.IRequest & Request, @@ -100,8 +103,13 @@ export const createArtifactRequestHandler = (deps: ArtifactRequestHandler) => { const params = parseResult.data; + breadcrumb( + `Artifact v1 handler (type=${params.artifactType}, targetId=${params.targetId}, contractName=${params.contractName})`, + ); + /** Legacy handling for old client SDK versions. */ if (params.artifactType === 'schema') { + breadcrumb('Redirecting from /schema to /services'); return createResponse( analytics, 'Found.', diff --git a/packages/services/cdn-worker/src/artifact-storage-reader.ts b/packages/services/cdn-worker/src/artifact-storage-reader.ts index c4ba2e3fb..8625fcccf 100644 --- a/packages/services/cdn-worker/src/artifact-storage-reader.ts +++ b/packages/services/cdn-worker/src/artifact-storage-reader.ts @@ -1,6 +1,7 @@ import zod from 'zod'; import type { Analytics } from './analytics'; import { AwsClient } from './aws'; +import type { Breadcrumb } from './breadcrumbs'; export function buildArtifactStorageKey( targetId: string, @@ -56,6 +57,8 @@ export function buildAppDeploymentIsEnabledKey( * Read an artifact/app deployment operation from S3. */ export class ArtifactStorageReader { + private breadcrumb: Breadcrumb; + constructor( private s3: { client: AwsClient; @@ -68,9 +71,12 @@ export class ArtifactStorageReader { bucketName: string; } | null, private analytics: Analytics | null, + breadcrumb: Breadcrumb | null, /** Timeout in milliseconds for S3 read calls. */ private timeout: number = 5_000, - ) {} + ) { + this.breadcrumb = breadcrumb ?? (() => {}); + } /** * Perform a request to S3, with retries and optional mirror. @@ -122,9 +128,11 @@ export class ArtifactStorageReader { }, }) .catch(err => { + this.breadcrumb('Failed to fetch from primary'); if (!this.s3Mirror) { return Promise.reject(err); } + this.breadcrumb('Fetching from primary and mirror now'); // Use two AbortSignals to avoid a situation // where Response.body is consumed, // but the request was aborted after being resolved. @@ -201,11 +209,18 @@ export class ArtifactStorageReader { artifactType = 'sdl'; } + this.breadcrumb( + `Reading artifact (targetId=${targetId}, artifactType=${artifactType}, contractName=${contractName})`, + ); + const key = buildArtifactStorageKey(targetId, artifactType, contractName); + this.breadcrumb(`Reading artifact from S3 key: ${key}`); + const headers: HeadersInit = {}; if (etagValue) { + this.breadcrumb('if-none-match detected'); headers['if-none-match'] = etagValue; } @@ -246,6 +261,8 @@ export class ArtifactStorageReader { } as const; } + this.breadcrumb(`Failed to read artifact`); + const body = await response.text(); throw new Error(`GET request failed with status ${response.status}: ${body}`); } @@ -330,8 +347,10 @@ export class ArtifactStorageReader { } async readLegacyAccessKey(targetId: string) { + const key = ['cdn-legacy-keys', targetId].join('/'); + this.breadcrumb(`Reading from S3 key: ${key}`); const response = await this.request({ - key: ['cdn-legacy-keys', targetId].join('/'), + key, method: 'GET', onAttempt: args => { this.analytics?.track( @@ -353,10 +372,11 @@ export class ArtifactStorageReader { } async readAccessKey(targetId: string, keyId: string) { - const s3KeyParts = ['cdn-keys', targetId, keyId]; + const key = ['cdn-keys', targetId, keyId].join('/'); + this.breadcrumb(`Reading from S3 key: ${key}`); const response = await this.request({ - key: s3KeyParts.join('/'), + key, method: 'GET', onAttempt: args => { this.analytics?.track( diff --git a/packages/services/cdn-worker/src/breadcrumbs.ts b/packages/services/cdn-worker/src/breadcrumbs.ts new file mode 100644 index 000000000..9029471b4 --- /dev/null +++ b/packages/services/cdn-worker/src/breadcrumbs.ts @@ -0,0 +1,7 @@ +export type Breadcrumb = (message: string) => void; + +export function createBreadcrumb() { + return (message: string) => { + console.debug(message); + }; +} diff --git a/packages/services/cdn-worker/src/dev.ts b/packages/services/cdn-worker/src/dev.ts index a969c6aa8..1945df77c 100644 --- a/packages/services/cdn-worker/src/dev.ts +++ b/packages/services/cdn-worker/src/dev.ts @@ -23,7 +23,7 @@ const s3 = { // eslint-disable-next-line no-process-env const PORT = process.env.PORT ? parseInt(process.env.PORT, 10) : 4010; -const artifactStorageReader = new ArtifactStorageReader(s3, null, null); +const artifactStorageReader = new ArtifactStorageReader(s3, null, null, null); const handleRequest = createRequestHandler({ isKeyValid: createIsKeyValid({ @@ -31,6 +31,10 @@ const handleRequest = createRequestHandler({ getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException(error) { + console.error(error); + }, }), async getArtifactAction(targetId, contractName, artifactType, eTag) { return artifactStorageReader.readArtifact(targetId, contractName, artifactType, eTag); @@ -52,6 +56,10 @@ const handleArtifactRequest = createArtifactRequestHandler({ getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException(error) { + console.error(error); + }, }), isAppDeploymentActive: createIsAppDeploymentActive({ artifactStorageReader, diff --git a/packages/services/cdn-worker/src/handler.ts b/packages/services/cdn-worker/src/handler.ts index 7332b88f1..885c52520 100644 --- a/packages/services/cdn-worker/src/handler.ts +++ b/packages/services/cdn-worker/src/handler.ts @@ -2,6 +2,7 @@ import { buildSchema, introspectionFromSchema } from 'graphql'; import { Analytics, createAnalytics } from './analytics'; import { GetArtifactActionFn } from './artifact-handler'; import { ArtifactsType as ModernArtifactsType } from './artifact-storage-reader'; +import { Breadcrumb, createBreadcrumb } from './breadcrumbs'; import { CDNArtifactNotFound, InvalidArtifactTypeResponse, @@ -192,6 +193,7 @@ async function parseIncomingRequest( request: Request, keyValidator: KeyValidator, analytics: Analytics, + breadcrumb: Breadcrumb, ): Promise< | { error: Response } | { @@ -239,6 +241,7 @@ async function parseIncomingRequest( legacyToModernArtifactTypeMap[artifactType], }; } catch (e) { + breadcrumb(`Failed to validate key for ${targetId}, error: ${e}`); console.warn(`Failed to validate key for ${targetId}, error:`, e); return { error: new InvalidAuthKeyResponse(analytics, request), @@ -255,15 +258,22 @@ interface RequestHandlerDependencies { isKeyValid: IsKeyValid; getArtifactAction: GetArtifactActionFn; analytics?: Analytics; + breadcrumb?: Breadcrumb; fetchText: (url: string) => Promise; } export function createRequestHandler(deps: RequestHandlerDependencies) { const analytics = deps.analytics ?? createAnalytics(); + const breadcrumb = deps.breadcrumb ?? createBreadcrumb(); const artifactTypesHandlers = createArtifactTypesHandlers(analytics); return async (request: Request): Promise => { - const parsedRequest = await parseIncomingRequest(request, deps.isKeyValid, analytics); + const parsedRequest = await parseIncomingRequest( + request, + deps.isKeyValid, + analytics, + breadcrumb, + ); if ('error' in parsedRequest) { return parsedRequest.error; diff --git a/packages/services/cdn-worker/src/index.ts b/packages/services/cdn-worker/src/index.ts index 20ec4bd99..2a66480b3 100644 --- a/packages/services/cdn-worker/src/index.ts +++ b/packages/services/cdn-worker/src/index.ts @@ -76,13 +76,46 @@ const handler: ExportedHandler = { s3: env.S3_ANALYTICS, }); - const artifactStorageReader = new ArtifactStorageReader(s3, s3Mirror, analytics); + const sentry = new Toucan({ + dsn: env.SENTRY_DSN, + environment: env.SENTRY_ENVIRONMENT, + release: env.SENTRY_RELEASE, + dist: 'cdn-worker', + context: ctx, + request, + requestDataOptions: { + allowedHeaders: [ + 'user-agent', + 'cf-ipcountry', + 'accept-encoding', + 'accept', + 'x-real-ip', + 'cf-connecting-ip', + ], + allowedSearchParams: /(.*)/, + }, + }); + + const artifactStorageReader = new ArtifactStorageReader( + s3, + s3Mirror, + analytics, + (message: string) => sentry.addBreadcrumb({ message }), + ); const isKeyValid = createIsKeyValid({ waitUntil: p => ctx.waitUntil(p), getCache: () => caches.open('artifacts-auth'), artifactStorageReader, analytics, + breadcrumb(message: string) { + sentry.addBreadcrumb({ + message, + }); + }, + captureException(error) { + sentry.captureException(error); + }, }); const handleRequest = createRequestHandler({ @@ -90,6 +123,11 @@ const handler: ExportedHandler = { return artifactStorageReader.readArtifact(targetId, contractName, artifactType, eTag); }, isKeyValid, + breadcrumb(message: string) { + sentry.addBreadcrumb({ + message, + }); + }, analytics, async fetchText(url) { // Yeah, it's not globally defined, but it makes no sense to define it globally @@ -134,6 +172,9 @@ const handler: ExportedHandler = { const handleArtifactRequest = createArtifactRequestHandler({ isKeyValid, analytics, + breadcrumb(message: string) { + sentry.addBreadcrumb({ message }); + }, artifactStorageReader, isAppDeploymentActive: createIsAppDeploymentActive({ artifactStorageReader, @@ -164,31 +205,16 @@ const handler: ExportedHandler = { // Legacy CDN Handlers .get('*', handleRequest); - const sentry = new Toucan({ - dsn: env.SENTRY_DSN, - environment: env.SENTRY_ENVIRONMENT, - release: env.SENTRY_RELEASE, - dist: 'cdn-worker', - context: ctx, - request, - requestDataOptions: { - allowedHeaders: [ - 'user-agent', - 'cf-ipcountry', - 'accept-encoding', - 'accept', - 'x-real-ip', - 'cf-connecting-ip', - ], - allowedSearchParams: /(.*)/, - }, - }); - try { return await router.handle(request, sentry.captureException.bind(sentry)).then(response => { if (response) { return response; } + + sentry.addBreadcrumb({ + message: 'No response from router', + }); + return createResponse(analytics, 'Not found', { status: 404 }, 'unknown', request); }); } catch (error) { diff --git a/packages/services/cdn-worker/src/key-validation.ts b/packages/services/cdn-worker/src/key-validation.ts index 9717b1321..746106c86 100644 --- a/packages/services/cdn-worker/src/key-validation.ts +++ b/packages/services/cdn-worker/src/key-validation.ts @@ -1,6 +1,7 @@ import bcrypt from 'bcryptjs'; import { Analytics } from './analytics'; import { ArtifactStorageReader } from './artifact-storage-reader'; +import type { Breadcrumb } from './breadcrumbs'; import { decodeCdnAccessTokenSafe, isCDNAccessToken } from './cdn-token'; export type KeyValidator = (targetId: string, headerKey: string) => Promise; @@ -14,6 +15,8 @@ type CreateKeyValidatorDeps = { artifactStorageReader: ArtifactStorageReader; getCache: null | GetCache; analytics: null | Analytics; + breadcrumb: null | Breadcrumb; + captureException: (error: Error) => void; }; export const createIsKeyValid = @@ -223,7 +226,20 @@ async function handleCDNAccessToken( return withCache(false); } - const isValid = await bcrypt.compare(decodeResult.token.privateKey, await key.text()); + const isValid = await bcrypt + .compare( + decodeResult.token.privateKey, + await key.text().catch(error => { + deps.breadcrumb?.('Failed to read body of key: ' + error.message); + deps.captureException(error); + return Promise.reject(error); + }), + ) + .catch(error => { + deps.breadcrumb?.(`Failed to compare keys: ${error.message}`); + deps.captureException(error); + return Promise.reject(error); + }); deps.analytics?.track( { diff --git a/packages/services/cdn-worker/tests/cdn.spec.ts b/packages/services/cdn-worker/tests/cdn.spec.ts index 55cd3b4b4..b07fcc91e 100644 --- a/packages/services/cdn-worker/tests/cdn.spec.ts +++ b/packages/services/cdn-worker/tests/cdn.spec.ts @@ -70,6 +70,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -84,6 +86,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -150,6 +153,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -164,6 +169,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -246,6 +252,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -260,6 +268,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -326,6 +335,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -340,6 +351,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -412,6 +424,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -426,6 +440,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -496,6 +511,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -510,6 +527,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -665,6 +683,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -679,6 +699,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -717,6 +738,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -731,6 +754,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction(targetId, _, artifactType) { @@ -767,6 +791,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader: new ArtifactStorageReader( { endpoint: 'http://localhost:1337', @@ -781,6 +807,7 @@ describe('CDN Worker', () => { }, null, null, + null, ), }), async getArtifactAction() { @@ -906,6 +933,7 @@ describe('CDN Worker', () => { }), }, null, + null, TIMEOUT, ); @@ -914,6 +942,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader, }), async getArtifactAction(targetId, contractName, artifactType, eTag) { @@ -1013,6 +1043,7 @@ describe('CDN Worker', () => { }), }, null, + null, TIMEOUT, ); @@ -1021,6 +1052,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader, }), async getArtifactAction(targetId, contractName, artifactType, eTag) { @@ -1118,6 +1151,7 @@ describe('CDN Worker', () => { }), }, null, + null, ); const handleRequest = createRequestHandler({ @@ -1125,6 +1159,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader, }), async getArtifactAction(targetId, contractName, artifactType, eTag) { @@ -1222,6 +1258,7 @@ describe('CDN Worker', () => { }), }, null, + null, ); const handleRequest = createRequestHandler({ @@ -1229,6 +1266,8 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, + breadcrumb: null, + captureException: console.error, artifactStorageReader, }), async getArtifactAction(targetId, contractName, artifactType, eTag) { diff --git a/packages/services/server/src/index.ts b/packages/services/server/src/index.ts index a9b0ed4d2..f0033ffdb 100644 --- a/packages/services/server/src/index.ts +++ b/packages/services/server/src/index.ts @@ -551,14 +551,24 @@ export async function main() { } : null; - const artifactStorageReader = new ArtifactStorageReader(s3, s3Mirror, null); + const artifactStorageReader = new ArtifactStorageReader(s3, s3Mirror, null, null); const artifactHandler = createArtifactRequestHandler({ isKeyValid: createIsKeyValid({ artifactStorageReader, analytics: null, + breadcrumb(message: string) { + server.log.debug(message); + }, getCache: null, waitUntil: null, + captureException(error) { + captureException(error, { + extra: { + source: 'artifactRequestHandler', + }, + }); + }, }), artifactStorageReader, isAppDeploymentActive: createIsAppDeploymentActive({