Enable more linters (#2545)

lucacome · web-flow · commit a4321512e978 · 2024-09-17T18:37:31.000Z
diff --git a/.golangci.yml b/.golangci.yml
@@ -53,14 +53,19 @@ linters:
   enable:
     - asasalint
     - asciicheck
+    - bidichk
     - copyloopvar
     - dupword
+    - durationcheck
     - errcheck
+    - errchkjson
     - errname
     - errorlint
     - fatcontext
     - ginkgolinter
     - gocheckcompilerdirectives
+    - gochecksumtype
+    - gocritic
     - gocyclo
     - godot
     - gofmt
@@ -76,6 +81,7 @@ linters:
     - loggercheck
     - makezero
     - misspell
+    - musttag
     - nilerr
     - noctx
     - nolintlint
@@ -86,6 +92,7 @@ linters:
     - spancheck
     - staticcheck
     - stylecheck
+    - tagalign
     - tenv
     - thelper
     - tparallel
diff --git a/internal/mode/provisioner/manager.go b/internal/mode/provisioner/manager.go
@@ -133,7 +133,7 @@ func StartManager(cfg Config) error {
 		statusUpdater,
 		mgr.GetClient(),
 		embeddedfiles.StaticModeDeploymentYAML,
-		func() metav1.Time { return metav1.Now() },
+		metav1.Now,
 	)
 
 	eventLoop := events.NewEventLoop(
diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go
@@ -471,8 +471,7 @@ func getGatewayAddresses(
 	}
 
 	var addresses, hostnames []string
-	switch gwSvc.Spec.Type {
-	case v1.ServiceTypeLoadBalancer:
+	if gwSvc.Spec.Type == v1.ServiceTypeLoadBalancer {
 		for _, ingress := range gwSvc.Status.LoadBalancer.Ingress {
 			if ingress.IP != "" {
 				addresses = append(addresses, ingress.IP)
diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go
@@ -107,7 +107,8 @@ func StartManager(cfg config.Config) error {
 		Namespace: cfg.GatewayPodConfig.Namespace,
 		Name:      cfg.ConfigName,
 	}
-	if err := registerControllers(ctx, cfg, mgr, recorder, logLevelSetter, eventCh, controlConfigNSName); err != nil {
+	err = registerControllers(ctx, cfg, mgr, recorder, logLevelSetter, eventCh, controlConfigNSName)
+	if err != nil {
 		return err
 	}
 
@@ -149,9 +150,11 @@ func StartManager(cfg config.Config) error {
 	processHandler := ngxruntime.NewProcessHandlerImpl(os.ReadFile, os.Stat)
 
 	// Ensure NGINX is running before registering metrics & starting the manager.
-	if _, err := processHandler.FindMainProcess(ctx, ngxruntime.PidFileTimeout); err != nil {
+	p, err := processHandler.FindMainProcess(ctx, ngxruntime.PidFileTimeout)
+	if err != nil {
 		return fmt.Errorf("NGINX is not running: %w", err)
 	}
+	cfg.Logger.V(1).Info("NGINX is running with PID", "pid", p)
 
 	var (
 		ngxruntimeCollector ngxruntime.MetricsCollector = collectors.NewManagerNoopCollector()
@@ -162,11 +165,6 @@ func StartManager(cfg config.Config) error {
 	var usageSecret *usage.Secret
 
 	if cfg.Plus {
-		ngxPlusClient, err = ngxruntime.CreatePlusClient()
-		if err != nil {
-			return fmt.Errorf("error creating NGINX plus client: %w", err)
-		}
-
 		if cfg.UsageReportConfig != nil {
 			usageSecret = usage.NewUsageSecret()
 			reporter, err := createUsageReporterJob(mgr.GetAPIReader(), cfg, usageSecret, nginxChecker.getReadyCh())
@@ -188,6 +186,10 @@ func StartManager(cfg config.Config) error {
 		constLabels := map[string]string{"class": cfg.GatewayClassName}
 		var ngxCollector prometheus.Collector
 		if cfg.Plus {
+			ngxPlusClient, err = ngxruntime.CreatePlusClient()
+			if err != nil {
+				return fmt.Errorf("error creating NGINX plus client: %w", err)
+			}
 			ngxCollector, err = collectors.NewNginxPlusMetricsCollector(ngxPlusClient, constLabels, promLogger)
 		} else {
 			ngxCollector = collectors.NewNginxMetricsCollector(constLabels, promLogger)
diff --git a/internal/mode/static/nginx/config/validation/common_test.go b/internal/mode/static/nginx/config/validation/common_test.go
@@ -48,7 +48,7 @@ func TestValidateEscapedStringNoVarExpansion(t *testing.T) {
 
 func TestValidateValidHeaderName(t *testing.T) {
 	t.Parallel()
-	validator := func(value string) error { return validateHeaderName(value) }
+	validator := validateHeaderName
 
 	testValidValuesForSimpleValidator(
 		t,
diff --git a/internal/mode/static/nginx/file/folders.go b/internal/mode/static/nginx/file/folders.go
@@ -28,12 +28,12 @@ func ClearFolders(fileMgr ClearFoldersOSFileManager, paths []string) (removedFil
 		}
 
 		for _, entry := range entries {
-			path := filepath.Join(path, entry.Name())
-			if err := fileMgr.Remove(path); err != nil {
-				return removedFiles, fmt.Errorf("failed to remove %q: %w", path, err)
+			entryPath := filepath.Join(path, entry.Name())
+			if err := fileMgr.Remove(entryPath); err != nil {
+				return removedFiles, fmt.Errorf("failed to remove %q: %w", entryPath, err)
 			}
 
-			removedFiles = append(removedFiles, path)
+			removedFiles = append(removedFiles, entryPath)
 		}
 	}
 
diff --git a/internal/mode/static/nginx/runtime/manager.go b/internal/mode/static/nginx/runtime/manager.go
@@ -121,9 +121,9 @@ func (m *ManagerImpl) Reload(ctx context.Context, configVersion int) error {
 
 	// send HUP signal to the NGINX main process reload configuration
 	// See https://nginx.org/en/docs/control.html
-	if err := m.processHandler.Kill(pid); err != nil {
+	if errP := m.processHandler.Kill(pid); errP != nil {
 		m.metricsCollector.IncReloadErrors()
-		return fmt.Errorf("failed to send the HUP signal to NGINX main: %w", err)
+		return fmt.Errorf("failed to send the HUP signal to NGINX main: %w", errP)
 	}
 
 	if err = m.verifyClient.WaitForCorrectVersion(
diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go
@@ -279,12 +279,12 @@ func buildBackendGroups(servers []VirtualServer) []BackendGroup {
 			for _, mr := range pr.MatchRules {
 				group := mr.BackendGroup
 
-				key := key{
+				k := key{
 					nsname:  group.Source,
 					ruleIdx: group.RuleIdx,
 				}
 
-				uniqueGroups[key] = group
+				uniqueGroups[k] = group
 			}
 		}
 	}
diff --git a/internal/mode/static/state/graph/backend_refs.go b/internal/mode/static/state/graph/backend_refs.go
@@ -238,12 +238,10 @@ func validateBackendTLSPolicyMatchingAllBackends(backendRefs []BackendRef) *cond
 		if referencePolicy == nil {
 			// First reference, store the policy as reference
 			referencePolicy = backendRef.BackendTLSPolicy
-		} else {
+		} else if checkPoliciesEqual(backendRef.BackendTLSPolicy.Source, referencePolicy.Source) {
 			// Check if the policies match
-			if checkPoliciesEqual(backendRef.BackendTLSPolicy.Source, referencePolicy.Source) {
-				mismatch = true
-				break
-			}
+			mismatch = true
+			break
 		}
 	}
 	if mismatch {
diff --git a/internal/mode/static/state/graph/backend_tls_policy.go b/internal/mode/static/state/graph/backend_tls_policy.go
@@ -86,23 +86,27 @@ func validateBackendTLSPolicy(
 
 	caCertRefs := backendTLSPolicy.Spec.Validation.CACertificateRefs
 	wellKnownCerts := backendTLSPolicy.Spec.Validation.WellKnownCACertificates
-	if len(caCertRefs) > 0 && wellKnownCerts != nil {
+	switch {
+	case len(caCertRefs) > 0 && wellKnownCerts != nil:
 		valid = false
 		msg := "CACertificateRefs and WellKnownCACertificates are mutually exclusive"
 		conds = append(conds, staticConds.NewPolicyInvalid(msg))
-	} else if len(caCertRefs) > 0 {
+
+	case len(caCertRefs) > 0:
 		if err := validateBackendTLSCACertRef(backendTLSPolicy, configMapResolver); err != nil {
 			valid = false
 			conds = append(conds, staticConds.NewPolicyInvalid(
 				fmt.Sprintf("invalid CACertificateRef: %s", err.Error())))
 		}
-	} else if wellKnownCerts != nil {
+
+	case wellKnownCerts != nil:
 		if err := validateBackendTLSWellKnownCACerts(backendTLSPolicy); err != nil {
 			valid = false
 			conds = append(conds, staticConds.NewPolicyInvalid(
 				fmt.Sprintf("invalid WellKnownCACertificates: %s", err.Error())))
 		}
-	} else {
+
+	default:
 		valid = false
 		conds = append(conds, staticConds.NewPolicyInvalid("CACertRefs and WellKnownCACerts are both nil"))
 	}
diff --git a/internal/mode/static/state/graph/backend_tls_policy_test.go b/internal/mode/static/state/graph/backend_tls_policy_test.go
@@ -122,7 +122,18 @@ func TestValidateBackendTLSPolicy(t *testing.T) {
 		},
 	}
 
-	localObjectRefTooManyCerts := append(localObjectRefNormalCase, localObjectRefInvalidName...)
+	localObjectRefTooManyCerts := []gatewayv1.LocalObjectReference{
+		{
+			Kind:  "ConfigMap",
+			Name:  "configmap",
+			Group: "",
+		},
+		{
+			Kind:  "ConfigMap",
+			Name:  "invalid",
+			Group: "",
+		},
+	}
 
 	getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus {
 		return v1alpha2.PolicyAncestorStatus{
diff --git a/internal/mode/static/state/graph/gateway_listener.go b/internal/mode/static/state/graph/gateway_listener.go
@@ -563,7 +563,7 @@ func GetAllowedRouteLabelSelector(l v1.Listener) *metav1.LabelSelector {
 
 // matchesWildcard checks if hostname2 matches the wildcard pattern of hostname1.
 func matchesWildcard(hostname1, hostname2 string) bool {
-	matchesWildcard := func(h1, h2 string) bool {
+	mw := func(h1, h2 string) bool {
 		if strings.HasPrefix(h1, "*.") {
 			// Remove the "*." from h1
 			h1 = h1[2:]
@@ -572,7 +572,7 @@ func matchesWildcard(hostname1, hostname2 string) bool {
 		}
 		return false
 	}
-	return matchesWildcard(hostname1, hostname2) || matchesWildcard(hostname2, hostname1)
+	return mw(hostname1, hostname2) || mw(hostname2, hostname1)
 }
 
 // haveOverlap checks for overlap between two hostnames.
diff --git a/internal/mode/static/state/graph/secret.go b/internal/mode/static/state/graph/secret.go
@@ -44,11 +44,14 @@ func (r *secretResolver) resolve(nsname types.NamespacedName) error {
 
 	var validationErr error
 
-	if !exist {
+	switch {
+	case !exist:
 		validationErr = errors.New("secret does not exist")
-	} else if secret.Type != apiv1.SecretTypeTLS {
+
+	case secret.Type != apiv1.SecretTypeTLS:
 		validationErr = fmt.Errorf("secret type must be %q not %q", apiv1.SecretTypeTLS, secret.Type)
-	} else {
+
+	default:
 		// A TLS Secret is guaranteed to have these data fields.
 		_, err := tls.X509KeyPair(secret.Data[apiv1.TLSCertKey], secret.Data[apiv1.TLSPrivateKeyKey])
 		if err != nil {
diff --git a/internal/mode/static/state/resolver/resolver.go b/internal/mode/static/state/resolver/resolver.go
@@ -155,11 +155,8 @@ func resolveEndpoints(
 // If the ServicePort has a non-zero integer TargetPort, the TargetPort integer value is returned.
 // Otherwise, the ServicePort port value is returned.
 func getDefaultPort(svcPort v1.ServicePort) int32 {
-	switch svcPort.TargetPort.Type {
-	case intstr.Int:
-		if svcPort.TargetPort.IntVal != 0 {
-			return svcPort.TargetPort.IntVal
-		}
+	if svcPort.TargetPort.Type == intstr.Int && svcPort.TargetPort.IntVal != 0 {
+		return svcPort.TargetPort.IntVal
 	}
 
 	return svcPort.Port
diff --git a/internal/mode/static/status/prepare_requests.go b/internal/mode/static/status/prepare_requests.go
@@ -68,7 +68,8 @@ func PrepareRouteRequests(
 			r.Source.GetGeneration(),
 		)
 
-		if r.RouteType == graph.RouteTypeHTTP {
+		switch r.RouteType {
+		case graph.RouteTypeHTTP:
 			status := v1.HTTPRouteStatus{
 				RouteStatus: routeStatus,
 			}
@@ -80,7 +81,8 @@ func PrepareRouteRequests(
 			}
 
 			reqs = append(reqs, req)
-		} else if r.RouteType == graph.RouteTypeGRPC {
+
+		case graph.RouteTypeGRPC:
 			status := v1.GRPCRouteStatus{
 				RouteStatus: routeStatus,
 			}
@@ -92,7 +94,8 @@ func PrepareRouteRequests(
 			}
 
 			reqs = append(reqs, req)
-		} else {
+
+		default:
 			panic(fmt.Sprintf("Unknown route type: %s", r.RouteType))
 		}
 	}
diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go
@@ -245,10 +245,10 @@ func computeRouteCount(
 
 	for _, r := range routes {
 		if r.RouteType == graph.RouteTypeHTTP {
-			httpRouteCount = httpRouteCount + 1
+			httpRouteCount++
 		}
 		if r.RouteType == graph.RouteTypeGRPC {
-			grpcRouteCount = grpcRouteCount + 1
+			grpcRouteCount++
 		}
 	}
 
diff --git a/internal/mode/static/usage/job_worker_test.go b/internal/mode/static/usage/job_worker_test.go
@@ -58,8 +58,7 @@ func TestCreateUsageJobWorker(t *testing.T) {
 				return nil
 			},
 			getCalls: func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error {
-				switch typedObject := object.(type) {
-				case *v1.Namespace:
+				if typedObject, ok := object.(*v1.Namespace); ok {
 					typedObject.Name = metav1.NamespaceSystem
 					typedObject.UID = "1234abcd"
 					return nil
@@ -83,8 +82,7 @@ func TestCreateUsageJobWorker(t *testing.T) {
 		{
 			name: "collect node count fails",
 			listCalls: func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error {
-				switch object.(type) {
-				case *v1.NodeList:
+				if _, ok := object.(*v1.NodeList); ok {
 					return errors.New("failed to collect node list")
 				}
 				return nil
@@ -127,8 +125,7 @@ func TestCreateUsageJobWorker(t *testing.T) {
 				return nil
 			},
 			getCalls: func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error {
-				switch object.(type) {
-				case *v1.Namespace:
+				if _, ok := object.(*v1.Namespace); ok {
 					return errors.New("failed to collect namespace")
 				}
 				return nil
diff --git a/tests/framework/ngf.go b/tests/framework/ngf.go
@@ -73,7 +73,7 @@ func InstallNGF(cfg InstallationConfig, extraArgs ...string) ([]byte, error) {
 	}
 
 	args = append(args, setImageArgs(cfg)...)
-	fullArgs := append(args, extraArgs...)
+	fullArgs := append(args, extraArgs...) //nolint:gocritic
 
 	GinkgoWriter.Printf("Installing NGF with command: helm %v\n", strings.Join(fullArgs, " "))
 
@@ -102,7 +102,7 @@ func UpgradeNGF(cfg InstallationConfig, extraArgs ...string) ([]byte, error) {
 	}
 
 	args = append(args, setImageArgs(cfg)...)
-	fullArgs := append(args, extraArgs...)
+	fullArgs := append(args, extraArgs...) //nolint:gocritic
 
 	GinkgoWriter.Printf("Upgrading NGF with command: helm %v\n", strings.Join(fullArgs, " "))
 
diff --git a/tests/suite/system_suite_test.go b/tests/suite/system_suite_test.go
@@ -148,11 +148,12 @@ func setup(cfg setupConfig, extraInstallArgs ...string) {
 		Skip("Graceful Recovery test must be run on Kind")
 	}
 
-	if *versionUnderTest != "" {
+	switch {
+	case *versionUnderTest != "":
 		version = *versionUnderTest
-	} else if *imageTag != "" {
+	case *imageTag != "":
 		version = *imageTag
-	} else {
+	default:
 		version = "edge"
 	}
 
@@ -203,11 +204,9 @@ func createNGFInstallConfig(cfg setupConfig, extraInstallArgs ...string) framewo
 		}
 		installCfg.ImageTag = *imageTag
 		installCfg.ImagePullPolicy = *imagePullPolicy
-	} else {
-		if version == "edge" {
-			chartVersion = "0.0.0-edge"
-			installCfg.ChartVersion = chartVersion
-		}
+	} else if version == "edge" {
+		chartVersion = "0.0.0-edge"
+		installCfg.ChartVersion = chartVersion
 	}
 
 	output, err := framework.InstallGatewayAPI(cfg.gwAPIVersion)
diff --git a/tests/suite/tracing_test.go b/tests/suite/tracing_test.go

Original file line number	Diff line number	Diff line change
`@@ -133,7 +133,7 @@ func StartManager(cfg Config) error {`
`133`	`133`	`statusUpdater,`
`134`	`134`	`mgr.GetClient(),`
`135`	`135`	`embeddedfiles.StaticModeDeploymentYAML,`
`136`		`- func() metav1.Time { return metav1.Now() },`
	`136`	`+ metav1.Now,`
`137`	`137`	`)`
`138`	`138`
`139`	`139`	`eventLoop := events.NewEventLoop(`
Original file line number	Diff line number	Diff line change
`@@ -471,8 +471,7 @@ func getGatewayAddresses(`
`471`	`471`	`}`
`472`	`472`
`473`	`473`	`var addresses, hostnames []string`
`474`		`- switch gwSvc.Spec.Type {`
`475`		`- case v1.ServiceTypeLoadBalancer:`
	`474`	`+ if gwSvc.Spec.Type == v1.ServiceTypeLoadBalancer {`
`476`	`475`	`for _, ingress := range gwSvc.Status.LoadBalancer.Ingress {`
`477`	`476`	`if ingress.IP != "" {`
`478`	`477`	`addresses = append(addresses, ingress.IP)`
Original file line number	Diff line number	Diff line change
`@@ -28,12 +28,12 @@ func ClearFolders(fileMgr ClearFoldersOSFileManager, paths []string) (removedFil`
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`for _, entry := range entries {`
`31`		`- path := filepath.Join(path, entry.Name())`
`32`		`- if err := fileMgr.Remove(path); err != nil {`
`33`		`- return removedFiles, fmt.Errorf("failed to remove %q: %w", path, err)`
	`31`	`+ entryPath := filepath.Join(path, entry.Name())`
	`32`	`+ if err := fileMgr.Remove(entryPath); err != nil {`
	`33`	`+ return removedFiles, fmt.Errorf("failed to remove %q: %w", entryPath, err)`
`34`	`34`	`}`
`35`	`35`
`36`		`- removedFiles = append(removedFiles, path)`
	`36`	`+ removedFiles = append(removedFiles, entryPath)`
`37`	`37`	`}`
`38`	`38`	`}`
`39`	`39`
Original file line number	Diff line number	Diff line change
`@@ -279,12 +279,12 @@ func buildBackendGroups(servers []VirtualServer) []BackendGroup {`
`279`	`279`	`for _, mr := range pr.MatchRules {`
`280`	`280`	`group := mr.BackendGroup`
`281`	`281`
`282`		`- key := key{`
	`282`	`+ k := key{`
`283`	`283`	`nsname: group.Source,`
`284`	`284`	`ruleIdx: group.RuleIdx,`
`285`	`285`	`}`
`286`	`286`
`287`		`- uniqueGroups[key] = group`
	`287`	`+ uniqueGroups[k] = group`
`288`	`288`	`}`
`289`	`289`	`}`
`290`	`290`	`}`