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

feat(asg): support keypair functionality for asg #29679

Merged
merged 21 commits into from
May 10, 2024

Conversation

GabrielHinz
Copy link
Contributor

@GabrielHinz GabrielHinz commented Apr 1, 2024

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


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

@github-actions 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 aws-cdk-automation requested a review from a team April 1, 2024 18:48
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@GabrielHinz GabrielHinz changed the title feat(asg): Support keypair functionality for asg feat(asg): support keypair functionality for asg Apr 1, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review April 1, 2024 22:27

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

@aws-cdk-automation 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
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 87 to 88
*/
readonly keyName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4940cd1

Comment on lines 93 to 95
* `keyName`, `launchTemplate` and `mixedInstancesPolicy` must not be specified
* when this property is specified
*
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4940cd1

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

Choose a reason for hiding this comment

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

We're missing this validation for keyPair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 73e2e99

@@ -1390,6 +1411,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
spotOptions: props.spotPrice !== undefined ? { maxPrice: parseFloat(props.spotPrice) } : undefined,
blockDevices: props.blockDevices,
instanceProfile,
...keyUsageType,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Given the above validation, we can simplify this assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 ?

@@ -1398,6 +1420,10 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
this._connections = new ec2.Connections({ securityGroups: [this.securityGroup] });
this.securityGroups = [this.securityGroup];

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

Choose a reason for hiding this comment

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

Do we need this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! f7a3896

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

Choose a reason for hiding this comment

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

Suggested change
* keyName
* keyName (deprecated, prefer keyPair)

Could you please add an example of keyPair usage in the code block 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.

Done in b88ea01

@aws-cdk-automation 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
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

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

@@ -1390,6 +1415,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
spotOptions: props.spotPrice !== undefined ? { maxPrice: parseFloat(props.spotPrice) } : undefined,
blockDevices: props.blockDevices,
instanceProfile,
...keyUsageType,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

@@ -1398,6 +1420,10 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
this._connections = new ec2.Connections({ securityGroups: [this.securityGroup] });
this.securityGroups = [this.securityGroup];

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

Choose a reason for hiding this comment

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

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

@GabrielHinz
Copy link
Contributor Author

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

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@@ -1390,6 +1415,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
spotOptions: props.spotPrice !== undefined ? { maxPrice: parseFloat(props.spotPrice) } : undefined,
blockDevices: props.blockDevices,
instanceProfile,
...keyUsageType,
Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

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

LGTM .. Thank you @GabrielHinz for your contribution.

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

@shikha372
Copy link
Contributor

ill be updated from main and then merged automatically (

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

@shikha372
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented May 2, 2024

update

❌ Mergify doesn't have permission to update

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

Copy link
Contributor

mergify bot commented May 10, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 10, 2024
Copy link
Contributor

mergify bot commented May 10, 2024

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

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 mergify bot merged commit f6b649d into aws:main May 10, 2024
9 checks passed
Copy link
Contributor

mergify bot commented May 10, 2024

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

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-autoscaling): Missing KeyPair field on autoScalingGroupProps
4 participants