Merge branch 'main' into docs/helm-docs

Author: lucacome

.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,
@@ -683,3 +683,57 @@ func countNumberOfReadyParents(parents []v1.RouteParentStatus) int {
 
 	return readyCount
 }
+
+func (rm *ResourceManager) WaitForAppsToBeReadyWithPodCount(namespace string, podCount int) error {
+	ctx, cancel := context.WithTimeout(context.Background(), rm.TimeoutConfig.CreateTimeout)
+	defer cancel()
+
+	return rm.WaitForAppsToBeReadyWithCtxWithPodCount(ctx, namespace, podCount)
+}
+
+func (rm *ResourceManager) WaitForAppsToBeReadyWithCtxWithPodCount(
+	ctx context.Context,
+	namespace string,
+	podCount int,
+) error {
+	if err := rm.WaitForPodsToBeReadyWithCount(ctx, namespace, podCount); err != nil {
+		return err
+	}
+
+	if err := rm.waitForHTTPRoutesToBeReady(ctx, namespace); err != nil {
+		return err
+	}
+
+	if err := rm.waitForGRPCRoutesToBeReady(ctx, namespace); err != nil {
+		return err
+	}
+
+	return rm.waitForGatewaysToBeReady(ctx, namespace)
+}
+
+// WaitForPodsToBeReady waits for all Pods in the specified namespace to be ready or
+// until the provided context is canceled.
+func (rm *ResourceManager) WaitForPodsToBeReadyWithCount(ctx context.Context, namespace string, count int) error {
+	return wait.PollUntilContextCancel(
+		ctx,
+		500*time.Millisecond,
+		true, /* poll immediately */
+		func(ctx context.Context) (bool, error) {
+			var podList core.PodList
+			if err := rm.K8sClient.List(ctx, &podList, client.InNamespace(namespace)); err != nil {
+				return false, err
+			}
+
+			var podsReady int
+			for _, pod := range podList.Items {
+				for _, cond := range pod.Status.Conditions {
+					if cond.Type == core.PodReady && cond.Status == core.ConditionTrue {
+						podsReady++
+					}
+				}
+			}
+
+			return podsReady == count, nil
+		},
+	)
+}

tests/suite/graceful_recovery_test.go

@@ -28,7 +28,7 @@ const (
 
 // Since checkContainerLogsForErrors may experience interference from previous tests (as explained in the function
 // documentation), this test is recommended to be run separate from other nfr tests.
-var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recovery"), func() {
+var _ = Describe("Graceful Recovery test", Ordered, Label("functional", "graceful-recovery"), func() {
 	files := []string{
 		"graceful-recovery/cafe.yaml",
 		"graceful-recovery/cafe-secret.yaml",
@@ -38,8 +38,10 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov
 
 	var ns core.Namespace
 
-	teaURL := "https://cafe.example.com/tea"
-	coffeeURL := "http://cafe.example.com/coffee"
+	baseHTTPURL := "http://cafe.example.com"
+	baseHTTPSURL := "https://cafe.example.com"
+	teaURL := baseHTTPSURL + "/tea"
+	coffeeURL := baseHTTPURL + "/coffee"
 
 	var ngfPodName string
 
@@ -56,6 +58,12 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov
 		Expect(podNames).To(HaveLen(1))
 
 		ngfPodName = podNames[0]
+		if portFwdPort != 0 {
+			coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHTTPURL, portFwdPort)
+		}
+		if portFwdHTTPSPort != 0 {
+			teaURL = fmt.Sprintf("%s:%d/tea", baseHTTPSURL, portFwdHTTPSPort)
+		}
 	})
 
 	BeforeEach(func() {
@@ -67,7 +75,7 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("nfr", "graceful-recov
 
 		Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed())
 		Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed())
-		Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed())
+		Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed())
 
 		Eventually(
 			func() error {
@@ -101,7 +109,7 @@ func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files
 	)
 
 	if containerName != nginxContainerName {
-		// Since we have already deployed resources and ran resourceManager.WaitForAppsToBeReady(ns.Name) earlier,
+		// Since we have already deployed resources and ran resourceManager.WaitForAppsToBeReadyWithPodCount earlier,
 		// we know that the applications are ready at this point. This could only be the case if NGF has written
 		// statuses, which could only be the case if NGF has the leader lease. Since there is only one instance
 		// of NGF in this test, we can be certain that this is the correct leaseholder name.
@@ -140,7 +148,7 @@ func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files
 		Should(Succeed())
 
 	Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed())
-	Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed())
+	Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed())
 
 	Eventually(
 		func() error {
@@ -191,7 +199,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
@@ -264,7 +273,11 @@ func checkContainerLogsForErrors(ngfPodName string) {
 		Expect(line).ToNot(ContainSubstring("[alert]"), line)
 		Expect(line).ToNot(ContainSubstring("[emerg]"), line)
 		if strings.Contains(line, "[error]") {
-			Expect(line).To(ContainSubstring("connect() failed (111: Connection refused)"), line)
+			expectedError1 := "connect() failed (111: Connection refused)"
+			// FIXME(salonichf5) remove this error message check
+			// when https://github.com/nginxinc/nginx-gateway-fabric/issues/2090 is completed.
+			expectedError2 := "no live upstreams while connecting to upstream"
+			Expect(line).To(Or(ContainSubstring(expectedError1), ContainSubstring(expectedError2)))
 		}
 	}
 
@@ -274,7 +287,14 @@ func checkContainerLogsForErrors(ngfPodName string) {
 		&core.PodLogOptions{Container: ngfContainerName},
 	)
 	Expect(err).ToNot(HaveOccurred())
-	Expect(logs).ToNot(ContainSubstring("\"level\":\"error\""), logs)
+
+	for _, line := range strings.Split(logs, "\n") {
+		if *plusEnabled && strings.Contains(line, "\"level\":\"error\"") {
+			Expect(line).To(ContainSubstring("Usage reporting must be enabled when using NGINX Plus"), line)
+		} else {
+			Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line)
+		}
+	}
 }
 
 func checkLeaderLeaseChange(originalLeaseName string) error {
@@ -314,7 +334,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 +353,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")