Run golang-ci lint for all the modules (#2081)

.github/workflows/lint.yml

@@ -23,6 +23,10 @@ jobs:
   lint:
     name: Lint
     runs-on: ubuntu-22.04
+    strategy:
+      fail-fast: false
+      matrix:
+        directory: [., tests] # we need to run golangci-lint for every module https://github.com/golangci/golangci-lint/issues/828
     steps:
       - name: Checkout Repository
         uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6
@@ -36,6 +40,7 @@ jobs:
         uses: golangci/golangci-lint-action@a4f60bb28d35aeee14e6880718e0c85ff1882e64 # v6.0.1
         with:
           args: --timeout 10m0s
+          working-directory: ${{ matrix.directory }}
 
   njs-lint:
     name: NJS Lint

.golangci.yml

@@ -17,7 +17,6 @@ linters-settings:
       - name: error-strings
       - name: errorf
       - name: exported
-      - name: if-return
       - name: increment-decrement
       - name: indent-error-flow
       - name: package-comments

.pre-commit-config.yaml

@@ -41,6 +41,13 @@ repos:
     rev: v1.59.0
     hooks:
       - id: golangci-lint-full
+        name: golangci-lint-root
+        alias: golangci-lint-root
+
+      - id: golangci-lint-full
+        name: golangci-lint-tests
+        alias: golangci-lint-tests
+        entry: bash -c 'cd tests && golangci-lint run --fix --config $OLDPWD/.golangci.yml'
 
   # Rules are in .markdownlint-cli2.yaml file
   # See https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md for rule descriptions

Makefile

@@ -25,6 +25,9 @@ GO_LINKER_FlAGS_VARS = -X main.version=${VERSION} -X main.commit=${GIT_COMMIT} -
 GO_LINKER_FLAGS_OPTIMIZATIONS = -s -w
 GO_LINKER_FLAGS = $(GO_LINKER_FLAGS_OPTIMIZATIONS) $(GO_LINKER_FlAGS_VARS)
 
+# tools versions
+GOLANGCI_LINT_VERSION := $(shell awk '/repo:.*golangci-lint/{getline; if ($$1 == "rev:") {sub(/^v/, "", $$2); print $$2}}' $(SELF_DIR).pre-commit-config.yaml)
+
 # variables that can be overridden by the user
 PREFIX ?= nginx-gateway-fabric## The name of the NGF image. For example, nginx-gateway-fabric
 NGINX_PREFIX ?= $(PREFIX)/nginx## The name of the nginx image. For example: nginx-gateway-fabric/nginx
@@ -168,9 +171,14 @@ njs-fmt: ## Run prettier against the njs httpmatches module
 vet: ## Run go vet against code
 	go vet ./...
 
+.PHONY: check-golangci-lint
+check-golangci-lint:
+	@golangci-lint --version || (code=$$?; printf "\033[0;31mError\033[0m: there was a problem with golangci-lint. Follow the docs to install it https://golangci-lint.run/welcome/install/\n"; exit $$code)
+	@golangci-lint --version | grep -q $(GOLANGCI_LINT_VERSION) || (printf "\033[0;33mWarning\033[0m: your golangci-lint version is different from the one specified in .pre-commit-config.yaml. The recommended version is $(GOLANGCI_LINT_VERSION)\n")
+
 .PHONY: lint
-lint: ## Run golangci-lint against code
-	docker run --pull always --rm -v $(CURDIR):/nginx-gateway-fabric -w /nginx-gateway-fabric -v $(shell go env GOCACHE):/cache/go -e GOCACHE=/cache/go -e GOLANGCI_LINT_CACHE=/cache/go -v $(shell go env GOPATH)/pkg:/go/pkg golangci/golangci-lint:latest golangci-lint --color always run
+lint: check-golangci-lint ## Run golangci-lint against code
+	golangci-lint run
 
 .PHONY: unit-test
 unit-test: ## Run unit tests for the go code

internal/mode/static/handler.go

@@ -282,9 +282,9 @@ func (h *eventHandlerImpl) updateStatuses(ctx context.Context, logger logr.Logge
 func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr.Logger, event interface{}) {
 	switch e := event.(type) {
 	case *events.UpsertEvent:
-		filterKey := objectFilterKey(e.Resource, client.ObjectKeyFromObject(e.Resource))
+		upFilterKey := objectFilterKey(e.Resource, client.ObjectKeyFromObject(e.Resource))
 
-		if filter, ok := h.objectFilters[filterKey]; ok {
+		if filter, ok := h.objectFilters[upFilterKey]; ok {
 			filter.upsert(ctx, logger, e.Resource)
 			if !filter.captureChangeInGraph {
 				return
@@ -293,9 +293,9 @@ func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr
 
 		h.cfg.processor.CaptureUpsertChange(e.Resource)
 	case *events.DeleteEvent:
-		filterKey := objectFilterKey(e.Type, e.NamespacedName)
+		delFilterKey := objectFilterKey(e.Type, e.NamespacedName)
 
-		if filter, ok := h.objectFilters[filterKey]; ok {
+		if filter, ok := h.objectFilters[delFilterKey]; ok {
 			filter.delete(ctx, logger, e.NamespacedName)
 			if !filter.captureChangeInGraph {
 				return
@@ -359,14 +359,14 @@ func (h *eventHandlerImpl) updateUpstreamServers(
 		}
 
 		for _, u := range conf.Upstreams {
-			upstream := upstream{
+			confUpstream := upstream{
 				name:    u.Name,
 				servers: ngxConfig.ConvertEndpoints(u.Endpoints),
 			}
 
-			if u, ok := prevUpstreams[upstream.name]; ok {
-				if !serversEqual(upstream.servers, u.Peers) {
-					upstreams = append(upstreams, upstream)
+			if u, ok := prevUpstreams[confUpstream.name]; ok {
+				if !serversEqual(confUpstream.servers, u.Peers) {
+					upstreams = append(upstreams, confUpstream)
 				}
 			}
 		}

tests/Makefile

@@ -9,7 +9,6 @@ GW_API_VERSION ?= $(shell sed -n 's/.*ref=v\(.*\)/\1/p' ../config/crd/gateway-ap
 GW_API_PREV_VERSION ?= 1.0.0## Supported Gateway API version from previous NGF release
 GW_SERVICE_TYPE = NodePort## Service type to use for the gateway
 GW_SVC_GKE_INTERNAL = false
-K8S_VERSION ?= latest## Kubernetes version to use. Expected format: 1.24 (major.minor) or latest
 NGF_VERSION ?= $(shell git describe --tags $(shell git rev-list --tags --max-count=1))## NGF version to be tested (defaults to latest tag)
 PULL_POLICY = Never## Pull policy for the images
 PROVISIONER_MANIFEST = conformance/provisioner/provisioner.yaml
@@ -114,7 +113,7 @@ stop-longevity-test: nfr-test ## Stop the longevity test and collects results
 	    --label-filter "nfr" $(GINKGO_FLAGS) ./suite --  --gateway-api-version=$(GW_API_VERSION) \
 		--gateway-api-prev-version=$(GW_API_PREV_VERSION) --image-tag=$(TAG) --version-under-test=$(NGF_VERSION) \
 		--plus-enabled=$(PLUS_ENABLED) --ngf-image-repo=$(PREFIX) --nginx-image-repo=$(NGINX_PREFIX) --nginx-plus-image-repo=$(NGINX_PLUS_PREFIX) \
-		--pull-policy=$(PULL_POLICY) --k8s-version=$(K8S_VERSION) --service-type=$(GW_SERVICE_TYPE) \
+		--pull-policy=$(PULL_POLICY) --service-type=$(GW_SERVICE_TYPE) \
 		--is-gke-internal-lb=$(GW_SVC_GKE_INTERNAL)
 
 .PHONY: test
@@ -124,7 +123,7 @@ test: ## Runs the functional tests on your default k8s cluster
 		--gateway-api-version=$(GW_API_VERSION) --gateway-api-prev-version=$(GW_API_PREV_VERSION) \
 		--image-tag=$(TAG) --version-under-test=$(NGF_VERSION) \
 		--plus-enabled=$(PLUS_ENABLED) --ngf-image-repo=$(PREFIX) --nginx-image-repo=$(NGINX_PREFIX) --nginx-plus-image-repo=$(NGINX_PLUS_PREFIX) \
-		--pull-policy=$(PULL_POLICY) --k8s-version=$(K8S_VERSION) --service-type=$(GW_SERVICE_TYPE) \
+		--pull-policy=$(PULL_POLICY) --service-type=$(GW_SERVICE_TYPE) \
 		--is-gke-internal-lb=$(GW_SVC_GKE_INTERNAL)
 
 .PHONY: test-with-plus

tests/framework/ngf.go

@@ -43,7 +43,7 @@ func InstallGatewayAPI(apiVersion string) ([]byte, error) {
 }
 
 // UninstallGatewayAPI uninstalls the specified version of the Gateway API resources.
-func UninstallGatewayAPI(apiVersion, k8sVersion string) ([]byte, error) {
+func UninstallGatewayAPI(apiVersion string) ([]byte, error) {
 	apiPath := fmt.Sprintf("%s/v%s/standard-install.yaml", gwInstallBasePath, apiVersion)
 
 	output, err := exec.Command("kubectl", "delete", "-f", apiPath).CombinedOutput()

tests/framework/portforward.go

@@ -3,13 +3,12 @@ package framework
 import (
 	"bytes"
 	"fmt"
+	"log/slog"
 	"net/http"
 	"net/url"
 	"path"
 	"time"
 
-	"log/slog"
-
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/portforward"
 	"k8s.io/client-go/transport/spdy"

tests/framework/prometheus.go

@@ -61,6 +61,7 @@ func InstallPrometheus(
 
 	scrapeInterval := fmt.Sprintf("%ds", int(cfg.ScrapeInterval.Seconds()))
 
+	// nolint:gosec
 	output, err = exec.Command(
 		"helm",
 		"install",
@@ -134,13 +135,12 @@ const (
 
 // PrometheusInstance represents a Prometheus instance in the cluster.
 type PrometheusInstance struct {
+	apiClient    v1.API
 	podIP        string
 	podName      string
 	podNamespace string
-	portForward  bool
 	queryTimeout time.Duration
-
-	apiClient v1.API
+	portForward  bool
 }
 
 // PortForward starts port forwarding to the Prometheus instance.
@@ -165,7 +165,7 @@ func (ins *PrometheusInstance) getAPIClient() (v1.API, error) {
 	}
 
 	cfg := api.Config{
-		Address: fmt.Sprintf("%s", endpoint),
+		Address: endpoint,
 	}
 
 	c, err := api.NewClient(cfg)
@@ -227,7 +227,9 @@ func (ins *PrometheusInstance) QueryRange(query string, promRange v1.Range) (mod
 }
 
 // QueryRangeWithCtx sends a range query to Prometheus with the specified context.
-func (ins *PrometheusInstance) QueryRangeWithCtx(ctx context.Context, query string, promRange v1.Range) (model.Value, error) {
+func (ins *PrometheusInstance) QueryRangeWithCtx(ctx context.Context,
+	query string, promRange v1.Range,
+) (model.Value, error) {
 	if err := ins.ensureAPIClient(); err != nil {
 		return nil, err
 	}

tests/framework/resourcemanager.go

@@ -307,7 +307,7 @@ func (rm *ResourceManager) WaitForAppsToBeReady(namespace string) error {
 }
 
 // WaitForAppsToBeReadyWithCtx waits for all apps in the specified namespace to be ready or
-// until the provided context is cancelled.
+// until the provided context is canceled.
 func (rm *ResourceManager) WaitForAppsToBeReadyWithCtx(ctx context.Context, namespace string) error {
 	if err := rm.WaitForPodsToBeReady(ctx, namespace); err != nil {
 		return err
@@ -325,7 +325,7 @@ func (rm *ResourceManager) WaitForAppsToBeReadyWithCtx(ctx context.Context, name
 }
 
 // WaitForPodsToBeReady waits for all Pods in the specified namespace to be ready or
-// until the provided context is cancelled.
+// until the provided context is canceled.
 func (rm *ResourceManager) WaitForPodsToBeReady(ctx context.Context, namespace string) error {
 	return wait.PollUntilContextCancel(
 		ctx,

tests/suite/graceful_recovery_test.go

@@ -191,7 +191,8 @@ func checkContainerRestart(ngfPodName, containerName string, currentRestartCount
 	}
 
 	if restartCount != currentRestartCount+1 {
-		return fmt.Errorf("expected current restart count: %d to match incremented restart count: %d", restartCount, currentRestartCount+1)
+		return fmt.Errorf("expected current restart count: %d to match incremented restart count: %d",
+			restartCount, currentRestartCount+1)
 	}
 
 	return nil
@@ -314,7 +315,7 @@ func getContainerRestartCount(ngfPodName, containerName string) (int, error) {
 
 	var ngfPod core.Pod
 	if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ngfNamespace, Name: ngfPodName}, &ngfPod); err != nil {
-		return 0, fmt.Errorf("error retriving NGF Pod: %w", err)
+		return 0, fmt.Errorf("error retrieving NGF Pod: %w", err)
 	}
 
 	var restartCount int
@@ -333,7 +334,7 @@ func runNodeDebuggerJob(ngfPodName, jobScript string) (*v1.Job, error) {
 
 	var ngfPod core.Pod
 	if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ngfNamespace, Name: ngfPodName}, &ngfPod); err != nil {
-		return nil, fmt.Errorf("error retriving NGF Pod: %w", err)
+		return nil, fmt.Errorf("error retrieving NGF Pod: %w", err)
 	}
 
 	b, err := resourceManager.GetFileContents("graceful-recovery/node-debugger-job.yaml")

tests/suite/scale_test.go

@@ -571,7 +571,7 @@ var _ = Describe("Scale test", Ordered, Label("nfr", "scale"), func() {
 		const testName = "TestScale_Listeners"
 
 		testResultsDir := filepath.Join(resultsDir, testName)
-		Expect(os.MkdirAll(testResultsDir, 0755)).To(Succeed())
+		Expect(os.MkdirAll(testResultsDir, 0o755)).To(Succeed())
 
 		objects, err := framework.GenerateScaleListenerObjects(httpListenerCount, false /*non-tls*/)
 		Expect(err).ToNot(HaveOccurred())
@@ -734,22 +734,18 @@ type bucket struct {
 }
 
 type scaleTestResults struct {
-	Name string
-
-	ReloadCount     int
-	ReloadErrsCount int
-	ReloadAvgTime   int
-	ReloadBuckets   []bucket
-
-	EventsCount   int
-	EventsAvgTime int
-	EventsBuckets []bucket
-
-	NGFErrors   int
-	NginxErrors int
-
+	Name                   string
+	EventsBuckets          []bucket
+	ReloadBuckets          []bucket
+	EventsAvgTime          int
+	EventsCount            int
 	NGFContainerRestarts   int
+	NGFErrors              int
 	NginxContainerRestarts int
+	NginxErrors            int
+	ReloadAvgTime          int
+	ReloadCount            int
+	ReloadErrsCount        int
 }
 
 const scaleResultTemplate = `

tests/suite/system_suite_test.go

@@ -48,7 +48,6 @@ var (
 	gatewayAPIPrevVersion = flag.String(
 		"gateway-api-prev-version", "", "Supported Gateway API version for previous NGF release",
 	)
-	k8sVersion = flag.String("k8s-version", "latest", "Version of k8s being tested on")
 	// Configurable NGF installation variables. Helm values will be used as defaults if not specified.
 	ngfImageRepository       = flag.String("ngf-image-repo", "", "Image repo for NGF control plane")
 	nginxImageRepository     = flag.String("nginx-image-repo", "", "Image repo for NGF data plane")
@@ -214,7 +213,7 @@ func teardown(relName string) {
 	output, err := framework.UninstallNGF(cfg, k8sClient)
 	Expect(err).ToNot(HaveOccurred(), string(output))
 
-	output, err = framework.UninstallGatewayAPI(*gatewayAPIVersion, *k8sVersion)
+	output, err = framework.UninstallGatewayAPI(*gatewayAPIVersion)
 	Expect(err).ToNot(HaveOccurred(), string(output))
 
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)