-
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
fix(ec2): AmazonLinuxImage
construct generates incorrect SSM parameter name for AL2023 images
#27698
fix(ec2): AmazonLinuxImage
construct generates incorrect SSM parameter name for AL2023 images
#27698
Conversation
…r name for AL2023 images
AmazonLinuxImage
construct generates incorrect SSM parameter name for AL2023 images
Signed-off-by: Vinayak Kukreja <vinakuk@amazon.com>
Hey, thank you for your contribution. I made some updates to the integ test to get it working. Since I have added code, I have asked someone else to add a review too. But it looks good to me. |
test('throw error if storage param is set for Amazon Linux 2023', () => { | ||
expect(() => { | ||
new ec2.AmazonLinuxImage({ | ||
generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2023, | ||
storage: ec2.AmazonLinuxStorage.GENERAL_PURPOSE, | ||
}).getImage(stack); | ||
}).toThrow(/Storage parameter does not exist in SSM parameter name for Amazon Linux 2023./); | ||
}); | ||
|
||
test('throw error if virtualization param is set for Amazon Linux 2023', () => { | ||
expect(() => { | ||
new ec2.AmazonLinuxImage({ | ||
generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2023, | ||
virtualization: ec2.AmazonLinuxVirt.HVM, | ||
}).getImage(stack); | ||
}).toThrow(/Virtualization parameter does not exist in SSM parameter name for Amazon Linux 2023./); |
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.
These tests are outside of the describe block. Is that intended? It looks like theres a mix of 2022 and 2023 tests in the latest describe block. Is that intended too?
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.
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.
@scanlonp Thank you for your comment!
These tests are outside of the describe block. Is that intended?
Yes, this is intentional. I added three tests.
- 'AmazonLinuxImage with AMAZON_LINUX_2023'
- 'throw error if storage param is set for Amazon Linux 2023'
- 'throw error if virtualization param is set for Amazon Linux 2023'
1 is located in latest
describe block. This is because I want to verify whether AmazonLinuxImage construct generates SSM parameter path for the latest AL2023 image by this change. 2 and 3 are not located in the describe block. This is also intentional because these tests are similar with the following tests located outside of the describe block.
aws-cdk/packages/aws-cdk-lib/aws-ec2/test/machine-image.test.ts
Lines 257 to 275 in 820bb99
test('throw error if storage param is set for Amazon Linux 2022', () => { | |
expect(() => { | |
ec2.MachineImage.latestAmazonLinux({ | |
cachedInContext: true, | |
generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2022, | |
storage: ec2.AmazonLinuxStorage.GENERAL_PURPOSE, | |
}).getImage(stack).imageId; | |
}).toThrow(/Storage parameter does not exist in smm parameter name for Amazon Linux 2022./); | |
}); | |
test('throw error if virtualization param is set for Amazon Linux 2022', () => { | |
expect(() => { | |
ec2.MachineImage.latestAmazonLinux({ | |
cachedInContext: true, | |
generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX_2022, | |
virtualization: ec2.AmazonLinuxVirt.HVM, | |
}).getImage(stack).imageId; | |
}).toThrow(/Virtualization parameter does not exist in smm parameter name for Amazon Linux 2022./); | |
}); |
If I should move 2 and 3 into directly below the above tests, please let me know.
It looks like theres a mix of 2022 and 2023 tests in the latest describe block. Is that intended too?
This is not my intention. Before I added these tests, tests for both 2022 and 2023 are located in the describe block as below.
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-ec2/test/machine-image.test.ts#L300-L407
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.
Thanks for your explanation. I think the formatting of the tests is fine if it was your intention. We can reorganize as necessary in the future, but it is likely ok, and certainly should not block this PR.
Pull request has been modified.
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.
See above.
@Mergifyio update |
✅ Branch has been successfully updated |
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). |
…ter name for AL2023 images (#27698) AmazonLinuxImage construct generates SSM parameter name for Amazon Linux images. The naming convention for Amazon Linux 2023 images is a bit different from Amazon Linux 2. For example, virtualization type (e.g. HVM) or backend storage type (e.g. GP2) are not included in parameter's name for AL2023. AL2: https://github.com/aws/aws-cdk/blob/d0d75478e1cf3bb9a06f33642b9a06fc68d0c99d/packages/aws-cdk-lib/aws-ec2/lib/machine-image/amazon-linux2.ts#L77-L84 AL2023: https://github.com/aws/aws-cdk/blob/d0d75478e1cf3bb9a06f33642b9a06fc68d0c99d/packages/aws-cdk-lib/aws-ec2/lib/machine-image/amazon-linux-2023.ts#L59-L66 Currently, AmazonLinuxImage construct generates incorrect SSM parameter name for AL2023 images, which includes virtualization and storage type in the name. This causes validation error against non-existing parameter name. This PR solves the issue by avoiding to include virtualization and storage in parameter's name when AMAZON_LINUX_2023 is specified as generation. Closes #27638 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AmazonLinuxImage construct generates SSM parameter name for Amazon Linux images. The naming convention for Amazon Linux 2023 images is a bit different from Amazon Linux 2. For example, virtualization type (e.g. HVM) or backend storage type (e.g. GP2) are not included in parameter's name for AL2023.
AL2:
aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/machine-image/amazon-linux2.ts
Lines 77 to 84 in d0d7547
AL2023:
aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/machine-image/amazon-linux-2023.ts
Lines 59 to 66 in d0d7547
Currently, AmazonLinuxImage construct generates incorrect SSM parameter name for AL2023 images, which includes virtualization and storage type in the name. This causes validation error against non-existing parameter name. This PR solves the issue by avoiding to include virtualization and storage in parameter's name when AMAZON_LINUX_2023 is specified as generation.
Closes #27638
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license