chore(codebase): add nilnil return check (#5415)

* chore(code-quality): added nilnil return check

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

* chore(code-quality): added nilnil return check

---------

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

.golangci.yml

@@ -19,6 +19,7 @@ linters:
     - predeclared # Find code that shadows one of Go's predeclared identifiers
     - sloglint # Ensure consistent code style when using log/slog
     - asciicheck  # Checks that all code identifiers does not have non-ASCII symbols in the name
+    - nilnil # Checks that there is no simultaneous return of nil error and an nil value. ref: https://golangci-lint.run/usage/linters/#nilnil
   settings:
     exhaustive:
       default-signifies-exhaustive: false
@@ -53,6 +54,10 @@ linters:
           - varcheck
           - whitespace
         path: _test\.go
+      # TODO: skiip as will require design changes
+      - linters:
+          - nilnil
+        path: istio_virtualservice.go|fqdn.go|cloudflare.go
     paths:
       - endpoint/zz_generated.deepcopy.go
       - third_party$

internal/testutils/env.go

@@ -0,0 +1,29 @@
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package testutils
+
+import (
+	"testing"
+)
+
+func TestHelperEnvSetter(t *testing.T, envs map[string]string) {
+	t.Helper()
+
+	for name, value := range envs {
+		t.Setenv(name, value)
+	}
+}

pkg/tlsutils/tlsconfig.go

@@ -35,14 +35,10 @@ func CreateTLSConfig(prefix string) (*tls.Config, error) {
 	serverName := os.Getenv(fmt.Sprintf("%s_TLS_SERVER_NAME", prefix))
 	isInsecureStr := strings.ToLower(os.Getenv(fmt.Sprintf("%s_TLS_INSECURE", prefix)))
 	isInsecure := isInsecureStr == "true" || isInsecureStr == "yes" || isInsecureStr == "1"
-	tlsConfig, err := NewTLSConfig(certFile, keyFile, caFile, serverName, isInsecure, defaultMinVersion)
-	if err != nil {
-		return nil, err
-	}
-	return tlsConfig, nil
+	return NewTLSConfig(certFile, keyFile, caFile, serverName, isInsecure, defaultMinVersion)
 }
 
-// NewTLSConfig creates a tls.Config instance from directly-passed parameters, loading the ca, cert, and key from disk
+// NewTLSConfig creates a tls.Config instance from directly passed parameters, loading the ca, cert, and key from disk
 func NewTLSConfig(certPath, keyPath, caPath, serverName string, insecure bool, minVersion uint16) (*tls.Config, error) {
 	if certPath != "" && keyPath == "" || certPath == "" && keyPath != "" {
 		return nil, errors.New("either both cert and key or none must be provided")
@@ -55,15 +51,21 @@ func NewTLSConfig(certPath, keyPath, caPath, serverName string, insecure bool, m
 		}
 		certificates = append(certificates, cert)
 	}
-	roots, err := loadRoots(caPath)
-	if err != nil {
-		return nil, err
+	// If rootCAs is nil, TLS uses the host's root CA set.
+	var rootCAs *x509.CertPool
+	var err error
+
+	if caPath != "" {
+		rootCAs, err = loadRoots(caPath)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	return &tls.Config{
 		MinVersion:         minVersion,
 		Certificates:       certificates,
-		RootCAs:            roots,
+		RootCAs:            rootCAs,
 		InsecureSkipVerify: insecure,
 		ServerName:         serverName,
 	}, nil
@@ -71,10 +73,6 @@ func NewTLSConfig(certPath, keyPath, caPath, serverName string, insecure bool, m
 
 // loads CA cert
 func loadRoots(caPath string) (*x509.CertPool, error) {
-	if caPath == "" {
-		return nil, nil
-	}
-
 	roots := x509.NewCertPool()
 	pem, err := os.ReadFile(caPath)
 	if err != nil {

provider/cloudflare/cloudflare.go

@@ -28,7 +28,7 @@ import (
 	"strconv"
 	"strings"
 
-	cloudflare "github.com/cloudflare/cloudflare-go"
+	"github.com/cloudflare/cloudflare-go"
 	log "github.com/sirupsen/logrus"
 
 	"sigs.k8s.io/external-dns/endpoint"

provider/coredns/coredns.go

@@ -18,8 +18,6 @@ package coredns
 
 import (
 	"context"
-	"crypto/tls"
-	"crypto/x509"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -32,6 +30,8 @@ import (
 	log "github.com/sirupsen/logrus"
 	etcdcv3 "go.etcd.io/etcd/client/v3"
 
+	"sigs.k8s.io/external-dns/pkg/tlsutils"
+
 	"sigs.k8s.io/external-dns/endpoint"
 	"sigs.k8s.io/external-dns/plan"
 	"sigs.k8s.io/external-dns/provider"
@@ -153,50 +153,6 @@ func (c etcdClient) DeleteService(key string) error {
 	return err
 }
 
-// loads TLS artifacts and builds tls.Config object
-func newTLSConfig(certPath, keyPath, caPath, serverName string, insecure bool) (*tls.Config, error) {
-	if certPath != "" && keyPath == "" || certPath == "" && keyPath != "" {
-		return nil, errors.New("either both cert and key or none must be provided")
-	}
-	var certificates []tls.Certificate
-	if certPath != "" {
-		cert, err := tls.LoadX509KeyPair(certPath, keyPath)
-		if err != nil {
-			return nil, fmt.Errorf("could not load TLS cert: %w", err)
-		}
-		certificates = append(certificates, cert)
-	}
-	roots, err := loadRoots(caPath)
-	if err != nil {
-		return nil, err
-	}
-
-	return &tls.Config{
-		Certificates:       certificates,
-		RootCAs:            roots,
-		InsecureSkipVerify: insecure,
-		ServerName:         serverName,
-	}, nil
-}
-
-// loads CA cert
-func loadRoots(caPath string) (*x509.CertPool, error) {
-	if caPath == "" {
-		return nil, nil
-	}
-
-	roots := x509.NewCertPool()
-	pem, err := os.ReadFile(caPath)
-	if err != nil {
-		return nil, fmt.Errorf("error reading %s: %w", caPath, err)
-	}
-	ok := roots.AppendCertsFromPEM(pem)
-	if !ok {
-		return nil, fmt.Errorf("could not read root certs: %w", err)
-	}
-	return roots, nil
-}
-
 // builds etcd client config depending on connection scheme and TLS parameters
 func getETCDConfig() (*etcdcv3.Config, error) {
 	etcdURLsStr := os.Getenv("ETCD_URLS")
@@ -210,16 +166,11 @@ func getETCDConfig() (*etcdcv3.Config, error) {
 	if strings.HasPrefix(firstURL, "http://") {
 		return &etcdcv3.Config{Endpoints: etcdURLs, Username: etcdUsername, Password: etcdPassword}, nil
 	} else if strings.HasPrefix(firstURL, "https://") {
-		caFile := os.Getenv("ETCD_CA_FILE")
-		certFile := os.Getenv("ETCD_CERT_FILE")
-		keyFile := os.Getenv("ETCD_KEY_FILE")
-		serverName := os.Getenv("ETCD_TLS_SERVER_NAME")
-		isInsecureStr := strings.ToLower(os.Getenv("ETCD_TLS_INSECURE"))
-		isInsecure := isInsecureStr == "true" || isInsecureStr == "yes" || isInsecureStr == "1"
-		tlsConfig, err := newTLSConfig(certFile, keyFile, caFile, serverName, isInsecure)
+		tlsConfig, err := tlsutils.CreateTLSConfig("ETCD")
 		if err != nil {
 			return nil, err
 		}
+		log.Debug("using TLS for etcd")
 		return &etcdcv3.Config{
 			Endpoints: etcdURLs,
 			TLS:       tlsConfig,
@@ -231,7 +182,7 @@ func getETCDConfig() (*etcdcv3.Config, error) {
 	}
 }
 
-// newETCDClient is an etcd client constructor
+// the newETCDClient is an etcd client constructor
 func newETCDClient() (coreDNSClient, error) {
 	cfg, err := getETCDConfig()
 	if err != nil {
@@ -283,7 +234,7 @@ func findLabelInTargets(targets []string, label string) (string, bool) {
 
 // Records returns all DNS records found in CoreDNS etcd backend. Depending on the record fields
 // it may be mapped to one or two records of type A, CNAME, TXT, A+TXT, CNAME+TXT
-func (p coreDNSProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error) {
+func (p coreDNSProvider) Records(_ context.Context) ([]*endpoint.Endpoint, error) {
 	var result []*endpoint.Endpoint
 	services, err := p.client.GetServices(p.coreDNSPrefix)
 	if err != nil {

provider/coredns/coredns_test.go

@@ -18,13 +18,14 @@ package coredns
 
 import (
 	"context"
-	"os"
 	"reflect"
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	etcdcv3 "go.etcd.io/etcd/client/v3"
 	"sigs.k8s.io/external-dns/endpoint"
+	"sigs.k8s.io/external-dns/internal/testutils"
 	"sigs.k8s.io/external-dns/plan"
 
 	"github.com/stretchr/testify/require"
@@ -87,37 +88,45 @@ func TestETCDConfig(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			closer := envSetter(tt.input)
+			testutils.TestHelperEnvSetter(t, tt.input)
 			cfg, _ := getETCDConfig()
 			if !reflect.DeepEqual(cfg, tt.want) {
 				t.Errorf("unexpected config. Got %v, want %v", cfg, tt.want)
 			}
-			t.Cleanup(closer)
 		})
 	}
-
 }
 
-func envSetter(envs map[string]string) (closer func()) {
-	originalEnvs := map[string]string{}
-
-	for name, value := range envs {
-		if originalValue, ok := os.LookupEnv(name); ok {
-			originalEnvs[name] = originalValue
-		}
-		_ = os.Setenv(name, value)
+func TestEtcdHttpsProtocol(t *testing.T) {
+	envs := map[string]string{
+		"ETCD_URLS": "https://example.com:2379",
 	}
+	testutils.TestHelperEnvSetter(t, envs)
 
-	return func() {
-		for name := range envs {
-			origValue, has := originalEnvs[name]
-			if has {
-				_ = os.Setenv(name, origValue)
-			} else {
-				_ = os.Unsetenv(name)
-			}
-		}
+	cfg, err := getETCDConfig()
+	assert.NoError(t, err)
+	assert.NotNil(t, cfg)
+}
+
+func TestEtcdHttpsIncorrectConfigError(t *testing.T) {
+	envs := map[string]string{
+		"ETCD_URLS":     "https://example.com:2379",
+		"ETCD_KEY_FILE": "incorrect-path-to-etcd-tls-key",
 	}
+	testutils.TestHelperEnvSetter(t, envs)
+
+	_, err := getETCDConfig()
+	assert.Errorf(t, err, "Error creating TLS config: either both cert and key or none must be provided")
+}
+
+func TestEtcdUnsupportedProtocolError(t *testing.T) {
+	envs := map[string]string{
+		"ETCD_URLS": "jdbc:ftp:RemoteHost=MyFTPServer",
+	}
+	testutils.TestHelperEnvSetter(t, envs)
+
+	_, err := getETCDConfig()
+	assert.Errorf(t, err, "etcd URLs must start with either http:// or https://")
 }
 
 func TestAServiceTranslation(t *testing.T) {

provider/digitalocean/digital_ocean_test.go

@@ -73,11 +73,11 @@ func (m *mockDigitalOceanClient) CreateRecord(context.Context, string, *godo.Dom
 }
 
 func (m *mockDigitalOceanClient) Delete(context.Context, string) (*godo.Response, error) {
-	return nil, nil
+	return nil, fmt.Errorf("failed to delete domain")
 }
 
 func (m *mockDigitalOceanClient) DeleteRecord(ctx context.Context, domain string, id int) (*godo.Response, error) {
-	return nil, nil
+	return nil, fmt.Errorf("failed to delete record")
 }
 
 func (m *mockDigitalOceanClient) EditRecord(ctx context.Context, domain string, id int, editRequest *godo.DomainRecordEditRequest) (*godo.DomainRecord, *godo.Response, error) {
@@ -160,11 +160,11 @@ func (m *mockDigitalOceanRecordsFail) CreateRecord(context.Context, string, *god
 }
 
 func (m *mockDigitalOceanRecordsFail) Delete(context.Context, string) (*godo.Response, error) {
-	return nil, nil
+	return nil, fmt.Errorf("failed to delete record")
 }
 
 func (m *mockDigitalOceanRecordsFail) DeleteRecord(ctx context.Context, domain string, id int) (*godo.Response, error) {
-	return nil, nil
+	return nil, fmt.Errorf("failed to delete record")
 }
 
 func (m *mockDigitalOceanRecordsFail) EditRecord(ctx context.Context, domain string, id int, editRequest *godo.DomainRecordEditRequest) (*godo.DomainRecord, *godo.Response, error) {

provider/ns1/ns1_test.go

@@ -39,15 +39,15 @@ type MockNS1DomainClient struct {
 }
 
 func (m *MockNS1DomainClient) CreateRecord(r *dns.Record) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1DomainClient) DeleteRecord(zone string, domain string, t string) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1DomainClient) UpdateRecord(r *dns.Record) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1DomainClient) GetZone(zone string) (*dns.Zone, *http.Response, error) {
@@ -81,16 +81,16 @@ func (m *MockNS1DomainClient) ListZones() ([]*dns.Zone, *http.Response, error) {
 
 type MockNS1GetZoneFail struct{}
 
-func (m *MockNS1GetZoneFail) CreateRecord(r *dns.Record) (*http.Response, error) {
-	return nil, nil
+func (m *MockNS1GetZoneFail) CreateRecord(_ *dns.Record) (*http.Response, error) {
+	return &http.Response{}, nil
 }
 
-func (m *MockNS1GetZoneFail) DeleteRecord(zone string, domain string, t string) (*http.Response, error) {
-	return nil, nil
+func (m *MockNS1GetZoneFail) DeleteRecord(_ string, _ string, _ string) (*http.Response, error) {
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1GetZoneFail) UpdateRecord(r *dns.Record) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1GetZoneFail) GetZone(zone string) (*dns.Zone, *http.Response, error) {
@@ -107,20 +107,20 @@ func (m *MockNS1GetZoneFail) ListZones() ([]*dns.Zone, *http.Response, error) {
 
 type MockNS1ListZonesFail struct{}
 
-func (m *MockNS1ListZonesFail) CreateRecord(r *dns.Record) (*http.Response, error) {
-	return nil, nil
+func (m *MockNS1ListZonesFail) CreateRecord(_ *dns.Record) (*http.Response, error) {
+	return &http.Response{}, nil
 }
 
-func (m *MockNS1ListZonesFail) DeleteRecord(zone string, domain string, t string) (*http.Response, error) {
-	return nil, nil
+func (m *MockNS1ListZonesFail) DeleteRecord(_ string, _ string, _ string) (*http.Response, error) {
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1ListZonesFail) UpdateRecord(r *dns.Record) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *MockNS1ListZonesFail) GetZone(zone string) (*dns.Zone, *http.Response, error) {
-	return &dns.Zone{}, nil, nil
+	return &dns.Zone{}, &http.Response{}, nil
 }
 
 func (m *MockNS1ListZonesFail) ListZones() ([]*dns.Zone, *http.Response, error) {

provider/pdns/pdns_test.go

@@ -690,7 +690,7 @@ func (c *PDNSAPIClientStub) ListZone(zoneID string) (pgo.Zone, *http.Response, e
 }
 
 func (c *PDNSAPIClientStub) PatchZone(zoneID string, zoneStruct pgo.Zone) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 /******************************************************************************/
@@ -721,7 +721,7 @@ func (c *PDNSAPIClientStubEmptyZones) ListZone(zoneID string) (pgo.Zone, *http.R
 
 func (c *PDNSAPIClientStubEmptyZones) PatchZone(zoneID string, zoneStruct pgo.Zone) (*http.Response, error) {
 	c.patchedZones = append(c.patchedZones, zoneStruct)
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 /******************************************************************************/

provider/scaleway/scaleway_test.go

@@ -106,8 +106,8 @@ func (m *mockScalewayDomain) ListDNSZoneRecords(req *domain.ListDNSZoneRecordsRe
 	}, nil
 }
 
-func (m *mockScalewayDomain) UpdateDNSZoneRecords(req *domain.UpdateDNSZoneRecordsRequest, opts ...scw.RequestOption) (*domain.UpdateDNSZoneRecordsResponse, error) {
-	return nil, nil
+func (m *mockScalewayDomain) UpdateDNSZoneRecords(_ *domain.UpdateDNSZoneRecordsRequest, _ ...scw.RequestOption) (*domain.UpdateDNSZoneRecordsResponse, error) {
+	return &domain.UpdateDNSZoneRecordsResponse{}, nil
 }
 
 func TestScalewayProvider_NewScalewayProvider(t *testing.T) {

provider/ultradns/ultradns_test.go

@@ -65,11 +65,11 @@ type mockUltraDNSRecord struct {
 	client *udnssdk.Client
 }
 
-func (m *mockUltraDNSRecord) Create(k udnssdk.RRSetKey, rrset udnssdk.RRSet) (*http.Response, error) {
-	return nil, nil
+func (m *mockUltraDNSRecord) Create(_ udnssdk.RRSetKey, _ udnssdk.RRSet) (*http.Response, error) {
+	return &http.Response{}, nil
 }
 
-func (m *mockUltraDNSRecord) Select(k udnssdk.RRSetKey) ([]udnssdk.RRSet, error) {
+func (m *mockUltraDNSRecord) Select(_ udnssdk.RRSetKey) ([]udnssdk.RRSet, error) {
 	return []udnssdk.RRSet{{
 		OwnerName: "test-ultradns-provider.com.",
 		RRType:    endpoint.RecordTypeA,
@@ -83,11 +83,11 @@ func (m *mockUltraDNSRecord) SelectWithOffset(k udnssdk.RRSetKey, offset int) ([
 }
 
 func (m *mockUltraDNSRecord) Update(udnssdk.RRSetKey, udnssdk.RRSet) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *mockUltraDNSRecord) Delete(k udnssdk.RRSetKey) (*http.Response, error) {
-	return nil, nil
+	return &http.Response{}, nil
 }
 
 func (m *mockUltraDNSRecord) SelectWithOffsetWithLimit(k udnssdk.RRSetKey, offset int, limit int) (rrsets []udnssdk.RRSet, ResultInfo udnssdk.ResultInfo, resp *http.Response, err error) {

source/istio_virtualservice.go

@@ -17,6 +17,7 @@ limitations under the License.
 package source
 
 import (
+	"cmp"
 	"context"
 	"fmt"
 	"sort"
@@ -193,7 +194,7 @@ func (sc *virtualServiceSource) Endpoints(ctx context.Context) ([]*endpoint.Endp
 }
 
 // AddEventHandler adds an event handler that should be triggered if the watched Istio VirtualService changes.
-func (sc *virtualServiceSource) AddEventHandler(ctx context.Context, handler func()) {
+func (sc *virtualServiceSource) AddEventHandler(_ context.Context, handler func()) {
 	log.Debug("Adding event handler for Istio VirtualService")
 
 	sc.virtualserviceInformer.Informer().AddEventHandler(eventHandlerFunc(handler))
@@ -210,21 +211,19 @@ func (sc *virtualServiceSource) getGateway(_ context.Context, gatewayStr string,
 		log.Debugf("Failed parsing gatewayStr %s of VirtualService %s/%s", gatewayStr, virtualService.Namespace, virtualService.Name)
 		return nil, err
 	}
-	if namespace == "" {
-		namespace = virtualService.Namespace
-	}
+	namespace = cmp.Or(namespace, virtualService.Namespace)
 
 	gateway, err := sc.gatewayInformer.Lister().Gateways(namespace).Get(name)
 	if errors.IsNotFound(err) {
 		log.Warnf("VirtualService (%s/%s) references non-existent gateway: %s ", virtualService.Namespace, virtualService.Name, gatewayStr)
-		return nil, nil
+		return gateway, nil
 	} else if err != nil {
 		log.Errorf("Failed retrieving gateway %s referenced by VirtualService %s/%s: %v", gatewayStr, virtualService.Namespace, virtualService.Name, err)
 		return nil, err
 	}
 	if gateway == nil {
 		log.Debugf("Gateway %s referenced by VirtualService %s/%s not found: %v", gatewayStr, virtualService.Namespace, virtualService.Name, err)
-		return nil, nil
+		return gateway, nil
 	}
 	return gateway, nil
 }
@@ -290,17 +289,17 @@ func (sc *virtualServiceSource) targetsFromVirtualService(ctx context.Context, v
 	var targets []string
 	// for each host we need to iterate through the gateways because each host might match for only one of the gateways
 	for _, gateway := range virtualService.Spec.Gateways {
-		gateway, err := sc.getGateway(ctx, gateway, virtualService)
+		gw, err := sc.getGateway(ctx, gateway, virtualService)
 		if err != nil {
 			return nil, err
 		}
-		if gateway == nil {
+		if gw == nil {
 			continue
 		}
-		if !virtualServiceBindsToGateway(virtualService, gateway, vsHost) {
+		if !virtualServiceBindsToGateway(virtualService, gw, vsHost) {
 			continue
 		}
-		tgs, err := sc.targetsFromGateway(ctx, gateway)
+		tgs, err := sc.targetsFromGateway(ctx, gw)
 		if err != nil {
 			return targets, err
 		}

source/traefik_proxy_test.go

@@ -19,6 +19,7 @@ package source
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/mock"
@@ -1806,7 +1807,7 @@ type testInformer struct {
 	times int
 }
 
-func (t *testInformer) AddEventHandler(handler cache.ResourceEventHandler) (cache.ResourceEventHandlerRegistration, error) {
+func (t *testInformer) AddEventHandler(_ cache.ResourceEventHandler) (cache.ResourceEventHandlerRegistration, error) {
 	t.times += 1
-	return nil, nil
+	return nil, fmt.Errorf("not implemented")
 }