diff --git a/conformance/tests/backendtlspolicy-preserve-foreign-status.go b/conformance/tests/backendtlspolicy-preserve-foreign-status.go new file mode 100644 index 0000000000..828358dae6 --- /dev/null +++ b/conformance/tests/backendtlspolicy-preserve-foreign-status.go @@ -0,0 +1,129 @@ +/* +Copyright 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 ( + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + confsuite "sigs.k8s.io/gateway-api/conformance/utils/suite" + "sigs.k8s.io/gateway-api/pkg/features" +) + +func init() { + ConformanceTests = append(ConformanceTests, BackendTLSPolicyPreserveForeignStatus) +} + +var BackendTLSPolicyPreserveForeignStatus = confsuite.ConformanceTest{ + ShortName: "BackendTLSPolicyPreserveForeignStatus", + Description: "An implementation must not remove or modify BackendTLSPolicy status.ancestors " + + "entries whose controllerName belongs to another implementation.", + Features: []features.FeatureName{ + features.SupportGateway, + features.SupportHTTPRoute, + features.SupportBackendTLSPolicy, + }, + Manifests: []string{"tests/backendtlspolicy-preserve-foreign-status.yaml"}, + Provisional: true, + Test: func(t *testing.T, suite *confsuite.ConformanceTestSuite) { + ns := confsuite.InfrastructureNamespace + routeNN := types.NamespacedName{Name: "backendtlspolicy-preserve-foreign-status", Namespace: ns} + gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} + policyNN := types.NamespacedName{Name: "backendtlspolicy-preserve-foreign-status", Namespace: ns} + + kubernetes.NamespacesMustBeReady(t, suite.Client, suite.TimeoutConfig, []string{ns}) + kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN) + kubernetes.BackendTLSPolicyMustHaveAcceptedConditionTrue(t, suite.Client, suite.TimeoutConfig, policyNN, gwNN) + + var seeded gatewayv1.PolicyAncestorStatus + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + policy := &gatewayv1.BackendTLSPolicy{} + if err := suite.Client.Get(t.Context(), policyNN, policy); err != nil { + return err + } + policy.Status.Ancestors = append(policy.Status.Ancestors, gatewayv1.PolicyAncestorStatus{ + AncestorRef: gatewayv1.ParentReference{ + Group: new(gatewayv1.Group(gatewayv1.GroupVersion.Group)), + Kind: new(gatewayv1.Kind("Gateway")), + Name: "unmanaged-gateway", + Namespace: new(gatewayv1.Namespace(ns)), + }, + ControllerName: kubernetes.StaleControllerName, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.PolicyReasonAccepted), + ObservedGeneration: policy.Generation, + LastTransitionTime: metav1.Now(), + }}, + }) + if err := suite.Client.Status().Update(t.Context(), policy); err != nil { + return err + } + // Read the entry back off the update response rather than reusing + // the value written above: the API server truncates + // lastTransitionTime to second precision. + stored := foreignAncestorStatus(policy.Status.Ancestors) + if stored == nil { + return errForeignStatusNotStored + } + seeded = *stored + return nil + }) + require.NoError(t, err, "error seeding a foreign controller's status entry on BackendTLSPolicy %s", policyNN) + + // Bump the generation so the implementation has to run a full + // read-modify-write cycle on a status that already holds the seeded entry. + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + policy := &gatewayv1.BackendTLSPolicy{} + if getErr := suite.Client.Get(t.Context(), policyNN, policy); getErr != nil { + return getErr + } + policy.Spec.Validation.Hostname = "second.example.com" + return suite.Client.Update(t.Context(), policy) + }) + require.NoError(t, err, "error updating BackendTLSPolicy %s", policyNN) + + // The implementation's own entry catching up to the bumped generation is + // what proves it wrote status while the seeded entry was there. + kubernetes.BackendTLSPolicyMustHaveAcceptedConditionTrue(t, suite.Client, suite.TimeoutConfig, policyNN, gwNN) + + policy := &gatewayv1.BackendTLSPolicy{} + require.NoError(t, suite.Client.Get(t.Context(), policyNN, policy), "error fetching BackendTLSPolicy %s", policyNN) + kubernetes.BackendTLSPolicyMustHaveLatestConditions(t, policy) + + stored := foreignAncestorStatus(policy.Status.Ancestors) + require.NotNilf(t, stored, "BackendTLSPolicy %s: status entry owned by %s was removed", policyNN, kubernetes.StaleControllerName) + require.Equalf(t, seeded, *stored, "BackendTLSPolicy %s: status entry owned by %s was modified", policyNN, kubernetes.StaleControllerName) + }, +} + +func foreignAncestorStatus(ancestors []gatewayv1.PolicyAncestorStatus) *gatewayv1.PolicyAncestorStatus { + for i := range ancestors { + if ancestors[i].ControllerName == kubernetes.StaleControllerName { + return &ancestors[i] + } + } + + return nil +} diff --git a/conformance/tests/backendtlspolicy-preserve-foreign-status.yaml b/conformance/tests/backendtlspolicy-preserve-foreign-status.yaml new file mode 100644 index 0000000000..6c4cae7543 --- /dev/null +++ b/conformance/tests/backendtlspolicy-preserve-foreign-status.yaml @@ -0,0 +1,55 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: backendtlspolicy-preserve-foreign-status + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: same-namespace + namespace: gateway-conformance-infra + hostnames: + - preserve.example.com + rules: + - backendRefs: + - group: "" + kind: Service + name: backendtlspolicy-preserve-foreign-status-test + port: 443 + matches: + - path: + type: Exact + value: /backendtlspolicy-preserve-foreign-status +--- +apiVersion: v1 +kind: Service +metadata: + name: backendtlspolicy-preserve-foreign-status-test + namespace: gateway-conformance-infra +spec: + selector: + app: tls-backend + ports: + - name: "btls" + protocol: TCP + port: 443 + targetPort: 8443 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: BackendTLSPolicy +metadata: + name: backendtlspolicy-preserve-foreign-status + namespace: gateway-conformance-infra +spec: + targetRefs: + - group: "" + kind: Service + name: "backendtlspolicy-preserve-foreign-status-test" + sectionName: "btls" + validation: + caCertificateRefs: + - group: "" + kind: ConfigMap + # This ConfigMap is generated dynamically by the test suite. + # It contains the CA certificate used to sign the tls-backend serving certificate. + name: "tls-checks-ca-certificate" + hostname: "abc.example.com" diff --git a/conformance/tests/gateway-preserve-foreign-conditions.go b/conformance/tests/gateway-preserve-foreign-conditions.go new file mode 100644 index 0000000000..fc75875d5f --- /dev/null +++ b/conformance/tests/gateway-preserve-foreign-conditions.go @@ -0,0 +1,179 @@ +/* +Copyright 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" + "errors" + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" + + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + confsuite "sigs.k8s.io/gateway-api/conformance/utils/suite" + "sigs.k8s.io/gateway-api/pkg/features" +) + +var errForeignConditionNotStored = errors.New("the seeded condition is missing from the update response") + +func init() { + ConformanceTests = append(ConformanceTests, GatewayPreserveForeignConditions) +} + +var GatewayPreserveForeignConditions = confsuite.ConformanceTest{ + ShortName: "GatewayPreserveForeignConditions", + Description: "An implementation must not remove or modify Gateway and Listener status conditions " + + "whose type it is not responsible for.", + Features: []features.FeatureName{ + features.SupportGateway, + }, + Manifests: []string{"tests/gateway-preserve-foreign-conditions.yaml"}, + Provisional: true, + Test: func(t *testing.T, suite *confsuite.ConformanceTestSuite) { + ns := confsuite.InfrastructureNamespace + gwNN := types.NamespacedName{Name: "gateway-preserve-foreign-conditions", Namespace: ns} + listenerName := gatewayv1.SectionName("http") + + kubernetes.NamespacesMustBeReady(t, suite.Client, suite.TimeoutConfig, []string{ns}) + kubernetes.GatewayMustHaveLatestConditions(t, suite.Client, suite.TimeoutConfig, gwNN) + + // Listener status is where the second seeded condition goes, so its + // entry has to exist first. + waitErr := wait.PollUntilContextTimeout(context.Background(), suite.TimeoutConfig.DefaultPollInterval, suite.TimeoutConfig.GatewayStatusMustHaveListeners, true, func(ctx context.Context) (bool, error) { + gw := &gatewayv1.Gateway{} + if err := suite.Client.Get(ctx, gwNN, gw); err != nil { + return false, err + } + return listenerStatus(gw, listenerName) != nil, nil + }) + require.NoErrorf(t, waitErr, "error waiting for Gateway %s to report status for listener %s", gwNN, listenerName) + + foreignCondition := func(observedGeneration int64) metav1.Condition { + return metav1.Condition{ + Type: kubernetes.StaleConditionType, + Status: metav1.ConditionTrue, + Reason: "Seeded", + ObservedGeneration: observedGeneration, + LastTransitionTime: metav1.Now(), + } + } + + var seededGw, seededListener metav1.Condition + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + gw := &gatewayv1.Gateway{} + if err := suite.Client.Get(t.Context(), gwNN, gw); err != nil { + return err + } + ls := listenerStatus(gw, listenerName) + if ls == nil { + return errForeignConditionNotStored + } + gw.Status.Conditions = append(gw.Status.Conditions, foreignCondition(gw.Generation)) + ls.Conditions = append(ls.Conditions, foreignCondition(gw.Generation)) + if err := suite.Client.Status().Update(t.Context(), gw); err != nil { + return err + } + // Read the conditions back off the update response rather than + // reusing the values written above: the API server truncates + // lastTransitionTime to second precision. + storedGw := foreignConditionIn(gw.Status.Conditions) + ls = listenerStatus(gw, listenerName) + if storedGw == nil || ls == nil { + return errForeignConditionNotStored + } + storedListener := foreignConditionIn(ls.Conditions) + if storedListener == nil { + return errForeignConditionNotStored + } + seededGw = *storedGw + seededListener = *storedListener + return nil + }) + require.NoError(t, err, "error seeding foreign conditions on Gateway %s", gwNN) + + // Bump the generation so the implementation has to run a full + // read-modify-write cycle on a status that already holds the seeded + // conditions. + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + gw := &gatewayv1.Gateway{} + if getErr := suite.Client.Get(t.Context(), gwNN, gw); getErr != nil { + return getErr + } + gw.Spec.Listeners[0].Hostname = new(gatewayv1.Hostname("second.example.com")) + return suite.Client.Update(t.Context(), gw) + }) + require.NoError(t, err, "error updating Gateway %s", gwNN) + + // The implementation's own conditions catching up to the bumped + // generation, on the Gateway and on every listener, is what proves it + // wrote status while the seeded conditions were there. + waitErr = wait.PollUntilContextTimeout(context.Background(), suite.TimeoutConfig.DefaultPollInterval, suite.TimeoutConfig.LatestObservedGenerationSet, true, func(ctx context.Context) (bool, error) { + gw := &gatewayv1.Gateway{} + if err := suite.Client.Get(ctx, gwNN, gw); err != nil { + return false, err + } + if err := kubernetes.ConditionsHaveLatestObservedGeneration(gw, gw.Status.Conditions); err != nil { + return false, nil + } + for _, ls := range gw.Status.Listeners { + if err := kubernetes.ConditionsHaveLatestObservedGeneration(gw, ls.Conditions); err != nil { + return false, nil + } + } + return true, nil + }) + require.NoErrorf(t, waitErr, "error waiting for Gateway %s conditions to catch up with the bumped generation", gwNN) + + gw := &gatewayv1.Gateway{} + require.NoError(t, suite.Client.Get(t.Context(), gwNN, gw), "error fetching Gateway %s", gwNN) + + stored := foreignConditionIn(gw.Status.Conditions) + require.NotNilf(t, stored, "Gateway %s: the %s condition was removed", gwNN, kubernetes.StaleConditionType) + require.Equalf(t, seededGw, *stored, "Gateway %s: the %s condition was modified", gwNN, kubernetes.StaleConditionType) + + ls := listenerStatus(gw, listenerName) + require.NotNilf(t, ls, "Gateway %s: status for listener %s is missing", gwNN, listenerName) + stored = foreignConditionIn(ls.Conditions) + require.NotNilf(t, stored, "Gateway %s listener %s: the %s condition was removed", gwNN, listenerName, kubernetes.StaleConditionType) + require.Equalf(t, seededListener, *stored, "Gateway %s listener %s: the %s condition was modified", gwNN, listenerName, kubernetes.StaleConditionType) + }, +} + +func listenerStatus(gw *gatewayv1.Gateway, name gatewayv1.SectionName) *gatewayv1.ListenerStatus { + for i := range gw.Status.Listeners { + if gw.Status.Listeners[i].Name == name { + return &gw.Status.Listeners[i] + } + } + + return nil +} + +func foreignConditionIn(conditions []metav1.Condition) *metav1.Condition { + for i := range conditions { + if conditions[i].Type == kubernetes.StaleConditionType { + return &conditions[i] + } + } + + return nil +} diff --git a/conformance/tests/gateway-preserve-foreign-conditions.yaml b/conformance/tests/gateway-preserve-foreign-conditions.yaml new file mode 100644 index 0000000000..8ac847f592 --- /dev/null +++ b/conformance/tests/gateway-preserve-foreign-conditions.yaml @@ -0,0 +1,14 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway-preserve-foreign-conditions + namespace: gateway-conformance-infra +spec: + gatewayClassName: "{GATEWAY_CLASS_NAME}" + listeners: + - name: http + port: 80 + protocol: HTTP + allowedRoutes: + namespaces: + from: Same diff --git a/conformance/tests/httproute-preserve-foreign-status.go b/conformance/tests/httproute-preserve-foreign-status.go new file mode 100644 index 0000000000..ca479b9409 --- /dev/null +++ b/conformance/tests/httproute-preserve-foreign-status.go @@ -0,0 +1,162 @@ +/* +Copyright 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 ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + + gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + confsuite "sigs.k8s.io/gateway-api/conformance/utils/suite" + "sigs.k8s.io/gateway-api/pkg/features" +) + +var errForeignStatusNotStored = errors.New("the seeded status entry is missing from the update response") + +func init() { + ConformanceTests = append(ConformanceTests, HTTPRoutePreserveForeignStatus) +} + +var HTTPRoutePreserveForeignStatus = confsuite.ConformanceTest{ + ShortName: "HTTPRoutePreserveForeignStatus", + Description: "An implementation must not remove or modify HTTPRoute status.parents entries " + + "whose controllerName belongs to another implementation.", + Features: []features.FeatureName{ + features.SupportGateway, + features.SupportHTTPRoute, + }, + Manifests: []string{"tests/httproute-preserve-foreign-status.yaml"}, + Provisional: true, + Test: func(t *testing.T, suite *confsuite.ConformanceTestSuite) { + ns := confsuite.InfrastructureNamespace + routeNN := types.NamespacedName{Name: "preserve-foreign-status", Namespace: ns} + gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} + + kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN) + + var seeded gatewayv1.RouteParentStatus + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + route := &gatewayv1.HTTPRoute{} + if err := suite.Client.Get(t.Context(), routeNN, route); err != nil { + return err + } + route.Status.Parents = append(route.Status.Parents, gatewayv1.RouteParentStatus{ + ParentRef: gatewayv1.ParentReference{ + Group: new(gatewayv1.Group(gatewayv1.GroupVersion.Group)), + Kind: new(gatewayv1.Kind("Gateway")), + Name: "unmanaged-gateway", + Namespace: new(gatewayv1.Namespace(ns)), + }, + ControllerName: kubernetes.StaleControllerName, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.RouteReasonAccepted), + ObservedGeneration: route.Generation, + LastTransitionTime: metav1.Now(), + }}, + }) + if err := suite.Client.Status().Update(t.Context(), route); err != nil { + return err + } + // Read the entry back off the update response rather than reusing + // the value written above: the API server truncates + // lastTransitionTime to second precision. + stored := foreignParentStatus(route.Status.Parents) + if stored == nil { + return errForeignStatusNotStored + } + seeded = *stored + return nil + }) + require.NoError(t, err, "error seeding a foreign controller's status entry on HTTPRoute %s", routeNN) + + // Bump the generation so the implementation has to run a full + // read-modify-write cycle on a status that already holds the seeded entry. + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + route := &gatewayv1.HTTPRoute{} + if getErr := suite.Client.Get(t.Context(), routeNN, route); getErr != nil { + return getErr + } + route.Spec.Rules[0].Matches[0].Path.Value = new("/preserve-foreign-status-updated") + return suite.Client.Update(t.Context(), route) + }) + require.NoError(t, err, "error updating HTTPRoute %s", routeNN) + + // The implementation's own entry catching up to the bumped generation is + // what proves it wrote status while the seeded entry was there. + kubernetes.HTTPRouteMustHaveParents(t, suite.Client, suite.TimeoutConfig, routeNN, + []gatewayv1.RouteParentStatus{ + { + ParentRef: gatewayv1.ParentReference{ + Group: new(gatewayv1.Group(gatewayv1.GroupVersion.Group)), + Kind: new(gatewayv1.Kind("Gateway")), + Name: gatewayv1.ObjectName(gwNN.Name), + Namespace: new(gatewayv1.Namespace(ns)), + }, + ControllerName: gatewayv1.GatewayController(suite.ControllerName), + Conditions: []metav1.Condition{ + { + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + }, + }, + }, + { + ParentRef: seeded.ParentRef, + ControllerName: kubernetes.StaleControllerName, + Conditions: []metav1.Condition{ + { + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.RouteReasonAccepted), + }, + }, + }, + }, + // The Route and the Gateway share a namespace, so an implementation + // that leaves parentRef.namespace unset still matches. + false, + ) + + route := &gatewayv1.HTTPRoute{} + require.NoError(t, suite.Client.Get(t.Context(), routeNN, route), "error fetching HTTPRoute %s", routeNN) + + // HTTPRouteMustHaveParents compares conditions by type, status and reason, + // so it cannot see a rewrite that keeps the entry Accepted but restamps + // observedGeneration or lastTransitionTime. + stored := foreignParentStatus(route.Status.Parents) + require.NotNilf(t, stored, "HTTPRoute %s: status entry owned by %s was removed", routeNN, kubernetes.StaleControllerName) + require.Equalf(t, seeded, *stored, "HTTPRoute %s: status entry owned by %s was modified", routeNN, kubernetes.StaleControllerName) + }, +} + +func foreignParentStatus(parents []gatewayv1.RouteParentStatus) *gatewayv1.RouteParentStatus { + for i := range parents { + if parents[i].ControllerName == kubernetes.StaleControllerName { + return &parents[i] + } + } + + return nil +} diff --git a/conformance/tests/httproute-preserve-foreign-status.yaml b/conformance/tests/httproute-preserve-foreign-status.yaml new file mode 100644 index 0000000000..df3c712092 --- /dev/null +++ b/conformance/tests/httproute-preserve-foreign-status.yaml @@ -0,0 +1,19 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: preserve-foreign-status + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: same-namespace + # No Gateway by this name exists; the test seeds a status entry for it under a + # stale controllerName and checks that the implementation leaves it alone. + - name: unmanaged-gateway + rules: + - matches: + - path: + type: PathPrefix + value: /preserve-foreign-status + backendRefs: + - name: infra-backend-v1 + port: 8080 diff --git a/conformance/utils/kubernetes/helpers.go b/conformance/utils/kubernetes/helpers.go index 95512f8a6e..c3eb8c7065 100644 --- a/conformance/utils/kubernetes/helpers.go +++ b/conformance/utils/kubernetes/helpers.go @@ -52,6 +52,18 @@ import ( // tests which validate fixing broken Gateways, e.t.c. const GatewayExcludedFromReadinessChecks = "gateway-api/skip-this-for-readiness" +// StaleControllerName is a controllerName that tests can put on a Route +// status.parents or Policy status.ancestors entry to stand in for another +// implementation sharing the cluster. Nothing reconciles such an entry, so the +// status helpers leave its observedGeneration alone. +const StaleControllerName = gatewayv1.GatewayController("gateway.networking.k8s.io/stale-controller") + +// StaleConditionType is a condition type that tests can seed into a status +// conditions list to stand in for a condition owned by another controller. +// Nothing reconciles such a condition, so FilterStaleConditions leaves its +// observedGeneration alone. +const StaleConditionType = "conformance.gateway.networking.k8s.io/StaleCondition" + const GatewayKind = gatewayv1.Kind("Gateway") // GatewayRef is a tiny type for specifying an HTTP Route ParentRef without @@ -239,10 +251,15 @@ func ConditionsHaveLatestObservedGeneration(obj metav1.Object, conditions []meta } // FilterStaleConditions returns the list of status condition whose observedGeneration does not -// match the object's metadata.Generation +// match the object's metadata.Generation. Conditions of StaleConditionType are +// exempt: they belong to no controller, so they never catch up. func FilterStaleConditions(obj metav1.Object, conditions []metav1.Condition) []metav1.Condition { stale := make([]metav1.Condition, 0, len(conditions)) for _, condition := range conditions { + if condition.Type == StaleConditionType { + continue + } + if obj.GetGeneration() != condition.ObservedGeneration { stale = append(stale, condition) } @@ -793,6 +810,23 @@ func RouteTypeMustHaveParentsField(t *testing.T, routeType any) string { return routeTypeName } +// staleParentStatus returns the first status entry whose conditions have not +// caught up with the object generation. Entries owned by StaleControllerName +// are exempt: nothing reconciles them, so their observedGeneration never moves. +func staleParentStatus(obj metav1.Object, parents []gatewayv1.RouteParentStatus) (gatewayv1.RouteParentStatus, error) { + for _, parent := range parents { + if parent.ControllerName == StaleControllerName { + continue + } + + if err := ConditionsHaveLatestObservedGeneration(obj, parent.Conditions); err != nil { + return parent, err + } + } + + return gatewayv1.RouteParentStatus{}, nil +} + func RouteMustHaveParents(t *testing.T, cli client.Client, timeoutConfig config.TimeoutConfig, routeName types.NamespacedName, parents []gatewayv1.RouteParentStatus, namespaceRequired bool, routeType any) { t.Helper() @@ -811,14 +845,13 @@ func RouteMustHaveParents(t *testing.T, cli client.Client, timeoutConfig config. return false, fmt.Errorf("error fetching %s: %w", routeTypeName, err) } - for _, parent := range actual { - if err := ConditionsHaveLatestObservedGeneration(metaObj, parent.Conditions); err != nil { - tlog.Logf(t, "%s(controller=%v,ref=%#v) %v", routeTypeName, parent.ControllerName, parent, err) - return false, nil - } + actual = reflect.ValueOf(cliObj).Elem().FieldByName("Status").FieldByName("Parents").Interface().([]v1alpha2.RouteParentStatus) + + if parent, err := staleParentStatus(metaObj, actual); err != nil { + tlog.Logf(t, "%s(controller=%v,ref=%#v) %v", routeTypeName, parent.ControllerName, parent, err) + return false, nil } - actual = reflect.ValueOf(cliObj).Elem().FieldByName("Status").FieldByName("Parents").Interface().([]v1alpha2.RouteParentStatus) return parentsForRouteMatch(t, routeName, parents, actual, namespaceRequired), nil }) require.NoErrorf(t, waitErr, "error waiting for %s to have parents matching expectations", routeTypeName) @@ -1628,6 +1661,10 @@ func BackendTLSPolicyMustHaveCondition(t *testing.T, client client.Client, timeo } for _, parent := range policy.Status.Ancestors { + if parent.ControllerName == StaleControllerName { + continue + } + if err := ConditionsHaveLatestObservedGeneration(policy, parent.Conditions); err != nil { tlog.Logf(t, "BackendTLSPolicy %s (parentRef=%v) %v", policyNN, parentRefToString(parent.AncestorRef), err, @@ -1662,6 +1699,10 @@ func BackendTLSPolicyMustHaveLatestConditions(t *testing.T, r *gatewayv1.Backend t.Helper() for _, ancestor := range r.Status.Ancestors { + if ancestor.ControllerName == StaleControllerName { + continue + } + if err := ConditionsHaveLatestObservedGeneration(r, ancestor.Conditions); err != nil { tlog.Fatalf(t, "BackendTLSPolicy(controller=%v, ancestorRef=%#v) %v", ancestor.ControllerName, parentRefToString(ancestor.AncestorRef), err) } diff --git a/conformance/utils/kubernetes/helpers_test.go b/conformance/utils/kubernetes/helpers_test.go index 1a77a1c821..3326d07085 100644 --- a/conformance/utils/kubernetes/helpers_test.go +++ b/conformance/utils/kubernetes/helpers_test.go @@ -29,7 +29,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" @@ -105,6 +107,23 @@ func TestVerifyConditionsMatchGeneration(t *testing.T) { {Type: "FakeCondition3", ObservedGeneration: 20}, }, }, + { + name: "a StaleConditionType condition is exempt from the generation check", + obj: &gatewayv1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "fake-gateway", Generation: 20}}, + conditions: []metav1.Condition{ + {Type: "FakeCondition1", ObservedGeneration: 20}, + {Type: StaleConditionType, ObservedGeneration: 3}, + }, + }, + { + name: "the StaleConditionType exemption does not extend to other conditions", + obj: &gatewayv1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "fake-gateway", Generation: 20}}, + conditions: []metav1.Condition{ + {Type: "FakeCondition1", ObservedGeneration: 19}, + {Type: StaleConditionType, ObservedGeneration: 3}, + }, + expected: fmt.Errorf("expected observedGeneration to be updated to 20 for all conditions, only 1/2 were updated. stale conditions are: FakeCondition1 (generation 19)"), + }, { name: "conditions where one does not match the generation fail verification", obj: &gatewayv1.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "fake-gateway", Generation: 20}}, @@ -379,3 +398,284 @@ func Test_listenersMatch(t *testing.T) { }) } } + +// TestRouteMustHaveParentsIgnoresStaleControllerEntries seeds an entry that is +// stale on purpose: it sits at generation 1 while the Route is at generation 2, +// so the wait only succeeds if entries owned by StaleControllerName are left out +// of the observedGeneration check. +func TestRouteMustHaveParentsIgnoresStaleControllerEntries(t *testing.T) { + const ownController = gatewayv1.GatewayController("example.com/gateway-controller") + + scheme := runtime.NewScheme() + require.NoError(t, InstallGatewayV1(scheme)) + + routeNN := types.NamespacedName{Name: "test-route", Namespace: "default"} + gwNamespace := gatewayv1.Namespace("default") + + acceptedCondition := func(observedGeneration int64) []metav1.Condition { + return []metav1.Condition{{ + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.RouteReasonAccepted), + ObservedGeneration: observedGeneration, + LastTransitionTime: metav1.Now(), + }} + } + + route := &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: routeNN.Name, + Namespace: routeNN.Namespace, + Generation: 2, + }, + Status: gatewayv1.HTTPRouteStatus{ + RouteStatus: gatewayv1.RouteStatus{ + Parents: []gatewayv1.RouteParentStatus{ + { + ParentRef: gatewayv1.ParentReference{ + Name: "test-gateway", + Namespace: &gwNamespace, + }, + ControllerName: ownController, + Conditions: acceptedCondition(2), + }, + { + ParentRef: gatewayv1.ParentReference{ + Name: "unmanaged-gateway", + Namespace: &gwNamespace, + }, + ControllerName: StaleControllerName, + Conditions: acceptedCondition(1), + }, + }, + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(route).Build() + + timeoutConfig := config.TimeoutConfig{ + RouteMustHaveParents: 2 * time.Second, + DefaultPollInterval: 100 * time.Millisecond, + } + + HTTPRouteMustHaveParents(t, c, timeoutConfig, routeNN, []gatewayv1.RouteParentStatus{ + { + ParentRef: gatewayv1.ParentReference{ + Name: "test-gateway", + Namespace: &gwNamespace, + }, + ControllerName: ownController, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + }}, + }, + }, true) +} + +func Test_staleParentStatus(t *testing.T) { + const ( + ownController = gatewayv1.GatewayController("example.com/gateway-controller") + otherController = gatewayv1.GatewayController("example.com/other-controller") + ) + + route := &gatewayv1.HTTPRoute{ObjectMeta: metav1.ObjectMeta{Generation: 2}} + + parent := func(controller gatewayv1.GatewayController, observedGeneration int64) gatewayv1.RouteParentStatus { + return gatewayv1.RouteParentStatus{ + ParentRef: gatewayv1.ParentReference{Name: "test-gateway"}, + ControllerName: controller, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.RouteReasonAccepted), + ObservedGeneration: observedGeneration, + }}, + } + } + + tests := []struct { + name string + parents []gatewayv1.RouteParentStatus + wantOwner gatewayv1.GatewayController + }{ + { + name: "every entry has caught up", + parents: []gatewayv1.RouteParentStatus{parent(ownController, 2), parent(otherController, 2)}, + }, + { + name: "own entry is behind", + parents: []gatewayv1.RouteParentStatus{parent(ownController, 1)}, + wantOwner: ownController, + }, + { + name: "sentinel entry is exempt while it is behind", + parents: []gatewayv1.RouteParentStatus{parent(ownController, 2), parent(StaleControllerName, 1)}, + }, + { + name: "the exemption does not extend to other foreign entries", + parents: []gatewayv1.RouteParentStatus{parent(ownController, 2), parent(otherController, 1)}, + wantOwner: otherController, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stale, err := staleParentStatus(route, tt.parents) + + if tt.wantOwner == "" { + require.NoError(t, err) + return + } + + require.Error(t, err) + require.Equal(t, tt.wantOwner, stale.ControllerName) + }) + } +} + +// TestBackendTLSPolicyMustHaveConditionIgnoresStaleAncestors seeds an ancestor +// that is stale on purpose: it sits at generation 1 while the policy is at +// generation 2, so the wait only succeeds if ancestors owned by +// StaleControllerName are left out of the observedGeneration check. +func TestBackendTLSPolicyMustHaveConditionIgnoresStaleAncestors(t *testing.T) { + const ownController = gatewayv1.GatewayController("example.com/gateway-controller") + + scheme := runtime.NewScheme() + require.NoError(t, InstallGatewayV1(scheme)) + + policyNN := types.NamespacedName{Name: "test-policy", Namespace: "default"} + gwNN := types.NamespacedName{Name: "test-gateway", Namespace: "default"} + gwNamespace := gatewayv1.Namespace(gwNN.Namespace) + + ancestor := func(name gatewayv1.ObjectName, controller gatewayv1.GatewayController, observedGeneration int64) gatewayv1.PolicyAncestorStatus { + return gatewayv1.PolicyAncestorStatus{ + AncestorRef: gatewayv1.ParentReference{ + Name: name, + Namespace: &gwNamespace, + }, + ControllerName: controller, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.PolicyReasonAccepted), + ObservedGeneration: observedGeneration, + LastTransitionTime: metav1.Now(), + }}, + } + } + + policy := &gatewayv1.BackendTLSPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: policyNN.Name, + Namespace: policyNN.Namespace, + Generation: 2, + }, + Status: gatewayv1.PolicyStatus{ + Ancestors: []gatewayv1.PolicyAncestorStatus{ + ancestor(gatewayv1.ObjectName(gwNN.Name), ownController, 2), + ancestor("unmanaged-gateway", StaleControllerName, 1), + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(policy).Build() + + timeoutConfig := config.TimeoutConfig{ + HTTPRouteMustHaveCondition: 2 * time.Second, + DefaultPollInterval: 100 * time.Millisecond, + } + + BackendTLSPolicyMustHaveCondition(t, c, timeoutConfig, policyNN, gwNN, metav1.Condition{ + Type: string(gatewayv1.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.PolicyReasonAccepted), + }) + + BackendTLSPolicyMustHaveLatestConditions(t, policy) +} + +// TestRouteMustHaveParentsChecksTheStatusItJustRead pins the order of the two +// steps inside the poll: the status has to be read before the +// observedGeneration check runs against it. When the read came last, the first +// iteration checked an empty slice, so a Route whose conditions still carried a +// stale observedGeneration satisfied the wait right away and the check never +// ran at all on the common fast path. +func TestRouteMustHaveParentsChecksTheStatusItJustRead(t *testing.T) { + const ownController = gatewayv1.GatewayController("example.com/gateway-controller") + + scheme := runtime.NewScheme() + require.NoError(t, InstallGatewayV1(scheme)) + + routeNN := types.NamespacedName{Name: "test-route", Namespace: "default"} + gwNamespace := gatewayv1.Namespace("default") + + route := &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: routeNN.Name, + Namespace: routeNN.Namespace, + Generation: 2, + }, + Status: gatewayv1.HTTPRouteStatus{ + RouteStatus: gatewayv1.RouteStatus{ + Parents: []gatewayv1.RouteParentStatus{{ + ParentRef: gatewayv1.ParentReference{ + Name: "test-gateway", + Namespace: &gwNamespace, + }, + ControllerName: ownController, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayv1.RouteReasonAccepted), + ObservedGeneration: 1, + LastTransitionTime: metav1.Now(), + }}, + }}, + }, + }, + } + + var reads int + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(route). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cli client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if err := cli.Get(ctx, key, obj, opts...); err != nil { + return err + } + + // The implementation catches up, but only after the first read. + reads++ + if reads > 1 { + obj.(*gatewayv1.HTTPRoute).Status.Parents[0].Conditions[0].ObservedGeneration = 2 + } + + return nil + }, + }). + Build() + + timeoutConfig := config.TimeoutConfig{ + RouteMustHaveParents: 2 * time.Second, + DefaultPollInterval: 100 * time.Millisecond, + } + + HTTPRouteMustHaveParents(t, c, timeoutConfig, routeNN, []gatewayv1.RouteParentStatus{ + { + ParentRef: gatewayv1.ParentReference{ + Name: "test-gateway", + Namespace: &gwNamespace, + }, + ControllerName: ownController, + Conditions: []metav1.Condition{{ + Type: string(gatewayv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + }}, + }, + }, true) + + require.Greater(t, reads, 1, "wait was satisfied by the first read, leaving the stale observedGeneration unchecked") +}