Skip to content

Commit c2043c8

Browse files
authored
fix(ec2): Invalid security group ID (#22859)
When using any of the static methods `fromLookup`, `fromLookupById`, `fromLookupByName` the context provider responsible for doing the lookup will be provided with dummy values: ``` { securityGroupId: 'sg-12345', allowAllOutbound: true, } ``` These values will apply during the construction phase. The actual lookup happens at a later stage. Unfortunately, the dummy value for `securityGroupId` is invalid – at least according to the input validation defined in the `peer` module: https://github.com/aws/aws-cdk/blob/9d1b2c7b1f0147089f912c32a61d7ba86edb543c/packages/@aws-cdk/aws-ec2/lib/peer.ts#L224 This means that any attempt to reference an existing security group retrieved through `fromLookup…()` as a peer causes an exception to be thrown during the construction phase (before CDK even attempts to perform the lookup). Example code: ``` const sg = ec2.SecurityGroup.fromLookupByName(this, "Group", "group-name", vpc); const peer = ec2.Peer.securityGroupId(sg.securityGroupId); ``` Example output: ``` $ cdk synth > Error: Invalid security group ID: "sg-12345" > at new SecurityGroupId (/Users/jsc/code/trustpilot/appmesh-demo/node_modules/aws-cdk-lib/aws-ec2/lib/peer.js:1:2617) > at Function.securityGroupId (/Users/jsc/code/trustpilot/appmesh-demo/node_modules/aws-cdk-lib/aws-ec2/lib/peer.js:1:549) ``` Changing the dummy value to match the expected pattern will allow the construction phase to complete, the lookup will come into play, and the synth will complete without errors and with the actual ID of the referenced security group rendered in the resulting CloudFormation template. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 1ded644 commit c2043c8

File tree

4 files changed

+33
-7
lines changed

4 files changed

+33
-7
lines changed

packages/@aws-cdk/aws-ec2/lib/security-group.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ export class SecurityGroup extends SecurityGroupBase {
432432
vpcId: options.vpc?.vpcId,
433433
},
434434
dummyValue: {
435-
securityGroupId: 'sg-12345',
435+
securityGroupId: 'sg-12345678',
436436
allowAllOutbound: true,
437437
} as cxapi.SecurityGroupContextResponse,
438438
}).value;

packages/@aws-cdk/aws-ec2/test/security-group.test.ts

+30-4
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,9 @@ describe('security group', () => {
516516
});
517517
});
518518
});
519+
});
519520

521+
describe('security group lookup', () => {
520522
testDeprecated('can look up a security group', () => {
521523
const app = new App();
522524
const stack = new Stack(app, 'stack', {
@@ -528,7 +530,7 @@ describe('security group', () => {
528530

529531
const securityGroup = SecurityGroup.fromLookup(stack, 'stack', 'sg-1234');
530532

531-
expect(securityGroup.securityGroupId).toEqual('sg-12345');
533+
expect(securityGroup.securityGroupId).toEqual('sg-12345678');
532534
expect(securityGroup.allowAllOutbound).toEqual(true);
533535

534536
});
@@ -547,7 +549,7 @@ describe('security group', () => {
547549
const securityGroup = SecurityGroup.fromLookupById(stack, 'SG1', 'sg-12345');
548550

549551
// THEN
550-
expect(securityGroup.securityGroupId).toEqual('sg-12345');
552+
expect(securityGroup.securityGroupId).toEqual('sg-12345678');
551553
expect(securityGroup.allowAllOutbound).toEqual(true);
552554

553555
});
@@ -571,7 +573,7 @@ describe('security group', () => {
571573
const securityGroup = SecurityGroup.fromLookupByName(stack, 'SG1', 'sg-12345', vpc);
572574

573575
// THEN
574-
expect(securityGroup.securityGroupId).toEqual('sg-12345');
576+
expect(securityGroup.securityGroupId).toEqual('sg-12345678');
575577
expect(securityGroup.allowAllOutbound).toEqual(true);
576578

577579
});
@@ -595,11 +597,35 @@ describe('security group', () => {
595597
const securityGroup = SecurityGroup.fromLookupByName(stack, 'SG1', 'my-security-group', vpc);
596598

597599
// THEN
598-
expect(securityGroup.securityGroupId).toEqual('sg-12345');
600+
expect(securityGroup.securityGroupId).toEqual('sg-12345678');
599601
expect(securityGroup.allowAllOutbound).toEqual(true);
600602

601603
});
602604

605+
test('can look up a security group and use it as a peer', () => {
606+
// GIVEN
607+
const app = new App();
608+
const stack = new Stack(app, 'stack', {
609+
env: {
610+
account: '1234',
611+
region: 'us-east-1',
612+
},
613+
});
614+
615+
const vpc = Vpc.fromVpcAttributes(stack, 'VPC', {
616+
vpcId: 'vpc-1234',
617+
availabilityZones: ['dummy1a', 'dummy1b', 'dummy1c'],
618+
});
619+
620+
// WHEN
621+
const securityGroup = SecurityGroup.fromLookupByName(stack, 'SG1', 'my-security-group', vpc);
622+
623+
// THEN
624+
expect(() => {
625+
Peer.securityGroupId(securityGroup.securityGroupId);
626+
}).not.toThrow();
627+
});
628+
603629
test('throws if securityGroupId is tokenized', () => {
604630
// GIVEN
605631
const app = new App();

packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/listener.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1719,7 +1719,7 @@ describe('tests', () => {
17191719
// THEN
17201720
Template.fromStack(stack).resourceCountIs('AWS::ElasticLoadBalancingV2::Listener', 0);
17211721
expect(listener.listenerArn).toEqual('arn:aws:elasticloadbalancing:us-west-2:123456789012:listener/application/my-load-balancer/50dc6c495c0c9188/f2f7dc8efc522ab2');
1722-
expect(listener.connections.securityGroups[0].securityGroupId).toEqual('sg-12345');
1722+
expect(listener.connections.securityGroups[0].securityGroupId).toEqual('sg-12345678');
17231723
});
17241724

17251725
test('Can add rules to a looked-up ApplicationListener', () => {

packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -579,7 +579,7 @@ describe('tests', () => {
579579
expect(loadBalancer.loadBalancerCanonicalHostedZoneId).toEqual('Z3DZXE0EXAMPLE');
580580
expect(loadBalancer.loadBalancerDnsName).toEqual('my-load-balancer-1234567890.us-west-2.elb.amazonaws.com');
581581
expect(loadBalancer.ipAddressType).toEqual(elbv2.IpAddressType.DUAL_STACK);
582-
expect(loadBalancer.connections.securityGroups[0].securityGroupId).toEqual('sg-12345');
582+
expect(loadBalancer.connections.securityGroups[0].securityGroupId).toEqual('sg-12345678');
583583
expect(loadBalancer.env.region).toEqual('us-west-2');
584584
});
585585

0 commit comments

Comments
 (0)