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

(aws-ecs): Improve confusing Connections documentation/implementation #17269

Open
matsaune opened this issue Nov 2, 2021 · 6 comments · May be fixed by #31447
Open

(aws-ecs): Improve confusing Connections documentation/implementation #17269

matsaune opened this issue Nov 2, 2021 · 6 comments · May be fixed by #31447
Assignees
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. p2

Comments

@matsaune
Copy link

matsaune commented Nov 2, 2021

What is the problem?

Hi, I am using a ecsPatterns.NetworkLoadBalancedFargateService and tried to add a SecurityGroup in order to allow connections from NLB to fargate service. However this sg was not added as I expected to the fargateService.

Reproduction Steps

Define an ecsPatterns.NetworkLoadBalancedFargateService and add the following:

const securityGroupNotAdded = new ec2.SecurityGroup(this,"NblSecurityGroupNotAddedToService", { vpc: coreVpc, description: "Security group for connections from NLB to FargateService" }); securityGroupNotAdded.addIngressRule(ec2.Peer.ipv4(coreVpc.vpcCidrBlock),ec2.Port.tcp(80),"Allow from anyone in VPC on 80");
/** Somehow this does not get added. Seems like ony one sg allowed for fg service **/ loadBalancedFargateService.service.connections.addSecurityGroup(securityGroupNotAdded);

What did you expect to happen?

I expected this to be added to the SecurityGroups [] of the NetworkConfiguration of the Fargate service in order to allow connections from NLB.

What actually happened?

The sg is defined in cloudformation template but not registered in the SecurityGroups [] of the NetworkConfiguration of the Fargate service.

If this is a bug or a feature I do not know, but if feature and only allowed with one sg on fargate service it should be documented

CDK CLI Version

1.127.0 (build 0ea309a)

Framework Version

No response

Node.js Version

v16.4.2

OS

macOS 12.0.1 (21A559)

Language

Typescript

Language Version

Typescript Version 3.9.10

Other information

The workaround I use is as follows:

const securityGroup = loadBalancedFargateService.service.connections.securityGroups[0]; securityGroup.addIngressRule(ec2.Peer.ipv4(coreVpc.vpcCidrBlock),ec2.Port.tcp(80),"Allow from anyone in VPC on 80");

@matsaune matsaune added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 2, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Nov 2, 2021
@peterwoodworth peterwoodworth changed the title (@aws-cdk/aws-ecs-patterns): FargateService does not add SecurityGroup (aws-ecs-patterns): FargateService does not add SecurityGroup Nov 2, 2021
@peterwoodworth peterwoodworth removed the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Nov 2, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Nov 2, 2021
@peterwoodworth
Copy link
Contributor

This isn't the first time I've seen this bug report - check out #16117

I'm not sure how the connections object is intended to be used for this use case, so my theory posted in the original issue could be incorrect. Either way, this issue is confusing and our docs should be more clear if this isn't just a bug.

@madeline-k could you take a look at this?

@matsaune
Copy link
Author

matsaune commented Nov 7, 2021

Another more elegant workaround:
loadBalancedFargateService.service.connections.allowFrom( ec2.Peer.ipv4(coreVpc.vpcCidrBlock), ec2.Port.tcp(8080), )
Due to company policies (probably applies to more than us) we are not allowed to have open cidr blocks so must specify this.

@madeline-k
Copy link
Contributor

@peterwoodworth, I agree with what you said here. And this is the same scenario, just with a different construct. But I also agree this is confusing and we could consider either changing how the connections objects work on all constructs or changing the documentation.

@matsaune Thanks for opening this issue and providing the workaround!

@madeline-k madeline-k removed the needs-triage This issue or PR still needs to be triaged. label Jan 13, 2022
@madeline-k madeline-k removed their assignment Jan 13, 2022
@madeline-k madeline-k changed the title (aws-ecs-patterns): FargateService does not add SecurityGroup (aws-ecs-patterns): FargateService.connections.addSecurityGroup() does not add SecurityGroup to the synthed AWS::ECS::Service resource Jan 13, 2022
@madeline-k
Copy link
Contributor

Oh, and this is happening with the underlying FargateService construct the is created by the NetworkLoadBalancedFargateServicePattern. So the same issue will happen with both.

@madeline-k madeline-k changed the title (aws-ecs-patterns): FargateService.connections.addSecurityGroup() does not add SecurityGroup to the synthed AWS::ECS::Service resource (aws-ecs): FargateService.connections.addSecurityGroup() does not add SecurityGroup to the synthed AWS::ECS::Service resource Jan 13, 2022
@madeline-k madeline-k removed their assignment Jan 13, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Jan 13, 2022
@madeline-k madeline-k removed their assignment Jan 13, 2022
@github-actions
Copy link

This issue has not received any attention in 1 year. 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 13, 2023
@neilferreira
Copy link
Contributor

This is still an issue.

@peterwoodworth peterwoodworth removed the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 16, 2023
@peterwoodworth peterwoodworth changed the title (aws-ecs): FargateService.connections.addSecurityGroup() does not add SecurityGroup to the synthed AWS::ECS::Service resource (aws-ecs): Improve confusing Connections documentation/implementation Feb 16, 2023
@pahud pahud self-assigned this May 19, 2023
@pahud pahud added the p1.5 label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. p2
Projects
None yet
7 participants