From 6e2ec052a61358b2f9834dd2ff4dc553ae2cfe6b Mon Sep 17 00:00:00 2001 From: Armando Luja Date: Thu, 12 Sep 2024 17:33:18 -0700 Subject: [PATCH 1/3] fix: use a single custom resource for fetching secrets --- .../engine/backend-secret/backend_secret.ts | 13 +-- .../backend_secret_fetcher_factory.test.ts | 44 +++++---- .../backend_secret_fetcher_factory.ts | 22 ++++- .../lambda/backend_secret_fetcher.test.ts | 8 +- .../lambda/backend_secret_fetcher.ts | 89 ++++++++++--------- .../lambda/backend_secret_fetcher_types.ts | 2 +- 6 files changed, 104 insertions(+), 74 deletions(-) diff --git a/packages/backend/src/engine/backend-secret/backend_secret.ts b/packages/backend/src/engine/backend-secret/backend_secret.ts index bb12316859..47b988c134 100644 --- a/packages/backend/src/engine/backend-secret/backend_secret.ts +++ b/packages/backend/src/engine/backend-secret/backend_secret.ts @@ -16,9 +16,11 @@ export class CfnTokenBackendSecret implements BackendSecret { * The name of the secret to fetch. */ constructor( - private readonly name: string, + private readonly secretName: string, private readonly secretResourceFactory: BackendSecretFetcherFactory - ) {} + ) { + BackendSecretFetcherFactory.registerSecret(secretName); + } /** * Get a reference to the value within a CDK scope. */ @@ -28,11 +30,10 @@ export class CfnTokenBackendSecret implements BackendSecret { ): SecretValue => { const secretResource = this.secretResourceFactory.getOrCreate( scope, - this.name, backendIdentifier ); - const val = secretResource.getAttString('secretValue'); + const val = secretResource.getAttString(`${this.secretName}`); return SecretValue.unsafePlainText(val); // safe since 'val' is a cdk token. }; @@ -43,11 +44,11 @@ export class CfnTokenBackendSecret implements BackendSecret { return { branchSecretPath: ParameterPathConversions.toParameterFullPath( backendIdentifier, - this.name + this.secretName ), sharedSecretPath: ParameterPathConversions.toParameterFullPath( backendIdentifier.namespace, - this.name + this.secretName ), }; }; diff --git a/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.test.ts b/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.test.ts index b2d5cd1bb9..052d68e391 100644 --- a/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.test.ts +++ b/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.test.ts @@ -1,5 +1,5 @@ import { App, Stack } from 'aws-cdk-lib'; -import { describe, it } from 'node:test'; +import { beforeEach, describe, it } from 'node:test'; import { BackendSecretFetcherProviderFactory } from './backend_secret_fetcher_provider_factory.js'; import { Template } from 'aws-cdk-lib/assertions'; import assert from 'node:assert'; @@ -25,34 +25,31 @@ void describe('getOrCreate', () => { const providerFactory = new BackendSecretFetcherProviderFactory(); const resourceFactory = new BackendSecretFetcherFactory(providerFactory); + beforeEach(() => { + BackendSecretFetcherFactory.clearRegisteredSecrets(); + }); + void it('create different secrets', () => { const app = new App(); const stack = new Stack(app); stack.node.setContext('secretLastUpdated', secretLastUpdated); - resourceFactory.getOrCreate(stack, secretName1, backendId); - resourceFactory.getOrCreate(stack, secretName2, backendId); + BackendSecretFetcherFactory.registerSecret(secretName1); + BackendSecretFetcherFactory.registerSecret(secretName2); + resourceFactory.getOrCreate(stack, backendId); const template = Template.fromStack(stack); - template.resourceCountIs(secretResourceType, 2); - let customResources = template.findResources(secretResourceType, { + // only one custom resource is created that fetches all secrets + template.resourceCountIs(secretResourceType, 1); + const customResources = template.findResources(secretResourceType, { Properties: { namespace, name, - secretName: secretName1, + secretNames: [secretName1, secretName2], secretLastUpdated, }, }); assert.equal(Object.keys(customResources).length, 1); - customResources = template.findResources(secretResourceType, { - Properties: { - namespace, - name, - secretName: secretName2, - }, - }); - assert.equal(Object.keys(customResources).length, 1); - // only 1 secret fetcher lambda and 1 resource provider lambda are created. const providers = template.findResources('AWS::Lambda::Function'); const names = Object.keys(providers); @@ -67,8 +64,18 @@ void describe('getOrCreate', () => { void it('does not create duplicate resource for the same secret name', () => { const app = new App(); const stack = new Stack(app); - resourceFactory.getOrCreate(stack, secretName1, backendId); - resourceFactory.getOrCreate(stack, secretName1, backendId); + // ensure only 1 secret name is registered if they are duplicates + BackendSecretFetcherFactory.registerSecret(secretName1); + BackendSecretFetcherFactory.registerSecret(secretName1); + assert.equal(BackendSecretFetcherFactory.secretNames.size, 1); + assert.equal( + Array.from(BackendSecretFetcherFactory.secretNames)[0], + secretName1 + ); + + // ensure only 1 resource is created even if this is called twice + resourceFactory.getOrCreate(stack, backendId); + resourceFactory.getOrCreate(stack, backendId); const template = Template.fromStack(stack); template.resourceCountIs(secretResourceType, 1); @@ -78,6 +85,7 @@ void describe('getOrCreate', () => { const body = customResources[resourceName]['Properties']; assert.strictEqual(body['namespace'], namespace); assert.strictEqual(body['name'], name); - assert.strictEqual(body['secretName'], secretName1); + assert.equal(body['secretNames'].length, 1); + assert.equal(body['secretNames'][0], secretName1); }); }); diff --git a/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.ts b/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.ts index f3fd0a2164..058dbfeb91 100644 --- a/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.ts +++ b/packages/backend/src/engine/backend-secret/backend_secret_fetcher_factory.ts @@ -18,6 +18,8 @@ const SECRET_RESOURCE_TYPE = `Custom::SecretFetcherResource`; * The factory to create backend secret-fetcher resource. */ export class BackendSecretFetcherFactory { + static secretNames: Set = new Set(); + /** * Creates a backend secret-fetcher resource factory. */ @@ -25,16 +27,30 @@ export class BackendSecretFetcherFactory { private readonly secretProviderFactory: BackendSecretFetcherProviderFactory ) {} + /** + * Register secrets that to be fetched by the BackendSecretFetcher custom resource.\ + * @param secretName the name of the secret + */ + static registerSecret = (secretName: string): void => { + BackendSecretFetcherFactory.secretNames.add(secretName); + }; + + /** + * Clear registered secrets that will be fetched by the BackendSecretFetcher custom resource. + */ + static clearRegisteredSecrets = (): void => { + BackendSecretFetcherFactory.secretNames.clear(); + }; + /** * Returns a resource if it exists in the input scope. Otherwise, * creates a new one. */ getOrCreate = ( scope: Construct, - secretName: string, backendIdentifier: BackendIdentifier ): CustomResource => { - const secretResourceId = `${secretName}SecretFetcherResource`; + const secretResourceId = `SecretFetcherResource`; const existingResource = scope.node.tryFindChild( secretResourceId ) as CustomResource; @@ -59,7 +75,7 @@ export class BackendSecretFetcherFactory { namespace: backendIdentifier.namespace, name: backendIdentifier.name, type: backendIdentifier.type, - secretName: secretName, + secretNames: Array.from(BackendSecretFetcherFactory.secretNames), }; return new CustomResource(scope, secretResourceId, { diff --git a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.test.ts b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.test.ts index 0cb0773446..71c4e6cb50 100644 --- a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.test.ts +++ b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.test.ts @@ -43,7 +43,7 @@ const customResourceEventCommon = { ResourceType: 'AWS::CloudFormation::CustomResource', ResourceProperties: { ...testBackendIdentifier, - secretName: testSecretName, + secretNames: [testSecretName], ServiceToken: 'token', }, OldResourceProperties: {}, @@ -84,7 +84,7 @@ void describe('handleCreateUpdateEvent', () => { Promise.resolve(testSecret) ); const val = await handleCreateUpdateEvent(secretHandler, createCfnEvent); - assert.equal(val, testSecretValue); + assert.equal(val[testSecretName], testSecretValue); assert.equal(mockGetSecret.mock.callCount(), 1); assert.deepStrictEqual(mockGetSecret.mock.calls[0].arguments, [ @@ -120,7 +120,7 @@ void describe('handleCreateUpdateEvent', () => { ); const val = await handleCreateUpdateEvent(secretHandler, createCfnEvent); - assert.equal(val, testSecretValue); + assert.equal(val[testSecretName], testSecretValue); assert.equal(mockGetSecret.mock.callCount(), 2); assert.deepStrictEqual(mockGetSecret.mock.calls[0].arguments, [ @@ -145,7 +145,7 @@ void describe('handleCreateUpdateEvent', () => { } ); const val = await handleCreateUpdateEvent(secretHandler, createCfnEvent); - assert.equal(val, testSecretValue); + assert.equal(val[testSecretName], testSecretValue); assert.equal(mockGetSecret.mock.callCount(), 2); assert.deepStrictEqual(mockGetSecret.mock.calls[0].arguments, [ diff --git a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.ts b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.ts index 6c434c211d..f240eb45db 100644 --- a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.ts +++ b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher.ts @@ -22,11 +22,11 @@ export const handler = async ( const physicalId = event.RequestType === 'Create' ? randomUUID() : event.PhysicalResourceId; - let data: { secretValue: string } | undefined = undefined; + let data: Record | undefined = undefined; if (event.RequestType === 'Update' || event.RequestType === 'Create') { - const val = await handleCreateUpdateEvent(secretClient, event); + const secretMap = await handleCreateUpdateEvent(secretClient, event); data = { - secretValue: val, + ...secretMap, }; } @@ -47,54 +47,59 @@ export const handler = async ( export const handleCreateUpdateEvent = async ( secretClient: SecretClient, event: CloudFormationCustomResourceEvent -): Promise => { +): Promise> => { const props = event.ResourceProperties as unknown as SecretResourceProps; - let secret: string | undefined; + const secretMap: Record = {}; + for (const secretName of props.secretNames) { + let secretValue: string | undefined = undefined; + try { + const resp = await secretClient.getSecret( + { + namespace: props.namespace, + name: props.name, + type: props.type, + }, + { + name: secretName, + } + ); + secretValue = resp.value; + } catch (err) { + const secretErr = err as SecretError; + if (secretErr.httpStatusCode && secretErr.httpStatusCode >= 500) { + throw new Error( + `Failed to retrieve backend secret '${secretName}' for '${ + props.namespace + }/${props.name}'. Reason: ${JSON.stringify(err)}` + ); + } + } - try { - const resp = await secretClient.getSecret( - { - namespace: props.namespace, - name: props.name, - type: props.type, - }, - { - name: props.secretName, + // if the secret is not available in branch path, try retrieving it at the app-level. + if (!secretValue) { + try { + const resp = await secretClient.getSecret(props.namespace, { + name: secretName, + }); + secretValue = resp.value; + } catch (err) { + throw new Error( + `Failed to retrieve backend secret '${secretName}' for '${ + props.namespace + }'. Reason: ${JSON.stringify(err)}` + ); } - ); - secret = resp?.value; - } catch (err) { - const secretErr = err as SecretError; - if (secretErr.httpStatusCode && secretErr.httpStatusCode >= 500) { - throw new Error( - `Failed to retrieve backend secret '${props.secretName}' for '${ - props.namespace - }/${props.name}'. Reason: ${JSON.stringify(err)}` - ); } - } - // if the secret is not available in branch path, try retrieving it at the app-level. - if (!secret) { - try { - const resp = await secretClient.getSecret(props.namespace, { - name: props.secretName, - }); - secret = resp?.value; - } catch (err) { + if (!secretValue) { throw new Error( - `Failed to retrieve backend secret '${props.secretName}' for '${ - props.namespace - }'. Reason: ${JSON.stringify(err)}` + `Unable to find backend secret for backend '${props.namespace}', branch '${props.name}', name '${secretName}'` ); } - } - if (!secret) { - throw new Error( - `Unable to find backend secret for backend '${props.namespace}', branch '${props.name}', name '${props.secretName}'` - ); + // store the secret->secretValue pair in the secret map + secretMap[secretName] = secretValue; } - return secret; + return secretMap; }; diff --git a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher_types.ts b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher_types.ts index a5cb72b4de..3e625238b5 100644 --- a/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher_types.ts +++ b/packages/backend/src/engine/backend-secret/lambda/backend_secret_fetcher_types.ts @@ -1,5 +1,5 @@ import { BackendIdentifier } from '@aws-amplify/plugin-types'; export type SecretResourceProps = Omit & { - secretName: string; + secretNames: string[]; }; From 9ea31cdb89838fcfaefb0934d32f3a778943af27 Mon Sep 17 00:00:00 2001 From: Armando Luja Date: Thu, 12 Sep 2024 17:36:44 -0700 Subject: [PATCH 2/3] chore: update package-lock.json --- package-lock.json | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/package-lock.json b/package-lock.json index 06b7cca03b..23833da26e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24670,7 +24670,7 @@ }, "packages/ai-constructs": { "name": "@aws-amplify/ai-constructs", - "version": "0.1.2", + "version": "0.1.4", "license": "Apache-2.0", "dependencies": { "@aws-amplify/plugin-types": "^1.0.1", @@ -24732,10 +24732,10 @@ }, "packages/backend-ai": { "name": "@aws-amplify/backend-ai", - "version": "0.1.0", + "version": "0.1.1", "license": "Apache-2.0", "dependencies": { - "@aws-amplify/ai-constructs": "^0.1.0", + "@aws-amplify/ai-constructs": "^0.1.4", "@aws-amplify/backend-output-schemas": "^1.1.0", "@aws-amplify/backend-output-storage": "^1.0.2", "@aws-amplify/platform-core": "^1.1.0", @@ -24749,7 +24749,7 @@ }, "packages/backend-auth": { "name": "@aws-amplify/backend-auth", - "version": "1.1.3", + "version": "1.1.4", "license": "Apache-2.0", "dependencies": { "@aws-amplify/auth-construct": "^1.3.0", @@ -24788,7 +24788,7 @@ }, "packages/backend-deployer": { "name": "@aws-amplify/backend-deployer", - "version": "1.1.0", + "version": "1.1.2", "license": "Apache-2.0", "dependencies": { "@aws-amplify/platform-core": "^1.0.6", @@ -24900,10 +24900,10 @@ }, "packages/cli": { "name": "@aws-amplify/backend-cli", - "version": "1.2.5", + "version": "1.2.6", "license": "Apache-2.0", "dependencies": { - "@aws-amplify/backend-deployer": "^1.1.0", + "@aws-amplify/backend-deployer": "^1.1.1", "@aws-amplify/backend-output-schemas": "^1.1.0", "@aws-amplify/backend-secret": "^1.1.0", "@aws-amplify/cli-core": "^1.1.2", @@ -25118,7 +25118,7 @@ } }, "packages/create-amplify": { - "version": "1.0.4", + "version": "1.0.5", "license": "Apache-2.0", "dependencies": { "@aws-amplify/cli-core": "^1.1.1", @@ -25343,7 +25343,7 @@ }, "packages/model-generator": { "name": "@aws-amplify/model-generator", - "version": "1.0.5", + "version": "1.0.6", "license": "Apache-2.0", "dependencies": { "@aws-amplify/backend-output-schemas": "^1.1.0", @@ -25543,7 +25543,7 @@ }, "packages/schema-generator": { "name": "@aws-amplify/schema-generator", - "version": "1.2.1", + "version": "1.2.2", "license": "Apache-2.0", "dependencies": { "@aws-amplify/graphql-schema-generator": "^0.9.4", From 3bb4b7462edc97538ef15989b7449ba7818e3cc9 Mon Sep 17 00:00:00 2001 From: Armando Luja Date: Thu, 12 Sep 2024 17:41:49 -0700 Subject: [PATCH 3/3] chore: add changeset --- .changeset/purple-otters-poke.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/purple-otters-poke.md diff --git a/.changeset/purple-otters-poke.md b/.changeset/purple-otters-poke.md new file mode 100644 index 0000000000..2c4c4f6bd5 --- /dev/null +++ b/.changeset/purple-otters-poke.md @@ -0,0 +1,5 @@ +--- +'@aws-amplify/backend': patch +--- + +Backend Secrets now use a single custom resource to reduce concurrent lambda executions.