-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(source)!: introduce optional force-default-targets #5316
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
569a82a
6b84cff
01f71b0
4418a37
a5e3ee0
8458661
404d464
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -180,14 +180,12 @@ func (cs *crdSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, error | |
} | ||
|
||
for _, dnsEndpoint := range result.Items { | ||
// Make sure that all endpoints have targets for A or CNAME type | ||
crdEndpoints := []*endpoint.Endpoint{} | ||
crdEndpoints := []*endpoint.Endpoint{} // Temporary list for endpoints from this specific CRD | ||
for _, ep := range dnsEndpoint.Spec.Endpoints { | ||
if (ep.RecordType == "CNAME" || ep.RecordType == "A" || ep.RecordType == "AAAA") && len(ep.Targets) < 1 { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
// to allow default-targets logic to function correctly. | ||
|
||
// Validate target format (e.g., trailing dots) | ||
illegalTarget := false | ||
for _, target := range ep.Targets { | ||
if ep.RecordType != "NAPTR" && strings.HasSuffix(target, ".") { | ||
|
@@ -200,18 +198,23 @@ func (cs *crdSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, error | |
} | ||
} | ||
if illegalTarget { | ||
log.Warnf("Endpoint %s with DNSName %s has an illegal target. The subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com')", dnsEndpoint.Name, ep.DNSName) | ||
continue | ||
log.Warnf("Endpoint %s/%s with DNSName %s has an illegal target format.", dnsEndpoint.Namespace, dnsEndpoint.Name, ep.DNSName) | ||
continue // Skip this specific endpoint | ||
} | ||
|
||
// Ensure labels are initialized | ||
alen-z marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably worth to rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is you preferred name for variable? |
||
crdEndpoints = append(crdEndpoints, ep) | ||
} | ||
|
||
// Add resource label to all valid endpoints from this CRD | ||
alen-z marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cs.setResourceLabel(&dnsEndpoint, crdEndpoints) | ||
|
||
// Add the processed endpoints for this CRD to the main list | ||
endpoints = append(endpoints, crdEndpoints...) | ||
|
||
if dnsEndpoint.Status.ObservedGeneration == dnsEndpoint.Generation { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,34 +18,54 @@ | |
|
||
import ( | ||
"context" | ||
"strings" | ||
|
||
log "github.com/sirupsen/logrus" | ||
"sigs.k8s.io/external-dns/endpoint" | ||
) | ||
|
||
// multiSource is a Source that merges the endpoints of its nested Sources. | ||
type multiSource struct { | ||
children []Source | ||
defaultTargets []string | ||
children []Source | ||
defaultTargets []string | ||
forceDefaultTargets bool | ||
} | ||
|
||
// Endpoints collects endpoints of all nested Sources and returns them in a single slice. | ||
func (ms *multiSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, error) { | ||
result := []*endpoint.Endpoint{} | ||
hasDefaultTargets := len(ms.defaultTargets) > 0 | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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])
} |
||
for i := range endpoints { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for _, endpoint := range endpoints {
.... So no longer required a construct |
||
|
||
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 | ||
} else 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...) | ||
} 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
result = append(result, eps...) | ||
} | ||
} else { | ||
// No default targets configured, just append source endpoints. | ||
result = append(result, endpoints...) | ||
} | ||
} | ||
|
@@ -60,6 +80,6 @@ | |
} | ||
|
||
// NewMultiSource creates a new multiSource. | ||
func NewMultiSource(children []Source, defaultTargets []string) Source { | ||
return &multiSource{children: children, defaultTargets: defaultTargets} | ||
func NewMultiSource(children []Source, defaultTargets []string, forceDefaultTargets bool) Source { | ||
return &multiSource{children: children, defaultTargets: defaultTargets, forceDefaultTargets: forceDefaultTargets} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.