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-patterns): (ApplicationLoadBalancedFargateService has publicLoadBalancer set to True by default but should be False) #31344

Closed
2 tasks
yatri1306 opened this issue Sep 6, 2024 · 2 comments
Assignees
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@yatri1306
Copy link

Describe the feature

ApplicationLoadBalancedFargateService construct creates an internet facing load balancer by default because prop publicLoadBalancer is by default set to true. publicLoadBalancer should be false by default as it is an optional parameter.

Use Case

If ApplicationLoadBalancedFargateService is a construct for everyone to use, why is publicLoadBalancer is by default set to true? This creates an internet facing load balancer by default for all users and risks exposing endpoints to the internet which can cause security issues. Even if users define the VPC prop, an internet facing load balancer is created. The default value for publicLoadBalancer should be set to false to allow users to create more secure, internal load balancers.

Proposed Solution

No response

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.156.0

Environment details (OS name and version, etc.)

All

@yatri1306 yatri1306 added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Sep 6, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Sep 6, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 6, 2024
@khushail khushail self-assigned this Sep 6, 2024
@khushail
Copy link
Contributor

khushail commented Sep 6, 2024

Hi @yatri1306 , thanks for reporting this. Sharing my observation here -

The patterns are designed to be well constructed services with opinionated defaults. So it seems to be usecase dependent when implemented.

In the CDK docs, it states the property publicLoadBalancer as optional and sets the default:true - https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ecs_patterns.ApplicationLoadBalancedFargateService.html#publicloadbalancer.

In the code, setting this to true, makes the internetfacing true -

const internetFacing = props.publicLoadBalancer ?? true;

which confirns what you are saying is true. I also tried to repro this by code sample and this is what the synthesized template looks like -

    const patterns = new ecspatterns.ApplicationLoadBalancedFargateService(this, 'PatternsIssue', {
      cpu: 512,
      memoryLimitMiB: 1024,
      taskImageOptions: {
        image: ecs.ContainerImage.fromRegistry('amazon/amazon-ecs-sample'),
      },
    });

Snippet of synthesized template which creates the public load balancer with internetfacing scheme-

{
 "Resources": {
  "PatternsIssueLB1BAE96F8": {
   "Type": "AWS::ElasticLoadBalancingV2::LoadBalancer",
   "Properties": {
    "LoadBalancerAttributes": [
     {
      "Key": "deletion_protection.enabled",
      "Value": "false"
     }
    ],
    "Scheme": "internet-facing",
    "SecurityGroups": [
     {
      "Fn::GetAtt": [
       "PatternsIssueLBSecurityGroup55B426A8",
       "GroupId"
      ]
     }
    ],

However the AWS doc mentions that a default internet-facing load balancer would be created, ref below -

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-elasticloadbalancingv2-loadbalancer.html#cfn-elasticloadbalancingv2-loadbalancer-scheme

Scheme

    The nodes of an Internet-facing load balancer have public IP addresses. The DNS name of an Internet-facing load balancer is publicly resolvable to the public IP addresses of the nodes. Therefore, Internet-facing load balancers can route requests from clients over the internet.

    The nodes of an internal load balancer have only private IP addresses. The DNS name of an internal load balancer is publicly resolvable to the private IP addresses of the nodes. Therefore, internal load balancers can route requests only from clients with access to the VPC for the load balancer.

    **The default is an Internet-facing load balancer.**

    You cannot specify a scheme for a Gateway Load Balancer.

    Required: No

    Type: String

    Allowed values: internet-facing | internal

So if I understood it correctly, it seems that its done with purpose and am not really sure if changing the default should be done but since this is opinionated, we are open to feedback from the community. Please feel free to share your thoughts on this if something is misinterpreted

@khushail khushail added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Sep 6, 2024
Copy link

github-actions bot commented Sep 9, 2024

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 the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 9, 2024
@github-actions github-actions bot added 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 Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 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

2 participants