search

openshift / external-dns-operator

The ExternalDNS Operator provides simplified ExternalDNS controller management.
Apache License 2.0
22 stars 33 forks source link

OCPBUGS-819: Azure provider - add wildcard replacement for TXT records #171

Closed alebedev87 closed 1 year ago

alebedev87 commented 1 year ago

Since the addition of the new format of TXT records in external-dns 0.12.0 the TXT records are prepended with the type of the DNS record which doesn't comply with the standards of Azure DNS in case of the wildcards: the wildcard symbol (*) has to be the leftmost one.

Related issue: https://github.com/kubernetes-sigs/external-dns/issues/2922

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

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: - ~~[OWNERS](https://github.com/openshift/external-dns-operator/blob/main/OWNERS)~~ [alebedev87] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
alebedev87 commented 1 year ago

Infoblox temporary licences had to be renewed

/retest

gcs278 commented 1 year ago

/assign

candita commented 1 year ago

@alebedev87 please add a JIRA so this can be tracked and tested.

candita commented 1 year ago

/assign @Miciah

openshift-ci-robot commented 1 year ago

@alebedev87: This pull request references Jira Issue OCPBUGS-819, which is invalid:

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/external-dns-operator/pull/171): >Since the addition of the new format of TXT records in external-dns `0.12.0` the TXT records are prepended with the type of the DNS record which doesn't comply with the standards of Azure DNS in case of the wildcards: the wildcard symbol (`*`) has to be the leftmost one. > >Related issue: https://github.com/kubernetes-sigs/external-dns/issues/2922 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
alebedev87 commented 1 year ago

/jira refresh

openshift-ci-robot commented 1 year ago

