Add gocritic linter (#1344)

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>

.golangci.yaml

@@ -2,6 +2,9 @@ run:
   timeout: 5m
 
 linters-settings:
+  gocritic:
+    enabled-tags:
+    - performance
   gosec:
     excludes:
     - G108
@@ -17,19 +20,20 @@ linters-settings:
 linters:
   disable-all: true
   enable:
+    - deadcode
     - errcheck
-    - govet
     - ineffassign
-    - revive # replacement for golint
+    - gocritic
     - goconst
     - gofmt
     - goimports
-    - unused
-    - varcheck
-    - deadcode
+    - gosec
+    - gosimple
+    - govet
     - misspell
-    - typecheck
-    - structcheck
+    - revive # replacement for golint
     - staticcheck
-    - gosimple
-    - gosec
+    - structcheck
+    - typecheck
+    - unused
+    - varcheck

apis/status/v1beta1/util.go

@@ -78,7 +78,7 @@ func dashPacker(vals ...string) (string, error) {
 		if i != 0 {
 			b.WriteString("-")
 		}
-		b.WriteString(strings.Replace(val, "-", "--", -1))
+		b.WriteString(strings.ReplaceAll(val, "-", "--"))
 	}
 	return b.String(), nil
 }

main.go

@@ -325,8 +325,8 @@ func setLoggerForProduction(encoder zapcore.LevelEncoder) {
 	opts = append(opts, zap.AddStacktrace(zap.ErrorLevel),
 		zap.WrapCore(func(core zapcore.Core) zapcore.Core {
 			return zapcore.NewSamplerWithOptions(core, time.Second, 100, 100)
-		}))
-	opts = append(opts, zap.AddCallerSkip(1), zap.ErrorOutput(sink))
+		}),
+		zap.AddCallerSkip(1), zap.ErrorOutput(sink))
 	zlog := zap.New(zapcore.NewCore(&crzap.KubeAwareEncoder{Encoder: enc, Verbose: false}, sink, lvl))
 	zlog = zlog.WithOptions(opts...)
 	newlogger := zapr.NewLogger(zlog)

pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller_test.go

@@ -94,10 +94,10 @@ violation[{"msg": "denied!"}] {
 	}
 
 	// Uncommenting the below enables logging of K8s internals like watch.
-	//fs := flag.NewFlagSet("", flag.PanicOnError)
-	//klog.InitFlags(fs)
-	//fs.Parse([]string{"--alsologtostderr", "-v=10"})
-	//klog.SetOutput(os.Stderr)
+	// fs := flag.NewFlagSet("", flag.PanicOnError)
+	// klog.InitFlags(fs)
+	// fs.Parse([]string{"--alsologtostderr", "-v=10"})
+	// klog.SetOutput(os.Stderr)
 
 	// Setup the Manager and Controller.  Wrap the Controller Reconcile function so it writes each request to a
 	// channel when it is finished.

pkg/mutation/match/match.go

