Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AwsCustomResource/EC2.SecurityGroup: can not add two Prefix Lists to an Egress Rule #13668

Closed
markilott opened this issue Mar 18, 2021 · 8 comments
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p1 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@markilott
Copy link

❓ General Issue

Trying to add prefix lists for S3 and DynamoDB Gateway Endpoints to a Security Group.

I am using a Custom Resource to lookup the Prefix List Id's so I can add them to Egress Rules (required as we can not allow all outbound for Lambda functions in production).

What I expected to happen:

Lookup Prefix Lists, and have both added to Egress Rules for the Security Group.

What actually happens:

Only the first Egress Rule specified is added.

Environment

  • CDK CLI Version: 1.89
  • Module Version: 1.89
  • Node.js Version: 12.18.2
  • Language: Javascript

To reproduce:

// Security Group
    const sg = new ec2.SecurityGroup(this, 'sg', {
      allowAllOutbound: false,
      vpc,
    });
    // Get Gateway Endpoint Prefix Lists from Custom Resource
    const prefixLists = new AwsCustomResource(this, 'prefixLists', {
      onUpdate: {
        service: 'EC2',
        action: 'describePrefixLists',
        parameters: {
          Filters: [
            {
              Name: 'prefix-list-name',
              Values: [`com.amazonaws.${this.region}.s3`, `com.amazonaws.${this.region}.dynamodb`],
            },
          ],
        },
        physicalResourceId: {},
      },
      policy: AwsCustomResourcePolicy.fromSdkCalls({ resources: AwsCustomResourcePolicy.ANY_RESOURCE }),
      logRetention: 1,
    });
    // check that tokens are created - working as expected
    console.log('prefix0: ', prefixLists.getResponseField('PrefixLists.0.PrefixListId')); // prefix0:  ${Token[TOKEN.xxx]}
    console.log('prefix1: ', prefixLists.getResponseField('PrefixLists.1.PrefixListId')); // prefix1:  ${Token[TOKEN.yyy]}

   // try to add both prefixes to the SG - only the first will be added
    sg.addEgressRule(ec2.Peer.prefixList(prefixLists.getResponseField('PrefixLists.0.PrefixListId')), ec2.Port.tcp(443), 'allow Gateway Endpoint access'); // created as expected
    sg.addEgressRule(ec2.Peer.prefixList(prefixLists.getResponseField('PrefixLists.1.PrefixListId')), ec2.Port.tcp(443), 'allow Gateway Endpoint access'); // not created

Other Information

I've tried a few different ways to do this but the result is always the same. I can see in CloudWatch that the Lambda is running and returning the correct data. The problem is that the second Egress Rule does not get created in the CloudFormation template.

If I manually add the Prefix List Id strings instead of using the CustomResource then both Egress Rules are created.

If I use two Security Groups and add one prefix to each using the CustomResource then both are created:

sg1.addEgressRule(ec2.Peer.prefixList(prefixLists.getResponseField('PrefixLists.0.PrefixListId')), ec2.Port.tcp(443), 'allow Gateway Endpoint access'); // created as expected
    sg2.addEgressRule(ec2.Peer.prefixList(prefixLists.getResponseField('PrefixLists.1.PrefixListId')), ec2.Port.tcp(443), 'allow Gateway Endpoint access'); // created as expected

The field order does not matter, the second specified is always omitted:

// reverse the order, same result
    sg.addEgressRule(ec2.Peer.prefixList(prefixLists.getResponseField('PrefixLists.1.PrefixListId')), ec2.Port.tcp(443), 'allow Gateway Endpoint access'); // created as expected
    sg.addEgressRule(ec2.Peer.prefixList(prefixLists.getResponseField('PrefixLists.0.PrefixListId')), ec2.Port.tcp(443), 'allow Gateway Endpoint access'); // not created

The issue seems to be that synth ignores a second rule when it is token based.

@markilott markilott added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Mar 18, 2021
@rix0rrr rix0rrr added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 and removed guidance Question that needs advice or information. labels Mar 22, 2021
@elemakil
Copy link

I am also affected by this issue. @markilott Did you come up with a viable workaround?

@markilott
Copy link
Author

@elemakil unfortunately the only workaround I found was to use one security group per prefix list.

I think it would also be possible to export the prefix lists in one stack them import them back in as strings in another stack, but I didn't try it.

@NGL321
Copy link
Contributor

NGL321 commented Jun 2, 2021

Hey @markilott,

Thank you for reporting this, I just did a little bit of quick research to see if I could find where the issue is. While I didnt find anything conclusive, it looks like the likely culprit is this block of code in the addEgressRule function of SecurtiyGroup:

// Skip duplicates
if (scope.node.tryFindChild(id) === undefined) {
new CfnSecurityGroupEgress(scope, id, {
groupId: this.securityGroupId,
...peer.toEgressRuleConfig(),
...connection.toRuleJson(),
description,
});
}

One of the devs on the team will look into a fix as time permits. In the interim, if you want to bump up the attention feel free to get more likes on this issue and we will increase its priority order.

😸 😷

@NGL321 NGL321 removed the needs-triage This issue or PR still needs to be triaged. label Jun 2, 2021
@rix0rrr rix0rrr assigned njlynch and unassigned rix0rrr Jun 3, 2021
@corymhall
Copy link
Contributor

@markilott can you let me know if this is still an issue for you? I believe this was fixed in #17221.

@corymhall corymhall added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 27, 2022
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jan 30, 2022
@github-actions github-actions bot closed this as completed Feb 4, 2022
@rustamrohit
Copy link

Workaround: assign Port.tcp(443) to one of egress rule and other with Port.tcpRange(443,443)

@SmoshySmosh
Copy link

This is still an issue, can we get this reopened?

@vumdao
Copy link

vumdao commented Oct 26, 2023

I use cdk v2.102.0 and don't face this issue

    //Egress
    for (let i = 0; i < 2; i++) {
      _sg.addEgressRule(
        Peer.prefixList(prefixLists.getResponseField(`PrefixLists.${i}.PrefixListId`)),
        Port.tcp(443),
        `Allow ${prefixLists.getResponseField(`PrefixLists.${i}.PrefixListName`)}`
      );
    }

    _sg.addEgressRule(
      Peer.ipv4(props.eksVpc.vpcCidrBlock),
      Port.allTraffic(),
      'Allow traffic out to CIDR block of VPC'
    );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p1 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

9 participants