ci(linter): add go-critic

.golangci.yml

@@ -28,6 +28,7 @@ linters:
     - nilnil # Checks that there is no simultaneous return of nil error and an nil value. ref: https://golangci-lint.run/usage/linters/#nilnil
     - nonamedreturns # Checks that functions with named return values do not return named values. https://golangci-lint.run/usage/linters/#nonamedreturns
     - cyclop # Checks function and package cyclomatic complexity. https://golangci-lint.run/usage/linters/#cyclop
+    - gocritic # Analyze source code for various issues, including bugs, performance hiccups, and non-idiomatic coding practices. https://golangci-lint.run/docs/linters/configuration/#gocritic
 
     # tests
     - testifylint # Checks usage of github.com/stretchr/testify. https://golangci-lint.run/usage/linters/#testifylint

controller/controller.go

@@ -353,7 +353,7 @@ func (c *Controller) Run(ctx context.Context) {
 					consecutiveSoftErrors.Gauge.Set(float64(softErrorCount))
 					log.Errorf("Failed to do run once: %v (consecutive soft errors: %d)", err, softErrorCount)
 				} else {
-					log.Fatalf("Failed to do run once: %v", err)
+					log.Fatalf("Failed to do run once: %v", err) // nolint: gocritic // exitAfterDefer
 				}
 			} else {
 				if softErrorCount > 0 {

controller/execute.go

@@ -104,7 +104,7 @@ func Execute() {
 
 	endpointsSource, err := buildSource(ctx, cfg)
 	if err != nil {
-		log.Fatal(err)
+		log.Fatal(err) // nolint: gocritic // exitAfterDefer
 	}
 
 	domainFilter := createDomainFilter(cfg)

endpoint/domain_filter.go

@@ -122,13 +122,12 @@ func matchFilter(filters []string, domain string, emptyval bool) bool {
 			continue
 		}
 
-		if strings.HasPrefix(filter, ".") && strings.HasSuffix(strippedDomain, filter) {
+		switch {
+		case strings.HasPrefix(filter, ".") && strings.HasSuffix(strippedDomain, filter):
 			return true
-		} else if strings.Count(strippedDomain, ".") == strings.Count(filter, ".") {
-			if strippedDomain == filter {
-				return true
-			}
-		} else if strings.HasSuffix(strippedDomain, "."+filter) {
+		case strings.Count(strippedDomain, ".") == strings.Count(filter, ".") && strippedDomain == filter:
+			return true
+		case strings.HasSuffix(strippedDomain, "."+filter):
 			return true
 		}
 	}

endpoint/endpoint.go

@@ -386,11 +386,12 @@ func FilterEndpointsByOwnerID(ownerID string, eps []*Endpoint) []*Endpoint {
 	filtered := []*Endpoint{}
 	for _, ep := range eps {
 		endpointOwner, ok := ep.Labels[OwnerLabelKey]
-		if !ok {
+		switch {
+		case !ok:
 			log.Debugf(`Skipping endpoint %v because of missing owner label (required: "%s")`, ep, ownerID)
-		} else if endpointOwner != ownerID {
+		case endpointOwner != ownerID:
 			log.Debugf(`Skipping endpoint %v because owner id does not match (found: "%s", required: "%s")`, ep, endpointOwner, ownerID)
-		} else {
+		default:
 			filtered = append(filtered, ep)
 		}
 	}

internal/gen/docs/flags/main.go

@@ -57,7 +57,7 @@ func main() {
 	if err != nil {
 		_ = fmt.Errorf("failed to generate markdown file '%s': %v", path, err.Error())
 	}
-	content = content + "\n"
+	content += "\n"
 	_ = utils.WriteToFile(path, content)
 }
 

internal/gen/docs/flags/main_test.go

@@ -73,7 +73,7 @@ func TestFlagsMdUpToDate(t *testing.T) {
 	flags := computeFlags()
 	actual, err := flags.generateMarkdownTable()
 	assert.NoError(t, err)
-	actual = actual + "\n"
+	actual += "\n"
 	assert.Len(t, actual, len(expected), "expected file '%s' to be up to date. execute 'make generate-flags-documentation", fileName)
 }
 

internal/gen/docs/metrics/main.go

@@ -54,7 +54,7 @@ func main() {
 		_, _ = fmt.Fprintf(os.Stderr, "failed to generate markdown file '%s': %v\n", path, err)
 		os.Exit(1)
 	}
-	content = content + "\n"
+	content += "\n"
 	_ = utils.WriteToFile(path, content)
 }
 
@@ -119,6 +119,7 @@ func getRuntimeMetrics(reg prometheus.Registerer) []string {
 				runtimeMetrics = append(runtimeMetrics, k)
 			}
 		}
+	default:
 	}
 	sort.Strings(runtimeMetrics)
 	return runtimeMetrics

internal/testutils/endpoint_test.go

@@ -84,7 +84,7 @@ func TestExampleSameEndpoints(t *testing.T) {
 	// example.org 0 IN TXT  load-balancer.org []
 }
 
-func makeEndpoint(DNSName string) *endpoint.Endpoint {
+func makeEndpoint(DNSName string) *endpoint.Endpoint { // nolint: gocritic // captLocal
 	return &endpoint.Endpoint{
 		DNSName:       DNSName,
 		Targets:       endpoint.Targets{"target.com"},

pkg/events/types.go

@@ -177,7 +177,7 @@ func sanitize(input string) string {
 
 	// the name should end with an alphanumeric character
 	if len(sanitized) > 0 && !endsWithAlphaNumeric.MatchString(sanitized) {
-		sanitized = sanitized + "z"
+		sanitized += "z"
 	}
 
 	sanitized = invalidChars.ReplaceAllString(sanitized, "-")

pkg/rfc2317/arpa.go

@@ -39,7 +39,7 @@ func CidrToInAddr(cidr string) (string, error) {
 			// address for all IPv4 addresses no matter how they are
 			// expressed internally.
 		} else {
-			cidr = cidr + "/128"
+			cidr += "/128"
 		}
 	}
 

provider/akamai/akamai_test.go

@@ -287,10 +287,11 @@ func TestCreateRecordsDomainFilter(t *testing.T) {
 	assert.NoError(t, err)
 
 	zoneNameIDMapper := provider.ZoneIDName{"example.com": "example.com"}
-	endpoints := make([]*endpoint.Endpoint, 0)
-	endpoints = append(endpoints, endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"))
-	endpoints = append(endpoints, endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"))
-	exclude := append(endpoints, endpoint.NewEndpoint("www.exclude.me", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"))
+	exclude := []*endpoint.Endpoint{
+		endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"),
+		endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"),
+		endpoint.NewEndpoint("www.exclude.me", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"),
+	}
 
 	err = c.createRecordsets(zoneNameIDMapper, exclude)
 	assert.NoError(t, err)
@@ -321,10 +322,11 @@ func TestDeleteRecordsDomainFilter(t *testing.T) {
 	require.NoError(t, err)
 
 	zoneNameIDMapper := provider.ZoneIDName{"example.com": "example.com"}
-	endpoints := make([]*endpoint.Endpoint, 0)
-	endpoints = append(endpoints, endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"))
-	endpoints = append(endpoints, endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"))
-	exclude := append(endpoints, endpoint.NewEndpoint("www.exclude.me", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"))
+	exclude := []*endpoint.Endpoint{
+		endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"),
+		endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"),
+		endpoint.NewEndpoint("www.exclude.me", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"),
+	}
 
 	err = c.deleteRecordsets(zoneNameIDMapper, exclude)
 	assert.NoError(t, err)
@@ -355,10 +357,11 @@ func TestUpdateRecordsDomainFilter(t *testing.T) {
 	require.NoError(t, err)
 
 	zoneNameIDMapper := provider.ZoneIDName{"example.com": "example.com"}
-	endpoints := make([]*endpoint.Endpoint, 0)
-	endpoints = append(endpoints, endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"))
-	endpoints = append(endpoints, endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"))
-	exclude := append(endpoints, endpoint.NewEndpoint("www.exclude.me", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"))
+	exclude := []*endpoint.Endpoint{
+		endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"),
+		endpoint.NewEndpoint("www.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"),
+		endpoint.NewEndpoint("www.exclude.me", endpoint.RecordTypeA, "10.0.0.2", "10.0.0.3"),
+	}
 
 	err = c.updateNewRecordsets(zoneNameIDMapper, exclude)
 	require.NoError(t, err)

provider/aws/aws.go

@@ -772,7 +772,7 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes Route53Changes,
 							log.Errorf("Failed submitting change (error: %v), it will be retried in a separate change batch in the next iteration", err)
 							p.failedChangesQueue[z] = append(p.failedChangesQueue[z], changes...)
 						} else {
-							successfulChanges = successfulChanges + len(changes)
+							successfulChanges += len(changes)
 						}
 					}
 				} else {

provider/awssd/aws_sd.go

@@ -195,26 +195,24 @@ func (p *AWSSDProvider) instancesToEndpoint(ns *sdtypes.NamespaceSummary, srv *s
 	}
 
 	for _, inst := range instances {
+		switch {
 		// CNAME
-		if inst.Attributes[sdInstanceAttrCname] != "" && srv.DnsConfig.DnsRecords[0].Type == sdtypes.RecordTypeCname {
+		case inst.Attributes[sdInstanceAttrCname] != "" && srv.DnsConfig.DnsRecords[0].Type == sdtypes.RecordTypeCname:
 			newEndpoint.RecordType = endpoint.RecordTypeCNAME
 			newEndpoint.Targets = append(newEndpoint.Targets, inst.Attributes[sdInstanceAttrCname])
-
-			// ALIAS
-		} else if inst.Attributes[sdInstanceAttrAlias] != "" {
+		// ALIAS
+		case inst.Attributes[sdInstanceAttrAlias] != "":
 			newEndpoint.RecordType = endpoint.RecordTypeCNAME
 			newEndpoint.Targets = append(newEndpoint.Targets, inst.Attributes[sdInstanceAttrAlias])
-
-			// IPv4-based target
-		} else if inst.Attributes[sdInstanceAttrIPV4] != "" {
+		// IPv4-based target
+		case inst.Attributes[sdInstanceAttrIPV4] != "":
 			newEndpoint.RecordType = endpoint.RecordTypeA
 			newEndpoint.Targets = append(newEndpoint.Targets, inst.Attributes[sdInstanceAttrIPV4])
-
-			// IPv6-based target
-		} else if inst.Attributes[sdInstanceAttrIPV6] != "" {
+		// IPv6-based target
+		case inst.Attributes[sdInstanceAttrIPV6] != "":
 			newEndpoint.RecordType = endpoint.RecordTypeAAAA
 			newEndpoint.Targets = append(newEndpoint.Targets, inst.Attributes[sdInstanceAttrIPV6])
-		} else {
+		default:
 			log.Warnf("Invalid instance \"%v\" found in service \"%v\"", inst, srv.Name)
 		}
 	}

provider/azure/azure_privatedns_test.go

@@ -23,6 +23,7 @@ import (
 	azcoreruntime "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
 	privatedns "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/privatedns/armprivatedns"
+
 	"sigs.k8s.io/external-dns/endpoint"
 	"sigs.k8s.io/external-dns/plan"
 	"sigs.k8s.io/external-dns/provider"
@@ -117,7 +118,6 @@ func (client *mockPrivateRecordSetsClient) CreateOrUpdate(ctx context.Context, r
 		),
 	)
 	return privatedns.RecordSetsClientCreateOrUpdateResponse{}, nil
-	//return parameters, nil
 }
 
 func createMockPrivateZone(zone string, id string) *privatedns.PrivateZone {

provider/civo/civo_test.go

@@ -1170,11 +1170,12 @@ func TestCivoChangesEmpty(t *testing.T) {
 // call to ObjectsAreEqual replaced with cmp.Equal which better handles struct's with pointers to
 // other structs. It also ignores ordering when comparing unlike cmp.Equal.
 func elementsMatch(t *testing.T, listA, listB interface{}, msgAndArgs ...interface{}) bool {
-	if listA == nil && listB == nil {
+	switch {
+	case listA == nil && listB == nil:
 		return true
-	} else if listA == nil {
+	case listA == nil:
 		return isEmpty(listB)
-	} else if listB == nil {
+	case listB == nil:
 		return isEmpty(listA)
 	}
 

provider/cloudflare/cloudflare.go

@@ -651,7 +651,8 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
 			if chErr != nil {
 				return fmt.Errorf("could not fetch custom hostnames from zone, %w", chErr)
 			}
-			if change.Action == cloudFlareUpdate {
+			switch change.Action {
+			case cloudFlareUpdate:
 				if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) {
 					failedChange = true
 				}
@@ -666,7 +667,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
 					failedChange = true
 					log.WithFields(logFields).Errorf("failed to update record: %v", err)
 				}
-			} else if change.Action == cloudFlareDelete {
+			case cloudFlareDelete:
 				recordID := p.getRecordID(records, change.ResourceRecord)
 				if recordID == "" {
 					log.WithFields(logFields).Errorf("failed to find previous record: %v", change.ResourceRecord)
@@ -680,7 +681,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
 				if !p.submitCustomHostnameChanges(ctx, zoneID, change, chs, logFields) {
 					failedChange = true
 				}
-			} else if change.Action == cloudFlareCreate {
+			case cloudFlareCreate:
 				recordParam := getCreateDNSRecordParam(zoneID, change)
 				_, err := p.Client.CreateDNSRecord(ctx, recordParam)
 				if err != nil {

provider/coredns/coredns.go

@@ -163,9 +163,10 @@ func getETCDConfig() (*etcdcv3.Config, error) {
 	firstURL := strings.ToLower(etcdURLs[0])
 	etcdUsername := os.Getenv("ETCD_USERNAME")
 	etcdPassword := os.Getenv("ETCD_PASSWORD")
-	if strings.HasPrefix(firstURL, "http://") {
+	switch {
+	case strings.HasPrefix(firstURL, "http://"):
 		return &etcdcv3.Config{Endpoints: etcdURLs, Username: etcdUsername, Password: etcdPassword}, nil
-	} else if strings.HasPrefix(firstURL, "https://") {
+	case strings.HasPrefix(firstURL, "https://"):
 		tlsConfig, err := tlsutils.CreateTLSConfig("ETCD")
 		if err != nil {
 			return nil, err
@@ -177,7 +178,7 @@ func getETCDConfig() (*etcdcv3.Config, error) {
 			Username:  etcdUsername,
 			Password:  etcdPassword,
 		}, nil
-	} else {
+	default:
 		return nil, errors.New("etcd URLs must start with either http:// or https://")
 	}
 }

provider/digitalocean/digital_ocean_test.go

@@ -195,11 +195,12 @@ func isEmpty(xs interface{}) bool {
 // call to ObjectsAreEqual replaced with cmp.Equal which better handles struct's with pointers to
 // other structs. It also ignores ordering when comparing unlike cmp.Equal.
 func elementsMatch(t *testing.T, listA, listB interface{}, msgAndArgs ...interface{}) bool {
-	if listA == nil && listB == nil {
+	switch {
+	case listA == nil && listB == nil:
 		return true
-	} else if listA == nil {
+	case listA == nil:
 		return isEmpty(listB)
-	} else if listB == nil {
+	case listB == nil:
 		return isEmpty(listA)
 	}
 

provider/godaddy/godaddy.go

@@ -178,16 +178,17 @@ func (p *GDProvider) zonesRecords(ctx context.Context, all bool) ([]string, []gd
 		return nil, nil, err
 	}
 
-	if len(zones) == 0 {
+	switch len(zones) {
+	case 0:
 		allRecords = []gdRecords{}
-	} else if len(zones) == 1 {
+	case 1:
 		record, err := p.records(&ctx, zones[0], all)
 		if err != nil {
 			return nil, nil, err
 		}
 
 		allRecords = append(allRecords, *record)
-	} else {
+	default:
 		chRecords := make(chan gdRecords, len(zones))
 
 		eg, ctx := errgroup.WithContext(ctx)

provider/oci/oci.go

@@ -100,7 +100,8 @@ func NewOCIProvider(cfg OCIConfig, domainFilter *endpoint.DomainFilter, zoneIDFi
 	if cfg.Auth.UseInstancePrincipal && cfg.Auth.UseWorkloadIdentity {
 		return nil, errors.New("only one of 'useInstancePrincipal' and 'useWorkloadIdentity' may be enabled for Oracle authentication")
 	}
-	if cfg.Auth.UseWorkloadIdentity {
+	switch {
+	case cfg.Auth.UseWorkloadIdentity:
 		// OCI SDK requires specific, dynamic environment variables for workload identity.
 		if err := os.Setenv(auth.ResourcePrincipalVersionEnvVar, auth.ResourcePrincipalVersion2_2); err != nil {
 			return nil, fmt.Errorf("unable to set OCI SDK environment variable: %s: %w", auth.ResourcePrincipalVersionEnvVar, err)
@@ -112,12 +113,12 @@ func NewOCIProvider(cfg OCIConfig, domainFilter *endpoint.DomainFilter, zoneIDFi
 		if err != nil {
 			return nil, fmt.Errorf("error creating OCI workload identity config provider: %w", err)
 		}
-	} else if cfg.Auth.UseInstancePrincipal {
+	case cfg.Auth.UseInstancePrincipal:
 		configProvider, err = auth.InstancePrincipalConfigurationProvider()
 		if err != nil {
 			return nil, fmt.Errorf("error creating OCI instance principal config provider: %w", err)
 		}
-	} else {
+	default:
 		configProvider = common.NewRawConfigurationProvider(
 			cfg.Auth.TenancyID,
 			cfg.Auth.UserID,

provider/ovh/ovh.go

@@ -552,7 +552,7 @@ func (p *OVHProvider) newOvhChangeCreateDelete(action int, endpoints []*endpoint
 	return ovhChanges, existingRecords
 }
 
-func convertDNSNameIntoSubDomain(DNSName string, zoneName string) string {
+func convertDNSNameIntoSubDomain(DNSName string, zoneName string) string { // nolint: gocritic // captLocal
 	if DNSName == zoneName {
 		return ""
 	}