Multitarget #376

multi-io · 2017-11-01T07:47:07Z

This PR enables having multiple targets per domain name. I'm aware of PR 243, but this one works by having one Endpoint structure per (name, target) rather than extending Endpoint.Target to be an array and having one Endpoint per name. This means that the existing providers should work without changes, EXCEPT if they're written explicitly with the assumption of one endpoint per name in mind. We're using the Cloudflare provider; I had to modify one line (2d45bc2) to make it work. I also modified the inmemory provider accordingly and added multi-target tests to for TXTRegistry, Controller and Plan.

I modified TXTRegistry to perform the ownerID tracking internally by caching some state in member variables between Records() and ApplyValues(), rather than using the Endpoints' .Labels["owner"] for that (storing per-name ownerIDs in per-target Endpoints would've been redundant and would've complicated things). This means that the labels are no longer used for anything now and might be removed unless we need them for something else in the future.

Everything works, AFAICS. We use the CF provider as I said, haven't really tested the others.

(update 11/08/17: inmemory provider multi-target capability, more tests)

…t rather than just per DNSName

… state rather than Endpoint.Labels

k8s-ci-robot · 2017-11-01T07:47:10Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

multi-io · 2017-11-01T14:19:31Z

I signed

BenTheElder · 2017-11-01T18:00:54Z

Bump for CLAbot 🤞

updateOld/-New can only be used to update an existing record without changing its identifying information, i.e. name and target. so instead of updateOld: foo.com => 1.1.1.1, updateNew: foo.com => 2.2.2.2 one must: delete: foo.com => 1.1.1.1, create: foo.com => 2.2.2.2

…t an owner record

szuecs · 2017-11-09T08:35:08Z

controller/controller_test.go

-				Target:  "8.8.8.8",
+				DNSName:   "update-record",
+				Target:    "8.8.4.4",
+				RecordTTL: 50,


Does this test only work with TTL?
I think we should have a default defined in the zone, similar to DNS origin definition to default TTLs.

If it works, can we make this testcase to have with and without TTL tests

