From 212c0f6898ffa5060c253bcfd3a6caed43f57296 Mon Sep 17 00:00:00 2001 From: Luca Comellini Date: Tue, 4 Jun 2024 11:27:44 -0700 Subject: [PATCH] Run golang-ci lint for all the modules (#2081) --- .github/workflows/lint.yml | 5 +++++ .golangci.yml | 1 - .pre-commit-config.yaml | 7 +++++++ Makefile | 12 ++++++++++-- internal/mode/static/handler.go | 16 ++++++++-------- tests/Makefile | 5 ++--- tests/framework/ngf.go | 2 +- tests/framework/portforward.go | 3 +-- tests/framework/prometheus.go | 12 +++++++----- tests/framework/resourcemanager.go | 4 ++-- tests/suite/graceful_recovery_test.go | 7 ++++--- tests/suite/scale_test.go | 26 +++++++++++--------------- tests/suite/system_suite_test.go | 3 +-- 13 files changed, 59 insertions(+), 44 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e57f8f3b97..b62d55af59 100644 --- a/.github/workflows/lint.yml +++ b/.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 diff --git a/.golangci.yml b/.golangci.yml index 88dfe6d3ba..041ff220a3 100644 --- a/.golangci.yml +++ b/.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 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 82ad0528a6..40d1b31400 100644 --- a/.pre-commit-config.yaml +++ b/.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 diff --git a/Makefile b/Makefile index e652e4c1a7..047dd875db 100644 --- a/Makefile +++ b/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 diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 9bfdc1daab..2dfcbb2e56 100644 --- a/internal/mode/static/handler.go +++ b/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) } } } diff --git a/tests/Makefile b/tests/Makefile index eacc490596..21d01dee4a 100644 --- a/tests/Makefile +++ b/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 diff --git a/tests/framework/ngf.go b/tests/framework/ngf.go index 5c6695ee8c..e2f2eb97ab 100644 --- a/tests/framework/ngf.go +++ b/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() diff --git a/tests/framework/portforward.go b/tests/framework/portforward.go index e8234fea7c..26cd4b3cfb 100644 --- a/tests/framework/portforward.go +++ b/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" diff --git a/tests/framework/prometheus.go b/tests/framework/prometheus.go index 3afe01521f..caec955443 100644 --- a/tests/framework/prometheus.go +++ b/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 } diff --git a/tests/framework/resourcemanager.go b/tests/framework/resourcemanager.go index 193556f9b3..3612c7af34 100644 --- a/tests/framework/resourcemanager.go +++ b/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, diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index b9cfe5d131..2288f1b0a0 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/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") diff --git a/tests/suite/scale_test.go b/tests/suite/scale_test.go index 8d7c187db7..4813a908df 100644 --- a/tests/suite/scale_test.go +++ b/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 = ` diff --git a/tests/suite/system_suite_test.go b/tests/suite/system_suite_test.go index 3cb155a3b9..ab927649fd 100644 --- a/tests/suite/system_suite_test.go +++ b/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)