release-1.26: golangci-lint: Fix revive rules (#5858)

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 includes lint fixes for outstanding 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

@@ -82,7 +82,7 @@ type opDelete struct {
 	obj any
 }
 
-func (e *EventHandler) OnAdd(obj any, isInInitialList bool) {
+func (e *EventHandler) OnAdd(obj any, _ bool) {
 	e.update <- opAdd{obj: obj}
 }
 
@@ -184,22 +184,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/envoy/route.go

@@ -64,18 +64,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 {

internal/k8s/helpers_test.go

@@ -96,20 +96,20 @@ func TestIsObjectEqual(t *testing.T) {
 			assert.Equal(t, 2, len(objects), "expected 2 objects in file")
 
 			// Decode the objects.
-			old, _, err := deserializer.Decode([]byte(objects[0]), nil, nil)
+			oldObj, _, err := deserializer.Decode([]byte(objects[0]), nil, nil)
 			assert.NoError(t, err)
-			new, _, err := deserializer.Decode([]byte(objects[1]), nil, nil)
+			newObj, _, err := deserializer.Decode([]byte(objects[1]), nil, nil)
 			assert.NoError(t, err)
 
-			got, err := IsObjectEqual(old.(client.Object), new.(client.Object))
+			got, err := IsObjectEqual(oldObj.(client.Object), newObj.(client.Object))
 			assert.NoError(t, err)
 			assert.Equal(t, tc.equals, got)
 		})
 	}
 }
 
 func TestIsEqualForResourceVersion(t *testing.T) {
-	old := &v1.Secret{
+	oldS := &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            "test",
 			Namespace:       "default",
@@ -120,23 +120,23 @@ func TestIsEqualForResourceVersion(t *testing.T) {
 		},
 	}
 
-	new := old.DeepCopy()
+	newS := oldS.DeepCopy()
 
 	// Objects with equal ResourceVersion should evaluate to true.
-	got, err := IsObjectEqual(old, new)
+	got, err := IsObjectEqual(oldS, newS)
 	assert.NoError(t, err)
 	assert.True(t, got)
 
 	// Differences in data should be ignored.
-	new.Data["foo"] = []byte("baz")
-	got, err = IsObjectEqual(old, new)
+	newS.Data["foo"] = []byte("baz")
+	got, err = IsObjectEqual(oldS, newS)
 	assert.NoError(t, err)
 	assert.True(t, got)
 }
 
 // TestIsEqualFallback compares with ConfigMap objects, which are not supported.
 func TestIsEqualFallback(t *testing.T) {
-	old := &v1.ConfigMap{
+	oldObj := &v1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:            "test",
 			Namespace:       "default",
@@ -147,37 +147,37 @@ func TestIsEqualFallback(t *testing.T) {
 		},
 	}
 
-	new := old.DeepCopy()
+	newObj := oldObj.DeepCopy()
 
 	// Any object (even unsupported types) with equal ResourceVersion should evaluate to true.
-	got, err := IsObjectEqual(old, new)
+	got, err := IsObjectEqual(oldObj, newObj)
 	assert.NoError(t, err)
 	assert.True(t, got)
 
 	// Unsupported types with unequal ResourceVersion should return an error.
-	new.ResourceVersion = "456"
-	got, err = IsObjectEqual(old, new)
+	newObj.ResourceVersion = "456"
+	got, err = IsObjectEqual(oldObj, newObj)
 	assert.Error(t, err)
 	assert.False(t, got)
 }
 
 func TestIsEqualForGeneration(t *testing.T) {
-	run := func(t *testing.T, old client.Object) {
+	run := func(t *testing.T, oldObj client.Object) {
 		t.Helper()
-		new := old.DeepCopyObject().(client.Object)
+		newObj := oldObj.DeepCopyObject().(client.Object)
 
 		// Set different ResourceVersion to ensure that Generation is the only difference.
-		old.SetResourceVersion("123")
-		new.SetResourceVersion("456")
+		oldObj.SetResourceVersion("123")
+		newObj.SetResourceVersion("456")
 
 		// Objects with equal Generation should evaluate to true.
-		got, err := IsObjectEqual(old, new)
+		got, err := IsObjectEqual(oldObj, newObj)
 		assert.NoError(t, err)
 		assert.True(t, got)
 
 		// Objects with unequal Generation should evaluate to false.
-		new.SetGeneration(old.GetGeneration() + 1)
-		got, err = IsObjectEqual(old, new)
+		newObj.SetGeneration(oldObj.GetGeneration() + 1)
+		got, err = IsObjectEqual(oldObj, newObj)
 		assert.NoError(t, err)
 		assert.False(t, got)
 	}