From d5aebb92484c08c417602dbeafd4ca9bda787de9 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 7 Dec 2022 17:03:17 -0500 Subject: [PATCH 01/22] Update helpers so they check conditions when they are up to date Prior when asserting against conditions the helpers didn't check their observedGeneration. This means the assertions could occur with a stale status. --- conformance/utils/kubernetes/helpers.go | 60 +++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index 8f567d9fb4..b132b07acb 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -37,7 +37,7 @@ import ( "sigs.k8s.io/gateway-api/conformance/utils/config" ) -// GatewayRef is a tiny type for specifying an HTTP Route ParentRef without +// GatewayRef is a tiny type for specifying an HTTP Route ParentRef withouthttps://www.cdw.ca/product/apple-airpods-pro-2nd-generation-true-wireless-earphones-with-mic/7171387?pfm=srh // relying on a specific api version. type GatewayRef struct { types.NamespacedName @@ -81,6 +81,12 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo } controllerName = string(gwc.Spec.ControllerName) + + if !ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions) { + t.Log("GatewayClass conditions didn't bump their observedGeneration") + return false, nil + } + // Passing an empty string as the Reason means that any Reason will do. return findConditionInList(t, gwc.Status.Conditions, "Accepted", "True", ""), nil }) @@ -89,6 +95,15 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo return controllerName } +func ConditionsHaveLatestObservedGeneration(obj metav1.Object, conditions []metav1.Condition) bool { + for _, condition := range conditions { + if obj.GetGeneration() != condition.ObservedGeneration { + return false + } + } + return true +} + // NamespacesMustBeAccepted waits until all Pods are marked ready and all Gateways // are marked accepted in the provided namespaces. This will cause the test to // halt if the specified timeout is exceeded. @@ -106,6 +121,12 @@ func NamespacesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig confi t.Errorf("Error listing Gateways: %v", err) } for _, gw := range gwList.Items { + gw := gw + if !ConditionsHaveLatestObservedGeneration(&gw, gw.Status.Conditions) { + t.Log("Gateway conditions didn't bump their observedGeneration") + return false, nil + } + // Passing an empty string as the Reason means that any Reason will do. if !findConditionInList(t, gw.Status.Conditions, string(v1beta1.GatewayConditionAccepted), "True", "") { t.Logf("%s/%s Gateway not ready yet", ns, gw.Name) @@ -194,6 +215,11 @@ func WaitForGatewayAddress(t *testing.T, client client.Client, timeoutConfig con return false, fmt.Errorf("error fetching Gateway: %w", err) } + if !ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) { + t.Log("Gateway conditions didn't bump their observedGeneration") + return false, nil + } + port = strconv.FormatInt(int64(gw.Spec.Listeners[0].Port), 10) // TODO: Support more than IPAddress @@ -220,6 +246,10 @@ func GatewayMustHaveZeroRoutes(t *testing.T, client client.Client, timeoutConfig defer cancel() err := client.Get(ctx, gwName, gw) require.NoError(t, err, "error fetching Gateway") + if !ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) { + t.Log("Gateway conditions didn't bump their observedGeneration") + return false, nil + } // There are two valid ways to represent this: // 1. No listeners in status // 2. One listener in status with 0 attached routes @@ -271,6 +301,14 @@ func HTTPRouteMustHaveNoAcceptedParents(t *testing.T, client client.Client, time // Only expect one parent return false, nil } + + for _, p := range route.Status.Parents { + if !ConditionsHaveLatestObservedGeneration(route, p.Conditions) { + t.Log("HTTPRoute conditions didn't bump their observedGeneration") + return false, nil + } + } + return conditionsMatch(t, []metav1.Condition{{ Type: string(v1beta1.RouteConditionAccepted), Status: "False", @@ -296,8 +334,14 @@ func HTTPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig return false, fmt.Errorf("error fetching HTTPRoute: %w", err) } - actual = route.Status.Parents + for _, p := range route.Status.Parents { + if !ConditionsHaveLatestObservedGeneration(route, p.Conditions) { + t.Log("HTTPRoute conditions didn't bump their observedGeneration") + return false, nil + } + } + actual = route.Status.Parents return parentsForRouteMatch(t, routeName, parents, actual, namespaceRequired), nil }) require.NoErrorf(t, waitErr, "error waiting for HTTPRoute to have parents matching expectations") @@ -362,7 +406,10 @@ func GatewayStatusMustHaveListeners(t *testing.T, client client.Client, timeoutC if err != nil { return false, fmt.Errorf("error fetching Gateway: %w", err) } - + if !ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) { + t.Log("Gateway conditions didn't bump their observedGeneration") + return false, nil + } actual = gw.Status.Listeners return listenersMatch(t, listeners, actual), nil @@ -387,6 +434,13 @@ func HTTPRouteMustHaveCondition(t *testing.T, client client.Client, timeoutConfi parents := route.Status.Parents + for _, p := range parents { + if !ConditionsHaveLatestObservedGeneration(route, p.Conditions) { + t.Log("HTTPRoute conditions didn't bump their observedGeneration") + return false, nil + } + } + var conditionFound bool for _, parent := range parents { if parent.ParentRef.Name == v1beta1.ObjectName(gwNN.Name) && (parent.ParentRef.Namespace == nil || string(*parent.ParentRef.Namespace) == gwNN.Namespace) { From 6873420cf739330048881eb062443380ab2cdcf0 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 7 Dec 2022 17:05:09 -0500 Subject: [PATCH 02/22] When updating Gateway status conditions should increment their observedGeneration --- .../tests/gateway-observed-generation-bump.go | 100 ++++++++++++++++++ .../gateway-observed-generation-bump.yaml | 14 +++ 2 files changed, 114 insertions(+) create mode 100644 conformance/tests/gateway-observed-generation-bump.go create mode 100644 conformance/tests/gateway-observed-generation-bump.yaml diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go new file mode 100644 index 0000000000..cdea55eab8 --- /dev/null +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -0,0 +1,100 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tests + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/gateway-api/apis/v1beta1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + "sigs.k8s.io/gateway-api/conformance/utils/suite" +) + +func init() { + ConformanceTests = append(ConformanceTests, GatewayObservedGenerationBump) +} + +var GatewayObservedGenerationBump = suite.ConformanceTest{ + ShortName: "GatewayObservedGenerationBump", + Description: "A Gateway in the gateway-conformance-infra namespace should update the observedGeneration in all of it's Status.Conditions after an update to the spec", + Manifests: []string{"tests/gateway-observed-generation-bump.yaml"}, + Test: func(t *testing.T, s *suite.ConformanceTestSuite) { + + gwNN := types.NamespacedName{Name: "observed-generation-bump", Namespace: "gateway-conformance-infra"} + + t.Run("observedGeneration should increment", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + namespaces := []string{"gateway-conformance-infra"} + kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) + + existing := &v1beta1.Gateway{} + err := s.Client.Get(ctx, gwNN, existing) + require.NoErrorf(t, err, "error getting Gateway: %v", err) + + // Sanity check + if kubernetes.ConditionsHaveLatestObservedGeneration(existing, existing.Status.Conditions) { + t.Fatal("Not all the condition's observedGeneration were updated") + } + + all := v1beta1.NamespacesFromAll + + // mutate the Gateway Spec + existing.Spec.Listeners = append(existing.Spec.Listeners, v1beta1.Listener{ + Name: "alternate", + Port: 80, + Protocol: v1beta1.HTTPProtocolType, + AllowedRoutes: &v1beta1.AllowedRoutes{ + Namespaces: &v1beta1.RouteNamespaces{From: &all}, + }, + }) + + err = s.Client.Update(ctx, existing) + require.NoErrorf(t, err, "error updating the Gateway: %v", err) + + // Ensure the generation and observedGeneration sync up + kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) + + updated := &v1beta1.Gateway{} + err = s.Client.Get(ctx, gwNN, updated) + require.NoErrorf(t, err, "error getting Gateway: %v", err) + + // Sanity check + if kubernetes.ConditionsHaveLatestObservedGeneration(updated, updated.Status.Conditions) { + t.Fatal("Not all the condition's observedGeneration were updated") + } + + if existing.Generation == updated.Generation { + t.Errorf("expected generation to change because of spec change - remained at %v", updated.Generation) + } + + for _, uc := range updated.Status.Conditions { + for _, ec := range existing.Status.Conditions { + if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { + t.Errorf("expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) + } + } + } + }) + }, +} diff --git a/conformance/tests/gateway-observed-generation-bump.yaml b/conformance/tests/gateway-observed-generation-bump.yaml new file mode 100644 index 0000000000..31fe593d9f --- /dev/null +++ b/conformance/tests/gateway-observed-generation-bump.yaml @@ -0,0 +1,14 @@ +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: Gateway +metadata: + name: gateway-observed-generation-bump + namespace: gateway-conformance-infra +spec: + gatewayClassName: "{GATEWAY_CLASS_NAME}" + listeners: + - name: http + port: 80 + protocol: HTTP + allowedRoutes: + namespaces: + from: All From 0e5ed4c5e93e83868f5f881712235b596971b60e Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 7 Dec 2022 22:09:59 -0500 Subject: [PATCH 03/22] allow injection of controllerNames --- conformance/utils/kubernetes/apply.go | 33 ++++++++++++++++------ conformance/utils/kubernetes/apply_test.go | 27 +++++++++++++++++- conformance/utils/suite/suite.go | 7 +++-- 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/conformance/utils/kubernetes/apply.go b/conformance/utils/kubernetes/apply.go index 9ff3f4b5ad..e8d26fd86d 100644 --- a/conformance/utils/kubernetes/apply.go +++ b/conformance/utils/kubernetes/apply.go @@ -49,25 +49,31 @@ type Applier struct { // four ValidUniqueListenerPorts. // If empty or nil, ports are not modified. ValidUniqueListenerPorts []v1beta1.PortNumber + + // GatewayClass + GatewayClass string + + // ControllerName + ControllerName string } // prepareGateway adjusts both listener ports and the gatewayClassName. It // returns an index pointing to the next valid listener port. -func prepareGateway(t *testing.T, uObj *unstructured.Unstructured, gatewayClassName string, validListenerPorts []v1beta1.PortNumber, portIndex int) int { - err := unstructured.SetNestedField(uObj.Object, gatewayClassName, "spec", "gatewayClassName") +func (a Applier) prepareGateway(t *testing.T, uObj *unstructured.Unstructured, portIndex int) int { + err := unstructured.SetNestedField(uObj.Object, a.GatewayClass, "spec", "gatewayClassName") require.NoErrorf(t, err, "error setting `spec.gatewayClassName` on %s Gateway resource", uObj.GetName()) - if len(validListenerPorts) > 0 { + if len(a.ValidUniqueListenerPorts) > 0 { listeners, _, err := unstructured.NestedSlice(uObj.Object, "spec", "listeners") require.NoErrorf(t, err, "error getting `spec.listeners` on %s Gateway resource", uObj.GetName()) for i, uListener := range listeners { - require.Less(t, portIndex, len(validListenerPorts), "not enough unassigned valid ports for `spec.listeners[%d]` on %s Gateway resource", i, uObj.GetName()) + require.Less(t, portIndex, len(a.ValidUniqueListenerPorts), "not enough unassigned valid ports for `spec.listeners[%d]` on %s Gateway resource", i, uObj.GetName()) listener, ok := uListener.(map[string]interface{}) require.Truef(t, ok, "unexpected type at `spec.listeners[%d]` on %s Gateway resource", i, uObj.GetName()) - nextPort := validListenerPorts[portIndex] + nextPort := a.ValidUniqueListenerPorts[portIndex] err = unstructured.SetNestedField(listener, int64(nextPort), "port") require.NoErrorf(t, err, "error setting `spec.listeners[%d].port` on %s Gateway resource", i, uObj.GetName()) @@ -82,6 +88,12 @@ func prepareGateway(t *testing.T, uObj *unstructured.Unstructured, gatewayClassN return portIndex } +// prepareGatewayClass adjust the spec.controllerName on the resource +func (a Applier) prepareGatewayClass(t *testing.T, uObj *unstructured.Unstructured) { + err := unstructured.SetNestedField(uObj.Object, a.ControllerName, "spec", "controllerName") + require.NoErrorf(t, err, "error setting `spec.controllerName` on %s GatewayClass resource", uObj.GetName()) +} + // prepareNamespace adjusts the Namespace labels. func prepareNamespace(t *testing.T, uObj *unstructured.Unstructured, namespaceLabels map[string]string) { labels, _, err := unstructured.NestedStringMap(uObj.Object, "metadata", "labels") @@ -104,7 +116,7 @@ func prepareNamespace(t *testing.T, uObj *unstructured.Unstructured, namespaceLa // prepareResources uses the options from an Applier to tweak resources given by // a set of manifests. -func (a Applier) prepareResources(t *testing.T, decoder *yaml.YAMLOrJSONDecoder, gcName string) ([]unstructured.Unstructured, error) { +func (a Applier) prepareResources(t *testing.T, decoder *yaml.YAMLOrJSONDecoder) ([]unstructured.Unstructured, error) { var resources []unstructured.Unstructured // portIndex is incremented for each listener we see. For a manifest file @@ -123,8 +135,11 @@ func (a Applier) prepareResources(t *testing.T, decoder *yaml.YAMLOrJSONDecoder, continue } + if uObj.GetKind() == "GatewayClass" { + a.prepareGatewayClass(t, &uObj) + } if uObj.GetKind() == "Gateway" { - portIndex = prepareGateway(t, &uObj, gcName, a.ValidUniqueListenerPorts, portIndex) + portIndex = a.prepareGateway(t, &uObj, portIndex) } if uObj.GetKind() == "Namespace" && uObj.GetObjectKind().GroupVersionKind().Group == "" { @@ -168,13 +183,13 @@ func (a Applier) MustApplyObjectsWithCleanup(t *testing.T, c client.Client, time // MustApplyWithCleanup creates or updates Kubernetes resources defined with the // provided YAML file and registers a cleanup function for resources it created. // Note that this does not remove resources that already existed in the cluster. -func (a Applier) MustApplyWithCleanup(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, location string, gcName string, cleanup bool) { +func (a Applier) MustApplyWithCleanup(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, location string, cleanup bool) { data, err := getContentsFromPathOrURL(location, timeoutConfig) require.NoError(t, err) decoder := yaml.NewYAMLOrJSONDecoder(data, 4096) - resources, err := a.prepareResources(t, decoder, gcName) + resources, err := a.prepareResources(t, decoder) if err != nil { t.Logf("manifest: %s", data.String()) require.NoErrorf(t, err, "error parsing manifest") diff --git a/conformance/utils/kubernetes/apply_test.go b/conformance/utils/kubernetes/apply_test.go index 3ee201fbe1..70f2593623 100644 --- a/conformance/utils/kubernetes/apply_test.go +++ b/conformance/utils/kubernetes/apply_test.go @@ -259,13 +259,38 @@ spec: }, }, }}, + }, { + name: "setting the controllerName for a GatewayClass", + applier: Applier{}, + given: ` +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: GatewayClass +metadata: + name: test +spec: + controllerName: {GATEWAY_CONTROLLER_NAME} +`, + expected: []unstructured.Unstructured{{ + Object: map[string]interface{}{ + "apiVersion": "gateway.networking.k8s.io/v1beta1", + "kind": "GatewayClass", + "metadata": map[string]interface{}{ + "name": "test", + }, + "spec": map[string]interface{}{ + "controllerName": "test-controller", + }, + }, + }}, }} for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { decoder := yaml.NewYAMLOrJSONDecoder(strings.NewReader(tc.given), 4096) - resources, err := tc.applier.prepareResources(t, decoder, "test-class") + tc.applier.GatewayClass = "test-class" + tc.applier.ControllerName = "test-controller" + resources, err := tc.applier.prepareResources(t, decoder) require.NoError(t, err, "unexpected error preparing resources") require.EqualValues(t, tc.expected, resources) diff --git a/conformance/utils/suite/suite.go b/conformance/utils/suite/suite.go index 42afe128d2..cadfaa8c86 100644 --- a/conformance/utils/suite/suite.go +++ b/conformance/utils/suite/suite.go @@ -145,8 +145,11 @@ func (suite *ConformanceTestSuite) Setup(t *testing.T) { t.Logf("Test Setup: Ensuring GatewayClass has been accepted") suite.ControllerName = kubernetes.GWCMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.GatewayClassName) + suite.Applier.GatewayClass = suite.GatewayClassName + suite.Applier.ControllerName = suite.ControllerName + t.Logf("Test Setup: Applying base manifests") - suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, suite.BaseManifests, suite.GatewayClassName, suite.Cleanup) + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, suite.BaseManifests, suite.Cleanup) t.Logf("Test Setup: Applying programmatic resources") secret := kubernetes.MustCreateSelfSignedCertSecret(t, "gateway-conformance-web-backend", "certificate", []string{"*"}) @@ -200,7 +203,7 @@ func (test *ConformanceTest) Run(t *testing.T, suite *ConformanceTestSuite) { for _, manifestLocation := range test.Manifests { t.Logf("Applying %s", manifestLocation) - suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, manifestLocation, suite.GatewayClassName, true) + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, manifestLocation, true) } test.Test(t, suite) From 5658f1d99d7f4eaabd4a23adf1b785f5bec6d529 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 7 Dec 2022 22:10:49 -0500 Subject: [PATCH 04/22] When updating GatewayClass the status conditions should increment their observedGeneration --- .../gatewayclass-observed-generation-bump.go | 90 +++++++++++++++++++ ...gatewayclass-observed-generation-bump.yaml | 7 ++ 2 files changed, 97 insertions(+) create mode 100644 conformance/tests/gatewayclass-observed-generation-bump.go create mode 100644 conformance/tests/gatewayclass-observed-generation-bump.yaml diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go new file mode 100644 index 0000000000..2cfe5ab7da --- /dev/null +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -0,0 +1,90 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tests + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/gateway-api/apis/v1beta1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + "sigs.k8s.io/gateway-api/conformance/utils/suite" +) + +func init() { + ConformanceTests = append(ConformanceTests, GatewayClassObservedGenerationBump) +} + +var GatewayClassObservedGenerationBump = suite.ConformanceTest{ + ShortName: "GatewayClassObservedGenerationBump", + Description: "A GatewayClass should update the observedGeneration in all of it's Status.Conditions after an update to the spec", + Manifests: []string{"tests/gatewayclass-observed-generation-bump.yaml"}, + Test: func(t *testing.T, s *suite.ConformanceTestSuite) { + + gwc := types.NamespacedName{Name: "gatewayclass-observed-generation-bump"} + + t.Run("observedGeneration should increment", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + kubernetes.GWCMustBeAccepted(t, s.Client, s.TimeoutConfig, gwc.Name) + + existing := &v1beta1.GatewayClass{} + err := s.Client.Get(ctx, gwc, existing) + require.NoErrorf(t, err, "error getting GatewayClass: %v", err) + + // Sanity check + if kubernetes.ConditionsHaveLatestObservedGeneration(existing, existing.Status.Conditions) { + t.Fatal("Not all the condition's observedGeneration were updated") + } + + desc := "new" + existing.Spec.Description = &desc + + err = s.Client.Update(ctx, existing) + require.NoErrorf(t, err, "error updating the GatewayClass: %v", err) + + // Ensure the generation and observedGeneration sync up + kubernetes.GWCMustBeAccepted(t, s.Client, s.TimeoutConfig, gwc.Name) + + updated := &v1beta1.GatewayClass{} + err = s.Client.Get(ctx, gwc, updated) + require.NoErrorf(t, err, "error getting GatewayClass: %v", err) + + // Sanity check + if kubernetes.ConditionsHaveLatestObservedGeneration(updated, updated.Status.Conditions) { + t.Fatal("Not all the condition's observedGeneration were updated") + } + + if existing.Generation == updated.Generation { + t.Errorf("expected generation to change because of spec change - remained at %v", updated.Generation) + } + + for _, uc := range updated.Status.Conditions { + for _, ec := range existing.Status.Conditions { + if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { + t.Errorf("expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) + } + } + } + }) + }, +} diff --git a/conformance/tests/gatewayclass-observed-generation-bump.yaml b/conformance/tests/gatewayclass-observed-generation-bump.yaml new file mode 100644 index 0000000000..0ad3d1fd36 --- /dev/null +++ b/conformance/tests/gatewayclass-observed-generation-bump.yaml @@ -0,0 +1,7 @@ +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: GatewayClass +metadata: + name: gatewayclass-observed-generation-bump +spec: + controllerName: "{GATEWAY_CONTROLLER_NAME}" + description: "old" From 3fbe62e03df83bc110145ba0fac492369dc81fbe Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 11:54:54 -0500 Subject: [PATCH 05/22] fix casing of t.Error/Fatal messages --- conformance/tests/gateway-observed-generation-bump.go | 4 ++-- conformance/tests/gatewayclass-observed-generation-bump.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index cdea55eab8..c060e87d67 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -85,13 +85,13 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ } if existing.Generation == updated.Generation { - t.Errorf("expected generation to change because of spec change - remained at %v", updated.Generation) + t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } for _, uc := range updated.Status.Conditions { for _, ec := range existing.Status.Conditions { if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { - t.Errorf("expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) + t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) } } } diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index 2cfe5ab7da..e08062f8b4 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -75,13 +75,13 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ } if existing.Generation == updated.Generation { - t.Errorf("expected generation to change because of spec change - remained at %v", updated.Generation) + t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } for _, uc := range updated.Status.Conditions { for _, ec := range existing.Status.Conditions { if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { - t.Errorf("expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) + t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) } } } From b58b45f4f92a368859ae635e3f073e6bb792ee2a Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 11:55:21 -0500 Subject: [PATCH 06/22] removed pasted link --- conformance/utils/kubernetes/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index b132b07acb..bd95d07733 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -37,7 +37,7 @@ import ( "sigs.k8s.io/gateway-api/conformance/utils/config" ) -// GatewayRef is a tiny type for specifying an HTTP Route ParentRef withouthttps://www.cdw.ca/product/apple-airpods-pro-2nd-generation-true-wireless-earphones-with-mic/7171387?pfm=srh +// GatewayRef is a tiny type for specifying an HTTP Route ParentRef without // relying on a specific api version. type GatewayRef struct { types.NamespacedName From 009e835a63a937d3dc695a779785708dc63ddd0b Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 11:56:32 -0500 Subject: [PATCH 07/22] refactor some helpers to assert observedGeneration is bumped --- .../tests/gateway-observed-generation-bump.go | 4 +- .../gatewayclass-observed-generation-bump.go | 4 +- conformance/utils/kubernetes/helpers.go | 60 ++++++++++++------- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index c060e87d67..d5def9dfa6 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -53,7 +53,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting Gateway: %v", err) // Sanity check - if kubernetes.ConditionsHaveLatestObservedGeneration(existing, existing.Status.Conditions) { + if kubernetes.GatewayConditionsHaveLatestObservedGeneration(existing) { t.Fatal("Not all the condition's observedGeneration were updated") } @@ -80,7 +80,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting Gateway: %v", err) // Sanity check - if kubernetes.ConditionsHaveLatestObservedGeneration(updated, updated.Status.Conditions) { + if kubernetes.GatewayConditionsHaveLatestObservedGeneration(updated) { t.Fatal("Not all the condition's observedGeneration were updated") } diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index e08062f8b4..debc9c2f7d 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -52,7 +52,7 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting GatewayClass: %v", err) // Sanity check - if kubernetes.ConditionsHaveLatestObservedGeneration(existing, existing.Status.Conditions) { + if kubernetes.GatewayClassConditionsHaveLatestObservedGeneration(existing) { t.Fatal("Not all the condition's observedGeneration were updated") } @@ -70,7 +70,7 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting GatewayClass: %v", err) // Sanity check - if kubernetes.ConditionsHaveLatestObservedGeneration(updated, updated.Status.Conditions) { + if kubernetes.GatewayClassConditionsHaveLatestObservedGeneration(updated) { t.Fatal("Not all the condition's observedGeneration were updated") } diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index bd95d07733..64f297f6a1 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -82,7 +82,7 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo controllerName = string(gwc.Spec.ControllerName) - if !ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions) { + if !GatewayClassConditionsHaveLatestObservedGeneration(gwc) { t.Log("GatewayClass conditions didn't bump their observedGeneration") return false, nil } @@ -95,6 +95,29 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo return controllerName } +func HTTPRouteConditionsHaveLatestObservedGeneration(r *v1beta1.HTTPRoute) bool { + for _, p := range r.Status.Parents { + if !ConditionsHaveLatestObservedGeneration(r, p.Conditions) { + return false + } + } + return true +} + +func GatewayClassConditionsHaveLatestObservedGeneration(gc *v1beta1.GatewayClass) bool { + if !ConditionsHaveLatestObservedGeneration(gc, gc.Status.Conditions) { + return false + } + return true +} + +func GatewayConditionsHaveLatestObservedGeneration(g *v1beta1.Gateway) bool { + if !ConditionsHaveLatestObservedGeneration(g, g.Status.Conditions) { + return false + } + return true +} + func ConditionsHaveLatestObservedGeneration(obj metav1.Object, conditions []metav1.Condition) bool { for _, condition := range conditions { if obj.GetGeneration() != condition.ObservedGeneration { @@ -122,7 +145,7 @@ func NamespacesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig confi } for _, gw := range gwList.Items { gw := gw - if !ConditionsHaveLatestObservedGeneration(&gw, gw.Status.Conditions) { + if !GatewayConditionsHaveLatestObservedGeneration(&gw) { t.Log("Gateway conditions didn't bump their observedGeneration") return false, nil } @@ -215,7 +238,7 @@ func WaitForGatewayAddress(t *testing.T, client client.Client, timeoutConfig con return false, fmt.Errorf("error fetching Gateway: %w", err) } - if !ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) { + if !GatewayConditionsHaveLatestObservedGeneration(gw) { t.Log("Gateway conditions didn't bump their observedGeneration") return false, nil } @@ -246,7 +269,7 @@ func GatewayMustHaveZeroRoutes(t *testing.T, client client.Client, timeoutConfig defer cancel() err := client.Get(ctx, gwName, gw) require.NoError(t, err, "error fetching Gateway") - if !ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) { + if !GatewayConditionsHaveLatestObservedGeneration(gw) { t.Log("Gateway conditions didn't bump their observedGeneration") return false, nil } @@ -302,11 +325,9 @@ func HTTPRouteMustHaveNoAcceptedParents(t *testing.T, client client.Client, time return false, nil } - for _, p := range route.Status.Parents { - if !ConditionsHaveLatestObservedGeneration(route, p.Conditions) { - t.Log("HTTPRoute conditions didn't bump their observedGeneration") - return false, nil - } + if !HTTPRouteConditionsHaveLatestObservedGeneration(route) { + t.Log("HTTPRoute conditions didn't bump their observedGeneration") + return false, nil } return conditionsMatch(t, []metav1.Condition{{ @@ -334,11 +355,9 @@ func HTTPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig return false, fmt.Errorf("error fetching HTTPRoute: %w", err) } - for _, p := range route.Status.Parents { - if !ConditionsHaveLatestObservedGeneration(route, p.Conditions) { - t.Log("HTTPRoute conditions didn't bump their observedGeneration") - return false, nil - } + if !HTTPRouteConditionsHaveLatestObservedGeneration(route) { + t.Log("HTTPRoute conditions didn't bump their observedGeneration") + return false, nil } actual = route.Status.Parents @@ -406,7 +425,7 @@ func GatewayStatusMustHaveListeners(t *testing.T, client client.Client, timeoutC if err != nil { return false, fmt.Errorf("error fetching Gateway: %w", err) } - if !ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) { + if !GatewayConditionsHaveLatestObservedGeneration(gw) { t.Log("Gateway conditions didn't bump their observedGeneration") return false, nil } @@ -432,15 +451,12 @@ func HTTPRouteMustHaveCondition(t *testing.T, client client.Client, timeoutConfi return false, fmt.Errorf("error fetching HTTPRoute: %w", err) } - parents := route.Status.Parents - - for _, p := range parents { - if !ConditionsHaveLatestObservedGeneration(route, p.Conditions) { - t.Log("HTTPRoute conditions didn't bump their observedGeneration") - return false, nil - } + if !HTTPRouteConditionsHaveLatestObservedGeneration(route) { + t.Log("HTTPRoute conditions didn't bump their observedGeneration") + return false, nil } + parents := route.Status.Parents var conditionFound bool for _, parent := range parents { if parent.ParentRef.Name == v1beta1.ObjectName(gwNN.Name) && (parent.ParentRef.Namespace == nil || string(*parent.ParentRef.Namespace) == gwNN.Namespace) { From de2d9c5a525b004bc19052e54db1be14c46ba3f0 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 14:27:16 -0500 Subject: [PATCH 08/22] add HTTPRoute observed generation tests --- .../httproute-observed-generation-bump.go | 114 ++++++++++++++++++ .../httproute-observed-generation-bump.yaml | 12 ++ 2 files changed, 126 insertions(+) create mode 100644 conformance/tests/httproute-observed-generation-bump.go create mode 100644 conformance/tests/httproute-observed-generation-bump.yaml diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go new file mode 100644 index 0000000000..0eac9de052 --- /dev/null +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -0,0 +1,114 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tests + +import ( + "context" + "reflect" + "testing" + "time" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + "sigs.k8s.io/gateway-api/apis/v1beta1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + "sigs.k8s.io/gateway-api/conformance/utils/suite" +) + +func init() { + ConformanceTests = append(ConformanceTests, HTTPRouteObservedGenerationBump) +} + +var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ + ShortName: "HTTPRouteObservedGenerationBump", + Description: "A HTTPRoute in the gateway-conformance-infra namespace should update the observedGeneration in all of it's Status.Conditions after an update to the spec", + Manifests: []string{"tests/httproute-observed-generation-bump.yaml"}, + Test: func(t *testing.T, s *suite.ConformanceTestSuite) { + + routeNN := types.NamespacedName{Name: "observed-generation-bump", Namespace: "gateway-conformance-infra"} + gwNN := types.NamespacedName{Name: "same-namespace", Namespace: "gateway-conformance-infra"} + + acceptedCondition := metav1.Condition{ + Type: string(v1beta1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "", // any reason + } + + t.Run("observedGeneration should increment", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + namespaces := []string{"gateway-conformance-infra"} + kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) + + existing := &v1beta1.HTTPRoute{} + err := s.Client.Get(ctx, routeNN, existing) + require.NoErrorf(t, err, "error getting HTTPRoute: %v", err) + + // Sanity check + if kubernetes.HTTPRouteConditionsHaveLatestObservedGeneration(existing) { + t.Fatal("Not all the condition's observedGeneration were updated") + } + + existing.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-new" + err = s.Client.Update(ctx, existing) + require.NoErrorf(t, err, "error updating the HTTPRoute: %v", err) + + kubernetes.HTTPRouteMustHaveCondition(t, s.Client, s.TimeoutConfig, routeNN, gwNN, acceptedCondition) + + updated := &v1beta1.HTTPRoute{} + err = s.Client.Get(ctx, routeNN, updated) + require.NoErrorf(t, err, "error getting Gateway: %v", err) + + // Sanity check + if kubernetes.HTTPRouteConditionsHaveLatestObservedGeneration(updated) { + t.Fatal("Not all the condition's observedGeneration were updated") + } + + if existing.Generation == updated.Generation { + t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) + } + + for _, up := range updated.Status.Parents { + existing := parentStatusForRef(existing.Status.Parents, up.ParentRef) + if existing == nil { + t.Logf("Observed unexpected new parent ref %#v", up.ParentRef) + continue + } + for _, uc := range up.Conditions { + for _, ec := range existing.Conditions { + if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { + t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) + } + } + } + } + }) + }, +} + +func parentStatusForRef(statuses []v1beta1.RouteParentStatus, ref v1beta1.ParentReference) *v1beta1.RouteParentStatus { + for _, status := range statuses { + if reflect.DeepEqual(status.ParentRef, ref) { + return &status + } + } + return nil + +} diff --git a/conformance/tests/httproute-observed-generation-bump.yaml b/conformance/tests/httproute-observed-generation-bump.yaml new file mode 100644 index 0000000000..8fbca59af4 --- /dev/null +++ b/conformance/tests/httproute-observed-generation-bump.yaml @@ -0,0 +1,12 @@ +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: HTTPRoute +metadata: + name: observed-generation-bump + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: same-namespace + rules: + - backendRefs: + - name: infra-backend-old + port: 8080 From db554dac01da789f1905f26193e8ace755eaa904 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 14:51:28 -0500 Subject: [PATCH 09/22] use a unique port when adding a new listener --- conformance/tests/gateway-observed-generation-bump.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index d5def9dfa6..158f5ebf08 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -62,7 +62,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ // mutate the Gateway Spec existing.Spec.Listeners = append(existing.Spec.Listeners, v1beta1.Listener{ Name: "alternate", - Port: 80, + Port: 8080, Protocol: v1beta1.HTTPProtocolType, AllowedRoutes: &v1beta1.AllowedRoutes{ Namespaces: &v1beta1.RouteNamespaces{From: &all}, From 658275485e4977717735e3394dde3efe9cc47e7e Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 14:53:20 -0500 Subject: [PATCH 10/22] increase test timeout to a minute --- conformance/tests/gateway-observed-generation-bump.go | 2 +- conformance/tests/gatewayclass-observed-generation-bump.go | 2 +- conformance/tests/httproute-observed-generation-bump.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index 158f5ebf08..84a1a9e31b 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -42,7 +42,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ gwNN := types.NamespacedName{Name: "observed-generation-bump", Namespace: "gateway-conformance-infra"} t.Run("observedGeneration should increment", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() namespaces := []string{"gateway-conformance-infra"} diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index debc9c2f7d..5fd00347ae 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -42,7 +42,7 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ gwc := types.NamespacedName{Name: "gatewayclass-observed-generation-bump"} t.Run("observedGeneration should increment", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() kubernetes.GWCMustBeAccepted(t, s.Client, s.TimeoutConfig, gwc.Name) diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 0eac9de052..46cebc8534 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -51,7 +51,7 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ } t.Run("observedGeneration should increment", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() namespaces := []string{"gateway-conformance-infra"} From c8f4c5d406a03ec216cbb4bea0ae665baf67d690 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Fri, 9 Dec 2022 14:56:50 -0500 Subject: [PATCH 11/22] use real backends --- conformance/tests/httproute-observed-generation-bump.go | 2 +- conformance/tests/httproute-observed-generation-bump.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 46cebc8534..9ba1cf1cd8 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -66,7 +66,7 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ t.Fatal("Not all the condition's observedGeneration were updated") } - existing.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-new" + existing.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-v2" err = s.Client.Update(ctx, existing) require.NoErrorf(t, err, "error updating the HTTPRoute: %v", err) diff --git a/conformance/tests/httproute-observed-generation-bump.yaml b/conformance/tests/httproute-observed-generation-bump.yaml index 8fbca59af4..8de5c76455 100644 --- a/conformance/tests/httproute-observed-generation-bump.yaml +++ b/conformance/tests/httproute-observed-generation-bump.yaml @@ -8,5 +8,5 @@ spec: - name: same-namespace rules: - backendRefs: - - name: infra-backend-old + - name: infra-backend-v1 port: 8080 From 546ad16f4711f79edc941761caf5ac0e92b14997 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Thu, 15 Dec 2022 20:44:31 -0500 Subject: [PATCH 12/22] fail test is an unexpected parent ref appears --- conformance/tests/httproute-observed-generation-bump.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 9ba1cf1cd8..25b3ffb9cd 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -88,8 +88,7 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ for _, up := range updated.Status.Parents { existing := parentStatusForRef(existing.Status.Parents, up.ParentRef) if existing == nil { - t.Logf("Observed unexpected new parent ref %#v", up.ParentRef) - continue + t.Fatalf("Observed unexpected new parent ref %#v", up.ParentRef) } for _, uc := range up.Conditions { for _, ec := range existing.Conditions { From 0b9d064dd14bed6dd024d2b405632cfc9429f220 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Thu, 15 Dec 2022 22:01:11 -0500 Subject: [PATCH 13/22] Provide better error messages for failed assertions --- .../tests/gateway-observed-generation-bump.go | 8 +- .../gatewayclass-observed-generation-bump.go | 8 +- .../httproute-observed-generation-bump.go | 8 +- conformance/utils/kubernetes/helpers.go | 125 ++++++++++++------ 4 files changed, 92 insertions(+), 57 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index 84a1a9e31b..d52348c0aa 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -53,9 +53,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting Gateway: %v", err) // Sanity check - if kubernetes.GatewayConditionsHaveLatestObservedGeneration(existing) { - t.Fatal("Not all the condition's observedGeneration were updated") - } + kubernetes.GatewayMustHaveLatestConditions(t, existing) all := v1beta1.NamespacesFromAll @@ -80,9 +78,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting Gateway: %v", err) // Sanity check - if kubernetes.GatewayConditionsHaveLatestObservedGeneration(updated) { - t.Fatal("Not all the condition's observedGeneration were updated") - } + kubernetes.GatewayMustHaveLatestConditions(t, updated) if existing.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index 5fd00347ae..af05ad7a4b 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -52,9 +52,7 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting GatewayClass: %v", err) // Sanity check - if kubernetes.GatewayClassConditionsHaveLatestObservedGeneration(existing) { - t.Fatal("Not all the condition's observedGeneration were updated") - } + kubernetes.GatewayClassMustHaveLatestConditions(t, existing) desc := "new" existing.Spec.Description = &desc @@ -70,9 +68,7 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting GatewayClass: %v", err) // Sanity check - if kubernetes.GatewayClassConditionsHaveLatestObservedGeneration(updated) { - t.Fatal("Not all the condition's observedGeneration were updated") - } + kubernetes.GatewayClassMustHaveLatestConditions(t, updated) if existing.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 25b3ffb9cd..6508c54100 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -62,9 +62,7 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting HTTPRoute: %v", err) // Sanity check - if kubernetes.HTTPRouteConditionsHaveLatestObservedGeneration(existing) { - t.Fatal("Not all the condition's observedGeneration were updated") - } + kubernetes.HTTPRouteMustHaveLatestConditions(t, existing) existing.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-v2" err = s.Client.Update(ctx, existing) @@ -77,9 +75,7 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ require.NoErrorf(t, err, "error getting Gateway: %v", err) // Sanity check - if kubernetes.HTTPRouteConditionsHaveLatestObservedGeneration(updated) { - t.Fatal("Not all the condition's observedGeneration were updated") - } + kubernetes.HTTPRouteMustHaveLatestConditions(t, updated) if existing.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index 64f297f6a1..9e445fadc9 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -18,6 +18,7 @@ package kubernetes import ( "context" + "errors" "fmt" "net" "reflect" @@ -82,8 +83,8 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo controllerName = string(gwc.Spec.ControllerName) - if !GatewayClassConditionsHaveLatestObservedGeneration(gwc) { - t.Log("GatewayClass conditions didn't bump their observedGeneration") + if err := ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions); err != nil { + t.Log("GatewayClass", err) return false, nil } @@ -95,36 +96,73 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo return controllerName } -func HTTPRouteConditionsHaveLatestObservedGeneration(r *v1beta1.HTTPRoute) bool { - for _, p := range r.Status.Parents { - if !ConditionsHaveLatestObservedGeneration(r, p.Conditions) { - return false - } +// GatewayMustHaveLatestConditions will fail the test if there are +// conditions that were not updated +func GatewayMustHaveLatestConditions(t *testing.T, gw *v1beta1.Gateway) { + t.Helper() + + err := ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) + if err != nil { + t.Fatalf("Gateway %v", err) } - return true } -func GatewayClassConditionsHaveLatestObservedGeneration(gc *v1beta1.GatewayClass) bool { - if !ConditionsHaveLatestObservedGeneration(gc, gc.Status.Conditions) { - return false +// GatewayClassMustHaveLatestConditions will fail the test if there are +// conditions that were not updated +func GatewayClassMustHaveLatestConditions(t *testing.T, gwc *v1beta1.GatewayClass) { + t.Helper() + + err := ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions) + if err != nil { + t.Fatalf("GatewayClass %v", err) } - return true } -func GatewayConditionsHaveLatestObservedGeneration(g *v1beta1.Gateway) bool { - if !ConditionsHaveLatestObservedGeneration(g, g.Status.Conditions) { - return false +// HTTPRouteMustHaveLatestConditions will fail the test if there are +// conditions that were not updated +func HTTPRouteMustHaveLatestConditions(t *testing.T, r *v1beta1.HTTPRoute) { + t.Helper() + + for _, parent := range r.Status.Parents { + err := ConditionsHaveLatestObservedGeneration(r, parent.Conditions) + if err != nil { + t.Fatalf("HTTPRoute(controller=%v, parentRef=%#v) %v", parent.ControllerName, parent, err) + } } - return true } -func ConditionsHaveLatestObservedGeneration(obj metav1.Object, conditions []metav1.Condition) bool { +func ConditionsHaveLatestObservedGeneration(obj metav1.Object, conditions []metav1.Condition) error { + staleConditions := FilterStaleConditions(obj, conditions) + + if len(staleConditions) == 0 { + return nil + } + + var b strings.Builder + fmt.Fprint(&b, "expected observedGeneration to be updated for all conditions") + fmt.Fprintf(&b, ", only %d/%d were updated.", len(staleConditions), len(conditions)) + fmt.Fprintf(&b, " stale conditions are: ") + + for i, c := range staleConditions { + fmt.Fprintf(&b, c.Type) + if i != len(staleConditions)-1 { + fmt.Fprintf(&b, ", ") + } + } + + return errors.New(b.String()) +} + +// FilterStaleConditions returns the list of status condition whos observedGeneration does not +// match the objects metadata.Generation +func FilterStaleConditions(obj metav1.Object, conditions []metav1.Condition) []metav1.Condition { + stale := make([]metav1.Condition, 0, len(conditions)) for _, condition := range conditions { if obj.GetGeneration() != condition.ObservedGeneration { - return false + stale = append(stale, condition) } } - return true + return stale } // NamespacesMustBeAccepted waits until all Pods are marked ready and all Gateways @@ -145,8 +183,9 @@ func NamespacesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig confi } for _, gw := range gwList.Items { gw := gw - if !GatewayConditionsHaveLatestObservedGeneration(&gw) { - t.Log("Gateway conditions didn't bump their observedGeneration") + + if err := ConditionsHaveLatestObservedGeneration(&gw, gw.Status.Conditions); err != nil { + t.Log(err) return false, nil } @@ -238,8 +277,8 @@ func WaitForGatewayAddress(t *testing.T, client client.Client, timeoutConfig con return false, fmt.Errorf("error fetching Gateway: %w", err) } - if !GatewayConditionsHaveLatestObservedGeneration(gw) { - t.Log("Gateway conditions didn't bump their observedGeneration") + if err := ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions); err != nil { + t.Log("Gateway", err) return false, nil } @@ -269,10 +308,12 @@ func GatewayMustHaveZeroRoutes(t *testing.T, client client.Client, timeoutConfig defer cancel() err := client.Get(ctx, gwName, gw) require.NoError(t, err, "error fetching Gateway") - if !GatewayConditionsHaveLatestObservedGeneration(gw) { - t.Log("Gateway conditions didn't bump their observedGeneration") + + if err := ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions); err != nil { + t.Log("Gateway ", err) return false, nil } + // There are two valid ways to represent this: // 1. No listeners in status // 2. One listener in status with 0 attached routes @@ -325,9 +366,11 @@ func HTTPRouteMustHaveNoAcceptedParents(t *testing.T, client client.Client, time return false, nil } - if !HTTPRouteConditionsHaveLatestObservedGeneration(route) { - t.Log("HTTPRoute conditions didn't bump their observedGeneration") - return false, nil + for _, parent := range actual { + if err := ConditionsHaveLatestObservedGeneration(route, parent.Conditions); err != nil { + t.Logf("HTTPRoute(controller=%v,ref=%#v) %v", parent.ControllerName, parent, err) + return false, nil + } } return conditionsMatch(t, []metav1.Condition{{ @@ -355,9 +398,11 @@ func HTTPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig return false, fmt.Errorf("error fetching HTTPRoute: %w", err) } - if !HTTPRouteConditionsHaveLatestObservedGeneration(route) { - t.Log("HTTPRoute conditions didn't bump their observedGeneration") - return false, nil + for _, parent := range actual { + if err := ConditionsHaveLatestObservedGeneration(route, parent.Conditions); err != nil { + t.Logf("HTTPRoute(controller=%v,ref=%#v) %v", parent.ControllerName, parent, err) + return false, nil + } } actual = route.Status.Parents @@ -425,12 +470,13 @@ func GatewayStatusMustHaveListeners(t *testing.T, client client.Client, timeoutC if err != nil { return false, fmt.Errorf("error fetching Gateway: %w", err) } - if !GatewayConditionsHaveLatestObservedGeneration(gw) { - t.Log("Gateway conditions didn't bump their observedGeneration") + + if err := ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions); err != nil { + t.Log("Gateway", err) return false, nil } - actual = gw.Status.Listeners + actual = gw.Status.Listeners return listenersMatch(t, listeners, actual), nil }) require.NoErrorf(t, waitErr, "error waiting for Gateway status to have listeners matching expectations") @@ -451,14 +497,15 @@ func HTTPRouteMustHaveCondition(t *testing.T, client client.Client, timeoutConfi return false, fmt.Errorf("error fetching HTTPRoute: %w", err) } - if !HTTPRouteConditionsHaveLatestObservedGeneration(route) { - t.Log("HTTPRoute conditions didn't bump their observedGeneration") - return false, nil - } - parents := route.Status.Parents var conditionFound bool for _, parent := range parents { + + if err := ConditionsHaveLatestObservedGeneration(route, parent.Conditions); err != nil { + t.Logf("HTTPRoute(controller=%v,ref=%#v) %v", parent.ControllerName, parent, err) + return false, nil + } + if parent.ParentRef.Name == v1beta1.ObjectName(gwNN.Name) && (parent.ParentRef.Namespace == nil || string(*parent.ParentRef.Namespace) == gwNN.Namespace) { if findConditionInList(t, parent.Conditions, condition.Type, string(condition.Status), condition.Reason) { conditionFound = true From ec9c9ca1459fbee24ea0de012baf1400e01934a5 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Thu, 15 Dec 2022 22:02:16 -0500 Subject: [PATCH 14/22] fix fixture name --- conformance/tests/gateway-observed-generation-bump.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index d52348c0aa..abfc581ea4 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -39,7 +39,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ Manifests: []string{"tests/gateway-observed-generation-bump.yaml"}, Test: func(t *testing.T, s *suite.ConformanceTestSuite) { - gwNN := types.NamespacedName{Name: "observed-generation-bump", Namespace: "gateway-conformance-infra"} + gwNN := types.NamespacedName{Name: "gateway-observed-generation-bump", Namespace: "gateway-conformance-infra"} t.Run("observedGeneration should increment", func(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) From 9441114aed6632919e7e954f1a4927100e687cbb Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 11:51:01 -0500 Subject: [PATCH 15/22] fix stale count logging --- conformance/utils/kubernetes/helpers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index 9e445fadc9..a09e9fef32 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -140,7 +140,7 @@ func ConditionsHaveLatestObservedGeneration(obj metav1.Object, conditions []meta var b strings.Builder fmt.Fprint(&b, "expected observedGeneration to be updated for all conditions") - fmt.Fprintf(&b, ", only %d/%d were updated.", len(staleConditions), len(conditions)) + fmt.Fprintf(&b, ", only %d/%d were updated.", len(conditions)-len(staleConditions), len(conditions)) fmt.Fprintf(&b, " stale conditions are: ") for i, c := range staleConditions { From f87815ba1b058207a4f6360770eb143ebf976fb4 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 11:58:57 -0500 Subject: [PATCH 16/22] create a deep copy prior to mutation and updating --- .../tests/gateway-observed-generation-bump.go | 16 +++++++------- .../gatewayclass-observed-generation-bump.go | 15 ++++++------- .../httproute-observed-generation-bump.go | 21 ++++++++++--------- 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index abfc581ea4..621388d555 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -48,17 +48,19 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ namespaces := []string{"gateway-conformance-infra"} kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) - existing := &v1beta1.Gateway{} - err := s.Client.Get(ctx, gwNN, existing) + original := &v1beta1.Gateway{} + err := s.Client.Get(ctx, gwNN, original) require.NoErrorf(t, err, "error getting Gateway: %v", err) // Sanity check - kubernetes.GatewayMustHaveLatestConditions(t, existing) + kubernetes.GatewayMustHaveLatestConditions(t, original) all := v1beta1.NamespacesFromAll + mutate := original.DeepCopy() + // mutate the Gateway Spec - existing.Spec.Listeners = append(existing.Spec.Listeners, v1beta1.Listener{ + mutate.Spec.Listeners = append(mutate.Spec.Listeners, v1beta1.Listener{ Name: "alternate", Port: 8080, Protocol: v1beta1.HTTPProtocolType, @@ -67,7 +69,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ }, }) - err = s.Client.Update(ctx, existing) + err = s.Client.Update(ctx, mutate) require.NoErrorf(t, err, "error updating the Gateway: %v", err) // Ensure the generation and observedGeneration sync up @@ -80,12 +82,12 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ // Sanity check kubernetes.GatewayMustHaveLatestConditions(t, updated) - if existing.Generation == updated.Generation { + if original.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } for _, uc := range updated.Status.Conditions { - for _, ec := range existing.Status.Conditions { + for _, ec := range original.Status.Conditions { if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) } diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index af05ad7a4b..21d9ff446f 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -47,17 +47,18 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ kubernetes.GWCMustBeAccepted(t, s.Client, s.TimeoutConfig, gwc.Name) - existing := &v1beta1.GatewayClass{} - err := s.Client.Get(ctx, gwc, existing) + original := &v1beta1.GatewayClass{} + err := s.Client.Get(ctx, gwc, original) require.NoErrorf(t, err, "error getting GatewayClass: %v", err) // Sanity check - kubernetes.GatewayClassMustHaveLatestConditions(t, existing) + kubernetes.GatewayClassMustHaveLatestConditions(t, original) + mutate := original.DeepCopy() desc := "new" - existing.Spec.Description = &desc + mutate.Spec.Description = &desc - err = s.Client.Update(ctx, existing) + err = s.Client.Update(ctx, mutate) require.NoErrorf(t, err, "error updating the GatewayClass: %v", err) // Ensure the generation and observedGeneration sync up @@ -70,12 +71,12 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ // Sanity check kubernetes.GatewayClassMustHaveLatestConditions(t, updated) - if existing.Generation == updated.Generation { + if original.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } for _, uc := range updated.Status.Conditions { - for _, ec := range existing.Status.Conditions { + for _, ec := range original.Status.Conditions { if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) } diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 6508c54100..3950c568df 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -57,15 +57,16 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ namespaces := []string{"gateway-conformance-infra"} kubernetes.NamespacesMustBeAccepted(t, s.Client, s.TimeoutConfig, namespaces) - existing := &v1beta1.HTTPRoute{} - err := s.Client.Get(ctx, routeNN, existing) + original := &v1beta1.HTTPRoute{} + err := s.Client.Get(ctx, routeNN, original) require.NoErrorf(t, err, "error getting HTTPRoute: %v", err) // Sanity check - kubernetes.HTTPRouteMustHaveLatestConditions(t, existing) + kubernetes.HTTPRouteMustHaveLatestConditions(t, original) - existing.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-v2" - err = s.Client.Update(ctx, existing) + mutate := original.DeepCopy() + mutate.Spec.Rules[0].BackendRefs[0].Name = "infra-backend-v2" + err = s.Client.Update(ctx, mutate) require.NoErrorf(t, err, "error updating the HTTPRoute: %v", err) kubernetes.HTTPRouteMustHaveCondition(t, s.Client, s.TimeoutConfig, routeNN, gwNN, acceptedCondition) @@ -77,18 +78,18 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ // Sanity check kubernetes.HTTPRouteMustHaveLatestConditions(t, updated) - if existing.Generation == updated.Generation { + if original.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } for _, up := range updated.Status.Parents { - existing := parentStatusForRef(existing.Status.Parents, up.ParentRef) - if existing == nil { + originalRef := parentStatusForRef(original.Status.Parents, up.ParentRef) + if originalRef == nil { t.Fatalf("Observed unexpected new parent ref %#v", up.ParentRef) } for _, uc := range up.Conditions { - for _, ec := range existing.Conditions { - if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { + for _, oc := range originalRef.Conditions { + if oc.Type == uc.Type && oc.ObservedGeneration == uc.ObservedGeneration { t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) } } From 3e97ba4fc90d03761be643c05996eea8cb62df75 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 12:02:54 -0500 Subject: [PATCH 17/22] drop redundant assertions *MustHaveLatestConditions will ensure the observedGeneration has changed --- .../tests/gateway-observed-generation-bump.go | 8 -------- .../tests/gatewayclass-observed-generation-bump.go | 9 --------- .../tests/httproute-observed-generation-bump.go | 14 -------------- 3 files changed, 31 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index 621388d555..db6f2d4418 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -85,14 +85,6 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ if original.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } - - for _, uc := range updated.Status.Conditions { - for _, ec := range original.Status.Conditions { - if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { - t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) - } - } - } }) }, } diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index 21d9ff446f..d125cb8b0e 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -38,7 +38,6 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ Description: "A GatewayClass should update the observedGeneration in all of it's Status.Conditions after an update to the spec", Manifests: []string{"tests/gatewayclass-observed-generation-bump.yaml"}, Test: func(t *testing.T, s *suite.ConformanceTestSuite) { - gwc := types.NamespacedName{Name: "gatewayclass-observed-generation-bump"} t.Run("observedGeneration should increment", func(t *testing.T) { @@ -74,14 +73,6 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ if original.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } - - for _, uc := range updated.Status.Conditions { - for _, ec := range original.Status.Conditions { - if ec.Type == uc.Type && ec.ObservedGeneration == uc.ObservedGeneration { - t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) - } - } - } }) }, } diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 3950c568df..9a4852cb55 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -81,20 +81,6 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ if original.Generation == updated.Generation { t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) } - - for _, up := range updated.Status.Parents { - originalRef := parentStatusForRef(original.Status.Parents, up.ParentRef) - if originalRef == nil { - t.Fatalf("Observed unexpected new parent ref %#v", up.ParentRef) - } - for _, uc := range up.Conditions { - for _, oc := range originalRef.Conditions { - if oc.Type == uc.Type && oc.ObservedGeneration == uc.ObservedGeneration { - t.Errorf("Expected status condition %q observedGeneration to change - remained at %v", uc.Type, uc.ObservedGeneration) - } - } - } - } }) }, } From 46fb65d5aeded3956bda7b9af98aec5ffd3a6dce Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 12:09:29 -0500 Subject: [PATCH 18/22] address linting --- .../tests/httproute-observed-generation-bump.go | 11 ----------- conformance/utils/kubernetes/helpers.go | 11 ++++------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 9a4852cb55..0726265d26 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -18,7 +18,6 @@ package tests import ( "context" - "reflect" "testing" "time" @@ -84,13 +83,3 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ }) }, } - -func parentStatusForRef(statuses []v1beta1.RouteParentStatus, ref v1beta1.ParentReference) *v1beta1.RouteParentStatus { - for _, status := range statuses { - if reflect.DeepEqual(status.ParentRef, ref) { - return &status - } - } - return nil - -} diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index a09e9fef32..19a79da0b9 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -101,8 +101,7 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.Timeo func GatewayMustHaveLatestConditions(t *testing.T, gw *v1beta1.Gateway) { t.Helper() - err := ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions) - if err != nil { + if err := ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions); err != nil { t.Fatalf("Gateway %v", err) } } @@ -112,8 +111,7 @@ func GatewayMustHaveLatestConditions(t *testing.T, gw *v1beta1.Gateway) { func GatewayClassMustHaveLatestConditions(t *testing.T, gwc *v1beta1.GatewayClass) { t.Helper() - err := ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions) - if err != nil { + if err := ConditionsHaveLatestObservedGeneration(gwc, gwc.Status.Conditions); err != nil { t.Fatalf("GatewayClass %v", err) } } @@ -124,8 +122,7 @@ func HTTPRouteMustHaveLatestConditions(t *testing.T, r *v1beta1.HTTPRoute) { t.Helper() for _, parent := range r.Status.Parents { - err := ConditionsHaveLatestObservedGeneration(r, parent.Conditions) - if err != nil { + if err := ConditionsHaveLatestObservedGeneration(r, parent.Conditions); err != nil { t.Fatalf("HTTPRoute(controller=%v, parentRef=%#v) %v", parent.ControllerName, parent, err) } } @@ -184,7 +181,7 @@ func NamespacesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig confi for _, gw := range gwList.Items { gw := gw - if err := ConditionsHaveLatestObservedGeneration(&gw, gw.Status.Conditions); err != nil { + if err = ConditionsHaveLatestObservedGeneration(&gw, gw.Status.Conditions); err != nil { t.Log(err) return false, nil } From 41f980222b0e9d1162ccbb82779dd9563df1a3d7 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 13:29:03 -0500 Subject: [PATCH 19/22] use assertion helper --- conformance/tests/gateway-observed-generation-bump.go | 4 +--- conformance/tests/gatewayclass-observed-generation-bump.go | 4 +--- conformance/tests/httproute-observed-generation-bump.go | 4 +--- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/conformance/tests/gateway-observed-generation-bump.go b/conformance/tests/gateway-observed-generation-bump.go index db6f2d4418..d7b5d87bda 100644 --- a/conformance/tests/gateway-observed-generation-bump.go +++ b/conformance/tests/gateway-observed-generation-bump.go @@ -82,9 +82,7 @@ var GatewayObservedGenerationBump = suite.ConformanceTest{ // Sanity check kubernetes.GatewayMustHaveLatestConditions(t, updated) - if original.Generation == updated.Generation { - t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) - } + require.NotEqual(t, original.Generation, updated.Generation, "generation should change after an update") }) }, } diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index d125cb8b0e..7f29c9d0d8 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -70,9 +70,7 @@ var GatewayClassObservedGenerationBump = suite.ConformanceTest{ // Sanity check kubernetes.GatewayClassMustHaveLatestConditions(t, updated) - if original.Generation == updated.Generation { - t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) - } + require.NotEqual(t, original.Generation, updated.Generation, "generation should change after an update") }) }, } diff --git a/conformance/tests/httproute-observed-generation-bump.go b/conformance/tests/httproute-observed-generation-bump.go index 0726265d26..590dcc6fed 100644 --- a/conformance/tests/httproute-observed-generation-bump.go +++ b/conformance/tests/httproute-observed-generation-bump.go @@ -77,9 +77,7 @@ var HTTPRouteObservedGenerationBump = suite.ConformanceTest{ // Sanity check kubernetes.HTTPRouteMustHaveLatestConditions(t, updated) - if original.Generation == updated.Generation { - t.Errorf("Expected generation to change because of spec change - remained at %v", updated.Generation) - } + require.NotEqual(t, original.Generation, updated.Generation, "generation should change after an update") }) }, } From e2ca2e9f63d427ccc5da855d571dbd3e7666ecb8 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 13:29:25 -0500 Subject: [PATCH 20/22] log only the parent ref name --- conformance/utils/kubernetes/helpers.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index 19a79da0b9..df0a1e571f 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -497,9 +497,9 @@ func HTTPRouteMustHaveCondition(t *testing.T, client client.Client, timeoutConfi parents := route.Status.Parents var conditionFound bool for _, parent := range parents { - if err := ConditionsHaveLatestObservedGeneration(route, parent.Conditions); err != nil { - t.Logf("HTTPRoute(controller=%v,ref=%#v) %v", parent.ControllerName, parent, err) + + t.Logf("HTTPRoute(parentRef=%v) %v", parentRefToString(parent.ParentRef), err) return false, nil } @@ -516,6 +516,13 @@ func HTTPRouteMustHaveCondition(t *testing.T, client client.Client, timeoutConfi require.NoErrorf(t, waitErr, "error waiting for HTTPRoute status to have a Condition matching expectations") } +func parentRefToString(p v1beta1.ParentReference) string { + if p.Namespace != nil && *p.Namespace != "" { + return fmt.Sprintf("%v/%v", p.Namespace, p.Name) + } + return string(p.Name) +} + // TODO(mikemorris): this and parentsMatch could possibly be rewritten as a generic function? func listenersMatch(t *testing.T, expected, actual []v1beta1.ListenerStatus) bool { t.Helper() From c73eadb59b94744a3e4b95d102f6734bfd253175 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 13:37:15 -0500 Subject: [PATCH 21/22] update godoc --- conformance/utils/kubernetes/apply.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conformance/utils/kubernetes/apply.go b/conformance/utils/kubernetes/apply.go index e8d26fd86d..da1f5bfe6f 100644 --- a/conformance/utils/kubernetes/apply.go +++ b/conformance/utils/kubernetes/apply.go @@ -50,10 +50,10 @@ type Applier struct { // If empty or nil, ports are not modified. ValidUniqueListenerPorts []v1beta1.PortNumber - // GatewayClass + // GatewayClass will be used as the spec.gatewayClassName when applying Gateway resources GatewayClass string - // ControllerName + // ControllerName will be used as the spec.controllerName when applying GatewayClass resources ControllerName string } From 759598d382fcfee7f06f229a6c8130e7f2356caa Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 16 Dec 2022 13:49:08 -0500 Subject: [PATCH 22/22] put the gateway class observed gen bump behind a support flag --- conformance/tests/gatewayclass-observed-generation-bump.go | 1 + conformance/utils/suite/suite.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/conformance/tests/gatewayclass-observed-generation-bump.go b/conformance/tests/gatewayclass-observed-generation-bump.go index 7f29c9d0d8..0bb08799ff 100644 --- a/conformance/tests/gatewayclass-observed-generation-bump.go +++ b/conformance/tests/gatewayclass-observed-generation-bump.go @@ -35,6 +35,7 @@ func init() { var GatewayClassObservedGenerationBump = suite.ConformanceTest{ ShortName: "GatewayClassObservedGenerationBump", + Features: []suite.SupportedFeature{suite.SupportGatewayClassObservedGenerationBump}, Description: "A GatewayClass should update the observedGeneration in all of it's Status.Conditions after an update to the spec", Manifests: []string{"tests/gatewayclass-observed-generation-bump.yaml"}, Test: func(t *testing.T, s *suite.ConformanceTestSuite) { diff --git a/conformance/utils/suite/suite.go b/conformance/utils/suite/suite.go index cadfaa8c86..72f317a129 100644 --- a/conformance/utils/suite/suite.go +++ b/conformance/utils/suite/suite.go @@ -51,6 +51,9 @@ const ( // This option indicates support for Destination Port matching (extended conformance). SupportRouteDestinationPortMatching SupportedFeature = "RouteDestinationPortMatching" + + // This option indicates GatewayClass will update the observedGeneration in it's conditions when reconciling + SupportGatewayClassObservedGenerationBump SupportedFeature = "GatewayClassObservedGenerationBump" ) // StandardCoreFeatures are the features that are required to be conformant with