From 9966f57f079029265e560514954a6658780be04a Mon Sep 17 00:00:00 2001 From: Eli Polonsky Date: Mon, 18 Nov 2024 15:57:37 +0200 Subject: [PATCH] fix(cli): externally managed stack notification arns are deleted on `deploy` (#32163) Closes https://github.com/aws/aws-cdk/issues/32153. ### Reason for this change When a stack contains externally managed notification ARNs (i.e ones that were added outside of CDK), a `cdk deploy` command will remove those ARNs. ### Description of changes We incorrectly default notification ARNs to `[]` instead of `undefined`. When an empty array is passed to the SDK , it reasonably assumes you want to delete existing ARNs (because how otherwise would you delete them). If on the other hand you don't pass notification ARNs at all to the SDK (e.g `undefined`), it will preserve them. This is the correct behavior and CDK should follow suite. This does however create a (maybe) quirky API ergonomic where in order to remove notification ARNs, one must pass `[]` instead of simply omitting the property. This stems from the fact notification ARNs are not managed through the template, but rather through imperative actions. So it seems reasonable al things considered. ### Description of how you validated changes Added both unit and integration tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- allowed-breaking-changes.txt | 8 +- .../cli-integ/resources/cdk-apps/app/app.js | 21 ++- .../tests/cli-integ-tests/cli.integtest.ts | 142 +++++++++++++++++- packages/aws-cdk-lib/core/README.md | 11 +- packages/aws-cdk-lib/core/lib/stack.ts | 4 +- packages/aws-cdk-lib/core/test/stack.test.ts | 15 ++ .../lib/artifacts/cloudformation-artifact.ts | 4 +- packages/aws-cdk/lib/cdk-toolkit.ts | 21 ++- packages/aws-cdk/lib/config.ts | 2 +- .../lib/parse-command-line-arguments.ts | 2 +- packages/aws-cdk/test/cdk-toolkit.test.ts | 2 +- 11 files changed, 205 insertions(+), 27 deletions(-) diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index 99d48726abbee..3e514afeee38b 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -936,4 +936,10 @@ removed:aws-cdk-lib.aws_ec2.WindowsVersion.WINDOWS_SERVER_2022_TURKISH_FULL_BASE # null() return a [] which is invalid filter rule. It should return [null]. # Hence the return type changes from string[] to any -change-return-type:aws-cdk-lib.aws_lambda.FilterRule.null \ No newline at end of file +change-return-type:aws-cdk-lib.aws_lambda.FilterRule.null + + +# output property was mistakenly marked as required even though it should have allowed +# for undefined, i.e optional +changed-type:@aws-cdk/cx-api.CloudFormationStackArtifact.notificationArns +changed-type:aws-cdk-lib.cx_api.CloudFormationStackArtifact.notificationArns \ No newline at end of file diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js index 54eb8842eeee8..61e58871ed2c1 100755 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js @@ -742,12 +742,23 @@ class BuiltinLambdaStack extends cdk.Stack { } } -class NotificationArnPropStack extends cdk.Stack { +class NotificationArnsStack extends cdk.Stack { constructor(parent, id, props) { - super(parent, id, props); - new sns.Topic(this, 'topic'); + + const arnsFromEnv = process.env.INTEG_NOTIFICATION_ARNS; + super(parent, id, { + ...props, + // comma separated list of arns. + // empty string means empty list. + // undefined means undefined + notificationArns: arnsFromEnv == '' ? [] : (arnsFromEnv ? arnsFromEnv.split(',') : undefined) + }); + + new cdk.CfnWaitConditionHandle(this, 'WaitConditionHandle'); + } } + class AppSyncHotswapStack extends cdk.Stack { constructor(parent, id, props) { super(parent, id, props); @@ -839,9 +850,7 @@ switch (stackSet) { new DockerInUseStack(app, `${stackPrefix}-docker-in-use`); new DockerStackWithCustomFile(app, `${stackPrefix}-docker-with-custom-file`); - new NotificationArnPropStack(app, `${stackPrefix}-notification-arn-prop`, { - notificationArns: [`arn:aws:sns:${defaultEnv.region}:${defaultEnv.account}:${stackPrefix}-test-topic-prop`], - }); + new NotificationArnsStack(app, `${stackPrefix}-notification-arns`); // SSO stacks new SsoInstanceAccessControlConfig(app, `${stackPrefix}-sso-access-control`); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts index 82f18672cfac2..8c73c74bb84f3 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts @@ -7,6 +7,8 @@ import { DescribeStacksCommand, GetTemplateCommand, ListChangeSetsCommand, + UpdateStackCommand, + waitUntilStackUpdateComplete, } from '@aws-sdk/client-cloudformation'; import { DescribeServicesCommand } from '@aws-sdk/client-ecs'; import { @@ -633,14 +635,14 @@ integTest( const topicArn = response.TopicArn!; try { - await fixture.cdkDeploy('test-2', { + await fixture.cdkDeploy('notification-arns', { options: ['--notification-arns', topicArn], }); // verify that the stack we deployed has our notification ARN const describeResponse = await fixture.aws.cloudFormation.send( new DescribeStacksCommand({ - StackName: fixture.fullStackName('test-2'), + StackName: fixture.fullStackName('notification-arns'), }), ); expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topicArn]); @@ -661,12 +663,63 @@ integTest('deploy with notification ARN as prop', withDefaultFixture(async (fixt const topicArn = response.TopicArn!; try { - await fixture.cdkDeploy('notification-arn-prop'); + await fixture.cdkDeploy('notification-arns', { + modEnv: { + INTEG_NOTIFICATION_ARNS: topicArn, + + }, + }); // verify that the stack we deployed has our notification ARN const describeResponse = await fixture.aws.cloudFormation.send( new DescribeStacksCommand({ - StackName: fixture.fullStackName('notification-arn-prop'), + StackName: fixture.fullStackName('notification-arns'), + }), + ); + expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topicArn]); + } finally { + await fixture.aws.sns.send( + new DeleteTopicCommand({ + TopicArn: topicArn, + }), + ); + } +})); + +// https://github.com/aws/aws-cdk/issues/32153 +integTest('deploy preserves existing notification arns when not specified', withDefaultFixture(async (fixture) => { + const topicName = `${fixture.stackNamePrefix}-topic`; + + const response = await fixture.aws.sns.send(new CreateTopicCommand({ Name: topicName })); + const topicArn = response.TopicArn!; + + try { + await fixture.cdkDeploy('notification-arns'); + + // add notification arns externally to cdk + await fixture.aws.cloudFormation.send( + new UpdateStackCommand({ + StackName: fixture.fullStackName('notification-arns'), + UsePreviousTemplate: true, + NotificationARNs: [topicArn], + }), + ); + + await waitUntilStackUpdateComplete( + { + client: fixture.aws.cloudFormation, + maxWaitTime: 600, + }, + { StackName: fixture.fullStackName('notification-arns') }, + ); + + // deploy again + await fixture.cdkDeploy('notification-arns'); + + // make sure the notification arn is preserved + const describeResponse = await fixture.aws.cloudFormation.send( + new DescribeStacksCommand({ + StackName: fixture.fullStackName('notification-arns'), }), ); expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topicArn]); @@ -679,6 +732,87 @@ integTest('deploy with notification ARN as prop', withDefaultFixture(async (fixt } })); +integTest('deploy deletes ALL notification arns when empty array is passed', withDefaultFixture(async (fixture) => { + const topicName = `${fixture.stackNamePrefix}-topic`; + + const response = await fixture.aws.sns.send(new CreateTopicCommand({ Name: topicName })); + const topicArn = response.TopicArn!; + + try { + await fixture.cdkDeploy('notification-arns', { + modEnv: { + INTEG_NOTIFICATION_ARNS: topicArn, + }, + }); + + // make sure the arn was added + let describeResponse = await fixture.aws.cloudFormation.send( + new DescribeStacksCommand({ + StackName: fixture.fullStackName('notification-arns'), + }), + ); + expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topicArn]); + + // deploy again with empty array + await fixture.cdkDeploy('notification-arns', { + modEnv: { + INTEG_NOTIFICATION_ARNS: '', + }, + }); + + // make sure the arn was deleted + describeResponse = await fixture.aws.cloudFormation.send( + new DescribeStacksCommand({ + StackName: fixture.fullStackName('notification-arns'), + }), + ); + expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([]); + } finally { + await fixture.aws.sns.send( + new DeleteTopicCommand({ + TopicArn: topicArn, + }), + ); + } +})); + +integTest('deploy with notification ARN as prop and flag', withDefaultFixture(async (fixture) => { + const topic1Name = `${fixture.stackNamePrefix}-topic1`; + const topic2Name = `${fixture.stackNamePrefix}-topic1`; + + const topic1Arn = (await fixture.aws.sns.send(new CreateTopicCommand({ Name: topic1Name }))).TopicArn!; + const topic2Arn = (await fixture.aws.sns.send(new CreateTopicCommand({ Name: topic2Name }))).TopicArn!; + + try { + await fixture.cdkDeploy('notification-arns', { + modEnv: { + INTEG_NOTIFICATION_ARNS: topic1Arn, + + }, + options: ['--notification-arns', topic2Arn], + }); + + // verify that the stack we deployed has our notification ARN + const describeResponse = await fixture.aws.cloudFormation.send( + new DescribeStacksCommand({ + StackName: fixture.fullStackName('notification-arns'), + }), + ); + expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topic1Arn, topic2Arn]); + } finally { + await fixture.aws.sns.send( + new DeleteTopicCommand({ + TopicArn: topic1Arn, + }), + ); + await fixture.aws.sns.send( + new DeleteTopicCommand({ + TopicArn: topic2Arn, + }), + ); + } +})); + // NOTE: this doesn't currently work with modern-style synthesis, as the bootstrap // role by default will not have permission to iam:PassRole the created role. integTest( diff --git a/packages/aws-cdk-lib/core/README.md b/packages/aws-cdk-lib/core/README.md index a6402770d6c66..ecec3322f5351 100644 --- a/packages/aws-cdk-lib/core/README.md +++ b/packages/aws-cdk-lib/core/README.md @@ -1332,7 +1332,16 @@ const stack = new Stack(app, 'StackName', { }); ``` -Stack events will be sent to any SNS Topics in this list. +Stack events will be sent to any SNS Topics in this list. These ARNs are added to those specified using +the `--notification-arns` command line option. + +Note that in order to do delete notification ARNs entirely, you must pass an empty array ([]) instead of omitting it. +If you omit the property, no action on existing ARNs will take place. + +> [!NOTICE] +> Adding the `notificationArns` property (or using the `--notification-arns` CLI options) will **override** +> any existing ARNs configured on the stack. If you have an external system managing notification ARNs, +> either migrate to use this mechanism, or avoid specfying notification ARNs with the CDK. ### CfnJson diff --git a/packages/aws-cdk-lib/core/lib/stack.ts b/packages/aws-cdk-lib/core/lib/stack.ts index 0ccb560636718..e3b21ea57de28 100644 --- a/packages/aws-cdk-lib/core/lib/stack.ts +++ b/packages/aws-cdk-lib/core/lib/stack.ts @@ -376,7 +376,7 @@ export class Stack extends Construct implements ITaggable { * * @internal */ - public readonly _notificationArns: string[]; + public readonly _notificationArns?: string[]; /** * Logical ID generation strategy @@ -471,7 +471,7 @@ export class Stack extends Construct implements ITaggable { } } - this._notificationArns = props.notificationArns ?? []; + this._notificationArns = props.notificationArns; if (!VALID_STACK_NAME_REGEX.test(this.stackName)) { throw new Error(`Stack name must match the regular expression: ${VALID_STACK_NAME_REGEX.toString()}, got '${this.stackName}'`); diff --git a/packages/aws-cdk-lib/core/test/stack.test.ts b/packages/aws-cdk-lib/core/test/stack.test.ts index 399e355063dfb..6fad4e4922045 100644 --- a/packages/aws-cdk-lib/core/test/stack.test.ts +++ b/packages/aws-cdk-lib/core/test/stack.test.ts @@ -2097,6 +2097,21 @@ describe('stack', () => { ]); }); + test('stack notification arns defaults to undefined', () => { + + const app = new App({ stackTraces: false }); + const stack1 = new Stack(app, 'stack1', {}); + + const asm = app.synth(); + + // It must be undefined and not [] because: + // + // - undefined => cdk ignores it entirely, as if it wasn't supported (allows external management). + // - []: => cdk manages it, and the user wants to wipe it out. + // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. + expect(asm.getStackArtifact(stack1.artifactId).notificationArns).toBeUndefined(); + }); + test('stack notification arns are reflected in the stack artifact properties', () => { // GIVEN const NOTIFICATION_ARNS = ['arn:aws:sns:bermuda-triangle-1337:123456789012:MyTopic']; diff --git a/packages/aws-cdk-lib/cx-api/lib/artifacts/cloudformation-artifact.ts b/packages/aws-cdk-lib/cx-api/lib/artifacts/cloudformation-artifact.ts index 8fcea525b4907..3009129056e93 100644 --- a/packages/aws-cdk-lib/cx-api/lib/artifacts/cloudformation-artifact.ts +++ b/packages/aws-cdk-lib/cx-api/lib/artifacts/cloudformation-artifact.ts @@ -56,7 +56,7 @@ export class CloudFormationStackArtifact extends CloudArtifact { /** * SNS Topics that will receive stack events. */ - public readonly notificationArns: string[]; + public readonly notificationArns?: string[]; /** * The physical name of this stack. @@ -174,7 +174,7 @@ export class CloudFormationStackArtifact extends CloudArtifact { // We get the tags from 'properties' if available (cloud assembly format >= 6.0.0), otherwise // from the stack metadata this.tags = properties.tags ?? this.tagsFromMetadata(); - this.notificationArns = properties.notificationArns ?? []; + this.notificationArns = properties.notificationArns; this.assumeRoleArn = properties.assumeRoleArn; this.assumeRoleExternalId = properties.assumeRoleExternalId; this.assumeRoleAdditionalOptions = properties.assumeRoleAdditionalOptions; diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 8ae254f6813ed..dc8b6cf34aca1 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -370,15 +370,20 @@ export class CdkToolkit { } } - let notificationArns: string[] = []; - notificationArns = notificationArns.concat(options.notificationArns ?? []); - notificationArns = notificationArns.concat(stack.notificationArns); - - notificationArns.map((arn) => { - if (!validateSnsTopicArn(arn)) { - throw new Error(`Notification arn ${arn} is not a valid arn for an SNS topic`); + // Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK) + // + // - undefined => cdk ignores it, as if it wasn't supported (allows external management). + // - []: => cdk manages it, and the user wants to wipe it out. + // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. + const notificationArns = (!!options.notificationArns || !!stack.notificationArns) + ? (options.notificationArns ?? []).concat(stack.notificationArns ?? []) + : undefined; + + for (const notificationArn of notificationArns ?? []) { + if (!validateSnsTopicArn(notificationArn)) { + throw new Error(`Notification arn ${notificationArn} is not a valid arn for an SNS topic`); } - }); + } const stackIndex = stacks.indexOf(stack) + 1; print('%s: deploying... [%s/%s]', chalk.bold(stack.displayName), stackIndex, stackCollection.stackCount); diff --git a/packages/aws-cdk/lib/config.ts b/packages/aws-cdk/lib/config.ts index d8f490dac67dc..349faf1126384 100644 --- a/packages/aws-cdk/lib/config.ts +++ b/packages/aws-cdk/lib/config.ts @@ -112,7 +112,7 @@ export function makeConfig(): CliConfig { 'build-exclude': { type: 'array', alias: 'E', nargs: 1, desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', default: [] }, 'exclusively': { type: 'boolean', alias: 'e', desc: 'Only deploy requested stacks, don\'t include dependencies' }, 'require-approval': { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'What security-sensitive changes need manual approval' }, - 'notification-arns': { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', nargs: 1, requiresArg: true }, + 'notification-arns': { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events. These will be added to ARNs specified with the \'notificationArns\' stack property.', nargs: 1, requiresArg: true }, // @deprecated(v2) -- tags are part of the Cloud Assembly and tags specified here will be overwritten on the next deployment 'tags': { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly (deprecated)', nargs: 1, requiresArg: true }, 'execute': { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet) (deprecated)', deprecated: true }, diff --git a/packages/aws-cdk/lib/parse-command-line-arguments.ts b/packages/aws-cdk/lib/parse-command-line-arguments.ts index 3f7ebd16aa4d8..a8d637cd28a98 100644 --- a/packages/aws-cdk/lib/parse-command-line-arguments.ts +++ b/packages/aws-cdk/lib/parse-command-line-arguments.ts @@ -355,7 +355,7 @@ export function parseCommandLineArguments( }) .option('notification-arns', { type: 'array', - desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', + desc: "ARNs of SNS topics that CloudFormation will notify with stack related events. These will be added to ARNs specified with the 'notificationArns' stack property.", nargs: 1, requiresArg: true, }) diff --git a/packages/aws-cdk/test/cdk-toolkit.test.ts b/packages/aws-cdk/test/cdk-toolkit.test.ts index cfc9aca1bc398..241e5987014f1 100644 --- a/packages/aws-cdk/test/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cdk-toolkit.test.ts @@ -1658,7 +1658,7 @@ class FakeCloudFormation extends Deployments { .map(([Key, Value]) => ({ Key, Value })) .sort((l, r) => l.Key.localeCompare(r.Key)); } - this.expectedNotificationArns = expectedNotificationArns ?? []; + this.expectedNotificationArns = expectedNotificationArns; } public deployStack(options: DeployStackOptions): Promise {