Multiple A records support for the same FQDN #1475

ytsarev · 2020-03-28T03:54:38Z

Fixes Update same domain name with multi IPs error #1313
Logic is to have unique key for every of multiple ep.Targets and keep
them around in ep.Labels for further consistency
Unit test is not very strict as testing functions are not ready for
multiple Services with the same key but is imho good enough

* Fixes kubernetes-sigs#1313 * Logic is to have unique key for every of multiple `ep.Targets` and keep them around in `ep.Labels` for further consistency * Unit test is not very strict as testing functions are not ready for multiple Services with the same key but is imho good enough

* Fixes #62 * Fixes kubernetes-sigs/external-dns#1313 * Upstream PR kubernetes-sigs/external-dns#1475 * Use custom build while upstream accepts the change

* Otherwise it would lead to unwanted deletion of etcd keys * Inline func comment fix

ytsarev · 2020-04-02T21:07:14Z

/assign @Raffo

Raffo

I added a few comments, logic looks okay overall but there are some things that need to be addressed.
Did you have the chance to test this in a real cluster?

Raffo · 2020-04-07T08:46:42Z

provider/coredns.go

@@ -258,6 +258,28 @@ func NewCoreDNSProvider(domainFilter endpoint.DomainFilter, prefix string, dryRu
 	}, nil
 }

+// Find takes a Endpoint slice and looks for an element in it. If found it will
+// return it's key, otherwise it will return -1 and a bool of false.


Suggested change

// return it's key, otherwise it will return -1 and a bool of false.

// return its key, otherwise it will return -1 and a bool of false.

Raffo · 2020-04-07T08:51:27Z

provider/coredns.go

@@ -258,6 +258,28 @@ func NewCoreDNSProvider(domainFilter endpoint.DomainFilter, prefix string, dryRu
 	}, nil
 }

+// Find takes a Endpoint slice and looks for an element in it. If found it will
+// return it's key, otherwise it will return -1 and a bool of false.
+func findEp(slice []*endpoint.Endpoint, dnsName string) (int, bool) {


Given that you only use the Endpoint in https://github.com/kubernetes-sigs/external-dns/pull/1475/files#diff-5e7d0936cf66e74a78954bf63c9a428aR301-R302, what about making this return directly the Endpoint instead of the index and simplify https://github.com/kubernetes-sigs/external-dns/pull/1475/files#diff-5e7d0936cf66e74a78954bf63c9a428aR301-R303 ?

An alternative is to ditch this function completely and embed it in the Records function as it is very small.

@Raffo very good idea, I've implemented it in c9f22b4 .

Raffo · 2020-04-07T08:53:15Z

provider/coredns.go

+}
+
+// Find takes a ep.Targets string slice and looks for an element in it. If found it will
+// return it's key, otherwise it will return -1 and a bool of false.


The code comment is wrong, this was likely a copy-paste, the function returns a string.

@Raffo thanks for the catch, comments are fixed

Raffo · 2020-04-07T08:55:07Z

provider/coredns_test.go

@@ -311,6 +329,7 @@ func applyServiceChanges(provider coreDNSProvider, changes *plan.Changes) {
 }

 func validateServices(services, expectedServices map[string]*Service, t *testing.T, step int) {
+	t.Helper()


What is this line doing?

@Raffo without t.Helper() the stackrace points to the line in the function instead of the specific failing test where this function was used. So adding t.Helper() makes it much easier to understand which tests is exactly failing during the test run

ytsarev · 2020-04-08T11:49:18Z

I added a few comments, logic looks okay overall but there are some things that need to be addressed.
Did you have the chance to test this in a real cluster?

@Raffo Yes, it was end-to-end tested thoroughly before I sent PR. Currently we use external-dns with this modification in context of https://github.com/AbsaOSS/ohmyglb project. It's in use for a while, works as expected

* Return Endpoint instead of index in `findEp()` * Simplify the logic within `Records()`

ytsarev · 2020-04-08T11:58:16Z

@Raffo All issues addressed, please check it out.

Raffo

Changes look good, please review my additional suggestions before we can merge this.

Raffo · 2020-04-11T14:24:39Z

provider/coredns.go

@@ -258,6 +258,28 @@ func NewCoreDNSProvider(domainFilter endpoint.DomainFilter, prefix string, dryRu
 	}, nil
 }

+// Find takes a Endpoint slice and looks for an element in it. If found it will


Suggested change

// Find takes a Endpoint slice and looks for an element in it. If found it will

// findEp takes an Endpoint slice and looks for an element in it. If found it will

Raffo · 2020-04-11T14:25:08Z

provider/coredns.go

+	return nil, false
+}
+
+// Find takes a ep.Targets string slice and looks for an element in it. If found it will


Suggested change

// Find takes a ep.Targets string slice and looks for an element in it. If found it will

// findLabelInTargets takes a ep.Targets string slice and looks for an element in it. If found it will

Raffo · 2020-04-11T14:25:37Z

provider/coredns.go

