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

core: cdk deploy wiping stack notification ARNs property #32153

Closed
1 task done
wilhen01 opened this issue Nov 15, 2024 · 8 comments · Fixed by #32163
Closed
1 task done

core: cdk deploy wiping stack notification ARNs property #32153

wilhen01 opened this issue Nov 15, 2024 · 8 comments · Fixed by #32163
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/medium Medium work item – several days of effort p0 potential-regression Marking this issue as a potential regression to be checked by team member

Comments

@wilhen01
Copy link
Contributor

Describe the bug

A change was introduced in v2.160.0 which appears to be wiping the notificationARNs property on a CloudFormation stack when cdk deploy is run against an existing stack with that property set. This represents a regression on previous behaviour.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

2.159.1

Expected Behavior

cdk deploy does not modify the notificationARNs property on existing deployed stacks

Current Behavior

cdk deploy wipes the notificationARNs property where it is set on existing deployed stacks

Reproduction Steps

Deploy a stack. Modify the notificationARNs property outside of CDK. Then deploy a subsequent update via cdk deploy

Possible Solution

No response

Additional Information/Context

We have an internal tooling platform which stacks can be registered with in order to provide deployment updates. In order to do this, the tool sets the notificationARNs property on the stack to listen to stack updates via SNS. Previously updating these stacks via cdk deploy did not conflict with that, but it does in recent versions.

CDK CLI Version

v2.167.1

Framework Version

No response

Node.js Version

18

OS

Mac OS

Language

TypeScript

Language Version

5.x

Other information

No response

@wilhen01 wilhen01 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 15, 2024
@github-actions github-actions bot added @aws-cdk/core Related to core CDK functionality potential-regression Marking this issue as a potential regression to be checked by team member labels Nov 15, 2024
@pahud pahud self-assigned this Nov 15, 2024
@pahud
Copy link
Contributor

pahud commented Nov 15, 2024

Hi

Looks like the PR you mentioned essentially added a new notificationArns attribute which didn't exist prior to that PR.

Did you mean you actually had notificationArns attribute in 2.159.1 but it was wiped out in 2.160.0 ?

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 15, 2024
@pahud
Copy link
Contributor

pahud commented Nov 15, 2024

trying to create a minimal sample to verify it now.

@mrgrain
Copy link
Contributor

mrgrain commented Nov 15, 2024

If I read this correctly, the scenario is:

  1. deploy stack with v2.159.1, do not set notificationArns, do not use --notification-arns
  2. manually add Notification ARNs via the CFN console
  3. deploy stack with v2.160.0, still do not set notificationArns, still do not use --notification-arns
  4. see how the manually added Notification ARNs are removed

Or in other words, with v2.159.1 and before you could "tell" the CDK to just not care about Notification ARNs at all.
But since v2.160.0, the CDK got greedy and wants to always own Notification ARNs even if they are empty.


If this is confirmed, the code bug is probably the difference between not setting a parameter (old) and passing an empty array to the parameter (new).

@pahud
Copy link
Contributor

pahud commented Nov 15, 2024

HI @wilhen01

I am assuming your code

export class MyStack extends cdk.Stack {
  constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    // Cfnout notificationArns
    new cdk.CfnOutput(this, 'NotificationARNs', {
      value: cdk.Fn.select(0, this.notificationArns),
    });
  }
}

in 2.159.1

you probably deploy like this

npx cdk deploy --notification-arns arn:aws:sns:us-east-1:123456789012:NotifyMe

And you see this

dummy-stack2.NotificationARNs = arn:aws:sns:us-east-1:123456789012:NotifyMe

Now, when you upgrade to 2.160.0 without changing any code.

From my test if you continue deploy with that, you should still see that output. The behavior won't change.

Can you tell us how did you deploy that with the notificationArn in 2.159.1? From what I can see the --notification-arns should be the only way you can specify that in CDK 2.159.1.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 15, 2024
@pahud pahud removed their assignment Nov 15, 2024
@wilhen01
Copy link
Contributor Author

@mrgrain's summary is accurate @pahud - the stacks we're experiencing this problem with don't specify noticationArns at all. Basically the flow is:

  • Deploy stack via cdk deploy with no specification of NotificationARNs
  • Register stack with my organisation's internal tooling - this populates NotificationARNs in order to listen to stack updates
  • Deploy updates to stack via cdk deploy, again with no specification of NotificationARNs

prior to 2.160.0, the notification ARNs added by the internal tooling platform would be maintained across subsequent stack updates via cdk deploy. From 2.160.0 onwards, that is no longer the case.

@pahud
Copy link
Contributor

pahud commented Nov 15, 2024

Register stack with my organisation's internal tooling - this populates NotificationARNs in order to listen to stack updates

Sounds like this is beyond CDK? Is the internal tooling executing the AWS SDK call to update the stack with a notificationArn and also pull that ARN via SDK calls as well? If that's the case, what's happening in 2.160.0 is that if the props.notificationArn is undefined, it would be explicit set to [] per

this.notificationArns = properties.notificationArns ?? [];

I am not sure if it's necessary to set it to [] if undefined or should we just

this.notificationArns = properties.notificationArns;

I will bring this up to the team for further inputs.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 15, 2024
@pahud pahud added p0 and removed p2 labels Nov 15, 2024
@mergify mergify bot closed this as completed in #32163 Nov 18, 2024
@mergify mergify bot closed this as completed in 9966f57 Nov 18, 2024
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

1 similar comment
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2024
iliapolo added a commit that referenced this issue Nov 18, 2024
…deploy` (#32163)

Closes #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*
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/medium Medium work item – several days of effort p0 potential-regression Marking this issue as a potential regression to be checked by team member
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants