Use "revive" instead of "golint" linter

Per the golint repository, the golint linter is no longer maintained.
The recommended golangci-lint replacement is revive, so this PR switches
us to use the new linter. This commit also includes all necessary
changes to make the new linter pass.

In configuring this I noticed the that our .golangci.yaml had a typo
which caused our misspell and lll configurations to be silently
discarded - I've corrected "linter" to "linters" and resolved the
spelling mistakes it found.

Also upgrade golangci-lint from 1.37.1 to 1.40.1 as previous versions
did not configure gosec properly.

Signed-off-by: Will Beason <willbeason@google.com>

.github/workflows/workflow.yaml

@@ -47,7 +47,7 @@ jobs:
           echo "$GITHUB_WORKSPACE/bin" >> $GITHUB_PATH
           curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $GITHUB_WORKSPACE/bin v${GOLANGCILINT_VERSION}
         env:
-          GOLANGCILINT_VERSION: 1.37.1
+          GOLANGCILINT_VERSION: 1.40.1
 
       - name: Make lint
         run: |

.golangci.yaml

@@ -1,17 +1,23 @@
-linter-settings:
+linters-settings:
+  gosec:
+    excludes:
+    - G108
   lll:
     line-length: 200
 
   misspell:
     locale: US
+  staticcheck:
+    # Select the Go version to target. The default is '1.13'.
+    go: "1.16"
 
 linters:
   disable-all: true
   enable:
     - errcheck
     - govet
     - ineffassign
-    - golint
+    - revive # replacement for golint
     - goconst
     - gofmt
     - goimports

Makefile

