fix(cli): add validation of --notification-arns structure #21270
Conversation
|
|
github-actions
bot
added
bug
| @@ -127,6 +128,14 @@ export class CdkToolkit { | |||
| return this.watch(options); | |||
| } | |||
|
|
|||
| if (options.notificationArns) { | |||
| options.notificationArns.map( arn => { | |||
| if (!validateSnsTopicArn(arn)) { | |||
There was a problem hiding this comment.
| if (!validateSnsTopicArn(arn)) { | |
| if (!Token.unresolved(arn) && !validateSnsTopicArn(arn)) { |
Also please add a test for this condition.
There was a problem hiding this comment.
@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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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?
merge main from aws/aws-cdk
mergify
bot
dismissed
otaviomacedo’s stale review
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
When deploying using the `--notification-arns` argument, it's possible to pass in an invalid arn as an SNS topic arn, causing a change set to be uploaded with an invalid arn. This PR adds a check for the structure of the input ARN. I was originally looking into possibly adding the check earlier in the process such as adding a .check() method to the yargs option in [packages/aws-cdk/lib/cli.ts](https://github.com/aws/aws-cdk/blob/f66f94e9201b9c9d5e0f1b713a6f30194b323b28/packages/aws-cdk/lib/cli.ts). But as I see it there are no tests for these, so it was difficult to validate the change. closes aws#20806 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When deploying using the `--notification-arns` argument, it's possible to pass in an invalid arn as an SNS topic arn, causing a change set to be uploaded with an invalid arn. This PR adds a check for the structure of the input ARN. I was originally looking into possibly adding the check earlier in the process such as adding a .check() method to the yargs option in [packages/aws-cdk/lib/cli.ts](https://github.com/aws/aws-cdk/blob/f66f94e9201b9c9d5e0f1b713a6f30194b323b28/packages/aws-cdk/lib/cli.ts). But as I see it there are no tests for these, so it was difficult to validate the change. closes aws#20806 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When deploying using the
--notification-arnsargument, it's possible to pass in an invalid arn as an SNS topic arn, causing a change set to be uploaded with an invalid arn. This PR adds a check for the structure of the input ARN.I was originally looking into possibly adding the check earlier in the process such as adding a .check() method to the yargs option in packages/aws-cdk/lib/cli.ts. But as I see it there are no tests for these, so it was difficult to validate the change.
closes #20806
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license