From 791601e2e35e5eba8887bccb5c09d365863d7875 Mon Sep 17 00:00:00 2001 From: Ivan Ka <5395690+ivankatliarchuk@users.noreply.github.com> Date: Mon, 19 May 2025 08:19:16 +0100 Subject: [PATCH] chore(codebase): add nilnil return check (#5415) * chore(code-quality): added nilnil return check Signed-off-by: ivan katliarchuk * chore(code-quality): added nilnil return check --------- Signed-off-by: ivan katliarchuk --- .golangci.yml | 5 ++ internal/testutils/env.go | 29 ++++++++++ pkg/tlsutils/tlsconfig.go | 26 ++++----- provider/cloudflare/cloudflare.go | 2 +- provider/coredns/coredns.go | 61 ++------------------- provider/coredns/coredns_test.go | 51 ++++++++++------- provider/digitalocean/digital_ocean_test.go | 8 +-- provider/ns1/ns1_test.go | 28 +++++----- provider/pdns/pdns_test.go | 4 +- provider/scaleway/scaleway_test.go | 4 +- provider/ultradns/ultradns_test.go | 10 ++-- source/istio_virtualservice.go | 19 +++---- source/traefik_proxy_test.go | 5 +- 13 files changed, 122 insertions(+), 130 deletions(-) create mode 100644 internal/testutils/env.go diff --git a/.golangci.yml b/.golangci.yml index 1b5f134a2..7b12c19c9 100644 --- a/.golangci.yml +++ b/.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$ diff --git a/internal/testutils/env.go b/internal/testutils/env.go new file mode 100644 index 000000000..7f90597af --- /dev/null +++ b/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) + } +} diff --git a/pkg/tlsutils/tlsconfig.go b/pkg/tlsutils/tlsconfig.go index 5275fad13..e6e4373b3 100644 --- a/pkg/tlsutils/tlsconfig.go +++ b/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 { diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index 87c41ad23..593b8da6e 100644 --- a/provider/cloudflare/cloudflare.go +++ b/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" diff --git a/provider/coredns/coredns.go b/provider/coredns/coredns.go index 18dc9cea7..21fb29293 100644 --- a/provider/coredns/coredns.go +++ b/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 { diff --git a/provider/coredns/coredns_test.go b/provider/coredns/coredns_test.go index baf5652f8..92b781e8b 100644 --- a/provider/coredns/coredns_test.go +++ b/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) { diff --git a/provider/digitalocean/digital_ocean_test.go b/provider/digitalocean/digital_ocean_test.go index 7088b0922..4fd6ec052 100644 --- a/provider/digitalocean/digital_ocean_test.go +++ b/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) { diff --git a/provider/ns1/ns1_test.go b/provider/ns1/ns1_test.go index 6d529879b..21e21fcbd 100644 --- a/provider/ns1/ns1_test.go +++ b/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) { diff --git a/provider/pdns/pdns_test.go b/provider/pdns/pdns_test.go index 773d4897c..c8a22165b 100644 --- a/provider/pdns/pdns_test.go +++ b/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 } /******************************************************************************/ diff --git a/provider/scaleway/scaleway_test.go b/provider/scaleway/scaleway_test.go index 560687c7b..45f09b321 100644 --- a/provider/scaleway/scaleway_test.go +++ b/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) { diff --git a/provider/ultradns/ultradns_test.go b/provider/ultradns/ultradns_test.go index 2d027cccc..380650e32 100644 --- a/provider/ultradns/ultradns_test.go +++ b/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) { diff --git a/source/istio_virtualservice.go b/source/istio_virtualservice.go index be9407d6c..7e76f597a 100644 --- a/source/istio_virtualservice.go +++ b/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 } diff --git a/source/traefik_proxy_test.go b/source/traefik_proxy_test.go index 4db395254..4cd1cec12 100644 --- a/source/traefik_proxy_test.go +++ b/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") }