diff --git a/internal/mode/static/provisioner/handler.go b/internal/mode/static/provisioner/handler.go index b3d7fe5efd..7757cec043 100644 --- a/internal/mode/static/provisioner/handler.go +++ b/internal/mode/static/provisioner/handler.go @@ -106,6 +106,10 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger, case *events.DeleteEvent: switch e.Type.(type) { case *gatewayv1.Gateway: + if !h.provisioner.isLeader() { + h.provisioner.setResourceToDelete(e.NamespacedName) + } + if err := h.provisioner.deprovisionNginx(ctx, e.NamespacedName); err != nil { logger.Error(err, "error deprovisioning nginx resources") } diff --git a/internal/mode/static/provisioner/handler_test.go b/internal/mode/static/provisioner/handler_test.go index 720690b972..fe1d63e9be 100644 --- a/internal/mode/static/provisioner/handler_test.go +++ b/internal/mode/static/provisioner/handler_test.go @@ -316,7 +316,25 @@ func TestHandleEventBatch_Delete(t *testing.T) { verifySecret(clientTestSecretName, userClientSSLSecret) verifySecret(dockerTestSecretName, userDockerSecret) - // 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) diff --git a/internal/mode/static/provisioner/objects.go b/internal/mode/static/provisioner/objects.go index dc73164a40..5e9710efd5 100644 --- a/internal/mode/static/provisioner/objects.go +++ b/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 secret names 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, diff --git a/internal/mode/static/provisioner/objects_test.go b/internal/mode/static/provisioner/objects_test.go index 27fba0d734..f59f4ce253 100644 --- a/internal/mode/static/provisioner/objects_test.go +++ b/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) + + dockerSecretRegistry1Name := dockerTestSecretName + "-registry1" + dockerSecretRegistry1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: dockerSecretRegistry1Name, + Namespace: ngfNamespace, + }, + Data: map[string][]byte{"data": []byte("docker-registry1")}, + } + + dockerSecretRegistry2Name := dockerTestSecretName + "-registry2" + dockerSecretRegistry2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: dockerSecretRegistry2Name, + Namespace: ngfNamespace, + }, + Data: map[string][]byte{"data": []byte("docker-registry2")}, + } + fakeClient := fake.NewFakeClient(dockerSecret, dockerSecretRegistry1, dockerSecretRegistry2) provisioner := &NginxProvisioner{ cfg: Config{ GatewayPodConfig: &config.GatewayPodConfig{ Namespace: ngfNamespace, }, - NginxDockerSecretNames: []string{dockerTestSecretName}, + NginxDockerSecretNames: []string{dockerTestSecretName, dockerSecretRegistry1Name, dockerSecretRegistry2Name}, }, k8sClient: fakeClient, baseLabelSelector: metav1.LabelSelector{ @@ -443,7 +492,7 @@ 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", @@ -451,18 +500,41 @@ func TestBuildNginxResourceObjects_DockerSecrets(t *testing.T) { "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] + registry1SecretObj := objects[1] + secret, ok = registry1SecretObj.(*corev1.Secret) + g.Expect(ok).To(BeTrue()) + g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretRegistry1Name))) + g.Expect(secret.GetLabels()).To(Equal(expLabels)) + + registry2SecretObj := objects[2] + secret, ok = registry2SecretObj.(*corev1.Secret) + g.Expect(ok).To(BeTrue()) + g.Expect(secret.GetName()).To(Equal(controller.CreateNginxResourceName(resourceName, dockerSecretRegistry2Name))) + 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, dockerSecretRegistry1Name), + }, + { + Name: controller.CreateNginxResourceName(resourceName, dockerSecretRegistry2Name), + }, })) } diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index facc7dc56a..1be3accf14 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/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 { diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 9935920d4f..80dcd8b077 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/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)) - }) }) }) }) diff --git a/tests/framework/collector.go b/tests/framework/collector.go index bd4b1bd398..9eb16b3cb9 100644 --- a/tests/framework/collector.go +++ b/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, diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index 81f47e3567..f2ada703c5 100644 --- a/tests/framework/crossplane.go +++ b/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) diff --git a/tests/framework/portforward.go b/tests/framework/portforward.go index 26cd4b3cfb..500dc354aa 100644 --- a/tests/framework/portforward.go +++ b/tests/framework/portforward.go @@ -52,13 +52,13 @@ func PortForward(config *rest.Config, namespace, podName string, ports []string, for { if err := forward(); err != nil { slog.Error("error forwarding ports", "error", err) - slog.Info("retrying port forward in 100ms...") + slog.Info("retrying port forward in 1s...") } select { case <-stopCh: return - case <-time.After(100 * time.Millisecond): + case <-time.After(1 * time.Second): // retrying } } diff --git a/tests/framework/resourcemanager.go b/tests/framework/resourcemanager.go index 434a5ecaed..50aab0653e 100644 --- a/tests/framework/resourcemanager.go +++ b/tests/framework/resourcemanager.go @@ -701,22 +701,57 @@ func GetReadyNGFPodNames( "app.kubernetes.io/instance": releaseName, }, ); err != nil { - return nil, fmt.Errorf("error getting list of Pods: %w", err) + return nil, fmt.Errorf("error getting list of NGF Pods: %w", err) } - if len(podList.Items) > 0 { - var names []string - for _, pod := range podList.Items { - for _, cond := range pod.Status.Conditions { - if cond.Type == core.PodReady && cond.Status == core.ConditionTrue { - names = append(names, pod.Name) - } + if len(podList.Items) == 0 { + return nil, errors.New("unable to find NGF Pod(s)") + } + + names := getReadyPodNames(podList) + + return names, nil +} + +// GetReadyNginxPodNames returns the name(s) of the NGINX Pod(s). +func GetReadyNginxPodNames( + k8sClient client.Client, + namespace string, + timeout time.Duration, +) ([]string, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + var podList core.PodList + if err := k8sClient.List( + ctx, + &podList, + client.InNamespace(namespace), + client.HasLabels{"gateway.networking.k8s.io/gateway-name"}, + ); err != nil { + return nil, fmt.Errorf("error getting list of NGINX Pods: %w", err) + } + + if len(podList.Items) == 0 { + return nil, errors.New("unable to find NGINX Pod(s)") + } + + names := getReadyPodNames(podList) + + return names, nil +} + +func getReadyPodNames(podList core.PodList) []string { + var names []string + for _, pod := range podList.Items { + for _, cond := range pod.Status.Conditions { + if cond.Type == core.PodReady && cond.Status == core.ConditionTrue { + names = append(names, pod.Name) } } - return names, nil } - return nil, errors.New("unable to find NGF Pod(s)") + return names } func countNumberOfReadyParents(parents []v1.RouteParentStatus) int { @@ -733,34 +768,7 @@ 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 +// WaitForPodsToBeReadyWithCount 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( @@ -817,17 +825,17 @@ func (rm *ResourceManager) WaitForGatewayObservedGeneration( } // GetNginxConfig uses crossplane to get the nginx configuration and convert it to JSON. -func (rm *ResourceManager) GetNginxConfig(ngfPodName, namespace string) (*Payload, error) { +func (rm *ResourceManager) GetNginxConfig(nginxPodName, namespace string) (*Payload, error) { if err := injectCrossplaneContainer( rm.ClientGoClient, rm.TimeoutConfig.UpdateTimeout, - ngfPodName, + nginxPodName, namespace, ); err != nil { return nil, err } - exec, err := createCrossplaneExecutor(rm.ClientGoClient, rm.K8sConfig, ngfPodName, namespace) + exec, err := createCrossplaneExecutor(rm.ClientGoClient, rm.K8sConfig, nginxPodName, namespace) if err != nil { return nil, err } diff --git a/tests/suite/advanced_routing_test.go b/tests/suite/advanced_routing_test.go index a58c9a7f7e..844e1db02c 100644 --- a/tests/suite/advanced_routing_test.go +++ b/tests/suite/advanced_routing_test.go @@ -39,9 +39,17 @@ var _ = Describe("AdvancedRouting", Ordered, Label("functional", "routing"), fun Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + setUpPortForward(nginxPodNames[0], namespace) }) AfterAll(func() { + cleanUpPortForward() + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) diff --git a/tests/suite/client_settings_test.go b/tests/suite/client_settings_test.go index f1f12304ee..835f3a9896 100644 --- a/tests/suite/client_settings_test.go +++ b/tests/suite/client_settings_test.go @@ -32,6 +32,8 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" } namespace = "clientsettings" + + nginxPodName string ) BeforeAll(func() { @@ -44,9 +46,19 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + nginxPodName = nginxPodNames[0] + + setUpPortForward(nginxPodName, namespace) }) AfterAll(func() { + cleanUpPortForward() + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) @@ -96,13 +108,8 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" filePrefix := fmt.Sprintf("/etc/nginx/includes/ClientSettingsPolicy_%s", namespace) BeforeAll(func() { - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) - - ngfPodName := podNames[0] - - conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + var err error + conf, err = resourceManager.GetNginxConfig(nginxPodName, namespace) Expect(err).ToNot(HaveOccurred()) }) diff --git a/tests/suite/dataplane_perf_test.go b/tests/suite/dataplane_perf_test.go index 5604b0188f..aa34131db1 100644 --- a/tests/suite/dataplane_perf_test.go +++ b/tests/suite/dataplane_perf_test.go @@ -18,44 +18,47 @@ import ( ) var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"), func() { - files := []string{ - "dp-perf/coffee.yaml", - "dp-perf/gateway.yaml", - "dp-perf/cafe-routes.yaml", - } - - var ns core.Namespace - - var addr string - targetURL := "http://cafe.example.com" - var outFile *os.File - - t1 := framework.Target{ - Method: "GET", - URL: fmt.Sprintf("%s%s", targetURL, "/latte"), - } - t2 := framework.Target{ - Method: "GET", - URL: fmt.Sprintf("%s%s", targetURL, "/coffee"), - Header: http.Header{"version": []string{"v2"}}, - } - t3 := framework.Target{ - Method: "GET", - URL: fmt.Sprintf("%s%s", targetURL, "/coffee?TEST=v2"), - } - t4 := framework.Target{ - Method: "GET", - URL: fmt.Sprintf("%s%s", targetURL, "/tea"), - } - t5 := framework.Target{ - Method: "POST", - URL: fmt.Sprintf("%s%s", targetURL, "/tea"), - } + var ( + files = []string{ + "dp-perf/coffee.yaml", + "dp-perf/gateway.yaml", + "dp-perf/cafe-routes.yaml", + } + + namespace = "dp-perf" + + targetURL = "http://cafe.example.com" + + t1 = framework.Target{ + Method: "GET", + URL: fmt.Sprintf("%s%s", targetURL, "/latte"), + } + t2 = framework.Target{ + Method: "GET", + URL: fmt.Sprintf("%s%s", targetURL, "/coffee"), + Header: http.Header{"version": []string{"v2"}}, + } + t3 = framework.Target{ + Method: "GET", + URL: fmt.Sprintf("%s%s", targetURL, "/coffee?TEST=v2"), + } + t4 = framework.Target{ + Method: "GET", + URL: fmt.Sprintf("%s%s", targetURL, "/tea"), + } + t5 = framework.Target{ + Method: "POST", + URL: fmt.Sprintf("%s%s", targetURL, "/tea"), + } + + outFile *os.File + addr string + ) BeforeAll(func() { - ns = core.Namespace{ + ns := core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "dp-perf", + Name: namespace, }, } @@ -63,6 +66,12 @@ var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"), Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + setUpPortForward(nginxPodNames[0], namespace) + port := ":80" if portFwdPort != 0 { port = fmt.Sprintf(":%s", strconv.Itoa(portFwdPort)) @@ -79,8 +88,10 @@ var _ = Describe("Dataplane performance", Ordered, Label("nfr", "performance"), }) AfterAll(func() { - Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed()) + cleanUpPortForward() + + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) outFile.Close() }) diff --git a/tests/suite/graceful_recovery_test.go b/tests/suite/graceful_recovery_test.go index 33c3c447d0..2e844f46e6 100644 --- a/tests/suite/graceful_recovery_test.go +++ b/tests/suite/graceful_recovery_test.go @@ -17,7 +17,6 @@ import ( core "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - ctlr "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/yaml" @@ -29,56 +28,162 @@ const ( ngfContainerName = "nginx-gateway" ) -// Since checkNGFContainerLogsForErrors may experience interference from previous tests (as explained in the function -// documentation), this test is recommended to be run separate from other tests. +// Since this test involves restarting of the test node, it is recommended to be run separate from other tests +// such that any issues in this test do not interfere with other tests. var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"), func() { - files := []string{ - "graceful-recovery/cafe.yaml", - "graceful-recovery/cafe-secret.yaml", - "graceful-recovery/gateway.yaml", - "graceful-recovery/cafe-routes.yaml", + var ( + files = []string{ + "graceful-recovery/cafe.yaml", + "graceful-recovery/cafe-secret.yaml", + "graceful-recovery/gateway.yaml", + "graceful-recovery/cafe-routes.yaml", + } + + ns core.Namespace + + baseHTTPURL = "http://cafe.example.com" + baseHTTPSURL = "https://cafe.example.com" + teaURL = baseHTTPSURL + "/tea" + coffeeURL = baseHTTPURL + "/coffee" + + activeNGFPodName, activeNginxPodName string + ) + + checkForWorkingTraffic := func(teaURL, coffeeURL string) error { + if err := expectRequestToSucceed(teaURL, address, "URI: /tea"); err != nil { + return err + } + if err := expectRequestToSucceed(coffeeURL, address, "URI: /coffee"); err != nil { + return err + } + return nil } - var ns core.Namespace + checkForFailingTraffic := func(teaURL, coffeeURL string) error { + if err := expectRequestToFail(teaURL, address); err != nil { + return err + } + if err := expectRequestToFail(coffeeURL, address); err != nil { + return err + } + return nil + } - baseHTTPURL := "http://cafe.example.com" - baseHTTPSURL := "https://cafe.example.com" - teaURL := baseHTTPSURL + "/tea" - coffeeURL := baseHTTPURL + "/coffee" + getContainerRestartCount := func(podName, namespace, containerName string) (int, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) + defer cancel() - var ngfPodName string + var pod core.Pod + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: podName}, &pod); err != nil { + return 0, fmt.Errorf("error retrieving Pod: %w", err) + } - 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) + var restartCount int + for _, containerStatus := range pod.Status.ContainerStatuses { + if containerStatus.Name == containerName { + restartCount = int(containerStatus.RestartCount) + } + } - setup(getDefaultSetupCfg()) + return restartCount, nil + } - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) + checkContainerRestart := func(podName, containerName, namespace string, currentRestartCount int) error { + restartCount, err := getContainerRestartCount(podName, namespace, containerName) + if err != nil { + return err + } - ngfPodName = podNames[0] - if portFwdPort != 0 { - coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHTTPURL, portFwdPort) + if restartCount != currentRestartCount+1 { + return fmt.Errorf("expected current restart count: %d to match incremented restart count: %d", + restartCount, currentRestartCount+1) } - if portFwdHTTPSPort != 0 { - teaURL = fmt.Sprintf("%s:%d/tea", baseHTTPSURL, portFwdHTTPSPort) + + return nil + } + + getNodeNames := func() ([]string, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) + defer cancel() + var nodes core.NodeList + + if err := k8sClient.List(ctx, &nodes); err != nil { + return nil, fmt.Errorf("error listing nodes: %w", err) } - ns = core.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "graceful-recovery", - }, + names := make([]string, 0, len(nodes.Items)) + + for _, node := range nodes.Items { + names = append(names, node.Name) } - Expect(resourceManager.Apply([]client.Object{&ns})).To(Succeed()) - Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed()) + return names, nil + } + + runNodeDebuggerJob := func(nginxPodName, jobScript string) (*v1.Job, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) + defer cancel() + var nginxPod core.Pod + if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns.Name, Name: nginxPodName}, &nginxPod); err != nil { + return nil, fmt.Errorf("error retrieving NGF Pod: %w", err) + } + + b, err := resourceManager.GetFileContents("graceful-recovery/node-debugger-job.yaml") + if err != nil { + return nil, fmt.Errorf("error processing node debugger job file: %w", err) + } + + job := &v1.Job{} + if err = yaml.Unmarshal(b.Bytes(), job); err != nil { + return nil, fmt.Errorf("error with yaml unmarshal: %w", err) + } + + job.Spec.Template.Spec.NodeSelector["kubernetes.io/hostname"] = nginxPod.Spec.NodeName + if len(job.Spec.Template.Spec.Containers) != 1 { + return nil, fmt.Errorf( + "expected node debugger job to contain one container, actual number: %d", + len(job.Spec.Template.Spec.Containers), + ) + } + job.Spec.Template.Spec.Containers[0].Args = []string{jobScript} + job.Namespace = ns.Name + + if err = resourceManager.Apply([]client.Object{job}); err != nil { + return nil, fmt.Errorf("error in applying job: %w", err) + } + + return job, nil + } + + restartNginxContainer := func(nginxPodName, namespace, containerName string) { + jobScript := "PID=$(pgrep -f \"nginx-agent\") && kill -9 $PID" + + restartCount, err := getContainerRestartCount(nginxPodName, namespace, containerName) + Expect(err).ToNot(HaveOccurred()) + + cleanUpPortForward() + job, err := runNodeDebuggerJob(nginxPodName, jobScript) + Expect(err).ToNot(HaveOccurred()) + + Eventually( + func() error { + return checkContainerRestart(nginxPodName, containerName, namespace, restartCount) + }). + WithTimeout(timeoutConfig.CreateTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + // default propagation policy is metav1.DeletePropagationOrphan which does not delete the underlying + // pod created through the job after the job is deleted. Setting it to metav1.DeletePropagationBackground + // deletes the underlying pod after the job is deleted. + Expect(resourceManager.Delete( + []client.Object{job}, + client.PropagationPolicy(metav1.DeletePropagationBackground), + )).To(Succeed()) + } + + checkNGFFunctionality := func(teaURL, coffeeURL string, files []string, ns *core.Namespace) { Eventually( func() error { return checkForWorkingTraffic(teaURL, coffeeURL) @@ -86,212 +191,333 @@ var _ = Describe("Graceful Recovery test", Ordered, Label("graceful-recovery"), WithTimeout(timeoutConfig.TestForTrafficTimeout). WithPolling(500 * time.Millisecond). Should(Succeed()) - }) - AfterAll(func() { + cleanUpPortForward() Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed()) - }) - It("recovers when NGF container is restarted", func() { - runRecoveryTest(teaURL, coffeeURL, ngfPodName, ngfContainerName, files, &ns) - }) + Eventually( + func() error { + return checkForFailingTraffic(teaURL, coffeeURL) + }). + WithTimeout(timeoutConfig.TestForTrafficTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) - It("recovers when nginx container is restarted", func() { - runRecoveryTest(teaURL, coffeeURL, ngfPodName, nginxContainerName, files, &ns) - }) + Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReady(ns.Name)).To(Succeed()) - It("recovers when drained node is restarted", func() { - runRestartNodeWithDrainingTest(teaURL, coffeeURL, files, &ns) - }) + var nginxPodNames []string + var err error + Eventually( + func() bool { + nginxPodNames, err = framework.GetReadyNginxPodNames(k8sClient, ns.Name, timeoutConfig.GetTimeout) + return len(nginxPodNames) == 1 && err == nil + }). + WithTimeout(timeoutConfig.CreateTimeout). + WithPolling(500 * time.Millisecond). + MustPassRepeatedly(10). + Should(BeTrue()) - It("recovers when node is restarted abruptly", func() { - runRestartNodeAbruptlyTest(teaURL, coffeeURL, files, &ns) - }) -}) + nginxPodName := nginxPodNames[0] + Expect(nginxPodName).ToNot(BeEmpty()) + activeNginxPodName = nginxPodName -func runRestartNodeWithDrainingTest(teaURL, coffeeURL string, files []string, ns *core.Namespace) { - runRestartNodeTest(teaURL, coffeeURL, files, ns, true) -} + setUpPortForward(activeNginxPodName, ns.Name) -func runRestartNodeAbruptlyTest(teaURL, coffeeURL string, files []string, ns *core.Namespace) { - runRestartNodeTest(teaURL, coffeeURL, files, ns, false) -} + Eventually( + func() error { + return checkForWorkingTraffic(teaURL, coffeeURL) + }). + WithTimeout(timeoutConfig.TestForTrafficTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + } -func runRestartNodeTest(teaURL, coffeeURL string, files []string, ns *core.Namespace, drain bool) { - nodeNames, err := getNodeNames() - Expect(err).ToNot(HaveOccurred()) - Expect(nodeNames).To(HaveLen(1)) + runRestartNodeTest := func(teaURL, coffeeURL string, files []string, ns *core.Namespace, drain bool) { + nodeNames, err := getNodeNames() + Expect(err).ToNot(HaveOccurred()) + Expect(nodeNames).To(HaveLen(1)) - kindNodeName := nodeNames[0] + kindNodeName := nodeNames[0] - Expect(clusterName).ToNot(BeNil(), "clusterName variable not set") - Expect(*clusterName).ToNot(BeEmpty()) - containerName := *clusterName + "-control-plane" + Expect(clusterName).ToNot(BeNil(), "clusterName variable not set") + Expect(*clusterName).ToNot(BeEmpty()) + containerName := *clusterName + "-control-plane" - if portFwdPort != 0 { - close(portForwardStopCh) - } + cleanUpPortForward() + + if drain { + output, err := exec.Command( + "kubectl", + "drain", + kindNodeName, + "--ignore-daemonsets", + "--delete-emptydir-data", + ).CombinedOutput() - if drain { - output, err := exec.Command( - "kubectl", - "drain", - kindNodeName, - "--ignore-daemonsets", - "--delete-emptydir-data", - ).CombinedOutput() + Expect(err).ToNot(HaveOccurred(), string(output)) - Expect(err).ToNot(HaveOccurred(), string(output)) + output, err = exec.Command("kubectl", "delete", "node", kindNodeName).CombinedOutput() + Expect(err).ToNot(HaveOccurred(), string(output)) + } - output, err = exec.Command("kubectl", "delete", "node", kindNodeName).CombinedOutput() - Expect(err).ToNot(HaveOccurred(), string(output)) + _, err = exec.Command("docker", "restart", containerName).CombinedOutput() + Expect(err).ToNot(HaveOccurred()) + + // need to wait for docker container to restart and be running before polling for ready NGF Pods or else we will error + Eventually( + func() bool { + output, err := exec.Command( + "docker", + "inspect", + "-f", + "{{.State.Running}}", + containerName, + ).CombinedOutput() + return strings.TrimSpace(string(output)) == "true" && err == nil + }). + WithTimeout(timeoutConfig.CreateTimeout). + WithPolling(500 * time.Millisecond). + Should(BeTrue()) + + // ngf can often oscillate between ready and error, so we wait for a stable readiness in ngf + var podNames []string + Eventually( + func() bool { + podNames, err = framework.GetReadyNGFPodNames( + k8sClient, + ngfNamespace, + releaseName, + timeoutConfig.GetStatusTimeout, + ) + return len(podNames) == 1 && err == nil + }). + WithTimeout(timeoutConfig.CreateTimeout * 2). + WithPolling(500 * time.Millisecond). + MustPassRepeatedly(20). + Should(BeTrue()) + newNGFPodName := podNames[0] + + // expected behavior is when node is drained, new pods will be created. when the node is + // abruptly restarted, new pods are not created. + if drain { + Expect(newNGFPodName).ToNot(Equal(activeNGFPodName)) + activeNGFPodName = newNGFPodName + } else { + Expect(newNGFPodName).To(Equal(activeNGFPodName)) + } + + var nginxPodNames []string + Eventually( + func() bool { + nginxPodNames, err = framework.GetReadyNginxPodNames(k8sClient, ns.Name, timeoutConfig.GetTimeout) + return len(nginxPodNames) == 1 && err == nil + }). + WithTimeout(timeoutConfig.CreateTimeout * 2). + WithPolling(500 * time.Millisecond). + MustPassRepeatedly(20). + Should(BeTrue()) + newNginxPodName := nginxPodNames[0] + + if drain { + Expect(newNginxPodName).ToNot(Equal(activeNginxPodName)) + activeNginxPodName = newNginxPodName + } else { + Expect(newNginxPodName).To(Equal(activeNginxPodName)) + } + + setUpPortForward(activeNginxPodName, ns.Name) + + // sets activeNginxPodName to new pod + checkNGFFunctionality(teaURL, coffeeURL, files, ns) + + if errorLogs := getNGFErrorLogs(activeNGFPodName); errorLogs != "" { + fmt.Printf("NGF has error logs: \n%s", errorLogs) + } + + if errorLogs := getUnexpectedNginxErrorLogs(activeNginxPodName, ns.Name); errorLogs != "" { + fmt.Printf("NGINX has unexpected error logs: \n%s", errorLogs) + } } - _, err = exec.Command("docker", "restart", containerName).CombinedOutput() - Expect(err).ToNot(HaveOccurred()) + runRestartNodeWithDrainingTest := func(teaURL, coffeeURL string, files []string, ns *core.Namespace) { + runRestartNodeTest(teaURL, coffeeURL, files, ns, true) + } - // need to wait for docker container to restart and be running before polling for ready NGF Pods or else we will error - Eventually( - func() bool { - output, err := exec.Command( - "docker", - "inspect", - "-f", - "{{.State.Running}}", - containerName, - ).CombinedOutput() - return strings.TrimSpace(string(output)) == "true" && err == nil - }). - WithTimeout(timeoutConfig.CreateTimeout). - WithPolling(500 * time.Millisecond). - Should(BeTrue()) - - // ngf can often oscillate between ready and error, so we wait for a stable readiness in ngf - var podNames []string - Eventually( - func() bool { - podNames, err = framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetStatusTimeout) - return len(podNames) == 1 && err == nil - }). - WithTimeout(timeoutConfig.CreateTimeout * 2). - WithPolling(500 * time.Millisecond). - MustPassRepeatedly(20). - Should(BeTrue()) - - ngfPodName := podNames[0] - Expect(ngfPodName).ToNot(BeEmpty()) - - if portFwdPort != 0 { - ports := []string{fmt.Sprintf("%d:80", ngfHTTPForwardedPort), fmt.Sprintf("%d:443", ngfHTTPSForwardedPort)} - portForwardStopCh = make(chan struct{}) - err = framework.PortForward(ctlr.GetConfigOrDie(), ngfNamespace, ngfPodName, ports, portForwardStopCh) - Expect(err).ToNot(HaveOccurred()) + runRestartNodeAbruptlyTest := func(teaURL, coffeeURL string, files []string, ns *core.Namespace) { + runRestartNodeTest(teaURL, coffeeURL, files, ns, false) } - checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, "", files, ns) - if errorLogs := getUnexpectedNginxErrorLogs(ngfPodName); errorLogs != "" { - Skip(fmt.Sprintf("NGINX has unexpected error logs: \n%s", errorLogs)) + 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 } -} -func runRecoveryTest(teaURL, coffeeURL, ngfPodName, containerName string, files []string, ns *core.Namespace) { - var ( - err error - leaseName string - ) + checkLeaderLeaseChange := func(originalLeaseName string) error { + leaseName, err := getLeaderElectionLeaseHolderName() + if err != nil { + return err + } - if containerName != nginxContainerName { - // 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. - leaseName, err = getLeaderElectionLeaseHolderName() - Expect(err).ToNot(HaveOccurred()) + if originalLeaseName == leaseName { + return fmt.Errorf( + "expected originalLeaseName: %s, to not match current leaseName: %s", + originalLeaseName, + leaseName, + ) + } + + return nil } - restartContainer(ngfPodName, containerName) + BeforeAll(func() { + podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(podNames).To(HaveLen(1)) + + activeNGFPodName = podNames[0] + + 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)) + + activeNginxPodName = nginxPodNames[0] + + setUpPortForward(activeNginxPodName, ns.Name) + + if portFwdPort != 0 { + coffeeURL = fmt.Sprintf("%s:%d/coffee", baseHTTPURL, portFwdPort) + } + if portFwdHTTPSPort != 0 { + teaURL = fmt.Sprintf("%s:%d/tea", baseHTTPSURL, portFwdHTTPSPort) + } - if containerName != nginxContainerName { Eventually( func() error { - return checkLeaderLeaseChange(leaseName) + return checkForWorkingTraffic(teaURL, coffeeURL) }). - WithTimeout(timeoutConfig.GetLeaderLeaseTimeout). + WithTimeout(timeoutConfig.TestForTrafficTimeout). WithPolling(500 * time.Millisecond). Should(Succeed()) - } + }) - checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, containerName, files, ns) - if errorLogs := getUnexpectedNginxErrorLogs(ngfPodName); errorLogs != "" { - Skip(fmt.Sprintf("NGINX has unexpected error logs: \n%s", errorLogs)) - } -} + AfterAll(func() { + cleanUpPortForward() + Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) + Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed()) + }) -func restartContainer(ngfPodName, containerName string) { - var jobScript string - if containerName == "nginx" { - jobScript = "PID=$(pgrep -f \"nginx: master process\") && kill -9 $PID" - } else { - jobScript = "PID=$(pgrep -f \"/usr/bin/gateway\") && kill -9 $PID" - } + It("recovers when nginx container is restarted", func() { + restartNginxContainer(activeNginxPodName, ns.Name, nginxContainerName) - restartCount, err := getContainerRestartCount(ngfPodName, containerName) - Expect(err).ToNot(HaveOccurred()) + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, ns.Name, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + activeNginxPodName = nginxPodNames[0] - job, err := runNodeDebuggerJob(ngfPodName, jobScript) - Expect(err).ToNot(HaveOccurred()) + setUpPortForward(activeNginxPodName, ns.Name) - Eventually( - func() error { - return checkContainerRestart(ngfPodName, containerName, restartCount) - }). - WithTimeout(timeoutConfig.ContainerRestartTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - - // default propagation policy is metav1.DeletePropagationOrphan which does not delete the underlying - // pod created through the job after the job is deleted. Setting it to metav1.DeletePropagationBackground - // deletes the underlying pod after the job is deleted. - Expect(resourceManager.Delete( - []client.Object{job}, - client.PropagationPolicy(metav1.DeletePropagationBackground), - )).To(Succeed()) -} + // sets activeNginxPodName to new pod + checkNGFFunctionality(teaURL, coffeeURL, files, &ns) -func checkContainerRestart(ngfPodName, containerName string, currentRestartCount int) error { - restartCount, err := getContainerRestartCount(ngfPodName, containerName) - if err != nil { - return err - } + if errorLogs := getNGFErrorLogs(activeNGFPodName); errorLogs != "" { + fmt.Printf("NGF has error logs: \n%s", errorLogs) + } - if restartCount != currentRestartCount+1 { - return fmt.Errorf("expected current restart count: %d to match incremented restart count: %d", - restartCount, currentRestartCount+1) - } + if errorLogs := getUnexpectedNginxErrorLogs(activeNginxPodName, ns.Name); errorLogs != "" { + fmt.Printf("NGINX has unexpected error logs: \n%s", errorLogs) + } + }) - return nil -} + It("recovers when NGF Pod is restarted", func() { + leaseName, err := getLeaderElectionLeaseHolderName() + Expect(err).ToNot(HaveOccurred()) -func checkForWorkingTraffic(teaURL, coffeeURL string) error { - if err := expectRequestToSucceed(teaURL, address, "URI: /tea"); err != nil { - return err - } - if err := expectRequestToSucceed(coffeeURL, address, "URI: /coffee"); err != nil { - return err - } - return nil -} + ngfPod, err := resourceManager.GetPod(ngfNamespace, activeNGFPodName) + Expect(err).ToNot(HaveOccurred()) -func checkForFailingTraffic(teaURL, coffeeURL string) error { - if err := expectRequestToFail(teaURL, address); err != nil { - return err - } - if err := expectRequestToFail(coffeeURL, address); err != nil { - return err - } - return nil -} + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.DeleteTimeout) + defer cancel() + + Expect(k8sClient.Delete(ctx, ngfPod)).To(Succeed()) + + var newNGFPodNames []string + Eventually( + func() bool { + newNGFPodNames, err = framework.GetReadyNGFPodNames( + k8sClient, + ngfNamespace, + releaseName, + timeoutConfig.GetStatusTimeout, + ) + return len(newNGFPodNames) == 1 && err == nil + }). + WithTimeout(timeoutConfig.CreateTimeout * 2). + WithPolling(500 * time.Millisecond). + MustPassRepeatedly(20). + Should(BeTrue()) + + newNGFPodName := newNGFPodNames[0] + Expect(newNGFPodName).ToNot(BeEmpty()) + + Expect(newNGFPodName).ToNot(Equal(activeNGFPodName)) + activeNGFPodName = newNGFPodName + + Eventually( + func() error { + return checkLeaderLeaseChange(leaseName) + }). + WithTimeout(timeoutConfig.GetLeaderLeaseTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + // sets activeNginxPodName to new pod + checkNGFFunctionality(teaURL, coffeeURL, files, &ns) + + if errorLogs := getNGFErrorLogs(activeNGFPodName); errorLogs != "" { + fmt.Printf("NGF has error logs: \n%s", errorLogs) + } + + if errorLogs := getUnexpectedNginxErrorLogs(activeNginxPodName, ns.Name); errorLogs != "" { + fmt.Printf("NGINX has unexpected error logs: \n%s", errorLogs) + } + }) + + It("recovers when drained node is restarted", func() { + runRestartNodeWithDrainingTest(teaURL, coffeeURL, files, &ns) + }) + + It("recovers when node is restarted abruptly", func() { + if *plusEnabled { + Skip(fmt.Sprintf("Skipping test when using NGINX Plus due to known issue:" + + " https://github.com/nginx/nginx-gateway-fabric/issues/3248")) + } + runRestartNodeAbruptlyTest(teaURL, coffeeURL, files, &ns) + }) +}) func expectRequestToSucceed(appURL, address string, responseBodyMessage string) error { status, body, err := framework.Get(appURL, address, timeoutConfig.RequestTimeout, nil, nil) @@ -324,48 +550,10 @@ func expectRequestToFail(appURL, address string) error { return nil } -func checkNGFFunctionality(teaURL, coffeeURL, ngfPodName, containerName string, files []string, ns *core.Namespace) { - Eventually( - func() error { - return checkForWorkingTraffic(teaURL, coffeeURL) - }). - WithTimeout(timeoutConfig.TestForTrafficTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - - Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) - - Eventually( - func() error { - return checkForFailingTraffic(teaURL, coffeeURL) - }). - WithTimeout(timeoutConfig.TestForTrafficTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - - Expect(resourceManager.ApplyFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.WaitForAppsToBeReadyWithPodCount(ns.Name, 2)).To(Succeed()) - - Eventually( - func() error { - return checkForWorkingTraffic(teaURL, coffeeURL) - }). - WithTimeout(timeoutConfig.TestForTrafficTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - - // When the NGINX process is killed, some errors are expected in the NGF logs while we wait for the - // NGINX container to be restarted. Therefore, we don't want to check the NGF logs for errors when the container - // we restarted was NGINX. - if containerName != nginxContainerName { - checkNGFContainerLogsForErrors(ngfPodName) - } -} - -func getNginxErrorLogs(ngfPodName string) string { +func getNginxErrorLogs(nginxPodName, namespace string) string { nginxLogs, err := resourceManager.GetPodLogs( - ngfNamespace, - ngfPodName, + namespace, + nginxPodName, &core.PodLogOptions{Container: nginxContainerName}, ) Expect(err).ToNot(HaveOccurred()) @@ -391,7 +579,7 @@ func getNginxErrorLogs(ngfPodName string) string { return errorLogs } -func getUnexpectedNginxErrorLogs(ngfPodName string) string { +func getUnexpectedNginxErrorLogs(nginxPodName, namespace string) string { expectedErrStrings := []string{ "connect() failed (111: Connection refused)", "could not be resolved (host not found) during usage report", @@ -403,7 +591,7 @@ func getUnexpectedNginxErrorLogs(ngfPodName string) string { unexpectedErrors := "" - errorLogs := getNginxErrorLogs(ngfPodName) + errorLogs := getNginxErrorLogs(nginxPodName, namespace) for _, line := range strings.Split(errorLogs, "\n") { if !slices.ContainsFunc(expectedErrStrings, func(s string) bool { @@ -416,8 +604,8 @@ func getUnexpectedNginxErrorLogs(ngfPodName string) string { return unexpectedErrors } -// checkNGFContainerLogsForErrors checks NGF container's logs for any possible errors. -func checkNGFContainerLogsForErrors(ngfPodName string) { +// getNGFErrorLogs gets NGF container error logs. +func getNGFErrorLogs(ngfPodName string) string { ngfLogs, err := resourceManager.GetPodLogs( ngfNamespace, ngfPodName, @@ -425,111 +613,28 @@ func checkNGFContainerLogsForErrors(ngfPodName string) { ) Expect(err).ToNot(HaveOccurred()) - for _, line := range strings.Split(ngfLogs, "\n") { - 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 -} - -func getContainerRestartCount(ngfPodName, containerName string) (int, error) { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) - defer cancel() - - var ngfPod core.Pod - if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ngfNamespace, Name: ngfPodName}, &ngfPod); err != nil { - return 0, fmt.Errorf("error retrieving NGF Pod: %w", err) - } + errorLogs := "" - var restartCount int - for _, containerStatus := range ngfPod.Status.ContainerStatuses { - if containerStatus.Name == containerName { - restartCount = int(containerStatus.RestartCount) + for _, line := range strings.Split(ngfLogs, "\n") { + if strings.Contains(line, "\"level\":\"error\"") { + errorLogs += line + "\n" + break } } - return restartCount, nil -} - -func getNodeNames() ([]string, error) { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) - defer cancel() - var nodes core.NodeList - - if err := k8sClient.List(ctx, &nodes); err != nil { - return nil, fmt.Errorf("error listing nodes: %w", err) - } - - names := make([]string, 0, len(nodes.Items)) - - for _, node := range nodes.Items { - names = append(names, node.Name) - } - - return names, nil + return errorLogs } -func runNodeDebuggerJob(ngfPodName, jobScript string) (*v1.Job, error) { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) - defer cancel() - - var ngfPod core.Pod - if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ngfNamespace, Name: ngfPodName}, &ngfPod); err != nil { - return nil, fmt.Errorf("error retrieving NGF Pod: %w", err) - } - - b, err := resourceManager.GetFileContents("graceful-recovery/node-debugger-job.yaml") - if err != nil { - return nil, fmt.Errorf("error processing node debugger job file: %w", err) - } - - job := &v1.Job{} - if err = yaml.Unmarshal(b.Bytes(), job); err != nil { - return nil, fmt.Errorf("error with yaml unmarshal: %w", err) - } - - job.Spec.Template.Spec.NodeSelector["kubernetes.io/hostname"] = ngfPod.Spec.NodeName - if len(job.Spec.Template.Spec.Containers) != 1 { - return nil, fmt.Errorf( - "expected node debugger job to contain one container, actual number: %d", - len(job.Spec.Template.Spec.Containers), - ) - } - job.Spec.Template.Spec.Containers[0].Args = []string{jobScript} - job.Namespace = ngfNamespace +// checkNGFContainerLogsForErrors checks NGF container's logs for any possible errors. +func checkNGFContainerLogsForErrors(ngfPodName string) { + ngfLogs, err := resourceManager.GetPodLogs( + ngfNamespace, + ngfPodName, + &core.PodLogOptions{Container: ngfContainerName}, + ) + Expect(err).ToNot(HaveOccurred()) - if err = resourceManager.Apply([]client.Object{job}); err != nil { - return nil, fmt.Errorf("error in applying job: %w", err) + for _, line := range strings.Split(ngfLogs, "\n") { + Expect(line).ToNot(ContainSubstring("\"level\":\"error\""), line) } - - return job, nil } diff --git a/tests/suite/manifests/tracing/nginxproxy.yaml b/tests/suite/manifests/tracing/nginxproxy.yaml deleted file mode 100644 index f4876eb186..0000000000 --- a/tests/suite/manifests/tracing/nginxproxy.yaml +++ /dev/null @@ -1,12 +0,0 @@ -apiVersion: gateway.nginx.org/v1alpha2 -kind: NginxProxy -metadata: - name: nginx-proxy -spec: - telemetry: - exporter: - endpoint: otel-collector-opentelemetry-collector.collector.svc:4317 - serviceName: my-test-svc - spanAttributes: - - key: testkey1 - value: testval1 diff --git a/tests/suite/reconfig_test.go b/tests/suite/reconfig_test.go index 7503aef764..fb4d6c02ce 100644 --- a/tests/suite/reconfig_test.go +++ b/tests/suite/reconfig_test.go @@ -406,7 +406,12 @@ var _ = Describe("Reconfiguration Performance Testing", Ordered, Label("nfr", "r } checkNGFContainerLogsForErrors(ngfPodName) - nginxErrorLogs := getNginxErrorLogs(ngfPodName) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, reconfigNamespace.Name, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + nginxErrorLogs := getNginxErrorLogs(nginxPodNames[0], reconfigNamespace.Name) reloadCount, err := framework.GetReloadCount(promInstance, ngfPodName) Expect(err).ToNot(HaveOccurred()) diff --git a/tests/suite/sample_test.go b/tests/suite/sample_test.go index bd883ae710..3426ed973f 100644 --- a/tests/suite/sample_test.go +++ b/tests/suite/sample_test.go @@ -17,29 +17,39 @@ import ( ) var _ = Describe("Basic test example", Label("functional"), func() { - files := []string{ - "hello-world/apps.yaml", - "hello-world/gateway.yaml", - "hello-world/routes.yaml", - } + var ( + files = []string{ + "hello-world/apps.yaml", + "hello-world/gateway.yaml", + "hello-world/routes.yaml", + } - var ns core.Namespace + namespace = "helloworld" + ) BeforeEach(func() { - ns = core.Namespace{ + ns := &core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "helloworld", + Name: namespace, }, } - 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.Apply([]client.Object{ns})).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + setUpPortForward(nginxPodNames[0], namespace) }) AfterEach(func() { - Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed()) + cleanUpPortForward() + + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) It("sends traffic", func() { diff --git a/tests/suite/snippets_filter_test.go b/tests/suite/snippets_filter_test.go index 1edf2b2fe3..39e099a8f6 100644 --- a/tests/suite/snippets_filter_test.go +++ b/tests/suite/snippets_filter_test.go @@ -10,7 +10,6 @@ import ( core "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -28,6 +27,8 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter } namespace = "snippets-filter" + + nginxPodName string ) BeforeAll(func() { @@ -40,9 +41,19 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + nginxPodName = nginxPodNames[0] + + setUpPortForward(nginxPodName, namespace) }) AfterAll(func() { + cleanUpPortForward() + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) @@ -68,8 +79,11 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter for _, name := range snippetsFilterNames { nsname := types.NamespacedName{Name: name, Namespace: namespace} - err := waitForSnippetsFilterToBeAccepted(nsname) - Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name)) + Eventually(checkForSnippetsFilterToBeAccepted). + WithArguments(nsname). + WithTimeout(timeoutConfig.GetStatusTimeout). + WithPolling(500*time.Millisecond). + Should(Succeed(), fmt.Sprintf("%s was not accepted", name)) } }) @@ -104,13 +118,8 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter grpcRouteSuffix := fmt.Sprintf("%s_grpc-all-contexts.conf", namespace) BeforeAll(func() { - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) - - ngfPodName := podNames[0] - - conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + var err error + conf, err = resourceManager.GetNginxConfig(nginxPodName, namespace) Expect(err).ToNot(HaveOccurred()) }) @@ -221,7 +230,11 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) nsname := types.NamespacedName{Name: "tea", Namespace: namespace} - Expect(waitForHTTPRouteToHaveGatewayNotProgrammedCond(nsname)).To(Succeed()) + Eventually(checkHTTPRouteToHaveGatewayNotProgrammedCond). + WithArguments(nsname). + WithTimeout(timeoutConfig.GetStatusTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) @@ -232,116 +245,99 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) nsname := types.NamespacedName{Name: "soda", Namespace: namespace} - Expect(waitForHTTPRouteToHaveGatewayNotProgrammedCond(nsname)).To(Succeed()) + Eventually(checkHTTPRouteToHaveGatewayNotProgrammedCond). + WithArguments(nsname). + WithTimeout(timeoutConfig.GetStatusTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) }) }) -func waitForHTTPRouteToHaveGatewayNotProgrammedCond(httpRouteNsName types.NamespacedName) error { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout*2) +func checkHTTPRouteToHaveGatewayNotProgrammedCond(httpRouteNsName types.NamespacedName) error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) defer cancel() GinkgoWriter.Printf( - "Waiting for HTTPRoute %q to have the condition Accepted/True/GatewayNotProgrammed\n", + "Checking for HTTPRoute %q to have the condition Accepted/True/GatewayNotProgrammed\n", httpRouteNsName, ) - return wait.PollUntilContextCancel( - ctx, - 500*time.Millisecond, - true, /* poll immediately */ - func(ctx context.Context) (bool, error) { - var hr v1.HTTPRoute - var err error + var hr v1.HTTPRoute + var err error - if err = k8sClient.Get(ctx, httpRouteNsName, &hr); err != nil { - return false, err - } + if err = k8sClient.Get(ctx, httpRouteNsName, &hr); err != nil { + return err + } - if len(hr.Status.Parents) == 0 { - return false, nil - } + if len(hr.Status.Parents) != 1 { + return fmt.Errorf("httproute has %d parent statuses, expected 1", len(hr.Status.Parents)) + } - if len(hr.Status.Parents) != 1 { - return false, fmt.Errorf("httproute has %d parent statuses, expected 1", len(hr.Status.Parents)) - } + parent := hr.Status.Parents[0] + if parent.Conditions == nil { + return fmt.Errorf("expected parent conditions to not be nil") + } - parent := hr.Status.Parents[0] - if parent.Conditions == nil { - return false, fmt.Errorf("expected parent conditions to not be nil") - } + cond := parent.Conditions[1] + if cond.Type != string(v1.GatewayConditionAccepted) { + return fmt.Errorf("expected condition type to be Accepted, got %s", cond.Type) + } - cond := parent.Conditions[1] - if cond.Type != string(v1.GatewayConditionAccepted) { - return false, fmt.Errorf("expected condition type to be Accepted, got %s", cond.Type) - } + if cond.Status != metav1.ConditionFalse { + return fmt.Errorf("expected condition status to be False, got %s", cond.Status) + } - if cond.Status != metav1.ConditionFalse { - return false, fmt.Errorf("expected condition status to be False, got %s", cond.Status) - } + if cond.Reason != string(conditions.RouteReasonGatewayNotProgrammed) { + return fmt.Errorf("expected condition reason to be GatewayNotProgrammed, got %s", cond.Reason) + } - if cond.Reason != string(conditions.RouteReasonGatewayNotProgrammed) { - return false, fmt.Errorf("expected condition reason to be GatewayNotProgrammed, got %s", cond.Reason) - } - return err == nil, err - }, - ) + return nil } -func waitForSnippetsFilterToBeAccepted(snippetsFilterNsNames types.NamespacedName) error { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout) +func checkForSnippetsFilterToBeAccepted(snippetsFilterNsNames types.NamespacedName) error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetTimeout) defer cancel() GinkgoWriter.Printf( - "Waiting for SnippetsFilter %q to have the condition Accepted/True/Accepted\n", + "Checking for SnippetsFilter %q to have the condition Accepted/True/Accepted\n", snippetsFilterNsNames, ) - return wait.PollUntilContextCancel( - ctx, - 500*time.Millisecond, - true, /* poll immediately */ - func(ctx context.Context) (bool, error) { - var sf ngfAPI.SnippetsFilter - var err error + var sf ngfAPI.SnippetsFilter + var err error - if err = k8sClient.Get(ctx, snippetsFilterNsNames, &sf); err != nil { - return false, err - } - - if len(sf.Status.Controllers) == 0 { - return false, nil - } + if err = k8sClient.Get(ctx, snippetsFilterNsNames, &sf); err != nil { + return err + } - if len(sf.Status.Controllers) != 1 { - return false, fmt.Errorf("snippetsFilter has %d controller statuses, expected 1", len(sf.Status.Controllers)) - } + if len(sf.Status.Controllers) != 1 { + return fmt.Errorf("snippetsFilter has %d controller statuses, expected 1", len(sf.Status.Controllers)) + } - status := sf.Status.Controllers[0] - if status.ControllerName != ngfControllerName { - return false, fmt.Errorf("expected controller name to be %s, got %s", ngfControllerName, status.ControllerName) - } + status := sf.Status.Controllers[0] + if status.ControllerName != ngfControllerName { + return fmt.Errorf("expected controller name to be %s, got %s", ngfControllerName, status.ControllerName) + } - condition := status.Conditions[0] - if condition.Type != string(ngfAPI.SnippetsFilterConditionTypeAccepted) { - return false, fmt.Errorf("expected condition type to be Accepted, got %s", condition.Type) - } + condition := status.Conditions[0] + if condition.Type != string(ngfAPI.SnippetsFilterConditionTypeAccepted) { + return fmt.Errorf("expected condition type to be Accepted, got %s", condition.Type) + } - if status.Conditions[0].Status != metav1.ConditionTrue { - return false, fmt.Errorf("expected condition status to be %s, got %s", metav1.ConditionTrue, condition.Status) - } + if status.Conditions[0].Status != metav1.ConditionTrue { + return fmt.Errorf("expected condition status to be %s, got %s", metav1.ConditionTrue, condition.Status) + } - if status.Conditions[0].Reason != string(ngfAPI.SnippetsFilterConditionReasonAccepted) { - return false, fmt.Errorf( - "expected condition reason to be %s, got %s", - ngfAPI.SnippetsFilterConditionReasonAccepted, - condition.Reason, - ) - } + if status.Conditions[0].Reason != string(ngfAPI.SnippetsFilterConditionReasonAccepted) { + return fmt.Errorf( + "expected condition reason to be %s, got %s", + ngfAPI.SnippetsFilterConditionReasonAccepted, + condition.Reason, + ) + } - return err == nil, err - }, - ) + return nil } diff --git a/tests/suite/system_suite_test.go b/tests/suite/system_suite_test.go index 684ad1d998..39e9e15524 100644 --- a/tests/suite/system_suite_test.go +++ b/tests/suite/system_suite_test.go @@ -71,7 +71,7 @@ var ( var ( //go:embed manifests/* manifests embed.FS - k8sClient client.Client + k8sClient client.Client // TODO: are the k8sClient and the resourceManager.k8sClient the same? resourceManager framework.ResourceManager portForwardStopCh chan struct{} portFwdPort int @@ -185,20 +185,34 @@ func setup(cfg setupConfig, extraInstallArgs ...string) { ) Expect(err).ToNot(HaveOccurred()) Expect(podNames).ToNot(BeEmpty()) +} + +func setUpPortForward(nginxPodName, nginxNamespace string) { + var err error if *serviceType != "LoadBalancer" { ports := []string{fmt.Sprintf("%d:80", ngfHTTPForwardedPort), fmt.Sprintf("%d:443", ngfHTTPSForwardedPort)} portForwardStopCh = make(chan struct{}) - err = framework.PortForward(k8sConfig, installCfg.Namespace, podNames[0], ports, portForwardStopCh) + err = framework.PortForward(resourceManager.K8sConfig, nginxNamespace, nginxPodName, ports, portForwardStopCh) address = "127.0.0.1" portFwdPort = ngfHTTPForwardedPort portFwdHTTPSPort = ngfHTTPSForwardedPort } else { - address, err = resourceManager.GetLBIPAddress(installCfg.Namespace) + address, err = resourceManager.GetLBIPAddress(nginxNamespace) } Expect(err).ToNot(HaveOccurred()) } +// cleanUpPortForward closes the port forward channel and needs to be called before deleting any gateways or else +// the logs will be flooded with port forward errors. +func cleanUpPortForward() { + if portFwdPort != 0 { + close(portForwardStopCh) + portFwdPort = 0 + portFwdHTTPSPort = 0 + } +} + func createNGFInstallConfig(cfg setupConfig, extraInstallArgs ...string) framework.InstallationConfig { installCfg := framework.InstallationConfig{ ReleaseName: cfg.releaseName, @@ -252,12 +266,6 @@ func createNGFInstallConfig(cfg setupConfig, extraInstallArgs ...string) framewo } func teardown(relName string) { - if portFwdPort != 0 { - close(portForwardStopCh) - portFwdPort = 0 - portFwdHTTPSPort = 0 - } - cfg := framework.InstallationConfig{ ReleaseName: relName, Namespace: ngfNamespace, diff --git a/tests/suite/telemetry_test.go b/tests/suite/telemetry_test.go index ba15f130f1..c823cc590c 100644 --- a/tests/suite/telemetry_test.go +++ b/tests/suite/telemetry_test.go @@ -12,6 +12,10 @@ import ( ) var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), 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 + BeforeEach(func() { // Because NGF reports telemetry on start, we need to install the collector first. @@ -22,10 +26,9 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( // Install NGF // Note: the BeforeSuite call doesn't install NGF for 'telemetry' label - setup( - getDefaultSetupCfg(), - "--set", "nginxGateway.productTelemetry.enable=true", - ) + cfg := getDefaultSetupCfg() + cfg.telemetry = true + setup(cfg) }) AfterEach(func() { @@ -86,7 +89,7 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( "GatewayAttachedClientSettingsPolicyCount: Int(0)", "RouteAttachedClientSettingsPolicyCount: Int(0)", "ObservabilityPolicyCount: Int(0)", - "NginxProxyCount: Int(0)", + "NginxProxyCount: Int(1)", "SnippetsFilterCount: Int(0)", "UpstreamSettingsPolicyCount: Int(0)", "NGFReplicaCount: Int(1)", diff --git a/tests/suite/tracing_test.go b/tests/suite/tracing_test.go index e1d6aceff5..83c46d4cb4 100644 --- a/tests/suite/tracing_test.go +++ b/tests/suite/tracing_test.go @@ -19,6 +19,7 @@ import ( ngfAPIv1alpha1 "github.com/nginx/nginx-gateway-fabric/apis/v1alpha1" ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/apis/v1alpha2" + "github.com/nginx/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginx/nginx-gateway-fabric/tests/framework" ) @@ -26,26 +27,58 @@ import ( // 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), Label("functional", "tracing"), func() { +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" - ns core.Namespace + namespace = "helloworld" collectorPodName, helloURL, worldURL, helloworldURL string ) + updateNginxProxyTelemetrySpec := func(telemetry ngfAPIv1alpha2.Telemetry) { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.UpdateTimeout) + defer cancel() + + key := types.NamespacedName{Name: "ngf-test-proxy-config", Namespace: "nginx-gateway"} + var nginxProxy ngfAPIv1alpha2.NginxProxy + Expect(k8sClient.Get(ctx, key, &nginxProxy)).To(Succeed()) + + nginxProxy.Spec.Telemetry = &telemetry + + Expect(k8sClient.Update(ctx, &nginxProxy)).To(Succeed()) + } + + BeforeAll(func() { + telemetry := ngfAPIv1alpha2.Telemetry{ + Exporter: &ngfAPIv1alpha2.TelemetryExporter{ + Endpoint: helpers.GetPointer("otel-collector-opentelemetry-collector.collector.svc:4317"), + }, + ServiceName: helpers.GetPointer("my-test-svc"), + SpanAttributes: []ngfAPIv1alpha1.SpanAttribute{{ + Key: "testkey1", + Value: "testval1", + }}, + } + + updateNginxProxyTelemetrySpec(telemetry) + }) + + // BeforeEach is needed because FlakeAttempts do not re-run BeforeAll/AfterAll nodes BeforeEach(func() { - ns = core.Namespace{ + ns := &core.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "helloworld", + Name: namespace, }, } @@ -55,9 +88,15 @@ var _ = Describe("Tracing", FlakeAttempts(2), Label("functional", "tracing"), fu collectorPodName, err = framework.GetCollectorPodName(resourceManager) Expect(err).ToNot(HaveOccurred()) - 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.Apply([]client.Object{ns})).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + setUpPortForward(nginxPodNames[0], namespace) url := "http://foo.example.com" helloURL = url + "/hello" @@ -74,41 +113,17 @@ var _ = Describe("Tracing", FlakeAttempts(2), Label("functional", "tracing"), fu output, err := framework.UninstallCollector(resourceManager) Expect(err).ToNot(HaveOccurred(), string(output)) - Expect(resourceManager.DeleteFromFiles(files, ns.Name)).To(Succeed()) - Expect(resourceManager.DeleteFromFiles( - []string{nginxProxyFile, policySingleFile, policyMultipleFile}, ns.Name)).To(Succeed()) - Expect(resourceManager.DeleteNamespace(ns.Name)).To(Succeed()) - - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.CreateTimeout) - defer cancel() - - key := types.NamespacedName{Name: gatewayClassName} - var gwClass gatewayv1.GatewayClass - Expect(k8sClient.Get(ctx, key, &gwClass)).To(Succeed()) + cleanUpPortForward() - gwClass.Spec.ParametersRef = nil - - Expect(k8sClient.Update(ctx, &gwClass)).To(Succeed()) + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.DeleteFromFiles( + []string{policySingleFile, policyMultipleFile}, namespace)).To(Succeed()) + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) - updateGatewayClass := func() error { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.CreateTimeout) - defer cancel() - - key := types.NamespacedName{Name: gatewayClassName} - var gwClass gatewayv1.GatewayClass - if err := k8sClient.Get(ctx, key, &gwClass); err != nil { - return err - } - - gwClass.Spec.ParametersRef = &gatewayv1.ParametersReference{ - Group: ngfAPIv1alpha1.GroupName, - Kind: gatewayv1.Kind("NginxProxy"), - Name: "nginx-proxy", - } - - return k8sClient.Update(ctx, &gwClass) - } + AfterAll(func() { + updateNginxProxyTelemetrySpec(ngfAPIv1alpha2.Telemetry{}) + }) sendRequests := func(url string, count int) { for range count { @@ -168,11 +183,9 @@ var _ = Describe("Tracing", FlakeAttempts(2), Label("functional", "tracing"), fu // install tracing configuration traceFiles := []string{ - nginxProxyFile, policySingleFile, } - Expect(resourceManager.ApplyFromFiles(traceFiles, ns.Name)).To(Succeed()) - Expect(updateGatewayClass()).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(traceFiles, namespace)).To(Succeed()) checkStatusAndTraces() @@ -192,11 +205,9 @@ var _ = Describe("Tracing", FlakeAttempts(2), Label("functional", "tracing"), fu It("sends tracing spans for one policy attached to multiple routes", func() { // install tracing configuration traceFiles := []string{ - nginxProxyFile, policyMultipleFile, } - Expect(resourceManager.ApplyFromFiles(traceFiles, ns.Name)).To(Succeed()) - Expect(updateGatewayClass()).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(traceFiles, namespace)).To(Succeed()) checkStatusAndTraces() diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index 23fceb768d..dad10bc0e5 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -31,6 +31,8 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic namespace = "uspolicy" gatewayName = "gateway" + + nginxPodName string ) zoneSize := "512k" @@ -48,9 +50,19 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + + nginxPodNames, err := framework.GetReadyNginxPodNames(k8sClient, namespace, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(nginxPodNames).To(HaveLen(1)) + + nginxPodName = nginxPodNames[0] + + setUpPortForward(nginxPodName, namespace) }) AfterAll(func() { + cleanUpPortForward() + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) @@ -117,13 +129,8 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic var conf *framework.Payload BeforeAll(func() { - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) - - ngfPodName := podNames[0] - - conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + var err error + conf, err = resourceManager.GetNginxConfig(nginxPodName, namespace) Expect(err).ToNot(HaveOccurred()) }) @@ -302,13 +309,8 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic var conf *framework.Payload BeforeAll(func() { - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) - - ngfPodName := podNames[0] - - conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + var err error + conf, err = resourceManager.GetNginxConfig(nginxPodName, namespace) Expect(err).ToNot(HaveOccurred()) })