feat(ec2): add helper method for creating intra-security-group traffic rules #5519

flemjame-at-amazon · 2019-12-22T21:08:24Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Helper method part of a security group that abstracts this code:

        ISecurityGroup sg = new SecurityGroup(construct, id, SecurityGroupProps.builder()
                .vpc(vpc)
                .build());
        Connections cn = new Connections(ConnectionsProps.builder()
                .securityGroups(Arrays.asList(sg))
                .build());
        sg.getConnections().allowTo(cn, Port.tcp(443));

and makes it into:

        ISecurityGroup sg = new SecurityGroup(construct, id, SecurityGroupProps.builder()
                .vpc(vpc)
                .build());
        sg.allowIntraSecurityGroupTraffic(cn, Port.tcp(443));

I created this because I spent a lot of time looking through documentation trying to allow security group members to talk to each other. Having a method that abstracts away the Connections object would eliminate figuring out how to accomplish this.

aws-cdk-automation · 2019-12-22T21:20:15Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-22T21:33:29Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T16:23:00Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T16:34:52Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T16:38:28Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T16:53:27Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T17:46:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T20:41:27Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-23T21:14:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

…azon/aws-cdk into add-intra-sg-method

aws-cdk-automation · 2019-12-23T22:26:02Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2019-12-24T08:41:44Z

Have you seen connections.allowInternally(port) ? https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.Connections.html#allow-wbr-internallyportrange-description

flemjame-at-amazon · 2019-12-25T17:31:39Z

Have you seen connections.allowInternally(port) ? https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-ec2.Connections.html#allow-wbr-internallyportrange-description

I actually made that change a few days ago, after checking out the Connections documentation.

rix0rrr · 2019-12-27T09:40:06Z

I actually made that change a few days ago, after checking out the Connections documentation.

Okay, but you have to talk me through the value proposition that is being offered in this PR. As far as I can tell, it's wrapper methods to do what can already be achieved. Presumably you would like this merged because in your opinion will be easier to find/more discoverable.

Can you share some motivation/justification on why you think the current methods are not discoverable and the ones you've added will be better, AND better for the majority of users?

rix0rrr

Marking this as "changes requested" so it will disappear from my TODO list. Please re-request a review using the GitHub interface when ready.

flemjame-at-amazon · 2020-01-02T18:32:39Z

I actually made that change a few days ago, after checking out the Connections documentation.

Okay, but you have to talk me through the value proposition that is being offered in this PR. As far as I can tell, it's wrapper methods to do what can already be achieved. Presumably you would like this merged because in your opinion will be easier to find/more discoverable.

Can you share some motivation/justification on why you think the current methods are not discoverable and the ones you've added will be better, AND better for the majority of users?

Re: discoverability -- if I am looking to create rules for a Security Group, the first step is to go to the Security Group documentation. On that page, I see methods for "addIngressRule" and "addEgressRule", which sounds exactly like what I want.

public addEgressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean): void

public addIngressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean): void

I go to the Peer page to check the syntax for adding a Security Group, but there is no method for doing so. I only see options for ipv4/ipv6 CIDRs and prefix lists. At this point, I am not sure where to turn, so I start searching Google for the answer.

Ideally, there would be a Peer for a security group, however when I tried implementing such a thing I ran into an error:

Exception in thread "main" software.amazon.jsii.JsiiException: Cannot use tokens in construct ID: to ${Token[Default.PrefixListID.350]}:443
Error: Cannot use tokens in construct ID: to ${Token[Default.PrefixListID.350]}:443
    at new ConstructNode (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/core/lib/construct.js:39:19)
    at new Construct (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/core/lib/construct.js:435:21)
    at new CfnElement (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/core/lib/cfn-element.js:20:9)
    at new CfnRefElement (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/core/lib/cfn-element.js:95:1)
    at new CfnResource (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/core/lib/cfn-resource.js:21:9)
    at new CfnSecurityGroupEgress (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/aws-ec2/lib/ec2.generated.js:4729:9)
    at SecurityGroup.addEgressRule (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/aws-ec2/lib/security-group.js:43:13)
    at SecurityGroup.addEgressRule (/private/var/folders/h1/l30xn7q95cg89_8vtpbcx4nr06cc3p/T/jsii-kernel-2NdZz1/node_modules/@aws-cdk/aws-ec2/lib/security-group.js:199:19)

