Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/purple-otters-poke.md
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.
13 changes: 7 additions & 6 deletions packages/backend/src/engine/backend-secret/backend_secret.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Copy link
Contributor

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

const secretProviderFactory = new BackendSecretFetcherProviderFactory();
const secretResourceFactory = new BackendSecretFetcherFactory(
secretProviderFactory
);
and promote them to global scope (to make factory a singleton)
Use cdk.Lazy to accumulate secret names inside factory?

Copy link
Contributor Author

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('')

Copy link
Contributor

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 ?

Copy link
Contributor

@sobolk sobolk Sep 17, 2024

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).

/**
* Get a reference to the value within a CDK scope.
*/
Expand All @@ -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.
};

Expand All @@ -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
),
};
};
Expand Down
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';
Expand All @@ -25,34 +25,31 @@ void describe('getOrCreate', () => {
const providerFactory = new BackendSecretFetcherProviderFactory();
const resourceFactory = new BackendSecretFetcherFactory(providerFactory);

beforeEach(() => {
BackendSecretFetcherFactory.clearRegisteredSecrets();
Copy link
Contributor

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.

});

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);
Expand All @@ -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);
Expand All @@ -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
Expand Up @@ -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`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const secretResourceId = `SecretFetcherResource`;
const secretResourceId = `AmplifySecretFetcherResource`;

just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated (AmplifySecretFetcherCustomResource)

Copy link
Contributor

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.

const existingResource = scope.node.tryFindChild(
secretResourceId
) as CustomResource;
Expand All @@ -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, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const customResourceEventCommon = {
ResourceType: 'AWS::CloudFormation::CustomResource',
ResourceProperties: {
...testBackendIdentifier,
secretName: testSecretName,
secretNames: [testSecretName],
ServiceToken: 'token',
},
OldResourceProperties: {},
Expand Down Expand Up @@ -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, [
Expand Down Expand Up @@ -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, [
Expand All @@ -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, [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}

Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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

{
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[];
};