-
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
Multiple A records support for the same FQDN #1475
Conversation
* 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
* 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
/assign @Raffo |
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 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?
provider/coredns.go
Outdated
@@ -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. |
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.
// 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. |
provider/coredns.go
Outdated
@@ -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) { |
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.
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.
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.
provider/coredns.go
Outdated
} | ||
|
||
// 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. |
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.
The code comment is wrong, this was likely a copy-paste, the function returns a string.
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.
@Raffo thanks for the catch, comments are fixed
@@ -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() |
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.
What is this line doing?
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.
@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
@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()`
@Raffo All issues addressed, please check it 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.
Changes look good, please review my additional suggestions before we can merge this.
provider/coredns.go
Outdated
@@ -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 |
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.
// 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 |
provider/coredns.go
Outdated
return nil, false | ||
} | ||
|
||
// Find takes a ep.Targets string slice and looks for an element in it. If found it will |
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.
// 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 |
provider/coredns.go
Outdated
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) |
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.
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) |
provider/coredns.go
Outdated
@@ -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) |
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.
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) |
@Raffo thanks a lot for the catches. All fixed. |
/lgmt |
[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:
Approvers can indicate their approval by writing |
@Raffo you've made small typo so bot didn't put the label, |
Merging PRs before coffee, always a bad idea. |
/lgtm |
ep.Targets
and keepthem around in
ep.Labels
for further consistencymultiple Services with the same key but is imho good enough