Update golangci-lint, add more linters

Fix minor issues, but plenty of them. Add new linters.
Most changes come from gocritic. Performance was optimised for changing
copy of big structures like GameServerSpec into to using a pointer.

.golangci.yml

@@ -46,23 +46,47 @@ output:
 
 linters:
   enable:
+    - deadcode
     - megacheck
     - govet
     - golint
     - interfacer
     - unconvert
     - dupl
     - goconst
+    - gocritic
     - gocyclo
     - goimports
+    - interfacer
     - maligned
     - megacheck
+    - scopelint
+    - staticcheck
+    - structcheck
     - misspell
     - unparam
     - nakedret
     - bodyclose
 
+linters-settings:
+  gocritic:
+    enabled-tags:
+      - performance
+      - opinionated
+      - diagnostic
+    disabled-checks:
+      - paramTypeCombine
+      - unnamedResult
+    settings: # settings passed to gocritic
+      hugeParam:
+        sizeThreshold: 512
+      rangeValCopy:
+        sizeThreshold: 512
+        skipTestFuncs: true
+
 issues:
   # This turns off the default excludes - which was causing the linter
   # to miss things like erroneous comments
-  exclude-use-default: false
+  exclude-use-default: false
+  exclude:
+    - Using the variable on range scope .* in function literal

build/Makefile

@@ -304,7 +304,7 @@ build-controller-binary: $(ensure-build-image)
 lint: LINT_TIMEOUT ?= 15m
 lint: $(ensure-build-image)
 	docker run -t -e "TERM=xterm-256color" -e "$(gomod_on)" --rm $(common_mounts) -w $(workdir_path) $(DOCKER_RUN_ARGS) $(build_tag) bash -c \
-		"golangci-lint run ./examples/... && golangci-lint run --deadline $(LINT_TIMEOUT) ./..."
+		"golangci-lint run ./examples/... && golangci-lint run --timeout $(LINT_TIMEOUT) ./..."
 
 # Extract licenses from source tree
 build-licenses:

build/build-image/Dockerfile

@@ -68,7 +68,7 @@ RUN curl -L  ${HELM_URL} > /tmp/helm.tar.gz \
 RUN echo "source <(helm completion bash)" >> /root/.bashrc
 
 # install golang-ci linter
-RUN curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $GOPATH/bin v1.18.0
+RUN curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $GOPATH/bin v1.22.2
 
 #
 #  \ \      / /__| |__  ___(_) |_ ___

cmd/allocator/main.go

