From 28f9e9c06eeb6e13a2a4053a4f34c778061c6af7 Mon Sep 17 00:00:00 2001 From: Alen Zubic Date: Wed, 18 Jun 2025 08:48:51 +0200 Subject: [PATCH] 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 --- controller/execute.go | 2 +- docs/flags.md | 1 + docs/tutorials/crd.md | 71 ++++++++++++++ pkg/apis/externaldns/types.go | 3 + source/crd.go | 6 +- source/crd_test.go | 6 +- source/multisource.go | 31 +++++-- source/multisource_test.go | 170 +++++++++++++++++++++++++++------- source/store.go | 2 + 9 files changed, 241 insertions(+), 51 deletions(-) create mode 100644 docs/tutorials/crd.md diff --git a/controller/execute.go b/controller/execute.go index 19c9ad736..0b0e427cf 100644 --- 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) diff --git a/docs/flags.md b/docs/flags.md index 7211ecb7e..c881660c3 100644 --- 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) | diff --git a/docs/tutorials/crd.md b/docs/tutorials/crd.md new file mode 100644 index 000000000..81834a8b9 --- /dev/null +++ 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. diff --git a/pkg/apis/externaldns/types.go b/pkg/apis/externaldns/types.go index 635c8aedc..fa5818bd6 100644 --- 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) diff --git a/source/crd.go b/source/crd.go index 9c46b6c74..a0c54e1e0 100644 --- 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 } diff --git a/source/crd_test.go b/source/crd_test.go index f08fc01ac..6214c74b3 100644 --- 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, }, { diff --git a/source/multisource.go b/source/multisource.go index 542b7d93d..80f5335b8 100644 --- 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} } diff --git a/source/multisource_test.go b/source/multisource_test.go index fcd9c7a40..8386c75b4 100644 --- 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) + }) } diff --git a/source/store.go b/source/store.go index dc5812ad5..82a03f1b9 100644 --- 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,