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

ec2: changes to enum values breaks deprecated enums for v1 #21131

Closed
c0state opened this issue Jul 13, 2022 · 4 comments · Fixed by #21140
Closed

ec2: changes to enum values breaks deprecated enums for v1 #21131

c0state opened this issue Jul 13, 2022 · 4 comments · Fixed by #21140
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug.

Comments

@c0state
Copy link

c0state commented Jul 13, 2022

Describe the bug

Starting with CDK version 1.162.0, if I previously had deployed a VPC with subnet of type ec2.SubnetType.PRIVATE (I understand that it's deprecated now), then simply updating to CDK 1.162.0 and running a deploy ends up causing the CDK to try to delete a bunch of NAT gateways, elastic IPs, and routes. If I change the type to ec2.SubnetType.PRIVATE_WITH_NAT, it doesn't do this.

I thought ec2.SubnetType.PRIVATE and ec2.SubnetType.PRIVATE_WITH_NAT were equivalent?

There were a bunch of changes to the consts to clarify private, public and/or isolated subnets recently. I'm assuming something there is causing this.

Expected Behavior

Updating the CDK to 1.162.0 without changes to our stack configuration shouldn't remove existing NAT gateways, elastic IPs or routes from a VPC

Current Behavior

Updating the CDK with a VPC that has a subnet with a type of PRIVATE causes the diff to show a deletion of various NAT gateways, elastic IPs and routes.

Reproduction Steps

Deploy a VPC with a subnet of type PRIVATE with CDK version v1.161.0.
Then simply update to v1.162.0 and run a diff--it will inexplicably try to delete various NAT gateways, elastic IPs and routes.

Possible Solution

It looks like changing the type to PRIVATE_WITH_NAT fixes this but as stated above, I thought PRIVATE and PRIVATE_WITH_NAT were the same.

Additional Information/Context

No response

CDK CLI Version

1.162.0

Framework Version

1.162.0

Node.js Version

v16.16.0

OS

macOS

Language

Typescript

Language Version

4.7.4

Other information

No response

@c0state c0state added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 13, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Jul 13, 2022
@peterwoodworth peterwoodworth added wontfix We have determined that we will not resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Jul 13, 2022
@peterwoodworth peterwoodworth removed the wontfix We have determined that we will not resolve the issue. label Jul 13, 2022
@peterwoodworth
Copy link
Contributor

I believe this is going to be just a v1 bug - which is in maintenance mode. CDK v2 only features the non deprecated version of each enum, and we've made changes recently to make sure this works and is not confusing in all languages.

We've renamed the value of each deprecated enum to have a prefix of Deprecated before 1.162.0 - I believe this is what would be causing the issue here. This was done for the reason described above for full support in all languages

Is there anything preventing you from changing values or migrating to v2?

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 13, 2022
@peterwoodworth peterwoodworth changed the title vpc: recent PRIVATE_WITH_NAT changes causing unexpected NAT gateway/elastic IP/route modifications ec2: changes to enum values breaks deprecated enums for v1 Jul 13, 2022
@c0state
Copy link
Author

c0state commented Jul 14, 2022

Is there anything preventing you from changing values or migrating to v2?

No good reason, just wondering. This also happens on v2 though. In addition, ec2.SubnetType.PRIVATE is still available on v2, at least with the TypeScript library.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 14, 2022
@corymhall corymhall changed the title ec2: changes to enum values breaks deprecated enums for v1 ‼️ NOTICE: ec2: changes to enum values breaks deprecated enums for v1 Jul 14, 2022
@corymhall corymhall added management/tracking Issues that track a subject or multiple issues p0 labels Jul 14, 2022
@corymhall corymhall changed the title ‼️ NOTICE: ec2: changes to enum values breaks deprecated enums for v1 ec2: changes to enum values breaks deprecated enums for v1 Jul 14, 2022
@corymhall corymhall removed management/tracking Issues that track a subject or multiple issues p0 labels Jul 14, 2022
@corymhall
Copy link
Contributor

Created a new pinned issue to track the p0 #21138

@mergify mergify bot closed this as completed in #21140 Jul 14, 2022
mergify bot pushed a commit that referenced this issue Jul 14, 2022
In #19320, we changed the values of deprecated enums to include a `Deprecated_` prefix. This also meant we had to do some gymnastics in the code to change usages of the value to a switch case. 

However, there were also places in the code that do enum comparison, which uses the enum value. Those comparison points need to also explicitly consider the equivalent deprecated enums.  

Fixes #21131, Fixes #21138 

----

### All Submissions:

* [ ] 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*
mergify bot pushed a commit that referenced this issue Jul 14, 2022
In #19320, we changed the values of deprecated enums to include a `Deprecated_` prefix. This also meant we had to do some gymnastics in the code to change usages of the value to a switch case.

However, there were also places in the code that do enum comparison, which uses the enum value. Those comparison points need to also explicitly consider the equivalent deprecated enums.

Fixes #21131, Fixes #21138

----

### All Submissions:

* [ ] 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*

(cherry picked from commit 0b5123a)
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants