Run pipeline

Author: bjee19

internal/mode/static/provisioner/handler.go

@@ -49,6 +49,7 @@ func newEventHandler(
 	}, nil
 }
 
+//nolint:gocyclo // will refactor at some point
 func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, batch events.EventBatch) {
 	for _, event := range batch {
 		switch e := event.(type) {

internal/mode/static/provisioner/provisioner.go

@@ -308,7 +308,7 @@ func (p *NginxProvisioner) deprovisionNginx(ctx context.Context, gatewayNSName t
 		Name:      controller.CreateNginxResourceName(gatewayNSName.Name, p.cfg.GCName),
 		Namespace: gatewayNSName.Namespace,
 	}
-	
+
 	if p.isLeader() {
 		p.cfg.Logger.Info(
 			"Removing nginx resources for Gateway",

internal/mode/static/telemetry/collector.go

@@ -194,13 +194,6 @@ func collectGraphResourceCount(
 	ngfResourceCounts := NGFResourceCounts{}
 	cfg := configurationGetter.GetLatestConfiguration()
 
-	// might need to change this and make this optional, if cfg is nil then we can't report on upstream count but
-	// we can do everything else.
-
-	// if cfg == nil {
-	//	return ngfResourceCounts, errors.New("latest configuration cannot be nil")
-	//}
-
 	ngfResourceCounts.GatewayClassCount = int64(len(g.IgnoredGatewayClasses))
 	if g.GatewayClass != nil {
 		ngfResourceCounts.GatewayClassCount++

tests/suite/graceful_recovery_test.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"net/http"
 	"os/exec"
-	"slices"
 	"strings"
 	"time"
 
@@ -65,61 +64,6 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 		return nil
 	}
 
-	// var ngfPodName string
-
-	BeforeEach(func() {
-		// this test is unique in that it will check the entire log of both ngf and nginx containers
-		// for any errors, so in order to avoid errors generated in previous tests we will uninstall
-		// NGF installed at the suite level, then re-deploy our own. We will also uninstall and re-install
-		// NGF between each graceful-recovery test for the same reason.
-		teardown(releaseName)
-
-		setup(getDefaultSetupCfg())
-
-		podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout)
-		Expect(err).ToNot(HaveOccurred())
-		Expect(podNames).To(HaveLen(1))
-
-		ns = core.Namespace{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "graceful-recovery",
-			},
-		}
-
-		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())
-
-		nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, ns.Name, timeoutConfig.GetTimeout)
-		Expect(err).ToNot(HaveOccurred())
-		Expect(nginxPodNames).To(HaveLen(1))
-
-		setUpPortForward(nginxPodNames[0], ns.Name)
-
-		if portFwdPort != 0 {
-			coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHTTPURL, portFwdPort)
-		}
-		if portFwdHTTPSPort != 0 {
-			teaURL = fmt.Sprintf("%s:%d/tea", baseHTTPSURL, portFwdHTTPSPort)
-		}
-
-		Eventually(
-			func() error {
-				return checkForWorkingTraffic(teaURL, coffeeURL)
-			}).
-			WithTimeout(timeoutConfig.TestForTrafficTimeout).
-			WithPolling(500 * time.Millisecond).
-			Should(Succeed())
-	})
-
-	AfterEach(func() {
-		cleanUpPortForward()
-
-		Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed())
-		Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed())
-	})
-
 	getContainerRestartCount := func(podName, namespace, containerName string) (int, error) {
 		ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout)
 		defer cancel()
@@ -234,7 +178,7 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 		)).To(Succeed())
 	}
 
-	checkNGFFunctionality := func(teaURL, coffeeURL, ngfPodName, containerName string, files []string, ns *core.Namespace) {
+	checkNGFFunctionality := func(teaURL, coffeeURL, _ string, files []string, ns *core.Namespace) {
 		Eventually(
 			func() error {
 				return checkForWorkingTraffic(teaURL, coffeeURL)
@@ -256,7 +200,6 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 
 		Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed())
 		Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed())
-		// Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed())
 
 		var nginxPodNames []string
 		var err error
@@ -342,7 +285,12 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 		var podNames []string
 		Eventually(
 			func() bool {
-				podNames, err = framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetStatusTimeout)
+				podNames, err = framework.GetReadyNGFPodNames(
+					k8sClient,
+					ngfNamespace,
+					releaseName,
+					timeoutConfig.GetStatusTimeout,
+				)
 				return len(podNames) == 1 && err == nil
 			}).
 			WithTimeout(timeoutConfig.CreateTimeout * 2).
@@ -369,7 +317,7 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 
 		setUpPortForward(nginxPodName, ns.Name)
 
-		checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, "", files, ns)
+		checkNGFFunctionality(teaURL, coffeeURL, "", files, ns)
 		// if errorLogs := getUnexpectedNginxErrorLogs(ngfPodName, ns.Name); errorLogs != "" {
 		//	Skip(fmt.Sprintf("NGINX has unexpected error logs: \n%s", errorLogs))
 		//}
