feat(source)!: introduce optional force-default-targets (#5316)

* BREAKING CHANGE: Improve default targets management * fix: Remove old test case * fix: Test confirming legacy mode allows empty CRD targets * fix: Remove comments * fix: Move flag definition closer to detault-targets * fix: Initial merge adaptation * fix: Improved legacy needs a chance to work with empty CRD list * fix: Code coverage and dead code * fix: Simpler Endpoints logic * fix: Flag description * feat: Add tutorial * fix: Improve linting * fix: Improve linting * fix: Import linting
2025-08-05 17:16:59 +02:00 · 2025-06-18 08:48:51 +02:00 · 2025-06-18 08:48:51 +02:00 · 28f9e9c06e
commit 28f9e9c06e
parent 675cc7c03f
9 changed files with 241 additions and 51 deletions
--- a/controller/execute.go
+++ b/controller/execute.go
@ -420,7 +420,7 @@ func buildSource(ctx context.Context, cfg *externaldns.Config) (source.Source, e
 		return nil, err
 	}
 	// Combine multiple sources into a single, deduplicated source.
-	combinedSource := source.NewDedupSource(source.NewMultiSource(sources, sourceCfg.DefaultTargets))
+	combinedSource := source.NewDedupSource(source.NewMultiSource(sources, sourceCfg.DefaultTargets, sourceCfg.ForceDefaultTargets))
 	// Filter targets
 	targetFilter := endpoint.NewTargetNetFilterWithExclusions(cfg.TargetNetFilter, cfg.ExcludeTargetNets)
 	combinedSource = source.NewNAT64Source(combinedSource, cfg.NAT64Networks)
--- a/docs/flags.md
+++ b/docs/flags.md
@ -25,6 +25,7 @@
 | `--crd-source-apiversion="externaldns.k8s.io/v1alpha1"` | API version of the CRD for crd source, e.g. `externaldns.k8s.io/v1alpha1`, valid only when using crd source |
 | `--crd-source-kind="DNSEndpoint"` | Kind of the CRD for the crd source in API group and version specified by crd-source-apiversion |
 | `--default-targets=DEFAULT-TARGETS` | Set globally default host/IP that will apply as a target instead of source addresses. Specify multiple times for multiple targets (optional) |
+| `--[no-]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 for migration to new state) |
 | `--exclude-record-types=EXCLUDE-RECORD-TYPES` | Record types to exclude from management; specify multiple times to exclude many; (optional) |
 | `--exclude-target-net=EXCLUDE-TARGET-NET` | Exclude target nets (optional) |
 | `--[no-]exclude-unschedulable` | Exclude nodes that are considered unschedulable (default: true) |
--- a/docs/tutorials/crd.md
+++ b/docs/tutorials/crd.md
@ -0,0 +1,71 @@
+# Using CRD Source for DNS Records
+
+This tutorial describes how to use the CRD source with ExternalDNS to manage DNS records. The CRD source allows you to define your desired DNS records declaratively using `DNSEndpoint` custom resources.
+
+## Default Targets and CRD Targets
+
+ExternalDNS has a `--default-targets` flag that can be used to specify a default set of targets for all created DNS records. The behavior of how these default targets interact with targets specified in a `DNSEndpoint` CRD has been refined.
+
+### New Behavior (default)
+
+By default, ExternalDNS now has the following behavior:
+
+- If a `DNSEndpoint` resource has targets specified in its `spec.endpoints[].targets` field, these targets will be used for the DNS record, **overriding** any targets specified via the `--default-targets` flag.
+- If a `DNSEndpoint` resource has an **empty** `targets` field, the targets from the `--default-targets` flag will be used. This allows for creating records that point to default load balancers or IPs without explicitly listing them in every `DNSEndpoint` resource.
+
+### Legacy Behavior (`--force-default-targets`)
+
+To maintain backward compatibility and support certain migration scenarios, the `--force-default-targets` flag is available.
+
+- When `--force-default-targets` is used, ExternalDNS will **always** use the targets from `--default-targets`, regardless of whether the `DNSEndpoint` resource has targets specified or not.
+This flag allows for a smooth migration path to the new behavior. It allow keeping old CRD resources, allows to start removing targets from one by one resource and then remove the flag.
+
+## Examples
+
+Let's look at how this works in practice. Assume ExternalDNS is running with `--default-targets=1.2.3.4`.
+
+### DNSEndpoint with Targets
+
+Here is a `DNSEndpoint` with a target specified.
+
+```yaml
+---
+apiVersion: externaldns.k8s.io/v1alpha1
+kind: DNSEndpoint
+metadata:
+  name: targets
+  namespace: default
+spec:
+  endpoints:
+  - dnsName: smoke-t.example.com
+    recordTTL: 300
+    recordType: CNAME
+    targets:
+      - placeholder
+```
+
+- **Without `--force-default-targets` (New Behavior):** A CNAME record for `smoke-t.example.com` will be created pointing to `placeholder`.
+- **With `--force-default-targets` (Legacy Behavior):** A CNAME record for `smoke-t.example.com` will be created pointing to `1.2.3.4`. The `placeholder` target will be ignored.
+
+### DNSEndpoint with Empty/No Targets
+
+Here is a `DNSEndpoint` without any targets specified.
+
+```yaml
+---
+apiVersion: externaldns.k8s.io/v1alpha1
+kind: DNSEndpoint
+metadata:
+  name: no-targets
+  namespace: default
+spec:
+  endpoints:
+  - dnsName: smoke-nt.example.com
+    recordTTL: 300
+    recordType: CNAME
+```
+
+- **Without `--force-default-targets` (New Behavior):** A CNAME record for `smoke-nt.example.com` will be created pointing to `1.2.3.4`.
+- **With `--force-default-targets` (Legacy Behavior):** A CNAME record for `smoke-nt.example.com` will be created pointing to `1.2.3.4`.
+
+`--force-default-targets` allows migration path to clean CRD resources.
--- a/pkg/apis/externaldns/types.go
+++ b/pkg/apis/externaldns/types.go
@ -213,6 +213,7 @@ type Config struct {
 	TraefikDisableNew                             bool
 	NAT64Networks                                 []string
 	ExcludeUnschedulable                          bool
+	ForceDefaultTargets                           bool
 }

 var defaultConfig = &Config{
@ -375,6 +376,7 @@ var defaultConfig = &Config{
 	WebhookProviderWriteTimeout:  10 * time.Second,
 	WebhookServer:                false,
 	ZoneIDFilter:                 []string{},
+	ForceDefaultTargets:          false,
 }

 // NewConfig returns new Config object
@ -458,6 +460,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 for migration to new state)").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)
--- a/source/crd.go
+++ b/source/crd.go
@ -182,12 +182,10 @@ 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
 		var crdEndpoints []*endpoint.Endpoint
 		for _, ep := range dnsEndpoint.Spec.Endpoints {
 			if (ep.RecordType == endpoint.RecordTypeCNAME || ep.RecordType == endpoint.RecordTypeA || ep.RecordType == endpoint.RecordTypeAAAA) && len(ep.Targets) < 1 {
-				log.Warnf("Endpoint %s with DNSName %s has an empty list of targets", dnsEndpoint.Name, ep.DNSName)
-				continue
+				log.Debugf("Endpoint %s with DNSName %s has an empty list of targets, allowing it to pass through for default-targets processing", dnsEndpoint.Name, ep.DNSName)
 			}

 			illegalTarget := false
@ -202,7 +200,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
 			}

--- a/source/crd_test.go
+++ b/source/crd_test.go
@ -224,7 +224,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",
@ -233,13 +233,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,
 		},
 		{
--- a/source/multisource.go
+++ b/source/multisource.go
@ -18,35 +18,50 @@ package source

 import (
 	"context"
+	"strings"

 	"sigs.k8s.io/external-dns/endpoint"
+
+	log "github.com/sirupsen/logrus"
 )

 // 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 {
-			for i := range endpoints {
+
+		if !hasDefaultTargets {
+			result = append(result, endpoints...)
+			continue
+		}
+
+		for i := range endpoints {
+			hasSourceTargets := len(endpoints[i].Targets) > 0
+
+			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...)
+				continue
 			}
-		} else {
-			result = append(result, endpoints...)
+
+			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])
 		}
 	}

@ -60,6 +75,6 @@ func (ms *multiSource) AddEventHandler(ctx context.Context, handler func()) {
 }

 // 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}
 }
--- a/source/multisource_test.go
+++ b/source/multisource_test.go
@ -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())
@ -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())
@ -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"}
+	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"}

-	// 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},
-	}
+		// Endpoints FROM SOURCE has NO targets
+		sourceEndpoints := []*endpoint.Endpoint{
+			{DNSName: "foo", Targets: endpoint.Targets{}, Labels: labels},
+			{DNSName: "bar", Targets: endpoint.Targets{}, 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},
-	}
+		// 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},
+		}

-	// Create a mocked source returning source targets
-	src := new(testutils.MockSource)
-	src.On("Endpoints").Return(sourceEndpoints, nil)
+		src := new(testutils.MockSource)
+		src.On("Endpoints").Return(sourceEndpoints, nil)

-	// Create our object under test with non-empty defaultTargets and get the endpoints.
-	source := NewMultiSource([]Source{src}, defaultTargets)
+		// Test with forceDefaultTargets=false (default behavior)
+		source := NewMultiSource([]Source{src}, defaultTargets, false)

-	// Get endpoints from our source.
-	endpoints, err := source.Endpoints(context.Background())
-	require.NoError(t, err)
+		endpoints, err := source.Endpoints(context.Background())
+		require.NoError(t, err)

-	// Validate returned endpoints against desired endpoints.
-	validateEndpoints(t, endpoints, expectedEndpoints)
+		validateEndpoints(t, endpoints, expectedEndpoints)

-	// Validate that the nested sources were called.
-	src.AssertExpectations(t)
+		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},
+		}
+
+		// 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},
+		}
+
+		src := new(testutils.MockSource)
+		src.On("Endpoints").Return(sourceEndpoints, nil)
+
+		// Test with forceDefaultTargets=true
+		source := NewMultiSource([]Source{src}, defaultTargets, true)
+
+		endpoints, err := source.Endpoints(context.Background())
+		require.NoError(t, err)
+
+		validateEndpoints(t, endpoints, expectedEndpoints)
+
+		src.AssertExpectations(t)
+	})
 }
--- a/source/store.go
+++ b/source/store.go
@ -78,6 +78,7 @@ type Config struct {
 	SkipperRouteGroupVersion       string
 	RequestTimeout                 time.Duration
 	DefaultTargets                 []string
+	ForceDefaultTargets            bool
 	OCPRouterName                  string
 	UpdateEvents                   bool
 	ResolveLoadBalancerHostname    bool
@ -123,6 +124,7 @@ func NewSourceConfig(cfg *externaldns.Config) *Config {
 		SkipperRouteGroupVersion:       cfg.SkipperRouteGroupVersion,
 		RequestTimeout:                 cfg.RequestTimeout,
 		DefaultTargets:                 cfg.DefaultTargets,
+		ForceDefaultTargets:            cfg.ForceDefaultTargets,
 		OCPRouterName:                  cfg.OCPRouterName,
 		UpdateEvents:                   cfg.UpdateEvents,
 		ResolveLoadBalancerHostname:    cfg.ResolveServiceLoadBalancerHostname,