-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2e02dbf
mid work
iliapolo 293e4d6
mid work
iliapolo c940d55
mid work
iliapolo 8a082de
mid work
iliapolo cc2c4b1
allow breaking change
iliapolo c65ccc3
Merge branch 'main' into epolon/preserver-notification-arns
iliapolo b63d5c0
wait for stack update complete
iliapolo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { | |
DescribeStacksCommand, | ||
GetTemplateCommand, | ||
ListChangeSetsCommand, | ||
UpdateStackCommand, | ||
} from '@aws-sdk/client-cloudformation'; | ||
import { DescribeServicesCommand } from '@aws-sdk/client-ecs'; | ||
import { | ||
|
@@ -633,14 +634,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 +662,55 @@ 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], | ||
}), | ||
); | ||
|
||
// 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 +723,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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is essentially testing SDK behavior but what the heck |
||
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( | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Moved this to inside the stack, which now reads it from an env variable to avoid hard coding stuff.