-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Change ApplyChanges in RFC2136 to batch update #1164
Conversation
This changes the ApplyChanges in the RFC2136 provider to issue a batch update to the DNS server instead of issueing an update for each updated record. An update is not sent to the server if there are no changes to send. Also the RemoveRecord function only issues a RR removal instead of an RRset removal so external-dns is now able to remove distinct records.
This fixes and adds some tests for the RFC2136 provider. The SendMessage stub function is improved with better DNS message parsing. Also a new test is introduced which simulates a single record with multiple targets. Also the test for ApplyChanges is fixed for the new batch updating.
Welcome @NIPE-SYSTEMS! |
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.
/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 |
Is there an amendment for a use case where I need to control DNS outside external DNS but I want to use that DNS for my deployment? Example: |
I'm sorry, but I don't get your point entirely, where the problem is. When I remember correctly (correct me if I'm wrong), external-dns manages its own state (so it stores all RRs itself). When it detects that its state differs from the plugin's state, it calls Before this PR, the RFC2136 plugin did non-batched updates, that means, it sends updates whenever it gets individual RRs. This is problematic because it is not atomic. A partial change may lead to an inconsistent update. This is what I experienced in my cluster (some updates had gone through, some had been dropped) and therefore created this PR. When looking closer at the code, the removal of RRs seems not correct. As I said, if I'm correct, external-dns passes individual RRs but the prior Now with my reasoning, can you point out problems with this? |
Thanks h3ndrk, I get the point. What you're doing make sense. However, I think this is an implementation that I use, which is trying to figure out how to rollover the entire cluster. I am using floating DNS to control the traffic into which cluster during the rollover. Without this patch, I don't get the conflict. I think it's due to external dns does not attempt to try updating the record set because it doesn't have the corresponding TXT. Let me know if it's not clear :) |
Okay, thanks for the clarifications. For me this is out of my scope since I've neither used Zalando AWS ingress nor Skipper, so can't help you in your particular setup. You could wait for an answer of the maintainers or post a fresh issue with a reference to your explanations here. |
This breaks external-dns for anyone who manages specific CNAMES outside of their cluster, but still needs to be able to route traffic for a hostname should it arrive. For example when you had a CNAME configured outside of the cluster that may route traffic into it for failover or latancy-routing reasons. See #1517 |
This changes the
ApplyChanges
in the RFC2136 provider to issue a batch update to the DNS server instead of issueing an update for each updated record. An update is not sent to the server if there are no changes to send.Also the
RemoveRecord
function only issues a RR removal instead of an RRset removal so external-dns is now able to remove distinct records.Some tests for the RFC2136 provider are fixed and added.