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

ARecord is created even with publicLoadBalancer is set to false in ApplicationLoadBalancedFargateService #6702

Closed
phaniram opened this issue Mar 12, 2020 · 9 comments · Fixed by #6895
Assignees
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library bug This issue is a bug. p2

Comments

@phaniram
Copy link

Even when the 'publicLoadBalancer' is set to false, ApplicationLoadBalancedFargateService still creates an ARecord with loadbalancer as target.

I don't see any reason to have an ARecord when the loadbalancer is private.

Reproduction Steps

    // CloudFormation Resources
    this.service = new ecsPatterns.ApplicationLoadBalancedFargateService(this, 'Service', {
        .......
        .......
        .......
        publicLoadBalancer: false,
        protocol: elbv2.ApplicationProtocol.HTTPS,
        domainName: `abc.com`,
        domainZone: serviceHostedZone,
        .......
        .......
        .......

    });

Error Log

Environment

  • CLI Version :
  • Framework Version:
  • OS :
  • Language :

Other

It doesn't make sense to create an ARecord which never gets resolved when the 'publicLoadBalancer' is set false. Ideally record creation can be optional or atleast do it only when publicLoadBalancer is set true.


This is 🐛 Bug Report

@phaniram phaniram added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 12, 2020
@SomayaB SomayaB added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Mar 12, 2020
@hencrice hencrice added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Mar 18, 2020
@hencrice
Copy link
Contributor

Marked this as p2 bug because it indeed seems unnecessary but would't impact the functionalities.

@hencrice hencrice assigned hencrice and unassigned uttarasridhar Mar 18, 2020
@mergify mergify bot closed this as completed in #6895 Mar 20, 2020
mergify bot pushed a commit that referenced this issue Mar 20, 2020
* fix(aws-ecs-patterns): only create an A record if LB is public

Before this change, we unnecessarily create A record when the load balancer(s) is non public-facing. This commit fixes #6702.

* Added tests

* Moved these 2 lines back to where they were
@hoegertn
Copy link
Contributor

I do not really understand why an A record needs to be a public endpoint? What about internal LBs with internal hostnames? Or am I missing something obvious?

@hencrice
Copy link
Contributor

Thanks @hoegertn for bringing it up. I pondered on this issue for quite a while as well due to the possible use case you mentioned.

Please correct me if I'm wrong. If an A record is created via Cloudformation with a private ip (i.e. private LBs), then it would be a public DNS record with a non-resolvable ip unless you are inside the VPC. Also, after some preliminary experiments, I found that a private LB automatically gets a DNS name without any A record.

After some discussions within the team, we deemed that to be an uncommon use case. But please let us know if my understanding of the issue is seriously wrong :)

@hoegertn
Copy link
Contributor

If it is public or not depends on the type of hosted zone you are using. But even if it is public this is not a real issue. I don't think hiding IPs is a security thing.

The DNS name the LB gets is an AWS one, but most of the time I want to use a vanity name to reach this for example via VPN/DirectConnect from on-prem. I don't see a reason why internal LBs should not have DNS records in my hosted zones.

Additionally, I think the ECS patterns package is marked stable so breaking changes, and this is one, are not allowed imho.

@hencrice
Copy link
Contributor

I see. Let me quickly bring this back to the team and revert the commit if necessary. Thanks for bringing this up!

@hencrice
Copy link
Contributor

Reverting: #6987

Thanks again @hoegertn for bringing this up!

@hencrice
Copy link
Contributor

hencrice commented Apr 6, 2020

@phaniram would you be interested to see #7206 implemented? Will the proposed flag satisfy your use cases?

@phaniram
Copy link
Author

@hencrice Yes. If possible eligibility to override too would work.

@piradeepk
Copy link
Contributor

piradeepk commented May 3, 2020

I don't think we should be making everything configurable in the patterns (the patterns are designed to be well constructed services with opinionated defaults). As this doesn't appear to be a common usecase, and the benefits don't outweigh the complexity, I'm of the opinion that we shouldn't add this flag as it would add unnecessary complexity to the constructs themselves. The patterns will start to become unmaintainable/unusable if everything is added as a configuration.

A workaround is to implement the pattern using L2s and remove the logic that creates the ARecord. Feel free to reopen and let me know if you have any further questions around how to do this with L2s.

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 bug This issue is a bug. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants