Multi target plan ( Page Not Found ) #404

ideahitme · 2017-11-28T17:54:36Z

Wanted to continue on #261 but unfortunately that branch is protected from push. Continue here

This PR is one of the steps outlined in #396 and does the following:

Each service/ingress sets additional label "Resource" to indicate who generated the endpoint
"Conflict Resolver" is added to abstract away the logic of resolving targets for the same hostname
"PerResource" resolver uses "Resource" label and acts as a "Conflict Resolver" by making targets the same as specified in the resource who first acquired a dns name (until it is deleted)
Plan is changed and tests rewritten. Now plan makes sure that dns provider will never get two endpoints which are requesting same dns name. However now plan allows to update record type (consequence of which we will have to test). It provides a consistent behaviour aligned with the proposal in add first proposal version for multi target #396
Registry adds the resource label into the txt records. It also parses the labels and correspondingly populates "Labels" map of each endpoint.
~~Docs outline the major breaking change introduced by this PR~~ changelog should suffice

~~/cc everyone, even though not all checkboxes are checked a premature review would be nice to have to make sure we are on the same page~~
Please review @szuecs @linki

With this change it becomes possible to work with endpoint of empty type in packages other than "provider". Also it seems logical for a smart property getter without side effects to be a method rather than a function in different package

ideahitme · 2017-11-29T15:48:03Z

@linki would u be available for review, PR should be functionally complete now. But need to test it on actual dns provider to make sure it works as expected.

ideahitme · 2017-11-29T16:02:45Z

~~I will also change Labels field to have a type, so that (de)serialization are available as methods -~~ DONE

…ndpoints

szuecs · 2017-11-30T13:56:49Z

One question from reading the PR description: How does a migration from one resource to another works?

Example case:

I have a service type LoadBalancer defnition for "foo.example.org" and now I want to shift it to an Ingress for example (could be also 2 ingress object or 2 services). Can I add both and delete the old one such that external DNS will shift the DNS record from one to the other as "atomic" operation delete and create in one cycle or do you have to wait a minute are eventually offline for that time frame?

ideahitme · 2017-11-30T13:58:19Z

@szuecs yes, it will happen as an atomic operation, from DNS provider point of view it will be:

Updating targets and record type (if needed)
Updating TXT record (to update resource label)

szuecs · 2017-11-30T15:42:13Z

plan/plan.go

+// TODO: allows record type change, which might not be supported by all dns providers
+func (t planTable) getUpdates() (updateNew []*endpoint.Endpoint, updateOld []*endpoint.Endpoint) {
+	for _, row := range t.rows {
+		if row.current != nil && len(row.candidates) != 0 { //dns name is taken


!= 0 should be > 0

you are right, that makes more sense

ideahitme · 2017-12-04T13:44:42Z

plan/plan.go

+		if row.current != nil && len(row.candidates) > 0 { //dns name is taken
+			update := t.resolver.ResolveUpdate(row.current, row.candidates)
+			// compare "update" to "current" to figure out if actual update is required
+			if shouldUpdateTTL(update, row.current) || targetChanged(update, row.current) {


should we maybe not allow record type change, or make it optional, or provide an option to disallow it ?

dereulenspiegel · 2017-12-04T15:35:40Z

I just wanted to provide some feedback. This PR seems to work perfectly fine Google Cloud DNS. I have multiple ingress definitions with multiple targets each. Of course only one A record per DNS name is currently created, but this works as expected.
Now multiple Targets per Endpoint are missing.

dereulenspiegel · 2017-12-05T14:51:40Z

I have started refactoring Endpoint.Target as []string (actually as its own type which is based on []string). You can see the progress here https://github.com/connctd/external-dns/tree/multi-target.

Right now all tests are green (but without putting much effort or thought into it, so there are probably a lot of new bugs), and ingress source and Google Cloud DNS provider should be able to handle multiple Targets on and endpoint. Now I have to work on the plan to make sure that the desired endpoint for a DNS name is selected.
My assumption right now is that CNAMEs are preferred over endpoints with A records and that endpoints with more A records are preferred over endpoints with fewer A records (if they come from the same ingress).
I hope that I can test this in a real world scenario this week.

ideahitme · 2017-12-05T14:59:11Z

@dereulenspiegel I would postpone this, there are multiple PRs already trying to implement this. I would strongly advise to check open PRs first: #326 and #243 . I would not want anyone to work on something and later find that all efforts are wasted

As for the plan, if u based it on this PR, then it should require minimum changes

dereulenspiegel · 2017-12-05T15:30:52Z

@ideahitme I checked these PRs actually, but I was assuming that they are stale.
Since I am quite far along, I will test my changes anyway including my custom conflict resolver. Probably this can still be interesting here.
Implementing this I hit the situation that in Ingress can still create multiple Endpoint instances. For example one for all IP addresses associated with it, and one for each hostname (through annotation or from the IngressStatus resource). Right now I return all of them and let the conflict resolver handle the selection.
My question would now be, if my assumption is correct that the Ingress resource can still create multiple Endpoint instances per Ingress or if this something which should not happen.

dereulenspiegel · 2017-12-05T16:39:54Z

My fork is now actually working as expected on GKE with multiple ingress resources, each having multiple IP addresses.
@ideahitme The more I think about the conflict resolution I get the feeling we need to specify this more.
If we actually get multiple endpoints from one ingress with different record types, what is the preferred record type or is this condition an error?
If we have multiple CNAME endpoints which CNAME should be chosen?
How do we handle multiple endpoint with A records but with different sets of targets? Is the longest target list the preferred one or the one with the least changes to the current one (if it makes any difference at all)?
Right now I assume that it is possible to generate multiple endpoints per Ingress (at least the API of the Ingress resource implies this).
This implies from my point of view, that we

prefer CNAME based endpoints over A record based endpoints, because the CNAME based endpoints likely point to some kind of load balancer
prefer A record based endpoints with the longer target list over A record based endpoints with a shorter target list, because we can assume that both target lists satisfy our needs to route traffic, but the endpoint with the longer list provides more redundancy.
currently my conflict resolver will only change the record of an existing record of no candidates matches the given record type

But of course all my assumptions could be wrong, since I am rather new to the kubernetes thing.

Just to advertise my fork a bit :) In my version Targets on Endpoint is of a type Targets just like Labels which possibly provides a bit nicer API than handling a raw string slice.

ideahitme · 2017-12-06T09:26:52Z

@dereulenspiegel thanks for thoughtful approach :)

If we actually get multiple endpoints from one ingress with different record types, what is the preferred record type or is this condition an error?

I mentioned this point in my proposal. I think for now we can just ignore this case and let DNS provider fail, or simply prefer IP over Hostname in this sort of scenario, or the other way round. This scenario is not common so I would not spend too much time thinking of best approach

If we have multiple CNAME endpoints which CNAME should be chosen?

This one is a bit more tricky. Some DNS providers support this scenario, therefore I think we should leave all of them in the Targets list and let DNS provider logic do what it should

How do we handle multiple endpoint with A records but with different sets of targets? Is the longest target list the preferred one or the one with the least changes to the current one (if it makes any difference at all)?

If I understood you correctly: Similar to this PR if it is update then prefer targets of current resource. If not, then even simply hashing all targets and picking minimum will do (for me the only worry here is consistency). It could also be two-step decision algorithm: pick maximum length target list. If there are multiple such lists, hash and pick minimum/maximum (again consistency)

currently my conflict resolver will only change the record of an existing record of no candidates matches the given record type

It should only change record type if owning resource indicates record type change

P.S: you are welcome to ping me in slack if you'd like to discuss this. Help in this direction is definitely appreciated

szuecs · 2017-12-11T15:19:20Z

registry/txt.go


+		// when we delete TXT records for which value has changed (due to new label) this would still work because
+		// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed


the comment is here and in line 128. I am not sure if this makes sense to me. I think this is more a property of the ApplyChanges function of how it does this change or do I miss something?

I added the comment here because what is going on here is a bit of magic. With this PR change we cannot predict what is the value stored in the TXT record, and for the TXT record to be correctly deleted/updated we should make sure its value matches what is stored on DNS provider side. And this is done via regenerating txt record value from labels

szuecs · 2017-12-11T15:24:50Z

registry/txt.go

 		filteredChanges.Delete = append(filteredChanges.Delete, txt)
 	}

+	// make sure TXT records are consistently updated as well
+	for _, r := range filteredChanges.UpdateNew {


All 3 for loops are basically the same, can you refactor them with "extract method" and call them 3 times, for example:

filteredChanges.Delete = append(filteredChanges.Delete, yourNewFunction()... ) filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, yourNewFunction()... ) filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, yourNewFunction()... )

szuecs · 2017-12-11T15:34:23Z

from my side lgtm, only small cleanups suggested

linki · 2017-12-12T16:27:16Z

endpoint/labels.go

+	// OwnerLabelKey is the name of the label that defines the owner of an Endpoint.
+	OwnerLabelKey = "owner"
+	// ResourceLabelKey is the name of the label that identifies k8s resource which wants to acquire the DNS name
+	ResourceLabelKey = "resource"


Owner was supposed to identify the owner of a resource. For us it was to say it's "the cluster" (or rather the - usually one - instance of external-dns). However now the owner is a particular resource in a particular namespace in a particular cluster. I wonder if it's confusing to separate them this way.

I would rather clarify it with either a combined owner resource (owner=extdns1/default/my-service, is that even easier to code?) or two owner related labels (ownerInstance=extdns1, ownerResource=default/my-service, just a rename then).

Although there's the concern of backwards compatibility...

@ideahitme let me know what you think.

yeah I agree, the naming is probably not the most descriptive.

However now the owner is a particular resource in a particular namespace in a particular cluster. I wonder if it's confusing to separate them this way.

I would still prefer them to be separated:

"owner" label signifies if the DNS Record can be modified at all

"resource" label helps to figure out how it should be updated

I also can see scenarios where "resource" label can be ignored altogether, so in my mind it is just a metadata, which allows to implement a "method" for determining resource-ownership, but it could be used for something else, or a different method could be implemented. That's why I thought it is better to keep the label name independent of how we use it.

With the previous statement though it is possibly better to rename "owner" label to something else, like "external-dns-instance", but since it would break compatibility, I would not do this change in this PR. However this change would be possible with some code change which would allow us to gradually migrate txt record values.

linki · 2017-12-12T16:29:05Z

endpoint/labels.go

+			continue
+		}
+		key := strings.Split(token, "=")[0]
+		val := strings.Split(token, "=")[1]


Can we use Kubernetes' label parsing functions? Seems easier to use: https://github.com/linki/chaoskube/blob/v0.6.1/main.go#L59 and we don't have to maintain this.

I will try it out :)

undone, because labels could not be parsed if "/" are present :(

linki · 2017-12-12T16:56:40Z

For anyone who wants to test this the docker image is available at:

docker pull registry.opensource.zalan.do/teapot/external-dns:v0.4.8-24-ga0c3911

linki · 2017-12-13T15:26:01Z

Tested on my GKE cluster and it works fine:

had existing Ingress with one IP as target
added another IP to ingress target => external-dns found both IPs but kept DNS records as is
dropped old IP from Ingress => external-dns switched to the new IP
added another IP again => external-dns found both IPs but kept DNS records as is

So that seems to work perfect.

ideahitme · 2017-12-13T15:48:50Z

@linki couldn't use apimachinery package, due to the parsing failures for label values, such as "default/ingress/my-ingress" due to "/"

ideahitme · 2017-12-14T10:05:12Z

@linki f so let's get it merged ?

linki · 2017-12-14T10:17:49Z

@ideahitme yes!

linki · 2017-12-14T10:21:22Z

🎉

ideahitme · 2017-12-14T10:22:05Z

awesome!

* Make suitableType() be Endpoint method With this change it becomes possible to work with endpoint of empty type in packages other than "provider". Also it seems logical for a smart property getter without side effects to be a method rather than a function in different package * Make plan computation work correctly with multi-target domains * fix drawing * drop comments * fix boilerplate header * fix comment * fix the bug with empty map * rework registry to support random lables * serialize->serializeLabel function rename * golint for err variable naming * add additional test * add tests for current case where one resource can generate multiple endpoints * make labels have its own type, add serialization as a method * add comment for exported error * use greater rather than not equal zero * update changelog

* Endpoint.Target is now Endpoint.Targets. This is its own type representing mutliple targets for a single DNS name while adding some convenience for sorting and comparing * Made everything compile and tests run through with the new Endpoint.Targets * The ingress source can now properly handle multiple target ips per host * Added custom conflict resolver, to better understand how conflict resolution has to work for me * My custom conflict resolver behaves a bit different than the PerResource resolver, therefore I needed to modify the expected test result Removed unnecessary FIXME * The ingress source now creates CNAME endpoints with multiple targets to let the DNS provider decide how to handle multiple CNAME targets. This could be interesting for weighted targets etc. * Adopted the expected results to the new way we create endpoints for CNAMEs * Removed Add method from Targets since manipulating the slice through here is unnecessary complicated and doesn't deliver enough convenience * Reverted ConflictResolver to the original one. There is some discussing to do what the best way is to handle conflicts * Added missing documenting comment to IsLess of Targets * Added documenting comments to Targets,Targets.Same and NewTargets to clarify their intention and usage * Service source now also generates endpoints with multiple targets * Service and Ingress source now sort all Targets for every Endpoint to make order of Targets predictable * Endpoints generated by the Google Cloud DNS provider now also have sorted Targets to make order of Targets predictable * Modified provider dyn to be able to compile with multi target changes * Fixed small nitpicks, so my code is acceptable * Fixed merge method after updating to new Targets. Replacing '!=' with .Same of course needs a boolean negation * Tests for dyn provider now also use the new Targets instead of Target * Simplified extractServiceIps as implied by linki to make it more readable * ref: change service ClusterIP retrieval again * Added entry to CHANGELOG.md describing the new features contained in this PR

…kubernetes-sigs#396 (kubernetes-sigs#418) * Endpoint.Target is now Endpoint.Targets. This is its own type representing mutliple targets for a single DNS name while adding some convenience for sorting and comparing * Made everything compile and tests run through with the new Endpoint.Targets * The ingress source can now properly handle multiple target ips per host * Added custom conflict resolver, to better understand how conflict resolution has to work for me * My custom conflict resolver behaves a bit different than the PerResource resolver, therefore I needed to modify the expected test result Removed unnecessary FIXME * The ingress source now creates CNAME endpoints with multiple targets to let the DNS provider decide how to handle multiple CNAME targets. This could be interesting for weighted targets etc. * Adopted the expected results to the new way we create endpoints for CNAMEs * Removed Add method from Targets since manipulating the slice through here is unnecessary complicated and doesn't deliver enough convenience * Reverted ConflictResolver to the original one. There is some discussing to do what the best way is to handle conflicts * Added missing documenting comment to IsLess of Targets * Added documenting comments to Targets,Targets.Same and NewTargets to clarify their intention and usage * Service source now also generates endpoints with multiple targets * Service and Ingress source now sort all Targets for every Endpoint to make order of Targets predictable * Endpoints generated by the Google Cloud DNS provider now also have sorted Targets to make order of Targets predictable * Modified provider dyn to be able to compile with multi target changes * Fixed small nitpicks, so my code is acceptable * Fixed merge method after updating to new Targets. Replacing '!=' with .Same of course needs a boolean negation * Tests for dyn provider now also use the new Targets instead of Target * Simplified extractServiceIps as implied by linki to make it more readable * ref: change service ClusterIP retrieval again * Added entry to CHANGELOG.md describing the new features contained in this PR

Stan Lagun and others added 5 commits July 5, 2017 22:54

Make suitableType() be Endpoint method

baaf72a

With this change it becomes possible to work with endpoint of empty type in packages other than "provider". Also it seems logical for a smart property getter without side effects to be a method rather than a function in different package

Make plan computation work correctly with multi-target domains

bc81730

rebase, add plantable, add conflict resolver, implement resource label

41a3119

fix drawing

1ff3d43

drop comments

b247a75

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 28, 2017

ideahitme changed the title ~~[WIP] Multi target plan~~ [WIP] Multi target plan ( Page Not Found ) Nov 28, 2017

fix boilerplate header

d1979ce

ideahitme mentioned this pull request Nov 29, 2017

Make plan computation work correctly with multi-target domains #261

Closed

ideahitme added 3 commits November 29, 2017 11:00

fix comment

ada43e4

fix the bug with empty map

c4e6188

rework registry to support random lables

d51b1b0

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 29, 2017

ideahitme added 3 commits November 29, 2017 16:13

serialize->serializeLabel function rename

a4fb4bc

golint for err variable naming

88ef640

add additional test

36ce919

ideahitme requested a review from linki November 29, 2017 15:48

add tests for current case where one resource can generate multiple e…

6e93afe

…ndpoints

ideahitme mentioned this pull request Nov 29, 2017

add first proposal version for multi target #396

Merged

ideahitme added 2 commits November 30, 2017 12:08

make labels have its own type, add serialization as a method

b543159

add comment for exported error

dbafc58

szuecs reviewed Nov 30, 2017

View reviewed changes

ideahitme added 2 commits December 1, 2017 11:03

use greater rather than not equal zero

93f40c6

merge with master

aaf5c9c

ideahitme commented Dec 4, 2017

View reviewed changes

update changelog

a0c3911

ideahitme mentioned this pull request Dec 4, 2017

Multitarget #376

Closed

ideahitme changed the title ~~[WIP] Multi target plan ( Page Not Found )~~ Multi target plan ( Page Not Found ) Dec 4, 2017

ideahitme added the area/multiple-targets label Dec 4, 2017

szuecs reviewed Dec 11, 2017

View reviewed changes

linki reviewed Dec 12, 2017

View reviewed changes

dereulenspiegel mentioned this pull request Dec 13, 2017

Implementation of multiple targets based on PR #404 and #396 #418

Merged

8 tasks

ideahitme force-pushed the multi-target-plan branch from b0ebff7 to a0c3911 Compare December 13, 2017 15:47

linki merged commit ec07f45 into master Dec 14, 2017

linki deleted the multi-target-plan branch December 14, 2017 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi target plan ( Page Not Found ) #404

Multi target plan ( Page Not Found ) #404

ideahitme commented Nov 28, 2017 •

edited

Loading

ideahitme commented Nov 29, 2017 •

edited

Loading

ideahitme commented Nov 29, 2017 •

edited

Loading

szuecs commented Nov 30, 2017

ideahitme commented Nov 30, 2017

szuecs Nov 30, 2017

ideahitme Nov 30, 2017

ideahitme Dec 4, 2017 •

edited

Loading

dereulenspiegel commented Dec 4, 2017

dereulenspiegel commented Dec 5, 2017

ideahitme commented Dec 5, 2017

dereulenspiegel commented Dec 5, 2017

dereulenspiegel commented Dec 5, 2017

ideahitme commented Dec 6, 2017 •

edited

Loading

szuecs Dec 11, 2017 •

edited

Loading

ideahitme Dec 12, 2017

szuecs Dec 12, 2017

szuecs Dec 11, 2017

szuecs commented Dec 11, 2017

linki Dec 12, 2017

ideahitme Dec 13, 2017 •

edited

Loading

linki Dec 13, 2017

linki Dec 12, 2017 •

edited

Loading

ideahitme Dec 13, 2017

ideahitme Dec 13, 2017

ideahitme Dec 13, 2017

linki commented Dec 12, 2017

linki commented Dec 13, 2017 •

edited

Loading

ideahitme commented Dec 13, 2017

ideahitme commented Dec 14, 2017 •

edited

Loading

linki commented Dec 14, 2017

linki commented Dec 14, 2017

ideahitme commented Dec 14, 2017


		// when we delete TXT records for which value has changed (due to new label) this would still work because
		// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed

Multi target plan ( Page Not Found ) #404

Multi target plan ( Page Not Found ) #404

Conversation

ideahitme commented Nov 28, 2017 • edited Loading

ideahitme commented Nov 29, 2017 • edited Loading

ideahitme commented Nov 29, 2017 • edited Loading

szuecs commented Nov 30, 2017

ideahitme commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ideahitme Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

dereulenspiegel commented Dec 4, 2017

dereulenspiegel commented Dec 5, 2017

ideahitme commented Dec 5, 2017

dereulenspiegel commented Dec 5, 2017

dereulenspiegel commented Dec 5, 2017

ideahitme commented Dec 6, 2017 • edited Loading

szuecs Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs commented Dec 11, 2017

Choose a reason for hiding this comment

ideahitme Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linki Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linki commented Dec 12, 2017

linki commented Dec 13, 2017 • edited Loading

ideahitme commented Dec 13, 2017

ideahitme commented Dec 14, 2017 • edited Loading

linki commented Dec 14, 2017

linki commented Dec 14, 2017

ideahitme commented Dec 14, 2017

ideahitme commented Nov 28, 2017 •

edited

Loading

ideahitme commented Nov 29, 2017 •

edited

Loading

ideahitme commented Nov 29, 2017 •

edited

Loading

ideahitme Dec 4, 2017 •

edited

Loading

ideahitme commented Dec 6, 2017 •

edited

Loading

szuecs Dec 11, 2017 •

edited

Loading

ideahitme Dec 13, 2017 •

edited

Loading

linki Dec 12, 2017 •

edited

Loading

linki commented Dec 13, 2017 •

edited

Loading

ideahitme commented Dec 14, 2017 •

edited

Loading