@alebedev87: This pull request references Jira Issue OCPBUGS-819, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.12.0) matches configured target version for branch (4.12.0) * bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)
In response to [this](https://github.com/openshift/external-dns-operator/pull/171#issuecomment-1234479344): >/jira refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
Miciah commented 1 year ago

What happens when a route or service with the host name "*.foo" exists and another route or service with the host name "ws.foo" is created? What happens in the reverse case: something with "ws.foo" exists and something with "*.foo" is created?

CoreDNS and SkyDNS treat "any" the same as "*", so we could use that instead of "ws", but "any" isn't standard. It doesn't seem like there is any value we can use in lieu of "ws" that wouldn't risk causing conflicts.

alebedev87 commented 1 year ago

What happens when a route or service with the host name ".foo" exists and another route or service with the host name "ws.foo" is created? What happens in the reverse case: something with "ws.foo" exists and something with ".foo" is created?

A conflict of the TXT record name would happen, in both cases. So, external-dns would reject the creation of the conflicting TXT record which would prevent the creation of the "target" DNS record too.

CoreDNS and SkyDNS treat "any" the same as "*", so we could use that instead of "ws", but "any" isn't standard. It doesn't seem like there is any value we can use in lieu of "ws" that wouldn't risk causing conflicts.

Even if it's not standard, any is used in at least 1 implementation and overall looks more educated than just the acronym (wc stands for wildcard). This won't help with the conflict described above but may be considered more intuitive at least.

openshift-ci[bot] commented 1 year ago

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
gcs278 commented 1 year ago

Not sure if I completely understand this one. Say I want to make a wildcard TXT record for grant-*.example.com in azure, so that I can query grant-a.example.com and grant-b.example.com.

How can I still query grant-a.example.com if the --txt-wildcard-replacement changes it to grant-any.example.com? Am I misunderstanding what field --txt-wildcard-replacement is operating on? Does the wildcard query still work?

alebedev87 commented 1 year ago

@gcs278 : Thanks for having a look!

Does the wildcard query still work?

Yes, the wildcard itself is not impacted by --txt-wildcard-replacement. It would still remain with the asterisk.

--txt-wildcard-replacement replaces the asterisk character for the corresponding TXT record. ExternalDNS Operator uses the TXT registry of the upstream ExternalDNS to track the ownership of the DNS records (A, CNAME for the moment). So each A or CNAME record is paired with a TXT record which contains the owner ID (unique name of externaldns instance). The --txt-wildcard-replacement flag impact only these TXT records.

Miciah commented 1 year ago

Not sure if I completely understand this one. Say I want to make a wildcard TXT record for grant-*.example.com in azure, so that I can query grant-a.example.com and grant-b.example.com.

How can I still query grant-a.example.com if the --txt-wildcard-replacement changes it to grant-any.example.com? Am I misunderstanding what field --txt-wildcard-replacement is operating on? Does the wildcard query still work?

Per https://www.rfc-editor.org/rfc/rfc4592#section-2.1.1, the entire first label must be an asterisk for the record to be a wildcard record. Accordingly, external-dns only substitutes (source) or matches (source) for "*" if it is equal to the entirety of the first label.

gcs278 commented 1 year ago

Oh okay, now I'm starting to understand. External DNS is creating TXT records to track what other DNS records it owns. And azure is rejecting TXT records with * not in the left most position. This fix just substitutes * with other characters.

But one thing I'm still confused about, is how/why a record such as a-*.example.com gets created, like in the issue. Isn't that already an invalid wildcard domain? Is someone purposely creating this type of record or is External DNS creating this a-*.example.com record in it's TXT registry logic?

Miciah commented 1 year ago

But one thing I'm still confused about, is how/why a record such as a-*.example.com gets created, like in the issue. Isn't that already an invalid wildcard domain? Is someone purposely creating this type of record or is External DNS creating this a-*.example.com record in it's TXT registry logic?

Yeah, a-*.example.com is invalid, or at least imprudent and not preferred, per https://www.rfc-editor.org/rfc/rfc1035#section-2.3.1. DNS might allow it, but I'd expect its use as a host name to be rejected. I'm surprised that --txt-wildcard-replacement resolved that issue.

alebedev87 commented 1 year ago

Yeah, a-*.example.com is invalid, or at least imprudent and not preferred, per https://www.rfc-editor.org/rfc/rfc1035#section-2.3.1. DNS might allow it, but I'd expect its use as a host name to be rejected. I'm surprised that --txt-wildcard-replacement resolved that issue.

a-*.example.com is the domain name for the TXT record only, the corresponding DNS record would look like *.example.com. a- is the default prefix added to TXT records by external dns since the version 0.12.0.

gcs278 commented 1 year ago

a-.example.com is the domain name for the TXT record only, the corresponding DNS record would look like .example.com. a- is the default prefix added to TXT records by external dns since the version 0.12.0.

Okay - that makes more sense now. I was curious why this was even a problem (aka let's just stop making invalid records). But now I know External DNS is making them.

Okay yikes. Sounds like this is our best path forward at the moment, but @Miciah does bring up a good point about naming collisions. Correct me if I'm wrong, but you can't have *.example.com and any.example.com co-exist with External DNS on Azure with this PR.

I'm good with this, but I feel hesitant, so @Miciah could you /lgtm (since it's already approved, it'll merge after one lgtm).

alebedev87 commented 1 year ago

Correct me if I'm wrong, but you can't have *.example.com and any.example.com co-exist with External DNS on Azure with this PR.

You are right, @gcs278. These 2 records won't be able to be served by ExternalDNS on Azure. This is a limitation (not only in this downstream but upstream too) which I think is preferable over the regression in the support of the wildcard record which we are currently having.

Miciah commented 1 year ago

You are right, @gcs278. These 2 records won't be able to be served by ExternalDNS on Azure. This is a limitation (not only in this downstream but upstream too) which I think is preferable over the regression in the support of the wildcard record which we are currently having.

Please make sure to address this caveat with a release note.

/lgtm

alebedev87 commented 1 year ago

/assign @quarterpin

alebedev87 commented 1 year ago

/assign @xenolinux

Note this comment: https://github.com/openshift/external-dns-operator/pull/171#issuecomment-1258844722

We need to mention the fact that wildcard TXT records get modified: asterisk is replaced with any on AzureDNS. And this may cause a conflict with records starting with any. So any prefixed records are to be avoided on Azure.

alebedev87 commented 1 year ago

/assign @lihongan

lihongan commented 1 year ago

/label qe-approved

xenolinux commented 1 year ago

/label docs-approved

alebedev87 commented 1 year ago

/label px-approved

openshift-ci-robot commented 1 year ago

@alebedev87: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-819 has been moved to the MODIFIED state.

In response to [this](https://github.com/openshift/external-dns-operator/pull/171): >Since the addition of the new format of TXT records in external-dns `0.12.0` the TXT records are prepended with the type of the DNS record which doesn't comply with the standards of Azure DNS in case of the wildcards: the wildcard symbol (`*`) has to be the leftmost one. > >Related issue: https://github.com/kubernetes-sigs/external-dns/issues/2922 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.