fix(ec2): `WindowsVersion` enum by nmussy · Pull Request #29738 · aws/aws-cdk

nmussy · 2024-04-05T09:22:21Z

Issue # (if applicable)

Reason for this change

Oversight on my part when #29435 was created. I'd assumed that Windows versions and their datestamped counterparts (e.g. Windows_Server-2022-English-Full-Base and Windows_Server-2022-English-Full-Base-2024.02.14) could be retrieved in the same manner, and that the non-datestamped versions had just been removed since they were not being listed by ec2:DescribeImages.

The non datestamped versions are actually SSM parameter name suffixes used to retrieve the latest AMI image ID for a given region:

$ aws ssm get-parameters --names /aws/service/ami-windows-latest/Windows_Server-2022-English-Full-Base | jq '.Parameters[] | .Value'
"ami-03cd80cfebcbb4481"
$  aws ec2 describe-images --image-ids ami-03cd80cfebcbb4481 | jq '.Images[] | .Name'
"Windows_Server-2022-English-Full-Base-2024.03.13"

Description of changes

Split WindowsVersion into a second enum, WindowsSpecificVersion
- Each WindowsSpecificVersion corresponds to a "datestampless" value in WindowsVersion, e.g. WindowsSpecificVersion.WINDOWS_SERVER_2022_ENGLISH_FULL_BASE_2024_02_14 and WindowsVersion.WINDOWS_SERVER_2022_ENGLISH_FULL_BASE
Removed @deprecated tags for WindowsVersions
Added MachineImage.specificWindows static method to lookup WindowsSpecificVersions
Updated documentation, removed references to deprecated versions
Updated enum values (see d852649, 7942755, 1bce268 and d7c3a7c)

Description of how you validated changes

I've added unit and integration tests to validate both MachineImage.specificWindows and MachineImage.latestWindows.

The WindowsSpecificVersion values were compared to the SDK results of ec2:DescribeImages with the following params:

{
	"Owners": ["amazon"],
	"Filters": [{ "Name": "name", "Values": ["Windows_Server*"] }]
}

The WindowsVersion values were compared to the SDK results of ssm:GetParametersByPath with the following params:

{
	"Path": "/aws/service/ami-windows-latest",
}

The parameters that did not start with /aws/service/ami-windows-latest/Windows_Server were ignored. Some are Amazon Linux images:

amzn2-ami-hvm-2.0.*
amzn2-x86_64-SQL_2019_*

Others are either EC2LaunchV2 or NitroTPM Windows images, neither currently supported by the CDK:

EC2LaunchV2-Windows_Server-2016-English-*
TPM-Windows_Server-2016-English-*
TPM-Windows_Server-2019-English-*
TPM-Windows_Server-2022-English-*

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

nmussy · 2024-04-05T09:28:25Z

packages/aws-cdk-lib/aws-ec2/test/example.images.lit.ts

