Skip to content

Commit 56fb17a

Browse files
ivankatliarchukivankatliarchuk
authored
refactor(controller): controller no longer responsible for SingletonClientGenerator creation (#6077)
* refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> * refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> * refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> * refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> * refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> * refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> * refactore(controller): abstract things that should not be in controller Signed-off-by: ivan katliarchuk <[email protected]> --------- Signed-off-by: ivan katliarchuk <[email protected]>
1 parent c0d9262 commit 56fb17a

File tree

4 files changed

+129
-32
lines changed

4 files changed

+129
-32
lines changed

‎controller/execute.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,8 @@ func Execute() {
108108
go serveMetrics(cfg.MetricsAddress)
109109
go handleSigterm(cancel)
110110

111-
endpointsSource, err := buildSource(ctx, cfg)
111+
sCfg := source.NewSourceConfig(cfg)
112+
endpointsSource, err := buildSource(ctx, sCfg)
112113
if err != nil {
113114
log.Fatal(err) // nolint: gocritic // exitAfterDefer
114115
}
@@ -168,6 +169,9 @@ func buildProvider(
168169
zoneTypeFilter := provider.NewZoneTypeFilter(cfg.AWSZoneType)
169170
zoneTagFilter := provider.NewZoneTagFilter(cfg.AWSZoneTagFilter)
170171

172+
// TODO: Controller focuses on orchestration, not provider construction
173+
// TODO: refactor to move this to provider package, cover with tests
174+
// TODO: example provider.SelectProvider(cfg, ...)
171175
switch cfg.Provider {
172176
case "akamai":
173177
p, err = akamai.NewAkamaiProvider(
@@ -413,18 +417,8 @@ func configureLogger(cfg *externaldns.Config) {
413417
// buildSource creates and configures the source(s) for endpoint discovery based on the provided configuration.
414418
// It initializes the source configuration, generates the required sources, and combines them into a single,
415419
// deduplicated source. Returns the combined source or an error if source creation fails.
416-
func buildSource(ctx context.Context, cfg *externaldns.Config) (source.Source, error) {
417-
sourceCfg := source.NewSourceConfig(cfg)
418-
sources, err := source.ByNames(ctx, &source.SingletonClientGenerator{
419-
KubeConfig: cfg.KubeConfig,
420-
APIServerURL: cfg.APIServerURL,
421-
RequestTimeout: func() time.Duration {
422-
if cfg.UpdateEvents {
423-
return 0
424-
}
425-
return cfg.RequestTimeout
426-
}(),
427-
}, cfg.Sources, sourceCfg)
420+
func buildSource(ctx context.Context, cfg *source.Config) (source.Source, error) {
421+
sources, err := source.ByNames(ctx, cfg, cfg.ClientGenerator())
428422
if err != nil {
429423
return nil, err
430424
}

‎controller/execute_test.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
"github.com/stretchr/testify/require"
3434
"sigs.k8s.io/external-dns/endpoint"
3535
"sigs.k8s.io/external-dns/pkg/apis/externaldns"
36+
"sigs.k8s.io/external-dns/source"
3637
)
3738

3839
// Logger
@@ -267,7 +268,7 @@ func TestBuildSourceWithWrappers(t *testing.T) {
267268

268269
for _, tt := range tests {
269270
t.Run(tt.name, func(t *testing.T) {
270-
_, err := buildSource(t.Context(), tt.cfg)
271+
_, err := buildSource(t.Context(), source.NewSourceConfig(tt.cfg))
271272
require.NoError(t, err)
272273
})
273274
}
@@ -297,14 +298,21 @@ func TestHelperProcess(t *testing.T) {
297298
// runExecuteSubprocess runs Execute in a separate process and returns exit code and output.
298299
func runExecuteSubprocess(t *testing.T, args []string) (int, string, error) {
299300
t.Helper()
301+
// make sure the subprocess does not run forever
302+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
303+
defer cancel()
304+
300305
cmdArgs := append([]string{"-test.run=TestHelperProcess", "--"}, args...)
301-
cmd := exec.Command(os.Args[0], cmdArgs...)
306+
cmd := exec.CommandContext(ctx, os.Args[0], cmdArgs...)
302307
cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1")
303308
var buf bytes.Buffer
304309
cmd.Stdout = &buf
305310
cmd.Stderr = &buf
306311
err := cmd.Run()
307312
output := buf.String()
313+
if errors.Is(ctx.Err(), context.DeadlineExceeded) {
314+
return -1, output, ctx.Err()
315+
}
308316
if err == nil {
309317
return 0, output, nil
310318
}
@@ -440,7 +448,7 @@ func TestControllerRunCancelContextStopsLoop(t *testing.T) {
440448
}
441449
ctx, cancel := context.WithCancel(context.Background())
442450
defer cancel()
443-
src, err := buildSource(ctx, cfg)
451+
src, err := buildSource(ctx, source.NewSourceConfig(cfg))
444452
require.NoError(t, err)
445453
domainFilter := endpoint.NewDomainFilterWithOptions(
446454
endpoint.WithDomainFilter(cfg.DomainFilter),

‎source/store.go‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,16 @@ type Config struct {
9797
TraefikDisableNew bool
9898
ExcludeUnschedulable bool
9999
ExposeInternalIPv6 bool
100+
ExcludeTargetNets []string
101+
TargetNetFilter []string
102+
NAT64Networks []string
103+
MinTTL time.Duration
104+
105+
sources []string
106+
107+
// clientGen is lazily initialized on first access for efficiency
108+
clientGen *SingletonClientGenerator
109+
clientGenOnce sync.Once
100110
}
101111

102112
func NewSourceConfig(cfg *externaldns.Config) *Config {
@@ -140,9 +150,36 @@ func NewSourceConfig(cfg *externaldns.Config) *Config {
140150
TraefikDisableNew: cfg.TraefikDisableNew,
141151
ExcludeUnschedulable: cfg.ExcludeUnschedulable,
142152
ExposeInternalIPv6: cfg.ExposeInternalIPV6,
153+
ExcludeTargetNets: cfg.ExcludeTargetNets,
154+
TargetNetFilter: cfg.TargetNetFilter,
155+
NAT64Networks: cfg.NAT64Networks,
156+
MinTTL: cfg.MinTTL,
157+
sources: cfg.Sources,
143158
}
144159
}
145160

161+
// ClientGenerator returns a SingletonClientGenerator from this Config's connection settings.
162+
// The generator is created once and cached for subsequent calls.
163+
// This ensures consistent Kubernetes client creation across all sources using this configuration.
164+
//
165+
// The timeout behavior is special-cased: when UpdateEvents is true, the timeout is set to 0
166+
// (no timeout) to allow long-running watch operations for event-driven source updates.
167+
func (cfg *Config) ClientGenerator() *SingletonClientGenerator {
168+
cfg.clientGenOnce.Do(func() {
169+
cfg.clientGen = &SingletonClientGenerator{
170+
KubeConfig: cfg.KubeConfig,
171+
APIServerURL: cfg.APIServerURL,
172+
RequestTimeout: func() time.Duration {
173+
if cfg.UpdateEvents {
174+
return 0
175+
}
176+
return cfg.RequestTimeout
177+
}(),
178+
}
179+
})
180+
return cfg.clientGen
181+
}
182+
146183
// ClientGenerator provides clients for various Kubernetes APIs and external services.
147184
// This interface abstracts client creation and enables dependency injection for testing.
148185
// It uses the singleton pattern to ensure only one instance of each client is created
@@ -251,9 +288,9 @@ func (p *SingletonClientGenerator) OpenShiftClient() (openshift.Interface, error
251288
}
252289

253290
// ByNames returns multiple Sources given multiple names.
254-
func ByNames(ctx context.Context, p ClientGenerator, names []string, cfg *Config) ([]Source, error) {
255-
sources := []Source{}
256-
for _, name := range names {
291+
func ByNames(ctx context.Context, cfg *Config, p ClientGenerator) ([]Source, error) {
292+
sources := make([]Source, 0, len(cfg.sources))
293+
for _, name := range cfg.sources {
257294
source, err := BuildWithConfig(ctx, name, p, cfg)
258295
if err != nil {
259296
return nil, err

‎source/store_test.go‎

Lines changed: 71 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ import (
2020
"context"
2121
"errors"
2222
"testing"
23+
"time"
2324

2425
openshift "github.com/openshift/client-go/route/clientset/versioned"
26+
"github.com/stretchr/testify/assert"
2527
"github.com/stretchr/testify/mock"
2628
"github.com/stretchr/testify/suite"
2729
istioclient "istio.io/client-go/pkg/clientset/versioned"
@@ -158,29 +160,35 @@ func (suite *ByNamesTestSuite) TestAllInitialized() {
158160
}: "IngressRouteUDPList",
159161
}), nil)
160162

161-
sources, err := ByNames(context.TODO(), mockClientGenerator, []string{
163+
ss := []string{
162164
types.Service, types.Ingress, types.IstioGateway, types.ContourHTTPProxy,
163165
types.KongTCPIngress, types.F5VirtualServer, types.F5TransportServer, types.TraefikProxy, types.Fake,
164-
}, &Config{})
166+
}
167+
sources, err := ByNames(context.TODO(), &Config{
168+
sources: ss,
169+
}, mockClientGenerator)
165170
suite.NoError(err, "should not generate errors")
166171
suite.Len(sources, 9, "should generate all nine sources")
167172
}
168173

169174
func (suite *ByNamesTestSuite) TestOnlyFake() {
170175
mockClientGenerator := new(MockClientGenerator)
171-
mockClientGenerator.On("KubeClient").Return(fakeKube.NewSimpleClientset(), nil)
176+
mockClientGenerator.On("KubeClient").Return(fakeKube.NewClientset(), nil)
172177

173-
sources, err := ByNames(context.TODO(), mockClientGenerator, []string{types.Fake}, &Config{})
178+
sources, err := ByNames(context.TODO(), &Config{
179+
sources: []string{types.Fake},
180+
}, mockClientGenerator)
174181
suite.NoError(err, "should not generate errors")
175182
suite.Len(sources, 1, "should generate fake source")
176183
suite.Nil(mockClientGenerator.kubeClient, "client should not be created")
177184
}
178185

179186
func (suite *ByNamesTestSuite) TestSourceNotFound() {
180187
mockClientGenerator := new(MockClientGenerator)
181-
mockClientGenerator.On("KubeClient").Return(fakeKube.NewSimpleClientset(), nil)
182-
183-
sources, err := ByNames(context.TODO(), mockClientGenerator, []string{"foo"}, &Config{})
188+
mockClientGenerator.On("KubeClient").Return(fakeKube.NewClientset(), nil)
189+
sources, err := ByNames(context.TODO(), &Config{
190+
sources: []string{"foo"},
191+
}, mockClientGenerator)
184192
suite.Equal(err, ErrSourceNotFound, "should return source not found")
185193
suite.Empty(sources, "should not returns any source")
186194
}
@@ -189,14 +197,16 @@ func (suite *ByNamesTestSuite) TestKubeClientFails() {
189197
mockClientGenerator := new(MockClientGenerator)
190198
mockClientGenerator.On("KubeClient").Return(nil, errors.New("foo"))
191199

192-
sourcesDependentOnKubeClient := []string{
200+
sourceUnderTest := []string{
193201
types.Node, types.Service, types.Ingress, types.Pod, types.IstioGateway, types.IstioVirtualService,
194202
types.AmbassadorHost, types.GlooProxy, types.TraefikProxy, types.CRD, types.KongTCPIngress,
195203
types.F5VirtualServer, types.F5TransportServer,
196204
}
197205

198-
for _, source := range sourcesDependentOnKubeClient {
199-
_, err := ByNames(context.TODO(), mockClientGenerator, []string{source}, &Config{})
206+
for _, source := range sourceUnderTest {
207+
_, err := ByNames(context.TODO(), &Config{
208+
sources: []string{source},
209+
}, mockClientGenerator)
200210
suite.Error(err, source+" should return an error if kubernetes client cannot be created")
201211
}
202212
}
@@ -210,14 +220,16 @@ func (suite *ByNamesTestSuite) TestIstioClientFails() {
210220
sourcesDependentOnIstioClient := []string{types.IstioGateway, types.IstioVirtualService}
211221

212222
for _, source := range sourcesDependentOnIstioClient {
213-
_, err := ByNames(context.TODO(), mockClientGenerator, []string{source}, &Config{})
223+
_, err := ByNames(context.TODO(), &Config{
224+
sources: []string{source},
225+
}, mockClientGenerator)
214226
suite.Error(err, source+" should return an error if istio client cannot be created")
215227
}
216228
}
217229

218230
func (suite *ByNamesTestSuite) TestDynamicKubernetesClientFails() {
219231
mockClientGenerator := new(MockClientGenerator)
220-
mockClientGenerator.On("KubeClient").Return(fakeKube.NewSimpleClientset(), nil)
232+
mockClientGenerator.On("KubeClient").Return(fakeKube.NewClientset(), nil)
221233
mockClientGenerator.On("IstioClient").Return(istiofake.NewSimpleClientset(), nil)
222234
mockClientGenerator.On("DynamicKubernetesClient").Return(nil, errors.New("foo"))
223235

@@ -227,7 +239,9 @@ func (suite *ByNamesTestSuite) TestDynamicKubernetesClientFails() {
227239
}
228240

229241
for _, source := range sourcesDependentOnDynamicKubernetesClient {
230-
_, err := ByNames(context.TODO(), mockClientGenerator, []string{source}, &Config{})
242+
_, err := ByNames(context.TODO(), &Config{
243+
sources: []string{source},
244+
}, mockClientGenerator)
231245
suite.Error(err, source+" should return an error if dynamic kubernetes client cannot be created")
232246
}
233247
}
@@ -266,3 +280,47 @@ func TestBuildWithConfig_InvalidSource(t *testing.T) {
266280
t.Errorf("expected ErrSourceNotFound, got: %v", err)
267281
}
268282
}
283+
284+
func TestConfig_ClientGenerator(t *testing.T) {
285+
cfg := &Config{
286+
KubeConfig: "/path/to/kubeconfig",
287+
APIServerURL: "https://api.example.com",
288+
RequestTimeout: 30 * time.Second,
289+
UpdateEvents: false,
290+
}
291+
292+
gen := cfg.ClientGenerator()
293+
294+
assert.Equal(t, "/path/to/kubeconfig", gen.KubeConfig)
295+
assert.Equal(t, "https://api.example.com", gen.APIServerURL)
296+
assert.Equal(t, 30*time.Second, gen.RequestTimeout)
297+
}
298+
299+
func TestConfig_ClientGenerator_UpdateEvents(t *testing.T) {
300+
cfg := &Config{
301+
KubeConfig: "/path/to/kubeconfig",
302+
APIServerURL: "https://api.example.com",
303+
RequestTimeout: 30 * time.Second,
304+
UpdateEvents: true, // Special case
305+
}
306+
307+
gen := cfg.ClientGenerator()
308+
309+
assert.Equal(t, time.Duration(0), gen.RequestTimeout, "UpdateEvents should set timeout to 0")
310+
}
311+
312+
func TestConfig_ClientGenerator_Caching(t *testing.T) {
313+
cfg := &Config{
314+
KubeConfig: "/path/to/kubeconfig",
315+
APIServerURL: "https://api.example.com",
316+
RequestTimeout: 30 * time.Second,
317+
UpdateEvents: false,
318+
}
319+
320+
// Call ClientGenerator twice
321+
gen1 := cfg.ClientGenerator()
322+
gen2 := cfg.ClientGenerator()
323+
324+
// Should return the same instance (cached)
325+
assert.Same(t, gen1, gen2, "ClientGenerator should return the same cached instance")
326+
}

0 commit comments

Comments
 (0)