Skip to content

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion controller/execute.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func Execute() {
targetFilter := endpoint.NewTargetNetFilterWithExclusions(cfg.TargetNetFilter, cfg.ExcludeTargetNets)

// Combine multiple sources into a single, deduplicated source.
endpointsSource := source.NewDedupSource(source.NewMultiSource(sources, sourceCfg.DefaultTargets))
endpointsSource := source.NewDedupSource(source.NewMultiSource(sources, sourceCfg.DefaultTargets, cfg.ForceDefaultTargets))
endpointsSource = source.NewNAT64Source(endpointsSource, cfg.NAT64Networks)
endpointsSource = source.NewTargetFilterSource(endpointsSource, targetFilter)

Expand Down
1 change: 1 addition & 0 deletions docs/flags.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
| `--service-type-filter=SERVICE-TYPE-FILTER` | The service types to take care about (default: all, expected: ClusterIP, NodePort, LoadBalancer or ExternalName) |
| `--source=source` | The resource types that are queried for endpoints; specify multiple times for multiple sources (required, options: service, ingress, node, pod, fake, connector, gateway-httproute, gateway-grpcroute, gateway-tlsroute, gateway-tcproute, gateway-udproute, istio-gateway, istio-virtualservice, cloudfoundry, contour-httpproxy, gloo-proxy, crd, empty, skipper-routegroup, openshift-route, ambassador-host, kong-tcpingress, f5-virtualserver, f5-transportserver, traefik-proxy) |
| `--target-net-filter=TARGET-NET-FILTER` | Limit possible targets by a net filter; specify multiple times for multiple possible nets (optional) |
| `--[no-]force-default-targets` | Force the application of --default-targets, overriding any targets provided by the source (DEPRECATED: This reverts to legacy behavior, default is false) |
| `--[no-]traefik-disable-legacy` | Disable listeners on Resources under the traefik.containo.us API Group |
| `--[no-]traefik-disable-new` | Disable listeners on Resources under the traefik.io API Group |
| `--provider=provider` | The DNS provider where the DNS records will be created (required, options: akamai, alibabacloud, aws, aws-sd, azure, azure-dns, azure-private-dns, civo, cloudflare, coredns, digitalocean, dnsimple, exoscale, gandi, godaddy, google, ibmcloud, inmemory, linode, ns1, oci, ovh, pdns, pihole, plural, rfc2136, scaleway, skydns, tencentcloud, transip, ultradns, webhook) |
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/externaldns/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ type Config struct {
TraefikDisableNew bool
NAT64Networks []string
ExcludeUnschedulable bool
ForceDefaultTargets bool
}

var defaultConfig = &Config{
Expand Down Expand Up @@ -383,6 +384,7 @@ var defaultConfig = &Config{
WebhookProviderWriteTimeout: 10 * time.Second,
WebhookServer: false,
ZoneIDFilter: []string{},
ForceDefaultTargets: false,
}

// NewConfig returns new Config object
Expand Down Expand Up @@ -466,6 +468,7 @@ func App(cfg *Config) *kingpin.Application {
app.Flag("crd-source-apiversion", "API version of the CRD for crd source, e.g. `externaldns.k8s.io/v1alpha1`, valid only when using crd source").Default(defaultConfig.CRDSourceAPIVersion).StringVar(&cfg.CRDSourceAPIVersion)
app.Flag("crd-source-kind", "Kind of the CRD for the crd source in API group and version specified by crd-source-apiversion").Default(defaultConfig.CRDSourceKind).StringVar(&cfg.CRDSourceKind)
app.Flag("default-targets", "Set globally default host/IP that will apply as a target instead of source addresses. Specify multiple times for multiple targets (optional)").StringsVar(&cfg.DefaultTargets)
app.Flag("force-default-targets", "Force the application of --default-targets, overriding any targets provided by the source (DEPRECATED: This reverts to improved legacy behavior which allows empty CRD targets, default is false)").Default(strconv.FormatBool(defaultConfig.ForceDefaultTargets)).BoolVar(&cfg.ForceDefaultTargets)
app.Flag("exclude-record-types", "Record types to exclude from management; specify multiple times to exclude many; (optional)").Default().StringsVar(&cfg.ExcludeDNSRecordTypes)
app.Flag("exclude-target-net", "Exclude target nets (optional)").StringsVar(&cfg.ExcludeTargetNets)
app.Flag("exclude-unschedulable", "Exclude nodes that are considered unschedulable (default: true)").Default(strconv.FormatBool(defaultConfig.ExcludeUnschedulable)).BoolVar(&cfg.ExcludeUnschedulable)
Expand Down
9 changes: 2 additions & 7 deletions source/crd.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,8 @@ 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{}
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
}

illegalTarget := false
for _, target := range ep.Targets {
if ep.RecordType != "NAPTR" && strings.HasSuffix(target, ".") {
Expand All @@ -200,7 +194,7 @@ 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)
log.Warnf("Endpoint %s/%s with DNSName %s has an illegal target format.", dnsEndpoint.Namespace, dnsEndpoint.Name, ep.DNSName)
continue
}

Expand All @@ -212,6 +206,7 @@ func (cs *crdSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, error
}

cs.setResourceLabel(&dnsEndpoint, crdEndpoints)

endpoints = append(endpoints, crdEndpoints...)

if dnsEndpoint.Status.ObservedGeneration == dnsEndpoint.Generation {
Expand Down
6 changes: 3 additions & 3 deletions source/crd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ func testCRDSourceEndpoints(t *testing.T) {
expectError: false,
},
{
title: "invalid crd with no targets",
title: "valid crd with no targets (relies on default-targets)",
registeredAPIVersion: "test.k8s.io/v1alpha1",
apiVersion: "test.k8s.io/v1alpha1",
registeredKind: "DNSEndpoint",
Expand All @@ -225,13 +225,13 @@ func testCRDSourceEndpoints(t *testing.T) {
registeredNamespace: "foo",
endpoints: []*endpoint.Endpoint{
{
DNSName: "abc.example.org",
DNSName: "no-targets.example.org",
Targets: endpoint.Targets{},
RecordType: endpoint.RecordTypeA,
RecordTTL: 180,
},
},
expectEndpoints: false,
expectEndpoints: true,
expectError: false,
},
{
Expand Down
33 changes: 24 additions & 9 deletions source/multisource.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,32 +18,47 @@

import (
"context"
"strings"

log "github.com/sirupsen/logrus"

Check failure on line 23 in source/multisource.go

View workflow job for this annotation

GitHub Actions / Markdown, Go and OAS

File is not properly formatted (goimports)
"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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for _, endpoint := range endpoints {
....

So no longer required a construct endpoints[i].Targets and similar


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, ", "))
result = append(result, endpoints[i])
continue
} else if ms.forceDefaultTargets || !hasSourceTargets {
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 {
result = append(result, endpoints[i])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not covered with unit test

Screenshot 2025-05-18 at 12 45 01

}
result = append(result, eps...)
}
} else {
result = append(result, endpoints...)
Expand All @@ -60,6 +75,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}
}
174 changes: 137 additions & 37 deletions source/multisource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func testMultiSourceEndpoints(t *testing.T) {
}

// Create our object under test and get the endpoints.
source := NewMultiSource(sources, nil)
source := NewMultiSource(sources, nil, false)

// Get endpoints from the source.
endpoints, err := source.Endpoints(context.Background())
Expand All @@ -116,7 +116,7 @@ func testMultiSourceEndpointsWithError(t *testing.T) {
src.On("Endpoints").Return(nil, errSomeError)

// Create our object under test and get the endpoints.
source := NewMultiSource([]Source{src}, nil)
source := NewMultiSource([]Source{src}, nil, false)

// Get endpoints from our source.
_, err := source.Endpoints(context.Background())
Expand All @@ -127,44 +127,144 @@ func testMultiSourceEndpointsWithError(t *testing.T) {
}

func testMultiSourceEndpointsDefaultTargets(t *testing.T) {
// Create the expected default targets
defaultTargetsA := []string{"127.0.0.1", "127.0.0.2"}
defaultTargetsAAAA := []string{"2001:db8::1"}
defaultTargetsCName := []string{"foo.example.org"}
defaultTargets := append(defaultTargetsA, defaultTargetsCName...)
defaultTargets = append(defaultTargets, defaultTargetsAAAA...)
labels := endpoint.Labels{"foo": "bar"}

// Create the expected endpoints
expectedEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "foo", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "foo", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
}

// Create the source endpoints with different targets
sourceEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: endpoint.Targets{"8.8.8.8"}, Labels: labels},
{DNSName: "bar", Targets: endpoint.Targets{"8.8.4.4"}, Labels: labels},
}
t.Run("Defaults applied when source targets are empty", func(t *testing.T) {
defaultTargetsA := []string{"127.0.0.1", "127.0.0.2"}
defaultTargetsAAAA := []string{"2001:db8::1"}
defaultTargetsCName := []string{"foo.example.org"}
defaultTargets := append(defaultTargetsA, defaultTargetsCName...)
defaultTargets = append(defaultTargets, defaultTargetsAAAA...)
labels := endpoint.Labels{"foo": "bar"}

// Endpoints FROM SOURCE has NO targets
sourceEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: endpoint.Targets{}, Labels: labels},
{DNSName: "bar", Targets: endpoint.Targets{}, Labels: labels},
}

