From ab12a19546aafaa7bae50d535a7b651f39dfef58 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:57:47 -0800 Subject: [PATCH 1/8] add functional tests for upstreamSettings policy --- tests/framework/crossplane.go | 17 + .../upstream-settings-policy/cafe.yaml | 66 +++ .../upstream-settings-policy/gateway.yaml | 11 + .../grpc-backend.yaml | 39 ++ .../invalid-usps.yaml | 84 ++++ .../upstream-settings-policy/routes.yaml | 35 ++ .../valid-merge-usps.yaml | 25 ++ .../valid-usps-first-wins.yaml | 21 + .../upstream-settings-policy/valid-usps.yaml | 49 +++ tests/suite/upstream_settings_test.go | 414 ++++++++++++++++++ 10 files changed, 761 insertions(+) create mode 100644 tests/suite/manifests/upstream-settings-policy/cafe.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/gateway.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/routes.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml create mode 100644 tests/suite/manifests/upstream-settings-policy/valid-usps.yaml create mode 100644 tests/suite/upstream_settings_test.go diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index b6925dc8e1..33046ff868 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -30,6 +30,8 @@ type ExpectedNginxField struct { Location string // Servers are the server names that the directive should exist in. Servers []string + // Upstream are the upstream names that the directive should exist in. + Upstreams []string // ValueSubstringAllowed allows the expected value to be a substring of the real value. // This makes it easier for cases when real values are complex file names or contain things we // don't care about, and we just want to check if a substring exists. @@ -64,6 +66,8 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err } } } + + return validateUpstreamDirectives(expFieldCfg, directive) } } @@ -75,6 +79,19 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, string(b)) } +func validateUpstreamDirectives(expFieldCfg ExpectedNginxField, directive *Directive) error { + for _, upstreamName := range expFieldCfg.Upstreams { + if directive.Directive == "upstream" && directive.Args[0] == upstreamName { + for _, upstreamDirective := range directive.Block { + if expFieldCfg.fieldFound(upstreamDirective) { + return nil + } + } + } + } + return nil +} + func getServerName(serverBlock Directives) string { for _, directive := range serverBlock { if directive.Directive == "server_name" { diff --git a/tests/suite/manifests/upstream-settings-policy/cafe.yaml b/tests/suite/manifests/upstream-settings-policy/cafe.yaml new file mode 100644 index 0000000000..c0466158f8 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/cafe.yaml @@ -0,0 +1,66 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: coffee +spec: + replicas: 1 + selector: + matchLabels: + app: coffee + template: + metadata: + labels: + app: coffee + spec: + containers: + - name: coffee + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: coffee +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: coffee +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tea +spec: + replicas: 1 + selector: + matchLabels: + app: tea + template: + metadata: + labels: + app: tea + spec: + containers: + - name: tea + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: tea +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: tea +--- diff --git a/tests/suite/manifests/upstream-settings-policy/gateway.yaml b/tests/suite/manifests/upstream-settings-policy/gateway.yaml new file mode 100644 index 0000000000..e6507f613b --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/gateway.yaml @@ -0,0 +1,11 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway +spec: + gatewayClassName: nginx + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: "*.example.com" diff --git a/tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml b/tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml new file mode 100644 index 0000000000..3ae352f573 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/grpc-backend.yaml @@ -0,0 +1,39 @@ +apiVersion: v1 +kind: Service +metadata: + name: grpc-backend +spec: + selector: + app: grpc-backend + ports: + - protocol: TCP + port: 8080 + targetPort: 50051 +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: grpc-backend + labels: + app: grpc-backend +spec: + replicas: 1 + selector: + matchLabels: + app: grpc-backend + template: + metadata: + labels: + app: grpc-backend + spec: + containers: + - name: grpc-backend + image: ghcr.io/nginxinc/kic-test-grpc-server:0.2.3 + env: + - name: POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + resources: + requests: + cpu: 10m diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml new file mode 100644 index 0000000000..575121900a --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml @@ -0,0 +1,84 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway-httproute-allowed +spec: + gatewayClassName: nginx + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: "soda.example.com" + allowedRoutes: + namespaces: + from: All + kinds: + kind: GRPCRoute +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: soda +spec: + replicas: 1 + selector: + matchLabels: + app: soda + template: + metadata: + labels: + app: soda + spec: + containers: + - name: soda + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: soda +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: soda +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: soda +spec: + parentRefs: + - name: gateway-httproute-allowed + sectionName: http + hostnames: + - "soda.example.com" + rules: + - matches: + - path: + type: Exact + value: /soda + backendRefs: + - name: soda + port: 80 +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: soda-svc-usp +spec: + zoneSize: 512k + targetRefs: + - group: core + kind: Service + name: soda + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s diff --git a/tests/suite/manifests/upstream-settings-policy/routes.yaml b/tests/suite/manifests/upstream-settings-policy/routes.yaml new file mode 100644 index 0000000000..37defe1a3e --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/routes.yaml @@ -0,0 +1,35 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: coffee +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: PathPrefix + value: /coffee + backendRefs: + - name: coffee + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: GRPCRoute +metadata: + name: grpc-route +spec: + parentRefs: + - name: gateway + sectionName: http + rules: + - matches: + - method: + service: helloworld.Greeter + method: SayHello + backendRefs: + - name: grpc-backend + port: 8080 diff --git a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml new file mode 100644 index 0000000000..d248f49cfb --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml @@ -0,0 +1,25 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: coffee-svc-usp-1 +spec: + zoneSize: 64k + targetRefs: + - group: core + kind: Service + name: coffee +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: coffee-svc-usp-2 +spec: + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + connections: 100 + requests: 55 + time: 1m + timeout: 5h diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml new file mode 100644 index 0000000000..1600aea97a --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml @@ -0,0 +1,21 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: coffee-svc-usp-1 +spec: + zoneSize: 64k + targetRefs: + - group: core + kind: Service + name: coffee +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: coffee-svc-usp-2 +spec: + zoneSize: 128k + targetRefs: + - group: core + kind: Service + name: coffee diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml new file mode 100644 index 0000000000..8b09e7ddc0 --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml @@ -0,0 +1,49 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: coffee-svc-usp +spec: + zoneSize: 512k + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: tea-multiple-svc-usp +spec: + zoneSize: 128k + targetRefs: + - group: core + kind: Service + name: tea + - group: core + kind: Service + name: coffee + keepAlive: + connections: 12 + requests: 31 + time: 20s + timeout: 40s +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: grpc-svc-usp +spec: + targetRef: + group: core + kind: Service + name: grpc-backend + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go new file mode 100644 index 0000000000..c7acb766ed --- /dev/null +++ b/tests/suite/upstream_settings_test.go @@ -0,0 +1,414 @@ +package main + +import ( + "context" + "fmt" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + 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" + "sigs.k8s.io/gateway-api/apis/v1alpha2" + + ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/tests/framework" +) + +var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { + var ( + files = []string{ + "upstream-settings-policy/cafe.yaml", + "upstream-settings-policy/gateway.yaml", + "upstream-settings-policy/grpc-backend.yaml", + "upstream-settings-policy/routes.yaml", + } + + namespace = "uspolicy" + ) + + BeforeAll(func() { + ns := &core.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespace, + }, + } + + Expect(resourceManager.Apply([]client.Object{ns})).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + Expect(resourceManager.WaitForAppsToBeReady(namespace)).To(Succeed()) + }) + + AfterAll(func() { + Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) + }) + + When("UpstreamSettingsPolicy are applied to the resources", func() { + snippetsFilter := []string{ + "upstream-settings-policy/valid-usps.yaml", + } + + BeforeAll(func() { + Expect(resourceManager.ApplyFromFiles(snippetsFilter, namespace)).To(Succeed()) + }) + + AfterAll(func() { + Expect(resourceManager.DeleteFromFiles(snippetsFilter, namespace)).To(Succeed()) + }) + + Specify("usPolicies are accepted", func() { + usPolicies := []string{ + "coffee-svc-usp", + "grpc-svc-usp", + "tea-multiple-svc-usp", + } + + for _, name := range usPolicies { + nsname := types.NamespacedName{Name: name, Namespace: namespace} + + err := waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name)) + } + }) + + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoute", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") + + Eventually( + func() error { + return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + Eventually( + func() error { + return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + }) + }) + + Context("nginx directives", func() { + 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) + Expect(err).ToNot(HaveOccurred()) + }) + + // TODO: important + // The directive file and field value need to be updated based on the + // implementation of the UpstreamSettingsPolicy and how they are specified in the config files. + DescribeTable("are set properly for", + func(expCfgs []framework.ExpectedNginxField) { + for _, expCfg := range expCfgs { + Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) + } + }, + Entry("HTTPRoute", []framework.ExpectedNginxField{ + { + Directive: "zone default_coffee_80", + Value: "128k", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + ValueSubstringAllowed: true, + }, + { + Directive: "keepalive", + Value: "12", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_requests", + Value: "31", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_time", + Value: "20s", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_timeout", + Value: "40s", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + }), + Entry("GRPCRoute", []framework.ExpectedNginxField{ + { + Directive: "zone default_grpc-backend_8080", + Value: "512k", + Upstreams: []string{"default_grpc-backend_8080"}, + File: "nginx.conf", + ValueSubstringAllowed: true, + }, + { + Directive: "keepalive", + Value: "10", + Upstreams: []string{"default_grpc-backend_8080"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_requests", + Value: "3", + Upstreams: []string{"default_grpc-backend_8080"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_time", + Value: "10s", + Upstreams: []string{"default_grpc-backend_8080"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_timeout", + Value: "50s", + Upstreams: []string{"default_grpc-backend_8080"}, + File: "nginx.conf", + }, + }), + ) + }) + }) + + When("When multiple UpstreamSettings Policy target the same service", func() { + Specify("configuring distinct settings then directives are merged", func() { + files := []string{"upstream-settings-policy/valid-merge-usps.yaml"} + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + + nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + + nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoute", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + + Eventually( + func() error { + return expectRequestToSucceed(baseURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + }) + }) + + Context("nginx directives", func() { + 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) + Expect(err).ToNot(HaveOccurred()) + }) + + // TODO: important + // The directive file and field value need to be updated based on the + // implementation of the UpstreamSettingsPolicy and how they are specified in the config files. + DescribeTable("are set properly for", + func(expCfgs []framework.ExpectedNginxField) { + for _, expCfg := range expCfgs { + Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) + } + }, + Entry("HTTPRoute", []framework.ExpectedNginxField{ + { + Directive: "zone default_coffee_80", + Value: "64k", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + ValueSubstringAllowed: true, + }, + { + Directive: "keepalive", + Value: "100", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_requests", + Value: "55", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_time", + Value: "1m", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + { + Directive: "keepalive_timeout", + Value: "5h", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + }, + }), + ) + }) + + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + }) + Specify("configuring overlapping settings, the policy created first wins", func() { + files := []string{"upstream-settings-policy/valid-usps-first-wins.yaml"} + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + + nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + + nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoute", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + + Eventually( + func() error { + return expectRequestToSucceed(baseURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + }) + }) + + Context("nginx directives", func() { + 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) + Expect(err).ToNot(HaveOccurred()) + }) + + // TODO: important + // The directive file and field value need to be updated based on the + // implementation of the UpstreamSettingsPolicy and how they are specified in the config files. + DescribeTable("are set properly for", + func(expCfgs []framework.ExpectedNginxField) { + for _, expCfg := range expCfgs { + Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) + } + }, + Entry("HTTPRoute", []framework.ExpectedNginxField{ + { + Directive: "zone default_coffee_80", + Value: "128k", + Upstreams: []string{"default_coffee_80"}, + File: "nginx.conf", + ValueSubstringAllowed: true, + }, + }), + ) + }) + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + }) + }) + + When("UpstreamSettings Policy is invalid", func() { + Specify("if service mentioned in the target ref is not present", func() { + files := []string{"upstream-settings-policy/invalid-usps.yaml"} + + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + + nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionFalse, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + }) + }) +}) + +func waitForUSPolicyStatus( + usPolicyNsNames types.NamespacedName, + condStatus metav1.ConditionStatus, + condReason v1alpha2.PolicyConditionReason, +) error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout) + defer cancel() + + GinkgoWriter.Printf( + "Waiting for UpstreamSettings Policy %q to have the condition Accepted/True/Accepted\n", + usPolicyNsNames, + ) + + return wait.PollUntilContextCancel( + ctx, + 500*time.Millisecond, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + var usPolicy ngfAPI.UpstreamSettingsPolicy + var err error + + if err := k8sClient.Get(ctx, usPolicyNsNames, &usPolicy); err != nil { + return false, err + } + + if len(usPolicy.Status.Ancestors) == 0 { + GinkgoWriter.Printf("UpstreamSettingsPolicy %q does not have an ancestor status yet\n", usPolicy) + + return false, nil + } + + if len(usPolicy.Status.Ancestors) != 1 { + return false, fmt.Errorf("policy has %d ancestors, expected 1", len(usPolicy.Status.Ancestors)) + } + + ancestors := usPolicy.Status.Ancestors + + for i, ancestor := range ancestors { + if err := ancestorMustEqualTargetRef(ancestor, usPolicy.GetTargetRefs()[i], usPolicy.Namespace); err != nil { + return false, err + } + + err = ancestorStatusMustHaveAcceptedCondition(ancestor, condStatus, condReason) + } + return err == nil, err + }, + ) +} From c226ff5fb73c336ef2c6ffabd29e13f6b33d53bf Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Tue, 10 Dec 2024 19:12:56 -0800 Subject: [PATCH 2/8] update based on reviews --- tests/framework/crossplane.go | 37 ++-- .../invalid-svc-usps.yaml | 15 ++ ...lid-usps.yaml => invalid-target-usps.yaml} | 8 +- .../upstream-settings-policy/valid-usps.yaml | 24 +-- tests/suite/upstream_settings_test.go | 200 +++++++++++++----- 5 files changed, 190 insertions(+), 94 deletions(-) create mode 100644 tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml rename tests/suite/manifests/upstream-settings-policy/{invalid-usps.yaml => invalid-target-usps.yaml} (90%) diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index 33046ff868..5076160620 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -54,20 +54,15 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err continue } - for _, serverName := range expFieldCfg.Servers { - if directive.Directive == "server" && getServerName(directive.Block) == serverName { - for _, serverDirective := range directive.Block { - if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { - return nil - } else if serverDirective.Directive == "location" && - fieldExistsInLocation(serverDirective, expFieldCfg) { - return nil - } - } - } + err := validateServerBlockDirectives(expFieldCfg, *directive) + if err != nil { + return err } - return validateUpstreamDirectives(expFieldCfg, directive) + err = validateUpstreamDirectives(expFieldCfg, directive) + if err != nil { + return err + } } } @@ -79,6 +74,22 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, string(b)) } +func validateServerBlockDirectives(expFieldCfg ExpectedNginxField, directive Directive) error { + for _, serverName := range expFieldCfg.Servers { + if directive.Directive == "server" && getServerName(directive.Block) == serverName { + for _, serverDirective := range directive.Block { + if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { + return nil + } else if serverDirective.Directive == "location" && + fieldExistsInLocation(serverDirective, expFieldCfg) { + return nil + } + } + } + } + return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, directive.Directive) +} + func validateUpstreamDirectives(expFieldCfg ExpectedNginxField, directive *Directive) error { for _, upstreamName := range expFieldCfg.Upstreams { if directive.Directive == "upstream" && directive.Args[0] == upstreamName { @@ -89,7 +100,7 @@ func validateUpstreamDirectives(expFieldCfg ExpectedNginxField, directive *Direc } } } - return nil + return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, directive.Directive) } func getServerName(serverBlock Directives) string { diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml new file mode 100644 index 0000000000..2d8f21223b --- /dev/null +++ b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml @@ -0,0 +1,15 @@ +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: latte-svc-usp +spec: + zoneSize: 512k + targetRefs: + - group: core + kind: Service + name: latte + keepAlive: + connections: 10 + requests: 3 + time: 10s + timeout: 50s diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml similarity index 90% rename from tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml rename to tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml index 575121900a..f10b3bb945 100644 --- a/tests/suite/manifests/upstream-settings-policy/invalid-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml @@ -1,19 +1,15 @@ apiVersion: gateway.networking.k8s.io/v1 kind: Gateway metadata: - name: gateway-httproute-allowed + name: gateway-not-valid spec: gatewayClassName: nginx + addresses: "10.0.0.1" listeners: - name: http port: 80 protocol: HTTP hostname: "soda.example.com" - allowedRoutes: - namespaces: - from: All - kinds: - kind: GRPCRoute --- apiVersion: apps/v1 kind: Deployment diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml index 8b09e7ddc0..28966e104e 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml @@ -1,13 +1,16 @@ apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: coffee-svc-usp + name: multiple-http-svc-usp spec: zoneSize: 512k targetRefs: - group: core kind: Service name: coffee + - group: core + kind: Service + name: tea keepAlive: connections: 10 requests: 3 @@ -16,25 +19,6 @@ spec: --- apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy -metadata: - name: tea-multiple-svc-usp -spec: - zoneSize: 128k - targetRefs: - - group: core - kind: Service - name: tea - - group: core - kind: Service - name: coffee - keepAlive: - connections: 12 - requests: 31 - time: 20s - timeout: 40s ---- -apiVersion: gateway.nginx.org/v1alpha1 -kind: UpstreamSettingsPolicy metadata: name: grpc-svc-usp spec: diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index c7acb766ed..6207b0f5d1 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -46,24 +46,23 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Expect(resourceManager.DeleteNamespace(namespace)).To(Succeed()) }) - When("UpstreamSettingsPolicy are applied to the resources", func() { - snippetsFilter := []string{ + When("UpstreamSettingsPolicies target distinct Services", func() { + usps := []string{ "upstream-settings-policy/valid-usps.yaml", } BeforeAll(func() { - Expect(resourceManager.ApplyFromFiles(snippetsFilter, namespace)).To(Succeed()) + Expect(resourceManager.ApplyFromFiles(usps, namespace)).To(Succeed()) }) AfterAll(func() { - Expect(resourceManager.DeleteFromFiles(snippetsFilter, namespace)).To(Succeed()) + Expect(resourceManager.DeleteFromFiles(usps, namespace)).To(Succeed()) }) - Specify("usPolicies are accepted", func() { + Specify("they are accepted", func() { usPolicies := []string{ - "coffee-svc-usp", + "multiple-http-svc-usp", "grpc-svc-usp", - "tea-multiple-svc-usp", } for _, name := range usPolicies { @@ -124,98 +123,131 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) } }, - Entry("HTTPRoute", []framework.ExpectedNginxField{ + Entry("HTTP upstreams", []framework.ExpectedNginxField{ { - Directive: "zone default_coffee_80", - Value: "128k", + Directive: "zone", + Value: "default_coffee_80 128k", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", ValueSubstringAllowed: true, }, { Directive: "keepalive", Value: "12", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_requests", Value: "31", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_time", Value: "20s", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_timeout", Value: "40s", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", + }, + { + Directive: "zone", + Value: "default_tea_80 64k", + Upstreams: []string{"default_tea_80"}, + File: "http.conf", + ValueSubstringAllowed: true, + }, + { + Directive: "keepalive", + Value: "100", + Upstreams: []string{"default_tea_80"}, + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "55", + Upstreams: []string{"default_tea_80"}, + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "1m", + Upstreams: []string{"default_tea_80"}, + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "5h", + Upstreams: []string{"default_tea_80"}, + File: "http.conf", }, }), - Entry("GRPCRoute", []framework.ExpectedNginxField{ + Entry("GRPC upstreams", []framework.ExpectedNginxField{ { - Directive: "zone default_grpc-backend_8080", - Value: "512k", + Directive: "zone", + Value: "default_grpc-backend_8080 512k", Upstreams: []string{"default_grpc-backend_8080"}, - File: "nginx.conf", + File: "http.conf", ValueSubstringAllowed: true, }, { Directive: "keepalive", Value: "10", Upstreams: []string{"default_grpc-backend_8080"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_requests", Value: "3", Upstreams: []string{"default_grpc-backend_8080"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_time", Value: "10s", Upstreams: []string{"default_grpc-backend_8080"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_timeout", Value: "50s", Upstreams: []string{"default_grpc-backend_8080"}, - File: "nginx.conf", + File: "http.conf", }, }), ) }) }) - When("When multiple UpstreamSettings Policy target the same service", func() { - Specify("configuring distinct settings then directives are merged", func() { - files := []string{"upstream-settings-policy/valid-merge-usps.yaml"} + When("multiple UpstreamSettingsPolicies with overlapping settings target the same Service", func() { + Specify("configuring distinct settings, the policies are merged", func() { + files := []string{ + "upstream-settings-policy/valid-merge-usps.yaml", + } Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) Context("verify working traffic", func() { - It("should return a 200 response for HTTPRoute", func() { + It("should return a 200 response for HTTPRoutes", func() { port := 80 if portFwdPort != 0 { port = portFwdPort } - baseURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") Eventually( func() error { - return expectRequestToSucceed(baseURL, address, "URI: /coffee") + return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") }). WithTimeout(timeoutConfig.RequestTimeout). WithPolling(500 * time.Millisecond). @@ -246,53 +278,52 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) } }, - Entry("HTTPRoute", []framework.ExpectedNginxField{ + Entry("Coffee upstream", []framework.ExpectedNginxField{ { - Directive: "zone default_coffee_80", - Value: "64k", + Directive: "zone", + Value: "default_coffee_80 64k", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", ValueSubstringAllowed: true, }, { Directive: "keepalive", Value: "100", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_requests", Value: "55", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_time", Value: "1m", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, { Directive: "keepalive_timeout", Value: "5h", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", }, }), ) }) - - Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) - Specify("configuring overlapping settings, the policy created first wins", func() { + + Specify("the policy created first wins", func() { files := []string{"upstream-settings-policy/valid-usps-first-wins.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) Context("verify working traffic", func() { It("should return a 200 response for HTTPRoute", func() { @@ -335,23 +366,26 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) } }, - Entry("HTTPRoute", []framework.ExpectedNginxField{ + Entry("Coffee upstream", []framework.ExpectedNginxField{ { - Directive: "zone default_coffee_80", - Value: "128k", + Directive: "zone", + Value: "default_coffee_80 128k", Upstreams: []string{"default_coffee_80"}, - File: "nginx.conf", + File: "http.conf", ValueSubstringAllowed: true, }, }), ) }) + }) + + AfterEach(func() { Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) }) - When("UpstreamSettings Policy is invalid", func() { - Specify("if service mentioned in the target ref is not present", func() { + When("UpstreamSettingsPolicy targets a Service that has an invalid Gateway", func() { + Specify("usptreamSettingsPolicy has a condition TargetNotFound", func() { files := []string{"upstream-settings-policy/invalid-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) @@ -359,13 +393,25 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} Expect(waitForUSPolicyStatus(nsname, metav1.ConditionFalse, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + }) + }) + When("UpstreamSettingsPolicy targets a Service that does not exist", func() { + Specify("usptreamSettingsPolicy has no condition", func() { + files := []string{"upstream-settings-policy/invalid-svc-usps.yaml"} + + Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + + nsname := types.NamespacedName{Name: "latte-svc-usp", Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, metav1.ConditionFalse, v1alpha2.PolicyReasonAccepted)).To(BeFalse()) + Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) }) }) func waitForUSPolicyStatus( - usPolicyNsNames types.NamespacedName, + usPolicyNsName types.NamespacedName, condStatus metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason, ) error { @@ -373,8 +419,10 @@ func waitForUSPolicyStatus( defer cancel() GinkgoWriter.Printf( - "Waiting for UpstreamSettings Policy %q to have the condition Accepted/True/Accepted\n", - usPolicyNsNames, + "Waiting for UpstreamSettings Policy %q to have the condition %q/%q\n", + usPolicyNsName, + condStatus, + condReason, ) return wait.PollUntilContextCancel( @@ -385,7 +433,7 @@ func waitForUSPolicyStatus( var usPolicy ngfAPI.UpstreamSettingsPolicy var err error - if err := k8sClient.Get(ctx, usPolicyNsNames, &usPolicy); err != nil { + if err := k8sClient.Get(ctx, usPolicyNsName, &usPolicy); err != nil { return false, err } @@ -402,7 +450,7 @@ func waitForUSPolicyStatus( ancestors := usPolicy.Status.Ancestors for i, ancestor := range ancestors { - if err := ancestorMustEqualTargetRef(ancestor, usPolicy.GetTargetRefs()[i], usPolicy.Namespace); err != nil { + if err := ancestorMustEqualGatewayRef(ancestor, usPolicy.GetTargetRefs()[i], usPolicy.Namespace); err != nil { return false, err } @@ -412,3 +460,45 @@ func waitForUSPolicyStatus( }, ) } + +func ancestorMustEqualGatewayRef( + ancestor v1alpha2.PolicyAncestorStatus, + targetRef v1alpha2.LocalPolicyTargetReference, + namespace string, +) error { + if ancestor.ControllerName != ngfControllerName { + return fmt.Errorf( + "expected ancestor controller name to be %s, got %s", + ngfControllerName, + ancestor.ControllerName, + ) + } + + if ancestor.AncestorRef.Namespace == nil { + return fmt.Errorf("expected ancestor namespace to be %s, got nil", namespace) + } + + if string(*ancestor.AncestorRef.Namespace) != namespace { + return fmt.Errorf( + "expected ancestor namespace to be %s, got %s", + namespace, + string(*ancestor.AncestorRef.Namespace), + ) + } + + ancestorRef := ancestor.AncestorRef + + if ancestorRef.Name != targetRef.Name { + return fmt.Errorf("expected ancestorRef to have name %s, got %s", targetRef.Name, ancestorRef.Name) + } + + if ancestorRef.Kind == nil { + return fmt.Errorf("expected ancestorRef to have kind %s, got nil", targetRef.Kind) + } + + if *ancestorRef.Kind != targetRef.Kind { + return fmt.Errorf("expected ancestorRef to have kind %s, got %s", targetRef.Kind, string(*ancestorRef.Kind)) + } + + return nil +} From bfdc3deadeb01e40641dece818aa32ea90b0851a Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 13 Dec 2024 10:56:31 -0800 Subject: [PATCH 3/8] update based on reviews --- tests/framework/crossplane.go | 24 ++- .../invalid-svc-usps.yaml | 2 +- .../valid-merge-usps.yaml | 2 +- .../valid-usps-first-wins.yaml | 4 +- .../upstream-settings-policy/valid-usps.yaml | 10 +- tests/suite/upstream_settings_test.go | 183 ++++++++++-------- 6 files changed, 124 insertions(+), 101 deletions(-) diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index 5076160620..e6b9689465 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -30,7 +30,7 @@ type ExpectedNginxField struct { Location string // Servers are the server names that the directive should exist in. Servers []string - // Upstream are the upstream names that the directive should exist in. + // Upstreams are the upstream names that the directive should exist in. Upstreams []string // ValueSubstringAllowed allows the expected value to be a substring of the real value. // This makes it easier for cases when real values are complex file names or contain things we @@ -54,13 +54,11 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err continue } - err := validateServerBlockDirectives(expFieldCfg, *directive) - if err != nil { + if err := validateServerBlockDirectives(expFieldCfg, *directive); err != nil { return err } - err = validateUpstreamDirectives(expFieldCfg, directive) - if err != nil { + if err := validateUpstreamDirectives(expFieldCfg, directive); err != nil { return err } } @@ -78,29 +76,29 @@ func validateServerBlockDirectives(expFieldCfg ExpectedNginxField, directive Dir for _, serverName := range expFieldCfg.Servers { if directive.Directive == "server" && getServerName(directive.Block) == serverName { for _, serverDirective := range directive.Block { - if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { - return nil + if expFieldCfg.Location == "" && !expFieldCfg.fieldFound(serverDirective) { + return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, serverDirective.Directive) } else if serverDirective.Directive == "location" && - fieldExistsInLocation(serverDirective, expFieldCfg) { - return nil + !fieldExistsInLocation(serverDirective, expFieldCfg) { + return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, serverDirective.Directive) } } } } - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, directive.Directive) + return nil } func validateUpstreamDirectives(expFieldCfg ExpectedNginxField, directive *Directive) error { for _, upstreamName := range expFieldCfg.Upstreams { if directive.Directive == "upstream" && directive.Args[0] == upstreamName { for _, upstreamDirective := range directive.Block { - if expFieldCfg.fieldFound(upstreamDirective) { - return nil + if !expFieldCfg.fieldFound(upstreamDirective) { + return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, upstreamDirective.Directive) } } } } - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, directive.Directive) + return nil } func getServerName(serverBlock Directives) string { diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml index 2d8f21223b..4bfe75788c 100644 --- a/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml @@ -1,7 +1,7 @@ apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: latte-svc-usp + name: does-not-exist spec: zoneSize: 512k targetRefs: diff --git a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml index d248f49cfb..bc232fa20e 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml @@ -3,7 +3,7 @@ kind: UpstreamSettingsPolicy metadata: name: coffee-svc-usp-1 spec: - zoneSize: 64k + zoneSize: 1g targetRefs: - group: core kind: Service diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml index 1600aea97a..3bdf1dabba 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml @@ -1,7 +1,7 @@ apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: coffee-svc-usp-1 + name: z-coffee-svc-usp spec: zoneSize: 64k targetRefs: @@ -12,7 +12,7 @@ spec: apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: coffee-svc-usp-2 + name: a-coffee-svc-usp spec: zoneSize: 128k targetRefs: diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml index 28966e104e..6865c5df7b 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml @@ -3,7 +3,6 @@ kind: UpstreamSettingsPolicy metadata: name: multiple-http-svc-usp spec: - zoneSize: 512k targetRefs: - group: core kind: Service @@ -26,8 +25,9 @@ spec: group: core kind: Service name: grpc-backend + zoneSize: 64k keepAlive: - connections: 10 - requests: 3 - time: 10s - timeout: 50s + connections: 100 + requests: 45 + time: 1m + timeout: 5h diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index 6207b0f5d1..c9b50358d3 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" @@ -66,9 +67,10 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { } for _, name := range usPolicies { - nsname := types.NamespacedName{Name: name, Namespace: namespace} + uspolicyNsName := types.NamespacedName{Name: name, Namespace: namespace} - err := waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted) + gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + err := waitForUSPolicyStatus(uspolicyNsName, gatewayNsName, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted) Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name)) } }) @@ -125,97 +127,70 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { }, Entry("HTTP upstreams", []framework.ExpectedNginxField{ { - Directive: "zone", - Value: "default_coffee_80 128k", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - ValueSubstringAllowed: true, - }, - { - Directive: "keepalive", - Value: "12", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - { - Directive: "keepalive_requests", - Value: "31", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - { - Directive: "keepalive_time", - Value: "20s", + Directive: "zone", + Value: "default_coffee_80 512k", Upstreams: []string{"default_coffee_80"}, File: "http.conf", }, { - Directive: "keepalive_timeout", - Value: "40s", - Upstreams: []string{"default_coffee_80"}, + Directive: "zone", + Value: "default_tea_80 512k", + Upstreams: []string{"default_tea_80"}, File: "http.conf", }, - { - Directive: "zone", - Value: "default_tea_80 64k", - Upstreams: []string{"default_tea_80"}, - File: "http.conf", - ValueSubstringAllowed: true, - }, { Directive: "keepalive", - Value: "100", - Upstreams: []string{"default_tea_80"}, + Value: "10", + Upstreams: []string{"default_coffee_80", "default_tea_80"}, File: "http.conf", }, { Directive: "keepalive_requests", - Value: "55", - Upstreams: []string{"default_tea_80"}, + Value: "3", + Upstreams: []string{"default_coffee_80", "default_tea_80"}, File: "http.conf", }, { Directive: "keepalive_time", - Value: "1m", - Upstreams: []string{"default_tea_80"}, + Value: "10s", + Upstreams: []string{"default_coffee_80", "default_tea_80"}, File: "http.conf", }, { Directive: "keepalive_timeout", - Value: "5h", - Upstreams: []string{"default_tea_80"}, + Value: "50s", + Upstreams: []string{"default_coffee_80", "default_tea_80"}, File: "http.conf", }, }), Entry("GRPC upstreams", []framework.ExpectedNginxField{ { - Directive: "zone", - Value: "default_grpc-backend_8080 512k", - Upstreams: []string{"default_grpc-backend_8080"}, - File: "http.conf", - ValueSubstringAllowed: true, + Directive: "zone", + Value: "default_grpc-backend_8080 64k", + Upstreams: []string{"default_grpc-backend_8080"}, + File: "http.conf", }, { Directive: "keepalive", - Value: "10", + Value: "100", Upstreams: []string{"default_grpc-backend_8080"}, File: "http.conf", }, { Directive: "keepalive_requests", - Value: "3", + Value: "45", Upstreams: []string{"default_grpc-backend_8080"}, File: "http.conf", }, { Directive: "keepalive_time", - Value: "10s", + Value: "1m", Upstreams: []string{"default_grpc-backend_8080"}, File: "http.conf", }, { Directive: "keepalive_timeout", - Value: "50s", + Value: "5h", Upstreams: []string{"default_grpc-backend_8080"}, File: "http.conf", }, @@ -231,11 +206,22 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { } Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) + Expect(waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + )).To(Succeed()) nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) + Expect(waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + )).To(Succeed()) Context("verify working traffic", func() { It("should return a 200 response for HTTPRoutes", func() { @@ -280,11 +266,10 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { }, Entry("Coffee upstream", []framework.ExpectedNginxField{ { - Directive: "zone", - Value: "default_coffee_80 64k", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - ValueSubstringAllowed: true, + Directive: "zone", + Value: "default_coffee_80 1g", + Upstreams: []string{"default_coffee_80"}, + File: "http.conf", }, { Directive: "keepalive", @@ -319,11 +304,22 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { files := []string{"upstream-settings-policy/valid-usps-first-wins.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) - - nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted)).To(Succeed()) + gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + nsname := types.NamespacedName{Name: "a-coffee-svc-usp", Namespace: namespace} + Expect(waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + )).To(Succeed()) + + nsname = types.NamespacedName{Name: "z-coffee-svc-usp", Namespace: namespace} + Expect(waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + )).To(Succeed()) Context("verify working traffic", func() { It("should return a 200 response for HTTPRoute", func() { @@ -368,11 +364,10 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { }, Entry("Coffee upstream", []framework.ExpectedNginxField{ { - Directive: "zone", - Value: "default_coffee_80 128k", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - ValueSubstringAllowed: true, + Directive: "zone", + Value: "default_coffee_80 128k", + Upstreams: []string{"default_coffee_80"}, + File: "http.conf", }, }), ) @@ -385,13 +380,19 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { }) When("UpstreamSettingsPolicy targets a Service that has an invalid Gateway", func() { - Specify("usptreamSettingsPolicy has a condition TargetNotFound", func() { + Specify("upstreamSettingsPolicy has a condition TargetNotFound", func() { files := []string{"upstream-settings-policy/invalid-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionFalse, v1alpha2.PolicyReasonTargetNotFound)).To(Succeed()) + gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + Expect(waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionFalse, + v1alpha2.PolicyReasonTargetNotFound, + )).To(Succeed()) Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) @@ -402,8 +403,27 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "latte-svc-usp", Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, metav1.ConditionFalse, v1alpha2.PolicyReasonAccepted)).To(BeFalse()) + nsname := types.NamespacedName{Name: "does-not-exists", Namespace: namespace} + gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + + Expect(waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionFalse, + v1alpha2.PolicyReasonAccepted, + )).To(BeFalse()) + + Consistently( + func() bool { + return waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + ) != nil + }).WithTimeout(timeoutConfig.GetTimeout). + WithPolling(500 * time.Millisecond). + Should(BeTrue()) Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) @@ -412,6 +432,7 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { func waitForUSPolicyStatus( usPolicyNsName types.NamespacedName, + gatewayNsName types.NamespacedName, condStatus metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason, ) error { @@ -449,8 +470,8 @@ func waitForUSPolicyStatus( ancestors := usPolicy.Status.Ancestors - for i, ancestor := range ancestors { - if err := ancestorMustEqualGatewayRef(ancestor, usPolicy.GetTargetRefs()[i], usPolicy.Namespace); err != nil { + for _, ancestor := range ancestors { + if err := ancestorMustEqualGatewayRef(ancestor, gatewayNsName, usPolicy.Namespace); err != nil { return false, err } @@ -463,7 +484,7 @@ func waitForUSPolicyStatus( func ancestorMustEqualGatewayRef( ancestor v1alpha2.PolicyAncestorStatus, - targetRef v1alpha2.LocalPolicyTargetReference, + gatewayNsName types.NamespacedName, namespace string, ) error { if ancestor.ControllerName != ngfControllerName { @@ -488,16 +509,20 @@ func ancestorMustEqualGatewayRef( ancestorRef := ancestor.AncestorRef - if ancestorRef.Name != targetRef.Name { - return fmt.Errorf("expected ancestorRef to have name %s, got %s", targetRef.Name, ancestorRef.Name) + if string(ancestorRef.Name) != gatewayNsName.Name { + return fmt.Errorf("expected ancestorRef to have name %s, got %s", gatewayNsName, ancestorRef.Name) } if ancestorRef.Kind == nil { - return fmt.Errorf("expected ancestorRef to have kind %s, got nil", targetRef.Kind) + return fmt.Errorf("expected ancestorRef to have kind %s, got nil", "Gateway") } - if *ancestorRef.Kind != targetRef.Kind { - return fmt.Errorf("expected ancestorRef to have kind %s, got %s", targetRef.Kind, string(*ancestorRef.Kind)) + if *ancestorRef.Kind != gatewayv1.Kind("Gateway") { + return fmt.Errorf( + "expected ancestorRef to have kind %s, got %s", + gatewayv1.Kind("Gateway"), + string(*ancestorRef.Kind), + ) } return nil From 1e04e1dc7705f45b1692499e893e64f115087256 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 19 Dec 2024 09:40:40 -0700 Subject: [PATCH 4/8] update tests as needed --- tests/framework/crossplane.go | 78 ++-- tests/suite/client_settings_test.go | 16 +- .../invalid-target-usps.yaml | 55 +-- .../upstream-settings-policy/routes.yaml | 19 + .../valid-merge-usps.yaml | 32 +- .../valid-usps-first-wins.yaml | 21 - .../upstream-settings-policy/valid-usps.yaml | 8 +- tests/suite/snippets_filter_test.go | 8 +- tests/suite/upstream_settings_test.go | 379 ++++++++---------- 9 files changed, 304 insertions(+), 312 deletions(-) delete mode 100644 tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index e6b9689465..f9e109a475 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -28,10 +28,10 @@ type ExpectedNginxField struct { File string // Location is the location name that the directive should exist in. Location string - // Servers are the server names that the directive should exist in. - Servers []string - // Upstreams are the upstream names that the directive should exist in. - Upstreams []string + // Server is the server name that the directive should exist in. + Server string + // Upstream is the upstream name that the directive should exist in. + Upstream string // ValueSubstringAllowed allows the expected value to be a substring of the real value. // This makes it easier for cases when real values are complex file names or contain things we // don't care about, and we just want to check if a substring exists. @@ -41,64 +41,74 @@ type ExpectedNginxField struct { // ValidateNginxFieldExists accepts the nginx config and the configuration for the expected field, // and returns whether or not that field exists where it should. func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) error { + var directiveFoundInServer, directiveFoundInUpstream bool + + b, err := json.Marshal(conf) + if err != nil { + return fmt.Errorf("error marshaling nginx config: %w", err) + } + for _, config := range conf.Config { if !strings.Contains(config.File, expFieldCfg.File) { continue } for _, directive := range config.Parsed { - if len(expFieldCfg.Servers) == 0 { + if len(expFieldCfg.Server) == 0 && len(expFieldCfg.Upstream) == 0 { if expFieldCfg.fieldFound(directive) { return nil } continue } - if err := validateServerBlockDirectives(expFieldCfg, *directive); err != nil { - return err + directiveFoundInServer = validateServerBlockDirectives(expFieldCfg, *directive) + + directiveFoundInUpstream = validateUpstreamDirectives(expFieldCfg, *directive) + + if len(expFieldCfg.Server) > 0 && directiveFoundInServer { + return nil } - if err := validateUpstreamDirectives(expFieldCfg, directive); err != nil { - return err + if len(expFieldCfg.Upstream) > 0 && directiveFoundInUpstream { + return nil } } } - b, err := json.Marshal(conf) - if err != nil { - return fmt.Errorf("error marshaling nginx config: %w", err) - } - - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, string(b)) + return fmt.Errorf("directive %s not found in: nginx config %s", expFieldCfg.Directive, string(b)) } -func validateServerBlockDirectives(expFieldCfg ExpectedNginxField, directive Directive) error { - for _, serverName := range expFieldCfg.Servers { - if directive.Directive == "server" && getServerName(directive.Block) == serverName { - for _, serverDirective := range directive.Block { - if expFieldCfg.Location == "" && !expFieldCfg.fieldFound(serverDirective) { - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, serverDirective.Directive) - } else if serverDirective.Directive == "location" && - !fieldExistsInLocation(serverDirective, expFieldCfg) { - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, serverDirective.Directive) - } +func validateServerBlockDirectives( + expFieldCfg ExpectedNginxField, + directive Directive, +) bool { + var fieldFound bool + if directive.Directive == "server" && getServerName(directive.Block) == expFieldCfg.Server { + for _, serverDirective := range directive.Block { + if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { + fieldFound = true + } else if serverDirective.Directive == "location" && + fieldExistsInLocation(serverDirective, expFieldCfg) { + fieldFound = true } } } - return nil + return fieldFound } -func validateUpstreamDirectives(expFieldCfg ExpectedNginxField, directive *Directive) error { - for _, upstreamName := range expFieldCfg.Upstreams { - if directive.Directive == "upstream" && directive.Args[0] == upstreamName { - for _, upstreamDirective := range directive.Block { - if !expFieldCfg.fieldFound(upstreamDirective) { - return fmt.Errorf("field not found; expected: %+v\nNGINX conf: %s", expFieldCfg, upstreamDirective.Directive) - } +func validateUpstreamDirectives( + expFieldCfg ExpectedNginxField, + directive Directive, +) bool { + var fieldFound bool + if directive.Directive == "upstream" && directive.Args[0] == expFieldCfg.Upstream { + for _, directive := range directive.Block { + if expFieldCfg.fieldFound(directive) { + fieldFound = true } } } - return nil + return fieldFound } func getServerName(serverBlock Directives) string { diff --git a/tests/suite/client_settings_test.go b/tests/suite/client_settings_test.go index b5e5a8ec48..f7ab05c88f 100644 --- a/tests/suite/client_settings_test.go +++ b/tests/suite/client_settings_test.go @@ -117,7 +117,13 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_gw-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"*.example.com", "cafe.example.com"}, + Server: "*.example.com", + }, + { + Directive: "include", + Value: fmt.Sprintf("%s_gw-csp.conf", filePrefix), + File: "http.conf", + Server: "cafe.example.com", }, { Directive: "client_max_body_size", @@ -150,7 +156,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_coffee-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", Location: "/coffee", }, { @@ -164,7 +170,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_tea-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", Location: "/tea", }, { @@ -178,7 +184,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_soda-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", Location: "/soda", }, { @@ -192,7 +198,7 @@ var _ = Describe("ClientSettingsPolicy", Ordered, Label("functional", "cspolicy" Directive: "include", Value: fmt.Sprintf("%s_grpc-route-csp.conf", filePrefix), File: "http.conf", - Servers: []string{"*.example.com"}, + Server: "*.example.com", Location: "/helloworld.Greeter/SayHello", }, { diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml index f10b3bb945..5a223f2f3b 100644 --- a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml @@ -4,12 +4,13 @@ metadata: name: gateway-not-valid spec: gatewayClassName: nginx - addresses: "10.0.0.1" + addresses: + - value: "10.0.0.1" listeners: - - name: http - port: 80 - protocol: HTTP - hostname: "soda.example.com" + - name: http + port: 80 + protocol: HTTP + hostname: "soda.example.com" --- apiVersion: apps/v1 kind: Deployment @@ -26,10 +27,10 @@ spec: app: soda spec: containers: - - name: soda - image: nginxdemos/nginx-hello:plain-text - ports: - - containerPort: 8080 + - name: soda + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 --- apiVersion: v1 kind: Service @@ -37,31 +38,31 @@ metadata: name: soda spec: ports: - - port: 80 - targetPort: 8080 - protocol: TCP - name: http + - port: 80 + targetPort: 8080 + protocol: TCP + name: http selector: app: soda --- apiVersion: gateway.networking.k8s.io/v1 kind: HTTPRoute metadata: - name: soda + name: sode spec: parentRefs: - - name: gateway-httproute-allowed - sectionName: http + - name: gateway-not-valid + sectionName: http hostnames: - - "soda.example.com" + - "soda.example.com" rules: - - matches: - - path: - type: Exact - value: /soda - backendRefs: - - name: soda - port: 80 + - matches: + - path: + type: Exact + value: /soda + backendRefs: + - name: soda + port: 80 --- apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy @@ -70,9 +71,9 @@ metadata: spec: zoneSize: 512k targetRefs: - - group: core - kind: Service - name: soda + - group: core + kind: Service + name: soda keepAlive: connections: 10 requests: 3 diff --git a/tests/suite/manifests/upstream-settings-policy/routes.yaml b/tests/suite/manifests/upstream-settings-policy/routes.yaml index 37defe1a3e..f26e705a40 100644 --- a/tests/suite/manifests/upstream-settings-policy/routes.yaml +++ b/tests/suite/manifests/upstream-settings-policy/routes.yaml @@ -18,6 +18,25 @@ spec: port: 80 --- apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: tea +spec: + parentRefs: + - name: gateway + sectionName: http + hostnames: + - "cafe.example.com" + rules: + - matches: + - path: + type: Exact + value: /tea + backendRefs: + - name: tea + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 kind: GRPCRoute metadata: name: grpc-route diff --git a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml index bc232fa20e..cdbab6c269 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml @@ -1,18 +1,20 @@ apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: coffee-svc-usp-1 + name: merge-usp-1 spec: - zoneSize: 1g targetRefs: - group: core kind: Service name: coffee + keepAlive: + time: 1m + timeout: 5h --- apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: coffee-svc-usp-2 + name: merge-usp-2 spec: targetRefs: - group: core @@ -21,5 +23,25 @@ spec: keepAlive: connections: 100 requests: 55 - time: 1m - timeout: 5h +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: z-usp-wins +spec: + zoneSize: 64k + targetRefs: + - group: core + kind: Service + name: tea +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: a-usp +spec: + zoneSize: 128k + targetRefs: + - group: core + kind: Service + name: tea diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml deleted file mode 100644 index 3bdf1dabba..0000000000 --- a/tests/suite/manifests/upstream-settings-policy/valid-usps-first-wins.yaml +++ /dev/null @@ -1,21 +0,0 @@ -apiVersion: gateway.nginx.org/v1alpha1 -kind: UpstreamSettingsPolicy -metadata: - name: z-coffee-svc-usp -spec: - zoneSize: 64k - targetRefs: - - group: core - kind: Service - name: coffee ---- -apiVersion: gateway.nginx.org/v1alpha1 -kind: UpstreamSettingsPolicy -metadata: - name: a-coffee-svc-usp -spec: - zoneSize: 128k - targetRefs: - - group: core - kind: Service - name: coffee diff --git a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml index 6865c5df7b..cb4e4bf058 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-usps.yaml @@ -21,10 +21,10 @@ kind: UpstreamSettingsPolicy metadata: name: grpc-svc-usp spec: - targetRef: - group: core - kind: Service - name: grpc-backend + targetRefs: + - group: core + kind: Service + name: grpc-backend zoneSize: 64k keepAlive: connections: 100 diff --git a/tests/suite/snippets_filter_test.go b/tests/suite/snippets_filter_test.go index 632a8199e0..e92e44d0ad 100644 --- a/tests/suite/snippets_filter_test.go +++ b/tests/suite/snippets_filter_test.go @@ -149,7 +149,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter { Directive: "include", Value: fmt.Sprintf("%s%s", httpServerContext, httpRouteSuffix), - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", File: "http.conf", }, { @@ -157,7 +157,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Value: fmt.Sprintf("%s%s", httpServerLocationContext, httpRouteSuffix), File: "http.conf", Location: "/coffee", - Servers: []string{"cafe.example.com"}, + Server: "cafe.example.com", }, { Directive: "keepalive_time", @@ -194,7 +194,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter { Directive: "include", Value: fmt.Sprintf("%s%s", httpServerContext, grpcRouteSuffix), - Servers: []string{"*.example.com"}, + Server: "*.example.com", File: "http.conf", }, { @@ -207,7 +207,7 @@ var _ = Describe("SnippetsFilter", Ordered, Label("functional", "snippets-filter Value: fmt.Sprintf("%s%s", httpServerLocationContext, grpcRouteSuffix), File: "http.conf", Location: "/helloworld.Greeter/SayHello", - Servers: []string{"*.example.com"}, + Server: "*.example.com", }, }), ) diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index c9b50358d3..0c634a62d6 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -19,7 +19,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/tests/framework" ) -var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { +var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolicy"), func() { var ( files = []string{ "upstream-settings-policy/cafe.yaml", @@ -61,22 +61,28 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { }) Specify("they are accepted", func() { - usPolicies := []string{ - "multiple-http-svc-usp", - "grpc-svc-usp", + usPolicies := map[string]int{ + "multiple-http-svc-usp": 2, + "grpc-svc-usp": 1, } - for _, name := range usPolicies { + for name, ancestorCount := range usPolicies { uspolicyNsName := types.NamespacedName{Name: name, Namespace: namespace} gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} - err := waitForUSPolicyStatus(uspolicyNsName, gatewayNsName, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted) + err := waitForUSPolicyStatus( + uspolicyNsName, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + ancestorCount, + ) Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name)) } }) Context("verify working traffic", func() { - It("should return a 200 response for HTTPRoute", func() { + It("should return a 200 response for HTTPRoutes", func() { port := 80 if portFwdPort != 0 { port = portFwdPort @@ -116,9 +122,6 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Expect(err).ToNot(HaveOccurred()) }) - // TODO: important - // The directive file and field value need to be updated based on the - // implementation of the UpstreamSettingsPolicy and how they are specified in the config files. DescribeTable("are set properly for", func(expCfgs []framework.ExpectedNginxField) { for _, expCfg := range expCfgs { @@ -128,70 +131,88 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { Entry("HTTP upstreams", []framework.ExpectedNginxField{ { Directive: "zone", - Value: "default_coffee_80 512k", - Upstreams: []string{"default_coffee_80"}, + Value: "uspolicy_coffee_80 512k", + Upstream: "uspolicy_coffee_80", File: "http.conf", }, { Directive: "zone", - Value: "default_tea_80 512k", - Upstreams: []string{"default_tea_80"}, + Value: "uspolicy_tea_80 512k", + Upstream: "uspolicy_tea_80", File: "http.conf", }, { Directive: "keepalive", Value: "10", - Upstreams: []string{"default_coffee_80", "default_tea_80"}, + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "3", + Upstream: "uspolicy_coffee_80", File: "http.conf", }, { Directive: "keepalive_requests", Value: "3", - Upstreams: []string{"default_coffee_80", "default_tea_80"}, + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "10s", + Upstream: "uspolicy_coffee_80", File: "http.conf", }, { Directive: "keepalive_time", Value: "10s", - Upstreams: []string{"default_coffee_80", "default_tea_80"}, + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "50s", + Upstream: "uspolicy_coffee_80", File: "http.conf", }, { Directive: "keepalive_timeout", Value: "50s", - Upstreams: []string{"default_coffee_80", "default_tea_80"}, + Upstream: "uspolicy_tea_80", File: "http.conf", }, }), Entry("GRPC upstreams", []framework.ExpectedNginxField{ { Directive: "zone", - Value: "default_grpc-backend_8080 64k", - Upstreams: []string{"default_grpc-backend_8080"}, + Value: "uspolicy_grpc-backend_8080 64k", + Upstream: "uspolicy_grpc-backend_8080", File: "http.conf", }, { Directive: "keepalive", Value: "100", - Upstreams: []string{"default_grpc-backend_8080"}, + Upstream: "uspolicy_grpc-backend_8080", File: "http.conf", }, { Directive: "keepalive_requests", Value: "45", - Upstreams: []string{"default_grpc-backend_8080"}, + Upstream: "uspolicy_grpc-backend_8080", File: "http.conf", }, { Directive: "keepalive_time", Value: "1m", - Upstreams: []string{"default_grpc-backend_8080"}, + Upstream: "uspolicy_grpc-backend_8080", File: "http.conf", }, { Directive: "keepalive_timeout", Value: "5h", - Upstreams: []string{"default_grpc-backend_8080"}, + Upstream: "uspolicy_grpc-backend_8080", File: "http.conf", }, }), @@ -200,219 +221,151 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { }) When("multiple UpstreamSettingsPolicies with overlapping settings target the same Service", func() { - Specify("configuring distinct settings, the policies are merged", func() { - files := []string{ - "upstream-settings-policy/valid-merge-usps.yaml", - } - Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) + usps := []string{ + "upstream-settings-policy/valid-merge-usps.yaml", + } - gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} - nsname := types.NamespacedName{Name: "coffee-svc-usp-1", Namespace: namespace} - Expect(waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionTrue, - v1alpha2.PolicyReasonAccepted, - )).To(Succeed()) - - nsname = types.NamespacedName{Name: "coffee-svc-usp-2", Namespace: namespace} - Expect(waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionTrue, - v1alpha2.PolicyReasonAccepted, - )).To(Succeed()) - - Context("verify working traffic", func() { - It("should return a 200 response for HTTPRoutes", func() { - port := 80 - if portFwdPort != 0 { - port = portFwdPort - } - baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") - - Eventually( - func() error { - return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") - }). - WithTimeout(timeoutConfig.RequestTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - }) - }) + BeforeAll(func() { + Expect(resourceManager.ApplyFromFiles(usps, namespace)).To(Succeed()) + }) - Context("nginx directives", func() { - var conf *framework.Payload + AfterAll(func() { + Expect(resourceManager.DeleteFromFiles(usps, namespace)).To(Succeed()) + }) - BeforeAll(func() { - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) + DescribeTable("upstreamSettingsPolicy status is set as expected", + func(name string, status metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason, ancestorCount int) { + gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + nsname := types.NamespacedName{Name: name, Namespace: namespace} + Expect(waitForUSPolicyStatus(nsname, gatewayNsName, status, condReason, ancestorCount)).To(Succeed()) + }, + Entry("uspolicy merge-usp-1", "merge-usp-1", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, 1), + Entry("uspolicy merge-usp-2", "merge-usp-2", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, 1), + Entry("uspolicy a-usp", "a-usp", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, 1), + Entry("uspolicy z-usp", "z-usp-wins", metav1.ConditionFalse, v1alpha2.PolicyReasonConflicted, 1), + ) - ngfPodName := podNames[0] + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoutes", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") - conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) - Expect(err).ToNot(HaveOccurred()) - }) + Eventually( + func() error { + return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(1000 * time.Millisecond). + Should(Succeed()) - // TODO: important - // The directive file and field value need to be updated based on the - // implementation of the UpstreamSettingsPolicy and how they are specified in the config files. - DescribeTable("are set properly for", - func(expCfgs []framework.ExpectedNginxField) { - for _, expCfg := range expCfgs { - Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) - } - }, - Entry("Coffee upstream", []framework.ExpectedNginxField{ - { - Directive: "zone", - Value: "default_coffee_80 1g", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - { - Directive: "keepalive", - Value: "100", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - { - Directive: "keepalive_requests", - Value: "55", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - { - Directive: "keepalive_time", - Value: "1m", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - { - Directive: "keepalive_timeout", - Value: "5h", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - }), - ) + Eventually( + func() error { + return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(1000 * time.Millisecond). + Should(Succeed()) }) }) - Specify("the policy created first wins", func() { - files := []string{"upstream-settings-policy/valid-usps-first-wins.yaml"} - Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - - gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} - nsname := types.NamespacedName{Name: "a-coffee-svc-usp", Namespace: namespace} - Expect(waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionTrue, - v1alpha2.PolicyReasonAccepted, - )).To(Succeed()) - - nsname = types.NamespacedName{Name: "z-coffee-svc-usp", Namespace: namespace} - Expect(waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionTrue, - v1alpha2.PolicyReasonAccepted, - )).To(Succeed()) - - Context("verify working traffic", func() { - It("should return a 200 response for HTTPRoute", func() { - port := 80 - if portFwdPort != 0 { - port = portFwdPort - } - baseURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") - - Eventually( - func() error { - return expectRequestToSucceed(baseURL, address, "URI: /coffee") - }). - WithTimeout(timeoutConfig.RequestTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - }) - }) - - Context("nginx directives", func() { - var conf *framework.Payload - - BeforeAll(func() { - podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) - Expect(err).ToNot(HaveOccurred()) - Expect(podNames).To(HaveLen(1)) + Context("nginx directives", func() { + var conf *framework.Payload - ngfPodName := podNames[0] + BeforeAll(func() { + podNames, err := framework.GetReadyNGFPodNames(k8sClient, ngfNamespace, releaseName, timeoutConfig.GetTimeout) + Expect(err).ToNot(HaveOccurred()) + Expect(podNames).To(HaveLen(1)) - conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) - Expect(err).ToNot(HaveOccurred()) - }) + ngfPodName := podNames[0] - // TODO: important - // The directive file and field value need to be updated based on the - // implementation of the UpstreamSettingsPolicy and how they are specified in the config files. - DescribeTable("are set properly for", - func(expCfgs []framework.ExpectedNginxField) { - for _, expCfg := range expCfgs { - Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) - } - }, - Entry("Coffee upstream", []framework.ExpectedNginxField{ - { - Directive: "zone", - Value: "default_coffee_80 128k", - Upstreams: []string{"default_coffee_80"}, - File: "http.conf", - }, - }), - ) + conf, err = resourceManager.GetNginxConfig(ngfPodName, ngfNamespace) + Expect(err).ToNot(HaveOccurred()) }) - }) - AfterEach(func() { - Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) + DescribeTable("are set properly for", + func(expCfgs []framework.ExpectedNginxField) { + for _, expCfg := range expCfgs { + Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) + } + }, + Entry("Coffee upstream", []framework.ExpectedNginxField{ + { + Directive: "zone", + Value: "uspolicy_tea_80 128k", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "zone", + Value: "uspolicy_coffee_80 512k", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "100", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "55", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "1m", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "5h", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + }), + ) }) }) - When("UpstreamSettingsPolicy targets a Service that has an invalid Gateway", func() { - Specify("upstreamSettingsPolicy has a condition TargetNotFound", func() { - files := []string{"upstream-settings-policy/invalid-usps.yaml"} + When("UpstreamSettingsPolicy targets a Service that does not exists", func() { + Specify("upstreamSettingsPolicy has no condition set", func() { + files := []string{"upstream-settings-policy/invalid-svc-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} + nsname := types.NamespacedName{Name: "does-not-exist", Namespace: namespace} gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} - Expect(waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionFalse, - v1alpha2.PolicyReasonTargetNotFound, - )).To(Succeed()) + Consistently( + func() bool { + return waitForUSPolicyStatus( + nsname, + gatewayNsName, + metav1.ConditionTrue, + v1alpha2.PolicyReasonAccepted, + 1, + ) != nil + }).WithTimeout(timeoutConfig.GetTimeout). + WithPolling(500 * time.Millisecond). + Should(BeTrue()) Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) }) - When("UpstreamSettingsPolicy targets a Service that does not exist", func() { - Specify("usptreamSettingsPolicy has no condition", func() { - files := []string{"upstream-settings-policy/invalid-svc-usps.yaml"} + When("UpstreamSettingsPolicy targets a Service that has an invalid Gateway", func() { + Specify("upstreamSettingsPolicy has no condition set", func() { + files := []string{"upstream-settings-policy/invalid-target-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "does-not-exists", Namespace: namespace} - gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} - - Expect(waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionFalse, - v1alpha2.PolicyReasonAccepted, - )).To(BeFalse()) - + nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} + gatewayNsName := types.NamespacedName{Name: "gateway-not-valid", Namespace: namespace} Consistently( func() bool { return waitForUSPolicyStatus( @@ -420,6 +373,7 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("uspolicy"), func() { gatewayNsName, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, + 1, ) != nil }).WithTimeout(timeoutConfig.GetTimeout). WithPolling(500 * time.Millisecond). @@ -435,8 +389,9 @@ func waitForUSPolicyStatus( gatewayNsName types.NamespacedName, condStatus metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason, + expectedAncestorCount int, ) error { - ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout) + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout*2) defer cancel() GinkgoWriter.Printf( @@ -448,7 +403,7 @@ func waitForUSPolicyStatus( return wait.PollUntilContextCancel( ctx, - 500*time.Millisecond, + 2000*time.Millisecond, true, /* poll immediately */ func(ctx context.Context) (bool, error) { var usPolicy ngfAPI.UpstreamSettingsPolicy @@ -464,7 +419,7 @@ func waitForUSPolicyStatus( return false, nil } - if len(usPolicy.Status.Ancestors) != 1 { + if len(usPolicy.Status.Ancestors) != expectedAncestorCount { return false, fmt.Errorf("policy has %d ancestors, expected 1", len(usPolicy.Status.Ancestors)) } From 7eaf06be0fb978779091f0c38c02d2b29b98d8d1 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 19 Dec 2024 11:11:15 -0700 Subject: [PATCH 5/8] update function names for crossplane --- tests/framework/crossplane.go | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index f9e109a475..ea2f5bc838 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -41,8 +41,6 @@ type ExpectedNginxField struct { // ValidateNginxFieldExists accepts the nginx config and the configuration for the expected field, // and returns whether or not that field exists where it should. func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) error { - var directiveFoundInServer, directiveFoundInUpstream bool - b, err := json.Marshal(conf) if err != nil { return fmt.Errorf("error marshaling nginx config: %w", err) @@ -61,15 +59,11 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err continue } - directiveFoundInServer = validateServerBlockDirectives(expFieldCfg, *directive) - - directiveFoundInUpstream = validateUpstreamDirectives(expFieldCfg, *directive) - - if len(expFieldCfg.Server) > 0 && directiveFoundInServer { + if len(expFieldCfg.Server) > 0 && fieldExistsInServer(expFieldCfg, *directive) { return nil } - if len(expFieldCfg.Upstream) > 0 && directiveFoundInUpstream { + if len(expFieldCfg.Upstream) > 0 && fieldExistsInUpstream(expFieldCfg, *directive) { return nil } } @@ -78,37 +72,35 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err return fmt.Errorf("directive %s not found in: nginx config %s", expFieldCfg.Directive, string(b)) } -func validateServerBlockDirectives( +func fieldExistsInServer( expFieldCfg ExpectedNginxField, directive Directive, ) bool { - var fieldFound bool if directive.Directive == "server" && getServerName(directive.Block) == expFieldCfg.Server { for _, serverDirective := range directive.Block { if expFieldCfg.Location == "" && expFieldCfg.fieldFound(serverDirective) { - fieldFound = true + return true } else if serverDirective.Directive == "location" && fieldExistsInLocation(serverDirective, expFieldCfg) { - fieldFound = true + return true } } } - return fieldFound + return false } -func validateUpstreamDirectives( +func fieldExistsInUpstream( expFieldCfg ExpectedNginxField, directive Directive, ) bool { - var fieldFound bool if directive.Directive == "upstream" && directive.Args[0] == expFieldCfg.Upstream { for _, directive := range directive.Block { if expFieldCfg.fieldFound(directive) { - fieldFound = true + return true } } } - return fieldFound + return false } func getServerName(serverBlock Directives) string { From 666686846a07f9f7c1de93b537b1507cc82939df Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 19 Dec 2024 17:05:11 -0700 Subject: [PATCH 6/8] update tests for uspolicy --- .../invalid-svc-usps.yaml | 4 +- .../invalid-target-usps.yaml | 2 +- .../valid-merge-usps.yaml | 17 +- tests/suite/upstream_settings_test.go | 306 ++++++++++-------- 4 files changed, 189 insertions(+), 140 deletions(-) diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml index 4bfe75788c..cc1ed6c55d 100644 --- a/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/invalid-svc-usps.yaml @@ -1,13 +1,13 @@ apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: does-not-exist + name: usps-target-not-found spec: zoneSize: 512k targetRefs: - group: core kind: Service - name: latte + name: targeted-svc-dne keepAlive: connections: 10 requests: 3 diff --git a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml index 5a223f2f3b..8bbd25366e 100644 --- a/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/invalid-target-usps.yaml @@ -48,7 +48,7 @@ spec: apiVersion: gateway.networking.k8s.io/v1 kind: HTTPRoute metadata: - name: sode + name: soda spec: parentRefs: - name: gateway-not-valid diff --git a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml index cdbab6c269..7c713b4752 100644 --- a/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml +++ b/tests/suite/manifests/upstream-settings-policy/valid-merge-usps.yaml @@ -27,7 +27,20 @@ spec: apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: z-usp-wins + name: z-merge-usp-3 +spec: + targetRefs: + - group: core + kind: Service + name: coffee + keepAlive: + connections: 11 + requests: 15 +--- +apiVersion: gateway.nginx.org/v1alpha1 +kind: UpstreamSettingsPolicy +metadata: + name: z-usp spec: zoneSize: 64k targetRefs: @@ -38,7 +51,7 @@ spec: apiVersion: gateway.nginx.org/v1alpha1 kind: UpstreamSettingsPolicy metadata: - name: a-usp + name: a-usp-wins spec: zoneSize: 128k targetRefs: diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index 0c634a62d6..4d92797e4a 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "time" @@ -28,7 +29,8 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic "upstream-settings-policy/routes.yaml", } - namespace = "uspolicy" + namespace = "uspolicy" + gatewayName = "gateway" ) BeforeAll(func() { @@ -61,52 +63,50 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic }) Specify("they are accepted", func() { - usPolicies := map[string]int{ - "multiple-http-svc-usp": 2, - "grpc-svc-usp": 1, + usPolicies := []string{ + // "multiple-http-svc-usp", + "grpc-svc-usp", } - for name, ancestorCount := range usPolicies { + for _, name := range usPolicies { uspolicyNsName := types.NamespacedName{Name: name, Namespace: namespace} - gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} err := waitForUSPolicyStatus( uspolicyNsName, - gatewayNsName, + gatewayName, metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, - ancestorCount, ) Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("%s was not accepted", name)) } }) - Context("verify working traffic", func() { - It("should return a 200 response for HTTPRoutes", func() { - port := 80 - if portFwdPort != 0 { - port = portFwdPort - } - baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") - baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") - - Eventually( - func() error { - return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") - }). - WithTimeout(timeoutConfig.RequestTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - - Eventually( - func() error { - return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") - }). - WithTimeout(timeoutConfig.RequestTimeout). - WithPolling(500 * time.Millisecond). - Should(Succeed()) - }) - }) + // Context("verify working traffic", func() { + // It("should return a 200 response for HTTPRoutes", func() { + // port := 80 + // if portFwdPort != 0 { + // port = portFwdPort + // } + // baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + // baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") + + // Eventually( + // func() error { + // return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") + // }). + // WithTimeout(timeoutConfig.RequestTimeout). + // WithPolling(500 * time.Millisecond). + // Should(Succeed()) + + // Eventually( + // func() error { + // return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") + // }). + // WithTimeout(timeoutConfig.RequestTimeout). + // WithPolling(500 * time.Millisecond). + // Should(Succeed()) + // }) + // }) Context("nginx directives", func() { var conf *framework.Payload @@ -128,62 +128,78 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) } }, - Entry("HTTP upstreams", []framework.ExpectedNginxField{ - { - Directive: "zone", - Value: "uspolicy_coffee_80 512k", - Upstream: "uspolicy_coffee_80", - File: "http.conf", - }, - { - Directive: "zone", - Value: "uspolicy_tea_80 512k", - Upstream: "uspolicy_tea_80", - File: "http.conf", - }, - { - Directive: "keepalive", - Value: "10", - Upstream: "uspolicy_coffee_80", - File: "http.conf", - }, - { - Directive: "keepalive_requests", - Value: "3", - Upstream: "uspolicy_coffee_80", - File: "http.conf", - }, - { - Directive: "keepalive_requests", - Value: "3", - Upstream: "uspolicy_tea_80", - File: "http.conf", - }, - { - Directive: "keepalive_time", - Value: "10s", - Upstream: "uspolicy_coffee_80", - File: "http.conf", - }, - { - Directive: "keepalive_time", - Value: "10s", - Upstream: "uspolicy_tea_80", - File: "http.conf", - }, - { - Directive: "keepalive_timeout", - Value: "50s", - Upstream: "uspolicy_coffee_80", - File: "http.conf", - }, - { - Directive: "keepalive_timeout", - Value: "50s", - Upstream: "uspolicy_tea_80", - File: "http.conf", - }, - }), + // Entry("HTTP upstreams", []framework.ExpectedNginxField{ + // { + // Directive: "upstream", + // Value: "uspolicy_coffee_80", + // File: "http.conf", + // }, + // { + // Directive: "upstream", + // Value: "uspolicy_tea_80", + // File: "http.conf", + // }, + // { + // Directive: "zone", + // Value: "uspolicy_coffee_80 512k", + // Upstream: "uspolicy_coffee_80", + // File: "http.conf", + // }, + // { + // Directive: "zone", + // Value: "uspolicy_tea_80 512k", + // Upstream: "uspolicy_tea_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive", + // Value: "10", + // Upstream: "uspolicy_coffee_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive", + // Value: "10", + // Upstream: "uspolicy_tea_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive_requests", + // Value: "3", + // Upstream: "uspolicy_coffee_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive_requests", + // Value: "3", + // Upstream: "uspolicy_tea_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive_time", + // Value: "10s", + // Upstream: "uspolicy_coffee_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive_time", + // Value: "10s", + // Upstream: "uspolicy_tea_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive_timeout", + // Value: "50s", + // Upstream: "uspolicy_coffee_80", + // File: "http.conf", + // }, + // { + // Directive: "keepalive_timeout", + // Value: "50s", + // Upstream: "uspolicy_tea_80", + // File: "http.conf", + // }, + // }), Entry("GRPC upstreams", []framework.ExpectedNginxField{ { Directive: "zone", @@ -234,15 +250,15 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic }) DescribeTable("upstreamSettingsPolicy status is set as expected", - func(name string, status metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason, ancestorCount int) { - gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + func(name string, status metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason) { nsname := types.NamespacedName{Name: name, Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, gatewayNsName, status, condReason, ancestorCount)).To(Succeed()) + Expect(waitForUSPolicyStatus(nsname, gatewayName, status, condReason)).To(Succeed()) }, - Entry("uspolicy merge-usp-1", "merge-usp-1", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, 1), - Entry("uspolicy merge-usp-2", "merge-usp-2", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, 1), - Entry("uspolicy a-usp", "a-usp", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted, 1), - Entry("uspolicy z-usp", "z-usp-wins", metav1.ConditionFalse, v1alpha2.PolicyReasonConflicted, 1), + Entry("uspolicy merge-usp-1", "merge-usp-1", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), + Entry("uspolicy merge-usp-2", "merge-usp-2", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), + Entry("uspolicy merge-usp-3", "z-merge-usp-3", metav1.ConditionFalse, v1alpha2.PolicyReasonConflicted), + Entry("uspolicy a-usp-wins", "a-usp-wins", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), + Entry("uspolicy z-usp", "z-usp", metav1.ConditionFalse, v1alpha2.PolicyReasonConflicted), ) Context("verify working traffic", func() { @@ -293,12 +309,6 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic } }, Entry("Coffee upstream", []framework.ExpectedNginxField{ - { - Directive: "zone", - Value: "uspolicy_tea_80 128k", - Upstream: "uspolicy_tea_80", - File: "http.conf", - }, { Directive: "zone", Value: "uspolicy_coffee_80 512k", @@ -330,26 +340,29 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic File: "http.conf", }, }), + Entry("Tea upstream", []framework.ExpectedNginxField{ + { + Directive: "zone", + Value: "uspolicy_tea_80 128k", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + }), ) }) }) When("UpstreamSettingsPolicy targets a Service that does not exists", func() { - Specify("upstreamSettingsPolicy has no condition set", func() { + Specify("upstreamSettingsPolicy sets no condition", func() { files := []string{"upstream-settings-policy/invalid-svc-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "does-not-exist", Namespace: namespace} - gatewayNsName := types.NamespacedName{Name: "gateway", Namespace: namespace} + nsname := types.NamespacedName{Name: "usps-target-not-found", Namespace: namespace} Consistently( func() bool { - return waitForUSPolicyStatus( + return waitForUSPolicyToHaveAncestor( nsname, - gatewayNsName, - metav1.ConditionTrue, - v1alpha2.PolicyReasonAccepted, - 1, ) != nil }).WithTimeout(timeoutConfig.GetTimeout). WithPolling(500 * time.Millisecond). @@ -358,38 +371,61 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) }) - When("UpstreamSettingsPolicy targets a Service that has an invalid Gateway", func() { + + When("UpstreamSettingsPolicy targets a Service that is owned by an invalid Gateway", func() { Specify("upstreamSettingsPolicy has no condition set", func() { - files := []string{"upstream-settings-policy/invalid-target-usps.yaml"} + // delete existing gateway + gatewayFileName := "upstream-settings-policy/gateway.yaml" + Expect(resourceManager.DeleteFromFiles([]string{gatewayFileName}, namespace)).To(Succeed()) + files := []string{"upstream-settings-policy/invalid-target-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} - gatewayNsName := types.NamespacedName{Name: "gateway-not-valid", Namespace: namespace} - Consistently( - func() bool { - return waitForUSPolicyStatus( - nsname, - gatewayNsName, - metav1.ConditionTrue, - v1alpha2.PolicyReasonAccepted, - 1, - ) != nil - }).WithTimeout(timeoutConfig.GetTimeout). - WithPolling(500 * time.Millisecond). - Should(BeTrue()) + gatewayName = "gateway-not-valid" + Expect(waitForUSPolicyStatus( + nsname, + gatewayName, + metav1.ConditionFalse, + v1alpha2.PolicyReasonTargetNotFound, + )).To(Succeed()) Expect(resourceManager.DeleteFromFiles(files, namespace)).To(Succeed()) }) }) }) +func waitForUSPolicyToHaveAncestor(usPolicyNsName types.NamespacedName) error { + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout) + defer cancel() + + GinkgoWriter.Printf("Polling for UpstreamSettings Policy %q to not have a condition", usPolicyNsName) + + return wait.PollUntilContextCancel( + ctx, + timeoutConfig.GetStatusTimeout, + true, /* poll immediately */ + func(ctx context.Context) (bool, error) { + var usPolicy ngfAPI.UpstreamSettingsPolicy + var err error + if err = k8sClient.Get(ctx, usPolicyNsName, &usPolicy); err != nil { + return false, err + } + + if len(usPolicy.Status.Ancestors) == 0 { + return false, nil + } + + return errors.Is(err, context.DeadlineExceeded), nil + }, + ) +} + func waitForUSPolicyStatus( usPolicyNsName types.NamespacedName, - gatewayNsName types.NamespacedName, + gatewayName string, condStatus metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason, - expectedAncestorCount int, ) error { ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout*2) defer cancel() @@ -419,14 +455,14 @@ func waitForUSPolicyStatus( return false, nil } - if len(usPolicy.Status.Ancestors) != expectedAncestorCount { + if len(usPolicy.Status.Ancestors) != 1 { return false, fmt.Errorf("policy has %d ancestors, expected 1", len(usPolicy.Status.Ancestors)) } ancestors := usPolicy.Status.Ancestors for _, ancestor := range ancestors { - if err := ancestorMustEqualGatewayRef(ancestor, gatewayNsName, usPolicy.Namespace); err != nil { + if err := ancestorMustEqualGatewayRef(ancestor, gatewayName, usPolicy.Namespace); err != nil { return false, err } @@ -439,7 +475,7 @@ func waitForUSPolicyStatus( func ancestorMustEqualGatewayRef( ancestor v1alpha2.PolicyAncestorStatus, - gatewayNsName types.NamespacedName, + gatewayName string, namespace string, ) error { if ancestor.ControllerName != ngfControllerName { @@ -464,18 +500,18 @@ func ancestorMustEqualGatewayRef( ancestorRef := ancestor.AncestorRef - if string(ancestorRef.Name) != gatewayNsName.Name { - return fmt.Errorf("expected ancestorRef to have name %s, got %s", gatewayNsName, ancestorRef.Name) + if string(ancestorRef.Name) != gatewayName { + return fmt.Errorf("expected ancestorRef to have name %s, got %s", gatewayName, ancestorRef.Name) } if ancestorRef.Kind == nil { - return fmt.Errorf("expected ancestorRef to have kind %s, got nil", "Gateway") + return errors.New("expected ancestorRef to have kind Gateway, got nil") } if *ancestorRef.Kind != gatewayv1.Kind("Gateway") { return fmt.Errorf( "expected ancestorRef to have kind %s, got %s", - gatewayv1.Kind("Gateway"), + "Gateway", string(*ancestorRef.Kind), ) } From 6e267ea00245085a3b381513434be78fc47ba622 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 20 Dec 2024 09:59:00 -0700 Subject: [PATCH 7/8] enable tests --- tests/framework/crossplane.go | 6 +- tests/suite/upstream_settings_test.go | 198 +++++++++++++------------- 2 files changed, 102 insertions(+), 102 deletions(-) diff --git a/tests/framework/crossplane.go b/tests/framework/crossplane.go index ea2f5bc838..81f47e3567 100644 --- a/tests/framework/crossplane.go +++ b/tests/framework/crossplane.go @@ -52,18 +52,18 @@ func ValidateNginxFieldExists(conf *Payload, expFieldCfg ExpectedNginxField) err } for _, directive := range config.Parsed { - if len(expFieldCfg.Server) == 0 && len(expFieldCfg.Upstream) == 0 { + if expFieldCfg.Server == "" && expFieldCfg.Upstream == "" { if expFieldCfg.fieldFound(directive) { return nil } continue } - if len(expFieldCfg.Server) > 0 && fieldExistsInServer(expFieldCfg, *directive) { + if expFieldCfg.Server != "" && fieldExistsInServer(expFieldCfg, *directive) { return nil } - if len(expFieldCfg.Upstream) > 0 && fieldExistsInUpstream(expFieldCfg, *directive) { + if expFieldCfg.Upstream != "" && fieldExistsInUpstream(expFieldCfg, *directive) { return nil } } diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index 4d92797e4a..8bd9638af6 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -64,7 +64,7 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic Specify("they are accepted", func() { usPolicies := []string{ - // "multiple-http-svc-usp", + "multiple-http-svc-usp", "grpc-svc-usp", } @@ -81,32 +81,32 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic } }) - // Context("verify working traffic", func() { - // It("should return a 200 response for HTTPRoutes", func() { - // port := 80 - // if portFwdPort != 0 { - // port = portFwdPort - // } - // baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") - // baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") - - // Eventually( - // func() error { - // return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") - // }). - // WithTimeout(timeoutConfig.RequestTimeout). - // WithPolling(500 * time.Millisecond). - // Should(Succeed()) - - // Eventually( - // func() error { - // return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") - // }). - // WithTimeout(timeoutConfig.RequestTimeout). - // WithPolling(500 * time.Millisecond). - // Should(Succeed()) - // }) - // }) + Context("verify working traffic", func() { + It("should return a 200 response for HTTPRoutes", func() { + port := 80 + if portFwdPort != 0 { + port = portFwdPort + } + baseCoffeeURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/coffee") + baseTeaURL := fmt.Sprintf("http://cafe.example.com:%d%s", port, "/tea") + + Eventually( + func() error { + return expectRequestToSucceed(baseCoffeeURL, address, "URI: /coffee") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + + Eventually( + func() error { + return expectRequestToSucceed(baseTeaURL, address, "URI: /tea") + }). + WithTimeout(timeoutConfig.RequestTimeout). + WithPolling(500 * time.Millisecond). + Should(Succeed()) + }) + }) Context("nginx directives", func() { var conf *framework.Payload @@ -128,78 +128,78 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic Expect(framework.ValidateNginxFieldExists(conf, expCfg)).To(Succeed()) } }, - // Entry("HTTP upstreams", []framework.ExpectedNginxField{ - // { - // Directive: "upstream", - // Value: "uspolicy_coffee_80", - // File: "http.conf", - // }, - // { - // Directive: "upstream", - // Value: "uspolicy_tea_80", - // File: "http.conf", - // }, - // { - // Directive: "zone", - // Value: "uspolicy_coffee_80 512k", - // Upstream: "uspolicy_coffee_80", - // File: "http.conf", - // }, - // { - // Directive: "zone", - // Value: "uspolicy_tea_80 512k", - // Upstream: "uspolicy_tea_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive", - // Value: "10", - // Upstream: "uspolicy_coffee_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive", - // Value: "10", - // Upstream: "uspolicy_tea_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive_requests", - // Value: "3", - // Upstream: "uspolicy_coffee_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive_requests", - // Value: "3", - // Upstream: "uspolicy_tea_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive_time", - // Value: "10s", - // Upstream: "uspolicy_coffee_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive_time", - // Value: "10s", - // Upstream: "uspolicy_tea_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive_timeout", - // Value: "50s", - // Upstream: "uspolicy_coffee_80", - // File: "http.conf", - // }, - // { - // Directive: "keepalive_timeout", - // Value: "50s", - // Upstream: "uspolicy_tea_80", - // File: "http.conf", - // }, - // }), + Entry("HTTP upstreams", []framework.ExpectedNginxField{ + { + Directive: "upstream", + Value: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "upstream", + Value: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "zone", + Value: "uspolicy_coffee_80 512k", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "zone", + Value: "uspolicy_tea_80 512k", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "10", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive", + Value: "10", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "3", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_requests", + Value: "3", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "10s", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_time", + Value: "10s", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "50s", + Upstream: "uspolicy_coffee_80", + File: "http.conf", + }, + { + Directive: "keepalive_timeout", + Value: "50s", + Upstream: "uspolicy_tea_80", + File: "http.conf", + }, + }), Entry("GRPC upstreams", []framework.ExpectedNginxField{ { Directive: "zone", From 3d09142a94eed14584c9201d618b34da5a758a29 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Fri, 20 Dec 2024 10:36:52 -0700 Subject: [PATCH 8/8] add directives check --- tests/suite/upstream_settings_test.go | 63 ++++++++++++++------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/tests/suite/upstream_settings_test.go b/tests/suite/upstream_settings_test.go index 8bd9638af6..39159e4406 100644 --- a/tests/suite/upstream_settings_test.go +++ b/tests/suite/upstream_settings_test.go @@ -201,6 +201,11 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic }, }), Entry("GRPC upstreams", []framework.ExpectedNginxField{ + { + Directive: "upstream", + Value: "uspolicy_grpc-backend_8080", + File: "http.conf", + }, { Directive: "zone", Value: "uspolicy_grpc-backend_8080 64k", @@ -251,8 +256,8 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic DescribeTable("upstreamSettingsPolicy status is set as expected", func(name string, status metav1.ConditionStatus, condReason v1alpha2.PolicyConditionReason) { - nsname := types.NamespacedName{Name: name, Namespace: namespace} - Expect(waitForUSPolicyStatus(nsname, gatewayName, status, condReason)).To(Succeed()) + uspolicyNsName := types.NamespacedName{Name: name, Namespace: namespace} + Expect(waitForUSPolicyStatus(uspolicyNsName, gatewayName, status, condReason)).To(Succeed()) }, Entry("uspolicy merge-usp-1", "merge-usp-1", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), Entry("uspolicy merge-usp-2", "merge-usp-2", metav1.ConditionTrue, v1alpha2.PolicyReasonAccepted), @@ -309,6 +314,11 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic } }, Entry("Coffee upstream", []framework.ExpectedNginxField{ + { + Directive: "upstream", + Value: "uspolicy_coffee_80", + File: "http.conf", + }, { Directive: "zone", Value: "uspolicy_coffee_80 512k", @@ -347,23 +357,27 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic Upstream: "uspolicy_tea_80", File: "http.conf", }, + { + Directive: "upstream", + Value: "uspolicy_tea_80", + File: "http.conf", + }, }), ) }) }) - When("UpstreamSettingsPolicy targets a Service that does not exists", func() { + When("UpstreamSettingsPolicy targets a Service that does not exist", func() { Specify("upstreamSettingsPolicy sets no condition", func() { files := []string{"upstream-settings-policy/invalid-svc-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "usps-target-not-found", Namespace: namespace} + uspolicyNsName := types.NamespacedName{Name: "usps-target-not-found", Namespace: namespace} + Consistently( func() bool { - return waitForUSPolicyToHaveAncestor( - nsname, - ) != nil + return usPolicyHasNoAncestors(uspolicyNsName) }).WithTimeout(timeoutConfig.GetTimeout). WithPolling(500 * time.Millisecond). Should(BeTrue()) @@ -373,7 +387,7 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic }) When("UpstreamSettingsPolicy targets a Service that is owned by an invalid Gateway", func() { - Specify("upstreamSettingsPolicy has no condition set", func() { + Specify("upstreamSettingsPolicy is not Accepted with the reason TargetNotFound", func() { // delete existing gateway gatewayFileName := "upstream-settings-policy/gateway.yaml" Expect(resourceManager.DeleteFromFiles([]string{gatewayFileName}, namespace)).To(Succeed()) @@ -381,10 +395,10 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic files := []string{"upstream-settings-policy/invalid-target-usps.yaml"} Expect(resourceManager.ApplyFromFiles(files, namespace)).To(Succeed()) - nsname := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} + uspolicyNsName := types.NamespacedName{Name: "soda-svc-usp", Namespace: namespace} gatewayName = "gateway-not-valid" Expect(waitForUSPolicyStatus( - nsname, + uspolicyNsName, gatewayName, metav1.ConditionFalse, v1alpha2.PolicyReasonTargetNotFound, @@ -395,30 +409,19 @@ var _ = Describe("UpstreamSettingsPolicy", Ordered, Label("functional", "uspolic }) }) -func waitForUSPolicyToHaveAncestor(usPolicyNsName types.NamespacedName) error { +func usPolicyHasNoAncestors(usPolicyNsName types.NamespacedName) bool { + GinkgoWriter.Printf("Checking that UpstreamSettingsPolicy %q has no ancestors in status", usPolicyNsName) + ctx, cancel := context.WithTimeout(context.Background(), timeoutConfig.GetStatusTimeout) defer cancel() - GinkgoWriter.Printf("Polling for UpstreamSettings Policy %q to not have a condition", usPolicyNsName) - - return wait.PollUntilContextCancel( - ctx, - timeoutConfig.GetStatusTimeout, - true, /* poll immediately */ - func(ctx context.Context) (bool, error) { - var usPolicy ngfAPI.UpstreamSettingsPolicy - var err error - if err = k8sClient.Get(ctx, usPolicyNsName, &usPolicy); err != nil { - return false, err - } - - if len(usPolicy.Status.Ancestors) == 0 { - return false, nil - } + var usPolicy ngfAPI.UpstreamSettingsPolicy + if err := k8sClient.Get(ctx, usPolicyNsName, &usPolicy); err != nil { + GinkgoWriter.Printf("Failed to get UpstreamSettingsPolicy %q: %s", usPolicyNsName, err.Error()) + return false + } - return errors.Is(err, context.DeadlineExceeded), nil - }, - ) + return len(usPolicy.Status.Ancestors) == 0 } func waitForUSPolicyStatus(