fix(ec2): Invalid security group ID #22859

schourode · 2022-11-10T10:05:44Z

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:

aws-cdk/packages/@aws-cdk/aws-ec2/lib/peer.ts

Line 224 in 9d1b2c7

const securityGroupMatch = securityGroupId.match(/^sg-[a-z0-9]{8,17}$/);

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:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2022-11-10T10:05:49Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

schourode · 2022-11-10T10:14:49Z

Seems the robot overlords are unimpressed by this PR with no changes to tests 😞

Hoping that a human will be reading this, I will attempt to defend myself: I did read the contribution guidelines and saw that a fix type PR "requires tests in principle, but can be suppressed". I believe the change I am proposing here is small enough, involving only a string literal dummy value.

I hope you agree 🙏

schourode · 2022-11-10T11:13:08Z

My initial attempt at this fix broke four existing tests. I have just now force pushed an updated version of my commit that should make the four tests pass again 🤞

However, it seems to me that these particular tests are really misleading. As far as I can tell, none of these go past the construction phase, so the lookup won't actually happen, and the asserts are all happening against the dummy value.

I am not exactly sure to do from here. Assuming the tests pass after my recent changes, I believe this PR will fix an actual bug, which is causing me some issues in the application I am working on currently. At the same time, I understand if you would prefer to see the tests improved before wrapping this up.

Let me know what you think.

mrgrain · 2022-11-10T11:36:06Z

My initial attempt at this fix broke four existing tests. I have just now force pushed an updated version of my commit that should make the four tests pass again 🤞

Looks like they don't 😢

However, it seems to me that these particular tests are really misleading. As far as I can tell, none of these go past the construction phase, so the lookup won't actually happen, and the asserts are all happening against the dummy value.

I am not exactly sure to do from here. Assuming the tests pass after my recent changes, I believe this PR will fix an actual bug, which is causing me some issues in the application I am working on currently. At the same time, I understand if you would prefer to see the tests improved before wrapping this up.

I'd like to see an integration test for your particular example code. At the moment the tests only really cover that you have changed the dummy value.

rix0rrr · 2022-11-10T12:58:05Z

I'd like to see an integration test for your particular example code. At the moment the tests only really cover that you have changed the dummy value.

I don't think it'll be possible to write an integ test for this. Integ tests generally don't cover dummy values...

mrgrain · 2022-11-10T13:05:43Z

const sg = ec2.SecurityGroup.fromLookupByName(this, "Group", "group-name", vpc);
const peer = ec2.Peer.securityGroupId(sg.securityGroupId);

Good point. I still think we should at least have a test that asserts that the example code does not throw (which it currently does):

const sg = ec2.SecurityGroup.fromLookupByName(this, "Group", "group-name", vpc);
const peer = ec2.Peer.securityGroupId(sg.securityGroupId);

schourode · 2022-11-11T10:27:51Z

Sorry about those failing unit tests. I should have known attempting to cowboy in three characters without taking the time to get a local build environment in place to run all of the tests would lead to suffering

I've amended my commit yet again to adjust the dummy value across another two tests.

As for integration tests, I was thinking if something along these lines might do the trick:

Create one app + stack with a VPC and a security group.
Create another app + stack looking up VPC and security group, then creates a second security group with an ingress rule referencing the looked up group.