@@ -243,19 +243,20 @@ func getCACertPool(path string) (*x509.CertPool, error) {
 	}
 
 	for _, file := range filesInfo {
-		if strings.HasSuffix(file.Name(), ".crt") || strings.HasSuffix(file.Name(), ".pem") {
-			certFile := filepath.Join(path, file.Name())
-			caCert, err := ioutil.ReadFile(certFile)
-			if err != nil {
-				logger.Errorf("ca cert is not readable or missing: %s", err.Error())
-				continue
-			}
-			if !caCertPool.AppendCertsFromPEM(caCert) {
-				logger.Errorf("client cert %s cannot be installed", certFile)
-				continue
-			}
-			logger.Infof("client cert %s is installed", certFile)
+		if !strings.HasSuffix(file.Name(), ".crt") && !strings.HasSuffix(file.Name(), ".pem") {
+			continue
+		}
+		certFile := filepath.Join(path, file.Name())
+		caCert, err := ioutil.ReadFile(certFile)
+		if err != nil {
+			logger.Errorf("ca cert is not readable or missing: %s", err.Error())
+			continue
+		}
+		if !caCertPool.AppendCertsFromPEM(caCert) {
+			logger.Errorf("client cert %s cannot be installed", certFile)
+			continue
 		}
+		logger.Infof("client cert %s is installed", certFile)
 	}
 
 	return caCertPool, nil
@@ -323,8 +324,7 @@ func (h *httpHandler) allocateHandler(w http.ResponseWriter, r *http.Request) {
 
 func httpCode(err error) int {
 	code := http.StatusInternalServerError
-	switch t := err.(type) {
-	case k8serror.APIStatus:
+	if t, ok := err.(k8serror.APIStatus); ok {
 		code = int(t.Status().Code)
 	}
 	return code

cmd/controller/main.go

@@ -359,7 +359,7 @@ type config struct {
 }
 
 // validate ensures the ctlConfig data is valid.
-func (c config) validate() error {
+func (c *config) validate() error {
 	if c.MinPort <= 0 || c.MaxPort <= 0 {
 		return errors.New("min Port and Max Port values are required")
 	}

cmd/ping/udp_test.go

@@ -120,9 +120,9 @@ func TestUDPServerHealth(t *testing.T) {
 	assert.Nil(t, err)
 }
 
-func defaultFixture(clock clock.Clock) (*udpServer, error) {
+func defaultFixture(cl clock.Clock) (*udpServer, error) {
 	u := newUDPServer(5)
-	u.clock = clock
+	u.clock = cl
 	var err error
 	u.conn, err = net.ListenPacket("udp", ":0")
 	return u, err

cmd/sdk-server/main.go

@@ -90,7 +90,8 @@ func main() {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	if ctlConf.IsLocal {
+	switch {
+	case ctlConf.IsLocal:
 		localSDK, err := registerLocal(grpcServer, ctlConf)
 		if err != nil {
 			logger.WithError(err).Fatal("Could not start local sdk server")
@@ -103,7 +104,7 @@ func main() {
 				close(timedStop)
 			}()
 		}
-	} else if ctlConf.Test != "" {
+	case ctlConf.Test != "":
 		localSDK, err := registerTestSdkServer(grpcServer, ctlConf)
 		if err != nil {
 			logger.WithError(err).Fatal("Could not start test sdk server")
@@ -116,7 +117,7 @@ func main() {
 				close(timedStop)
 			}()
 		}
-	} else {
+	default:
 		var config *rest.Config
 		config, err := rest.InClusterConfig()
 		if err != nil {

pkg/allocation/converters/converter.go

@@ -127,6 +127,7 @@ func convertInternalLabelSelectorToLabelSelector(in *metav1.LabelSelector) *pb.L
 func convertInternalLabelSelectorsToLabelSelectors(in []metav1.LabelSelector) []*pb.LabelSelector {
 	var result []*pb.LabelSelector
 	for _, l := range in {
+		l := l
 		c := convertInternalLabelSelectorToLabelSelector(&l)
 		result = append(result, c)
 	}

pkg/apis/agones/v1/common.go

@@ -74,7 +74,7 @@ func validateGSSpec(gs gsSpec) []metav1.StatusCause {
 
 // validateObjectMeta Check ObjectMeta specification
 // Used by Fleet, GameServerSet and GameServer
-func validateObjectMeta(objMeta metav1.ObjectMeta) []metav1.StatusCause {
+func validateObjectMeta(objMeta *metav1.ObjectMeta) []metav1.StatusCause {
 	var causes []metav1.StatusCause
 
 	errs := metav1validation.ValidateLabels(objMeta.Labels, field.NewPath("labels"))

pkg/apis/agones/v1/fleet.go

@@ -179,7 +179,7 @@ func (f *Fleet) Validate() ([]metav1.StatusCause, bool) {
 	if len(gsCauses) > 0 {
 		causes = append(causes, gsCauses...)
 	}
-	objMetaCauses := validateObjectMeta(f.Spec.Template.ObjectMeta)
+	objMetaCauses := validateObjectMeta(&f.Spec.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)
 	}

pkg/apis/agones/v1/gameserver.go

@@ -312,7 +312,7 @@ func (gss *GameServerSpec) applySchedulingDefaults() {
 // devAddress is a specific IP address used for local Gameservers, for fleets "" is used
 // If a GameServer Spec is invalid there will be > 0 values in
 // the returned array
-func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bool) {
+func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bool) {
 	var causes []metav1.StatusCause
 	if devAddress != "" {
 		// verify that the value is a valid IP address.
@@ -342,7 +342,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo
 		}
 	} else {
 		// make sure a name is specified when there is multiple containers in the pod.
-		if len(gss.Container) == 0 && len(gss.Template.Spec.Containers) > 1 {
+		if gss.Container == "" && len(gss.Template.Spec.Containers) > 1 {
 			causes = append(causes, metav1.StatusCause{
 				Type:    metav1.CauseTypeFieldValueInvalid,
 				Field:   "container",
@@ -389,7 +389,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo
 			})
 		}
 	}
-	objMetaCauses := validateObjectMeta(gss.Template.ObjectMeta)
+	objMetaCauses := validateObjectMeta(&gss.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)
 	}

pkg/apis/agones/v1/gameserverset.go

@@ -79,9 +79,9 @@ type GameServerSetStatus struct {
 
 // ValidateUpdate validates when updates occur. The argument
 // is the new GameServerSet, being passed into the old GameServerSet
-func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) ([]metav1.StatusCause, bool) {
-	causes := validateName(new)
-	if !apiequality.Semantic.DeepEqual(gsSet.Spec.Template, new.Spec.Template) {
+func (gsSet *GameServerSet) ValidateUpdate(newGSS *GameServerSet) ([]metav1.StatusCause, bool) {
+	causes := validateName(newGSS)
+	if !apiequality.Semantic.DeepEqual(gsSet.Spec.Template, newGSS.Spec.Template) {
 		causes = append(causes, metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueInvalid,
 			Field:   "template",
@@ -102,7 +102,7 @@ func (gsSet *GameServerSet) Validate() ([]metav1.StatusCause, bool) {
 		causes = append(causes, gsCauses...)
 	}
 
-	objMetaCauses := validateObjectMeta(gsSet.Spec.Template.ObjectMeta)
+	objMetaCauses := validateObjectMeta(&gsSet.Spec.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)
 	}

pkg/apis/agones/v1/register.go

@@ -53,15 +53,15 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&GameServer{},
 		&GameServerList{},
 		&GameServerSet{},
 		&GameServerSetList{},
 		&Fleet{},
 		&FleetList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/apis/allocation/v1/gameserverallocation.go

@@ -101,6 +101,7 @@ func (gsas *GameServerAllocationSpec) PreferredSelectors() ([]labels.Selector, e
 
 	var err error
 	for i, p := range gsas.Preferred {
+		p := p
 		list[i], err = metav1.LabelSelectorAsSelector(&p)
 		if err != nil {
 			break

pkg/apis/allocation/v1/register.go

@@ -49,11 +49,11 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&GameServerAllocation{},
 		&GameServerAllocationList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/apis/autoscaling/v1/register.go

@@ -49,11 +49,11 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&FleetAutoscaler{},
 		&FleetAutoscalerList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/apis/multicluster/v1alpha1/gameserverallocationpolicy.go

@@ -175,11 +175,11 @@ func selectRandomWeighted(connections []*ClusterConnectionInfo, weights []int) *
 		return nil
 	}
 
-	rand := rand.Intn(sum)
+	randValue := rand.Intn(sum)
 	sum = 0
 	for i, weight := range weights {
 		sum += weight
-		if rand < sum {
+		if randValue < sum {
 			return connections[i]
 		}
 	}

pkg/apis/multicluster/v1alpha1/register.go

@@ -49,11 +49,11 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&GameServerAllocationPolicy{},
 		&GameServerAllocationPolicyList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/fleets/controller.go

@@ -386,16 +386,17 @@ func (c *Controller) deleteEmptyGameServerSets(fleet *agonesv1.Fleet, list []*ag
 // GameServerSets, and return the replica count for the active GameServerSet
 func (c *Controller) recreateDeployment(fleet *agonesv1.Fleet, rest []*agonesv1.GameServerSet) (int32, error) {
 	for _, gsSet := range rest {
-		if gsSet.Spec.Replicas != 0 {
-			c.loggerForFleet(fleet).WithField("gameserverset", gsSet.ObjectMeta.Name).Info("applying recreate deployment: scaling to 0")
-			gsSetCopy := gsSet.DeepCopy()
-			gsSetCopy.Spec.Replicas = 0
-			if _, err := c.gameServerSetGetter.GameServerSets(gsSetCopy.ObjectMeta.Namespace).Update(gsSetCopy); err != nil {
-				return 0, errors.Wrapf(err, "error updating gameserverset %s", gsSetCopy.ObjectMeta.Name)
-			}
-			c.recorder.Eventf(fleet, corev1.EventTypeNormal, "ScalingGameServerSet",
-				"Scaling inactive GameServerSet %s from %d to %d", gsSetCopy.ObjectMeta.Name, gsSet.Spec.Replicas, gsSetCopy.Spec.Replicas)
+		if gsSet.Spec.Replicas == 0 {
+			continue
+		}
+		c.loggerForFleet(fleet).WithField("gameserverset", gsSet.ObjectMeta.Name).Info("applying recreate deployment: scaling to 0")
+		gsSetCopy := gsSet.DeepCopy()
+		gsSetCopy.Spec.Replicas = 0
+		if _, err := c.gameServerSetGetter.GameServerSets(gsSetCopy.ObjectMeta.Namespace).Update(gsSetCopy); err != nil {
+			return 0, errors.Wrapf(err, "error updating gameserverset %s", gsSetCopy.ObjectMeta.Name)
 		}
+		c.recorder.Eventf(fleet, corev1.EventTypeNormal, "ScalingGameServerSet",
+			"Scaling inactive GameServerSet %s from %d to %d", gsSetCopy.ObjectMeta.Name, gsSet.Spec.Replicas, gsSetCopy.Spec.Replicas)
 	}
 
 	return fleet.LowerBoundReplicas(fleet.Spec.Replicas - agonesv1.SumStatusAllocatedReplicas(rest)), nil

pkg/fleets/controller_test.go

@@ -340,7 +340,7 @@ func TestControllerRun(t *testing.T) {
 
 	// test updating fleet
 	fCopy := fleet.DeepCopy()
-	fCopy.Spec.Replicas = fCopy.Spec.Replicas + 10
+	fCopy.Spec.Replicas += 10
 	fleetWatch.Modify(fCopy)
 	assert.Equal(t, expected, f())
 

pkg/gameserverallocations/allocator.go

@@ -238,11 +238,12 @@ func (c *Allocator) allocateFromLocalCluster(gsa *allocationv1.GameServerAllocat
 		return nil, err
 	}
 
-	if err == ErrNoGameServerReady {
+	switch err {
+	case ErrNoGameServerReady:
 		gsa.Status.State = allocationv1.GameServerAllocationUnAllocated
-	} else if err == ErrConflictInGameServerSelection {
+	case ErrConflictInGameServerSelection:
 		gsa.Status.State = allocationv1.GameServerAllocationContention
-	} else {
+	default:
 		gsa.ObjectMeta.Name = gs.ObjectMeta.Name
 		gsa.Status.State = allocationv1.GameServerAllocationAllocated
 		gsa.Status.GameServerName = gs.ObjectMeta.Name
@@ -532,7 +533,7 @@ func (c *Allocator) allocationUpdateWorkers(workerCount int, stop <-chan struct{
 			for {
 				select {
 				case res := <-updateQueue:
-					gs, err := c.readyGameServerCache.PatchGameServerMetadata(res.request.gsa.Spec.MetaPatch, *res.gs)
+					gs, err := c.readyGameServerCache.PatchGameServerMetadata(res.request.gsa.Spec.MetaPatch, res.gs)
 					if err != nil {
 						// since we could not allocate, we should put it back
 						c.readyGameServerCache.AddToReadyGameServer(gs)

pkg/gameserverallocations/cache_test.go

@@ -44,11 +44,8 @@ func TestGameServerCacheEntry(t *testing.T) {
 
 	count := 0
 	cache.Range(func(key string, gs *agonesv1.GameServer) bool {
-		if count++; count == 2 {
-			return false
-		}
-
-		return true
+		count++
+		return count != 2
 	})
 
 	assert.Equal(t, 2, count, "Should only process one item")