feat(wrappers): resolve load balancer hostnames to A/AAAA records#6289
Conversation
|
Hi @Apoorva64. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@ivankatliarchuk i've looked more into it and i've implemented the annotation for the Ingress and Gateway Resources. |
| - hostname: example.com | ||
| expected: | ||
| - dnsName: svc.example.com | ||
| targets: ["104.18.26.120", "104.18.27.120"] |
There was a problem hiding this comment.
If they may change we have a problem. The IP should be fairly static, otherwise we end-up with fragile tests
There was a problem hiding this comment.
moved to checking if the Record types A and AAAA are present instead of checking the direct ips. The example domain is owned by iana so it should be stable https://www.iana.org/help/example-domains
|
|
||
| ### Use Cases for `external-dns.alpha.kubernetes.io/resolve-target` annotation | ||
|
|
||
| #### Opt in to resolution for a single Ingress |
There was a problem hiding this comment.
I'm still struggling a bit. Could you share actually WHY we need resolve IP for a CNAME? Opt in to resolution for a single Ingress or Opt out of resolution for a single Gateway Route -> a not a use cases` in normal sense.
There was a problem hiding this comment.
Yep,
In a public cloud provider, when we create a Gateway and an Http Route or an Ingress we usually end up with something like this:
When we create a Gateway we end up with a hostname in the status which points to the loadbalancer's ips.

The loadbalancer IPs are then routed in an infrastructure where we can't resolve external Records like example-gateway.region.mycloud.com.
If we create a CNAME my-app.internal to example-gateway.region.mycloud.com it won't work as the app my-second-app won't be able to resolve the example-gateway.region.mycloud.com Record
We must then create an A Record in the air-gapped infrastructure instead of a CNAME.

But :
- not all apps are equal and some will be able to resolve the CNAME of
example-gateway.region.mycloud.com
which is why it would be useful to have the control over the annotations. - Most of the apps won't be able to resolve the CNAME Record and this is why we may need a global flag
|
|
||
| // Propagate the per-resource annotation as a label so the PostProcessor can | ||
| // apply or suppress hostname resolution on a per-Ingress basis. | ||
| if v, ok := ing.Annotations[annotations.ResolveTargetKey]; ok { |
There was a problem hiding this comment.
A good idea, need to re-think our approach long-term. At the moment just bootstrap it as provider-specific https://github.com/kubernetes-sigs/external-dns/blob/master/source/annotations/provider_specific.go and it should be available at wrapper layer without any code changes at source layer.
| cfg PostProcessorConfig | ||
| } | ||
|
|
||
| type PostProcessorConfig struct { |
There was a problem hiding this comment.
use dedupwrapper. This is already to late, as dedup-wrapper may drop endpoints
There was a problem hiding this comment.
Instead of modifying the dedupwrapper i wrote a new 'resolveSource' wrapper that handles the hostname resolving.
I've put the wrapper before the dedupwrapper
There was a problem hiding this comment.
Should i still use the dedupwrapper? i think i saw i message yesterday about it
There was a problem hiding this comment.
Yeah. I deleted the message. Not yet sure. One one side it make sense to have dedicaetd wrapper, on the other side too many wrappers, not too clear yet
| | `--kubeconfig=""` | Retrieve target cluster configuration from a Kubernetes configuration file (default: auto-detect) | | ||
| | `--request-timeout=30s` | Request timeout when calling Kubernetes APIs. 0s means no timeout | | ||
| | `--[no-]resolve-service-load-balancer-hostname` | Resolve the hostname of LoadBalancer-type Service object to IP addresses in order to create DNS A/AAAA records instead of CNAMEs | | ||
| | `--[no-]resolve-load-balancer-hostname` | Resolve the hostname of LoadBalancer addresses in source status to IP addresses in order to create DNS A/AAAA records instead of CNAMEs | |
There was a problem hiding this comment.
I'm not too-sure about flag at this stage. Probably worth to plan it as follow-up if this PR will get merged
There was a problem hiding this comment.
With the description of the use-case in #6289 (comment). Should i remove the flag ?
There was a problem hiding this comment.
I think the better option is to add/remove/modify flags in follow up PR.
| - hostname: example.com | ||
| expected: | ||
| - dnsName: svc.example.com | ||
| targets: ["104.18.26.120", "104.18.27.120"] |
There was a problem hiding this comment.
If they may change we have a problem. The IP should be fairly static, otherwise we end-up with fragile tests
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/ok-to-test |
Pull Request Test Coverage Report for Build 23438696528Details
💛 - Coveralls |
… using post processor wrapper
|
/ok-to-test |
|
@ivankatliarchuk, is there something to change that I missed or another requirement? |
|
Yeah something strange with this PR. Our tests not triggered. /ok-to-test |
|
/close |
|
@ivankatliarchuk: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/reopen |
|
@ivankatliarchuk: Reopened this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
I think this PR is screwed |
|
Should i close and reopen with a new branch name ? PS: Does External Dns have community meetings ? I've been looking around for it but i can't find it xD |
|
We don't hold any community meetings. There is slack channel, but we not too active there as well. Try to open a new PR. I seriously do not get what is going on here. |
What does this PR do?
This Pr adds per-resource annotation:
external-dns.alpha.kubernetes.io/resolve-target. Setting this annotation totruecauses ExternalDNS to resolve any CNAME target (i.e. a hostname returned by a cloud load balancer) to its A and AAAA records at sync time, and emit those IP addresses as A/AAAA endpoints instead. DNS resolution uses net.LookupIP and honours the system resolver.Implementation
Resolution is implemented as a Source Wrapper in resolvesource.go, wrapping any source. Each source propagates the annotation value as an endpoint label using provider_specific.go; the resolvesource wrapper reads the label and resolves the hostname
If resolution fails for a target (e.g. the hostname is temporarily unresolvable), that target is silently skipped and a debug log entry is emitted.
Motivation
Some DNS providers or configurations do not support
CNAMErecords (e.g., internal Dns), requiring IP-based records.More