The TTLs are there in some tests now to have record updates rather than creations/deletions. Basically, with one Endpoint per (DNSName, Target), to update a record rather than delete it and create another one, you must not change either DNSName or Target, so I'm changing the TTL instead. If you change the target, the record won't be updated, it will be deleted and another one with the new target created -- similar to what would happen if you change the name. I have modified Plan.Calculate() to operate this way. (that's really the crux of this PR)

ok thanks to clarify this

szuecs · 2017-11-09T08:37:32Z

Pretty clean code from scrolling over, thanks for this PR.
I am unsure if we have to have TTL defined in all tests or if we should have also tested the default ttl.
If this is not super complicated please also add tests for not defined TTL, because if this break, this could also break current production installations.

szuecs · 2017-11-09T14:05:52Z

👍

szuecs · 2017-11-09T14:06:09Z

@ideahitme @linki

linki · 2017-11-13T14:06:02Z

plan/plan.go

 		shouldUpdateTTL := shouldUpdateTTL(desired, current)

-		if !targetChanged && !shouldUpdateTTL {
+		if !shouldUpdateTTL {


This condition skipped the record in case nothing was changed (target nor TTL). With this it skips the record whenever TTL was changed (but target could have) which seems wrong to me. Can you double-check?

desired and current are guaranteed to have the same target (see the recordExists() modification below), so targetChanged would always be false. That's why I removed it. As I said, with this PR you'd have one Endpoint per DNSName and Target rather than just per DNSName, so if the target changes, the corresponding endpoint will be removed and a new one with the same name and the new target created. So it treats target changes just like it always treated name changes -- the old Endpoint will be removed and a new one created. The TTL would be the only remaining thing in an Endpoint that can actually "change".

That's really the crux of this PR -- don't turn Endpoint.Target into an array, instead have one Endpoint per name and target. I did this because I figured it would require fewer (possibly no) changes to the providers, and I wanted to get the CF provider up and running with multiple targets quickly (I initially thought you'd basically get away with just changing plan.recordExists(), then saw that I should also modify the ownerId tracking in TXTRegistry). I guess we'd have to discuss whether this is the best way forward or not, since this is obviously incompatible with the other multitarget PRs that turn Endpoint.Target into an array.

dereulenspiegel · 2017-12-04T14:37:33Z

Since I am very interested in supporting multiple targets for a single DNS name and I was intrigued by the idea to keep changes in providers to a minimum, I tried to adapt this PR for our environment. We are using Google Cloud DNS on GKE.
Unfortunately I don't think, that this approach is viable as a general solution. In Google Cloud DNS you encounter the problem that deletes need to match exactly the existing ResourceRecordSet. You can't delete a single A record for a DNS name (or you probably can, but not with the current provider implementation).
This PR makes deletes virtually impossible in Google Cloud DNS. And without working deletes, updates also fail (since they are 'delete and recreate').
From my point of view it looks we need to turn Endpoint.Target into an array, to be able to keep using the Google Cloud DNS provider.
But i won't deny the possibility that I am absolutely wrong and I just don't see the correct solution yet.

ideahitme · 2017-12-04T14:54:14Z

@dereulenspiegel the idea is to go with the Endpoint.Target to array transition. Personally I would prefer to immediately go with that solution which would work for all providers (presumably). Please refer to: #396, #404 it gives some context on how we are planning to enable multi-target support

multi-io · 2017-12-04T14:57:01Z

I agree that one endpoint per target is probably not the best general solution, and I certainly don't cling to it or anything. It was just something to get going quickly on CF.

dereulenspiegel · 2017-12-04T14:58:45Z

@ideahitme I already read #396 and #404 . But I thought I will try this idea, to a) provide some feedback on another provider than the one used by the creator of this PR and b) have short term solution for our cluster, since #404 seems to need a bit more time.
I generally think that the combination of #404 and #396 is probably the more generalstic approach, since it also addresses other problems.
I will follow #404 and #396 more closely now.

ideahitme · 2017-12-04T15:02:41Z

@dereulenspiegel yes, feedback would be awesome. However, based on my analysis, #404 should be safe for rollout in any dns-provider, however I only tested it on AWS. I am afraid any short term solution have to also fix internal component logic (plan) (in a fashion similar to #404), I could be wrong though. I am basically waiting for #404 to be reviewed and need additional pair of eyes to make sure it is correct approach, but from coding perspective it should be complete

linki · 2018-10-02T16:00:43Z

Multi-target functionality has successfully landed in another PR. Therefore I'm closing this. Feel free to re-open if you think it's wrong.

Thanks @multi-io for all your effort!

multi-io added 3 commits November 1, 2017 02:07

support multiple targets by having one Endpoint per DNSName and Targe…

ffe076d

…t rather than just per DNSName

cloudflare provider fixed for multi-target

2d45bc2

TXTRegistry: ownerID tracking (for multi-target records) via internal…

e1dd79f

… state rather than Endpoint.Labels

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2017

multi-io force-pushed the multitarget branch from 6bc030d to daf1474 Compare November 1, 2017 13:51

multi-io force-pushed the multitarget branch from daf1474 to 870498c Compare November 1, 2017 14:29

tests adapted

28fec84

multi-io force-pushed the multitarget branch from 870498c to 28fec84 Compare November 1, 2017 15:03

This was referenced Nov 1, 2017

Umbrella issue for the Linux Foundation (CNCF) CLA system kubernetes/kubernetes#27796

Closed

Open source CLAbot cncf/foundation#3

Closed

multi-io force-pushed the multitarget branch 2 times, most recently from 62212f0 to 28fec84 Compare November 1, 2017 17:00

multi-io added 4 commits November 8, 2017 18:47

inmemory provider fixed for multi-target

2531925

TestNoopRegistry fixed for multi-target

6ac1b1a

TXTRegistry: do not create records if the name existed already withou…

612236d

…t an owner record

multi-io force-pushed the multitarget branch from dcea2ed to 612236d Compare November 8, 2017 17:48

szuecs reviewed Nov 9, 2017

View reviewed changes

linki added the area/multiple-targets label Nov 13, 2017

linki reviewed Nov 13, 2017

View reviewed changes

linki added area/multiple-targets and removed area/multiple-targets labels Feb 21, 2018

linki closed this Oct 2, 2018

lou-lan pushed a commit to lou-lan/external-dns that referenced this pull request May 11, 2022

Update golangci-lint to v1.21.0 (kubernetes-sigs#376)

6eb962a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multitarget #376

Multitarget #376

multi-io commented Nov 1, 2017 •

edited

k8s-ci-robot commented Nov 1, 2017

multi-io commented Nov 1, 2017

BenTheElder commented Nov 1, 2017

szuecs Nov 9, 2017

multi-io Nov 9, 2017 •

edited

szuecs Nov 9, 2017

szuecs commented Nov 9, 2017

szuecs commented Nov 9, 2017

szuecs commented Nov 9, 2017

linki Nov 13, 2017

multi-io Nov 13, 2017

dereulenspiegel commented Dec 4, 2017

ideahitme commented Dec 4, 2017

multi-io commented Dec 4, 2017

dereulenspiegel commented Dec 4, 2017

ideahitme commented Dec 4, 2017 •

edited

linki commented Oct 2, 2018

Multitarget #376

Multitarget #376

Conversation

multi-io commented Nov 1, 2017 • edited

k8s-ci-robot commented Nov 1, 2017

multi-io commented Nov 1, 2017

BenTheElder commented Nov 1, 2017

szuecs Nov 9, 2017

Choose a reason for hiding this comment

multi-io Nov 9, 2017 • edited

Choose a reason for hiding this comment

szuecs Nov 9, 2017

Choose a reason for hiding this comment

szuecs commented Nov 9, 2017

szuecs commented Nov 9, 2017

szuecs commented Nov 9, 2017

linki Nov 13, 2017

Choose a reason for hiding this comment

multi-io Nov 13, 2017

Choose a reason for hiding this comment

dereulenspiegel commented Dec 4, 2017

ideahitme commented Dec 4, 2017

multi-io commented Dec 4, 2017

dereulenspiegel commented Dec 4, 2017

ideahitme commented Dec 4, 2017 • edited

linki commented Oct 2, 2018

multi-io commented Nov 1, 2017 •

edited

multi-io Nov 9, 2017 •

edited

ideahitme commented Dec 4, 2017 •

edited