Add updates to functional tests

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) {
@@ -87,8 +88,13 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger,
 				panic(fmt.Errorf("unknown resource type %T", e.Resource))
 			}
 		case *events.DeleteEvent:
+			obj := e
 			switch e.Type.(type) {
 			case *gatewayv1.Gateway:
+				if !h.provisioner.isLeader() {
+					h.provisioner.setResourceToDelete(obj.NamespacedName)
+				}
+
 				if err := h.provisioner.deprovisionNginx(ctx, e.NamespacedName); err != nil {
 					logger.Error(err, "error deprovisioning nginx resources")
 				}

internal/mode/static/provisioner/handler_test.go

@@ -9,6 +9,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
 
@@ -164,7 +165,25 @@ func TestHandleEventBatch_Delete(t *testing.T) {
 
 	g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(deployment), &appsv1.Deployment{})).To(Succeed())
 
-	// delete Gateway
+	// delete Gateway when provisioner is not leader
+	provisioner.leader = false
+
+	deleteEvent = &events.DeleteEvent{Type: gateway, NamespacedName: client.ObjectKeyFromObject(gateway)}
+	batch = events.EventBatch{deleteEvent}
+	handler.HandleEventBatch(ctx, logger, batch)
+
+	g.Expect(provisioner.resourcesToDeleteOnStartup).To(Equal([]types.NamespacedName{
+		{
+			Namespace: "default",
+			Name:      "gw",
+		},
+	}))
+	g.Expect(store.getGateway(client.ObjectKeyFromObject(gateway))).To(BeNil())
+	g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(deployment), &appsv1.Deployment{})).To(Succeed())
+
+	// delete Gateway when provisioner is leader
+	provisioner.leader = true
+
 	deleteEvent = &events.DeleteEvent{Type: gateway, NamespacedName: client.ObjectKeyFromObject(gateway)}
 	batch = events.EventBatch{deleteEvent}
 	handler.HandleEventBatch(ctx, logger, batch)

internal/mode/static/provisioner/objects.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"maps"
+	"sort"
 	"strconv"
 	"time"
 
@@ -43,6 +44,10 @@ func (p *NginxProvisioner) buildNginxResourceObjects(
 	gateway *gatewayv1.Gateway,
 	nProxyCfg *graph.EffectiveNginxProxy,
 ) ([]client.Object, error) {
+	// Need to ensure nginx resource objects are generated deterministically. Specifically when generating
+	// an object's field by ranging over a map, since ranging over a map is done in random order, we need to
+	// do some processing to ensure the generated results are the same each time.
+
 	ngxIncludesConfigMapName := controller.CreateNginxResourceName(resourceName, nginxIncludesConfigMapNameSuffix)
 	ngxAgentConfigMapName := controller.CreateNginxResourceName(resourceName, nginxAgentConfigMapNameSuffix)
 
@@ -174,6 +179,12 @@ func (p *NginxProvisioner) buildNginxSecrets(
 		}
 	}
 
+	// need to sort secrets so everytime buildNginxSecrets is called it will generate the exact same
+	// array of secrets. This is needed to satisfy deterministic results of the method.
+	sort.Slice(secrets, func(i, j int) bool {
+		return secrets[i].GetName() < secrets[j].GetName()
+	})
+
 	if jwtSecretName != "" {
 		newSecret, err := p.getAndUpdateSecret(
 			p.cfg.PlusUsageConfig.SecretName,
@@ -358,6 +369,12 @@ func buildNginxService(
 		servicePorts = append(servicePorts, servicePort)
 	}
 
+	// need to sort ports so everytime buildNginxService is called it will generate the exact same
+	// array of ports. This is needed to satisfy deterministic results of the method.
+	sort.Slice(servicePorts, func(i, j int) bool {
+		return servicePorts[i].Port < servicePorts[j].Port
+	})
+
 	svc := &corev1.Service{
 		ObjectMeta: objectMeta,
 		Spec: corev1.ServiceSpec{
@@ -467,6 +484,12 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec(
 		podAnnotations["prometheus.io/port"] = strconv.Itoa(int(metricsPort))
 	}
 
+	// need to sort ports so everytime buildNginxPodTemplateSpec is called it will generate the exact same
+	// array of ports. This is needed to satisfy deterministic results of the method.
+	sort.Slice(containerPorts, func(i, j int) bool {
+		return containerPorts[i].ContainerPort < containerPorts[j].ContainerPort
+	})
+
 	image, pullPolicy := p.buildImage(nProxyCfg)
 
 	spec := corev1.PodTemplateSpec{
@@ -622,6 +645,12 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec(
 		spec.Spec.ImagePullSecrets = append(spec.Spec.ImagePullSecrets, ref)
 	}
 
+	// need to sort ports so everytime buildNginxPodTemplateSpec is called it will generate the exact same
+	// array of secrets. This is needed to satisfy deterministic results of the method.
+	sort.Slice(spec.Spec.ImagePullSecrets, func(i, j int) bool {
+		return spec.Spec.ImagePullSecrets[i].Name < spec.Spec.ImagePullSecrets[j].Name
+	})
+
 	if p.cfg.Plus {
 		initCmd := spec.Spec.InitContainers[0].Command
 		initCmd = append(initCmd,

internal/mode/static/provisioner/objects_test.go

@@ -59,6 +59,12 @@ func TestBuildNginxResourceObjects(t *testing.T) {
 				{
 					Port: 80,
 				},
+				{
+					Port: 8888,
+				},
+				{
+					Port: 9999,
+				},
 			},
 		},
 	}
@@ -116,10 +122,24 @@ func TestBuildNginxResourceObjects(t *testing.T) {
 	validateMeta(svc)
 	g.Expect(svc.Spec.Type).To(Equal(defaultServiceType))
 	g.Expect(svc.Spec.ExternalTrafficPolicy).To(Equal(defaultServicePolicy))
-	g.Expect(svc.Spec.Ports).To(ContainElement(corev1.ServicePort{
-		Port:       80,
-		Name:       "port-80",
-		TargetPort: intstr.FromInt(80),
+
+	// service ports is sorted in ascending order by port number when we make the nginx object
+	g.Expect(svc.Spec.Ports).To(Equal([]corev1.ServicePort{
+		{
+			Port:       80,
+			Name:       "port-80",
+			TargetPort: intstr.FromInt(80),
+		},
+		{
+			Port:       8888,
+			Name:       "port-8888",
+			TargetPort: intstr.FromInt(8888),
+		},
+		{
+			Port:       9999,
+			Name:       "port-9999",
+			TargetPort: intstr.FromInt(9999),
+		},
 	}))
 
 	depObj := objects[4]
@@ -132,13 +152,24 @@ func TestBuildNginxResourceObjects(t *testing.T) {
 	g.Expect(template.Spec.Containers).To(HaveLen(1))
 	container := template.Spec.Containers[0]
 
-	g.Expect(container.Ports).To(ContainElement(corev1.ContainerPort{
-		ContainerPort: config.DefaultNginxMetricsPort,
-		Name:          "metrics",
-	}))
-	g.Expect(container.Ports).To(ContainElement(corev1.ContainerPort{
-		ContainerPort: 80,
-		Name:          "port-80",
+	// container ports is sorted in ascending order by port number when we make the nginx object
+	g.Expect(container.Ports).To(Equal([]corev1.ContainerPort{
+		{
+			ContainerPort: 80,
+			Name:          "port-80",
+		},
+		{
+			ContainerPort: 8888,
+			Name:          "port-8888",
+		},
+		{
+			ContainerPort: config.DefaultNginxMetricsPort,
+			Name:          "metrics",
+		},
+		{
+			ContainerPort: 9999,
+			Name:          "port-9999",
+		},
 	}))
 
 	g.Expect(container.Image).To(Equal(fmt.Sprintf("%s:1.0.0", defaultNginxImagePath)))
@@ -415,14 +446,32 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) {
 		},
 		Data: map[string][]byte{"data": []byte("docker")},
 	}
-	fakeClient := fake.NewFakeClient(dockerSecret)
+
+	dockerSecretTeaName := dockerTestSecretName + "-Tea"
+	dockerSecretTea := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      dockerSecretTeaName,
+			Namespace: ngfNamespace,
+		},
+		Data: map[string][]byte{"data": []byte("docker-Tea")},
+	}
+
+	dockerSecretChaiName := dockerTestSecretName + "-Chai"
+	dockerSecretChai := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      dockerSecretChaiName,
+			Namespace: ngfNamespace,
+		},
+		Data: map[string][]byte{"data": []byte("docker-Chai")},
+	}
+	fakeClient := fake.NewFakeClient(dockerSecret, dockerSecretTea, dockerSecretChai)
 
 	provisioner := &NginxProvisioner{
 		cfg: Config{
 			GatewayPodConfig: &config.GatewayPodConfig{
 				Namespace: ngfNamespace,
 			},
-			NginxDockerSecretNames: []string{dockerTestSecretName},
+			NginxDockerSecretNames: []string{dockerTestSecretName, dockerSecretTeaName, dockerSecretChaiName},
 		},
 		k8sClient: fakeClient,
 		baseLabelSelector: metav1.LabelSelector{
@@ -443,26 +492,49 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) {
 	objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, &graph.EffectiveNginxProxy{})
 	g.Expect(err).ToNot(HaveOccurred())
 
-	g.Expect(objects).To(HaveLen(6))
+	g.Expect(objects).To(HaveLen(8))
 
 	expLabels := map[string]string{
 		"app":                                    "nginx",
 		"gateway.networking.k8s.io/gateway-name": "gw",
 		"app.kubernetes.io/name":                 "gw-nginx",
 	}
 
+	// the (docker-only) secret order in the object list is sorted by secret name
+
 	secretObj := objects[0]
 	secret, ok := secretObj.(*corev1.Secret)
 	g.Expect(ok).To(BeTrue())
 	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerTestSecretName)))
 	g.Expect(secret.GetLabels()).To(Equal(expLabels))
 
-	depObj := objects[5]
+	chaiSecretObj := objects[1]
+	secret, ok = chaiSecretObj.(*corev1.Secret)
+	g.Expect(ok).To(BeTrue())
+	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretChaiName)))
+	g.Expect(secret.GetLabels()).To(Equal(expLabels))
+
+	teaSecretObj := objects[2]
+	secret, ok = teaSecretObj.(*corev1.Secret)
+	g.Expect(ok).To(BeTrue())
+	g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretTeaName)))
+	g.Expect(secret.GetLabels()).To(Equal(expLabels))
+
+	depObj := objects[7]
 	dep, ok := depObj.(*appsv1.Deployment)
 	g.Expect(ok).To(BeTrue())
 
-	g.Expect(dep.Spec.Template.Spec.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{
-		Name: controller.CreateNginxResourceName(resourceName, dockerTestSecretName),
+	// imagePullSecrets is sorted by name when we make the nginx object
+	g.Expect(dep.Spec.Template.Spec.ImagePullSecrets).To(Equal([]corev1.LocalObjectReference{
+		{
+			Name: controller.CreateNginxResourceName(resourceName, dockerTestSecretName),
+		},
+		{
+			Name: controller.CreateNginxResourceName(resourceName, dockerSecretChaiName),
+		},
+		{
+			Name: controller.CreateNginxResourceName(resourceName, dockerSecretTeaName),
+		},
 	}))
 }
 

internal/mode/static/telemetry/collector.go

@@ -145,10 +145,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) {
 		return Data{}, fmt.Errorf("failed to collect cluster information: %w", err)
 	}
 
-	graphResourceCount, err := collectGraphResourceCount(g, c.cfg.ConfigurationGetter)
-	if err != nil {
-		return Data{}, fmt.Errorf("failed to collect NGF resource counts: %w", err)
-	}
+	graphResourceCount := collectGraphResourceCount(g, c.cfg.ConfigurationGetter)
 
 	replicaSet, err := getPodReplicaSet(ctx, c.cfg.K8sClientReader, c.cfg.PodNSName)
 	if err != nil {
@@ -193,14 +190,10 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) {
 func collectGraphResourceCount(
 	g *graph.Graph,
 	configurationGetter ConfigurationGetter,
-) (NGFResourceCounts, error) {
+) NGFResourceCounts {
 	ngfResourceCounts := NGFResourceCounts{}
 	cfg := configurationGetter.GetLatestConfiguration()
 
-	if cfg == nil {
-		return ngfResourceCounts, errors.New("latest configuration cannot be nil")
-	}
-
 	ngfResourceCounts.GatewayClassCount = int64(len(g.IgnoredGatewayClasses))
 	if g.GatewayClass != nil {
 		ngfResourceCounts.GatewayClassCount++
@@ -219,9 +212,11 @@ func collectGraphResourceCount(
 	ngfResourceCounts.SecretCount = int64(len(g.ReferencedSecrets))
 	ngfResourceCounts.ServiceCount = int64(len(g.ReferencedServices))
 
-	for _, upstream := range cfg.Upstreams {
-		if upstream.ErrorMsg == "" {
-			ngfResourceCounts.EndpointCount += int64(len(upstream.Endpoints))
+	if cfg != nil {
+		for _, upstream := range cfg.Upstreams {
+			if upstream.ErrorMsg == "" {
+				ngfResourceCounts.EndpointCount += int64(len(upstream.Endpoints))
+			}
 		}
 	}
 
@@ -249,7 +244,7 @@ func collectGraphResourceCount(
 	ngfResourceCounts.NginxProxyCount = int64(len(g.ReferencedNginxProxies))
 	ngfResourceCounts.SnippetsFilterCount = int64(len(g.SnippetsFilters))
 
-	return ngfResourceCounts, nil
+	return ngfResourceCounts
 }
 
 type RouteCounts struct {

internal/mode/static/telemetry/collector_test.go

@@ -728,14 +728,6 @@ var _ = Describe("Collector", Ordered, func() {
 					_, err := dataCollector.Collect(ctx)
 					Expect(err).To(MatchError(expectedError))
 				})
-
-				It("should error on nil latest configuration", func(ctx SpecContext) {
-					expectedError := errors.New("latest configuration cannot be nil")
-					fakeConfigurationGetter.GetLatestConfigurationReturns(nil)
-
-					_, err := dataCollector.Collect(ctx)
-					Expect(err).To(MatchError(expectedError))
-				})
 			})
 		})
 	})

tests/framework/collector.go

@@ -28,6 +28,14 @@ func InstallCollector() ([]byte, error) {
 		return output, err
 	}
 
+	if output, err := exec.Command(
+		"helm",
+		"repo",
+		"update",
+	).CombinedOutput(); err != nil {
+		return output, fmt.Errorf("failed to update helm repos: %w; output: %s", err, string(output))
+	}
+
 	args := []string{
 		"install",
 		collectorChartReleaseName,

tests/framework/crossplane.go

@@ -203,7 +203,7 @@ func injectCrossplaneContainer(
 func createCrossplaneExecutor(
 	k8sClient kubernetes.Interface,
 	k8sConfig *rest.Config,
-	ngfPodName,
+	nginxPodName,
 	namespace string,
 ) (remotecommand.Executor, error) {
 	cmd := []string{"./crossplane", "/etc/nginx/nginx.conf"}
@@ -217,7 +217,7 @@ func createCrossplaneExecutor(
 	req := k8sClient.CoreV1().RESTClient().Post().
 		Resource("pods").
 		SubResource("exec").
-		Name(ngfPodName).
+		Name(nginxPodName).
 		Namespace(namespace).
 		VersionedParams(opts, scheme.ParameterCodec)