From 910a40ff9a2ea8f92e750bcb0a3c6774be76c424 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rn=20Schou=20Sandager?= Date: Thu, 10 Nov 2022 11:03:20 +0100 Subject: [PATCH] fix(ec2): Invalid security group ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-12345678', 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. --- .../@aws-cdk/aws-ec2/lib/security-group.ts | 2 +- .../aws-ec2/test/security-group.test.ts | 34 ++++++++++++++++--- .../test/alb/listener.test.ts | 2 +- .../test/alb/load-balancer.test.ts | 2 +- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index e3b63b42f48af..ab66118204d2c 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/packages/@aws-cdk/aws-ec2/lib/security-group.ts @@ -432,7 +432,7 @@ export class SecurityGroup extends SecurityGroupBase { vpcId: options.vpc?.vpcId, }, dummyValue: { - securityGroupId: 'sg-12345', + securityGroupId: 'sg-12345678', allowAllOutbound: true, } as cxapi.SecurityGroupContextResponse, }).value; diff --git a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts index a480c53f58dfa..5a5e6e41a8009 100644 --- a/packages/@aws-cdk/aws-ec2/test/security-group.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/security-group.test.ts @@ -516,7 +516,9 @@ describe('security group', () => { }); }); }); +}); +describe('security group lookup', () => { testDeprecated('can look up a security group', () => { const app = new App(); const stack = new Stack(app, 'stack', { @@ -528,7 +530,7 @@ describe('security group', () => { const securityGroup = SecurityGroup.fromLookup(stack, 'stack', 'sg-1234'); - expect(securityGroup.securityGroupId).toEqual('sg-12345'); + expect(securityGroup.securityGroupId).toEqual('sg-12345678'); expect(securityGroup.allowAllOutbound).toEqual(true); }); @@ -547,7 +549,7 @@ describe('security group', () => { const securityGroup = SecurityGroup.fromLookupById(stack, 'SG1', 'sg-12345'); // THEN - expect(securityGroup.securityGroupId).toEqual('sg-12345'); + expect(securityGroup.securityGroupId).toEqual('sg-12345678'); expect(securityGroup.allowAllOutbound).toEqual(true); }); @@ -571,7 +573,7 @@ describe('security group', () => { const securityGroup = SecurityGroup.fromLookupByName(stack, 'SG1', 'sg-12345', vpc); // THEN - expect(securityGroup.securityGroupId).toEqual('sg-12345'); + expect(securityGroup.securityGroupId).toEqual('sg-12345678'); expect(securityGroup.allowAllOutbound).toEqual(true); }); @@ -595,11 +597,35 @@ describe('security group', () => { const securityGroup = SecurityGroup.fromLookupByName(stack, 'SG1', 'my-security-group', vpc); // THEN - expect(securityGroup.securityGroupId).toEqual('sg-12345'); + expect(securityGroup.securityGroupId).toEqual('sg-12345678'); expect(securityGroup.allowAllOutbound).toEqual(true); }); + test('can look up a security group and use it as a peer', () => { + // GIVEN + const app = new App(); + const stack = new Stack(app, 'stack', { + env: { + account: '1234', + region: 'us-east-1', + }, + }); + + const vpc = Vpc.fromVpcAttributes(stack, 'VPC', { + vpcId: 'vpc-1234', + availabilityZones: ['dummy1a', 'dummy1b', 'dummy1c'], + }); + + // WHEN + const securityGroup = SecurityGroup.fromLookupByName(stack, 'SG1', 'my-security-group', vpc); + + // THEN + expect(() => { + Peer.securityGroupId(securityGroup.securityGroupId); + }).not.toThrow(); + }); + test('throws if securityGroupId is tokenized', () => { // GIVEN const app = new App(); diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/listener.test.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/listener.test.ts index b181d086633e8..838897fa5dfe1 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/listener.test.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/listener.test.ts @@ -1719,7 +1719,7 @@ describe('tests', () => { // THEN Template.fromStack(stack).resourceCountIs('AWS::ElasticLoadBalancingV2::Listener', 0); expect(listener.listenerArn).toEqual('arn:aws:elasticloadbalancing:us-west-2:123456789012:listener/application/my-load-balancer/50dc6c495c0c9188/f2f7dc8efc522ab2'); - expect(listener.connections.securityGroups[0].securityGroupId).toEqual('sg-12345'); + expect(listener.connections.securityGroups[0].securityGroupId).toEqual('sg-12345678'); }); test('Can add rules to a looked-up ApplicationListener', () => { diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts index 102d016bf6bec..4124a222b7228 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts @@ -579,7 +579,7 @@ describe('tests', () => { expect(loadBalancer.loadBalancerCanonicalHostedZoneId).toEqual('Z3DZXE0EXAMPLE'); expect(loadBalancer.loadBalancerDnsName).toEqual('my-load-balancer-1234567890.us-west-2.elb.amazonaws.com'); expect(loadBalancer.ipAddressType).toEqual(elbv2.IpAddressType.DUAL_STACK); - expect(loadBalancer.connections.securityGroups[0].securityGroupId).toEqual('sg-12345'); + expect(loadBalancer.connections.securityGroups[0].securityGroupId).toEqual('sg-12345678'); expect(loadBalancer.env.region).toEqual('us-west-2'); });