golangci-lint: Fix revive rules (#5857)

When we enabled the use-any rule we disabled all the default rules that
are run by revive (see: https://revive.run/docs#golangci-lint)

This change grabs all the default rules from
https://github.com/mgechev/revive/blob/master/defaults.toml and adds the
use-any rule

Also fixes outstanding lint issues

Signed-off-by: Sunjay Bhatia <[email protected]>

.golangci.yml

@@ -32,7 +32,30 @@ linters-settings:
       - http.DefaultTransport
   revive:
     rules:
-      - name: use-any
+    - name: blank-imports
+    - name: context-as-argument
+    - name: context-keys-type
+    - name: dot-imports
+    - name: empty-block
+    - name: error-naming
+    - name: error-return
+    - name: error-strings
+    - name: errorf
+    - name: exported
+    - name: increment-decrement
+    - name: indent-error-flow
+    - name: package-comments
+    - name: range
+    - name: receiver-naming
+    - name: redefines-builtin-id
+    - name: superfluous-else
+    - name: time-naming
+    - name: unexported-return
+    - name: unreachable-code
+    - name: unused-parameter
+    - name: use-any
+    - name: var-declaration
+    - name: var-naming
 
 issues:
   exclude-rules:

cmd/contour/shutdownmanager.go

@@ -98,7 +98,7 @@ func newShutdownContext() *shutdownContext {
 }
 
 // healthzHandler handles the /healthz endpoint which is used for the shutdown-manager's liveness probe.
-func (s *shutdownmanagerContext) healthzHandler(w http.ResponseWriter, r *http.Request) {
+func (s *shutdownmanagerContext) healthzHandler(w http.ResponseWriter, _ *http.Request) {
 	if _, err := w.Write([]byte(http.StatusText(http.StatusOK))); err != nil {
 		s.WithField("context", "healthzHandler").Error(err)
 	}

internal/contour/handler.go

@@ -233,22 +233,22 @@ func (e *EventHandler) onUpdate(op any) bool {
 	case opAdd:
 		return e.builder.Source.Insert(op.obj)
 	case opUpdate:
-		old, oldOk := op.oldObj.(client.Object)
-		new, newOk := op.newObj.(client.Object)
+		oldO, oldOk := op.oldObj.(client.Object)
+		newO, newOk := op.newObj.(client.Object)
 		if oldOk && newOk {
-			equal, err := k8s.IsObjectEqual(old, new)
+			equal, err := k8s.IsObjectEqual(oldO, newO)
 			// Error is returned if there was no support for comparing equality of the specific object type.
 			// We can still process the object but it will be always considered as changed.
 			if err != nil {
 				e.WithError(err).WithField("op", "update").
-					WithField("name", new.GetName()).WithField("namespace", new.GetNamespace()).
-					WithField("gvk", reflect.TypeOf(new)).Errorf("error comparing objects")
+					WithField("name", newO.GetName()).WithField("namespace", newO.GetNamespace()).
+					WithField("gvk", reflect.TypeOf(newO)).Errorf("error comparing objects")
 			}
 			if equal {
 				// log the object name and namespace to help with debugging.
 				e.WithField("op", "update").
-					WithField("name", old.GetName()).WithField("namespace", old.GetNamespace()).
-					WithField("gvk", reflect.TypeOf(new)).Debugf("skipping update, no changes to relevant fields")
+					WithField("name", oldO.GetName()).WithField("namespace", oldO.GetNamespace()).
+					WithField("gvk", reflect.TypeOf(newO)).Debugf("skipping update, no changes to relevant fields")
 				return false
 			}
 			remove := e.builder.Source.Remove(op.oldObj)

internal/controller/gateway.go

@@ -188,7 +188,7 @@ func (r *gatewayReconciler) Reconcile(ctx context.Context, request reconcile.Req
 	r.log.WithField("namespace", request.Namespace).WithField("name", request.Name).Info("reconciling gateway")
 
 	var gatewayClasses gatewayapi_v1beta1.GatewayClassList
-	if err := r.client.List(context.Background(), &gatewayClasses); err != nil {
+	if err := r.client.List(ctx, &gatewayClasses); err != nil {
 		return reconcile.Result{}, fmt.Errorf("error listing gateway classes")
 	}
 
@@ -219,7 +219,7 @@ func (r *gatewayReconciler) Reconcile(ctx context.Context, request reconcile.Req
 	}
 
 	var allGateways gatewayapi_v1beta1.GatewayList
-	if err := r.client.List(context.Background(), &allGateways); err != nil {
+	if err := r.client.List(ctx, &allGateways); err != nil {
 		return reconcile.Result{}, fmt.Errorf("error listing gateways")
 	}
 
@@ -280,8 +280,8 @@ func (r *gatewayReconciler) Reconcile(ctx context.Context, request reconcile.Req
 			})
 		} else {
 			// this branch makes testing easier by not going through the StatusUpdater.
-			copy := setGatewayNotAccepted(gw.DeepCopy())
-			if err := r.client.Status().Update(context.Background(), copy); err != nil {
+			gwCopy := setGatewayNotAccepted(gw.DeepCopy())
+			if err := r.client.Status().Update(ctx, gwCopy); err != nil {
 				r.log.WithError(err).Error("error updating gateway status")
 				return reconcile.Result{}, fmt.Errorf("error updating status of gateway %s/%s: %v", gw.Namespace, gw.Name, err)
 			}

internal/controller/gatewayclass.go

@@ -136,7 +136,7 @@ func (r *gatewayClassReconciler) Reconcile(ctx context.Context, request reconcil
 	r.log.WithField("name", request.Name).Info("reconciling gatewayclass")
 
 	var gatewayClasses gatewayapi_v1beta1.GatewayClassList
-	if err := r.client.List(context.Background(), &gatewayClasses); err != nil {
+	if err := r.client.List(ctx, &gatewayClasses); err != nil {
 		return reconcile.Result{}, fmt.Errorf("error listing gatewayclasses: %w", err)
 	}
 
@@ -177,15 +177,15 @@ func (r *gatewayClassReconciler) Reconcile(ctx context.Context, request reconcil
 						panic(fmt.Sprintf("unsupported object type %T", obj))
 					}
 
-					return status.SetGatewayClassAccepted(context.Background(), r.client, gwc.DeepCopy(), accepted)
+					return status.SetGatewayClassAccepted(gwc.DeepCopy(), accepted)
 				}),
 			})
 		} else {
 			// this branch makes testing easier by not going through the StatusUpdater.
-			copy := status.SetGatewayClassAccepted(context.Background(), r.client, gc.DeepCopy(), accepted)
+			gcCopy := status.SetGatewayClassAccepted(gc.DeepCopy(), accepted)
 
-			if err := r.client.Status().Update(context.Background(), copy); err != nil {
-				return fmt.Errorf("error updating status of gateway class %s: %v", copy.Name, err)
+			if err := r.client.Status().Update(ctx, gcCopy); err != nil {
+				return fmt.Errorf("error updating status of gateway class %s: %v", gcCopy.Name, err)
 			}
 		}
 		return nil

internal/dag/cache.go

@@ -671,9 +671,8 @@ func (kc *KubernetesCache) LookupUpstreamValidation(uv *contour_api_v1.UpstreamV
 	if err != nil {
 		if _, ok := err.(DelegationNotPermittedError); ok {
 			return nil, err
-		} else {
-			return nil, fmt.Errorf("invalid CA Secret %q: %s", caCertificate, err)
 		}
+		return nil, fmt.Errorf("invalid CA Secret %q: %s", caCertificate, err)
 	}
 
 	if uv.SubjectName == "" {

internal/dag/cache_test.go

@@ -1146,11 +1146,11 @@ func TestKubernetesCacheInsert(t *testing.T) {
 // correctly.
 type fakeReader struct{}
 
-func (r *fakeReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+func (r *fakeReader) Get(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error {
 	return errors.New("not implemented")
 }
 
-func (r *fakeReader) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+func (r *fakeReader) List(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error {
 	panic("not implemented")
 }
 

internal/dag/policy.go

@@ -185,10 +185,9 @@ func headersPolicyRoute(policy *contour_api_v1.HeadersPolicy, allowHostRewrite b
 			if extractedHostRewriteHeader := extractHostRewriteHeaderValue(entry.Value); extractedHostRewriteHeader != "" {
 				hostRewriteHeader = http.CanonicalHeaderKey(extractedHostRewriteHeader)
 				continue
-			} else {
-				hostRewrite = entry.Value
-				continue
 			}
+			hostRewrite = entry.Value
+			continue
 		}
 		if msgs := validation.IsHTTPHeaderName(key); len(msgs) != 0 {
 			return nil, fmt.Errorf("invalid set header %q: %v", key, msgs)

internal/envoy/route.go

@@ -71,18 +71,16 @@ func SingleSimpleCluster(route *dag.Route) bool {
 	// If the target cluster performs any kind of header manipulation,
 	// then we should use a WeightedCluster to encode the additional
 	// configuration.
-	if cluster.RequestHeadersPolicy == nil {
-		// no request headers policy
-	} else if len(cluster.RequestHeadersPolicy.Set) != 0 ||
-		len(cluster.RequestHeadersPolicy.Add) != 0 ||
-		len(cluster.RequestHeadersPolicy.Remove) != 0 ||
-		len(cluster.RequestHeadersPolicy.HostRewrite) != 0 {
+	if cluster.RequestHeadersPolicy != nil &&
+		(len(cluster.RequestHeadersPolicy.Set) != 0 ||
+			len(cluster.RequestHeadersPolicy.Add) != 0 ||
+			len(cluster.RequestHeadersPolicy.Remove) != 0 ||
+			len(cluster.RequestHeadersPolicy.HostRewrite) != 0) {
 		return false
 	}
-	if cluster.ResponseHeadersPolicy == nil {
-		// no response headers policy
-	} else if len(cluster.ResponseHeadersPolicy.Set) != 0 ||
-		len(cluster.ResponseHeadersPolicy.Remove) != 0 {
+	if cluster.ResponseHeadersPolicy != nil &&
+		(len(cluster.ResponseHeadersPolicy.Set) != 0 ||
+			len(cluster.ResponseHeadersPolicy.Remove) != 0) {
 		return false
 	}
 	if len(cluster.CookieRewritePolicies) > 0 {

internal/envoy/v3/bootstrap.go

@@ -241,7 +241,7 @@ func bootstrapConfig(c *envoy.BootstrapConfig) *envoy_bootstrap_v3.Bootstrap {
 				LoadAssignment: &envoy_endpoint_v3.ClusterLoadAssignment{
 					ClusterName: "envoy-admin",
 					Endpoints: Endpoints(
-						UnixSocketAddress(c.GetAdminAddress(), c.GetAdminPort()),
+						UnixSocketAddress(c.GetAdminAddress()),
 					),
 				},
 			}},
@@ -252,7 +252,7 @@ func bootstrapConfig(c *envoy.BootstrapConfig) *envoy_bootstrap_v3.Bootstrap {
 		},
 		Admin: &envoy_bootstrap_v3.Admin{
 			AccessLog: adminAccessLog(c.GetAdminAccessLogPath()),
-			Address:   UnixSocketAddress(c.GetAdminAddress(), c.GetAdminPort()),
+			Address:   UnixSocketAddress(c.GetAdminAddress()),
 		},
 	}
 	if c.MaximumHeapSizeBytes > 0 {

internal/envoy/v3/listener.go

@@ -660,7 +660,7 @@ func TCPProxy(statPrefix string, proxy *dag.TCPProxy, accesslogger []*accesslog.
 }
 
 // UnixSocketAddress creates a new Unix Socket envoy_core_v3.Address.
-func UnixSocketAddress(address string, port int) *envoy_core_v3.Address {
+func UnixSocketAddress(address string) *envoy_core_v3.Address {
 	return &envoy_core_v3.Address{
 		Address: &envoy_core_v3.Address_Pipe{
 			Pipe: &envoy_core_v3.Pipe{

internal/k8s/helpers.go

@@ -94,22 +94,22 @@ func isStatusEqual(objA, objB any) bool {
 //
 // Make an attempt to avoid comparing full objects since it can be very CPU intensive.
 // Prefer comparing Generation when only interested in spec changes.
-func IsObjectEqual(old, new client.Object) (bool, error) {
+func IsObjectEqual(oldObj, newObj client.Object) (bool, error) {
 
 	// Fast path for any object: when ResourceVersions are equal, the objects are equal.
 	// NOTE: This optimizes the case when controller-runtime executes full sync and sends updates for all objects.
-	if isResourceVersionEqual(old, new) {
+	if isResourceVersionEqual(oldObj, newObj) {
 		return true, nil
 	}
 
-	switch old := old.(type) {
+	switch oldObj := oldObj.(type) {
 
 	// Fast path for objects that implement Generation and where only spec changes matter.
 	// Status/annotations/labels changes are ignored.
 	// Generation is implemented in CRDs, Ingress and IngressClass.
 	case *contour_api_v1alpha1.ExtensionService,
 		*contour_api_v1.TLSCertificateDelegation:
-		return isGenerationEqual(old, new), nil
+		return isGenerationEqual(oldObj, newObj), nil
 
 	case *gatewayapi_v1beta1.GatewayClass,
 		*gatewayapi_v1beta1.Gateway,
@@ -118,36 +118,36 @@ func IsObjectEqual(old, new client.Object) (bool, error) {
 		*gatewayapi_v1alpha2.TLSRoute,
 		*gatewayapi_v1alpha2.GRPCRoute,
 		*gatewayapi_v1alpha2.TCPRoute:
-		return isGenerationEqual(old, new), nil
+		return isGenerationEqual(oldObj, newObj), nil
 
 	// Slow path: compare the content of the objects.
 	case *contour_api_v1.HTTPProxy,
 		*networking_v1.Ingress:
-		return isGenerationEqual(old, new) &&
-			apiequality.Semantic.DeepEqual(old.GetAnnotations(), new.GetAnnotations()), nil
+		return isGenerationEqual(oldObj, newObj) &&
+			apiequality.Semantic.DeepEqual(oldObj.GetAnnotations(), newObj.GetAnnotations()), nil
 	case *v1.Secret:
-		if new, ok := new.(*v1.Secret); ok {
-			return reflect.DeepEqual(old.Data, new.Data), nil
+		if newObj, ok := newObj.(*v1.Secret); ok {
+			return reflect.DeepEqual(oldObj.Data, newObj.Data), nil
 		}
 	case *v1.Service:
-		if new, ok := new.(*v1.Service); ok {
-			return apiequality.Semantic.DeepEqual(old.Spec, new.Spec) &&
-				apiequality.Semantic.DeepEqual(old.Status, new.Status) &&
-				apiequality.Semantic.DeepEqual(old.GetAnnotations(), new.GetAnnotations()), nil
+		if newObj, ok := newObj.(*v1.Service); ok {
+			return apiequality.Semantic.DeepEqual(oldObj.Spec, newObj.Spec) &&
+				apiequality.Semantic.DeepEqual(oldObj.Status, newObj.Status) &&
+				apiequality.Semantic.DeepEqual(oldObj.GetAnnotations(), newObj.GetAnnotations()), nil
 		}
 	case *v1.Endpoints:
-		if new, ok := new.(*v1.Endpoints); ok {
-			return apiequality.Semantic.DeepEqual(old.Subsets, new.Subsets), nil
+		if newObj, ok := newObj.(*v1.Endpoints); ok {
+			return apiequality.Semantic.DeepEqual(oldObj.Subsets, newObj.Subsets), nil
 		}
 	case *v1.Namespace:
-		if new, ok := new.(*v1.Namespace); ok {
-			return apiequality.Semantic.DeepEqual(old.Labels, new.Labels), nil
+		if newObj, ok := newObj.(*v1.Namespace); ok {
+			return apiequality.Semantic.DeepEqual(oldObj.Labels, newObj.Labels), nil
 		}
 	}
 
 	// ResourceVersions are not equal and we don't know how to compare the object type.
 	// This should never happen and indicates that new type was added to the code but is missing in the switch above.
-	return false, fmt.Errorf("do not know how to compare %T", new)
+	return false, fmt.Errorf("do not know how to compare %T", newObj)
 }
 
 func isGenerationEqual(a, b client.Object) bool {