Skip to content

refactor(controller): reduce cyclomatic complexity of buildProvider #6133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AndrewCharlesHay wants to merge 10 commits into from
Closed

refactor(controller): reduce cyclomatic complexity of buildProvider #6133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3904c55
refactor(controller): reduce cyclomatic complexity of buildProvider
AndrewCharlesHay Jan 20, 2026
fcaeb4a
test(controller): add coverage for provider factory map
AndrewCharlesHay Jan 20, 2026
31baed0
chore: adjust linter cyclomatic complexity to 32
AndrewCharlesHay Jan 20, 2026
d610a5c
fix: use t.context
AndrewCharlesHay Jan 20, 2026
44190db
fix: revert t.Context, restore TODOs, and pine E2E to go.mod version
AndrewCharlesHay Jan 20, 2026
79cb98e
fix: use t.Context() and pin E2E workflow to go.mod version
AndrewCharlesHay Jan 20, 2026
37ad968
refactor(controller): reduce cyclomatic complexity of buildProvider
AndrewCharlesHay Jan 21, 2026
f9cb37c
fix(controller): add missing license header to provider_factories.go
AndrewCharlesHay Jan 21, 2026
68e27e3
fix(ci): harden kubectl download in e2e-test.sh to prevent flakiness
AndrewCharlesHay Jan 21, 2026
594170f
Revert "fix(ci): harden kubectl download in e2e-test.sh to prevent fl…
AndrewCharlesHay Jan 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/end-to-end-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v6
- name: Set up Go
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what this go is fo?

Copy link
Copy Markdown
Contributor Author

@AndrewCharlesHay AndrewCharlesHay Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Context() was added in Go 1.24. The e2e tests were failing because the Go Version with Ubuntu latest was too old

Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm too tired ;-). This e2e script does not require any go installed https://github.com/kubernetes-sigs/external-dns/blob/master/scripts/e2e-test.sh. Could you point where go is required for e2e tests?

Copy link
Copy Markdown
Contributor Author

@AndrewCharlesHay AndrewCharlesHay Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ko needs a Go toolchain installed and avaible

Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ko is a binary that is donwloaded with curl, it does not require go. Unless installed with go install github.com/google/ko@latest command. Could you share where it was failing?

