fix(eks): Could not use ec2 instance type and size that their names contains dashes

[moelasmar]

Fix the Ec2 instance class regular expression to accept values that contains dashes like (m7i-flex, and metal-48xl).

Closes #27587.

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[vinayak-kukreja]

Hey, @moelasmar thank you for opening a contribution with us.

Couple of comments,

• The PR title, when it is a fix should tell more about the error/issue and not the solution. So for instance, fix(ec2): instance type and size regex does not accommodate dashes
• Could you add to integ test present in ec2 to verify we are able to provision these instances. If you are unable to run these test on your account, please let me know and I can verify those.

[vinayak-kukreja]

Were you able to run this integ test and deploy the resource to your account?

[moelasmar]

I was able to run the test case, but I did not try to deploy it, let me try to deploy it.

[vinayak-kukreja]

Thank you. Let me know if I can help.

[moelasmar]

Hey, @moelasmar thank you for opening a contribution with us.

Couple of comments,

• The PR title, when it is a fix should tell more about the error/issue and not the solution. So for instance, fix(ec2): instance type and size regex does not accommodate dashes
• Could you add to integ test present in ec2 to verify we are able to provision these instances. If you are unable to run these test on your account, please let me know and I can verify those.

The problem is mainly related to adding new managed node groups to an EKS Cluster, and not while creating EC2 instances. The problem is mainly in these 2 functions:

aws-cdk/packages/aws-cdk-lib/aws-ec2/lib/instance-types.ts

Lines 1517 to 1544 in 3b3fc3a

	public get architecture(): InstanceArchitecture {
	// capture the family, generation, capabilities, and size portions of the instance type id
	const instanceTypeComponents = this.instanceTypeIdentifier.match(/^([a-z]+)(\d{1,2})([a-z\-]*)\.([a-z0-9\-]+)$/);
	if (instanceTypeComponents == null) {
	throw new Error('Malformed instance type identifier');
	}
	const family = instanceTypeComponents[1];
	const capabilities = instanceTypeComponents[3];
	// Instance family `a` are first-gen Graviton instances
	// Capability `g` indicates the instance is Graviton2 powered
	if (family === 'a' || capabilities.includes('g')) {
	return InstanceArchitecture.ARM_64;
	}
	return InstanceArchitecture.X86_64;
	}
	public sameInstanceClassAs(other: InstanceType): boolean {
	const instanceClass: RegExp = /^([a-z]+\d{1,2}[a-z\-]*)\.([a-z0-9\-]+)$/;
	const instanceClassId = this.instanceTypeIdentifier.match(instanceClass);
	const otherInstanceClassId = other.instanceTypeIdentifier.match(instanceClass);
	if (instanceClassId == null || otherInstanceClassId == null) {
	throw new Error('Malformed instance type identifier');
	}
	return instanceClassId[1] === otherInstanceClassId[1];
	}

Where are mainly used in these functions in eks managed nodegroup

aws-cdk/packages/aws-cdk-lib/aws-eks/lib/managed-nodegroup.ts

Lines 559 to 579 in f9ee5e6

	function isGpuInstanceType(instanceType: InstanceType): boolean {
	//compare instanceType to known GPU InstanceTypes
	const knownGpuInstanceTypes = [InstanceClass.P2, InstanceClass.P3, InstanceClass.P3DN, InstanceClass.P4DE, InstanceClass.P4D,
	InstanceClass.G3S, InstanceClass.G3, InstanceClass.G4DN, InstanceClass.G4AD, InstanceClass.G5, InstanceClass.G5G,
	InstanceClass.INF1, InstanceClass.INF2];
	return knownGpuInstanceTypes.some((c) => instanceType.sameInstanceClassAs(InstanceType.of(c, InstanceSize.LARGE)));
	}
	/**
	* This function check if the instanceType is supported by Windows AMI.
	* https://docs.aws.amazon.com/eks/latest/userguide/windows-support.html
	* @param instanceType The EC2 instance type
	*/
	function isWindowsSupportedInstanceType(instanceType: InstanceType): boolean {
	//compare instanceType to forbidden InstanceTypes for Windows. Add exception for m6a.16xlarge.
	//NOTE: i2 instance class is not present in the InstanceClass enum.
	const forbiddenInstanceClasses: InstanceClass[] = [InstanceClass.C3, InstanceClass.C4, InstanceClass.D2, InstanceClass.M4,
	InstanceClass.M6A, InstanceClass.R3];
	return instanceType.toString() === InstanceType.of(InstanceClass.M4, InstanceSize.XLARGE16).toString() ||
	forbiddenInstanceClasses.every((c) => !instanceType.sameInstanceClassAs(InstanceType.of(c, InstanceSize.LARGE)) && !instanceType.toString().match(/^i2/));
	}

Anyway, I will try to add a new Ec2 test case

[vinayak-kukreja]

Hey @moelasmar , I ran the integ test and have added that. Looks like the fork needs to be updated. Could you please do that and update here? I can resolve the other conflicts.

[moelasmar]

Hey @moelasmar , I ran the integ test and have added that. Looks like the fork needs to be updated. Could you please do that and update here? I can resolve the other conflicts.

I found some issues on resolving the conflicts, so I start from a fresh fork of the main branch, and added my changes again.
I also executed yarn integ --directory test/aws-eks/test --update-on-failed to run the integration test cases and update the snapshot, and also executed yarn test aws-ec2 to verify the unit test cases.

[paulhcsun]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

it is already approved by paulhcsun, and the requested change got resolved

[mergify]

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