@@ -383,6 +331,92 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 		runRestartNodeTest(teaURL, coffeeURL, files, ns, false)
 	}
 
+	getLeaderElectionLeaseHolderName := func() (string, error) {
+		ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout)
+		defer cancel()
+
+		var lease coordination.Lease
+		key := types.NamespacedName{Name: "ngf-test-nginx-gateway-fabric-leader-election", Namespace: ngfNamespace}
+
+		if err := k8sClient.Get(ctx, key, &lease); err != nil {
+			return "", errors.New("could not retrieve leader election lease")
+		}
+
+		if *lease.Spec.HolderIdentity == "" {
+			return "", errors.New("leader election lease holder identity is empty")
+		}
+
+		return *lease.Spec.HolderIdentity, nil
+	}
+
+	checkLeaderLeaseChange := func(originalLeaseName string) error {
+		leaseName, err := getLeaderElectionLeaseHolderName()
+		if err != nil {
+			return err
+		}
+
+		if originalLeaseName == leaseName {
+			return fmt.Errorf(
+				"expected originalLeaseName: %s, to not match current leaseName: %s",
+				originalLeaseName,
+				leaseName,
+			)
+		}
+
+		return nil
+	}
+
+	BeforeEach(func() {
+		// this test is unique in that it will check the entire log of both ngf and nginx containers
+		// for any errors, so in order to avoid errors generated in previous tests we will uninstall
+		// NGF installed at the suite level, then re-deploy our own. We will also uninstall and re-install
+		// NGF between each graceful-recovery test for the same reason.
+		teardown(releaseName)
+
+		setup(getDefaultSetupCfg())
+
+		podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(podNames).To(HaveLen(1))
+
+		ns = core.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "graceful-recovery",
+			},
+		}
+
+		Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed())
+		Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed())
+		Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed())
+
+		nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, ns.Name, timeoutConfig.GetTimeout)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(nginxPodNames).To(HaveLen(1))
+
+		setUpPortForward(nginxPodNames[0], ns.Name)
+
+		if portFwdPort != 0 {
+			coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHTTPURL, portFwdPort)
+		}
+		if portFwdHTTPSPort != 0 {
+			teaURL = fmt.Sprintf("%s:%d/tea", baseHTTPSURL, portFwdHTTPSPort)
+		}
+
+		Eventually(
+			func() error {
+				return checkForWorkingTraffic(teaURL, coffeeURL)
+			}).
+			WithTimeout(timeoutConfig.TestForTrafficTimeout).
+			WithPolling(500 * time.Millisecond).
+			Should(Succeed())
+	})
+
+	AfterEach(func() {
+		cleanUpPortForward()
+		Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed())
+		Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed())
+	})
+
 	It("recovers when nginx container is restarted", func() {
 		nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, ns.Name, timeoutConfig.GetTimeout)
 		Expect(err).ToNot(HaveOccurred())
@@ -398,7 +432,7 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 
 		setUpPortForward(nginxPodNames[0], ns.Name)
 
-		checkNGFFunctionality(teaURL, coffeeURL, nginxPodName, nginxContainerName, files, &ns)
+		checkNGFFunctionality(teaURL, coffeeURL, nginxContainerName, files, &ns)
 		// if errorLogs := getUnexpectedNginxErrorLogs(nginxPodName, ns.Name); errorLogs != "" {
 		//	Skip(fmt.Sprintf("NGINX has unexpected error logs: \n%s", errorLogs))
 		//}
@@ -417,6 +451,9 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 		ngfPod, err := resourceManager.GetPod(ngfNamespace, startingNGFPodNames[0])
 		Expect(err).ToNot(HaveOccurred())
 
+		leaseName, err := getLeaderElectionLeaseHolderName()
+		Expect(err).ToNot(HaveOccurred())
+
 		ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.DeleteTimeout)
 		defer cancel()
 
@@ -443,7 +480,15 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"),
 
 		Expect(newNGFPodName).ToNot(Equal(startingNGFPodNames[0]))
 
-		checkNGFFunctionality(teaURL, coffeeURL, newNGFPodName, "", files, &ns)
+		Eventually(
+			func() error {
+				return checkLeaderLeaseChange(leaseName)
+			}).
+			WithTimeout(timeoutConfig.GetLeaderLeaseTimeout).
+			WithPolling(500 * time.Millisecond).
+			Should(Succeed())
+
+		checkNGFFunctionality(teaURL, coffeeURL, "", files, &ns)
 	})
 
 	It("recovers when drained node is restarted", func() {
@@ -548,30 +593,30 @@ func getNginxErrorLogs(nginxPodName, namespace string) string {
 	return errorLogs
 }
 
-func getUnexpectedNginxErrorLogs(nginxPodName, namespace string) string {
-	expectedErrStrings := []string{
-		"connect() failed (111: Connection refused)",
-		"could not be resolved (host not found) during usage report",
-		"server returned 429",
-		// FIXME(salonichf5) remove this error message check
-		// when https://github.com/nginx/nginx-gateway-fabric/issues/2090 is completed.
-		"no live upstreams while connecting to upstream",
-	}
-
-	unexpectedErrors := ""
-
-	errorLogs := getNginxErrorLogs(nginxPodName, namespace)
-
-	for _, line := range strings.Split(errorLogs, "\n") {
-		if !slices.ContainsFunc(expectedErrStrings, func(s string) bool {
-			return strings.Contains(line, s)
-		}) {
-			unexpectedErrors += line
-		}
-	}
-
-	return unexpectedErrors
-}
+// func getUnexpectedNginxErrorLogs(nginxPodName, namespace string) string {
+//	expectedErrStrings := []string{
+//		"connect() failed (111: Connection refused)",
+//		"could not be resolved (host not found) during usage report",
+//		"server returned 429",
+//		// FIXME(salonichf5) remove this error message check
+//		// when https://github.com/nginx/nginx-gateway-fabric/issues/2090 is completed.
+//		"no live upstreams while connecting to upstream",
+//	}
+//
+//	unexpectedErrors := ""
+//
+//	errorLogs := getNginxErrorLogs(nginxPodName, namespace)
+//
+//	for _, line := range strings.Split(errorLogs, "\n") {
+//		if !slices.ContainsFunc(expectedErrStrings, func(s string) bool {
+//			return strings.Contains(line, s)
+//		}) {
+//			unexpectedErrors += line
+//		}
+//	}
+//
+//	return unexpectedErrors
+//}
 
 // checkNGFContainerLogsForErrors checks NGF container's logs for any possible errors.
 func checkNGFContainerLogsForErrors(ngfPodName string) {
@@ -586,34 +631,3 @@ func checkNGFContainerLogsForErrors(ngfPodName string) {
 		Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line)
 	}
 }
-
-func checkLeaderLeaseChange(originalLeaseName string) error {
-	leaseName, err := getLeaderElectionLeaseHolderName()
-	if err != nil {
-		return err
-	}
-
-	if originalLeaseName == leaseName {
-		return fmt.Errorf("expected originalLeaseName: %s, to not match current leaseName: %s", originalLeaseName, leaseName)
-	}
-
-	return nil
-}
-
-func getLeaderElectionLeaseHolderName() (string, error) {
-	ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout)
-	defer cancel()
-
-	var lease coordination.Lease
-	key := types.NamespacedName{Name: "ngf-test-nginx-gateway-fabric-leader-election", Namespace: ngfNamespace}
-
-	if err := k8sClient.Get(ctx, key, &lease); err != nil {
-		return "", errors.New("could not retrieve leader election lease")
-	}
-
-	if *lease.Spec.HolderIdentity == "" {
-		return "", errors.New("leader election lease holder identity is empty")
-	}
-
-	return *lease.Spec.HolderIdentity, nil
-}

tests/suite/system_suite_test.go

@@ -321,7 +321,7 @@ var _ = BeforeSuite(func() {
 		"telemetry",          // - running telemetry test (NGF will be deployed as part of the test)
 		"scale",              // - running scale test (this test will deploy its own version)
 		"reconfiguration",    // - running reconfiguration test (test will deploy its own instances)
-		// "graceful-recovery",
+		"graceful-recovery",
 	}
 	for _, s := range skipSubstrings {
 		if strings.Contains(labelFilter, s) {

tests/suite/tracing_test.go

@@ -24,21 +24,20 @@ import (
 	"github.com/nginx/nginx-gateway-fabric/tests/framework"
 )
 
-// To run the tracing test, you must build NGF with the following values:
-// TELEMETRY_ENDPOINT=otel-collector-opentelemetry-collector.collector.svc.cluster.local:4317
-// TELEMETRY_ENDPOINT_INSECURE = true
-//
 // This test can be flaky when waiting to see traces show up in the collector logs.
 // Sometimes they get there right away, sometimes it takes 30 seconds. Retries were
 // added to attempt to mitigate the issue, but it didn't fix it 100%.
 var _ = Describe("Tracing", FlakeAttempts(2), Ordered, Label("functional", "tracing"), func() {
+	// To run the tracing test, you must build NGF with the following values:
+	// TELEMETRY_ENDPOINT=otel-collector-opentelemetry-collector.collector.svc.cluster.local:4317
+	// TELEMETRY_ENDPOINT_INSECURE = true
+
 	var (
 		files = []string{
 			"hello-world/apps.yaml",
 			"hello-world/gateway.yaml",
 			"hello-world/routes.yaml",
 		}
-		// nginxProxyFile     = "tracing/nginxproxy.yaml"
 		policySingleFile   = "tracing/policy-single.yaml"
 		policyMultipleFile = "tracing/policy-multiple.yaml"