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): add validation of --notification-arns structure #21270

Merged
merged 6 commits into from
Jul 23, 2022
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
9 changes: 9 additions & 0 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { data, debug, error, highlight, print, success, warning } from './loggin
import { deserializeStructure, serializeStructure } from './serialize';
import { Configuration, PROJECT_CONFIG } from './settings';
import { numberFromBool, partition } from './util';
import { validateSnsTopicArn } from './util/validate-notification-arn';

export interface CdkToolkitProps {

Expand Down Expand Up @@ -127,6 +128,14 @@ export class CdkToolkit {
return this.watch(options);
}

if (options.notificationArns) {
options.notificationArns.map( arn => {
if (!validateSnsTopicArn(arn)) {
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
if (!validateSnsTopicArn(arn)) {
if (!Token.unresolved(arn) && !validateSnsTopicArn(arn)) {

Also please add a test for this condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@otaviomacedo I have looked into it and can indeed add the !Token.isUnresolved(arn) condition to the check of notification arn. However, as this check happens prior to stack synthesis, it seems redundant to check if the token has been resolved.
Let me know if I am misunderstanding :)

Copy link
Contributor

Choose a reason for hiding this comment

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

arn may be a "normal" string (in which case it makes sense to validate it) or it may a token, which is a special kind of string, that is a stand-in for something else (in which case it doesn't make sense to validate it because it will always fail). In the second case, Token isUnresolved() returns true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@otaviomacedo thanks for the reply! I do get that tokens can be passed around and are converted to strings during synthesis to CloudFormation templates.
What I am thinking is that the Arn in this specific case is passed as a parameter when using the CLI. for example:
cdk deploy --all --notification-arns arn:aws:sns:us-east-1::some-sns-topic
in which case the arn will never be a token as these only exist in a stack prior to synthesis.

Again, I might be wrong, but is there a case where the arn is passed to the CLI command as a token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@otaviomacedo Readng around I found #8581 which does ask to implement a way to add the notification arn to the stack prior to synthesis. The issue has been open for a while, but would require the Token.isUnresolved() in the check.

What do you think? should we add it just in case?

throw new Error(`Notification arn ${arn} is not a valid arn for an SNS topic`);
}
});
}

const startSynthTime = new Date().getTime();
const stacks = await this.selectStacksForDeploy(options.selector, options.exclusively, options.cacheCloudAssembly);
const elapsedSynthTime = new Date().getTime() - startSynthTime;
Expand Down
6 changes: 6 additions & 0 deletions packages/aws-cdk/lib/util/validate-notification-arn.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* Validate SNS topic arn
*/
export function validateSnsTopicArn(arn: string): boolean {
return /^arn:aws:sns:[a-z0-9\-]+:[0-9]+:[a-z0-9\-\_]+$/i.test(arn);
}
29 changes: 27 additions & 2 deletions packages/aws-cdk/test/cdk-toolkit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,10 @@ describe('deploy', () => {

test('with sns notification arns', async () => {
// GIVEN
const notificationArns = ['arn:aws:sns:::cfn-notifications', 'arn:aws:sns:::my-cool-topic'];
const notificationArns = [
'arn:aws:sns:us-east-2:444455556666:MyTopic',
'arn:aws:sns:eu-west-1:111155556666:my-great-topic',
];
const toolkit = new CdkToolkit({
cloudExecutable,
configuration: cloudExecutable.configuration,
Expand All @@ -475,8 +478,30 @@ describe('deploy', () => {
});
});

test('globless bootstrap uses environment without question', async () => {
test('fail with incorrect sns notification arns', async () => {
// GIVEN
const notificationArns = ['arn:::cfn-my-cool-topic'];
const toolkit = new CdkToolkit({
cloudExecutable,
configuration: cloudExecutable.configuration,
sdkProvider: cloudExecutable.sdkProvider,
cloudFormation: new FakeCloudFormation({
'Test-Stack-A': { Foo: 'Bar' },
}, notificationArns),
});

// WHEN
await expect(() =>
toolkit.deploy({
selector: { patterns: ['Test-Stack-A'] },
notificationArns,
}),
).rejects.toThrow('Notification arn arn:::cfn-my-cool-topic is not a valid arn for an SNS topic');

});

test('globless bootstrap uses environment without question', async () => {
// GIVEN
const toolkit = defaultToolkitSetup();

// WHEN
Expand Down
29 changes: 29 additions & 0 deletions packages/aws-cdk/test/util/validate-notification-arn.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { validateSnsTopicArn } from '../../lib/util/validate-notification-arn';

describe('validate sns arns', () => {
test('empty string', () => {
const arn = '';
expect(validateSnsTopicArn(arn)).toEqual(false);
});

test('colon in topic name', () => {
const arn = 'arn:aws:sns:eu-west-1:abc:foo';
expect(validateSnsTopicArn(arn)).toEqual(false);
});

test('missing :aws: in arn', () => {
const arn = 'arn:sns:eu-west-1:foobar';
expect(validateSnsTopicArn(arn)).toEqual(false);
});

test('dash in topic name', () => {
const arn = 'arn:aws:sns:eu-west-1:123456789876:foo-bar';
expect(validateSnsTopicArn(arn)).toEqual(true);
});

test('underscore in topic name', () => {
const arn = 'arn:aws:sns:eu-west-1:123456789876:foo-bar_baz';
expect(validateSnsTopicArn(arn)).toEqual(true);
});
});