Looking at the implementation of SecurityGroup, it looks like it uses the Peer as an input to compute the construct id for ingress/egress rules. And since the construct id is used to compute the logical resource name, that can’t depend on a ref. Changing this is definitely NOT backwards compatible, so I scrapped that idea.

The right way to add an inter or intra-SG rule is to use the SecurityGroupIngress and SecurityGroupEgress resources, per AWS documentation. Connections will do this for you, but it isn't advertised at all on the Security Group documentation. Searching for "Connections" on the Security Group documentation brings up the property "connections" but there is no documentation on what it is or does.

I saw two options: to increase the visibility of the Connections class and how to use it for common cases, or abstract it away in the Security Group class. I prefer the abstraction because it provides a quick method for a CDK user to add an intra-security group rule, which in my experience is a common use case, and which is visible from the Security Group page, which is where I would expect to find it.

For people already familiar with the inner workings of Security Groups, our `.connections` pattern is a little confusing. Add some more verbiage to the documentation which points people in the right direction with respect to security group manipulation. Closes #5519.

rix0rrr · 2020-01-06T14:05:13Z

I tried improving the docs you searched in this PR: #5662 to hopefully make it easier for you and others to find the right methods.

I hope to explain the philosophy using these sentences:

Like IAM Roles, Security Groups need to exist to control access between AWS resources, but CDK will automatically generate and populate them with least-privilege permissions for you so you can concentrate on your business logic.

After construction, you can selectively allow connections to and between constructs via--for example-- the instance.connections object. Think of it as "allowing connections to your instance", rather than "adding ingress rules a security group".

flemjame-at-amazon · 2020-01-06T14:15:36Z

I tried improving the docs you searched in this PR: #5662 to hopefully make it easier for you and others to find the right methods.

I hope to explain the philosophy using these sentences:

Like IAM Roles, Security Groups need to exist to control access between AWS resources, but CDK will automatically generate and populate them with least-privilege permissions for you so you can concentrate on your business logic.

After construction, you can selectively allow connections to and between constructs via--for example-- the instance.connections object. Think of it as "allowing connections to your instance", rather than "adding ingress rules a security group".

OH! It makes so much more sense after reading PR #5662. That's a really nice way to abstract security groups from CDK users.

flemjame-at-amazon · 2020-01-06T19:32:39Z

I tried improving the docs you searched in this PR: #5662 to hopefully make it easier for you and others to find the right methods.

I hope to explain the philosophy using these sentences:

Like IAM Roles, Security Groups need to exist to control access between AWS resources, but CDK will automatically generate and populate them with least-privilege permissions for you so you can concentrate on your business logic.

After construction, you can selectively allow connections to and between constructs via--for example-- the instance.connections object. Think of it as "allowing connections to your instance", rather than "adding ingress rules a security group".

How does this work for things not managed with a security group, like Network Load Balancers? I only see options like "allowFromAnyIpv4", which isn't ideal, and a better would be something like "allowFromVpcIpv4"

Pull request has been modified.

aws-cdk-automation · 2020-01-06T20:17:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2020-01-07T09:10:16Z

How does this work for things not managed with a security group, like Network Load Balancers? I only see options like "allowFromAnyIpv4", which isn't ideal, and a better would be something like "allowFromVpcIpv4"

You can always make a Peer.ipv4(vpc.cidrRange) if you need that. We need to think about the right way to do this. The problem is that if you say instance.connections.allowFrom(vpc), we can create the ingress rule with the CIDR, but we can't update all SGs of instances in the VPC to allow outbound connections. For default SGs (allowAllOutbound=true) that doesn't matter, but I don't really like that it won't work for locked-down SGs.

