From b5ddafb5f296b07d243f6a75375b45bdacd4b262 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Wed, 28 Aug 2024 14:42:45 +0200 Subject: [PATCH] feat: skip presigned url step + analytics (#5542) --- packages/services/cdn-worker/src/analytics.ts | 10 +- .../cdn-worker/src/artifact-handler.ts | 2 +- .../cdn-worker/src/artifact-storage-reader.ts | 192 ++++++++++----- packages/services/cdn-worker/src/aws.ts | 48 +++- packages/services/cdn-worker/src/dev.ts | 21 +- packages/services/cdn-worker/src/index.ts | 9 +- .../services/cdn-worker/src/key-validation.ts | 50 +--- .../services/cdn-worker/tests/cdn.spec.ts | 226 ++++++++++-------- packages/services/server/src/index.ts | 7 +- 9 files changed, 335 insertions(+), 230 deletions(-) diff --git a/packages/services/cdn-worker/src/analytics.ts b/packages/services/cdn-worker/src/analytics.ts index 15d1e6fda..dd23d2307 100644 --- a/packages/services/cdn-worker/src/analytics.ts +++ b/packages/services/cdn-worker/src/analytics.ts @@ -57,12 +57,15 @@ type Event = | { type: 'r2'; action: - | 'HEAD artifact' + | 'GET artifact' | 'GET cdn-legacy-keys' | 'GET cdn-access-token' | 'GET persistedOperation' | 'HEAD appDeploymentIsEnabled'; - statusCode: number; + // Either 3 digit status code or error code e.g. timeout, http error etc. + statusCodeOrErrCode: number | string; + /** duration in milliseconds */ + duration: number; } | { type: 'response'; @@ -97,7 +100,8 @@ export function createAnalytics( }); case 'r2': return engines.r2.writeDataPoint({ - blobs: [event.action, event.statusCode.toString(), targetId], + blobs: [event.action, event.statusCodeOrErrCode.toString(), targetId], + doubles: [event.duration], indexes: [targetId.substring(0, 32)], }); case 'response': diff --git a/packages/services/cdn-worker/src/artifact-handler.ts b/packages/services/cdn-worker/src/artifact-handler.ts index 45a68f96d..4250a8be6 100644 --- a/packages/services/cdn-worker/src/artifact-handler.ts +++ b/packages/services/cdn-worker/src/artifact-handler.ts @@ -129,7 +129,7 @@ export const createArtifactRequestHandler = (deps: ArtifactRequestHandler) => { const eTag = request.headers.get('if-none-match'); - const result = await deps.artifactStorageReader.generateArtifactReadUrl( + const result = await deps.artifactStorageReader.readArtifact( params.targetId, params.contractName, params.artifactType, diff --git a/packages/services/cdn-worker/src/artifact-storage-reader.ts b/packages/services/cdn-worker/src/artifact-storage-reader.ts index 7510d0677..f0005e340 100644 --- a/packages/services/cdn-worker/src/artifact-storage-reader.ts +++ b/packages/services/cdn-worker/src/artifact-storage-reader.ts @@ -65,11 +65,16 @@ export class ArtifactStorageReader { endpoint: string; bucketName: string; }, + // private s3Mirror: { + // client: AwsClient; + // endpoint: string; + // bucketName: string; + // }, private analytics: Analytics | null, ) {} - /** Generate a pre-signed url for reading an artifact from a bucket for a limited time period. */ - async generateArtifactReadUrl( + /** Read an artifact from S3 */ + async readArtifact( targetId: string, contractName: string | null, artifactType: ArtifactsType, @@ -81,55 +86,57 @@ export class ArtifactStorageReader { const key = buildArtifactStorageKey(targetId, artifactType, contractName); - const headResponse = await this.s3.client.fetch( + const headers: HeadersInit = {}; + + if (etagValue) { + headers['if-none-match'] = etagValue; + } + + const response = await this.s3.client.fetch( [this.s3.endpoint, this.s3.bucketName, key].join('/'), { - method: 'HEAD', + method: 'GET', + headers, aws: { signQuery: true, }, timeout: READ_TIMEOUT_MS, - }, - ); - this.analytics?.track( - { - type: 'r2', - statusCode: headResponse.status, - action: 'HEAD artifact', - }, - targetId, - ); - - if (headResponse.status === 200) { - if (etagValue && headResponse.headers.get('etag') === etagValue) { - return { type: 'notModified' } as const; - } - - const getResponse = await this.s3.client.fetch( - [this.s3.endpoint, this.s3.bucketName, key].join('/'), - { - method: 'GET', - aws: { - signQuery: true, - }, - timeout: READ_TIMEOUT_MS, + onAttempt: args => { + this.analytics?.track( + { + type: 'r2', + statusCodeOrErrCode: + args.result.type === 'error' + ? String(args.result.error.name ?? 'unknown') + : args.result.response.status, + action: 'GET artifact', + duration: args.duration, + }, + targetId, + ); }, - ); + }, + ); - if (getResponse.ok) { - return { - type: 'response', - response: getResponse, - } as const; - } - - throw new Error(`GET request failed with status ${getResponse.status}`); - } - if (headResponse.status === 404) { + if (response.status === 404) { return { type: 'notFound' } as const; } - const body = await headResponse.text(); - throw new Error(`HEAD request failed with status ${headResponse.status}: ${body}`); + + if (response.status === 304) { + return { + type: 'notModified', + } as const; + } + + if (response.status === 200) { + return { + type: 'response', + response, + } as const; + } + + const body = await response.text(); + throw new Error(`GET request failed with status ${response.status}: ${body}`); } async isAppDeploymentEnabled(targetId: string, appName: string, appVersion: string) { @@ -143,16 +150,22 @@ export class ArtifactStorageReader { signQuery: true, }, timeout: READ_TIMEOUT_MS, + onAttempt: args => { + this.analytics?.track( + { + type: 'r2', + statusCodeOrErrCode: + args.result.type === 'error' + ? String(args.result.error.name ?? 'unknown') + : args.result.response.status, + action: 'HEAD appDeploymentIsEnabled', + duration: args.duration, + }, + targetId, + ); + }, }, ); - this.analytics?.track( - { - type: 'r2', - statusCode: response.status, - action: 'HEAD appDeploymentIsEnabled', - }, - targetId, - ); return response.status === 200; } @@ -180,18 +193,23 @@ export class ArtifactStorageReader { }, headers, timeout: READ_TIMEOUT_MS, + onAttempt: args => { + this.analytics?.track( + { + type: 'r2', + statusCodeOrErrCode: + args.result.type === 'error' + ? String(args.result.error.name ?? 'unknown') + : args.result.response.status, + action: 'GET persistedOperation', + duration: args.duration, + }, + targetId, + ); + }, }, ); - this.analytics?.track( - { - type: 'r2', - statusCode: response.status, - action: 'GET persistedOperation', - }, - targetId, - ); - if (etagValue && response.status === 304) { return { type: 'notModified' } as const; } @@ -211,4 +229,62 @@ export class ArtifactStorageReader { const body = await response.text(); throw new Error(`HEAD request failed with status ${response.status}: ${body}`); } + + async readLegacyAccessKey(targetId: string) { + const response = await this.s3.client.fetch( + [this.s3.endpoint, this.s3.bucketName, 'cdn-legacy-keys', targetId].join('/'), + { + method: 'GET', + timeout: READ_TIMEOUT_MS, + onAttempt: args => { + this.analytics?.track( + { + type: 'r2', + statusCodeOrErrCode: + args.result.type === 'error' + ? String(args.result.error.name ?? 'unknown') + : args.result.response.status, + action: 'GET cdn-legacy-keys', + duration: args.duration, + }, + targetId, + ); + }, + }, + ); + + return response; + } + + async readAccessKey(targetId: string, keyId: string) { + const s3KeyParts = ['cdn-keys', targetId, keyId]; + + const response = await this.s3.client.fetch( + [this.s3.endpoint, this.s3.bucketName, ...s3KeyParts].join('/'), + { + method: 'GET', + aws: { + // This boolean makes Google Cloud Storage & AWS happy. + signQuery: true, + }, + timeout: READ_TIMEOUT_MS, + onAttempt: args => { + this.analytics?.track( + { + type: 'r2', + statusCodeOrErrCode: + args.result.type === 'error' + ? String(args.result.error.name ?? 'unknown') + : args.result.response.status, + action: 'GET cdn-access-token', + duration: args.duration, + }, + targetId, + ); + }, + }, + ); + + return response; + } } diff --git a/packages/services/cdn-worker/src/aws.ts b/packages/services/cdn-worker/src/aws.ts index bcff52041..9c835dfeb 100644 --- a/packages/services/cdn-worker/src/aws.ts +++ b/packages/services/cdn-worker/src/aws.ts @@ -49,6 +49,25 @@ type AwsRequestInit = RequestInit & { * Timeout in milliseconds for each fetch call. */ timeout?: number; + /** Hook being invoked for each attempt for gathering analytics or similar. */ + onAttempt?: (args: { + /** attempt number */ + attempt: number; + /** attempt duration in ms */ + duration: number; + /** result */ + result: + | { + // HTTP or other unexpected error + type: 'error'; + error: Error; + } + | { + // HTTP response sent by upstream server + type: 'success'; + response: Response; + }; + }) => void; }; export type AWSClientConfig = { @@ -82,7 +101,7 @@ export class AwsClient { this.service = args.service; this.region = args.region; this.cache = args.cache || new Map(); - this.retries = args.retries != null ? args.retries : 10; // Up to 25.6 secs + this.retries = args.retries != null ? args.retries : 3; this.initRetryMs = args.initRetryMs || 50; this._fetch = args.fetch || fetch.bind(globalThis); } @@ -129,18 +148,31 @@ export class AwsClient { async fetch(input: RequestInfo, init: AwsRequestInit): Promise { for (let i = 0; i <= this.retries; i++) { - const fetched = this._fetch(...(await this.sign(input, init))); - if (i === this.retries) { - return fetched; // No need to await if we're returning anyway - } + const attemptStart = performance.now(); try { - const res = await fetched; - if (res.status < 500 && res.status !== 429 && res.status !== 499) { - return res; + const response = await this._fetch(...(await this.sign(input, init))); + const duration = performance.now() - attemptStart; + init.onAttempt?.({ attempt: i, duration, result: { type: 'success', response } }); + + if ( + (response.status < 500 && response.status !== 429 && response.status !== 499) || + i === this.retries + ) { + return response; } } catch (error) { + const duration = performance.now() - attemptStart; // Retry also when there's an exception console.error(error); + init.onAttempt?.({ + attempt: i, + duration, + result: { type: 'error', error: error as Error }, + }); + + if (i === this.retries) { + throw error; + } } await new Promise(resolve => setTimeout(resolve, Math.random() * this.initRetryMs * Math.pow(2, i)), diff --git a/packages/services/cdn-worker/src/dev.ts b/packages/services/cdn-worker/src/dev.ts index c286f5ae5..3837ef2db 100644 --- a/packages/services/cdn-worker/src/dev.ts +++ b/packages/services/cdn-worker/src/dev.ts @@ -26,14 +26,14 @@ const PORT = process.env.PORT ? parseInt(process.env.PORT, 10) : 4010; const artifactStorageReader = new ArtifactStorageReader(s3, null); const handleRequest = createRequestHandler({ - isKeyValid: createIsKeyValid({ s3, getCache: null, waitUntil: null, analytics: null }), + isKeyValid: createIsKeyValid({ + artifactStorageReader, + getCache: null, + waitUntil: null, + analytics: null, + }), async getArtifactAction(targetId, contractName, artifactType, eTag) { - return artifactStorageReader.generateArtifactReadUrl( - targetId, - contractName, - artifactType, - eTag, - ); + return artifactStorageReader.readArtifact(targetId, contractName, artifactType, eTag); }, async fetchText(url) { const r = await fetch(url); @@ -47,7 +47,12 @@ const handleRequest = createRequestHandler({ }); const handleArtifactRequest = createArtifactRequestHandler({ - isKeyValid: createIsKeyValid({ s3, getCache: null, waitUntil: null, analytics: null }), + isKeyValid: createIsKeyValid({ + artifactStorageReader, + getCache: null, + waitUntil: null, + analytics: null, + }), isAppDeploymentActive: createIsAppDeploymentActive({ artifactStorageReader, getCache: null, diff --git a/packages/services/cdn-worker/src/index.ts b/packages/services/cdn-worker/src/index.ts index 850738ddc..a42e2ff5a 100644 --- a/packages/services/cdn-worker/src/index.ts +++ b/packages/services/cdn-worker/src/index.ts @@ -61,18 +61,13 @@ const handler: ExportedHandler = { const isKeyValid = createIsKeyValid({ waitUntil: p => ctx.waitUntil(p), getCache: () => caches.open('artifacts-auth'), - s3, + artifactStorageReader, analytics, }); const handleRequest = createRequestHandler({ async getArtifactAction(targetId, contractName, artifactType, eTag) { - return artifactStorageReader.generateArtifactReadUrl( - targetId, - contractName, - artifactType, - eTag, - ); + return artifactStorageReader.readArtifact(targetId, contractName, artifactType, eTag); }, isKeyValid, analytics, diff --git a/packages/services/cdn-worker/src/key-validation.ts b/packages/services/cdn-worker/src/key-validation.ts index accb61745..9717b1321 100644 --- a/packages/services/cdn-worker/src/key-validation.ts +++ b/packages/services/cdn-worker/src/key-validation.ts @@ -1,23 +1,17 @@ import bcrypt from 'bcryptjs'; import { Analytics } from './analytics'; -import { type AwsClient } from './aws'; +import { ArtifactStorageReader } from './artifact-storage-reader'; import { decodeCdnAccessTokenSafe, isCDNAccessToken } from './cdn-token'; export type KeyValidator = (targetId: string, headerKey: string) => Promise; type WaitUntil = (promise: Promise) => void; -type S3Config = { - client: AwsClient; - bucketName: string; - endpoint: string; -}; - type GetCache = () => Promise; type CreateKeyValidatorDeps = { waitUntil: null | WaitUntil; - s3: S3Config; + artifactStorageReader: ArtifactStorageReader; getCache: null | GetCache; analytics: null | Analytics; }; @@ -39,7 +33,7 @@ export const createIsKeyValid = const handleLegacyCDNAccessToken = async (args: { targetId: string; accessToken: string; - s3: S3Config; + artifactStorageReader: ArtifactStorageReader; getCache: null | GetCache; waitUntil: null | WaitUntil; analytics: null | Analytics; @@ -118,21 +112,7 @@ const handleLegacyCDNAccessToken = async (args: { } } - const key = await args.s3.client.fetch( - [args.s3.endpoint, args.s3.bucketName, 'cdn-legacy-keys', args.targetId].join('/'), - { - method: 'GET', - }, - ); - - args.analytics?.track( - { - type: 'r2', - statusCode: key.status, - action: 'GET cdn-legacy-keys', - }, - args.targetId, - ); + const key = await args.artifactStorageReader.readLegacyAccessKey(args.targetId); if (key.status !== 200) { return withCache(false); @@ -237,27 +217,7 @@ async function handleCDNAccessToken( return withCache(false); } - const s3KeyParts = ['cdn-keys', targetId, decodeResult.token.keyId]; - - const key = await deps.s3.client.fetch( - [deps.s3.endpoint, deps.s3.bucketName, ...s3KeyParts].join('/'), - { - method: 'GET', - aws: { - // This boolean makes Google Cloud Storage & AWS happy. - signQuery: true, - }, - }, - ); - - deps.analytics?.track( - { - type: 'r2', - statusCode: key.status, - action: 'GET cdn-access-token', - }, - targetId, - ); + const key = await deps.artifactStorageReader.readAccessKey(targetId, decodeResult.token.keyId); if (key.status !== 200) { return withCache(false); diff --git a/packages/services/cdn-worker/tests/cdn.spec.ts b/packages/services/cdn-worker/tests/cdn.spec.ts index 6550ed7e7..9789e822d 100644 --- a/packages/services/cdn-worker/tests/cdn.spec.ts +++ b/packages/services/cdn-worker/tests/cdn.spec.ts @@ -3,6 +3,7 @@ import * as bcrypt from 'bcryptjs'; import '../src/dev-polyfill'; // eslint-disable-next-line import/no-extraneous-dependencies import { describe, expect, test } from 'vitest'; +import { ArtifactStorageReader } from '../src/artifact-storage-reader'; import { InvalidArtifactTypeResponse, InvalidAuthKeyResponse, @@ -48,17 +49,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -124,17 +128,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -216,17 +223,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -292,17 +302,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -374,17 +387,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -454,17 +470,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -619,17 +638,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash(token, await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -667,17 +689,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(null, { - status: 404, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(null, { + status: 404, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction(targetId, _, artifactType) { return map.has(`target:${targetId}:${artifactType}`) @@ -713,17 +738,20 @@ describe('CDN Worker', () => { getCache: null, waitUntil: null, analytics: null, - s3: { - endpoint: 'http://localhost:1337', - bucketName: 'artifacts', - client: { - async fetch() { - return new Response(await bcrypt.hash('foobars', await bcrypt.genSalt()), { - status: 200, - }); - }, - } as any, - }, + artifactStorageReader: new ArtifactStorageReader( + { + endpoint: 'http://localhost:1337', + bucketName: 'artifacts', + client: { + async fetch() { + return new Response(await bcrypt.hash('foobars', await bcrypt.genSalt()), { + status: 200, + }); + }, + } as any, + }, + null, + ), }), async getArtifactAction() { return { diff --git a/packages/services/server/src/index.ts b/packages/services/server/src/index.ts index 5aa06d99b..d5ccfbd99 100644 --- a/packages/services/server/src/index.ts +++ b/packages/services/server/src/index.ts @@ -542,7 +542,12 @@ export async function main() { const artifactStorageReader = new ArtifactStorageReader(s3, null); const artifactHandler = createArtifactRequestHandler({ - isKeyValid: createIsKeyValid({ s3, analytics: null, getCache: null, waitUntil: null }), + isKeyValid: createIsKeyValid({ + artifactStorageReader, + analytics: null, + getCache: null, + waitUntil: null, + }), artifactStorageReader, isAppDeploymentActive: createIsAppDeploymentActive({ artifactStorageReader,