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(ec2): launch template names in imdsv2 not unique across stacks (under feature flag) #17766

Merged
merged 6 commits into from
Jan 18, 2022

Conversation

jericht
Copy link
Contributor

@jericht jericht commented Nov 30, 2021

Fixes #17656

Notes

Changes the name for the LaunchTemplate created in the aspect that enforces IMDSv2 on EC2 instances to a unique name.

Introduces a new feature flag (@aws-cdk/aws-ec2:uniqueImdsv2TemplateName) to change the launch template name.

Testing

Added a unit test


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Nov 30, 2021

@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Nov 30, 2021
@jericht jericht force-pushed the jericht/fix_imdsv2_ec2 branch 2 times, most recently from ea45e50 to f12f10a Compare November 30, 2021 23:19
@jericht jericht marked this pull request as ready for review December 1, 2021 16:46
@jericht
Copy link
Contributor Author

jericht commented Dec 1, 2021

@njlynch This is ready for review now. Could you take a look when you get the chance?

njlynch
njlynch previously requested changes Dec 7, 2021
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

However, I'm not sure this approach will work from a backwards-compatibility perspective. It appears from the docs that changing the launchTemplateName on the LaunchTemplate requires replacement, as does altering the launchTemplateName on the Instance.

The above means that accepting this change would cause instance replacements for any user who's using this feature, potentially causing unwanted and/or unexpected data loss.

An alternative approach is required, preferably by adding some mechanism (e.g., new property) for users to explicitly opt into this new naming scheme. Suggestions welcome!

@mergify mergify bot dismissed njlynch’s stale review December 8, 2021 23:26

Pull request has been modified.

@jericht jericht force-pushed the jericht/fix_imdsv2_ec2 branch 2 times, most recently from 4804d0b to d62fce2 Compare December 9, 2021 01:04
@jericht
Copy link
Contributor Author

jericht commented Dec 9, 2021

@njlynch I've updated the PR to have an explicit opt-in property to use the fixed naming scheme, so this is ready for another review.

I decided to make it very clear (in docstrings and the README) that users should almost always have this property set to true to avoid the name collision issues, since this should've been the default behavior in the first place. Let me know what you think.

@jericht jericht requested a review from njlynch December 9, 2021 01:13
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Looking at how the implementation turned out, I don't think this is optimal. Deprecating the requireImdsv2 property for the options property isn't great, and the naming of uniqueLaunchTemplateNames is awkward (but I can't think of much better names with this design).

Let's flip this around a bit -- I think this is probably a good place to introduce a feature flag, so we can adjust the default behavior without uglifying the API. :) Maybe something like @aws-cdk/aws-ec2:uniqueImdsv2TemplateName. Take a look at the above contributing guide on flags, and let me know if you have any questions on this approach.

@mergify mergify bot dismissed njlynch’s stale review January 12, 2022 23:17

Pull request has been modified.

@jericht jericht changed the title fix(ec2): use unique launch template name in imdsv2 aspect for instances fix(ec2): use unique launch template name in imdsv2 aspect for instances (under feature flag) Jan 12, 2022
@jericht jericht force-pushed the jericht/fix_imdsv2_ec2 branch 3 times, most recently from 9a49e29 to c4ef025 Compare January 12, 2022 23:27
@jericht
Copy link
Contributor Author

jericht commented Jan 13, 2022

@njlynch Thanks for pointing me to the feature flag approach, it's much cleaner! I've updated the CR, so it's ready for another review.

@jericht jericht requested a review from njlynch January 13, 2022 16:32
@njlynch njlynch changed the title fix(ec2): use unique launch template name in imdsv2 aspect for instances (under feature flag) fix(ec2): launch template names in imdsv2 not unique across stacks (under feature flag) Jan 18, 2022
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Looks great! I agree - much cleaner.

I made a minor update to use some of our newer best-practice helpers for feature flags. I'll take an action to document them in our contributing guide as well.

Thanks!

@mergify
Copy link
Contributor

mergify bot commented Jan 18, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 2a80e4b into aws:master Jan 18, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 18, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 56c01ae
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…nder feature flag) (aws#17766)

Fixes aws#17656

### Notes
Changes the name for the `LaunchTemplate` created in the aspect that enforces IMDSv2 on EC2 instances to a unique name.

Introduces a new feature flag (`@aws-cdk/aws-ec2:uniqueImdsv2TemplateName`) to change the launch template name.

### Testing
Added a unit test

----

*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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(@aws-ec2): Using require_imdsv2=True creates non-unique launch templates
3 participants