feat(source)!: introduce optional force-default-targets #5316
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @alen-z! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @alen-z. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
As far as I know, that's not in our current plans, but things might have shifted. A breaking CRD change usually means a new API version, and we're not there yet. To explore this further, could you help us understand the underlying issue by providing example manifests and any alternative solutions you've considered? Breaking changes are generally something we try to avoid, so I'm not going to review that. |
|
I can switch the script to keep current behavior and allow new flag (Default: With set apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
name: demo
labels:
demo: foo
annotations:
external-dns.demo.io/access: public
spec:
endpoints:
- dnsName: demo.example.com
recordTTL: 300
recordType: CNAME
targets:
- placeholderI'd like to avoid placeholders while allowing to override Expected CRD that should be allowed with apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
name: demo
labels:
demo: foo
annotations:
external-dns.demo.io/access: public
spec:
endpoints:
- dnsName: demo.example.com
recordTTL: 300
recordType: CNAME |
|
I'm working on path to beta at the moment #5243 |
|
For this case Which provider you using, and what the result DNS is expected |
|
It's Cloudflare: Result DNS with In PR, you can see if we do set |
|
Not sure about practicality, may need some time to re-think. How this DNSEndpoint(s) created? I'm using helm, example relevant part {{- range $hostNames }}
{{- if .name }}
{{- $target := printf "eks-cluster-%s-ingress-%s.%s" $.Values.cluster.env $accessType $defaultDomainName }}
{{- $subdomain := printf "%s-%s-eks" .name $accessType }}
{{- if and (eq (include "this.allPortsAreGrpc" $) "false") (ne .protocol "gRPC") }}
- dnsName: {{ $subdomain }}.{{ $defaultDomainName }}
recordTTL: 60
recordType: CNAME
targets:
- {{ $target }}
{{- end }}
{{- if and (eq $accessType "internal") (eq (include "this.http2Enabled" $) "true") }}
- dnsName: {{ $subdomain }}.http2.{{ $defaultDomainName }}
recordTTL: 60
recordType: CNAME
targets:
- {{ $target }}
{{- end }}
{{- end }}
{{- end }}
or
- dnsName: {{ .Values.dns.host }}
recordTTL: 60
recordType: CNAME
targets:
- {{ include "common.dns.target" . }} # target or default targetI may missing some other use case Or similar with kustomize or kyverno admission webhooks. |
|
It's simple:
This is PR to improve 4th step. Another approach I'm using is different starting with 3rd step, which is setting annotations on Service of type Both approaches have product teams using DNSEndpoint resources, while load balancer DNS records and ExternalDNS instances are maintained by operations team. Product team just creates proper DNSEndpoint with annotation for public or private (again, bummer is it needs to contain |
If I understand correctly, we have exactly same cases in environments I look after. For the use case you shared, I'm not sure how you setting annotations, but if annotations on ALB are set with helm, you could add a template for DNSEndpoint as well and set all required values, use kustomize and any other tools. The targets are static, so it works. If teams using Crossplane, it could manage DNSEndpoint targets as well at runtime. Regardless of how DNSEndpoint manifests are generated (Helm, Kustomize, Admission WebHooks, ArgoCD, Jsonnet, etc.), any DNSEndpoint with the I apologise, but I don't understand the proposed approach and don't see any clear benefits. However, other contributors or maintainers might have a different perspective and see value in this feature. |
|
Again, it's simple proposition: No need for Additional comment on your suggestion that it's easy to add |
|
We have differing opinions on this. In my view, the number of DNSEndpoints shouldn't matter – automation tools exist to handle scaling so 1, 100, 10000 should not be matter. If other maintainers or contributors believe external-dns should support this specific scenario, I don't see a fundamental conflict as consensus is not needed. However, from my perspective, this problem seems too niche and is better addressed with dedicated tools. This is just my personal opinion, and others may have different views. |
|
Maybe CRD should support FQDN templating |
|
All good. I'm not attached to this, only offered a fix to make it better for our use case. I can close the PR. |
|
@alen-z do you wanna try FQDN approach? @mloiseleur wdyt? |
|
@ivankatliarchuk To me, this use case is quite straightforward and valid. We are using the same pattern on some clusters: a single target (the LB) with as many CNAME as we need. So, setting it once in the CLI arg looks better to me than set the same value in all the CR. It's also consistent: that's how other sources works. Since there is no CEL validation ATM in the CRD, I'm not sure if this is really a breaking change. With current version, it will fail without target so, to me, a warning in release notes should be enough. But maybe I missed something, I need to double check that. |
|
@mloiseleur, in terms of looking at breaking change, depends on the approach (current PR or one other possible non-breaking). Challenge is, to make current ExternalDNS work, many have put placeholders in CRD To me, it's best to be able to upgrade to new version, clean DNSEndpoint under the flag, remove the flag. This actually requires this PR to be a bit more improved to allow empty If you folks align on the approach, I can prepare PR. |
|
@alen-z We took the time to discuss it with @szuecs @Raffo and @ivankatliarchuk . We are aligned on this approach. You can prepare the PR. |
|
Feel free to re-open this one or open a new one. |
|
Hey @mloiseleur and all involved, sounds good. Appreciate you folks spending time on this one. I'll re-open the PR.
I'll follow through with this approach and ask for review. ⚾ is in my court now. Will keep you posted. |
k8s-ci-robot
added
needs-rebase
|
Hi @mloiseleur, re-opened the PR and added new test. Now we should be able to:
|
k8s-ci-robot
removed
the
needs-rebase
alen-z
changed the title
source/crd.go
Outdated
| if ep.Labels == nil { | ||
| ep.Labels = endpoint.NewLabels() | ||
| } | ||
|
|
||
| // Add the valid endpoint to a temporary list for this CRD |
There was a problem hiding this comment.
probably worth to rename crdEndpoints to something more meaningful to avoid a comment?
There was a problem hiding this comment.
What is you preferred name for variable?
source/crd.go
Outdated
| log.Warnf("Endpoint %s with DNSName %s has an empty list of targets", dnsEndpoint.Name, ep.DNSName) | ||
| continue | ||
| } | ||
| // Note: Target validation (like empty target check for A/CNAME) is removed here |
There was a problem hiding this comment.
This check was quite useful. Moving forward, as cluster operators, we'll no longer have automatic indicators of incorrectly configured CRDs by teams or tenants. Therefore, cluster operators or SREs will need to be aware that external-dns behavior has changed, and they'll be responsible for manually checking CRD configurations if DNS issues arise, as automatic compensation is no longer in place.
What we should have
- If flag true - skip extra checks
- if flag false - same as before behaviour/check
There was a problem hiding this comment.
If you read #5316 (comment), legacy mode still improved compared to current state in a sense it allows empty targets which this check would fail to allow.
Why improved legacy? It still allows running the current legacy definitions, but prepare them to new way of managing resources and business logic.
If we were to follow your advice, migration would require all DNSEndpoint definitions to be changed at once when legacy mode is turned off. My suggested approach allows to run in legacy, adjust all DNSEndpoint resources one by one and then just turn off legacy mode (this is what current code implements). I'd suggest we keep this PR behavior. Is it fine by you?
| result = append(result, eps...) | ||
| } else { | ||
| // This case should logically not be reached given the conditions above, but handles completeness. | ||
| result = append(result, endpoints[i]) |
There was a problem hiding this comment.
not covered with unit test
|
|
||
| for _, s := range ms.children { | ||
| endpoints, err := s.Endpoints(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(ms.defaultTargets) > 0 { | ||
|
|
||
| if hasDefaultTargets { |
There was a problem hiding this comment.
nesting is quite deep. can we reduce it?
if !hasDefaultTargets {
result = append(result, endpoints...)
continue
}
for i := range endpoints {
hasSourceTargets := len(endpoints[i].Targets) > 0
if !ms.forceDefaultTargets && hasSourceTargets {
// New behavior (default): Source targets exist, use them and ignore defaults.
// Log a warning every time this happens if defaults are configured.
log.Warnf("Source provided targets for %q (%s), ignoring default targets [%s] due to new behavior. Use --force-default-targets to revert to old behavior.", endpoints[i].DNSName, endpoints[i].RecordType, strings.Join(ms.defaultTargets, ", "))
result = append(result, endpoints[i])
continue
}
if ms.forceDefaultTargets || !hasSourceTargets {
// Old behavior (forced via flag) OR New behavior (source targets are empty): Apply default targets.
eps := endpointsForHostname(endpoints[i].DNSName, ms.defaultTargets, endpoints[i].RecordTTL, endpoints[i].ProviderSpecific, endpoints[i].SetIdentifier, "")
for _, ep := range eps {
ep.Labels = endpoints[i].Labels
}
result = append(result, eps...)
continue
}
// This case should logically not be reached given the conditions above, but handles completeness.
result = append(result, endpoints[i])
}^ just an example, not necessary an exact solution
There was a problem hiding this comment.
reducing to this, may get us to something even simpler
for i := range endpoints {
hasSourceTargets := len(endpoints[i].Targets) > 0
if ms.forceDefaultTargets || !hasSourceTargets {
// Old behavior (forced via flag) OR New behavior (source targets are empty): Apply default targets.
eps := endpointsForHostname(endpoints[i].DNSName, ms.defaultTargets, endpoints[i].RecordTTL, endpoints[i].ProviderSpecific, endpoints[i].SetIdentifier, "")
for _, ep := range eps {
ep.Labels = endpoints[i].Labels
}
result = append(result, eps...)
continue
}
if !ms.forceDefaultTargets && hasSourceTargets {
log.Warnf("Source provided targets for %q (%s), ignoring default targets [%s] due to new behavior. Use --force-default-targets to revert to old behavior.", endpoints[i].DNSName, endpoints[i].RecordType, strings.Join(ms.defaultTargets, ", "))
}
// This case should logically not be reached given the conditions above, but handles completeness.
result = append(result, endpoints[i])
}
| eps := endpointsForHostname(endpoints[i].DNSName, ms.defaultTargets, endpoints[i].RecordTTL, endpoints[i].ProviderSpecific, endpoints[i].SetIdentifier, "") | ||
| for _, ep := range eps { | ||
| ep.Labels = endpoints[i].Labels | ||
| hasSourceTargets := len(endpoints[i].Targets) > 0 |
There was a problem hiding this comment.
for _, endpoint := range endpoints {
....So no longer required a construct endpoints[i].Targets and similar
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
|
Hey @ivankatliarchuk, appreciate suggestions. Let me send improvements your way :) |
mloiseleur
changed the title
|
@alen-z: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
|
Let us know when ready. What else is required, share results of a smoke tests with flag enabled/disabled, my understanding this are the maniests #5316 (comment). This should help to speed up a review. |
Description
Improved
default-targetsbehavior to allow empty CRDtargetssection in case default targets are set. Default targets are overridden if it's set anywhere else. Since it introduces breaking change in situations where targets are set, we offerforce-default-targetsflag to keep current behavior (hopefully easing migration).Breaking change should be stated in release documentation.
Fixes #3163
Checklist