-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Do not replace TXT records with A/CNAME records in planner (#580) #581
Conversation
No idea why gometalinter deadlined on Travis. I ran it locally and everything completed pretty quickly. |
Thanks for the PR. Currently we have a problem running gometalinter in Travis. We already incresed the timeout to 120s but unfortunately it doesn't seem enough. However I will take a look and probably create a PR to increase the deadline. Afterwards I can retrigger the build again. Update: I've retriggered the job, it seems to pass now We will have a look on your PR anyway :). |
plan/plan.go
Outdated
filtered := []*endpoint.Endpoint{} | ||
|
||
for _, record := range records { | ||
switch record.RecordType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it only affect TXT records? If so it would make sense to rewrite the logic IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my intent here is to be explicit that only certain records "conflict." The truth is, the only record that "conflicts" with anything is CNAME. From RFC 1034 (emphasis mine:)
The domain system provides such a feature using the canonical name
(CNAME) RR. A CNAME RR identifies its owner name as an alias, and
specifies the corresponding canonical name in the RDATA section of the
RR. If a CNAME RR is present at a node, no other data should be
present; this ensures that the data for a canonical name and its aliases
cannot be different. This rule also insures that a cached CNAME can be
used without checking with an authoritative server for other RR types.
So, if you have a CNAME record, then you MUST NOT have ANY other kind of record.
This actually brings a pretty interesting conundrum: It means that ExternalDNS can only create a CNAME record if no other record exists with this name, but it can create A records if any other record exists.
ExternalDNS today only cares about CNAME, A, and TXT records, and it only cares about TXT records for the TXT registry. To fix this bug properly, I argue one would have to include every kind of record and consider it. And in fact, I think that would probably not be a bad idea. However, it would only allow you to output a better error message, probably, since you can't actually do much to fix the problem itself from ExternalDNS's PoV.
So I opted to go for a simple solution of explicitly stating which records the planner cares about, which I believe is CNAME and A records. Specifically, the planner never wants to create CNAME records together with A records anyways, so logically this makes sense. Likewise, the planner probably never wants to delete a TXT record that it had nothing to do with, so this probably makes sense too.
As a sidenote... doesn't this mean the TXT registry should never have a blank prefix, since it could never work with CNAME records? I imagine this is probably an issue for Amazon ELB users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point and I agree.
There's currently also an IPv6 PR (#576) open, so it is also planned to support AAAA records.
Additionally to that there's another PR open #579 which maybe make sense to merge first.
We have a logic for CNAME's in AWS which will be handled as A or AAAA records. It will create ALIAS records pointing to ELB's and a TXT record as well.
Could you add some comment above the filterRecordsForPlan func?
Otherwise LGTM
Thanks for your input. I've added a few comments rationalizing the filtering function, hopefully this is sufficient. |
/LGTM |
plan/plan.go
Outdated
@@ -175,3 +175,27 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool { | |||
} | |||
return desired.RecordTTL != current.RecordTTL | |||
} | |||
|
|||
// filterRecordsForPlan removes records that are not relevant to the planner. | |||
// Currently this just removes TXT records to prevent them from being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"currently this just removes TXT records" --- I think the comment is now wrong as all records except A
and CNAME
are filtered out...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say currently because TXT, CNAME and A are the only record types that ExternalDNS looks at, as far as I can tell. Should I reword that?
@ideahitme Can you please leave your opinion here? Ideally, this conversion from TXT to A
shouldn't happen as there's no corresponding TXT record with the owner label. |
Of course, that's assuming that you use the TXT registry; if you use the noop registry, you'd get this bad behavior either way. Frankly, I would like a registry that stores data elsewhere, but maybe that can be a different PR. |
Hey @linki @hjacobs @njuettner, thanks for your help thusfar. It seems this PR fell through the cracks. Is there anything I can do to help push this through, such as particular improvements that could be made? |
@linki I will take a look at this in the nearest time possible. Sorry I was extremely busy with travelling recently |
@jchv is there a particular test which reveals the problem ? I currently see no difference if u use noop vs txt registry. Both will include custom TXT records and should not remove them as per |
@ideahitme Firstly, it is worth noting that the problem may have been resolved by an unrelated change since this PR was made. I have not looked, but there is at least one conflict. With that out of the way, I'll try to be as terse as possible.
For an example of a test that (should) fail when this bug is present, see |
I can confirm that txt registry conflicts with exiting TXT (SPF) records and See also #573 |
This bug (see also #573, txt-prefix doesn't work here) is becoming a pain in the ass for me. Is someone willing to get PR up2date or provide another PR? I could try get this done but I'm fairly new to the external-dns codebase. |
Well, I haven't checked, but I'm guessing this is still the same problem and same solution. This PR still doesn't conflict according to GitHub. Is there anything that needs updating? I'm relatively certain of the approach as long as the planner hasn't been re-hauled. |
Is there anything that can be done to move this forward? |
👍 Yes this hits me on GCE Cloud DNS and using SPF in TXT records. |
/LGTM |
/APPROVE |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njuettner 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 |
nice! will test it soon :) |
The Problem
If you have a zone that has unrelated TXT records, such as SPF records, you might think you can use the
--txt-prefix
option to allow the registry to continue working. However, this is not the case, because even with the registry fully disabled, the planner will see and treat TXT records equally. This means when it sees a record that has the same name as a planned A or CNAME record that does not yet exist, it will attempt to delete the TXT record even though it doesn't conflict.This actually happens in
planTable.getUpdates
:As far as I can tell, it will happily include TXT records here, and you'll have plans that look like:
TXT myrecord.com
A myrecord.com 1.2.3.4
The Solution
Since TXT records are the domain of the TXT registry, which does not use the
Plan.Calculate
method, I decided to implement the solution there, by filtering TXT records out. There may be other ways to go about doing this, probably inplanTable.getUpdates
, but I wanted to be as careful as possible to not disturb other functionality.Closes #580