-
Notifications
You must be signed in to change notification settings - Fork 103
fix: consolidate backend secret custom resources #2011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
6e2ec05
5f1a4e8
9ea31cd
3bb4b74
ac67ec4
0faa76a
d901be3
4f4d04c
6bdaecd
6d47e1b
e99ba20
ad0a911
63efabb
bdbd373
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@aws-amplify/backend': patch | ||
| --- | ||
|
|
||
| Backend Secrets now use a single custom resource to reduce concurrent lambda executions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -18,23 +18,39 @@ const SECRET_RESOURCE_TYPE = `Custom::SecretFetcherResource`; | |||||
| * The factory to create backend secret-fetcher resource. | ||||||
| */ | ||||||
| export class BackendSecretFetcherFactory { | ||||||
| static secretNames: Set<string> = new Set<string>(); | ||||||
|
|
||||||
| /** | ||||||
| * Creates a backend secret-fetcher resource factory. | ||||||
| */ | ||||||
| constructor( | ||||||
| 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 secretResourceId = `SecretFetcherResource`; | |
| const secretResourceId = `AmplifySecretFetcherResource`; |
just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated (AmplifySecretFetcherCustomResource)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant resource id not the class name.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<string, string> | 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<string> => { | ||
| ): Promise<Record<string, string>> => { | ||
| const props = event.ResourceProperties as unknown as SecretResourceProps; | ||
| let secret: string | undefined; | ||
| const secretMap: Record<string, string> = {}; | ||
| for (const secretName of props.secretNames) { | ||
| let secretValue: string | undefined = undefined; | ||
| try { | ||
| const resp = await secretClient.getSecret( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An optimization idea: since we're fetching multiple secrets.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may be a good optimization as separate item, it would be https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/client/ssm/command/GetParametersCommand/ since we don't use secrets manager ; but we have custom try-catch logic for each secret here, so it would be something like batching all secrets with one command, then batch the retries, and aggregate errors |
||
| { | ||
| 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; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { BackendIdentifier } from '@aws-amplify/plugin-types'; | ||
|
|
||
| export type SecretResourceProps = Omit<BackendIdentifier, 'hash'> & { | ||
| secretName: string; | ||
| secretNames: string[]; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like a healthy data flow.
Could we move this detail into factory (since we do receive reference to factory in this ctor already)?
For example.
Change scope of these variables
amplify-backend/packages/backend/src/secret.ts
Lines 30 to 33 in 8dd7286
Use cdk.Lazy to accumulate secret names inside factory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are singleton factories, except they do not use DI system like other factories, they use the scope.node.tryFindChild('')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can
registerSecretbecome internal detail of the factory then ?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awsluja this is what I have in mind 9816f17 .
(I haven't touched tests, but they should be covered by different scoping of factories).