// Expected endpoints SHOULD HAVE the default targets applied
expectedEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "foo", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "foo", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
}

src := new(testutils.MockSource)
src.On("Endpoints").Return(sourceEndpoints, nil)

// Test with forceDefaultTargets=false (default behavior)
source := NewMultiSource([]Source{src}, defaultTargets, false)

endpoints, err := source.Endpoints(context.Background())
require.NoError(t, err)

validateEndpoints(t, endpoints, expectedEndpoints)

src.AssertExpectations(t)
})

t.Run("Defaults NOT applied when source targets exist", func(t *testing.T) {
defaultTargets := []string{"127.0.0.1"} // Default target
labels := endpoint.Labels{"foo": "bar"}

// Endpoints FROM SOURCE HAS targets
sourceEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: endpoint.Targets{"8.8.8.8"}, Labels: labels},
{DNSName: "bar", Targets: endpoint.Targets{"8.8.4.4"}, Labels: labels},
}

// Expected endpoints SHOULD MATCH the source endpoints (defaults ignored)
expectedEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: endpoint.Targets{"8.8.8.8"}, Labels: labels},
{DNSName: "bar", Targets: endpoint.Targets{"8.8.4.4"}, Labels: labels},
}

src := new(testutils.MockSource)
src.On("Endpoints").Return(sourceEndpoints, nil)

