feat(source)!: introduce optional force-default-targets (kubernetes-sigs#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

Author: alen-z

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)

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) |

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.

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)

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
 			}
 

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,
 		},
 		{

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}
 }