Skip to content

Commit

Permalink
fix(ec2): passing keypair to instance unexpectedly does nothing (#28482)
Browse files Browse the repository at this point in the history
Fixes by Specifying key pair reference in cfnInstance.

This will change behavior if both `keyName` and `keyPair` is set on an existing resource, since we will use `keyPair.keyPairName` instead of `keyName` now. However, there is no correct use case for specifying both `keyPair` and `keyName`, and given `keyName` is deprecated, this PR is introducing the correct behavior.

Closes #28478.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
ayush-shah-1501 authored Jan 8, 2024
1 parent 0042e53 commit 22e6ce8
Show file tree
Hide file tree
Showing 14 changed files with 194 additions and 165 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Large diffs are not rendered by default.

This file was deleted.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@
"S3Bucket": {
"Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
},
"S3Key": "a099fdfc61c84ffc56cef4fb2c9472483623ac865ce5d8fca88c89cf60d48d03.zip"
"S3Key": "4554b47be6f57b68c6c7a7391dcc73894866d2377fe174883351e7639097f292.zip"
},
"Timeout": 900,
"MemorySize": 128,
Expand Down Expand Up @@ -588,6 +588,9 @@
"Ref": "SsmParameterValueawsserviceamiamazonlinuxlatestal2023amikernel61x8664C96584B6F00A464EAD1953AFF4B05118Parameter"
},
"InstanceType": "t3.micro",
"KeyName": {
"Ref": "TestKeyPair38B6CD21"
},
"SecurityGroupIds": [
{
"Fn::GetAtt": [
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-ec2/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export interface InstanceProps {
* Name of SSH keypair to grant access to instance
*
* @default - No SSH access will be possible.
* @deprecated - Use {@link keyPair} instead
* @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;

Expand Down Expand Up @@ -437,7 +437,7 @@ export class Instance extends Resource implements IInstance {
// there is no need to configure them on the instance level
this.instance = new CfnInstance(this, 'Resource', {
imageId: imageConfig.imageId,
keyName: props.keyName,
keyName: props.keyPair?.keyPairName ?? props?.keyName,
instanceType: props.instanceType.toString(),
subnetId: networkInterfaces ? undefined : subnet.subnetId,
securityGroupIds: networkInterfaces ? undefined : securityGroupsToken,
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-ec2/lib/launch-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ export interface LaunchTemplateProps {
* Name of SSH keypair to grant access to instance
*
* @default - No SSH access will be possible.
* @deprecated - Use `keyPair` instead.
* @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;

Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-ec2/lib/nat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ export interface NatInstanceProps {
* Name of SSH keypair to grant access to instance
*
* @default - No SSH access will be possible.
* @deprecated - Use `keyPair` instead.
* @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;

Expand Down
20 changes: 20 additions & 0 deletions packages/aws-cdk-lib/aws-ec2/test/instance.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,26 @@ describe('instance', () => {
})).toThrow('Cannot specify both of \'keyName\' and \'keyPair\'; prefer \'keyPair\'');
});

it('correctly associates a key pair', () => {
// GIVEN
const keyPair = new KeyPair(stack, 'KeyPair', {
keyPairName: 'test-key-pair',
});

// WHEN
new Instance(stack, 'Instance', {
vpc,
instanceType: new InstanceType('t2.micro'),
machineImage: new AmazonLinuxImage(),
keyPair,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::EC2::Instance', {
KeyName: stack.resolve(keyPair.keyPairName),
});
});

describe('Detailed Monitoring', () => {
test('instance with Detailed Monitoring enabled', () => {
// WHEN
Expand Down

0 comments on commit 22e6ce8

Please sign in to comment.