From 09a6574c88e5abac40faed46d79cb9550864dbe5 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 12:06:34 +0000 Subject: [PATCH 1/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- controller/execute.go | 13 ++++++++----- controller/execute_test.go | 5 +++-- source/store.go | 12 +++++++++--- source/store_test.go | 32 ++++++++++++++++++++++---------- 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/controller/execute.go b/controller/execute.go index 2800966927..5ffc4ec28b 100644 --- a/controller/execute.go +++ b/controller/execute.go @@ -108,7 +108,8 @@ func Execute() { go serveMetrics(cfg.MetricsAddress) go handleSigterm(cancel) - endpointsSource, err := buildSource(ctx, cfg) + sCfg := source.NewSourceConfig(cfg) + endpointsSource, err := buildSource(ctx, sCfg) if err != nil { log.Fatal(err) // nolint: gocritic // exitAfterDefer } @@ -168,6 +169,9 @@ func buildProvider( zoneTypeFilter := provider.NewZoneTypeFilter(cfg.AWSZoneType) zoneTagFilter := provider.NewZoneTagFilter(cfg.AWSZoneTagFilter) + // TODO: refactor to move this to provider package, cover with tests + // TODO: Controller focuses on orchestration, not provider construction + // TODO: example provider.SelectProvider(cfg, ...) switch cfg.Provider { case "akamai": p, err = akamai.NewAkamaiProvider( @@ -413,9 +417,8 @@ func configureLogger(cfg *externaldns.Config) { // buildSource creates and configures the source(s) for endpoint discovery based on the provided configuration. // It initializes the source configuration, generates the required sources, and combines them into a single, // deduplicated source. Returns the combined source or an error if source creation fails. -func buildSource(ctx context.Context, cfg *externaldns.Config) (source.Source, error) { - sourceCfg := source.NewSourceConfig(cfg) - sources, err := source.ByNames(ctx, &source.SingletonClientGenerator{ +func buildSource(ctx context.Context, cfg *source.Config) (source.Source, error) { + sources, err := source.ByNames(ctx, cfg, &source.SingletonClientGenerator{ KubeConfig: cfg.KubeConfig, APIServerURL: cfg.APIServerURL, RequestTimeout: func() time.Duration { @@ -424,7 +427,7 @@ func buildSource(ctx context.Context, cfg *externaldns.Config) (source.Source, e } return cfg.RequestTimeout }(), - }, cfg.Sources, sourceCfg) + }) if err != nil { return nil, err } diff --git a/controller/execute_test.go b/controller/execute_test.go index ee49d5b9c3..c6c5e592af 100644 --- a/controller/execute_test.go +++ b/controller/execute_test.go @@ -33,6 +33,7 @@ import ( "github.com/stretchr/testify/require" "sigs.k8s.io/external-dns/endpoint" "sigs.k8s.io/external-dns/pkg/apis/externaldns" + "sigs.k8s.io/external-dns/source" ) // Logger @@ -267,7 +268,7 @@ func TestBuildSourceWithWrappers(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := buildSource(t.Context(), tt.cfg) + _, err := buildSource(t.Context(), source.NewSourceConfig(tt.cfg)) require.NoError(t, err) }) } @@ -440,7 +441,7 @@ func TestControllerRunCancelContextStopsLoop(t *testing.T) { } ctx, cancel := context.WithCancel(context.Background()) defer cancel() - src, err := buildSource(ctx, cfg) + src, err := buildSource(ctx, source.NewSourceConfig(cfg)) require.NoError(t, err) domainFilter := endpoint.NewDomainFilterWithOptions( endpoint.WithDomainFilter(cfg.DomainFilter), diff --git a/source/store.go b/source/store.go index bb166b5ad8..467d2a50d4 100644 --- a/source/store.go +++ b/source/store.go @@ -101,6 +101,12 @@ type Config struct { TraefikDisableNew bool ExcludeUnschedulable bool ExposeInternalIPv6 bool + ExcludeTargetNets []string + TargetNetFilter []string + NAT64Networks []string + MinTTL time.Duration + + sources []string } func NewSourceConfig(cfg *externaldns.Config) *Config { @@ -286,9 +292,9 @@ func (p *SingletonClientGenerator) OpenShiftClient() (openshift.Interface, error } // ByNames returns multiple Sources given multiple names. -func ByNames(ctx context.Context, p ClientGenerator, names []string, cfg *Config) ([]Source, error) { - sources := []Source{} - for _, name := range names { +func ByNames(ctx context.Context, cfg *Config, p ClientGenerator) ([]Source, error) { + sources := make([]Source, len(cfg.sources)) + for _, name := range cfg.sources { source, err := BuildWithConfig(ctx, name, p, cfg) if err != nil { return nil, err diff --git a/source/store_test.go b/source/store_test.go index 77e29fa50f..63578c604a 100644 --- a/source/store_test.go +++ b/source/store_test.go @@ -169,19 +169,24 @@ func (suite *ByNamesTestSuite) TestAllInitialized() { }: "IngressRouteUDPList", }), nil) - sources, err := ByNames(context.TODO(), mockClientGenerator, []string{ + ss := []string{ types.Service, types.Ingress, types.IstioGateway, types.ContourHTTPProxy, types.KongTCPIngress, types.F5VirtualServer, types.F5TransportServer, types.TraefikProxy, types.Fake, - }, &Config{}) + } + sources, err := ByNames(context.TODO(), &Config{ + sources: ss, + }, mockClientGenerator) suite.NoError(err, "should not generate errors") suite.Len(sources, 9, "should generate all nine sources") } func (suite *ByNamesTestSuite) TestOnlyFake() { mockClientGenerator := new(MockClientGenerator) - mockClientGenerator.On("KubeClient").Return(fakeKube.NewSimpleClientset(), nil) + mockClientGenerator.On("KubeClient").Return(fakeKube.NewClientset(), nil) - sources, err := ByNames(context.TODO(), mockClientGenerator, []string{types.Fake}, &Config{}) + sources, err := ByNames(context.TODO(), &Config{ + sources: []string{types.Fake}, + }, mockClientGenerator) suite.NoError(err, "should not generate errors") suite.Len(sources, 1, "should generate fake source") suite.Nil(mockClientGenerator.kubeClient, "client should not be created") @@ -189,9 +194,10 @@ func (suite *ByNamesTestSuite) TestOnlyFake() { func (suite *ByNamesTestSuite) TestSourceNotFound() { mockClientGenerator := new(MockClientGenerator) - mockClientGenerator.On("KubeClient").Return(fakeKube.NewSimpleClientset(), nil) - - sources, err := ByNames(context.TODO(), mockClientGenerator, []string{"foo"}, &Config{}) + mockClientGenerator.On("KubeClient").Return(fakeKube.NewClientset(), nil) + sources, err := ByNames(context.TODO(), &Config{ + sources: []string{"foo"}, + }, mockClientGenerator) suite.Equal(err, ErrSourceNotFound, "should return source not found") suite.Empty(sources, "should not returns any source") } @@ -207,7 +213,9 @@ func (suite *ByNamesTestSuite) TestKubeClientFails() { } for _, source := range sourcesDependentOnKubeClient { - _, err := ByNames(context.TODO(), mockClientGenerator, []string{source}, &Config{}) + _, err := ByNames(context.TODO(), &Config{ + sources: []string{source}, + }, mockClientGenerator) suite.Error(err, source+" should return an error if kubernetes client cannot be created") } } @@ -221,7 +229,9 @@ func (suite *ByNamesTestSuite) TestIstioClientFails() { sourcesDependentOnIstioClient := []string{types.IstioGateway, types.IstioVirtualService} for _, source := range sourcesDependentOnIstioClient { - _, err := ByNames(context.TODO(), mockClientGenerator, []string{source}, &Config{}) + _, err := ByNames(context.TODO(), &Config{ + sources: []string{source}, + }, mockClientGenerator) suite.Error(err, source+" should return an error if istio client cannot be created") } } @@ -238,7 +248,9 @@ func (suite *ByNamesTestSuite) TestDynamicKubernetesClientFails() { } for _, source := range sourcesDependentOnDynamicKubernetesClient { - _, err := ByNames(context.TODO(), mockClientGenerator, []string{source}, &Config{}) + _, err := ByNames(context.TODO(), &Config{ + sources: []string{source}, + }, mockClientGenerator) suite.Error(err, source+" should return an error if dynamic kubernetes client cannot be created") } } From 417b7edfdb4dba70cc5e03a639bcd6749e2f680c Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 12:32:56 +0000 Subject: [PATCH 2/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- source/store.go | 35 ++++++++++++++++++++++++++++- source/store_test.go | 52 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/source/store.go b/source/store.go index 467d2a50d4..727b38bc31 100644 --- a/source/store.go +++ b/source/store.go @@ -107,6 +107,10 @@ type Config struct { MinTTL time.Duration sources []string + + // clientGen is lazily initialized on first access for efficiency + clientGen *SingletonClientGenerator + clientGenOnce sync.Once } func NewSourceConfig(cfg *externaldns.Config) *Config { @@ -156,6 +160,35 @@ func NewSourceConfig(cfg *externaldns.Config) *Config { } } +// ClientGenerator returns a SingletonClientGenerator from this Config's connection settings. +// The generator is created once and cached for subsequent calls. +// This ensures consistent Kubernetes client creation across all sources using this configuration. +// +// The timeout behavior is special-cased: when UpdateEvents is true, the timeout is set to 0 +// (no timeout) to allow long-running watch operations for event-driven source updates. +func (cfg *Config) ClientGenerator() *SingletonClientGenerator { + // cfg.clientGen = &SingletonClientGenerator{ + // KubeConfig: cfg.KubeConfig, + // APIServerURL: cfg.APIServerURL, + // RequestTimeout: func() time.Duration { + // if cfg.UpdateEvents { + // return 0 + // } + // return cfg.RequestTimeout + // }(), + // } + return &SingletonClientGenerator{ + KubeConfig: cfg.KubeConfig, + APIServerURL: cfg.APIServerURL, + RequestTimeout: func() time.Duration { + if cfg.UpdateEvents { + return 0 + } + return cfg.RequestTimeout + }(), + } +} + // ClientGenerator provides clients for various Kubernetes APIs and external services. // This interface abstracts client creation and enables dependency injection for testing. // It uses the singleton pattern to ensure only one instance of each client is created @@ -293,7 +326,7 @@ func (p *SingletonClientGenerator) OpenShiftClient() (openshift.Interface, error // ByNames returns multiple Sources given multiple names. func ByNames(ctx context.Context, cfg *Config, p ClientGenerator) ([]Source, error) { - sources := make([]Source, len(cfg.sources)) + sources := make([]Source, 0, len(cfg.sources)) for _, name := range cfg.sources { source, err := BuildWithConfig(ctx, name, p, cfg) if err != nil { diff --git a/source/store_test.go b/source/store_test.go index 63578c604a..a9a5597ab2 100644 --- a/source/store_test.go +++ b/source/store_test.go @@ -20,9 +20,11 @@ import ( "context" "errors" "testing" + "time" "github.com/cloudfoundry-community/go-cfclient" openshift "github.com/openshift/client-go/route/clientset/versioned" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" istioclient "istio.io/client-go/pkg/clientset/versioned" @@ -206,13 +208,13 @@ func (suite *ByNamesTestSuite) TestKubeClientFails() { mockClientGenerator := new(MockClientGenerator) mockClientGenerator.On("KubeClient").Return(nil, errors.New("foo")) - sourcesDependentOnKubeClient := []string{ + sourceUnderTest := []string{ types.Node, types.Service, types.Ingress, types.Pod, types.IstioGateway, types.IstioVirtualService, types.AmbassadorHost, types.GlooProxy, types.TraefikProxy, types.CRD, types.KongTCPIngress, types.F5VirtualServer, types.F5TransportServer, } - for _, source := range sourcesDependentOnKubeClient { + for _, source := range sourceUnderTest { _, err := ByNames(context.TODO(), &Config{ sources: []string{source}, }, mockClientGenerator) @@ -238,7 +240,7 @@ func (suite *ByNamesTestSuite) TestIstioClientFails() { func (suite *ByNamesTestSuite) TestDynamicKubernetesClientFails() { mockClientGenerator := new(MockClientGenerator) - mockClientGenerator.On("KubeClient").Return(fakeKube.NewSimpleClientset(), nil) + mockClientGenerator.On("KubeClient").Return(fakeKube.NewClientset(), nil) mockClientGenerator.On("IstioClient").Return(istiofake.NewSimpleClientset(), nil) mockClientGenerator.On("DynamicKubernetesClient").Return(nil, errors.New("foo")) @@ -291,3 +293,47 @@ func TestBuildWithConfig_InvalidSource(t *testing.T) { t.Errorf("expected ErrSourceNotFound, got: %v", err) } } + +func TestConfig_ClientGenerator(t *testing.T) { + cfg := &Config{ + KubeConfig: "/path/to/kubeconfig", + APIServerURL: "https://api.example.com", + RequestTimeout: 30 * time.Second, + UpdateEvents: false, + } + + gen := cfg.ClientGenerator() + + assert.Equal(t, "/path/to/kubeconfig", gen.KubeConfig) + assert.Equal(t, "https://api.example.com", gen.APIServerURL) + assert.Equal(t, 30*time.Second, gen.RequestTimeout) +} + +func TestConfig_ClientGenerator_UpdateEvents(t *testing.T) { + cfg := &Config{ + KubeConfig: "/path/to/kubeconfig", + APIServerURL: "https://api.example.com", + RequestTimeout: 30 * time.Second, + UpdateEvents: true, // Special case + } + + gen := cfg.ClientGenerator() + + assert.Equal(t, time.Duration(0), gen.RequestTimeout, "UpdateEvents should set timeout to 0") +} + +func TestConfig_ClientGenerator_Caching(t *testing.T) { + cfg := &Config{ + KubeConfig: "/path/to/kubeconfig", + APIServerURL: "https://api.example.com", + RequestTimeout: 30 * time.Second, + UpdateEvents: false, + } + + // Call ClientGenerator twice + gen1 := cfg.ClientGenerator() + gen2 := cfg.ClientGenerator() + + // Should return the same instance (cached) + assert.Same(t, gen1, gen2, "ClientGenerator should return the same cached instance") +} From fe3e0e637dbdcf55e1dfeae423f9bf53356a5966 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 13:10:50 +0000 Subject: [PATCH 3/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- controller/execute.go | 11 +------- controller/execute_test.go | 10 +++++++- source/store.go | 51 +++++++++++++++++++++++--------------- 3 files changed, 41 insertions(+), 31 deletions(-) diff --git a/controller/execute.go b/controller/execute.go index 5ffc4ec28b..867ba971f6 100644 --- a/controller/execute.go +++ b/controller/execute.go @@ -418,16 +418,7 @@ func configureLogger(cfg *externaldns.Config) { // It initializes the source configuration, generates the required sources, and combines them into a single, // deduplicated source. Returns the combined source or an error if source creation fails. func buildSource(ctx context.Context, cfg *source.Config) (source.Source, error) { - sources, err := source.ByNames(ctx, cfg, &source.SingletonClientGenerator{ - KubeConfig: cfg.KubeConfig, - APIServerURL: cfg.APIServerURL, - RequestTimeout: func() time.Duration { - if cfg.UpdateEvents { - return 0 - } - return cfg.RequestTimeout - }(), - }) + sources, err := source.ByNames(ctx, cfg, cfg.ClientGenerator()) if err != nil { return nil, err } diff --git a/controller/execute_test.go b/controller/execute_test.go index c6c5e592af..646a2bc6d4 100644 --- a/controller/execute_test.go +++ b/controller/execute_test.go @@ -298,14 +298,21 @@ func TestHelperProcess(t *testing.T) { // runExecuteSubprocess runs Execute in a separate process and returns exit code and output. func runExecuteSubprocess(t *testing.T, args []string) (int, string, error) { t.Helper() + // make sure the subprocess does not run forever + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + cmdArgs := append([]string{"-test.run=TestHelperProcess", "--"}, args...) - cmd := exec.Command(os.Args[0], cmdArgs...) + cmd := exec.CommandContext(ctx, os.Args[0], cmdArgs...) cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") var buf bytes.Buffer cmd.Stdout = &buf cmd.Stderr = &buf err := cmd.Run() output := buf.String() + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return -1, output, ctx.Err() + } if err == nil { return 0, output, nil } @@ -375,6 +382,7 @@ func TestExecuteConfigValidationErrorExitsNonZero(t *testing.T) { // buildSource failure triggers log.Fatal. func TestExecuteBuildSourceErrorExitsNonZero(t *testing.T) { + // Use a valid source name (ingress) and an invalid kubeconfig path to // force client creation failure inside buildSource. code, _, err := runExecuteSubprocess(t, []string{ diff --git a/source/store.go b/source/store.go index 727b38bc31..dcc82cd296 100644 --- a/source/store.go +++ b/source/store.go @@ -157,6 +157,11 @@ func NewSourceConfig(cfg *externaldns.Config) *Config { TraefikDisableNew: cfg.TraefikDisableNew, ExcludeUnschedulable: cfg.ExcludeUnschedulable, ExposeInternalIPv6: cfg.ExposeInternalIPV6, + ExcludeTargetNets: cfg.ExcludeTargetNets, + TargetNetFilter: cfg.TargetNetFilter, + NAT64Networks: cfg.NAT64Networks, + MinTTL: cfg.MinTTL, + sources: cfg.Sources, } } @@ -167,26 +172,19 @@ func NewSourceConfig(cfg *externaldns.Config) *Config { // The timeout behavior is special-cased: when UpdateEvents is true, the timeout is set to 0 // (no timeout) to allow long-running watch operations for event-driven source updates. func (cfg *Config) ClientGenerator() *SingletonClientGenerator { - // cfg.clientGen = &SingletonClientGenerator{ - // KubeConfig: cfg.KubeConfig, - // APIServerURL: cfg.APIServerURL, - // RequestTimeout: func() time.Duration { - // if cfg.UpdateEvents { - // return 0 - // } - // return cfg.RequestTimeout - // }(), - // } - return &SingletonClientGenerator{ - KubeConfig: cfg.KubeConfig, - APIServerURL: cfg.APIServerURL, - RequestTimeout: func() time.Duration { - if cfg.UpdateEvents { - return 0 - } - return cfg.RequestTimeout - }(), - } + cfg.clientGenOnce.Do(func() { + cfg.clientGen = &SingletonClientGenerator{ + KubeConfig: cfg.KubeConfig, + APIServerURL: cfg.APIServerURL, + RequestTimeout: func() time.Duration { + if cfg.UpdateEvents { + return 0 + } + return cfg.RequestTimeout + }(), + } + }) + return cfg.clientGen } // ClientGenerator provides clients for various Kubernetes APIs and external services. @@ -338,6 +336,19 @@ func ByNames(ctx context.Context, cfg *Config, p ClientGenerator) ([]Source, err return sources, nil } +func ByNamesV0(ctx context.Context, p ClientGenerator, names []string, cfg *Config) ([]Source, error) { + sources := []Source{} + for _, name := range cfg.sources { + source, err := BuildWithConfig(ctx, name, p, cfg) + if err != nil { + return nil, err + } + sources = append(sources, source) + } + + return sources, nil +} + // BuildWithConfig creates a Source implementation using the factory pattern. // This function serves as the central registry for all available source types. // From 2de9f52517ccbec90628e68fb4499d45e0b59e03 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 13:27:15 +0000 Subject: [PATCH 4/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- source/store_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/store_test.go b/source/store_test.go index a9a5597ab2..cd98c36f6b 100644 --- a/source/store_test.go +++ b/source/store_test.go @@ -77,7 +77,7 @@ func (m *MockClientGenerator) IstioClient() (istioclient.Interface, error) { return nil, args.Error(1) } -func (m *MockClientGenerator) CloudFoundryClient(cfAPIEndpoint string, cfUsername string, cfPassword string) (*cfclient.Client, error) { +func (m *MockClientGenerator) CloudFoundryClient(_ string, _ string, _ string) (*cfclient.Client, error) { args := m.Called() if args.Error(1) == nil { m.cloudFoundryClient = args.Get(0).(*cfclient.Client) From d52e74d205aaff85fb3f2675fb6ae51404d8c807 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 13:31:06 +0000 Subject: [PATCH 5/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- controller/execute.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/execute.go b/controller/execute.go index 867ba971f6..eeb9673c29 100644 --- a/controller/execute.go +++ b/controller/execute.go @@ -169,8 +169,8 @@ func buildProvider( zoneTypeFilter := provider.NewZoneTypeFilter(cfg.AWSZoneType) zoneTagFilter := provider.NewZoneTagFilter(cfg.AWSZoneTagFilter) - // TODO: refactor to move this to provider package, cover with tests // TODO: Controller focuses on orchestration, not provider construction + // TODO: refactor to move this to provider package, cover with tests // TODO: example provider.SelectProvider(cfg, ...) switch cfg.Provider { case "akamai": From 11b682d34f40712bfbeb168c7ccc8186b67f919b Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 13:33:00 +0000 Subject: [PATCH 6/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- controller/execute_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controller/execute_test.go b/controller/execute_test.go index 646a2bc6d4..d89bb61228 100644 --- a/controller/execute_test.go +++ b/controller/execute_test.go @@ -382,7 +382,6 @@ func TestExecuteConfigValidationErrorExitsNonZero(t *testing.T) { // buildSource failure triggers log.Fatal. func TestExecuteBuildSourceErrorExitsNonZero(t *testing.T) { - // Use a valid source name (ingress) and an invalid kubeconfig path to // force client creation failure inside buildSource. code, _, err := runExecuteSubprocess(t, []string{ From b281839cf614503af756e0333ae0769c3d56ac10 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Fri, 2 Jan 2026 15:12:48 +0000 Subject: [PATCH 7/7] refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk --- source/store.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/source/store.go b/source/store.go index dcc82cd296..e5ad7913fe 100644 --- a/source/store.go +++ b/source/store.go @@ -336,19 +336,6 @@ func ByNames(ctx context.Context, cfg *Config, p ClientGenerator) ([]Source, err return sources, nil } -func ByNamesV0(ctx context.Context, p ClientGenerator, names []string, cfg *Config) ([]Source, error) { - sources := []Source{} - for _, name := range cfg.sources { - source, err := BuildWithConfig(ctx, name, p, cfg) - if err != nil { - return nil, err - } - sources = append(sources, source) - } - - return sources, nil -} - // BuildWithConfig creates a Source implementation using the factory pattern. // This function serves as the central registry for all available source types. //