Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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.
10 changes: 5 additions & 5 deletions packages/backend/src/engine/backend-secret/backend_secret.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ 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
) {}
/**
Expand All @@ -28,11 +28,11 @@ export class CfnTokenBackendSecret implements BackendSecret {
): SecretValue => {
const secretResource = this.secretResourceFactory.getOrCreate(
scope,
this.name,
this.secretName,
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 +43,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
Expand Up @@ -33,26 +33,18 @@ void describe('getOrCreate', () => {
resourceFactory.getOrCreate(stack, secretName2, 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,6 +59,8 @@ void describe('getOrCreate', () => {
void it('does not create duplicate resource for the same secret name', () => {
const app = new App();
const stack = new Stack(app);

// ensure only 1 resource is created even if this is called twice
resourceFactory.getOrCreate(stack, secretName1, backendId);
resourceFactory.getOrCreate(stack, secretName1, backendId);

Expand All @@ -78,6 +72,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
@@ -1,6 +1,6 @@
import { Construct } from 'constructs';
import { BackendSecretFetcherProviderFactory } from './backend_secret_fetcher_provider_factory.js';
import { CustomResource } from 'aws-cdk-lib';
import { CustomResource, CustomResourceProps, Lazy } from 'aws-cdk-lib';
import { BackendIdentifier } from '@aws-amplify/plugin-types';
import { SecretResourceProps } from './lambda/backend_secret_fetcher_types.js';

Expand All @@ -9,6 +9,25 @@ import { SecretResourceProps } from './lambda/backend_secret_fetcher_types.js';
*/
export const SECRET_RESOURCE_PROVIDER_ID = 'SecretFetcherResourceProvider';

class SecretFetcherCustomResource extends CustomResource {
private secrets: Set<string>;
constructor(
scope: Construct,
id: string,
props: CustomResourceProps,
secrets: Set<string>
) {
super(scope, id, {
...props,
});
this.secrets = secrets;
}

public addSecret = (secretName: string) => {
this.secrets.add(secretName);
};
}

/**
* Type of the backend custom CFN resource.
*/
Expand All @@ -33,15 +52,18 @@ export class BackendSecretFetcherFactory {
scope: Construct,
secretName: string,
backendIdentifier: BackendIdentifier
): CustomResource => {
const secretResourceId = `${secretName}SecretFetcherResource`;
): SecretFetcherCustomResource => {
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;
) as SecretFetcherCustomResource;

if (existingResource) {
existingResource.addSecret(secretName);
return existingResource;
}
const secrets: Set<string> = new Set();
secrets.add(secretName);

const provider = this.secretProviderFactory.getOrCreateInstance(
scope,
Expand All @@ -59,16 +81,25 @@ export class BackendSecretFetcherFactory {
namespace: backendIdentifier.namespace,
name: backendIdentifier.name,
type: backendIdentifier.type,
secretName: secretName,
secretNames: Lazy.list({
produce: () => {
return Array.from(secrets);
},
}),
};

return new CustomResource(scope, secretResourceId, {
serviceToken: provider.serviceToken,
properties: {
...customResourceProps,
secretLastUpdated, // this property is only to trigger resource update event.
return new SecretFetcherCustomResource(
scope,
secretResourceId,
{
serviceToken: provider.serviceToken,
properties: {
...customResourceProps,
secretLastUpdated, // this property is only to trigger resource update event.
},
resourceType: SECRET_RESOURCE_TYPE,
},
resourceType: SECRET_RESOURCE_TYPE,
});
secrets
);
};
}
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[];
};