For people already familiar with the inner workings of Security Groups, our `.connections` pattern is a little confusing. Add some more verbiage to the documentation which points people in the right direction with respect to security group manipulation. Closes #5519. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

flemjame-at-amazon added 8 commits December 22, 2019 03:52

Add feature and basic test

fa44f08

Expand on test

a1bf5d9

Fix whitespace

0819d2c

Add SecurityGroup peer and expand intra-sg test

5e05c67

Fix test with CDK magic strings

679d9a3

More precise test

a74b678

Security groups as peers is problematic

3a0187c

Change test names

370bd1b

flemjame-at-amazon requested a review from rix0rrr as a code owner December 22, 2019 21:08

Merge branch 'master' into add-intra-sg-method

ac871cb

Add inter-security group static helper method

042ba44

flemjame-at-amazon and others added 3 commits December 23, 2019 16:24

Accidentally overwrote an intermediate commit someone else made

dd5e81b

Newline at end

370d50e

Merge branch 'master' into add-intra-sg-method

393521f

Merge branch 'master' into add-intra-sg-method

a85c054

flemjame-at-amazon and others added 2 commits December 23, 2019 20:08

Found a native Connections method for doing this

a858674

Merge branch 'master' into add-intra-sg-method

04478c4

SomayaB assigned rix0rrr Dec 23, 2019

flemjame-at-amazon added 2 commits December 23, 2019 21:51

Add test for inter-SG rules

3a7405f

Merge branch 'add-intra-sg-method' of ssh://github.com/flemjame-at-am…

1a54802

…azon/aws-cdk into add-intra-sg-method

rix0rrr previously requested changes Jan 2, 2020

View reviewed changes

flemjame-at-amazon requested a review from rix0rrr January 2, 2020 18:32

rix0rrr mentioned this pull request Jan 6, 2020

docs(ec2): improve SecurityGroup documentation #5662

Merged

Merge branch 'master' into add-intra-sg-method

c35ef4b

mergify bot closed this in #5662 Jan 7, 2020

flemjame-at-amazon deleted the add-intra-sg-method branch September 2, 2020 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): add helper method for creating intra-security-group traffic rules #5519

feat(ec2): add helper method for creating intra-security-group traffic rules #5519

flemjame-at-amazon commented Dec 22, 2019

aws-cdk-automation commented Dec 22, 2019

aws-cdk-automation commented Dec 22, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

aws-cdk-automation commented Dec 23, 2019

rix0rrr commented Dec 24, 2019

flemjame-at-amazon commented Dec 25, 2019

rix0rrr commented Dec 27, 2019

rix0rrr left a comment

flemjame-at-amazon commented Jan 2, 2020 •

edited

Loading

rix0rrr commented Jan 6, 2020

flemjame-at-amazon commented Jan 6, 2020

flemjame-at-amazon commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

rix0rrr commented Jan 7, 2020

feat(ec2): add helper method for creating intra-security-group traffic rules #5519

feat(ec2): add helper method for creating intra-security-group traffic rules #5519

Conversation

flemjame-at-amazon commented Dec 22, 2019

aws-cdk-automation commented Dec 22, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 22, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 23, 2019

AWS CodeBuild CI Report

rix0rrr commented Dec 24, 2019

flemjame-at-amazon commented Dec 25, 2019

rix0rrr commented Dec 27, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

flemjame-at-amazon commented Jan 2, 2020 • edited Loading

rix0rrr commented Jan 6, 2020

flemjame-at-amazon commented Jan 6, 2020

flemjame-at-amazon commented Jan 6, 2020

aws-cdk-automation commented Jan 6, 2020

AWS CodeBuild CI Report

rix0rrr commented Jan 7, 2020

flemjame-at-amazon commented Jan 2, 2020 •

edited

Loading