// Test with forceDefaultTargets=false (default behavior)
source := NewMultiSource([]Source{src}, defaultTargets, false)

endpoints, err := source.Endpoints(context.Background())
require.NoError(t, err)

validateEndpoints(t, endpoints, expectedEndpoints)

src.AssertExpectations(t)
})

t.Run("Defaults forced when source targets exist and flag is set", func(t *testing.T) {
defaultTargetsA := []string{"127.0.0.1", "127.0.0.2"}
defaultTargetsAAAA := []string{"2001:db8::1"}
defaultTargetsCName := []string{"foo.example.org"}
defaultTargets := append(defaultTargetsA, defaultTargetsCName...)
defaultTargets = append(defaultTargets, defaultTargetsAAAA...)
labels := endpoint.Labels{"foo": "bar"}

// Endpoints FROM SOURCE HAS targets
sourceEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: endpoint.Targets{"8.8.8.8"}, Labels: labels},
{DNSName: "bar", Targets: endpoint.Targets{"8.8.4.4"}, Labels: labels},
}

// Expected endpoints SHOULD HAVE the default targets applied (old behavior)
expectedEndpoints := []*endpoint.Endpoint{
{DNSName: "foo", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "foo", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "foo", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
{DNSName: "bar", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
}

src := new(testutils.MockSource)
src.On("Endpoints").Return(sourceEndpoints, nil)

// Test with forceDefaultTargets=true (legacy behavior)
source := NewMultiSource([]Source{src}, defaultTargets, true)

endpoints, err := source.Endpoints(context.Background())
require.NoError(t, err)

validateEndpoints(t, endpoints, expectedEndpoints)

src.AssertExpectations(t)
})

t.Run("Defaults applied when source targets are empty and flag is set", func(t *testing.T) {
defaultTargetsA := []string{"127.0.0.1", "127.0.0.2"}
defaultTargetsAAAA := []string{"2001:db8::1"}
defaultTargetsCName := []string{"foo.example.org"}
defaultTargets := append(defaultTargetsA, defaultTargetsAAAA...)
defaultTargets = append(defaultTargets, defaultTargetsCName...)

labels := endpoint.Labels{"foo": "bar"}

// Endpoints FROM SOURCE has NO targets
sourceEndpoints := []*endpoint.Endpoint{
{DNSName: "empty-target-test", Targets: endpoint.Targets{}, Labels: labels},
}

// Create a mocked source returning source targets
src := new(testutils.MockSource)
src.On("Endpoints").Return(sourceEndpoints, nil)
// Expected endpoints SHOULD HAVE the default targets applied
expectedEndpoints := []*endpoint.Endpoint{
{DNSName: "empty-target-test", Targets: defaultTargetsA, RecordType: "A", Labels: labels},
{DNSName: "empty-target-test", Targets: defaultTargetsAAAA, RecordType: "AAAA", Labels: labels},
{DNSName: "empty-target-test", Targets: defaultTargetsCName, RecordType: "CNAME", Labels: labels},
}

// Create our object under test with non-empty defaultTargets and get the endpoints.
source := NewMultiSource([]Source{src}, defaultTargets)
src := new(testutils.MockSource)
src.On("Endpoints").Return(sourceEndpoints, nil)

// Get endpoints from our source.
endpoints, err := source.Endpoints(context.Background())
require.NoError(t, err)
// Test with forceDefaultTargets=true
source := NewMultiSource([]Source{src}, defaultTargets, true)

// Validate returned endpoints against desired endpoints.
validateEndpoints(t, endpoints, expectedEndpoints)
endpoints, err := source.Endpoints(context.Background())
require.NoError(t, err)

validateEndpoints(t, endpoints, expectedEndpoints)

// Validate that the nested sources were called.
src.AssertExpectations(t)
src.AssertExpectations(t)
})
}
Loading