-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(ec2): add aspect to require imdsv2 #16051
Conversation
4e39bd4
to
d95bbae
Compare
d95bbae
to
75c9920
Compare
@njlynch I've rebased these changes and they are ready for review. Would you be able to take a look? |
75c9920
to
13c311c
Compare
924c117
to
ebfd5f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the code duplication, but I would appreciate naming consistency. Let's name it requireImdsv2
here as well.
Partially fixes: #5137 Related PR: #16051 **Note:** I have some concerns about duplicated code between this and the above linked PR. Please see that PR for more details. ### Changes Adds an aspect that can enable/disable IMDSv1 on AutoScalingGroups ### Testing Added unit tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
13c311c
to
6d95697
Compare
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 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Partially fixes: aws#5137 Related PR: aws#16051 **Note:** I have some concerns about duplicated code between this and the above linked PR. Please see that PR for more details. ### Changes Adds an aspect that can enable/disable IMDSv1 on AutoScalingGroups ### Testing Added unit tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Partially fixes: aws#5137 Related PR: aws#16052 **Note:** This PR and the above related PR have common code that has been duplicated across these two PRs because I decided it made more sense for these Aspects to be in the same package with the constructs they work with. However, it means I had to duplicate some of the base class code across the two PRs. Looking for an opinion on what's better here: - Should we keep it as is (2 PRs) so these Aspects are cleanly separated? or, - Does it make sense to either combine them in some way (e.g. a new package `@aws-cdk/aspects`) or have one reference the other (maybe the AutoScalingGroup aspect can reference the code in this PR since it already depends on this package). ### Changes Adds an aspect that can enable/disable IMDSv1 on Instances and Launch Templates. ### Testing Added unit tests ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Partially fixes: #5137
Related PR: #16052
Note: This PR and the above related PR have common code that has been duplicated across these two PRs because I decided it made more sense for these Aspects to be in the same package with the constructs they work with. However, it means I had to duplicate some of the base class code across the two PRs. Looking for an opinion on what's better here:
@aws-cdk/aspects
) or have one reference the other (maybe the AutoScalingGroup aspect can reference the code in this PR since it already depends on this package).Changes
Adds an aspect that can enable/disable IMDSv1 on Instances and Launch Templates.
Testing
Added unit tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license