Skip to content

Commit

Permalink
fix(ec2): Invalid security group ID (aws#22859)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
schourode authored and Brennan Ho committed Feb 22, 2023
1 parent 2280a74 commit 7800ecb
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 7 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ec2/lib/security-group.ts
Expand Up @@ -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;
Expand Down
34 changes: 30 additions & 4 deletions packages/@aws-cdk/aws-ec2/test/security-group.test.ts
Expand Up @@ -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', {
Expand All @@ -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);

});
Expand All @@ -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);

});
Expand All @@ -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);

});
Expand All @@ -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();
Expand Down
Expand Up @@ -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', () => {
Expand Down
Expand Up @@ -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');
});

Expand Down

0 comments on commit 7800ecb

Please sign in to comment.