+			ep, found := findEp(result, dnsName)
+			if found {
+				ep.Targets = append(ep.Targets, service.Host)
+				log.Debugf("Exteding ep (%s) with new service host (%s)", ep, service.Host)


Suggested change

log.Debugf("Exteding ep (%s) with new service host (%s)", ep, service.Host)

log.Debugf("Extending ep (%s) with new service host (%s)", ep, service.Host)

Raffo · 2020-04-11T14:25:57Z

provider/coredns.go

@@ -305,7 +336,8 @@ func (p coreDNSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes
 		grouped[ep.DNSName] = append(grouped[ep.DNSName], ep)
 	}
 	for i, ep := range changes.UpdateNew {
-		ep.Labels[randomPrefixLabel] = changes.UpdateOld[i].Labels[randomPrefixLabel]
+		ep.Labels = changes.UpdateOld[i].Labels
+		log.Debugf("Updating labels (%s)  with old labels(%s)", ep.Labels, changes.UpdateOld[i].Labels)


Suggested change

log.Debugf("Updating labels (%s) with old labels(%s)", ep.Labels, changes.UpdateOld[i].Labels)

log.Debugf("Updating labels (%s) with old labels(%s)", ep.Labels, changes.UpdateOld[i].Labels)

ytsarev · 2020-04-13T11:29:01Z

@Raffo thanks a lot for the catches. All fixed.

Raffo · 2020-04-15T06:46:23Z

/lgmt
/approve

k8s-ci-robot · 2020-04-15T06:46:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, ytsarev

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~~ [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ytsarev · 2020-04-15T07:47:20Z

@Raffo you've made small typo so bot didn't put the label, s/lgmt/lgtm please :)

Raffo · 2020-04-15T09:08:37Z

Merging PRs before coffee, always a bad idea.

Raffo · 2020-04-15T09:08:44Z

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 28, 2020

k8s-ci-robot requested review from hjacobs and njuettner March 28, 2020 03:54

ytsarev mentioned this pull request Mar 28, 2020

Use custom build of external-dns with multi A fixes k8gb-io/k8gb#68

Merged

Skip not A Target related labels

83ea480

* Otherwise it would lead to unwanted deletion of etcd keys * Inline func comment fix

ytsarev force-pushed the corednstest branch from a0fddfd to 83ea480 Compare March 30, 2020 16:36

k8s-ci-robot assigned Raffo Apr 2, 2020

Raffo reviewed Apr 7, 2020

View reviewed changes

ytsarev added 2 commits April 8, 2020 13:49

Fix function comments

4d0af61

Make findEp return Endpoint directly

c9f22b4

* Return Endpoint instead of index in `findEp()` * Simplify the logic within `Records()`

ytsarev requested a review from Raffo April 8, 2020 12:49

Raffo reviewed Apr 11, 2020

View reviewed changes

Fix comments and log messages

d3b4126

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2020

k8s-ci-robot merged commit 3487945 into kubernetes-sigs:master Apr 15, 2020

ytsarev mentioned this pull request May 12, 2020

CoreDNS provider ownership is broken with multiple A records of the same host #1414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple A records support for the same FQDN #1475

Multiple A records support for the same FQDN #1475

ytsarev commented Mar 28, 2020

ytsarev commented Apr 2, 2020

Raffo left a comment

Raffo Apr 7, 2020

Raffo Apr 7, 2020

ytsarev Apr 8, 2020

Raffo Apr 7, 2020

ytsarev Apr 8, 2020

Raffo Apr 7, 2020

ytsarev Apr 8, 2020

ytsarev commented Apr 8, 2020 •

edited

Loading

ytsarev commented Apr 8, 2020

Raffo left a comment

Raffo Apr 11, 2020

Raffo Apr 11, 2020

Raffo Apr 11, 2020

Raffo Apr 11, 2020

ytsarev commented Apr 13, 2020

Raffo commented Apr 15, 2020

k8s-ci-robot commented Apr 15, 2020

ytsarev commented Apr 15, 2020

Raffo commented Apr 15, 2020

Raffo commented Apr 15, 2020

	// return it's key, otherwise it will return -1 and a bool of false.
	// return its key, otherwise it will return -1 and a bool of false.

	// Find takes a Endpoint slice and looks for an element in it. If found it will
	// findEp takes an Endpoint slice and looks for an element in it. If found it will

	// Find takes a ep.Targets string slice and looks for an element in it. If found it will
	// findLabelInTargets takes a ep.Targets string slice and looks for an element in it. If found it will

	log.Debugf("Exteding ep (%s) with new service host (%s)", ep, service.Host)
	log.Debugf("Extending ep (%s) with new service host (%s)", ep, service.Host)

	log.Debugf("Updating labels (%s) with old labels(%s)", ep.Labels, changes.UpdateOld[i].Labels)
	log.Debugf("Updating labels (%s) with old labels(%s)", ep.Labels, changes.UpdateOld[i].Labels)

Multiple A records support for the same FQDN #1475

Multiple A records support for the same FQDN #1475

Conversation

ytsarev commented Mar 28, 2020

ytsarev commented Apr 2, 2020

Raffo left a comment

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

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

ytsarev commented Apr 8, 2020 • edited Loading

ytsarev commented Apr 8, 2020

Raffo left a comment

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

Choose a reason for hiding this comment

ytsarev commented Apr 13, 2020

Raffo commented Apr 15, 2020

k8s-ci-robot commented Apr 15, 2020

ytsarev commented Apr 15, 2020

Raffo commented Apr 15, 2020

Raffo commented Apr 15, 2020

ytsarev commented Apr 8, 2020 •

edited

Loading