test(controller): reduce complexity and improve code coverage (#5523)

* chore(codebase): reduce complexity and improve code coverage for controller/execute.go

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>

* apply suggestions from code review

Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

* chore(codebase): reduce complexity and improve code coverage for controller/execute.go

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>

* chore(codebase): reduce complexity and improve code coverage for controller/execute.go

Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

---------

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

.golangci.yml

@@ -39,7 +39,7 @@ linters:
         - name: confusing-naming
           disabled: true
     cyclop: # Lower cyclomatic complexity threshold after the max complexity is lowered
-      max-complexity: 52
+      max-complexity: 51
     testifylint:
       # Enable all checkers (https://github.com/Antonboom/testifylint#checkers).
       # Default: false

controller/execute.go

@@ -100,40 +100,61 @@ func Execute() {
 	go serveMetrics(cfg.MetricsAddress)
 	go handleSigterm(cancel)
 
-	// Create a source.Config from the flags passed by the user.
+	endpointsSource, err := buildSource(ctx, cfg)
-	sourceCfg := source.NewSourceConfig(cfg)
-
-	// Lookup all the selected sources by names and pass them the desired configuration.
-	sources, err := source.ByNames(ctx, &source.SingletonClientGenerator{
-		KubeConfig:   cfg.KubeConfig,
-		APIServerURL: cfg.APIServerURL,
-		// If update events are enabled, disable timeout.
-		RequestTimeout: func() time.Duration {
-			if cfg.UpdateEvents {
-				return 0
-			}
-			return cfg.RequestTimeout
-		}(),
-	}, cfg.Sources, sourceCfg)
 	if err != nil {
 		log.Fatal(err)
 	}
 
-	// Filter targets
-	targetFilter := endpoint.NewTargetNetFilterWithExclusions(cfg.TargetNetFilter, cfg.ExcludeTargetNets)
-
-	// Combine multiple sources into a single, deduplicated source.
-	endpointsSource := source.NewDedupSource(source.NewMultiSource(sources, sourceCfg.DefaultTargets))
-	endpointsSource = source.NewNAT64Source(endpointsSource, cfg.NAT64Networks)
-	endpointsSource = source.NewTargetFilterSource(endpointsSource, targetFilter)
-
 	domainFilter := createDomainFilter(cfg)
+
+	prvdr, err := buildProvider(ctx, cfg, domainFilter)
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	if cfg.WebhookServer {
+		webhookapi.StartHTTPApi(prvdr, nil, cfg.WebhookProviderReadTimeout, cfg.WebhookProviderWriteTimeout, "127.0.0.1:8888")
+		os.Exit(0)
+	}
+
+	ctrl, err := buildController(cfg, endpointsSource, prvdr, domainFilter)
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	if cfg.Once {
+		err := ctrl.RunOnce(ctx)
+		if err != nil {
+			log.Fatal(err)
+		}
+
+		os.Exit(0)
+	}
+
+	if cfg.UpdateEvents {
+		// Add RunOnce as the handler function that will be called when ingress/service sources have changed.
+		// Note that k8s Informers will perform an initial list operation, which results in the handler
+		// function initially being called for every Service/Ingress that exists
+		ctrl.Source.AddEventHandler(ctx, func() { ctrl.ScheduleRunOnce(time.Now()) })
+	}
+
+	ctrl.ScheduleRunOnce(time.Now())
+	ctrl.Run(ctx)
+}
+
+func buildProvider(
+	ctx context.Context,
+	cfg *externaldns.Config,
+	domainFilter endpoint.DomainFilter,
+) (provider.Provider, error) {
+	var p provider.Provider
+	var err error
+
 	zoneNameFilter := endpoint.NewDomainFilter(cfg.ZoneNameFilter)
 	zoneIDFilter := provider.NewZoneIDFilter(cfg.ZoneIDFilter)
 	zoneTypeFilter := provider.NewZoneTypeFilter(cfg.AWSZoneType)
 	zoneTagFilter := provider.NewZoneTagFilter(cfg.AWSZoneTagFilter)
 
-	var p provider.Provider
 	switch cfg.Provider {
 	case "akamai":
 		p, err = akamai.NewAkamaiProvider(
@@ -307,63 +328,36 @@ func Execute() {
 	case "webhook":
 		p, err = webhook.NewWebhookProvider(cfg.WebhookProviderURL)
 	default:
-		log.Fatalf("unknown dns provider: %s", cfg.Provider)
+		err = fmt.Errorf("unknown dns provider: %s", cfg.Provider)
 	}
-	if err != nil {
+	if p != nil && cfg.ProviderCacheTime > 0 {
-		log.Fatal(err)
-	}
-
-	if cfg.WebhookServer {
-		webhookapi.StartHTTPApi(p, nil, cfg.WebhookProviderReadTimeout, cfg.WebhookProviderWriteTimeout, "127.0.0.1:8888")
-		os.Exit(0)
-	}
-
-	if cfg.ProviderCacheTime > 0 {
 		p = provider.NewCachedProvider(
 			p,
 			cfg.ProviderCacheTime,
 		)
 	}
+	return p, err
+}
 
+func buildController(cfg *externaldns.Config, src source.Source, p provider.Provider, filter endpoint.DomainFilter) (*Controller, error) {
+	policy, ok := plan.Policies[cfg.Policy]
+	if !ok {
+		return nil, fmt.Errorf("unknown policy: %s", cfg.Policy)
+	}
 	reg, err := selectRegistry(cfg, p)
 	if err != nil {
-		log.Fatal(err)
+		return nil, err
 	}
-
+	return &Controller{
-	policy, exists := plan.Policies[cfg.Policy]
+		Source:               src,
-	if !exists {
-		log.Fatalf("unknown policy: %s", cfg.Policy)
-	}
-
-	ctrl := Controller{
-		Source:               endpointsSource,
 		Registry:             reg,
 		Policy:               policy,
 		Interval:             cfg.Interval,
-		DomainFilter:         domainFilter,
+		DomainFilter:         filter,
 		ManagedRecordTypes:   cfg.ManagedDNSRecordTypes,
 		ExcludeRecordTypes:   cfg.ExcludeDNSRecordTypes,
 		MinEventSyncInterval: cfg.MinEventSyncInterval,
-	}
+	}, nil
-
-	if cfg.Once {
-		err := ctrl.RunOnce(ctx)
-		if err != nil {
-			log.Fatal(err)
-		}
-
-		os.Exit(0)
-	}
-
-	if cfg.UpdateEvents {
-		// Add RunOnce as the handler function that will be called when ingress/service sources have changed.
-		// Note that k8s Informers will perform an initial list operation, which results in the handler
-		// function initially being called for every Service/Ingress that exists
-		ctrl.Source.AddEventHandler(ctx, func() { ctrl.ScheduleRunOnce(time.Now()) })
-	}
-
-	ctrl.ScheduleRunOnce(time.Now())
-	ctrl.Run(ctx)
 }
 
 // This function configures the logger format and level based on the provided configuration.
@@ -407,6 +401,33 @@ func selectRegistry(cfg *externaldns.Config, p provider.Provider) (registry.Regi
 	return r, err
 }
 
+// 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{
+		KubeConfig:   cfg.KubeConfig,
+		APIServerURL: cfg.APIServerURL,
+		RequestTimeout: func() time.Duration {
+			if cfg.UpdateEvents {
+				return 0
+			}
+			return cfg.RequestTimeout
+		}(),
+	}, cfg.Sources, sourceCfg)
+	if err != nil {
+		return nil, err
+	}
+	// Combine multiple sources into a single, deduplicated source.
+	combinedSource := source.NewDedupSource(source.NewMultiSource(sources, sourceCfg.DefaultTargets))
+	// Filter targets
+	targetFilter := endpoint.NewTargetNetFilterWithExclusions(cfg.TargetNetFilter, cfg.ExcludeTargetNets)
+	combinedSource = source.NewNAT64Source(combinedSource, cfg.NAT64Networks)
+	combinedSource = source.NewTargetFilterSource(combinedSource, targetFilter)
+	return combinedSource, nil
+}
+
 // RegexDomainFilter overrides DomainFilter
 func createDomainFilter(cfg *externaldns.Config) endpoint.DomainFilter {
 	if cfg.RegexDomainFilter != nil && cfg.RegexDomainFilter.String() != "" {
@@ -436,8 +457,8 @@ func serveMetrics(address string) {
 		_, _ = w.Write([]byte("OK"))
 	})
 
-	log.Debugf("serving 'healthz' on 'localhost:%s/healthz'", address)
+	log.Debugf("serving 'healthz' on '%s/healthz'", address)
-	log.Debugf("serving 'metrics' on 'localhost:%s/metrics'", address)
+	log.Debugf("serving 'metrics' on '%s/metrics'", address)
 	log.Debugf("registered '%d' metrics", len(metrics.RegisterMetric.Metrics))
 
 	http.Handle("/metrics", promhttp.Handler())

controller/execute_test.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"os/signal"
 	"reflect"
@@ -327,6 +328,156 @@ func TestConfigureLogger(t *testing.T) {
 	}
 }
 
+func TestBuildProvider(t *testing.T) {
+	tests := []struct {
+		name          string
+		cfg           *externaldns.Config
+		expectedType  string
+		expectedError string
+	}{
+		{
+			name: "aws provider",
+			cfg: &externaldns.Config{
+				Provider: "aws",
+			},
+			expectedType: "*aws.AWSProvider",
+		},
+		{
+			name: "rfc2136 provider",
+			cfg: &externaldns.Config{
+				Provider:             "rfc2136",
+				RFC2136TSIGSecretAlg: "hmac-sha256",
+			},
+			expectedType: "*rfc2136.rfc2136Provider",
+		},
+		{
+			name: "gandi provider",
+			cfg: &externaldns.Config{
+				Provider: "gandi",
+			},
+			expectedError: "no environment variable GANDI_KEY or GANDI_PAT provided",
+		},
+		{
+			name: "inmemory provider",
+			cfg: &externaldns.Config{
+				Provider: "inmemory",
+			},
+			expectedType: "*inmemory.InMemoryProvider",
+		},
+		{
+			name: "inmemory cached provider",
+			cfg: &externaldns.Config{
+				Provider:          "inmemory",
+				ProviderCacheTime: 10 * time.Millisecond,
+			},
+			expectedType: "*provider.CachedProvider",
+		},
+		{
+			name: "coredns provider",
+			cfg: &externaldns.Config{
+				Provider: "coredns",
+			},
+			expectedType: "coredns.coreDNSProvider",
+		},
+		{
+			name: "pihole provider",
+			cfg: &externaldns.Config{
+				Provider:         "pihole",
+				PiholeApiVersion: "6",
+				PiholeServer:     "http://localhost:8080",
+			},
+			expectedType: "*pihole.PiholeProvider",
+		},
+		{
+			name: "dnsimple provider",
+			cfg: &externaldns.Config{
+				Provider: "dnsimple",
+			},
+			expectedError: "no dnsimple oauth token provided",
+		},
+		{
+			name: "unknown provider",
+			cfg: &externaldns.Config{
+				Provider: "unknown",
+			},
+			expectedError: "unknown dns provider: unknown",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			domainFilter := endpoint.NewDomainFilter([]string{"example.com"})
+
+			p, err := buildProvider(t.Context(), tt.cfg, domainFilter)
+
+			if tt.expectedError != "" {
+				assert.Error(t, err)
+				assert.EqualError(t, err, tt.expectedError)
+			} else {
+				assert.NoError(t, err)
+				assert.NotNil(t, p)
+				assert.Equal(t, tt.expectedType, reflect.TypeOf(p).String())
+			}
+		})
+	}
+}
+
+func TestBuildSource(t *testing.T) {
+	svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNotImplemented)
+	}))
+	defer svr.Close()
+
+	tests := []struct {
+		name          string
+		cfg           *externaldns.Config
+		expectedError bool
+	}{
+		{
+			name: "Valid configuration with sources",
+			cfg: &externaldns.Config{
+				APIServerURL:   svr.URL,
+				Sources:        []string{"fake"},
+				RequestTimeout: 6 * time.Millisecond,
+			},
+			expectedError: false,
+		},
+		{
+			name: "Empty sources configuration",
+			cfg: &externaldns.Config{
+				APIServerURL:   svr.URL,
+				Sources:        []string{},
+				RequestTimeout: 6 * time.Millisecond,
+			},
+			expectedError: false,
+		},
+		{
+			name: "Update events enabled",
+			cfg: &externaldns.Config{
+				KubeConfig:   "path-to-kubeconfig-not-exists",
+				APIServerURL: svr.URL,
+				Sources:      []string{"ingress"},
+				UpdateEvents: true,
+			},
+			expectedError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			src, err := buildSource(t.Context(), tt.cfg)
+
+			if tt.expectedError {
+				assert.Error(t, err)
+				assert.Nil(t, src)
+			} else {
+				require.NoError(t, err)
+				assert.NotNil(t, src)
+			}
+		})
+	}
+}
+
 // mocks
 type MockProvider struct{}