-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: consolidate backend secret custom resources #2011
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ad0a911 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
) { | ||
BackendSecretFetcherFactory.registerSecret(secretName); | ||
} |
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
const secretProviderFactory = new BackendSecretFetcherProviderFactory(); | |
const secretResourceFactory = new BackendSecretFetcherFactory( | |
secretProviderFactory | |
); |
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 registerSecret
become internal detail of the factory then ?
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.
@@ -25,34 +25,31 @@ void describe('getOrCreate', () => { | |||
const providerFactory = new BackendSecretFetcherProviderFactory(); | |||
const resourceFactory = new BackendSecretFetcherFactory(providerFactory); | |||
|
|||
beforeEach(() => { | |||
BackendSecretFetcherFactory.clearRegisteredSecrets(); |
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.
Instead of using static methods, we should rather control scope of variables and make the factory an effective singleton in the code, but not in tests.
): CustomResource => { | ||
const secretResourceId = `${secretName}SecretFetcherResource`; | ||
): SecretFetcherCustomResource => { | ||
const secretResourceId = `SecretFetcherResource`; |
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.
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.
for (const secretName of props.secretNames) { | ||
let secretValue: string | undefined = undefined; | ||
try { | ||
const resp = await secretClient.getSecret( |
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.
An optimization idea:
https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/client/secrets-manager/command/BatchGetSecretValueCommand/
since we're fetching multiple secrets.
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.
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
Problem
This consolidates secret fetcher custom resources into one.
A single custom resource will be created, instead of a resource for each secret.
Issue number, if available:
#1797
#1825
Changes
Corresponding docs PR, if applicable:
Validation
Checklist
run-e2e
label set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.