fix(source): filter provider-specific properties to configured provider#6220
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Pull Request Test Coverage Report for Build 23116432762Details
💛 - Coveralls |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
There was a problem hiding this comment.
I don't think this will works with cloudflare. In endpoint.ProviderSpecificAnnotations(), other providers cut their annotation prefix (external-dns.alpha.kubernetes.io/provider-xxx) before adding a specific provider entry on the endpoint. Cloudflare keep it so the provider specifics are external-dns.alpha.kubernetes.io/cloudflare-xxx, not cloudflare-xxx. With this implementation those specifics will be filtered.
It could be tempting to simply change this and align with other providers, but we'd have to keep compatibility with DNSEndpoints that set those provider specifics.
|
Wouldn't it be better to let each provider handle its own filtering logic? Isn’t |
|
Btw, thank for flagging. Indeed, cloudflare is very different to other providers. I have a PR #6158 to start capturing end-2-end behaviour, it may help a bit soon. Tested with fixtures |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
I've added a bypass/exception for cloudflare. No plans solving cloudflare diff in this pr
All providers should be consistent where possible to reduce drift. This is potential benefits of consistent providers
|
|
I've added test, that should fail, if someone is going to add a new provider with properties and support of other format. At the moment, short syntax
Full syntax
|
|
/test pull-external-dns-unit-test |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
Hi @vflaux when you around, pls have, let'f finish this review. |
source/wrappers/post_processor.go
Outdated
| cfg.preferAlias = enabled | ||
| if enabled { | ||
| cfg.isConfigured = true | ||
| cfg.preferAlias = enabled |
There was a problem hiding this comment.
No sure about this change. This option with enable=false will do nothing. And is that related to this PR?
There was a problem hiding this comment.
Yeah, will roll it back. Basically cfg.preferAlias is false be default, so the assigment only make sense when value is true
Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does it do ?
As part of this pr, found Cloudflare special case that is architecturally fragile (it's going to be difficult to just fix the provider in this PR)
Follow-up PR
Motivation
When endpoints carry provider-specific properties from multiple providers (e.g., both aws/evaluate-target-health and coredns/group), all of them were passed downstream regardless of which provider was actually configured. This means a provider receives properties that belong to a different provider, which can cause unexpected behaviour or misconfiguration during record synchronisation.
Fixes #6217
Fixes #4951
Relates #4347
Relates #5947
More
Current

With the fix -> the problem gone
Fixtures