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

feat: resolve LB-type Service hostname to create A/AAAA instead of CNAME #3554

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

cxuu
Copy link
Contributor

@cxuu cxuu commented Apr 19, 2023

Description

This PR adds an optional feature (turned off by default).

On EKS for example, the LoadBalancer-type Service object will not have external IP addresses but external hostnames. In this case, external-dns will always create an Endpoint (an external-dns concept, see its go struct) whose RecordType is CNAME. But the user might want to create A or AAAA records instead, and perhaps on a different DNS provider (e.g. Cloudflare) than the cluster provider (e.g. AWS).

This PR adds an optional feature that allows external-dns to resolve the LB hostname into IP addresses before creating the Endpoint struct.

Fixes #ISSUE

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 19, 2023
@cxuu cxuu marked this pull request as ready for review April 19, 2023 04:26
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2023
@k8s-ci-robot k8s-ci-robot requested a review from seanmalloy April 19, 2023 04:26
@nefelim4ag
Copy link
Contributor

I'm sorry, I can't seem to get what you are trying to solve here.
External-DNS can & will create CNAME on HostName load balancer, even if this is a different provider, from where Load Balancer Host it just will be CNAME.

On AWS much better solution is Aliases. About resolving - the only place where it "can" work as duct tape - with domain/zone name. Because we can not create CNAME Record example.com in Zone example.com.

Thanks!

@reltuk
Copy link

reltuk commented Apr 19, 2023

FWIW, I think this PR is trying to solve the exact same issue as #2050.

@reltuk
Copy link

reltuk commented Apr 19, 2023

FWIW, I think this PR is trying to solve the exact same issue as #2050.

Never mind. I initially though the approach was related since I saw EKS, LoadBalancer-type Service and AAAA. While it may have been tangentially related, it doesn't seem to be solving the same problem.

@cxuu
Copy link
Contributor Author

cxuu commented Apr 19, 2023

@nefelim4ag Thanks for taking a look!

My specific use case is that I want to create Cloudflare DNS records for AWS NLB. The problem I ran into is that Cloudflare does not allow CNAME for us-west-2.app.example.com in the DNS zone app.example.com. External-DNS was trying to create a CNAME for us-west-2.app.example.com that maps to somethingsomething.elb.us-west-2.amazonaws.com.

With this PR, the workaround is to not create CNAME but A/AAAA records by first resolving somethingsomething.elb.us-west-2.amazonaws.com to IP addresses.

The feature flag I added is meant to be a generic solution regardless of which DNS providers the user want. Thus, I instrumented the DNS resolution in the serviceSource struct.

@benedikt-bartscher
Copy link

benedikt-bartscher commented Apr 23, 2023

Hey @cxuu thanks for your PR. This would solve my problem with hetzner where i have to set the "load-balancer.hetzner.cloud/hostname: " annotation to fix some issues (see hetznercloud/hcloud-cloud-controller-manager#160). This causes external-dns to create CNAME records instead of A records which does not work for apex:
RRSet of type CNAME with DNS name ****.de. is not permitted at apex in zone ****.de.

Edit: This PR currently only implements the dns resolution for Services, for fixing my issue with Hetzner i would need the same for Ingresses.

@benedikt-bartscher
Copy link

benedikt-bartscher commented Apr 23, 2023

@cxuu i just build an image for this PR and i found one bug. You named the argument "resolve-lb-hostname" in go but "resolve-load-balancer-hostname" in the helm chart.

I would also consider either renaming the argument to resolve-service-load-balancer-hostname or adjust the behaviour so it works for ingresses to. Antoher option would be to add 2 arguments for enabling this seperatly for services/ingresses but i do not see a usecase for this.

@cxuu
Copy link
Contributor Author

cxuu commented Apr 23, 2023

@benedikt-bartscher Thank you for the bug report! I have fixed it in 5e6f1a8. I have also renamed the option to resolveServiceLoadBalancerHostname to indicate that it only works for Service. I imagine the code change for Ingress will look similar but personally I would prefer to add Ingress support in a second PR to make this PR smaller and reviewable.

@cxuu cxuu force-pushed the resolve-lb-hostname branch from 1cf6530 to 5e6f1a8 Compare April 23, 2023 23:25
@szuecs
Copy link
Contributor

szuecs commented Apr 24, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 24, 2023
@szuecs
Copy link
Contributor

szuecs commented Apr 24, 2023

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cxuu, szuecs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2023
@k8s-ci-robot k8s-ci-robot merged commit 305c576 into kubernetes-sigs:master Apr 24, 2023
@cxuu cxuu deleted the resolve-lb-hostname branch April 24, 2023 18:51
@stevehipwell
Copy link
Contributor

@szuecs sorry I wasn't able to review this with you but I don't usually accept combined merges with code changes and Helm chart config.

@cxuu we don't accept flags as structure inputs unless they're required so I'm going to open a PR to remove the submitted Helm chart changes. Please use extraArgs for this purpose.

@benedikt-bartscher
Copy link

benedikt-bartscher commented Apr 26, 2023

I would think about writing a seperate function which does the DNS Lookups. This function could then be used for both, Service and Ingress Resources. We could also implement a configureable timeout for the DNS Lookups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants