feat(asg): support keypair functionality for asg by gabrielhinz · Pull Request #29679 · aws/aws-cdk

gabrielhinz · 2024-04-01T18:48:08Z

Issue

When creating an instance directly through the asg, it is not possible to define the keyPair, in addition to bringing a warning message that the keyName will be removed

This configuration allows sending the keyPair to the asg since the LaunchTemplate allows its integration

Warning:

[WARNING] aws-cdk-lib.aws_ec2.LaunchTemplateProps#keyName is deprecated.

Closes #29237

Reason for this change

I'm working directly with CDK and needed to implement a way to use my keyPair and avoid warning that keyName will be removed soon when i'm creating my ASG

Description of changes

Add keyPair to CommonAutoScalingGroupProps interface
Prevent keyPair and keyName from being set at the same time
Send keyPair when creating LaunchTemplate if flag AUTOSCALING_GENERATE_LAUNCH_TEMPLATE is enabled
Prevent keyPair if the flag AUTOSCALING_GENERATE_LAUNCH_TEMPLATE is disabled

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

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.

lpizzinidev

Thanks 👍
Left some comments for adjustments.
Also, the new property and all added validations should have proper unit test coverage here.

lpizzinidev · 2024-04-06T15:00:03Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

   */
  readonly keyName?: string;


Suggested change

*/

readonly keyName?: string;

* @deprecated - Use `keyPair` instead - https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ec2-readme.html#using-an-existing-ec2-key-pair

*/

readonly keyName?: string;

We should mark the property as deprecated.

Fixed in 4940cd1

lpizzinidev · 2024-04-06T15:01:58Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+   * `keyName`, `launchTemplate` and `mixedInstancesPolicy` must not be specified
+   *  when this property is specified
+   *


Suggested change

* `keyName`, `launchTemplate` and `mixedInstancesPolicy` must not be specified

* when this property is specified

*

* `launchTemplate` and `mixedInstancesPolicy` must not be specified

* when this property is specified.

*

* You can either specify `keyPair` or `keyName`, not both.

Suggestion for different wording. Can you please update keyName documentation as well?

Fixed in 4940cd1

lpizzinidev · 2024-04-06T15:06:13Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

+  /**
+   * The SSH keypair to grant access to the instance.
+   *
+   * `keyName`, `launchTemplate` and `mixedInstancesPolicy` must not be specified


We're missing this validation for keyPair.

Fixed in 73e2e99

lpizzinidev · 2024-04-06T15:08:56Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

          spotOptions: props.spotPrice !== undefined ? { maxPrice: parseFloat(props.spotPrice) } : undefined,
          blockDevices: props.blockDevices,
          instanceProfile,
+          ...keyUsageType,


Suggested change

...keyUsageType,

keyName: props.keyPair?.keyPairName ?? props?.keyName,

Given the above validation, we can simplify this assignment.

@lpizzinidev The idea of creating the keyUsageType was to send just one of them (keyPair or keyName) to the LaunchTemplate, to avoid sending the keyName if keyPair is set

let keyUsageType: { keyPair?: ec2.IKeyPair; keyName?: string } = {}; if (props.keyPair) { keyUsageType = { keyPair: props.keyPair }; } else { keyUsageType = { keyName: props.keyName }; }

It makes sense ?

lpizzinidev · 2024-04-06T15:10:29Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

        this.securityGroups = [this.securityGroup];

+        if (props.keyPair) {
+          throw new Error('Can only use \'keyPair\' when feature flag \'AUTOSCALING_GENERATE_LAUNCH_TEMPLATE\' is set');


Do we need this check?

I added this validation because if AUTOSCALING_GENERATE_LAUNCH_TEMPLATE is disabled, a CfnLaunchConfiguration will be created instead LaunchTemplate .. and doesn't have support for the keyPair

I see, thanks for clarifying! Can you please add it to the documentation of the variable?

Done! f7a3896

lpizzinidev · 2024-04-06T15:12:51Z

packages/aws-cdk-lib/aws-autoscaling/README.md

@@ -24,6 +24,7 @@ new autoscaling.AutoScalingGroup(this, 'ASG', {
 Creating an `AutoScalingGroup` from a Launch Configuration has been deprecated. All new accounts created after December 31, 2023 will no longer be able to create Launch Configurations. With the `@aws-cdk/aws-autoscaling:generateLaunchTemplateInsteadOfLaunchConfig` feature flag set to true, `AutoScalingGroup` properties used to create a Launch Configuration will now be used to create a `LaunchTemplate` using a [Launch Configuration to `LaunchTemplate` mapping](https://docs.aws.amazon.com/autoscaling/ec2/userguide/migrate-launch-configurations-with-cloudformation.html#launch-configuration-mapping-reference). Specifically, the following `AutoScalingGroup` properties will be used to generate a `LaunchTemplate`:
 * machineImage
 * keyName


Suggested change

* keyName

* keyName (deprecated, prefer keyPair)

Could you please add an example of keyPair usage in the code block below?

Done in b88ea01

lpizzinidev

Thanks for implementing the changes and adding the comments 👍
Left a couple of suggestions.
Plus, we're still missing coverage on unit tests (#29679 (review)).

lpizzinidev · 2024-04-09T19:30:46Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

          spotOptions: props.spotPrice !== undefined ? { maxPrice: parseFloat(props.spotPrice) } : undefined,
          blockDevices: props.blockDevices,
          instanceProfile,
+          ...keyUsageType,


Suggested change

...keyUsageType,

keyPair: props.keyPair,

keyName: props.keyName,

The idea of creating the keyUsageType was to send just one of them (keyPair or keyName) to the LaunchTemplate, to avoid sending the keyName if keyPair is set

Makes sense, thanks for clarifying!
However, we could simply specify it like this then if I'm not mistaken.

I tried the way you mentioned, but I believe that because of the LaunchTemplate, when sending the keyName in your constructor, it will display the warning on the screen, even if the user did not define it when creating the ASG 😵‍💫

[WARNING] aws-cdk-lib.aws_ec2.LaunchTemplateProps#keyName is deprecated.

Well, that's annoying 🫠

Suggested change

...keyUsageType,

keyPair: props.keyPair,

...{ props.keyName ? { keyName: props.keyName } : {} },

I guess this should work then.
Your implementation is correct of course, but I'd like to keep it more concise if possible.
Thanks for following up on this 👍

lpizzinidev · 2024-04-09T19:33:11Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

        this.securityGroups = [this.securityGroup];

+        if (props.keyPair) {
+          throw new Error('Can only use \'keyPair\' when feature flag \'AUTOSCALING_GENERATE_LAUNCH_TEMPLATE\' is set');


I see, thanks for clarifying! Can you please add it to the documentation of the variable?

gabrielhinz · 2024-04-09T22:56:58Z

Thanks for implementing the changes and adding the comments 👍 Left a couple of suggestions. Plus, we're still missing coverage on unit tests (#29679 (review)).

Unit tests add: 51fda7f

lpizzinidev

Thanks 👍

lpizzinidev · 2024-04-12T06:57:23Z

packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts

          spotOptions: props.spotPrice !== undefined ? { maxPrice: parseFloat(props.spotPrice) } : undefined,
          blockDevices: props.blockDevices,
          instanceProfile,
+          ...keyUsageType,


Well, that's annoying 🫠

Suggested change

...keyUsageType,

keyPair: props.keyPair,

...{ props.keyName ? { keyName: props.keyName } : {} },

I guess this should work then.
Your implementation is correct of course, but I'd like to keep it more concise if possible.
Thanks for following up on this 👍

gabrielhinz · 2024-04-30T13:02:56Z

LGTM .. Thank you @gabrielhinz for your contribution.

@shikha372 I believe I updated the branch by mistake, sorry 😵

shikha372 · 2024-04-30T17:11:29Z

ill be updated from main and then merged automatically (

yeah, manual merge not recommended with mergifyio, i'll see it from here

shikha372 · 2024-05-02T21:26:34Z

@Mergifyio update

mergify · 2024-05-02T21:26:42Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

mergify · 2024-05-10T21:07:14Z

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

mergify · 2024-05-10T21:11:09Z

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-cdk-automation · 2024-05-10T21:55:51Z

AWS CodeBuild CI Report

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

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

mergify · 2024-05-10T21:56:11Z

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-cdk-automation · 2024-07-25T21:18:51Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

feat(asg): Support keypair functionality for asg

50c44d9

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Apr 1, 2024

aws-cdk-automation requested a review from a team April 1, 2024 18:48

aws-cdk-automation previously requested changes Apr 1, 2024

View reviewed changes

gabrielhinz changed the title ~~feat(asg): Support keypair functionality for asg~~ feat(asg): support keypair functionality for asg Apr 1, 2024

gabrielhinz and others added 4 commits April 1, 2024 16:09

Update README.md adding keyPair reference

077cf5e

Merge branch 'main' into asg-keypair

0f53e85

fix: adjust lib for use only keyPair or keyName

b08b7f0

test: adding asg keypair test and snapshot

24dc6e3

gabrielhinz and others added 2 commits April 1, 2024 19:43

Fix: Using ESLint to identify and remove problems

cb03ef3

Merge branch 'main' into asg-keypair

e710a2f

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 1, 2024

gabrielhinz added 2 commits April 1, 2024 20:42

Fix: include removed line from aws-autoscaling README.md

14a6df4

Merge branch 'main' into asg-keypair

7deb69e

lpizzinidev suggested changes Apr 6, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 6, 2024

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

Weekly PR metrics report #29758

Closed

gabrielhinz added 3 commits April 9, 2024 15:00

Fix: improving documentation

4940cd1

Fix: adding missing validation

73e2e99

Fix: updating README with example

b88ea01

lpizzinidev suggested changes Apr 9, 2024

View reviewed changes

gabrielhinz added 2 commits April 9, 2024 17:13

docs: updating keyPair docs

f7a3896

Test: improving tests to asg-keypair

51fda7f

lpizzinidev approved these changes Apr 12, 2024

View reviewed changes

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

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

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

Monthly PR metrics report #30026

Closed

Merge branch 'main' into asg-keypair

f13afe0

shikha372 approved these changes May 10, 2024

View reviewed changes

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

Merge branch 'main' into asg-keypair

7b6bfb2

mergify bot merged commit f6b649d into aws:main May 10, 2024

tengell mentioned this pull request Jun 6, 2024

[Snyk] Upgrade aws-cdk-lib from 2.141.0 to 2.142.0 capralifecycle/liflig-cdk#267

Closed

NOUIY mentioned this pull request Jun 7, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#102

Open

niravparikh05 mentioned this pull request Jun 13, 2024

[Snyk] Upgrade aws-cdk from 2.65.0 to 2.142.1 paralus/eks-blueprints-addon#3

Open

NOUIY mentioned this pull request Jun 14, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#103

Open

snyk-io bot mentioned this pull request Jun 20, 2024

[Snyk] Upgrade aws-cdk-lib from 2.138.0 to 2.143.1 Opetushallitus/heratepalvelu#280

Closed

NOUIY mentioned this pull request Jun 20, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#104

Open

snyk-io bot mentioned this pull request Jun 21, 2024

[Snyk] Upgrade aws-cdk-lib from 2.139.0 to 2.143.1 Opetushallitus/ovara#36

Open

This was referenced Jun 22, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#105

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#106

Open

This was referenced Jul 5, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#107

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#108

Open

This was referenced Jul 16, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#109

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#110

Open

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

-   * `keyName`, `launchTemplate` and `mixedInstancesPolicy` must not be specified
-   *  when this property is specified
-   *
+   * `launchTemplate` and `mixedInstancesPolicy` must not be specified
+   *  when this property is specified.
+   *
+   * You can either specify `keyPair` or `keyName`, not both.

	...keyUsageType,
	keyName: props.keyPair?.keyPairName ?? props?.keyName,

	...keyUsageType,
	keyPair: props.keyPair,
	keyName: props.keyName,

	...keyUsageType,
	keyPair: props.keyPair,
	...{ props.keyName ? { keyName: props.keyName } : {} },

Conversation

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

Uh oh!

Issue

Reason for this change

Description of changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

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!

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!

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!

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!

lpizzinidev left a comment

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!

Choose a reason for hiding this comment

Uh oh!

gabrielhinz commented Apr 9, 2024

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielhinz commented Apr 30, 2024

Uh oh!

shikha372 commented Apr 30, 2024

Uh oh!

shikha372 commented May 2, 2024

Uh oh!

mergify bot commented May 2, 2024

❌ Mergify doesn't have permission to update

Uh oh!

mergify bot commented May 10, 2024

Uh oh!

mergify bot commented May 10, 2024

Uh oh!

aws-cdk-automation commented May 10, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented May 10, 2024

Uh oh!

aws-cdk-automation commented Jul 25, 2024

Uh oh!

gabrielhinz commented Apr 1, 2024 •

edited

Loading