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): WindowsVersion enum #29738

Closed
wants to merge 14 commits into from
Closed

Conversation

nmussy
Copy link
Contributor

@nmussy nmussy commented Apr 5, 2024

Issue # (if applicable)

Closes #29736

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


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

@aws-cdk-automation aws-cdk-automation requested a review from a team April 5, 2024 09:22
@github-actions github-actions bot added bug This issue is a bug. p1 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Apr 5, 2024
Comment on lines -7 to +17
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@nmussy nmussy marked this pull request as ready for review April 6, 2024 06:42
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 6, 2024
/**
* Specific, datestamped Windows version name.
*/
export enum WindowsSpecificVersion {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this break folks currently using a timestamped version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying 👍🏼

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,
];
Copy link
Contributor

Choose a reason for hiding this comment

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

nice test of both 👍🏼

Copy link
Contributor

@msambol msambol left a comment

Choose a reason for hiding this comment

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

I like the direction you took this. This LGTM.

@aws-cdk-automation
Copy link
Collaborator

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

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

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

  1. I see the addition of deprecated versions. We shouldn't be adding anything that's deprecated.
  2. 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.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 8, 2024
@nmussy
Copy link
Contributor Author

nmussy commented Apr 9, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

nmussy commented Apr 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p1
Projects
None yet
4 participants