In code, it would look something like this (though I still haven't managed to build/test it locally):

import * as cdk from '@aws-cdk/core';
import * as ec2 from '../lib';

const appWithVpc = new cdk.App();
const stackWithVpc = new cdk.Stack(appWithVpc, 'StackWithVpc');

const vpc = new ec2.Vpc(stackWithVpc, 'MyVpc', {
  vpcName: 'my-vpc-name',
});

new ec2.SecurityGroup(stackWithVpc, 'SgToLookup', {
  securityGroupName: 'sg-to-lookup',
  vpc
});

const appUnderTest = new cdk.App();
const stackUnderTest = new cdk.Stack(appUnderTest, 'StackUnderTest');

const vpcFromLookup = ec2.Vpc.fromLookup(stackUnderTest, 'VpcFromLookup', {
  vpcName: 'my-vpc-name',
});

const sgLookedUp = ec2.SecurityGroup.fromLookupByName(
  stackUnderTest,
  'SgLookedUp',
  'sg-to-lookup',
  vpcFromLookup
);

const newSg = new ec2.SecurityGroup(appUnderTest, 'NewSg', {
  vpc: vpcFromLookup,
});

newSg.addIngressRule(
  ec2.Peer.securityGroupId(sgLookedUp.securityGroupId),
  ec2.Port.allTcp()
);

appWithVpc.synth();
appUnderTest.synth();

mrgrain · 2022-11-11T15:23:14Z

@schourode that test would certainly be possible. We're doing something similar here: https://github.com/aws/aws-cdk/blob/main/packages/@aws-cdk/aws-ssm/test/integ.parameter-store-string.ts

But like Rico pointed out, there's maybe not much point to an integration test. Maybe a simple unit test with your example code would be sufficient and easier? Your choice.

comcalvi

+1 for a unit test that confirms the code that used to throw no longer throws.

aws-cdk-automation · 2022-12-02T00:09:16Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

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.

schourode · 2022-12-07T14:02:24Z

@rix0rrr @mrgrain I have added a unit test asserting that Peer.securityGroup() doesn't throw when using the id of a SG obtained through lookup. Does it look similar to what you expected?

The CI linter is still unhappy that my PR is not adding any new integration test. Is that something you are able to overrule (assuming you are happy with the unit test of course)?

mrgrain · 2022-12-07T14:03:21Z

@schourode looks great now, thank you! I'll take it from here

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

mergify · 2022-12-07T17:41:04Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…oup-id

Pull request has been modified.

aws-cdk-automation · 2022-12-08T12:22:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5e63b6a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-12-08T12:36:16Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

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*

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Nov 10, 2022

aws-cdk-automation requested a review from a team November 10, 2022 10:05

aws-cdk-automation previously requested changes Nov 10, 2022

View reviewed changes

schourode force-pushed the invalid-security-group-id branch from 9250950 to ca08e78 Compare November 10, 2022 11:02

schourode force-pushed the invalid-security-group-id branch from ca08e78 to aa2a96e Compare November 11, 2022 10:13

comcalvi reviewed Nov 14, 2022

View reviewed changes

schourode force-pushed the invalid-security-group-id branch from aa2a96e to 910a40f Compare December 7, 2022 13:20

mrgrain self-assigned this Dec 7, 2022

mrgrain added pr-linter/exempt-integ-test The PR linter will not require integ test changes pr/do-not-merge This PR should not be merged at this time. labels Dec 7, 2022

mrgrain previously approved these changes Dec 7, 2022

View reviewed changes

mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Dec 7, 2022

Merge remote-tracking branch 'upstream/main' into invalid-security-gr…

5e63b6a

…oup-id

mrgrain approved these changes Dec 8, 2022

View reviewed changes

mergify bot merged commit c2043c8 into aws:main Dec 8, 2022

peterwoodworth mentioned this pull request Dec 15, 2022

(aws_ec2): Dummy Security Group id has changed #23361

Closed

schourode deleted the invalid-security-group-id branch January 6, 2023 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ec2): Invalid security group ID #22859

fix(ec2): Invalid security group ID #22859

schourode commented Nov 10, 2022 •

edited

gitpod-io bot commented Nov 10, 2022

aws-cdk-automation left a comment

schourode commented Nov 10, 2022

schourode commented Nov 10, 2022

mrgrain commented Nov 10, 2022

rix0rrr commented Nov 10, 2022

mrgrain commented Nov 10, 2022

schourode commented Nov 11, 2022

mrgrain commented Nov 11, 2022

comcalvi left a comment

aws-cdk-automation commented Dec 2, 2022

schourode commented Dec 7, 2022

mrgrain commented Dec 7, 2022 •

edited

mergify bot commented Dec 7, 2022

aws-cdk-automation commented Dec 8, 2022

mergify bot commented Dec 8, 2022

fix(ec2): Invalid security group ID #22859

fix(ec2): Invalid security group ID #22859

Conversation

schourode commented Nov 10, 2022 • edited

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Nov 10, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

schourode commented Nov 10, 2022

schourode commented Nov 10, 2022

mrgrain commented Nov 10, 2022

rix0rrr commented Nov 10, 2022

mrgrain commented Nov 10, 2022

schourode commented Nov 11, 2022

mrgrain commented Nov 11, 2022

comcalvi left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 2, 2022

schourode commented Dec 7, 2022

mrgrain commented Dec 7, 2022 • edited

mergify bot commented Dec 7, 2022

aws-cdk-automation commented Dec 8, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 8, 2022

schourode commented Nov 10, 2022 •

edited

mrgrain commented Dec 7, 2022 •

edited