uses: actions/setup-go@v5
with:
go-version-file: go.mod
- name: e2e
run: |
./scripts/e2e-test.sh
2 changes: 1 addition & 1 deletion .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ linters:
- name: confusing-naming
disabled: true
cyclop: # Lower cyclomatic complexity threshold after the max complexity is lowered
max-complexity: 37 # Controller/execute.go:147:1: calculated cyclomatic complexity for function buildProvider is 37
max-complexity: 32 # provider/rfc2136/rfc2136.go:350:1: calculated cyclomatic complexity for function ApplyChanges is 32. See https://github.com/kubernetes-sigs/external-dns/issues/5419
goconst:
min-occurrences: 3
# Ignore well-known DNS record types, boolean strings, and common values
Expand Down
228 changes: 11 additions & 217 deletions controller/execute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ import (
"syscall"
"time"

"github.com/aws/aws-sdk-go-v2/service/route53"
sd "github.com/aws/aws-sdk-go-v2/service/servicediscovery"
"github.com/go-logr/logr"
"github.com/prometheus/client_golang/prometheus/promhttp"
log "github.com/sirupsen/logrus"
Expand All @@ -39,32 +37,6 @@ import (
"sigs.k8s.io/external-dns/pkg/metrics"
"sigs.k8s.io/external-dns/plan"
"sigs.k8s.io/external-dns/provider"
"sigs.k8s.io/external-dns/provider/akamai"
"sigs.k8s.io/external-dns/provider/alibabacloud"
"sigs.k8s.io/external-dns/provider/aws"
"sigs.k8s.io/external-dns/provider/awssd"
"sigs.k8s.io/external-dns/provider/azure"
"sigs.k8s.io/external-dns/provider/civo"
"sigs.k8s.io/external-dns/provider/cloudflare"
"sigs.k8s.io/external-dns/provider/coredns"
"sigs.k8s.io/external-dns/provider/digitalocean"
"sigs.k8s.io/external-dns/provider/dnsimple"
"sigs.k8s.io/external-dns/provider/exoscale"
"sigs.k8s.io/external-dns/provider/gandi"
"sigs.k8s.io/external-dns/provider/godaddy"
"sigs.k8s.io/external-dns/provider/google"
"sigs.k8s.io/external-dns/provider/inmemory"
"sigs.k8s.io/external-dns/provider/linode"
"sigs.k8s.io/external-dns/provider/ns1"
"sigs.k8s.io/external-dns/provider/oci"
"sigs.k8s.io/external-dns/provider/ovh"
"sigs.k8s.io/external-dns/provider/pdns"
"sigs.k8s.io/external-dns/provider/pihole"
"sigs.k8s.io/external-dns/provider/plural"
"sigs.k8s.io/external-dns/provider/rfc2136"
"sigs.k8s.io/external-dns/provider/scaleway"
"sigs.k8s.io/external-dns/provider/transip"
"sigs.k8s.io/external-dns/provider/webhook"
webhookapi "sigs.k8s.io/external-dns/provider/webhook/api"
"sigs.k8s.io/external-dns/registry"
"sigs.k8s.io/external-dns/source"
Expand Down Expand Up @@ -109,6 +81,7 @@ func Execute() {
go handleSigterm(cancel)

sCfg := source.NewSourceConfig(cfg)
// TODO: Move source construction to the source package (blocked by cyclic dependency with wrappers)
endpointsSource, err := buildSource(ctx, sCfg)
if err != nil {
log.Fatal(err) // nolint: gocritic // exitAfterDefer
Expand All @@ -121,6 +94,7 @@ func Execute() {
endpoint.WithRegexDomainExclude(cfg.RegexDomainExclude),
)

// TODO: Move provider construction to the provider package
prvdr, err := buildProvider(ctx, cfg, domainFilter)
if err != nil {
log.Fatal(err)
Expand Down Expand Up @@ -161,202 +135,22 @@ func buildProvider(
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)

// TODO: Controller focuses on orchestration, not provider construction
// TODO: refactor to move this to provider package, cover with tests
// TODO: example provider.SelectProvider(cfg, ...)
Comment thread
AndrewCharlesHay marked this conversation as resolved.
switch cfg.Provider {
case "akamai":
p, err = akamai.NewAkamaiProvider(
akamai.AkamaiConfig{
DomainFilter: domainFilter,
ZoneIDFilter: zoneIDFilter,
ServiceConsumerDomain: cfg.AkamaiServiceConsumerDomain,
ClientToken: cfg.AkamaiClientToken,
ClientSecret: cfg.AkamaiClientSecret,
AccessToken: cfg.AkamaiAccessToken,
EdgercPath: cfg.AkamaiEdgercPath,
EdgercSection: cfg.AkamaiEdgercSection,
DryRun: cfg.DryRun,
}, nil)
case "alibabacloud":
p, err = alibabacloud.NewAlibabaCloudProvider(cfg.AlibabaCloudConfigFile, domainFilter, zoneIDFilter, cfg.AlibabaCloudZoneType, cfg.DryRun)
case "aws":
configs := aws.CreateV2Configs(cfg)
clients := make(map[string]aws.Route53API, len(configs))
for profile, config := range configs {
clients[profile] = route53.NewFromConfig(config)
}

p, err = aws.NewAWSProvider(
aws.AWSConfig{
DomainFilter: domainFilter,
ZoneIDFilter: zoneIDFilter,
ZoneTypeFilter: zoneTypeFilter,
ZoneTagFilter: zoneTagFilter,
ZoneMatchParent: cfg.AWSZoneMatchParent,
BatchChangeSize: cfg.AWSBatchChangeSize,
BatchChangeSizeBytes: cfg.AWSBatchChangeSizeBytes,
BatchChangeSizeValues: cfg.AWSBatchChangeSizeValues,
BatchChangeInterval: cfg.AWSBatchChangeInterval,
EvaluateTargetHealth: cfg.AWSEvaluateTargetHealth,
PreferCNAME: cfg.AWSPreferCNAME,
DryRun: cfg.DryRun,
ZoneCacheDuration: cfg.AWSZoneCacheDuration,
},
clients,
)
case "aws-sd":
// Check that only compatible Registry is used with AWS-SD
if cfg.Registry != "noop" && cfg.Registry != "aws-sd" {
log.Infof("Registry \"%s\" cannot be used with AWS Cloud Map. Switching to \"aws-sd\".", cfg.Registry)
cfg.Registry = "aws-sd"
}
p, err = awssd.NewAWSSDProvider(domainFilter, cfg.AWSZoneType, cfg.DryRun, cfg.AWSSDServiceCleanup, cfg.TXTOwnerID, cfg.AWSSDCreateTag, sd.NewFromConfig(aws.CreateDefaultV2Config(cfg)))
case "azure-dns", "azure":
p, err = azure.NewAzureProvider(cfg.AzureConfigFile, domainFilter, zoneNameFilter, zoneIDFilter, cfg.AzureSubscriptionID, cfg.AzureResourceGroup, cfg.AzureUserAssignedIdentityClientID, cfg.AzureActiveDirectoryAuthorityHost, cfg.AzureZonesCacheDuration, cfg.AzureMaxRetriesCount, cfg.DryRun)
case "azure-private-dns":
p, err = azure.NewAzurePrivateDNSProvider(cfg.AzureConfigFile, domainFilter, zoneNameFilter, zoneIDFilter, cfg.AzureSubscriptionID, cfg.AzureResourceGroup, cfg.AzureUserAssignedIdentityClientID, cfg.AzureActiveDirectoryAuthorityHost, cfg.AzureZonesCacheDuration, cfg.AzureMaxRetriesCount, cfg.DryRun)
case "civo":
p, err = civo.NewCivoProvider(domainFilter, cfg.DryRun)
case "cloudflare":
p, err = cloudflare.NewCloudFlareProvider(
domainFilter,
zoneIDFilter,
cfg.CloudflareProxied,
cfg.DryRun,
cloudflare.RegionalServicesConfig{
Enabled: cfg.CloudflareRegionalServices,
RegionKey: cfg.CloudflareRegionKey,
},
cloudflare.CustomHostnamesConfig{
Enabled: cfg.CloudflareCustomHostnames,
MinTLSVersion: cfg.CloudflareCustomHostnamesMinTLSVersion,
CertificateAuthority: cfg.CloudflareCustomHostnamesCertificateAuthority,
},
cloudflare.DNSRecordsConfig{
PerPage: cfg.CloudflareDNSRecordsPerPage,
Comment: cfg.CloudflareDNSRecordsComment,
})
case "google":
p, err = google.NewGoogleProvider(ctx, cfg.GoogleProject, domainFilter, zoneIDFilter, cfg.GoogleBatchChangeSize, cfg.GoogleBatchChangeInterval, cfg.GoogleZoneVisibility, cfg.DryRun)
case "digitalocean":
p, err = digitalocean.NewDigitalOceanProvider(ctx, domainFilter, cfg.DryRun, cfg.DigitalOceanAPIPageSize)
case "ovh":
p, err = ovh.NewOVHProvider(ctx, domainFilter, cfg.OVHEndpoint, cfg.OVHApiRateLimit, cfg.OVHEnableCNAMERelative, cfg.DryRun)
case "linode":
p, err = linode.NewLinodeProvider(domainFilter, cfg.DryRun)
case "dnsimple":
p, err = dnsimple.NewDnsimpleProvider(domainFilter, zoneIDFilter, cfg.DryRun)
case "coredns", "skydns":
p, err = coredns.NewCoreDNSProvider(domainFilter, cfg.CoreDNSPrefix, cfg.TXTOwnerID, cfg.CoreDNSStrictlyOwned, cfg.DryRun)
case "exoscale":
p, err = exoscale.NewExoscaleProvider(
cfg.ExoscaleAPIEnvironment,
cfg.ExoscaleAPIZone,
cfg.ExoscaleAPIKey,
cfg.ExoscaleAPISecret,
cfg.DryRun,
exoscale.ExoscaleWithDomain(domainFilter),
exoscale.ExoscaleWithLogging(),
)
case "inmemory":
p, err = inmemory.NewInMemoryProvider(inmemory.InMemoryInitZones(cfg.InMemoryZones), inmemory.InMemoryWithDomain(domainFilter), inmemory.InMemoryWithLogging()), nil
case "pdns":
p, err = pdns.NewPDNSProvider(
ctx,
pdns.PDNSConfig{
DomainFilter: domainFilter,
DryRun: cfg.DryRun,
Server: cfg.PDNSServer,
ServerID: cfg.PDNSServerID,
APIKey: cfg.PDNSAPIKey,
TLSConfig: pdns.TLSConfig{
SkipTLSVerify: cfg.PDNSSkipTLSVerify,
CAFilePath: cfg.TLSCA,
ClientCertFilePath: cfg.TLSClientCert,
ClientCertKeyFilePath: cfg.TLSClientCertKey,
},
},
)
case "oci":
var config *oci.OCIConfig
// if the instance-principals flag was set, and a compartment OCID was provided, then ignore the
// OCI config file, and provide a config that uses instance principal authentication.
if cfg.OCIAuthInstancePrincipal {
if len(cfg.OCICompartmentOCID) == 0 {
err = fmt.Errorf("instance principal authentication requested, but no compartment OCID provided")
break
}
authConfig := oci.OCIAuthConfig{UseInstancePrincipal: true}
config = &oci.OCIConfig{Auth: authConfig, CompartmentID: cfg.OCICompartmentOCID}
} else {
if config, err = oci.LoadOCIConfig(cfg.OCIConfigFile); err != nil {
break
}
}
config.ZoneCacheDuration = cfg.OCIZoneCacheDuration
p, err = oci.NewOCIProvider(*config, domainFilter, zoneIDFilter, cfg.OCIZoneScope, cfg.DryRun)
case "rfc2136":
tlsConfig := rfc2136.TLSConfig{
UseTLS: cfg.RFC2136UseTLS,
SkipTLSVerify: cfg.RFC2136SkipTLSVerify,
CAFilePath: cfg.TLSCA,
ClientCertFilePath: cfg.TLSClientCert,
ClientCertKeyFilePath: cfg.TLSClientCertKey,
}
p, err = rfc2136.NewRfc2136Provider(cfg.RFC2136Host, cfg.RFC2136Port, cfg.RFC2136Zone, cfg.RFC2136Insecure, cfg.RFC2136TSIGKeyName, cfg.RFC2136TSIGSecret, cfg.RFC2136TSIGSecretAlg, cfg.RFC2136TAXFR, domainFilter, cfg.DryRun, cfg.RFC2136MinTTL, cfg.RFC2136CreatePTR, cfg.RFC2136GSSTSIG, cfg.RFC2136KerberosUsername, cfg.RFC2136KerberosPassword, cfg.RFC2136KerberosRealm, cfg.RFC2136BatchChangeSize, tlsConfig, cfg.RFC2136LoadBalancingStrategy, nil)
case "ns1":
p, err = ns1.NewNS1Provider(
ns1.NS1Config{
DomainFilter: domainFilter,
ZoneIDFilter: zoneIDFilter,
NS1Endpoint: cfg.NS1Endpoint,
NS1IgnoreSSL: cfg.NS1IgnoreSSL,
DryRun: cfg.DryRun,
MinTTLSeconds: cfg.NS1MinTTLSeconds,
},
)
case "transip":
p, err = transip.NewTransIPProvider(cfg.TransIPAccountName, cfg.TransIPPrivateKeyFile, domainFilter, cfg.DryRun)
case "scaleway":
p, err = scaleway.NewScalewayProvider(ctx, domainFilter, cfg.DryRun)
case "godaddy":
p, err = godaddy.NewGoDaddyProvider(ctx, domainFilter, cfg.GoDaddyTTL, cfg.GoDaddyAPIKey, cfg.GoDaddySecretKey, cfg.GoDaddyOTE, cfg.DryRun)
case "gandi":
p, err = gandi.NewGandiProvider(ctx, domainFilter, cfg.DryRun)
case "pihole":
p, err = pihole.NewPiholeProvider(
pihole.PiholeConfig{
Server: cfg.PiholeServer,
Password: cfg.PiholePassword,
TLSInsecureSkipVerify: cfg.PiholeTLSInsecureSkipVerify,
DomainFilter: domainFilter,
DryRun: cfg.DryRun,
APIVersion: cfg.PiholeApiVersion,
},
)
case "plural":
p, err = plural.NewPluralProvider(cfg.PluralCluster, cfg.PluralProvider)
case "webhook":
p, err = webhook.NewWebhookProvider(cfg.WebhookProviderURL)
default:
err = fmt.Errorf("unknown dns provider: %s", cfg.Provider)
factory, ok := providerFactories[cfg.Provider]
if !ok {
return nil, fmt.Errorf("unknown dns provider: %s", cfg.Provider)
}
p, err := factory(ctx, cfg, domainFilter)
if err != nil {
return nil, err
}

if p != nil && cfg.ProviderCacheTime > 0 {
p = provider.NewCachedProvider(
p,
cfg.ProviderCacheTime,
)
}
return p, err
return p, nil
}

func buildController(
Expand Down
28 changes: 26 additions & 2 deletions controller/execute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,30 @@ func TestBuildProvider(t *testing.T) {
}
}

func TestBuildProvider_Coverage(t *testing.T) {
// this test ensures that all providers are registered in the factory map
// and that the buildProvider function does not return an "unknown provider" error
knownProviders := []string{
"akamai", "alibabacloud", "aws", "aws-sd", "azure", "azure-dns", "azure-private-dns",
"civo", "cloudflare", "coredns", "digitalocean", "dnsimple", "exoscale", "gandi",
"godaddy", "google", "inmemory", "linode", "ns1", "oci", "ovh", "pdns", "pihole",
"plural", "rfc2136", "scaleway", "skydns", "transip", "webhook",
}

for _, providerName := range knownProviders {
t.Run(providerName, func(t *testing.T) {
cfg := &externaldns.Config{
Provider: providerName,
}
// We don't care about the specific error (missing config), just that it's NOT "unknown provider"
_, err := buildProvider(t.Context(), cfg, nil)
if err != nil {
assert.NotContains(t, err.Error(), "unknown dns provider")
}
})
}
}

func TestBuildSourceWithWrappers(t *testing.T) {
svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNotImplemented)
Expand Down Expand Up @@ -299,7 +323,7 @@ func TestHelperProcess(t *testing.T) {
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)
ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second)
defer cancel()

cmdArgs := append([]string{"-test.run=TestHelperProcess", "--"}, args...)
Expand Down Expand Up @@ -446,7 +470,7 @@ func TestControllerRunCancelContextStopsLoop(t *testing.T) {
Registry: "txt",
TXTOwnerID: "test-owner",
}
ctx, cancel := context.WithCancel(context.Background())
ctx, cancel := context.WithCancel(t.Context())
defer cancel()
src, err := buildSource(ctx, source.NewSourceConfig(cfg))
require.NoError(t, err)
Expand Down
Loading
Loading