Skip to content
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(cli): externally managed stack notification arns are deleted on deploy #32163

Merged
merged 7 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ if (process.env.PACKAGE_LAYOUT_VERSION === '1') {
aws_lambda: lambda,
aws_ecr_assets: docker,
aws_appsync: appsync,
aws_cloudformation: cfn,
Stack
} = require('aws-cdk-lib');
}
Expand Down Expand Up @@ -748,6 +749,16 @@ class NotificationArnPropStack extends cdk.Stack {
new sns.Topic(this, 'topic');
}
}

class EmptyStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);

new cdk.CfnWaitConditionHandle(this, 'WaitConditionHandle');

}
}

class AppSyncHotswapStack extends cdk.Stack {
constructor(parent, id, props) {
super(parent, id, props);
Expand Down Expand Up @@ -812,6 +823,7 @@ switch (stackSet) {
new MissingSSMParameterStack(app, `${stackPrefix}-missing-ssm-parameter`, { env: defaultEnv });

new LambdaStack(app, `${stackPrefix}-lambda`);
new EmptyStack(app, `${stackPrefix}-empty`);

// This stack is used to test diff with large templates by creating a role with a ton of metadata
new IamRolesStack(app, `${stackPrefix}-iam-roles`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
DescribeStacksCommand,
GetTemplateCommand,
ListChangeSetsCommand,
UpdateStackCommand,
} from '@aws-sdk/client-cloudformation';
import { DescribeServicesCommand } from '@aws-sdk/client-ecs';
import {
Expand Down Expand Up @@ -679,6 +680,44 @@ integTest('deploy with notification ARN as prop', withDefaultFixture(async (fixt
}
}));

// 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('empty');

// add notification arns externally to cdk
await fixture.aws.cloudFormation.send(
new UpdateStackCommand({
StackName: fixture.fullStackName('empty'),
UsePreviousTemplate: true,
NotificationARNs: [topicArn],
}),
);

// deploy again
await fixture.cdkDeploy('empty');

// make sure the notification arn is preserved
const describeResponse = await fixture.aws.cloudFormation.send(
new DescribeStacksCommand({
StackName: fixture.fullStackName('empty'),
}),
);
expect(describeResponse.Stacks?.[0].NotificationARNs).toEqual([topicArn]);
} finally {
await fixture.aws.sns.send(
new DeleteTopicCommand({
TopicArn: topicArn,
}),
);
}
}));

// 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(
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ export class Stack extends Construct implements ITaggable {
*
* @internal
*/
public readonly _notificationArns: string[];
public readonly _notificationArns?: string[];

/**
* Logical ID generation strategy
Expand Down Expand Up @@ -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}'`);
Expand Down
15 changes: 15 additions & 0 deletions packages/aws-cdk-lib/core/test/stack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
30 changes: 22 additions & 8 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -370,15 +370,29 @@ 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`);
// 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix.

? (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);
Expand Down
Loading