@@ -47,7 +47,7 @@ type Kinds struct {
 
 // Matches verifies if the given object belonging to the given namespace
 // matches the current mutator.
-func Matches(match Match, obj runtime.Object, ns *corev1.Namespace) (bool, error) {
+func Matches(match *Match, obj runtime.Object, ns *corev1.Namespace) (bool, error) {
 	meta, err := meta.Accessor(obj)
 	if err != nil {
 		return false, fmt.Errorf("accessor failed for %s", obj.GetObjectKind().GroupVersionKind().Kind)

pkg/mutation/match/match_test.go

@@ -354,7 +354,7 @@ func TestMatch(t *testing.T) {
 			}
 			// namespace is not populated in the object metadata for mutation requests
 			tc.toMatch.SetNamespace("")
-			matches, err := Matches(tc.match, tc.toMatch, ns)
+			matches, err := Matches(&tc.match, tc.toMatch, ns)
 			if err != nil {
 				t.Error("Match failed for ", tc.tname)
 			}

pkg/mutation/mutators/assign_mutator.go

@@ -43,7 +43,7 @@ func (m *AssignMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
 	if !match.AppliesTo(m.assign.Spec.ApplyTo, obj) {
 		return false
 	}
-	matches, err := match.Matches(m.assign.Spec.Match, obj, ns)
+	matches, err := match.Matches(&m.assign.Spec.Match, obj, ns)
 	if err != nil {
 		log.Error(err, "AssignMutator.Matches failed", "assign", m.assign.Name)
 		return false

pkg/mutation/mutators/assignmeta_mutator.go

@@ -38,7 +38,7 @@ var (
 	}
 )
 
-//AssignMetadataMutator is a mutator built out of an
+// AssignMetadataMutator is a mutator built out of an
 // AssignMeta instance.
 type AssignMetadataMutator struct {
 	id             types.ID
@@ -50,7 +50,7 @@ type AssignMetadataMutator struct {
 var _ types.Mutator = &AssignMetadataMutator{}
 
 func (m *AssignMetadataMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
-	matches, err := match.Matches(m.assignMetadata.Spec.Match, obj, ns)
+	matches, err := match.Matches(&m.assignMetadata.Spec.Match, obj, ns)
 	if err != nil {
 		log.Error(err, "AssignMetadataMutator.Matches failed", "assignMeta", m.assignMetadata.Name)
 		return false

pkg/mutation/mutators/core/mutation_function.go

@@ -110,20 +110,18 @@ func (s *mutatorState) mutateInternal(current interface{}, depth int) (bool, int
 				}
 				mutated = mutated || m
 				elementFound = true
-			} else {
-				if listElementAsObject, ok := listElement.(map[string]interface{}); ok {
-					if elementValue, ok := listElementAsObject[key]; ok {
-						if *castPathEntry.KeyValue == elementValue {
-							if !s.tester.ExistsOkay(depth) {
-								return false, nil, nil
-							}
-							m, _, err := s.mutateInternal(listElement, depth+1)
-							if err != nil {
-								return false, nil, err
-							}
-							mutated = mutated || m
-							elementFound = true
+			} else if listElementAsObject, ok := listElement.(map[string]interface{}); ok {
+				if elementValue, ok := listElementAsObject[key]; ok {
+					if *castPathEntry.KeyValue == elementValue {
+						if !s.tester.ExistsOkay(depth) {
+							return false, nil, nil
 						}
+						m, _, err := s.mutateInternal(listElement, depth+1)
+						if err != nil {
+							return false, nil, err
+						}
+						mutated = mutated || m
+						elementFound = true
 					}
 				}
 			}

pkg/mutation/mutators/core/mutation_function_test.go

@@ -309,10 +309,8 @@ func TestListsAsLastElementAlreadyExistsWithKeyConflict(t *testing.T) {
 		t,
 	); err == nil {
 		t.Errorf("Expected error not raised. Conflicting name must not be applied.")
-	} else {
-		if err.Error() != "key value of replaced object must not change" {
-			t.Errorf("Incorrect error message: %s", err.Error())
-		}
+	} else if err.Error() != "key value of replaced object must not change" {
+		t.Errorf("Incorrect error message: %s", err.Error())
 	}
 }
 

pkg/mutation/mutators/testhelpers/dummy_mutator.go

@@ -44,7 +44,7 @@ func (d *DummyMutator) Path() parser.Path {
 }
 
 func (d *DummyMutator) Matches(obj runtime.Object, ns *corev1.Namespace) bool {
-	matches, err := match.Matches(d.match, obj, ns)
+	matches, err := match.Matches(&d.match, obj, ns)
 	if err != nil {
 		return false
 	}

pkg/mutation/system.go

@@ -178,14 +178,15 @@ func logAppliedMutations(message string, mutationUUID uuid.UUID, obj *unstructur
 		iterations = append(iterations, fmt.Sprintf("iteration_%d", i), strings.Join(appliedMutationsText, ", "))
 	}
 	if len(iterations) > 0 {
-		logDetails := []interface{}{}
-		logDetails = append(logDetails, "Mutation Id", mutationUUID)
-		logDetails = append(logDetails, logging.EventType, logging.MutationApplied)
-		logDetails = append(logDetails, logging.ResourceGroup, obj.GroupVersionKind().Group)
-		logDetails = append(logDetails, logging.ResourceKind, obj.GroupVersionKind().Kind)
-		logDetails = append(logDetails, logging.ResourceAPIVersion, obj.GroupVersionKind().Version)
-		logDetails = append(logDetails, logging.ResourceNamespace, obj.GetNamespace())
-		logDetails = append(logDetails, logging.ResourceName, obj.GetName())
+		logDetails := []interface{}{
+			"Mutation Id", mutationUUID,
+			logging.EventType, logging.MutationApplied,
+			logging.ResourceGroup, obj.GroupVersionKind().Group,
+			logging.ResourceKind, obj.GroupVersionKind().Kind,
+			logging.ResourceAPIVersion, obj.GroupVersionKind().Version,
+			logging.ResourceNamespace, obj.GetNamespace(),
+			logging.ResourceName, obj.GetName(),
+		}
 		logDetails = append(logDetails, iterations...)
 		log.Info(message, logDetails...)
 	}

pkg/readiness/object_tracker.go

@@ -118,6 +118,7 @@ func (t *objectTracker) Expect(o runtime.Object) {
 	t.expect[k] = struct{}{}
 }
 
+// nolint: gocritic // Using a pointer here is less efficient and results in more copying.
 func (t *objectTracker) cancelExpectNoLock(k objKey) {
 	delete(t.expect, k)
 	delete(t.seen, k)

pkg/readiness/objset.go

@@ -27,7 +27,7 @@ type objKey struct {
 	namespacedName types.NamespacedName
 }
 
-func (k objKey) String() string {
+func (k *objKey) String() string {
 	return fmt.Sprintf("%s [%s]", k.namespacedName.String(), k.gvk.String())
 }
 

pkg/readiness/ready_tracker_test.go

@@ -272,10 +272,10 @@ func Test_Tracker(t *testing.T) {
 		g.Expect(err).NotTo(gomega.HaveOccurred(), "checking cache for constraint")
 	}
 	// TODO: Verify data if we add the corresponding API to opa.Client.
-	//for _, d := range testData {
-	//	_, err := opaClient.GetData(ctx, c)
-	//	g.Expect(err).NotTo(gomega.HaveOccurred(), "checking cache for constraint")
-	//}
+	// for _, d := range testData {
+	// 	_, err := opaClient.GetData(ctx, c)
+	// 	g.Expect(err).NotTo(gomega.HaveOccurred(), "checking cache for constraint")
+	// }
 
 	// Add additional templates/constraints and verify that we remain satisfied
 	err = applyFixtures("testdata/post")

pkg/readiness/ready_tracker_unit_test.go

@@ -55,8 +55,7 @@ var testConstraintTemplate = templates.ConstraintTemplate{
 }
 
 func (dl dummyLister) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
-	switch l := list.(type) {
-	case *v1beta1.ConstraintTemplateList:
+	if l, ok := list.(*v1beta1.ConstraintTemplateList); ok {
 		i := v1beta1.ConstraintTemplate{}
 		if err := scheme.Convert(&testConstraintTemplate, &i, nil); err != nil {
 			// These failures will be swallowed by readiness.retryAll

pkg/target/target_integration_test.go

@@ -419,7 +419,7 @@ func TestConstraintEnforcement(t *testing.T) {
 				t.Errorf("allowed = %v, expected %v:\n%s\n\n%s", !tc.allowed, tc.allowed, res.TraceDump(), dump)
 			}
 
-			//also test oldObject
+			// also test oldObject
 			req2 := &admissionv1.AdmissionRequest{
 				Kind: metav1.GroupVersionKind{
 					Group:   tc.obj.GroupVersionKind().Group,

pkg/webhook/mutation.go

@@ -101,6 +101,7 @@ type mutationHandler struct {
 }
 
 // Handle the mutation request
+// nolint: gocritic // Must accept admission.Request to satisfy interface.
 func (h *mutationHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
 	log := log.WithValues("hookType", "mutation")
 	var timeStart = time.Now()
@@ -153,7 +154,8 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ
 	ns := &corev1.Namespace{}
 
 	// if the object being mutated is a namespace itself, we use it as namespace
-	if req.Kind.Kind == namespaceKind && req.Kind.Group == "" {
+	switch {
+	case req.Kind.Kind == namespaceKind && req.Kind.Group == "":
 		req.Namespace = ""
 		obj, _, err := deserializer.Decode(req.Object.Raw, nil, &corev1.Namespace{})
 		if err != nil {
@@ -164,7 +166,7 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ
 		if !ok {
 			return admission.Errored(int32(http.StatusInternalServerError), errors.New("failed to cast namespace object")), nil
 		}
-	} else if req.AdmissionRequest.Namespace != "" {
+	case req.AdmissionRequest.Namespace != "":
 		if err := h.client.Get(ctx, types.NamespacedName{Name: req.AdmissionRequest.Namespace}, ns); err != nil {
 			if !k8serrors.IsNotFound(err) {
 				log.Error(err, "error retrieving namespace", "name", req.AdmissionRequest.Namespace)
@@ -177,7 +179,7 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ
 				return admission.Errored(int32(http.StatusInternalServerError), err), nil
 			}
 		}
-	} else {
+	default:
 		ns = nil
 	}
 	obj := unstructured.Unstructured{}

pkg/webhook/namespacelabel.go

@@ -50,6 +50,7 @@ var _ admission.Handler = &namespaceLabelHandler{}
 
 type namespaceLabelHandler struct{}
 
+// nolint: gocritic // Must accept admission.Request as a struct to satisfy Handler interface.
 func (h *namespaceLabelHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
 	if req.Operation == admissionv1.Delete {
 		return admission.Allowed("Delete is always allowed")

pkg/webhook/policy.go

@@ -114,6 +114,7 @@ type validationHandler struct {
 }
 
 // Handle the validation request
+// nolint: gocritic // Must accept admission.Request as a struct to satisfy Handler interface.
 func (h *validationHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
 	log := log.WithValues("hookType", "validation")
 

pkg/webhook/policy_benchmark_test.go

@@ -172,10 +172,10 @@ func addConstraints(opa *opa.Client, list []unstructured.Unstructured) error {
 	return nil
 }
 
-// generateConstraints generates M constraints based on representative constraint in crList
-func generateConstraints(M int, crList []unstructured.Unstructured) []unstructured.Unstructured {
-	result := make([]unstructured.Unstructured, M)
-	for i := 0; i < M; i++ {
+// generateConstraints generates m constraints based on representative constraint in crList
+func generateConstraints(m int, crList []unstructured.Unstructured) []unstructured.Unstructured {
+	result := make([]unstructured.Unstructured, m)
+	for i := 0; i < m; i++ {
 		r := crList[i%len(crList)]
 		result[i] = *(r.DeepCopy())
 		r.SetName(genRandString(10))

pkg/webhook/stats_reporter.go

@@ -83,7 +83,6 @@ func (r *reporter) ReportMutationRequest(response requestResponse, d time.Durati
 
 // Captures req count metric, recording the count and the duration
 func (r *reporter) reportRequest(response requestResponse, statusKey tag.Key, m stats.Measurement) error {
-	//mutationResponseTimeInSecM.M(d.Seconds())
 	ctx, err := tag.New(
 		r.ctx,
 		tag.Insert(statusKey, string(response)),