@@ -183,7 +183,7 @@ manifests: __controller-gen
 	docker run --rm -v $(shell pwd):/gatekeeper --entrypoint /usr/local/bin/kustomize line/kubectl-kustomize:${KUBECTL_KUSTOMIZE_VERSION} build /gatekeeper/cmd/build/helmify | go run cmd/build/helmify/*.go
 
 lint:
-	golangci-lint -v run --exclude G108 ./... --timeout 5m
+	golangci-lint -v run ./... --timeout 5m
 
 # Generate code
 generate: __controller-gen target-template-source

apis/status/v1beta1/constraintpodstatus_types.go

@@ -28,9 +28,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
-const (
-	ConstraintsGroup = "constraints.gatekeeper.sh"
-)
+// ConstraintsGroup is the API Group for Gatekeeper Constraints.
+const ConstraintsGroup = "constraints.gatekeeper.sh"
 
 // ConstraintPodStatusStatus defines the observed state of ConstraintPodStatus
 type ConstraintPodStatusStatus struct {

apis/status/v1beta1/labels.go

@@ -1,5 +1,6 @@
 package v1beta1
 
+// Label keys used for internal gatekeeper operations.
 const (
 	ConstraintNameLabel         = "internal.gatekeeper.sh/constraint-name"
 	ConstraintKindLabel         = "internal.gatekeeper.sh/constraint-kind"

apis/status/v1beta1/mutatorpodstatus_types.go

@@ -28,9 +28,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
-const (
-	MutationsGroup = "mutations.gatekeeper.sh"
-)
+// MutationsGroup is the API Group for Gatekeeper Mutators.
+const MutationsGroup = "mutations.gatekeeper.sh"
 
 // MutatorPodStatusStatus defines the observed state of MutatorPodStatus
 type MutatorPodStatusStatus struct {
@@ -103,7 +102,7 @@ func NewMutatorStatusForPod(pod *corev1.Pod, mutatorID mtypes.ID, scheme *runtim
 	return obj, nil
 }
 
-// KeyForMutator returns a unique status object name given the Pod ID and
+// KeyForMutatorID returns a unique status object name given the Pod ID and
 // a mutator object
 func KeyForMutatorID(id string, mID mtypes.ID) (string, error) {
 	// This adds a requirement that the lowercase of all mutator kinds must be unique.

pkg/controller/config/config_controller_test.go

@@ -227,7 +227,7 @@ func TestReconcile(t *testing.T) {
 	cs.Stop()
 }
 
-// tests that expectations for sync only resource gets cancelled when it gets deleted
+// tests that expectations for sync only resource gets canceled when it gets deleted
 func TestConfig_DeleteSyncResources(t *testing.T) {
 	log.Info("Running test: Cancel the expectations when sync only resource gets deleted")
 

pkg/controller/config/process/excluder.go

@@ -10,8 +10,10 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
+// Process indicates the Gatekeeper component making the request.
 type Process string
 
+// The set of defined Gatekeeper processes.
 const (
 	Audit    = Process("audit")
 	Sync     = Process("sync")

pkg/controller/constrainttemplate/constrainttemplate_controller.go

@@ -558,20 +558,14 @@ func (r *ReconcileConstraintTemplate) addWatch(kind schema.GroupVersionKind) err
 	if err := r.watcher.AddWatch(kind); err != nil {
 		return err
 	}
-	if err := r.statusWatcher.AddWatch(kind); err != nil {
-		return err
-	}
-	return nil
+	return r.statusWatcher.AddWatch(kind)
 }
 
 func (r *ReconcileConstraintTemplate) removeWatch(kind schema.GroupVersionKind) error {
 	if err := r.watcher.RemoveWatch(kind); err != nil {
 		return err
 	}
-	if err := r.statusWatcher.RemoveWatch(kind); err != nil {
-		return err
-	}
-	return nil
+	return r.statusWatcher.RemoveWatch(kind)
 }
 
 type action string

pkg/controller/constrainttemplate/constrainttemplate_controller_test.go

@@ -383,7 +383,7 @@ violation[{"msg": "denied!"}] {
 	})
 }
 
-// Tests that expectations for constraints are cancelled if the corresponding constraint is deleted.
+// Tests that expectations for constraints are canceled if the corresponding constraint is deleted.
 func TestReconcile_DeleteConstraintResources(t *testing.T) {
 	log.Info("Running test: Cancel the expectations when constraint gets deleted")
 

pkg/controller/sync/stats_reporter.go

@@ -62,7 +62,7 @@ type Reporter struct {
 	now func() float64
 }
 
-// newStatsReporter creates a reporter for sync metrics
+// NewStatsReporter creates a reporter for sync metrics
 func NewStatsReporter() (*Reporter, error) {
 	ctx, err := tag.New(
 		context.TODO(),

pkg/logging/logging.go

@@ -1,5 +1,6 @@
 package logging
 
+// Log keys.
 const (
 	Process              = "process"
 	EventType            = "event_type"

pkg/metrics/status.go

@@ -1,12 +1,17 @@
 package metrics
 
+// Status is whether a ConstraintTemplate is functioning properly.
+// Reported in metrics.
 type Status string
 
 const (
+	// ActiveStatus indicates a ConstraintTemplate is operating normally.
 	ActiveStatus Status = "active"
-	ErrorStatus  Status = "error"
+	// ErrorStatus indicates there is a problem with a ConstraintTemplate.
+	ErrorStatus Status = "error"
 )
 
 var (
+	// AllStatuses is the set of all allowed values of Status.
 	AllStatuses = []Status{ActiveStatus, ErrorStatus}
 )

pkg/mutation/match/match.go

@@ -21,6 +21,7 @@ type ApplyTo struct {
 	Versions []string `json:"versions,omitempty"`
 }
 
+// Match selects objects to apply mutations to.
 // +kubebuilder:object:generate=true
 type Match struct {
 	Kinds              []Kinds                            `json:"kinds,omitempty"`

pkg/mutation/mutators/assign_mutator.go

@@ -156,7 +156,7 @@ func MutatorForAssign(assign *mutationsv1alpha1.Assign) (*AssignMutator, error)
 	}
 
 	if hasMetadataRoot(path) {
-		return nil, errors.New(fmt.Sprintf("assign %s can't change metadata", assign.GetName()))
+		return nil, fmt.Errorf("assign %s can't change metadata", assign.GetName())
 	}
 
 	err = checkKeyNotChanged(path, assign.GetName())
@@ -172,7 +172,7 @@ func MutatorForAssign(assign *mutationsv1alpha1.Assign) (*AssignMutator, error)
 
 	value, ok := toAssign["value"]
 	if !ok {
-		return nil, errors.New(fmt.Sprintf("spec.parameters.assign for Assign %s must have a value field", assign.GetName()))
+		return nil, fmt.Errorf("spec.parameters.assign for Assign %s must have a value field", assign.GetName())
 	}
 
 	err = validateObjectAssignedToList(path, value, assign.GetName())
@@ -292,7 +292,7 @@ func checkKeyNotChanged(p *parser.Path, assignName string) error {
 		return nil
 	}
 	if lastNode.Type() != parser.ObjectNode {
-		return errors.New(fmt.Sprintf("invalid path format in Assign %s: child of a list can't be a list", assignName))
+		return fmt.Errorf("invalid path format in Assign %s: child of a list can't be a list", assignName)
 	}
 	addedObject, ok := lastNode.(*parser.Object)
 	if !ok {
@@ -304,7 +304,7 @@ func checkKeyNotChanged(p *parser.Path, assignName string) error {
 	}
 
 	if addedObject.Reference == listNode.KeyField {
-		return errors.New(fmt.Sprintf("invalid path format in Assign %s: changing the item key is not allowed", assignName))
+		return fmt.Errorf("invalid path format in Assign %s: changing the item key is not allowed", assignName)
 	}
 	return nil
 }

pkg/mutation/mutators/testhelpers/dummy_mutator.go

@@ -15,7 +15,7 @@ import (
 
 var _ types.Mutator = &DummyMutator{}
 
-// dummyMutator is a blank mutator that makes it easier to test the core mutation function
+// DummyMutator is a blank mutator that makes it easier to test the core mutation function
 type DummyMutator struct {
 	name  string
 	value interface{}

pkg/mutation/path/parser/node.go

@@ -18,8 +18,11 @@ package parser
 type NodeType string
 
 const (
-	PathNode   NodeType = "Path"
-	ListNode   NodeType = "List"
+	// PathNode is a string segment of a path.
+	PathNode NodeType = "Path"
+	// ListNode is an array element of a path.
+	ListNode NodeType = "List"
+	// ObjectNode is the final Node in a path, what is being referenced.
 	ObjectNode NodeType = "Object"
 )
 

pkg/mutation/path/token/token.go

@@ -17,6 +17,7 @@ package token
 
 import "fmt"
 
+// The set of Token types.
 const (
 	ERROR     = "ERROR"
 	EOF       = "EOF"

pkg/mutation/schema/schema_test.go

@@ -1,7 +1,6 @@
 package schema
 
 import (
-	"fmt"
 	"reflect"
 	"testing"
 
@@ -1235,7 +1234,7 @@ func TestErrors(t *testing.T) {
 				t.Fatal("unexpected nil error")
 			}
 			if err.Error() != test.expectedErr {
-				t.Error(fmt.Sprintf("got %v, wanted %v", err.Error(), test.expectedErr))
+				t.Errorf("got %v, wanted %v", err.Error(), test.expectedErr)
 			}
 		})
 	}

pkg/operations/operations.go

@@ -12,6 +12,7 @@ import (
 
 type Operation string
 
+// All defined Operations.
 const (
 	Audit          = Operation("audit")
 	Status         = Operation("status")
@@ -20,9 +21,9 @@ const (
 )
 
 var (
-	// allOperations is a list of all possible operations that can be assigned to
-	// a pod it is NOT intended to be mutated. It should be kept in alphabetical
-	// order so that it can be readily compared to the results from AssignedOperations
+	// allOperations is a list of all possible Operations that can be assigned to
+	// a pod. It is NOT intended to be mutated. It should be kept in alphabetical
+	// order so that it can be readily compared to the results from AssignedOperations.
 	allOperations = []Operation{
 		Audit,
 		Status,

pkg/readiness/object_tracker.go

@@ -53,15 +53,15 @@ type Expectations interface {
 type objectTracker struct {
 	mu            sync.RWMutex
 	gvk           schema.GroupVersionKind
-	cancelled     objSet                    // expectations that have been cancelled
+	canceled      objSet                    // expectations that have been canceled
 	expect        objSet                    // unresolved expectations
-	tryCancelled  objRetrySet               // tracks TryCancelExpect calls, decrementing allotted retries for an object
+	tryCanceled   objRetrySet               // tracks TryCancelExpect calls, decrementing allotted retries for an object
 	seen          objSet                    // observations made before their expectations
 	satisfied     objSet                    // tracked to avoid re-adding satisfied expectations and to support unsatisfied()
 	populated     bool                      // all expectations have been provided
 	allSatisfied  bool                      // true once all expectations have been satisfied. Acts as a circuit-breaker.
 	kindsSnapshot []schema.GroupVersionKind // Snapshot of kinds before freeing memory in Satisfied.
-	tryCancelObj  objDataFactory            // Function that creates objData types used in tryCancelled
+	tryCancelObj  objDataFactory            // Function that creates objData types used in tryCanceled
 }
 
 func newObjTracker(gvk schema.GroupVersionKind, fn objDataFactory) *objectTracker {
@@ -71,9 +71,9 @@ func newObjTracker(gvk schema.GroupVersionKind, fn objDataFactory) *objectTracke
 
 	return &objectTracker{
 		gvk:          gvk,
-		cancelled:    make(objSet),
+		canceled:     make(objSet),
 		expect:       make(objSet),
-		tryCancelled: make(objRetrySet),
+		tryCanceled:  make(objRetrySet),
 		seen:         make(objSet),
 		satisfied:    make(objSet),
 		tryCancelObj: fn,
@@ -102,8 +102,8 @@ func (t *objectTracker) Expect(o runtime.Object) {
 		return
 	}
 
-	// Cancelled objects cannot be expected again.
-	if _, ok := t.cancelled[k]; ok {
+	// Canceled objects cannot be expected again.
+	if _, ok := t.canceled[k]; ok {
 		return
 	}
 
@@ -122,8 +122,8 @@ func (t *objectTracker) cancelExpectNoLock(k objKey) {
 	delete(t.expect, k)
 	delete(t.seen, k)
 	delete(t.satisfied, k)
-	delete(t.tryCancelled, k)
-	t.cancelled[k] = struct{}{}
+	delete(t.tryCanceled, k)
+	t.canceled[k] = struct{}{}
 }
 
 // CancelExpect cancels an expectation and marks it so it
@@ -148,7 +148,7 @@ func (t *objectTracker) CancelExpect(o runtime.Object) {
 
 // TryCancelExpect will check the readinessRetries left on an Object, and cancel
 // the expectation for that object if no retries remain.  Returns True if the
-// expectation was cancelled.
+// expectation was canceled.
 func (t *objectTracker) TryCancelExpect(o runtime.Object) bool {
 	t.mu.Lock()
 	defer t.mu.Unlock()
@@ -165,13 +165,13 @@ func (t *objectTracker) TryCancelExpect(o runtime.Object) bool {
 	}
 
 	// Check if it's time to delete an expectation or just decrement its allotted retries
-	obj, ok := t.tryCancelled[k]
+	obj, ok := t.tryCanceled[k]
 	if !ok {
 		// If the item isn't in the map, add it.  This is the only place t.newObjData() should be called.
 		obj = t.tryCancelObj()
 	}
 	shouldDel := obj.decrementRetries()
-	t.tryCancelled[k] = obj // set the changed obj back to the map, as the value is not a pointer
+	t.tryCanceled[k] = obj // set the changed obj back to the map, as the value is not a pointer
 
 	if shouldDel {
 		t.cancelExpectNoLock(k)
@@ -222,8 +222,8 @@ func (t *objectTracker) Observe(o runtime.Object) {
 		return
 	}
 
-	// Ignore cancelled expectations
-	if _, ok := t.cancelled[k]; ok {
+	// Ignore canceled expectations
+	if _, ok := t.canceled[k]; ok {
 		return
 	}
 
@@ -323,8 +323,8 @@ func (t *objectTracker) Satisfied() bool {
 		t.seen = nil
 		t.expect = nil
 		t.satisfied = nil
-		t.cancelled = nil
-		t.tryCancelled = nil
+		t.canceled = nil
+		t.tryCanceled = nil
 	}
 	return t.allSatisfied
 }