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
Merged
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
50c44d9
feat(asg): Support keypair functionality for asg
GabrielHinz Apr 1, 2024
077cf5e
Update README.md adding keyPair reference
GabrielHinz Apr 1, 2024
0f53e85
Merge branch 'main' into asg-keypair
GabrielHinz Apr 1, 2024
b08b7f0
fix: adjust lib for use only keyPair or keyName
GabrielHinz Apr 1, 2024
24dc6e3
test: adding asg keypair test and snapshot
GabrielHinz Apr 1, 2024
cb03ef3
Fix: Using ESLint to identify and remove problems
GabrielHinz Apr 1, 2024
e710a2f
Merge branch 'main' into asg-keypair
GabrielHinz Apr 1, 2024
14a6df4
Fix: include removed line from aws-autoscaling README.md
GabrielHinz Apr 1, 2024
7deb69e
Merge branch 'main' into asg-keypair
GabrielHinz Apr 3, 2024
4940cd1
Fix: improving documentation
GabrielHinz Apr 9, 2024
73e2e99
Fix: adding missing validation
GabrielHinz Apr 9, 2024
b88ea01
Fix: updating README with example
GabrielHinz Apr 9, 2024
f7a3896
docs: updating keyPair docs
GabrielHinz Apr 9, 2024
51fda7f
Test: improving tests to asg-keypair
GabrielHinz Apr 9, 2024
7044409
Refactor: pass keyName only if is defined
GabrielHinz Apr 12, 2024
355295f
Merge branch 'main' into asg-keypair
GabrielHinz Apr 17, 2024
78734fb
Merge branch 'main' into asg-keypair
GabrielHinz Apr 18, 2024
5b723f9
Merge branch 'main' into asg-keypair
GabrielHinz Apr 20, 2024
b0ebc8e
Merge branch 'main' into asg-keypair
GabrielHinz Apr 28, 2024
f13afe0
Merge branch 'main' into asg-keypair
shikha372 May 3, 2024
7b6bfb2
Merge branch 'main' into asg-keypair
mergify[bot] May 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ export interface CommonAutoScalingGroupProps {
*/
readonly keyName?: string;
Comment on lines 90 to 91
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


/**
* The SSH keypair to grant access to the instance.
*
* `keyName`, `launchTemplate` and `mixedInstancesPolicy` must not be specified
* when this property is specified
*
* @default - No SSH access will be possible.
*/
readonly keyPair?: ec2.IKeyPair;

/**
* Where to place instances within the VPC
*
Expand Down Expand Up @@ -1355,6 +1365,10 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
throw new Error('Setting \'instanceType\' is required when \'launchTemplate\' and \'mixedInstancesPolicy\' is not set');
}

if (props.keyName && props.keyPair) {
throw new Error('Cannot specify both of \'keyName\' and \'keyPair\'; prefer \'keyPair\'');
}

Tags.of(this).add(NAME_TAG, this.node.path);

this.securityGroup = props.securityGroup || new ec2.SecurityGroup(this, 'InstanceSecurityGroup', {
Expand All @@ -1381,6 +1395,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements

launchTemplateFromConfig = new ec2.LaunchTemplate(this, 'LaunchTemplate', {
machineImage: props.machineImage,
keyPair: props.keyPair,
keyName: props.keyName,
instanceType: props.instanceType,
detailedMonitoring: props.instanceMonitoring !== undefined && props.instanceMonitoring === Monitoring.DETAILED,
Expand All @@ -1398,6 +1413,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

}

// use delayed evaluation
const imageConfig = props.machineImage.getImage(this);
this._userData = props.userData ?? imageConfig.userData;
Expand Down
Loading