-  generation: ec2.AmazonLinuxGeneration.AMAZON_LINUX,
+const amznLinux2 = ec2.MachineImage.latestAmazonLinux2({
  edition: ec2.AmazonLinuxEdition.STANDARD,
  virtualization: ec2.AmazonLinuxVirt.HVM,
  storage: ec2.AmazonLinuxStorage.GENERAL_PURPOSE,
  cpuType: ec2.AmazonLinuxCpuType.X86_64,
+  kernel: ec2.AmazonLinux2Kernel.KERNEL_5_10,
+});
+
+const amznLinux2023 = ec2.MachineImage.latestAmazonLinux2023({
+  edition: ec2.AmazonLinuxEdition.STANDARD,
+  cpuType: ec2.AmazonLinuxCpuType.X86_64,
+  kernel: ec2.AmazonLinux2023Kernel.KERNEL_6_1,


This can be moved to another PR, just noticed the deprecated method usage when updating the Windows example below

nmussy · 2024-04-05T09:55:43Z

allowed-breaking-changes.txt

Not thrilled about adding a ton of noise to the breaking changes list, but I don't think it would safe to not list them
all individually and use something like removed:aws-cdk-lib.aws_ec2.WindowsVersion.*

Might not be required, I'm trying to have WindowsVersion become a union of two new enums instead, WindowsLatest and WindowsSpecific. Will see if JSII doesn't complain too much

Aborting this idea, it would be even messier:

/** * @deprecated use {@link WindowsLatestVersion} or {@link WindowsSpecificVersion} instead */ export const WindowsVersion = { ...WindowsLatestVersion, ...WindowsSpecificVersion }; export type WindowsVersion = typeof WindowsVersion[keyof typeof WindowsVersion];

API elements with incompatible changes: err - ENUM aws-cdk-lib.aws_ec2.WindowsVersion: has been removed [removed:aws-cdk-lib.aws_ec2.WindowsVersion] err - METHOD aws-cdk-lib.aws_ec2.MachineImage.latestWindows: argument version, takes aws-cdk-lib.aws_ec2.WindowsLatestVersion (formerly aws-cdk-lib.aws_ec2.WindowsVersion): could not find type aws-cdk-lib.aws_ec2.WindowsVersion [incompatible-argument:aws-cdk-lib.aws_ec2.MachineImage.latestWindows] err - INITIALIZER aws-cdk-lib.aws_ec2.WindowsImage.<initializer>: argument version, takes aws-cdk-lib.aws_ec2.WindowsLatestVersion | aws-cdk-lib.aws_ec2.WindowsSpecificVersion (formerly aws-cdk-lib.aws_ec2.WindowsVersion): none of aws-cdk-lib.aws_ec2.WindowsVersion are assignable to aws-cdk-lib.aws_ec2.WindowsLatestVersion | aws-cdk-lib.aws_ec2.WindowsSpecificVersion, could not find type aws-cdk-lib.aws_ec2.WindowsVersion, could not find type aws-cdk-lib.aws_ec2.WindowsVersion [incompatible-argument:aws-cdk-lib.aws_ec2.WindowsImage.<initializer>]

Unsurprisingly, the enum appears as being deleted, even though it retains its values and typing. The bigger issue is that I would need to introduce another breaking change by changing the type of MachineImage.latestWindows.

Removing the unusable enum values added to WindowsVersion is not really an issue for users, but requiring them to change their existing stacks would be a huge pain for basically no gain.

So we're back to the original plan of removing the newly added datestamped WindowsVersion values

This reverts commit 195a484.

…P3_STANDARD

packages/aws-cdk-lib/aws-ec2/lib/windows-versions.ts

msambol · 2024-04-07T17:40:48Z

packages/aws-cdk-lib/aws-ec2/lib/windows-versions.ts

+/**
+ * Specific, datestamped Windows version name.
+ */
+export enum WindowsSpecificVersion {


Will this break folks currently using a timestamped version?

The datestamped versions never worked with WindowsVersion, since the constructor uses ssm:GetParameter, looking for the /aws/service/ami-windows-latest/<version> key. Only non-datestamped versions keys are listed, and their corresponding value points to the latest AMI version, as shown here:

$ aws ssm get-parameters --names /aws/service/ami-windows-latest/Windows_Server-2022-English-Full-Base | jq '.Parameters[] | .Value' "ami-03cd80cfebcbb4481" $ aws ec2 describe-images --image-ids ami-03cd80cfebcbb4481 | jq '.Images[] | .Name' "Windows_Server-2022-English-Full-Base-2024.03.13"

Thanks for clarifying 👍🏼

msambol · 2024-04-08T03:08:08Z

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.windows-machine-image.ts

+  ec2.WindowsSpecificVersion.WINDOWS_SERVER_2016_ENGLISH_FULL_BASE_2023_11_15,
+  ec2.WindowsSpecificVersion.WINDOWS_SERVER_2019_ENGLISH_FULL_BASE_2023_12_13,
+  ec2.WindowsSpecificVersion.WINDOWS_SERVER_2022_ENGLISH_CORE_ECS_OPTIMIZED_2024_01_10,
+];


nice test of both 👍🏼

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.windows-machine-image.ts

msambol

I like the direction you took this. This LGTM.

aws-cdk-automation · 2024-04-08T06:33:44Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 55af1c9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

TheRealAmazonKendra

A couple of things here before I start looking at specifics:

I see the addition of deprecated versions. We shouldn't be adding anything that's deprecated.
What's the actual value add of all these hyper specific versions? If we are going to need to maintain a hyper specific and extremely long list like this, we need a pretty compelling reason for it.

nmussy · 2024-04-09T04:32:19Z

Main use case I see for this PR is giving users an easier access to stable versions so their instances don't get replaced on deploy when there's a new latest version. I don't think this is a good enough reason to continue down this path, reverting the previous change with #29737 should be enough 👍

TheRealAmazonKendra · 2024-04-10T18:27:31Z

Main use case I see for this PR is giving users an easier access to stable versions so their instances don't get replaced on deploy when there's a new latest version. I don't think this is a good enough reason to continue down this path, reverting the previous change with #29737 should be enough 👍

What I'd suggest here is rather than us keeping an exhaustive list in an enum, is having a function where they can provide the date themselves. If no date is provided, yolo, we choose one for you and it gets replaced on deploy, if one is provided, we always use that.

nmussy · 2024-04-10T18:45:55Z

Good call. For now, I'm going to close this PR. I'll open another one to re-update WindowsVersion with the correct deprecated and missing values, and another one to implement dated versions in a more sensible way

nmussy added 3 commits April 5, 2024 10:39

fix(ec2): windows version

68f626d

chore: fix example

d685ed7

chore: remove newline

efb807d

aws-cdk-automation requested a review from a team April 5, 2024 09:22

github-actions bot added bug This issue is a bug. p1 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Apr 5, 2024

nmussy commented Apr 5, 2024

View reviewed changes

nmussy added 2 commits April 5, 2024 11:50

chore: update integ

bca689e

chore: document breaking changes

ff550f9

nmussy commented Apr 5, 2024

View reviewed changes

nmussy added 8 commits April 5, 2024 12:37

chore: integ cleanup

6954aa4

fix: union enum

195a484

chore: add linter exception

9220927

Revert "fix: union enum"

a5c5a8d

This reverts commit 195a484.

fix: remove deprecated for WINDOWS_SERVER_2016_KOREAN_FULL_SQL_2016_S…

d852649

…P3_STANDARD

chore: add missing EKS_OPTIMIZED_1_29 versions

7942755

chore: remove all deprecated -2023.11.14 versions

1bce268

chore: add new 2024.03.* versions

d7c3a7c

nmussy marked this pull request as ready for review April 6, 2024 06:42

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 6, 2024

msambol reviewed Apr 7, 2024

View reviewed changes

packages/aws-cdk-lib/aws-ec2/lib/windows-versions.ts Outdated Show resolved Hide resolved

msambol reviewed Apr 7, 2024

View reviewed changes

msambol reviewed Apr 8, 2024

View reviewed changes

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.windows-machine-image.ts Outdated Show resolved Hide resolved

msambol reviewed Apr 8, 2024

View reviewed changes

packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.windows-machine-image.ts Outdated Show resolved Hide resolved

msambol approved these changes Apr 8, 2024

View reviewed changes

chore: formatting

55af1c9

github-actions bot mentioned this pull request Apr 8, 2024

Weekly PR metrics report #29758

Closed

TheRealAmazonKendra requested changes Apr 8, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 8, 2024

nmussy closed this Apr 10, 2024

nmussy mentioned this pull request Apr 11, 2024

chore(ec2): update WindowsVersions enum #29796

Closed

1 task

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

Conversation

nmussy commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

msambol left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Apr 8, 2024

AWS CodeBuild CI Report

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

nmussy commented Apr 9, 2024

Uh oh!

TheRealAmazonKendra commented Apr 10, 2024

Uh oh!

nmussy commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nmussy commented Apr 5, 2024 •

edited

Loading