From bf94eb335918f8806571e28d01fb1c26a9179b2d Mon Sep 17 00:00:00 2001 From: Will Beason Date: Thu, 10 Jun 2021 12:36:17 -0500 Subject: [PATCH] Add gocritic linter (#1344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> --- .golangci.yaml | 22 ++++++++++------- apis/status/v1beta1/util.go | 2 +- main.go | 4 ++-- ...onstrainttemplatestatus_controller_test.go | 8 +++---- pkg/mutation/match/match.go | 2 +- pkg/mutation/match/match_test.go | 2 +- pkg/mutation/mutators/assign_mutator.go | 2 +- pkg/mutation/mutators/assignmeta_mutator.go | 4 ++-- .../mutators/core/mutation_function.go | 24 +++++++++---------- .../mutators/core/mutation_function_test.go | 6 ++--- .../mutators/testhelpers/dummy_mutator.go | 2 +- pkg/mutation/system.go | 17 ++++++------- pkg/readiness/object_tracker.go | 1 + pkg/readiness/objset.go | 2 +- pkg/readiness/ready_tracker_test.go | 8 +++---- pkg/readiness/ready_tracker_unit_test.go | 3 +-- pkg/target/target_integration_test.go | 2 +- pkg/webhook/mutation.go | 8 ++++--- pkg/webhook/namespacelabel.go | 1 + pkg/webhook/policy.go | 1 + pkg/webhook/policy_benchmark_test.go | 8 +++---- pkg/webhook/stats_reporter.go | 1 - 22 files changed, 67 insertions(+), 63 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 53681d13faa..0c613a22763 100644 --- a/.golangci.yaml +++ b/.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 diff --git a/apis/status/v1beta1/util.go b/apis/status/v1beta1/util.go index bc510ee9f37..5f4964190f9 100644 --- a/apis/status/v1beta1/util.go +++ b/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 } diff --git a/main.go b/main.go index a4d8538c310..04816b7adfa 100644 --- a/main.go +++ b/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) diff --git a/pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller_test.go b/pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller_test.go index 5799edb8290..db9ce8ef558 100644 --- a/pkg/controller/constrainttemplatestatus/constrainttemplatestatus_controller_test.go +++ b/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. diff --git a/pkg/mutation/match/match.go b/pkg/mutation/match/match.go index 5331d437227..3be08611477 100644 --- a/pkg/mutation/match/match.go +++ b/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) diff --git a/pkg/mutation/match/match_test.go b/pkg/mutation/match/match_test.go index ada098b218c..bb17bfde419 100644 --- a/pkg/mutation/match/match_test.go +++ b/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) } diff --git a/pkg/mutation/mutators/assign_mutator.go b/pkg/mutation/mutators/assign_mutator.go index 91103800595..028282dfe7c 100644 --- a/pkg/mutation/mutators/assign_mutator.go +++ b/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 diff --git a/pkg/mutation/mutators/assignmeta_mutator.go b/pkg/mutation/mutators/assignmeta_mutator.go index 7c50ea52660..79f38d5cc60 100644 --- a/pkg/mutation/mutators/assignmeta_mutator.go +++ b/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 diff --git a/pkg/mutation/mutators/core/mutation_function.go b/pkg/mutation/mutators/core/mutation_function.go index 7d4816c9ba9..33be7a091a8 100644 --- a/pkg/mutation/mutators/core/mutation_function.go +++ b/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 } } } diff --git a/pkg/mutation/mutators/core/mutation_function_test.go b/pkg/mutation/mutators/core/mutation_function_test.go index dc1ba524af4..06fee5642bd 100644 --- a/pkg/mutation/mutators/core/mutation_function_test.go +++ b/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()) } } diff --git a/pkg/mutation/mutators/testhelpers/dummy_mutator.go b/pkg/mutation/mutators/testhelpers/dummy_mutator.go index f4353db8594..f5160221f51 100644 --- a/pkg/mutation/mutators/testhelpers/dummy_mutator.go +++ b/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 } diff --git a/pkg/mutation/system.go b/pkg/mutation/system.go index f0b6b529d62..de37b781d06 100644 --- a/pkg/mutation/system.go +++ b/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...) } diff --git a/pkg/readiness/object_tracker.go b/pkg/readiness/object_tracker.go index adff87da8ac..dc7edaff92e 100644 --- a/pkg/readiness/object_tracker.go +++ b/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) diff --git a/pkg/readiness/objset.go b/pkg/readiness/objset.go index 648f3abf039..61bd9d34797 100644 --- a/pkg/readiness/objset.go +++ b/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()) } diff --git a/pkg/readiness/ready_tracker_test.go b/pkg/readiness/ready_tracker_test.go index c1bae488ab4..02b1bd19fd7 100644 --- a/pkg/readiness/ready_tracker_test.go +++ b/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") diff --git a/pkg/readiness/ready_tracker_unit_test.go b/pkg/readiness/ready_tracker_unit_test.go index 8a9be11d57b..1a25f4ef201 100644 --- a/pkg/readiness/ready_tracker_unit_test.go +++ b/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 diff --git a/pkg/target/target_integration_test.go b/pkg/target/target_integration_test.go index 0b058da47f8..90d27d07f9f 100644 --- a/pkg/target/target_integration_test.go +++ b/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, diff --git a/pkg/webhook/mutation.go b/pkg/webhook/mutation.go index 831d8d5884f..3448004b9d5 100644 --- a/pkg/webhook/mutation.go +++ b/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{} diff --git a/pkg/webhook/namespacelabel.go b/pkg/webhook/namespacelabel.go index 5fcf15a79ea..3771dc8c2b3 100644 --- a/pkg/webhook/namespacelabel.go +++ b/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") diff --git a/pkg/webhook/policy.go b/pkg/webhook/policy.go index d426188e309..f5ac204194f 100644 --- a/pkg/webhook/policy.go +++ b/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") diff --git a/pkg/webhook/policy_benchmark_test.go b/pkg/webhook/policy_benchmark_test.go index 8545040efed..641131f342f 100644 --- a/pkg/webhook/policy_benchmark_test.go +++ b/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)) diff --git a/pkg/webhook/stats_reporter.go b/pkg/webhook/stats_reporter.go index fe6c4f0d9f1..e5a0cb1a67b 100644 --- a/pkg/webhook/stats_reporter.go +++ b/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)),