From bee70a2257a773e630dada2309b0c93fe53a0b3a Mon Sep 17 00:00:00 2001 From: y-rabie Date: Tue, 28 Oct 2025 13:56:27 +0300 Subject: [PATCH 1/4] fix: cleanup dangling parents in route and policy statuses Signed-off-by: y-rabie --- internal/gatewayapi/runner/runner.go | 380 +++++++++++++++----- internal/provider/kubernetes/status.go | 87 +++-- internal/provider/kubernetes/status_test.go | 155 +++----- 3 files changed, 407 insertions(+), 215 deletions(-) diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go index f3852f773d6..61ab213ccef 100644 --- a/internal/gatewayapi/runner/runner.go +++ b/internal/gatewayapi/runner/runner.go @@ -18,11 +18,14 @@ import ( "github.com/telepresenceio/watchable" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" ctrl "sigs.k8s.io/controller-runtime" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + gwapiv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + gwapiv1a3 "sigs.k8s.io/gateway-api/apis/v1alpha3" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/envoyproxy/gateway/internal/crypto" @@ -143,6 +146,9 @@ func (r *Runner) subscribeAndTranslate(sub <-chan watchable.Snapshot[string, *re var backendTLSPolicyStatusCount, clientTrafficPolicyStatusCount, backendTrafficPolicyStatusCount int var securityPolicyStatusCount, envoyExtensionPolicyStatusCount, backendStatusCount, extensionServerPolicyStatusCount int + // `aggregatedResult` aggregates status result of resources from all + // parents/ancestors, and then stores the status once for every resource. + var aggregatedResult resource.Resources for _, resources := range *val { // Translate and publish IRs. t := &gatewayapi.Translator{ @@ -218,123 +224,327 @@ func (r *Runner) subscribeAndTranslate(sub <-chan watchable.Snapshot[string, *re r.ProviderResources.GatewayClassStatuses.Store(key, &result.GatewayClass.Status) } - for _, gateway := range result.Gateways { - key := utils.NamespacedName(gateway) - r.ProviderResources.GatewayStatuses.Store(key, &gateway.Status) - gatewayStatusCount++ - delete(keysToDelete.GatewayStatus, key) - r.keyCache.GatewayStatus[key] = true - } + // Aggregate status (parents/ancestors) from all GatewayClasses. + aggregatedResult.Gateways = append(aggregatedResult.Gateways, result.Gateways...) for _, httpRoute := range result.HTTPRoutes { - key := utils.NamespacedName(httpRoute) - r.ProviderResources.HTTPRouteStatuses.Store(key, &httpRoute.Status) - httpRouteStatusCount++ - delete(keysToDelete.HTTPRouteStatus, key) - r.keyCache.HTTPRouteStatus[key] = true + var aggregatedRoute *gwapiv1.HTTPRoute + for _, ar := range aggregatedResult.HTTPRoutes { + if utils.NamespacedName(httpRoute) == utils.NamespacedName(ar) { + aggregatedRoute = ar + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status parents. + // Otherwise, append it to the list of *Routes in the accumulated result. + if aggregatedRoute != nil { + aggregatedRoute.Status.Parents = append(aggregatedRoute.Status.Parents, httpRoute.Status.Parents...) + } else { + aggregatedRoute = httpRoute + aggregatedResult.HTTPRoutes = append(aggregatedResult.HTTPRoutes, aggregatedRoute) + } } for _, grpcRoute := range result.GRPCRoutes { - key := utils.NamespacedName(grpcRoute) - r.ProviderResources.GRPCRouteStatuses.Store(key, &grpcRoute.Status) - grpcRouteStatusCount++ - delete(keysToDelete.GRPCRouteStatus, key) - r.keyCache.GRPCRouteStatus[key] = true + var aggregatedRoute *gwapiv1.GRPCRoute + for _, ar := range aggregatedResult.GRPCRoutes { + if utils.NamespacedName(grpcRoute) == utils.NamespacedName(ar) { + aggregatedRoute = ar + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status parents. + // Otherwise, append it to the list of *Routes in the accumulated result. + if aggregatedRoute != nil { + aggregatedRoute.Status.Parents = append(aggregatedRoute.Status.Parents, grpcRoute.Status.Parents...) + } else { + aggregatedRoute = grpcRoute + aggregatedResult.GRPCRoutes = append(aggregatedResult.GRPCRoutes, aggregatedRoute) + } } for _, tlsRoute := range result.TLSRoutes { - key := utils.NamespacedName(tlsRoute) - r.ProviderResources.TLSRouteStatuses.Store(key, &tlsRoute.Status) - tlsRouteStatusCount++ - delete(keysToDelete.TLSRouteStatus, key) - r.keyCache.TLSRouteStatus[key] = true + var aggregatedRoute *gwapiv1a3.TLSRoute + for _, ar := range aggregatedResult.TLSRoutes { + if utils.NamespacedName(tlsRoute) == utils.NamespacedName(ar) { + aggregatedRoute = ar + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status parents. + // Otherwise, append it to the list of *Routes in the accumulated result. + if aggregatedRoute != nil { + aggregatedRoute.Status.Parents = append(aggregatedRoute.Status.Parents, tlsRoute.Status.Parents...) + } else { + aggregatedRoute = tlsRoute + aggregatedResult.TLSRoutes = append(aggregatedResult.TLSRoutes, aggregatedRoute) + } } for _, tcpRoute := range result.TCPRoutes { - key := utils.NamespacedName(tcpRoute) - r.ProviderResources.TCPRouteStatuses.Store(key, &tcpRoute.Status) - tcpRouteStatusCount++ - delete(keysToDelete.TCPRouteStatus, key) - r.keyCache.TCPRouteStatus[key] = true + var aggregatedRoute *gwapiv1a2.TCPRoute + for _, ar := range aggregatedResult.TCPRoutes { + if utils.NamespacedName(tcpRoute) == utils.NamespacedName(ar) { + aggregatedRoute = ar + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status parents. + // Otherwise, append it to the list of *Routes in the accumulated result. + if aggregatedRoute != nil { + aggregatedRoute.Status.Parents = append(aggregatedRoute.Status.Parents, tcpRoute.Status.Parents...) + } else { + aggregatedRoute = tcpRoute + aggregatedResult.TCPRoutes = append(aggregatedResult.TCPRoutes, aggregatedRoute) + } } for _, udpRoute := range result.UDPRoutes { - key := utils.NamespacedName(udpRoute) - r.ProviderResources.UDPRouteStatuses.Store(key, &udpRoute.Status) - udpRouteStatusCount++ - delete(keysToDelete.UDPRouteStatus, key) - r.keyCache.UDPRouteStatus[key] = true + var aggregatedRoute *gwapiv1a2.UDPRoute + for _, ar := range aggregatedResult.UDPRoutes { + if utils.NamespacedName(udpRoute) == utils.NamespacedName(ar) { + aggregatedRoute = ar + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status parents. + // Otherwise, append it to the list of *Routes in the accumulated result. + if aggregatedRoute != nil { + aggregatedRoute.Status.Parents = append(aggregatedRoute.Status.Parents, udpRoute.Status.Parents...) + } else { + aggregatedRoute = udpRoute + aggregatedResult.UDPRoutes = append(aggregatedResult.UDPRoutes, aggregatedRoute) + } } - // Skip updating status for policies with empty status - // They may have been skipped in this translation because - // their target is not found (not relevant) - for _, backendTLSPolicy := range result.BackendTLSPolicies { - key := utils.NamespacedName(backendTLSPolicy) - if len(backendTLSPolicy.Status.Ancestors) > 0 { - r.ProviderResources.BackendTLSPolicyStatuses.Store(key, &backendTLSPolicy.Status) - backendTLSPolicyStatusCount++ + var aggregatedPolicy *gwapiv1.BackendTLSPolicy + for _, ap := range aggregatedResult.BackendTLSPolicies { + if utils.NamespacedName(backendTLSPolicy) == utils.NamespacedName(ap) { + aggregatedPolicy = ap + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status ancestors. + // Otherwise, append it to the list of *Policies in the accumulated result. + if aggregatedPolicy != nil { + aggregatedPolicy.Status.Ancestors = append(aggregatedPolicy.Status.Ancestors, backendTLSPolicy.Status.Ancestors...) + } else { + aggregatedPolicy = backendTLSPolicy + aggregatedResult.BackendTLSPolicies = append(aggregatedResult.BackendTLSPolicies, aggregatedPolicy) } - delete(keysToDelete.BackendTLSPolicyStatus, key) - r.keyCache.BackendTLSPolicyStatus[key] = true } - for _, clientTrafficPolicy := range result.ClientTrafficPolicies { - key := utils.NamespacedName(clientTrafficPolicy) - if len(clientTrafficPolicy.Status.Ancestors) > 0 { - r.ProviderResources.ClientTrafficPolicyStatuses.Store(key, &clientTrafficPolicy.Status) - clientTrafficPolicyStatusCount++ + var aggregatedPolicy *egv1a1.ClientTrafficPolicy + for _, ap := range aggregatedResult.ClientTrafficPolicies { + if utils.NamespacedName(clientTrafficPolicy) == utils.NamespacedName(ap) { + aggregatedPolicy = ap + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status ancestors. + // Otherwise, append it to the list of *Policies in the accumulated result. + if aggregatedPolicy != nil { + aggregatedPolicy.Status.Ancestors = append(aggregatedPolicy.Status.Ancestors, clientTrafficPolicy.Status.Ancestors...) + } else { + aggregatedPolicy = clientTrafficPolicy + aggregatedResult.ClientTrafficPolicies = append(aggregatedResult.ClientTrafficPolicies, aggregatedPolicy) } - delete(keysToDelete.ClientTrafficPolicyStatus, key) - r.keyCache.ClientTrafficPolicyStatus[key] = true } for _, backendTrafficPolicy := range result.BackendTrafficPolicies { - key := utils.NamespacedName(backendTrafficPolicy) - if len(backendTrafficPolicy.Status.Ancestors) > 0 { - r.ProviderResources.BackendTrafficPolicyStatuses.Store(key, &backendTrafficPolicy.Status) - backendTrafficPolicyStatusCount++ + var aggregatedPolicy *egv1a1.BackendTrafficPolicy + for _, ap := range aggregatedResult.BackendTrafficPolicies { + if utils.NamespacedName(backendTrafficPolicy) == utils.NamespacedName(ap) { + aggregatedPolicy = ap + break + } + } + // If already found (because processed in an earlier gatewayclass), append its status ancestors. + // Otherwise, append it to the list of *Policies in the accumulated result. + if aggregatedPolicy != nil { + aggregatedPolicy.Status.Ancestors = append(aggregatedPolicy.Status.Ancestors, backendTrafficPolicy.Status.Ancestors...) + } else { + aggregatedPolicy = backendTrafficPolicy + aggregatedResult.BackendTrafficPolicies = append(aggregatedResult.BackendTrafficPolicies, aggregatedPolicy) } - delete(keysToDelete.BackendTrafficPolicyStatus, key) - r.keyCache.BackendTrafficPolicyStatus[key] = true } for _, securityPolicy := range result.SecurityPolicies { - key := utils.NamespacedName(securityPolicy) - if len(securityPolicy.Status.Ancestors) > 0 { - r.ProviderResources.SecurityPolicyStatuses.Store(key, &securityPolicy.Status) - securityPolicyStatusCount++ + var aggregatedPolicy *egv1a1.SecurityPolicy + for _, ap := range aggregatedResult.SecurityPolicies { + if utils.NamespacedName(securityPolicy) == utils.NamespacedName(ap) { + aggregatedPolicy = ap + break + } } - delete(keysToDelete.SecurityPolicyStatus, key) - r.keyCache.SecurityPolicyStatus[key] = true + // If already found (because processed in an earlier gatewayclass), append its status ancestors. + // Otherwise, append it to the list of *Policies in the accumulated result. + if aggregatedPolicy != nil { + aggregatedPolicy.Status.Ancestors = append(aggregatedPolicy.Status.Ancestors, securityPolicy.Status.Ancestors...) + } else { + aggregatedPolicy = securityPolicy + aggregatedResult.SecurityPolicies = append(aggregatedResult.SecurityPolicies, aggregatedPolicy) + } + } for _, envoyExtensionPolicy := range result.EnvoyExtensionPolicies { - key := utils.NamespacedName(envoyExtensionPolicy) - if len(envoyExtensionPolicy.Status.Ancestors) > 0 { - r.ProviderResources.EnvoyExtensionPolicyStatuses.Store(key, &envoyExtensionPolicy.Status) - envoyExtensionPolicyStatusCount++ + var aggregatedPolicy *egv1a1.EnvoyExtensionPolicy + for _, ap := range aggregatedResult.EnvoyExtensionPolicies { + if utils.NamespacedName(envoyExtensionPolicy) == utils.NamespacedName(ap) { + aggregatedPolicy = ap + break + } } - delete(keysToDelete.EnvoyExtensionPolicyStatus, key) - r.keyCache.EnvoyExtensionPolicyStatus[key] = true - } - for _, backend := range result.Backends { - key := utils.NamespacedName(backend) - if len(backend.Status.Conditions) > 0 { - r.ProviderResources.BackendStatuses.Store(key, &backend.Status) - backendStatusCount++ + // If already found (because processed in an earlier gatewayclass), append its status ancestors. + // Otherwise, append it to the list of *Policies in the accumulated result. + if aggregatedPolicy != nil { + aggregatedPolicy.Status.Ancestors = append(aggregatedPolicy.Status.Ancestors, envoyExtensionPolicy.Status.Ancestors...) + } else { + aggregatedPolicy = envoyExtensionPolicy + aggregatedResult.EnvoyExtensionPolicies = append(aggregatedResult.EnvoyExtensionPolicies, aggregatedPolicy) } - delete(keysToDelete.BackendStatus, key) - r.keyCache.BackendStatus[key] = true } + aggregatedResult.Backends = append(aggregatedResult.Backends, result.Backends...) for _, extServerPolicy := range result.ExtensionServerPolicies { - key := message.NamespacedNameAndGVK{ - NamespacedName: utils.NamespacedName(&extServerPolicy), - GroupVersionKind: extServerPolicy.GroupVersionKind(), + var aggregatedPolicy *unstructured.Unstructured + for _, ap := range aggregatedResult.ExtensionServerPolicies { + if utils.NamespacedName(&extServerPolicy) == utils.NamespacedName(&ap) { + aggregatedPolicy = &ap + break + } } - if statusObj, hasStatus := extServerPolicy.Object["status"]; hasStatus && statusObj != nil { - if statusMap, ok := statusObj.(map[string]any); ok && len(statusMap) > 0 { - policyStatus := unstructuredToPolicyStatus(statusMap) - r.ProviderResources.ExtensionPolicyStatuses.Store(key, &policyStatus) - extensionServerPolicyStatusCount++ + + // If already found (because processed in an earlier gatewayclass), append its status ancestors. + // Otherwise, append it to the list of *Policies in the accumulated result. + if aggregatedPolicy != nil { + ancestorsObject, found, err := unstructured.NestedFieldCopy(aggregatedPolicy.Object, "status", "ancestors") + if !found || err != nil { + r.Logger.Error(err, "failed to get existing ancestors", "policyName", aggregatedPolicy.GetName(), "ancestorsFound", found) + continue + } + ancestors, ok := ancestorsObject.([]gwapiv1.PolicyAncestorStatus) + if !ok { + r.Logger.Error(err, "failed to assert existing ancestors type", "policyName", aggregatedPolicy.GetName()) + continue } + + newAncestorsObject, found, err := unstructured.NestedFieldCopy(extServerPolicy.Object, "status", "ancestors") + if !found || err != nil { + r.Logger.Error(err, "failed to get new ancestors", "policyName", aggregatedPolicy.GetName(), "ancestorsFound", found) + continue + } + newAncestors, ok := newAncestorsObject.([]gwapiv1.PolicyAncestorStatus) + if !ok { + r.Logger.Error(err, "failed to assert new ancestors type", "policyName", aggregatedPolicy.GetName()) + continue + } + + ancestors = append(ancestors, newAncestors...) + err = unstructured.SetNestedField(aggregatedPolicy.Object, ancestors, "status", "ancestors") + if err != nil { + r.Logger.Error(err, "failed to update ancestors of policy", "policyName", aggregatedPolicy.GetName()) + } + } else { + aggregatedPolicy = &extServerPolicy + aggregatedResult.ExtensionServerPolicies = append(aggregatedResult.ExtensionServerPolicies, *aggregatedPolicy) + } + } + } + + // Store the stauses of all objects atomically with the aggregated status. + for _, gateway := range aggregatedResult.Gateways { + key := utils.NamespacedName(gateway) + r.ProviderResources.GatewayStatuses.Store(key, &gateway.Status) + gatewayStatusCount++ + delete(keysToDelete.GatewayStatus, key) + r.keyCache.GatewayStatus[key] = true + } + for _, httpRoute := range aggregatedResult.HTTPRoutes { + key := utils.NamespacedName(httpRoute) + r.ProviderResources.HTTPRouteStatuses.Store(key, &httpRoute.Status) + httpRouteStatusCount++ + delete(keysToDelete.HTTPRouteStatus, key) + r.keyCache.HTTPRouteStatus[key] = true + } + for _, grpcRoute := range aggregatedResult.GRPCRoutes { + key := utils.NamespacedName(grpcRoute) + r.ProviderResources.GRPCRouteStatuses.Store(key, &grpcRoute.Status) + grpcRouteStatusCount++ + delete(keysToDelete.GRPCRouteStatus, key) + r.keyCache.GRPCRouteStatus[key] = true + + } + for _, tlsRoute := range aggregatedResult.TLSRoutes { + key := utils.NamespacedName(tlsRoute) + r.ProviderResources.TLSRouteStatuses.Store(key, &tlsRoute.Status) + tlsRouteStatusCount++ + delete(keysToDelete.TLSRouteStatus, key) + r.keyCache.TLSRouteStatus[key] = true + } + for _, tcpRoute := range aggregatedResult.TCPRoutes { + key := utils.NamespacedName(tcpRoute) + r.ProviderResources.TCPRouteStatuses.Store(key, &tcpRoute.Status) + tcpRouteStatusCount++ + delete(keysToDelete.TCPRouteStatus, key) + r.keyCache.TCPRouteStatus[key] = true + } + for _, udpRoute := range aggregatedResult.UDPRoutes { + key := utils.NamespacedName(udpRoute) + r.ProviderResources.UDPRouteStatuses.Store(key, &udpRoute.Status) + udpRouteStatusCount++ + delete(keysToDelete.UDPRouteStatus, key) + r.keyCache.UDPRouteStatus[key] = true + } + for _, backendTLSPolicy := range aggregatedResult.BackendTLSPolicies { + key := utils.NamespacedName(backendTLSPolicy) + r.ProviderResources.BackendTLSPolicyStatuses.Store(key, &backendTLSPolicy.Status) + backendTLSPolicyStatusCount++ + delete(keysToDelete.BackendTLSPolicyStatus, key) + r.keyCache.BackendTLSPolicyStatus[key] = true + } + for _, clientTrafficPolicy := range aggregatedResult.ClientTrafficPolicies { + key := utils.NamespacedName(clientTrafficPolicy) + r.ProviderResources.ClientTrafficPolicyStatuses.Store(key, &clientTrafficPolicy.Status) + clientTrafficPolicyStatusCount++ + delete(keysToDelete.ClientTrafficPolicyStatus, key) + r.keyCache.ClientTrafficPolicyStatus[key] = true + } + for _, backendTrafficPolicy := range aggregatedResult.BackendTrafficPolicies { + key := utils.NamespacedName(backendTrafficPolicy) + r.ProviderResources.BackendTrafficPolicyStatuses.Store(key, &backendTrafficPolicy.Status) + backendTrafficPolicyStatusCount++ + delete(keysToDelete.BackendTrafficPolicyStatus, key) + r.keyCache.BackendTrafficPolicyStatus[key] = true + } + for _, securityPolicy := range aggregatedResult.SecurityPolicies { + key := utils.NamespacedName(securityPolicy) + r.ProviderResources.SecurityPolicyStatuses.Store(key, &securityPolicy.Status) + securityPolicyStatusCount++ + delete(keysToDelete.SecurityPolicyStatus, key) + r.keyCache.SecurityPolicyStatus[key] = true + } + for _, envoyExtensionPolicy := range aggregatedResult.EnvoyExtensionPolicies { + key := utils.NamespacedName(envoyExtensionPolicy) + r.ProviderResources.EnvoyExtensionPolicyStatuses.Store(key, &envoyExtensionPolicy.Status) + envoyExtensionPolicyStatusCount++ + delete(keysToDelete.EnvoyExtensionPolicyStatus, key) + r.keyCache.EnvoyExtensionPolicyStatus[key] = true + } + for _, backend := range aggregatedResult.Backends { + key := utils.NamespacedName(backend) + r.ProviderResources.BackendStatuses.Store(key, &backend.Status) + backendStatusCount++ + delete(keysToDelete.BackendStatus, key) + r.keyCache.BackendStatus[key] = true + } + for _, extServerPolicy := range aggregatedResult.ExtensionServerPolicies { + key := message.NamespacedNameAndGVK{ + NamespacedName: utils.NamespacedName(&extServerPolicy), + GroupVersionKind: extServerPolicy.GroupVersionKind(), + } + if statusObj, hasStatus := extServerPolicy.Object["status"]; hasStatus && statusObj != nil { + if statusMap, ok := statusObj.(map[string]any); ok && len(statusMap) > 0 { + policyStatus := unstructuredToPolicyStatus(statusMap) + r.ProviderResources.ExtensionPolicyStatuses.Store(key, &policyStatus) + extensionServerPolicyStatusCount++ } - delete(keysToDelete.ExtensionServerPolicyStatus, key) - r.keyCache.ExtensionServerPolicyStatus[key] = true } + delete(keysToDelete.ExtensionServerPolicyStatus, key) + r.keyCache.ExtensionServerPolicyStatus[key] = true } // Publish aggregated metrics diff --git a/internal/provider/kubernetes/status.go b/internal/provider/kubernetes/status.go index bf038249d73..22156be4ede 100644 --- a/internal/provider/kubernetes/status.go +++ b/internal/provider/kubernetes/status.go @@ -8,6 +8,7 @@ package kubernetes import ( "context" "fmt" + "reflect" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" @@ -107,7 +108,7 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context Spec: h.Spec, Status: gwapiv1.HTTPRouteStatus{ RouteStatus: gwapiv1.RouteStatus{ - Parents: mergeRouteParentStatus(h.Namespace, h.Status.Parents, val.Parents), + Parents: mergeStatus(h.Namespace, r.envoyGateway.Gateway.ControllerName, h.Status.Parents, val.Parents), }, }, } @@ -145,7 +146,7 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context Spec: g.Spec, Status: gwapiv1.GRPCRouteStatus{ RouteStatus: gwapiv1.RouteStatus{ - Parents: mergeRouteParentStatus(g.Namespace, g.Status.Parents, val.Parents), + Parents: mergeStatus(g.Namespace, r.envoyGateway.Gateway.ControllerName, g.Status.Parents, val.Parents), }, }, } @@ -185,7 +186,7 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context Spec: t.Spec, Status: gwapiv1a2.TLSRouteStatus{ RouteStatus: gwapiv1.RouteStatus{ - Parents: mergeRouteParentStatus(t.Namespace, t.Status.Parents, val.Parents), + Parents: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Parents, val.Parents), }, }, } @@ -225,7 +226,7 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context Spec: t.Spec, Status: gwapiv1a2.TCPRouteStatus{ RouteStatus: gwapiv1.RouteStatus{ - Parents: mergeRouteParentStatus(t.Namespace, t.Status.Parents, val.Parents), + Parents: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Parents, val.Parents), }, }, } @@ -265,7 +266,7 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context Spec: u.Spec, Status: gwapiv1a2.UDPRouteStatus{ RouteStatus: gwapiv1.RouteStatus{ - Parents: mergeRouteParentStatus(u.Namespace, u.Status.Parents, val.Parents), + Parents: mergeStatus(u.Namespace, r.envoyGateway.Gateway.ControllerName, u.Status.Parents, val.Parents), }, }, } @@ -303,7 +304,9 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context TypeMeta: t.TypeMeta, ObjectMeta: t.ObjectMeta, Spec: t.Spec, - Status: *val, + Status: gwapiv1.PolicyStatus{ + Ancestors: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Ancestors, val.Ancestors), + }, } return tCopy }), @@ -339,7 +342,9 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context TypeMeta: t.TypeMeta, ObjectMeta: t.ObjectMeta, Spec: t.Spec, - Status: *val, + Status: gwapiv1.PolicyStatus{ + Ancestors: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Ancestors, val.Ancestors), + }, } return tCopy }), @@ -375,7 +380,9 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context TypeMeta: t.TypeMeta, ObjectMeta: t.ObjectMeta, Spec: t.Spec, - Status: *val, + Status: gwapiv1.PolicyStatus{ + Ancestors: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Ancestors, val.Ancestors), + }, } return tCopy }), @@ -411,7 +418,9 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context TypeMeta: t.TypeMeta, ObjectMeta: t.ObjectMeta, Spec: t.Spec, - Status: *val, + Status: gwapiv1.PolicyStatus{ + Ancestors: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Ancestors, val.Ancestors), + }, } return tCopy }), @@ -445,7 +454,9 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context TypeMeta: t.TypeMeta, ObjectMeta: t.ObjectMeta, Spec: t.Spec, - Status: *val, + Status: gwapiv1.PolicyStatus{ + Ancestors: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Ancestors, val.Ancestors), + }, } return tCopy }), @@ -481,7 +492,9 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context TypeMeta: t.TypeMeta, ObjectMeta: t.ObjectMeta, Spec: t.Spec, - Status: *val, + Status: gwapiv1.PolicyStatus{ + Ancestors: mergeStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Ancestors, val.Ancestors), + }, } return tCopy }), @@ -554,6 +567,15 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context panic(err) } tCopy := t.DeepCopy() + var oldAncestors []gwapiv1.PolicyAncestorStatus + o, found, err := unstructured.NestedFieldCopy(tCopy.Object, "status", "ancestors") + if found && err == nil { + oldAncestors, ok = o.([]gwapiv1.PolicyAncestorStatus) + if ok { + tCopy.Object["status"] = mergeStatus(t.GetNamespace(), r.envoyGateway.Gateway.ControllerName, oldAncestors, val.Ancestors) + return tCopy + } + } tCopy.Object["status"] = *val return tCopy }), @@ -565,31 +587,28 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context } } -// mergeRouteParentStatus merges the old and new RouteParentStatus. -// This is needed because the RouteParentStatus doesn't support strategic merge patch yet. -func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []gwapiv1.RouteParentStatus { +// mergeStatus merges the old and new `RouteParentStatus`/`PolicyAncestorStatus`. +// This is needed because the `RouteParentStatus`/`PolicyAncestorStatus` doesn't support strategic merge patch yet. +// This depends on the fact that we get the full updated status of the route/policy (all parents/ancestors), and will break otherwise. +func mergeStatus[K interface{}](ns, controllerName string, old, new []K) []K { // Allocating with worst-case capacity to avoid reallocation. - merged := make([]gwapiv1.RouteParentStatus, 0, len(old)+len(new)) + merged := make([]K, 0, len(old)+len(new)) // Range over old status parentRefs in order: // 1. The parentRef exists in the new status: append the new one to the final status. // 2. The parentRef doesn't exist in the new status and it's not our controller: append it to the final status. - // 3. The parentRef doesn't exist in the new status, and it is our controller: keep it in the final status. - // This is important for routes with multiple parent references - not all parents are updated in each reconciliation. + // 3. The parentRef doesn't exist in the new status, and it is our controller: don't append it to the final status. for _, oldP := range old { found := -1 for newI, newP := range new { - if gatewayapi.IsParentRefEqual(oldP.ParentRef, newP.ParentRef, ns) { + if isParentOrAncestorRefEqual(oldP, newP, ns) { found = newI break } } if found >= 0 { merged = append(merged, new[found]) - } else { - // Keep all old parent statuses, regardless of controller. - // For routes with multiple parents managed by the same controller, - // not all parents are necessarily updated in each reconciliation. + } else if parentOrAncestorControllerName(oldP) != gwapiv1.GatewayController(controllerName) { merged = append(merged, oldP) } } @@ -598,7 +617,7 @@ func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []g for _, newP := range new { found := false for _, mergedP := range merged { - if gatewayapi.IsParentRefEqual(newP.ParentRef, mergedP.ParentRef, ns) { + if isParentOrAncestorRefEqual(newP, mergedP, ns) { found = true break } @@ -611,6 +630,28 @@ func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []g return merged } +func isParentOrAncestorRefEqual[K any](firstRef, secondRef K, ns string) bool { + switch reflect.TypeOf(firstRef) { + case reflect.TypeOf(gwapiv1.RouteParentStatus{}): + return gatewayapi.IsParentRefEqual(any(firstRef).(gwapiv1.RouteParentStatus).ParentRef, any(secondRef).(gwapiv1.RouteParentStatus).ParentRef, ns) + case reflect.TypeOf(gwapiv1.PolicyAncestorStatus{}): + return gatewayapi.IsParentRefEqual(any(firstRef).(gwapiv1.PolicyAncestorStatus).AncestorRef, any(secondRef).(gwapiv1.PolicyAncestorStatus).AncestorRef, ns) + default: + return false + } +} + +func parentOrAncestorControllerName[K any](ref K) gwapiv1.GatewayController { + switch reflect.TypeOf(ref) { + case reflect.TypeOf(gwapiv1.RouteParentStatus{}): + return any(ref).(gwapiv1.RouteParentStatus).ControllerName + case reflect.TypeOf(gwapiv1.PolicyAncestorStatus{}): + return any(ref).(gwapiv1.PolicyAncestorStatus).ControllerName + default: + return gwapiv1.GatewayController("") + } +} + func (r *gatewayAPIReconciler) updateStatusForGateway(ctx context.Context, gtw *gwapiv1.Gateway) { // nil check for unit tests. if r.statusUpdater == nil { diff --git a/internal/provider/kubernetes/status_test.go b/internal/provider/kubernetes/status_test.go index 1cb4c5730a3..d760ed06c29 100644 --- a/internal/provider/kubernetes/status_test.go +++ b/internal/provider/kubernetes/status_test.go @@ -14,7 +14,7 @@ import ( gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" ) -func Test_mergeRouteParentStatus(t *testing.T) { +func Test_mergeStatusForRoutes(t *testing.T) { type args struct { old []gwapiv1.RouteParentStatus new []gwapiv1.RouteParentStatus @@ -361,24 +361,6 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, - { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", - }, - Conditions: []metav1.Condition{ - { - Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: "Accepted", - }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", - }, - }, - }, { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ @@ -474,24 +456,6 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, - { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", - }, - Conditions: []metav1.Condition{ - { - Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: "Accepted", - }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", - }, - }, - }, }, }, @@ -706,24 +670,6 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, want: []gwapiv1.RouteParentStatus{ - { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", - }, - Conditions: []metav1.Condition{ - { - Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: "Accepted", - }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", - }, - }, - }, { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ @@ -744,10 +690,9 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, - // Test that parent refs managed by our controller are preserved even when not in new update. - // This is important for routes with multiple parent references. + // Similar note about practicality of occurrence. { - name: "old contains one parentRef of ours, and it's not in new - should be preserved.", + name: "old contains one parentRef of ours, and it gets dropped in new.", args: args{ old: []gwapiv1.RouteParentStatus{ { @@ -771,36 +716,39 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, new: []gwapiv1.RouteParentStatus{}, }, - want: []gwapiv1.RouteParentStatus{ - { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", - }, - Conditions: []metav1.Condition{ - { - Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: "Accepted", - }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", - }, - }, - }, - }, + want: []gwapiv1.RouteParentStatus{}, }, - // Test multi-parent scenario where only one parent is updated at a time. + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := mergeStatus("default", "gateway.envoyproxy.io/gatewayclass-controller", tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { + t.Errorf("mergeStatus() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_mergeStatusForPolicies(t *testing.T) { + type args struct { + old []gwapiv1.PolicyAncestorStatus + new []gwapiv1.PolicyAncestorStatus + } + tests := []struct { + name string + args args + want []gwapiv1.PolicyAncestorStatus + }{ { - name: "multiple parents from same controller - update one, preserve others", + name: "old contains one ancestorRef of ours and one of another controller's, status of ours changed in new.", args: args{ - old: []gwapiv1.RouteParentStatus{ + old: []gwapiv1.PolicyAncestorStatus{ { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + ControllerName: "istio.io/gateway-controller", + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), }, Conditions: []metav1.Condition{ { @@ -812,7 +760,7 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ + AncestorRef: gwapiv1.ParentReference{ Name: "gateway2", }, Conditions: []metav1.Condition{ @@ -824,32 +772,30 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, - new: []gwapiv1.RouteParentStatus{ + new: []gwapiv1.PolicyAncestorStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway2", }, Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, + Status: metav1.ConditionFalse, Reason: "Accepted", }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", - }, }, }, }, }, - want: []gwapiv1.RouteParentStatus{ + want: []gwapiv1.PolicyAncestorStatus{ { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + ControllerName: "istio.io/gateway-controller", + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), }, Conditions: []metav1.Condition{ { @@ -857,22 +803,17 @@ func Test_mergeRouteParentStatus(t *testing.T) { Status: metav1.ConditionTrue, Reason: "Accepted", }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", - }, }, }, { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", - ParentRef: gwapiv1.ParentReference{ + AncestorRef: gwapiv1.ParentReference{ Name: "gateway2", }, Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, + Status: metav1.ConditionFalse, Reason: "Accepted", }, }, @@ -882,8 +823,8 @@ func Test_mergeRouteParentStatus(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := mergeRouteParentStatus("default", tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { - t.Errorf("mergeRouteParentStatus() = %v, want %v", got, tt.want) + if got := mergeStatus("default", "gateway.envoyproxy.io/gatewayclass-controller", tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { + t.Errorf("mergeStatus() = %v, want %v", got, tt.want) } }) } From 31273229c9b4e54c05f09bdf8d78fe22bd2d88ad Mon Sep 17 00:00:00 2001 From: y-rabie Date: Tue, 28 Oct 2025 13:57:08 +0300 Subject: [PATCH 2/4] fix: only process security/backend/envoyextension policies of our controller Signed-off-by: y-rabie --- internal/gatewayapi/backendtrafficpolicy.go | 22 +++++++----- internal/gatewayapi/envoyextensionpolicy.go | 38 +++++++++++++------- internal/gatewayapi/securitypolicy.go | 39 ++++++++++++++------- internal/gatewayapi/securitypolicy_test.go | 26 ++++++++++++-- 4 files changed, 90 insertions(+), 35 deletions(-) diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index a339d3b8c8d..0d186aba79d 100644 --- a/internal/gatewayapi/backendtrafficpolicy.go +++ b/internal/gatewayapi/backendtrafficpolicy.go @@ -105,7 +105,7 @@ func (t *Translator) ProcessBackendTrafficPolicies(resources *resource.Resources } t.processBackendTrafficPolicyForRoute(resources, xdsIR, - routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) + gatewayMap, routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) } } } @@ -125,7 +125,7 @@ func (t *Translator) ProcessBackendTrafficPolicies(resources *resource.Resources } t.processBackendTrafficPolicyForRoute(resources, xdsIR, - routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) + gatewayMap, routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) } } } @@ -224,6 +224,7 @@ func (t *Translator) buildGatewayPolicyMap( func (t *Translator) processBackendTrafficPolicyForRoute( resources *resource.Resources, xdsIR resource.XdsIRMap, + gatewayMap map[types.NamespacedName]*policyGatewayTargetContext, routeMap map[policyTargetRouteKey]*policyRouteTargetContext, gatewayRouteMap *GatewayPolicyRouteMap, gatewayPolicyMergedMap *GatewayPolicyRouteMap, @@ -253,12 +254,17 @@ func (t *Translator) processBackendTrafficPolicyForRoute( // parentRefCtxs holds parent gateway/listener contexts for using in policy merge logic. parentRefCtxs := make([]*RouteParentContext, 0, len(parentRefs)) for _, p := range parentRefs { - if p.Kind == nil || *p.Kind == resource.KindGateway { - namespace := targetedRoute.GetNamespace() - if p.Namespace != nil { - namespace = string(*p.Namespace) - } - + namespace := targetedRoute.GetNamespace() + if p.Namespace != nil { + namespace = string(*p.Namespace) + } + gwNN := types.NamespacedName{ + Namespace: namespace, + Name: string(p.Name), + } + // Check if it's a gateway, and that it's a gateway that belongs to the gatewayclass we're processing. + // Otherwise it may belong to another gatewayclass or another controller. + if _, ok := gatewayMap[gwNN]; ok && (p.Kind == nil || *p.Kind == resource.KindGateway) { mapKey := NamespacedNameWithSection{ NamespacedName: types.NamespacedName{ Name: string(p.Name), diff --git a/internal/gatewayapi/envoyextensionpolicy.go b/internal/gatewayapi/envoyextensionpolicy.go index c0322b943e5..9aa4e8d0529 100644 --- a/internal/gatewayapi/envoyextensionpolicy.go +++ b/internal/gatewayapi/envoyextensionpolicy.go @@ -86,7 +86,7 @@ func (t *Translator) ProcessEnvoyExtensionPolicies(envoyExtensionPolicies []*egv } t.processEnvoyExtensionPolicyForRoute(resources, xdsIR, - routeMap, gatewayRouteMap, policy, currTarget) + routeMap, gatewayMap, gatewayRouteMap, policy, currTarget) } } } @@ -106,7 +106,7 @@ func (t *Translator) ProcessEnvoyExtensionPolicies(envoyExtensionPolicies []*egv } t.processEnvoyExtensionPolicyForRoute(resources, xdsIR, - routeMap, gatewayRouteMap, policy, currTarget) + routeMap, gatewayMap, gatewayRouteMap, policy, currTarget) } } } @@ -164,6 +164,7 @@ func (t *Translator) processEnvoyExtensionPolicyForRoute( resources *resource.Resources, xdsIR resource.XdsIRMap, routeMap map[policyTargetRouteKey]*policyRouteTargetContext, + gatewayMap map[types.NamespacedName]*policyGatewayTargetContext, gatewayRouteMap map[string]map[string]sets.Set[string], policy *egv1a1.EnvoyExtensionPolicy, currTarget gwapiv1.LocalPolicyTargetReferenceWithSectionName, @@ -188,15 +189,16 @@ func (t *Translator) processEnvoyExtensionPolicyForRoute( // policy overrides and populate its ancestor status. parentRefs := GetParentReferences(targetedRoute) for _, p := range parentRefs { - if p.Kind == nil || *p.Kind == resource.KindGateway { - namespace := targetedRoute.GetNamespace() - if p.Namespace != nil { - namespace = string(*p.Namespace) - } - gwNN := types.NamespacedName{ - Namespace: namespace, - Name: string(p.Name), - } + namespace := targetedRoute.GetNamespace() + if p.Namespace != nil { + namespace = string(*p.Namespace) + } + gwNN := types.NamespacedName{ + Namespace: namespace, + Name: string(p.Name), + } + + if _, ok := gatewayMap[gwNN]; ok && (p.Kind == nil || *p.Kind == resource.KindGateway) { key := gwNN.String() if _, ok := gatewayRouteMap[key]; !ok { @@ -230,7 +232,7 @@ func (t *Translator) processEnvoyExtensionPolicyForRoute( } // Set conditions for translation error if it got any - if err := t.translateEnvoyExtensionPolicyForRoute(policy, targetedRoute, currTarget, xdsIR, resources); err != nil { + if err := t.translateEnvoyExtensionPolicyForRoute(policy, targetedRoute, currTarget, gatewayMap, xdsIR, resources); err != nil { status.SetTranslationErrorForPolicyAncestors(&policy.Status, ancestorRefs, t.GatewayControllerName, @@ -454,6 +456,7 @@ func (t *Translator) translateEnvoyExtensionPolicyForRoute( policy *egv1a1.EnvoyExtensionPolicy, route RouteContext, target gwapiv1.LocalPolicyTargetReferenceWithSectionName, + gatewayMap map[types.NamespacedName]*policyGatewayTargetContext, xdsIR resource.XdsIRMap, resources *resource.Resources, ) error { @@ -475,6 +478,17 @@ func (t *Translator) translateEnvoyExtensionPolicyForRoute( parentRefs := GetParentReferences(route) routesWithDirectResponse := sets.New[string]() for _, p := range parentRefs { + namespace := route.GetNamespace() + if p.Namespace != nil { + namespace = string(*p.Namespace) + } + gwNN := types.NamespacedName{ + Namespace: namespace, + Name: string(p.Name), + } + if _, ok := gatewayMap[gwNN]; !ok { + continue + } parentRefCtx := GetRouteParentContext(route, p, t.GatewayControllerName) gtwCtx := parentRefCtx.GetGateway() if gtwCtx == nil { diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go index 4dca669f882..fecf79fddf1 100644 --- a/internal/gatewayapi/securitypolicy.go +++ b/internal/gatewayapi/securitypolicy.go @@ -113,7 +113,7 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security } t.processSecurityPolicyForRoute(resources, xdsIR, - routeMap, gatewayRouteMap, policy, currTarget) + routeMap, gatewayMap, gatewayRouteMap, policy, currTarget) } } } @@ -132,7 +132,7 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security } t.processSecurityPolicyForRoute(resources, xdsIR, - routeMap, gatewayRouteMap, policy, currTarget) + routeMap, gatewayMap, gatewayRouteMap, policy, currTarget) } } } @@ -188,6 +188,7 @@ func (t *Translator) processSecurityPolicyForRoute( resources *resource.Resources, xdsIR resource.XdsIRMap, routeMap map[policyTargetRouteKey]*policyRouteTargetContext, + gatewayMap map[types.NamespacedName]*policyGatewayTargetContext, gatewayRouteMap map[string]map[string]sets.Set[string], policy *egv1a1.SecurityPolicy, currTarget gwapiv1.LocalPolicyTargetReferenceWithSectionName, @@ -212,16 +213,15 @@ func (t *Translator) processSecurityPolicyForRoute( // The parent gateways are also used to set the status of the policy. parentRefs := GetParentReferences(targetedRoute) for _, p := range parentRefs { - if p.Kind == nil || *p.Kind == resource.KindGateway { - namespace := targetedRoute.GetNamespace() - if p.Namespace != nil { - namespace = string(*p.Namespace) - } - gwNN := types.NamespacedName{ - Namespace: namespace, - Name: string(p.Name), - } - + namespace := targetedRoute.GetNamespace() + if p.Namespace != nil { + namespace = string(*p.Namespace) + } + gwNN := types.NamespacedName{ + Namespace: namespace, + Name: string(p.Name), + } + if _, ok := gatewayMap[gwNN]; ok && (p.Kind == nil || *p.Kind == resource.KindGateway) { key := gwNN.String() if _, ok := gatewayRouteMap[key]; !ok { gatewayRouteMap[key] = make(map[string]sets.Set[string]) @@ -271,7 +271,7 @@ func (t *Translator) processSecurityPolicyForRoute( return } - if err := t.translateSecurityPolicyForRoute(policy, targetedRoute, currTarget, resources, xdsIR); err != nil { + if err := t.translateSecurityPolicyForRoute(policy, targetedRoute, currTarget, gatewayMap, resources, xdsIR); err != nil { status.SetTranslationErrorForPolicyAncestors(&policy.Status, parentGateways, t.GatewayControllerName, @@ -598,6 +598,7 @@ func (t *Translator) translateSecurityPolicyForRoute( policy *egv1a1.SecurityPolicy, route RouteContext, target gwapiv1.LocalPolicyTargetReferenceWithSectionName, + gatewayMap map[types.NamespacedName]*policyGatewayTargetContext, resources *resource.Resources, xdsIR resource.XdsIRMap, ) error { @@ -647,6 +648,18 @@ func (t *Translator) translateSecurityPolicyForRoute( parentRefs := GetParentReferences(route) routesWithDirectResponse := sets.New[string]() for _, p := range parentRefs { + namespace := route.GetNamespace() + if p.Namespace != nil { + namespace = string(*p.Namespace) + } + gwNN := types.NamespacedName{ + Namespace: namespace, + Name: string(p.Name), + } + if _, ok := gatewayMap[gwNN]; !ok { + continue + } + parentRefCtx := GetRouteParentContext(route, p, t.GatewayControllerName) gtwCtx := parentRefCtx.GetGateway() if gtwCtx == nil { diff --git a/internal/gatewayapi/securitypolicy_test.go b/internal/gatewayapi/securitypolicy_test.go index 16067ef8a75..c173a5eedc9 100644 --- a/internal/gatewayapi/securitypolicy_test.go +++ b/internal/gatewayapi/securitypolicy_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/utils/ptr" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -23,6 +24,7 @@ import ( egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/envoyproxy/gateway/internal/gatewayapi/resource" + "github.com/envoyproxy/gateway/internal/utils" ) func Test_wildcard2regex(t *testing.T) { @@ -933,12 +935,22 @@ func Test_SecurityPolicy_TCP_Invalid_setsStatus_and_returns(t *testing.T) { } routeMap[key] = &policyRouteTargetContext{RouteContext: tcpRoute} + // Create the gateway map with gateways that belong to our controller + gw := &gwapiv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-gateway", + Namespace: "default", + }, + } + gatewayMap := make(map[types.NamespacedName]*policyGatewayTargetContext, 1) + gatewayMap[utils.NamespacedName(gw)] = &policyGatewayTargetContext{GatewayContext: &GatewayContext{Gateway: gw}} + gatewayRouteMap := make(map[string]map[string]sets.Set[string]) resources := resource.NewResources() xdsIR := make(resource.XdsIRMap) // Process the policy - this should set error status - tr.processSecurityPolicyForRoute(resources, xdsIR, routeMap, gatewayRouteMap, policy, target) + tr.processSecurityPolicyForRoute(resources, xdsIR, routeMap, gatewayMap, gatewayRouteMap, policy, target) // Assert that the policy has a False condition (error was set) require.True(t, hasParentFalseCondition(policy)) @@ -1007,12 +1019,22 @@ func Test_SecurityPolicy_HTTP_Invalid_setsStatus_and_returns(t *testing.T) { } routeMap[key] = &policyRouteTargetContext{RouteContext: httpRoute} + // Create the gateway map with gateways that belong to our controller + gw := &gwapiv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-gateway", + Namespace: "default", + }, + } + gatewayMap := make(map[types.NamespacedName]*policyGatewayTargetContext, 1) + gatewayMap[utils.NamespacedName(gw)] = &policyGatewayTargetContext{GatewayContext: &GatewayContext{Gateway: gw}} + gatewayRouteMap := make(map[string]map[string]sets.Set[string]) resources := resource.NewResources() xdsIR := make(resource.XdsIRMap) // Process the policy - this should set error status - tr.processSecurityPolicyForRoute(resources, xdsIR, routeMap, gatewayRouteMap, policy, target) + tr.processSecurityPolicyForRoute(resources, xdsIR, routeMap, gatewayMap, gatewayRouteMap, policy, target) // Assert that the policy has a False condition (error was set) require.True(t, hasParentFalseCondition(policy)) From b8decb8363da68025df18132424a96b9ee7b5e5e Mon Sep 17 00:00:00 2001 From: y-rabie Date: Tue, 4 Nov 2025 19:59:22 +0200 Subject: [PATCH 3/4] fix and add unit test for merging ancestors of ExtensionServerPolicies Signed-off-by: y-rabie --- internal/gatewayapi/extensionserverpolicy.go | 54 +++++++-- .../gatewayapi/extensionserverpolicy_test.go | 112 ++++++++++++++++++ internal/gatewayapi/runner/runner.go | 42 +------ 3 files changed, 159 insertions(+), 49 deletions(-) diff --git a/internal/gatewayapi/extensionserverpolicy.go b/internal/gatewayapi/extensionserverpolicy.go index e378c270ba1..c7f5fe54d8f 100644 --- a/internal/gatewayapi/extensionserverpolicy.go +++ b/internal/gatewayapi/extensionserverpolicy.go @@ -81,7 +81,7 @@ func (t *Translator) ProcessExtensionServerPolicies(policies []unstructured.Unst } if accepted { res = append(res, *policy) - policy.Object["status"] = policyStatusToUnstructured(policyStatus) + policy.Object["status"] = PolicyStatusToUnstructured(policyStatus) } } @@ -108,14 +108,6 @@ func extractTargetRefs(policy *unstructured.Unstructured, gateways []*GatewayCon return ret, nil } -func policyStatusToUnstructured(policyStatus gwapiv1.PolicyStatus) map[string]any { - ret := map[string]any{} - // No need to check the marshal/unmarshal error here - d, _ := json.Marshal(policyStatus) - _ = json.Unmarshal(d, &ret) - return ret -} - func resolveExtServerPolicyGatewayTargetRef(policy *unstructured.Unstructured, target gwapiv1.LocalPolicyTargetReferenceWithSectionName, gateways map[types.NamespacedName]*policyGatewayTargetContext) *GatewayContext { // Check if the gateway exists key := types.NamespacedName{ @@ -132,6 +124,24 @@ func resolveExtServerPolicyGatewayTargetRef(policy *unstructured.Unstructured, t return gateway.GatewayContext } +func PolicyStatusToUnstructured(policyStatus gwapiv1.PolicyStatus) map[string]any { + ret := map[string]any{} + // No need to check the marshal/unmarshal error here + d, _ := json.Marshal(policyStatus) + _ = json.Unmarshal(d, &ret) + return ret +} + +func UnstructuredToPolicyStatus(policyStatus map[string]any) gwapiv1.PolicyStatus { + var ret gwapiv1.PolicyStatus + // No need to check the json marshal/unmarshal error, the policyStatus was + // created via a typed object so the marshalling/unmarshalling will always + // work + d, _ := json.Marshal(policyStatus) + _ = json.Unmarshal(d, &ret) + return ret +} + func (t *Translator) translateExtServerPolicyForGateway( policy *unstructured.Unstructured, gateway *GatewayContext, @@ -173,3 +183,29 @@ func (t *Translator) translateExtServerPolicyForGateway( } return found } + +// Appends status ancestors from newPolicy into aggregatedPolicy's list of ancestors. +func MergeAncestorsForExtensionServerPolicies(aggregatedPolicy, newPolicy *unstructured.Unstructured) { + aggStatusObj := aggregatedPolicy.Object["status"] + var aggStatus gwapiv1.PolicyStatus + if _, ok := aggStatusObj.(map[string]any); ok { + aggStatus = UnstructuredToPolicyStatus(aggStatusObj.(map[string]any)) + } else if _, ok := aggStatusObj.(gwapiv1.PolicyStatus); ok { + aggStatus = aggStatusObj.(gwapiv1.PolicyStatus) + } else { + aggStatus = gwapiv1.PolicyStatus{} + } + + newStatusObj := newPolicy.Object["status"] + var newStatus gwapiv1.PolicyStatus + if _, ok := newStatusObj.(map[string]any); ok { + newStatus = UnstructuredToPolicyStatus(newStatusObj.(map[string]any)) + } else if _, ok := newStatusObj.(gwapiv1.PolicyStatus); ok { + newStatus = newStatusObj.(gwapiv1.PolicyStatus) + } else { + newStatus = gwapiv1.PolicyStatus{} + } + + aggStatus.Ancestors = append(aggStatus.Ancestors, newStatus.Ancestors...) + aggregatedPolicy.Object["status"] = PolicyStatusToUnstructured(aggStatus) +} diff --git a/internal/gatewayapi/extensionserverpolicy_test.go b/internal/gatewayapi/extensionserverpolicy_test.go index fbfc8418d6f..1d2457a1833 100644 --- a/internal/gatewayapi/extensionserverpolicy_test.go +++ b/internal/gatewayapi/extensionserverpolicy_test.go @@ -123,3 +123,115 @@ func TestExtractTargetRefs(t *testing.T) { }) } } + +func TestMergeAncestorsForExtensionServerPolicies(t *testing.T) { + tests := []struct { + aggStatus *gwapiv1.PolicyStatus + newStatus *gwapiv1.PolicyStatus + noStatus bool + }{ + { + aggStatus: &gwapiv1.PolicyStatus{ + Ancestors: []gwapiv1.PolicyAncestorStatus{ + { + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway-1", + }, + }, + }, + }, + newStatus: &gwapiv1.PolicyStatus{ + Ancestors: []gwapiv1.PolicyAncestorStatus{ + { + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway-2", + }, + }, + }, + }, + }, + { + aggStatus: &gwapiv1.PolicyStatus{}, + newStatus: &gwapiv1.PolicyStatus{ + Ancestors: []gwapiv1.PolicyAncestorStatus{ + { + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway-2", + }, + }, + }, + }, + }, + { + aggStatus: &gwapiv1.PolicyStatus{ + Ancestors: []gwapiv1.PolicyAncestorStatus{ + { + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway-1", + }, + }, + }, + }, + newStatus: &gwapiv1.PolicyStatus{}, + }, + { + aggStatus: &gwapiv1.PolicyStatus{}, + newStatus: &gwapiv1.PolicyStatus{}, + }, + { + aggStatus: nil, + newStatus: &gwapiv1.PolicyStatus{ + Ancestors: []gwapiv1.PolicyAncestorStatus{ + { + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway-1", + }, + }, + }, + }, + }, + { + aggStatus: &gwapiv1.PolicyStatus{ + Ancestors: []gwapiv1.PolicyAncestorStatus{ + { + AncestorRef: gwapiv1.ParentReference{ + Name: "gateway-1", + }, + }, + }, + }, + newStatus: nil, + }, + { + aggStatus: nil, + newStatus: nil, + }, + } + + for _, test := range tests { + aggPolicy := unstructured.Unstructured{Object: make(map[string]interface{})} + newPolicy := unstructured.Unstructured{Object: make(map[string]interface{})} + desiredMergedStatus := gwapiv1.PolicyStatus{} + + // aggStatus == nil, means simulate not setting status at all within the policy. + if test.aggStatus != nil { + aggPolicy.Object["status"] = PolicyStatusToUnstructured(*test.aggStatus) + desiredMergedStatus.Ancestors = append(desiredMergedStatus.Ancestors, test.aggStatus.Ancestors...) + } + + // newStatus == nil, means simulate not setting status at all within the policy. + if test.newStatus != nil { + newPolicy.Object["status"] = PolicyStatusToUnstructured(*test.newStatus) + desiredMergedStatus.Ancestors = append(desiredMergedStatus.Ancestors, test.newStatus.Ancestors...) + } + + MergeAncestorsForExtensionServerPolicies(&aggPolicy, &newPolicy) + + // The product object will always have an existing `status`, even if with 0 ancestors. + newAggPolicy := UnstructuredToPolicyStatus(aggPolicy.Object["status"].(map[string]any)) + require.Len(t, newAggPolicy.Ancestors, len(desiredMergedStatus.Ancestors)) + for i := range newAggPolicy.Ancestors { + require.Equal(t, desiredMergedStatus.Ancestors[i].AncestorRef.Name, newAggPolicy.Ancestors[i].AncestorRef.Name) + } + } +} diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go index 61ab213ccef..aa9da040a60 100644 --- a/internal/gatewayapi/runner/runner.go +++ b/internal/gatewayapi/runner/runner.go @@ -8,7 +8,6 @@ package runner import ( "context" "crypto/tls" - "encoding/json" "fmt" "os" "path" @@ -407,37 +406,10 @@ func (r *Runner) subscribeAndTranslate(sub <-chan watchable.Snapshot[string, *re break } } - // If already found (because processed in an earlier gatewayclass), append its status ancestors. // Otherwise, append it to the list of *Policies in the accumulated result. if aggregatedPolicy != nil { - ancestorsObject, found, err := unstructured.NestedFieldCopy(aggregatedPolicy.Object, "status", "ancestors") - if !found || err != nil { - r.Logger.Error(err, "failed to get existing ancestors", "policyName", aggregatedPolicy.GetName(), "ancestorsFound", found) - continue - } - ancestors, ok := ancestorsObject.([]gwapiv1.PolicyAncestorStatus) - if !ok { - r.Logger.Error(err, "failed to assert existing ancestors type", "policyName", aggregatedPolicy.GetName()) - continue - } - - newAncestorsObject, found, err := unstructured.NestedFieldCopy(extServerPolicy.Object, "status", "ancestors") - if !found || err != nil { - r.Logger.Error(err, "failed to get new ancestors", "policyName", aggregatedPolicy.GetName(), "ancestorsFound", found) - continue - } - newAncestors, ok := newAncestorsObject.([]gwapiv1.PolicyAncestorStatus) - if !ok { - r.Logger.Error(err, "failed to assert new ancestors type", "policyName", aggregatedPolicy.GetName()) - continue - } - - ancestors = append(ancestors, newAncestors...) - err = unstructured.SetNestedField(aggregatedPolicy.Object, ancestors, "status", "ancestors") - if err != nil { - r.Logger.Error(err, "failed to update ancestors of policy", "policyName", aggregatedPolicy.GetName()) - } + gatewayapi.MergeAncestorsForExtensionServerPolicies(aggregatedPolicy, &extServerPolicy) } else { aggregatedPolicy = &extServerPolicy aggregatedResult.ExtensionServerPolicies = append(aggregatedResult.ExtensionServerPolicies, *aggregatedPolicy) @@ -538,7 +510,7 @@ func (r *Runner) subscribeAndTranslate(sub <-chan watchable.Snapshot[string, *re } if statusObj, hasStatus := extServerPolicy.Object["status"]; hasStatus && statusObj != nil { if statusMap, ok := statusObj.(map[string]any); ok && len(statusMap) > 0 { - policyStatus := unstructuredToPolicyStatus(statusMap) + policyStatus := gatewayapi.UnstructuredToPolicyStatus(statusMap) r.ProviderResources.ExtensionPolicyStatuses.Store(key, &policyStatus) extensionServerPolicyStatusCount++ } @@ -622,16 +594,6 @@ func (r *Runner) loadTLSConfig(ctx context.Context) (*tls.Config, []byte, error) } } -func unstructuredToPolicyStatus(policyStatus map[string]any) gwapiv1.PolicyStatus { - var ret gwapiv1.PolicyStatus - // No need to check the json marshal/unmarshal error, the policyStatus was - // created via a typed object so the marshalling/unmarshalling will always - // work - d, _ := json.Marshal(policyStatus) - _ = json.Unmarshal(d, &ret) - return ret -} - // deleteAllIRKeys deletes all XdsIR and InfraIR using tracked keys func (r *Runner) deleteAllKeys() { // Delete IR keys From 4e4e84671ac93da6b17d1105225c5c377e35e339 Mon Sep 17 00:00:00 2001 From: y-rabie Date: Tue, 4 Nov 2025 20:08:35 +0200 Subject: [PATCH 4/4] Adjust and add e2e tests for status cleanup Signed-off-by: y-rabie --- ...anup-multiple-ancestors-multiple-gwcs.yaml | 14 ++ ...s-cleanup-multiple-ancestors-same-gwc.yaml | 13 ++ .../policy-status-cleanup-no-ancestor.yaml | 10 ++ ...policy-status-cleanup-single-ancestor.yaml | 10 ++ ...leanup-multiple-parents-multiple-gwcs.yaml | 31 ++++ ...tus-cleanup-multiple-parents-same-gwc.yaml | 45 +++++ .../route-status-cleanup-single-parent.yaml | 13 ++ .../status-cleanup-gateway-different-gwc.yaml | 34 ++++ test/e2e/tests/policy_status_cleanup.go | 169 ++++++++++++++++++ test/e2e/tests/route_status_cleanup.go | 105 +++++++++++ test/e2e/tests/tcproute.go | 72 ++++---- .../tests/tcproute_authorization_client_ip.go | 6 +- test/e2e/tests/tcproute_with_backend.go | 6 +- test/e2e/tests/udproute.go | 8 +- test/e2e/tests/utils.go | 55 ++++++ 15 files changed, 549 insertions(+), 42 deletions(-) create mode 100644 test/e2e/testdata/policy-status-cleanup-multiple-ancestors-multiple-gwcs.yaml create mode 100644 test/e2e/testdata/policy-status-cleanup-multiple-ancestors-same-gwc.yaml create mode 100644 test/e2e/testdata/policy-status-cleanup-no-ancestor.yaml create mode 100644 test/e2e/testdata/policy-status-cleanup-single-ancestor.yaml create mode 100644 test/e2e/testdata/route-status-cleanup-multiple-parents-multiple-gwcs.yaml create mode 100644 test/e2e/testdata/route-status-cleanup-multiple-parents-same-gwc.yaml create mode 100644 test/e2e/testdata/route-status-cleanup-single-parent.yaml create mode 100644 test/e2e/testdata/status-cleanup-gateway-different-gwc.yaml create mode 100644 test/e2e/tests/policy_status_cleanup.go create mode 100644 test/e2e/tests/route_status_cleanup.go diff --git a/test/e2e/testdata/policy-status-cleanup-multiple-ancestors-multiple-gwcs.yaml b/test/e2e/testdata/policy-status-cleanup-multiple-ancestors-multiple-gwcs.yaml new file mode 100644 index 00000000000..955393ae0de --- /dev/null +++ b/test/e2e/testdata/policy-status-cleanup-multiple-ancestors-multiple-gwcs.yaml @@ -0,0 +1,14 @@ +apiVersion: gateway.envoyproxy.io/v1alpha1 +kind: BackendTrafficPolicy +metadata: + name: backendtrafficpolicy-multiple-ancestors-same-gwc + namespace: gateway-conformance-infra +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: same-namespace + # This gateway (and its gatewayclass) is created through `status-cleanup-gateway-different-gwc.yaml` manifest. + - group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 diff --git a/test/e2e/testdata/policy-status-cleanup-multiple-ancestors-same-gwc.yaml b/test/e2e/testdata/policy-status-cleanup-multiple-ancestors-same-gwc.yaml new file mode 100644 index 00000000000..b5f82c16af1 --- /dev/null +++ b/test/e2e/testdata/policy-status-cleanup-multiple-ancestors-same-gwc.yaml @@ -0,0 +1,13 @@ +apiVersion: gateway.envoyproxy.io/v1alpha1 +kind: BackendTrafficPolicy +metadata: + name: backendtrafficpolicy-multiple-ancestors-same-gwc + namespace: gateway-conformance-infra +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: same-namespace + - group: gateway.networking.k8s.io + kind: Gateway + name: all-namespaces diff --git a/test/e2e/testdata/policy-status-cleanup-no-ancestor.yaml b/test/e2e/testdata/policy-status-cleanup-no-ancestor.yaml new file mode 100644 index 00000000000..0ecf36b2e54 --- /dev/null +++ b/test/e2e/testdata/policy-status-cleanup-no-ancestor.yaml @@ -0,0 +1,10 @@ +apiVersion: gateway.envoyproxy.io/v1alpha1 +kind: BackendTrafficPolicy +metadata: + name: backendtrafficpolicy-multiple-ancestors-same-gwc + namespace: gateway-conformance-infra +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: other-controller-ancestor diff --git a/test/e2e/testdata/policy-status-cleanup-single-ancestor.yaml b/test/e2e/testdata/policy-status-cleanup-single-ancestor.yaml new file mode 100644 index 00000000000..d2f8de24433 --- /dev/null +++ b/test/e2e/testdata/policy-status-cleanup-single-ancestor.yaml @@ -0,0 +1,10 @@ +apiVersion: gateway.envoyproxy.io/v1alpha1 +kind: BackendTrafficPolicy +metadata: + name: backendtrafficpolicy-multiple-ancestors-same-gwc + namespace: gateway-conformance-infra +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: same-namespace diff --git a/test/e2e/testdata/route-status-cleanup-multiple-parents-multiple-gwcs.yaml b/test/e2e/testdata/route-status-cleanup-multiple-parents-multiple-gwcs.yaml new file mode 100644 index 00000000000..76e3097ae2f --- /dev/null +++ b/test/e2e/testdata/route-status-cleanup-multiple-parents-multiple-gwcs.yaml @@ -0,0 +1,31 @@ +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: Gateway +metadata: + name: gateway-1 + namespace: gateway-conformance-infra +spec: + gatewayClassName: "{GATEWAY_CLASS_NAME}" + listeners: + - name: foo + protocol: TCP + port: 8080 + allowedRoutes: + kinds: + - kind: TCPRoute +--- +apiVersion: gateway.networking.k8s.io/v1alpha2 +kind: TCPRoute +metadata: + name: tcp-route-status-cleanup + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: gateway-1 + sectionName: foo + # This gateway (and its gatewayclass) is created through `status-cleanup-gateway-different-gwc.yaml` manifest. + - name: gateway-2 + sectionName: foo + rules: + - backendRefs: + - name: infra-backend-v1 + port: 8080 diff --git a/test/e2e/testdata/route-status-cleanup-multiple-parents-same-gwc.yaml b/test/e2e/testdata/route-status-cleanup-multiple-parents-same-gwc.yaml new file mode 100644 index 00000000000..b9585646af9 --- /dev/null +++ b/test/e2e/testdata/route-status-cleanup-multiple-parents-same-gwc.yaml @@ -0,0 +1,45 @@ +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: Gateway +metadata: + name: gateway-1 + namespace: gateway-conformance-infra +spec: + gatewayClassName: "{GATEWAY_CLASS_NAME}" + listeners: + - name: foo + protocol: TCP + port: 8080 + allowedRoutes: + kinds: + - kind: TCPRoute +--- +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: Gateway +metadata: + name: gateway-2 + namespace: gateway-conformance-infra +spec: + gatewayClassName: "{GATEWAY_CLASS_NAME}" + listeners: + - name: foo + protocol: TCP + port: 8080 + allowedRoutes: + kinds: + - kind: TCPRoute +--- +apiVersion: gateway.networking.k8s.io/v1alpha2 +kind: TCPRoute +metadata: + name: tcp-route-status-cleanup + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: gateway-1 + sectionName: foo + - name: gateway-2 + sectionName: foo + rules: + - backendRefs: + - name: infra-backend-v1 + port: 8080 diff --git a/test/e2e/testdata/route-status-cleanup-single-parent.yaml b/test/e2e/testdata/route-status-cleanup-single-parent.yaml new file mode 100644 index 00000000000..e584f591f38 --- /dev/null +++ b/test/e2e/testdata/route-status-cleanup-single-parent.yaml @@ -0,0 +1,13 @@ +apiVersion: gateway.networking.k8s.io/v1alpha2 +kind: TCPRoute +metadata: + name: tcp-route-status-cleanup + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: gateway-1 + sectionName: foo + rules: + - backendRefs: + - name: infra-backend-v1 + port: 8080 diff --git a/test/e2e/testdata/status-cleanup-gateway-different-gwc.yaml b/test/e2e/testdata/status-cleanup-gateway-different-gwc.yaml new file mode 100644 index 00000000000..03659367ffc --- /dev/null +++ b/test/e2e/testdata/status-cleanup-gateway-different-gwc.yaml @@ -0,0 +1,34 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: GatewayClass +metadata: + name: status-cleanup +spec: + controllerName: gateway.envoyproxy.io/gatewayclass-controller +--- +apiVersion: gateway.networking.k8s.io/v1beta1 +kind: Gateway +metadata: + name: gateway-2 + namespace: gateway-conformance-infra +spec: + gatewayClassName: status-cleanup + listeners: + - name: foo + protocol: TCP + port: 8080 + allowedRoutes: + kinds: + - kind: TCPRoute + infrastructure: + parametersRef: + group: gateway.envoyproxy.io + kind: EnvoyProxy + name: status-cleanup +--- +apiVersion: gateway.envoyproxy.io/v1alpha1 +kind: EnvoyProxy +metadata: + name: status-cleanup + namespace: gateway-conformance-infra +spec: + ipFamily: IPv4 diff --git a/test/e2e/tests/policy_status_cleanup.go b/test/e2e/tests/policy_status_cleanup.go new file mode 100644 index 00000000000..8feab945b6e --- /dev/null +++ b/test/e2e/tests/policy_status_cleanup.go @@ -0,0 +1,169 @@ +// Copyright Envoy Gateway Authors +// SPDX-License-Identifier: Apache-2.0 +// The full text of the Apache license is available in the LICENSE file at +// the root of the repo. + +//go:build e2e + +package tests + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/conformance/utils/suite" + + egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" + "github.com/envoyproxy/gateway/internal/gatewayapi" + "github.com/envoyproxy/gateway/internal/gatewayapi/resource" +) + +func init() { + ConformanceTests = append(ConformanceTests, PolicyStatusCleanupSameGatewayClass, PolicyStatusCleanupMultipleGatewayClasses) +} + +var PolicyStatusCleanupSameGatewayClass = suite.ConformanceTest{ + ShortName: "PolicyStatusCleanupSameGatewayClass", + Description: "Testing Policy Status Cleanup With Ancestors of The Same GatewayClass", + Manifests: []string{"testdata/policy-status-cleanup-multiple-ancestors-same-gwc.yaml"}, + Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { + t.Run("PolicyStatusCleanup", func(t *testing.T) { + ns := "gateway-conformance-infra" + gw1NN, gw2NN := types.NamespacedName{Name: "same-namespace", Namespace: ns}, types.NamespacedName{Name: "all-namespaces", Namespace: ns} + + policyNamespacedName := types.NamespacedName{Name: "backendtrafficpolicy-multiple-ancestors-same-gwc", Namespace: ns} + + // Check the policy has two ancestors in its status. + BackendTrafficPolicyMustBeAcceptedByAllAncestors(t, + suite.Client, + policyNamespacedName, + suite.ControllerName, + gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Namespace: gatewayapi.NamespacePtr(gw1NN.Namespace), + Name: gwapiv1.ObjectName(gw1NN.Name), + }, + gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Namespace: gatewayapi.NamespacePtr(gw2NN.Namespace), + Name: gwapiv1.ObjectName(gw2NN.Name), + }, + ) + + // Change the policy to have a single ancestor, and check its status. + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/policy-status-cleanup-single-ancestor.yaml", false) + BackendTrafficPolicyMustBeAcceptedByAllAncestors(t, + suite.Client, + policyNamespacedName, + suite.ControllerName, + gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Namespace: gatewayapi.NamespacePtr(gw1NN.Namespace), + Name: gwapiv1.ObjectName(gw1NN.Name), + }, + ) + + // Update the policy status to include a status ancestor of another controller. + policy := &egv1a1.BackendTrafficPolicy{} + err := suite.Client.Get(context.Background(), policyNamespacedName, policy) + require.NoErrorf(t, err, "error getting BackendTrafficPolicy %s", policyNamespacedName.String()) + otherControllerAncestor := gwapiv1.PolicyAncestorStatus{ + AncestorRef: gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Name: "other-controller-ancestor", + Namespace: gatewayapi.NamespacePtr(policy.Namespace), + }, + ControllerName: "gateway.envoyproxy.io/other-gatewayclass-controller", + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + Reason: string(gwapiv1.PolicyConditionAccepted), + }, + }, + } + policy.Status.Ancestors = append(policy.Status.Ancestors, otherControllerAncestor) + err = suite.Client.Status().Update(context.Background(), policy) + require.NoErrorf(t, err, "error updating BackendTrafficPolicy status %s", policyNamespacedName.String()) + + // Change the policy spec to have a corresponding ancestor and trigger reconciliation. + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/policy-status-cleanup-no-ancestor.yaml", false) + + // Check its status to only have the ancestor from the other controller. + BackendTrafficPolicyMustBeAcceptedByAllAncestors(t, + suite.Client, + policyNamespacedName, + "gateway.envoyproxy.io/other-gatewayclass-controller", + []gwapiv1.ParentReference{ + { + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Name: "other-controller-ancestor", + Namespace: gatewayapi.NamespacePtr(policy.Namespace), + }, + }..., + ) + }) + }, +} + +var PolicyStatusCleanupMultipleGatewayClasses = suite.ConformanceTest{ + ShortName: "PolicyStatusCleanupMultipleGatewayClasses", + Description: "Testing Policy Status Cleanup With Ancestors of Multiple GatewayClasses", + Manifests: []string{"testdata/policy-status-cleanup-multiple-ancestors-multiple-gwcs.yaml"}, + Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { + t.Run("PolicyStatusCleanup", func(t *testing.T) { + // Create the second gateway of a different gatewayclass, which the backendtrafficpolicy is already attached to. + prevGwc := suite.Applier.GatewayClass + suite.Applier.GatewayClass = "status-cleanup" + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/status-cleanup-gateway-different-gwc.yaml", true) + suite.Applier.GatewayClass = prevGwc + + ns := "gateway-conformance-infra" + gw1NN, gw2NN := types.NamespacedName{Name: "same-namespace", Namespace: ns}, types.NamespacedName{Name: "gateway-2", Namespace: ns} + + // Check the policy has two ancestors in its status. + BackendTrafficPolicyMustBeAcceptedByAllAncestors(t, + suite.Client, + types.NamespacedName{Name: "backendtrafficpolicy-multiple-ancestors-same-gwc", Namespace: ns}, + suite.ControllerName, + gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Namespace: gatewayapi.NamespacePtr(gw1NN.Namespace), + Name: gwapiv1.ObjectName(gw1NN.Name), + }, + gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Namespace: gatewayapi.NamespacePtr(gw2NN.Namespace), + Name: gwapiv1.ObjectName(gw2NN.Name), + }, + ) + + // Change the policy to have a single ancestor, and check its status. + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/policy-status-cleanup-single-ancestor.yaml", false) + BackendTrafficPolicyMustBeAcceptedByAllAncestors(t, + suite.Client, + types.NamespacedName{Name: "backendtrafficpolicy-multiple-ancestors-same-gwc", Namespace: ns}, + suite.ControllerName, + gwapiv1.ParentReference{ + Group: gatewayapi.GroupPtr(gwapiv1.GroupName), + Kind: gatewayapi.KindPtr(resource.KindGateway), + Namespace: gatewayapi.NamespacePtr(gw1NN.Namespace), + Name: gwapiv1.ObjectName(gw1NN.Name), + }, + ) + }) + }, +} diff --git a/test/e2e/tests/route_status_cleanup.go b/test/e2e/tests/route_status_cleanup.go new file mode 100644 index 00000000000..3e6b10f075b --- /dev/null +++ b/test/e2e/tests/route_status_cleanup.go @@ -0,0 +1,105 @@ +// Copyright Envoy Gateway Authors +// SPDX-License-Identifier: Apache-2.0 +// The full text of the Apache license is available in the LICENSE file at +// the root of the repo. + +// This file contains code derived from upstream gateway-api, it will be moved to upstream. + +//go:build e2e + +package tests + +import ( + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/gateway-api/conformance/utils/http" + "sigs.k8s.io/gateway-api/conformance/utils/suite" +) + +func init() { + ConformanceTests = append(ConformanceTests, RouteStatusCleanupSameGatewayClass, RouteStatusCleanupMultipleGatewayClasses) +} + +var RouteStatusCleanupSameGatewayClass = suite.ConformanceTest{ + ShortName: "RouteStatusCleanupSameGatewayClass", + Description: "Testing Route Status Cleanup With Parents of The Same GatewayClass", + Manifests: []string{"testdata/route-status-cleanup-multiple-parents-same-gwc.yaml"}, + Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { + t.Run("RouteStatusCleanup", func(t *testing.T) { + ns := "gateway-conformance-infra" + routeNN := types.NamespacedName{Name: "tcp-route-status-cleanup", Namespace: ns} + gw1NN, gw2NN := types.NamespacedName{Name: "gateway-1", Namespace: ns}, types.NamespacedName{Name: "gateway-2", Namespace: ns} + gwRefs := []GatewayRef{NewGatewayRef(gw1NN), NewGatewayRef(gw2NN)} + gwAddrs := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, gwRefs, routeNN) + OkResp := http.ExpectedResponse{ + Request: http.Request{ + Path: "/", + }, + Response: http.Response{ + StatusCodes: []int{200}, + }, + Namespace: ns, + } + + // Send a request to an valid path and expect a successful response + require.Len(t, gwAddrs, 2) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[1], OkResp) + + // Change the route to have a single parent, and check its status + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/route-status-cleanup-single-parent.yaml", false) + gwRefs = []GatewayRef{NewGatewayRef(gw1NN)} + gwAddrs = GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, gwRefs, routeNN) + + // Send a request to an valid path and expect a successful response + require.Len(t, gwAddrs, 1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) + }) + }, +} + +var RouteStatusCleanupMultipleGatewayClasses = suite.ConformanceTest{ + ShortName: "RouteStatusCleanupMultipleGatewayClasses", + Description: "Testing Route Status Cleanup With Parents of Multiple GatewayClasses", + Manifests: []string{"testdata/route-status-cleanup-multiple-parents-multiple-gwcs.yaml"}, + Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { + t.Run("RouteStatusCleanup", func(t *testing.T) { + // Create the second gateway of a different gatewayclass, which the route is already attached to. + prevGwc := suite.Applier.GatewayClass + suite.Applier.GatewayClass = "status-cleanup" + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/status-cleanup-gateway-different-gwc.yaml", true) + suite.Applier.GatewayClass = prevGwc + + ns := "gateway-conformance-infra" + routeNN := types.NamespacedName{Name: "tcp-route-status-cleanup", Namespace: ns} + gw1NN, gw2NN := types.NamespacedName{Name: "gateway-1", Namespace: ns}, types.NamespacedName{Name: "gateway-2", Namespace: ns} + gwRefs := []GatewayRef{NewGatewayRef(gw1NN), NewGatewayRef(gw2NN)} + gwAddrs := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, gwRefs, routeNN) + OkResp := http.ExpectedResponse{ + Request: http.Request{ + Path: "/", + }, + Response: http.Response{ + StatusCodes: []int{200}, + }, + Namespace: ns, + } + + // Send a request to an valid path and expect a successful response + require.Len(t, gwAddrs, 2) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[1], OkResp) + + // Change the route to have a single parent, and check its status + suite.Applier.MustApplyWithCleanup(t, suite.Client, suite.TimeoutConfig, "testdata/route-status-cleanup-single-parent.yaml", false) + gwRefs = []GatewayRef{NewGatewayRef(gw1NN)} + gwAddrs = GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, gwRefs, routeNN) + + // Send a request to an valid path and expect a successful response + require.Len(t, gwAddrs, 1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) + }) + }, +} diff --git a/test/e2e/tests/tcproute.go b/test/e2e/tests/tcproute.go index 877663ceac3..fe57fa8dc5d 100644 --- a/test/e2e/tests/tcproute.go +++ b/test/e2e/tests/tcproute.go @@ -44,7 +44,8 @@ var TCPRouteTest = suite.ConformanceTest{ ns := "gateway-conformance-infra" routeNN := types.NamespacedName{Name: "tcp-app-1", Namespace: ns} gwNN := types.NamespacedName{Name: "my-tcp-gateway", Namespace: ns} - gwAddr := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, NewGatewayRef(gwNN), routeNN) + gwRefs := []GatewayRef{NewGatewayRef(gwNN)} + gwAddrs := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, gwRefs, routeNN) OkResp := http.ExpectedResponse{ Request: http.Request{ Path: "/", @@ -56,13 +57,15 @@ var TCPRouteTest = suite.ConformanceTest{ } // Send a request to an valid path and expect a successful response - http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, OkResp) + require.Len(t, gwAddrs, 1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) }) t.Run("tcp-route-2", func(t *testing.T) { ns := "gateway-conformance-infra" routeNN := types.NamespacedName{Name: "tcp-app-2", Namespace: ns} gwNN := types.NamespacedName{Name: "my-tcp-gateway", Namespace: ns} - gwAddr := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, NewGatewayRef(gwNN), routeNN) + gwRefs := []GatewayRef{NewGatewayRef(gwNN)} + gwAddrs := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, gwRefs, routeNN) OkResp := http.ExpectedResponse{ Request: http.Request{ Path: "/", @@ -74,12 +77,13 @@ var TCPRouteTest = suite.ConformanceTest{ } // Send a request to an valid path and expect a successful response - http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, OkResp) + require.Len(t, gwAddrs, 1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) }) }, } -func GatewayAndTCPRoutesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig *config.TimeoutConfig, controllerName string, gw GatewayRef, routeNNs ...types.NamespacedName) string { +func GatewayAndTCPRoutesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig *config.TimeoutConfig, controllerName string, gws []GatewayRef, routeNNs ...types.NamespacedName) []string { t.Helper() if timeoutConfig == nil { @@ -92,40 +96,42 @@ func GatewayAndTCPRoutesMustBeAccepted(t *testing.T, c client.Client, timeoutCon tlog.Logf(t, "error fetching TCPRoute: %v", err) } - gwAddr, err := WaitForGatewayAddress(t, c, timeoutConfig, gw.NamespacedName, string(*tcpRoute.Spec.ParentRefs[0].SectionName)) - require.NoErrorf(t, err, "timed out waiting for Gateway address to be assigned") - - ns := gwapiv1.Namespace(gw.Namespace) - kind := gwapiv1.Kind("Gateway") + gwAddrs := make([]string, 0, len(gws)) + for _, gw := range gws { + gwAddr, err := WaitForGatewayAddress(t, c, timeoutConfig, gw.NamespacedName, string(*tcpRoute.Spec.ParentRefs[0].SectionName)) + require.NoErrorf(t, err, "timed out waiting for Gateway address to be assigned") + gwAddrs = append(gwAddrs, gwAddr) + } for _, routeNN := range routeNNs { - namespaceRequired := true - if routeNN.Namespace == gw.Namespace { - namespaceRequired = false - } - var parents []gwapiv1.RouteParentStatus - for _, listener := range gw.listenerNames { - parents = append(parents, gwapiv1.RouteParentStatus{ - ParentRef: gwapiv1.ParentReference{ - Group: (*gwapiv1.Group)(&gwapiv1.GroupVersion.Group), - Kind: &kind, - Name: gwapiv1.ObjectName(gw.Name), - Namespace: &ns, - SectionName: listener, - }, - ControllerName: gwapiv1.GatewayController(controllerName), - Conditions: []metav1.Condition{{ - Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: string(gwapiv1.RouteReasonAccepted), - }}, - }) + var namespaceRequired []bool + for _, gw := range gws { + ns := gwapiv1.Namespace(gw.Namespace) + kind := gwapiv1.Kind("Gateway") + namespaceRequired = append(namespaceRequired, routeNN.Namespace != gw.Namespace) + for _, listener := range gw.listenerNames { + parents = append(parents, gwapiv1.RouteParentStatus{ + ParentRef: gwapiv1.ParentReference{ + Group: (*gwapiv1.Group)(&gwapiv1.GroupVersion.Group), + Kind: &kind, + Name: gwapiv1.ObjectName(gw.Name), + Namespace: &ns, + SectionName: listener, + }, + ControllerName: gwapiv1.GatewayController(controllerName), + Conditions: []metav1.Condition{{ + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gwapiv1.RouteReasonAccepted), + }}, + }) + } } TCPRouteMustHaveParents(t, c, timeoutConfig, routeNN, parents, namespaceRequired) } - return gwAddr + return gwAddrs } // WaitForGatewayAddress waits until at least one IP Address has been set in the @@ -174,7 +180,7 @@ func WaitForGatewayAddress(t *testing.T, client client.Client, timeoutConfig *co return net.JoinHostPort(ipAddr, port), waitErr } -func TCPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig *config.TimeoutConfig, routeName types.NamespacedName, parents []gwapiv1.RouteParentStatus, namespaceRequired bool) { +func TCPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig *config.TimeoutConfig, routeName types.NamespacedName, parents []gwapiv1.RouteParentStatus, namespaceRequired []bool) { t.Helper() if timeoutConfig == nil { diff --git a/test/e2e/tests/tcproute_authorization_client_ip.go b/test/e2e/tests/tcproute_authorization_client_ip.go index a1a21d5af9a..b25d08a0e3c 100644 --- a/test/e2e/tests/tcproute_authorization_client_ip.go +++ b/test/e2e/tests/tcproute_authorization_client_ip.go @@ -36,7 +36,7 @@ var TCPRouteAuthzWithClientIP = suite.ConformanceTest{ tcpRouteNN := types.NamespacedName{Name: "tcp-backend-authorization-ip", Namespace: ns} tcpRouteFqdnNN := types.NamespacedName{Name: "tcp-backend-authorization-fqdn", Namespace: ns} gwNN := types.NamespacedName{Name: "tcp-authorization-backend", Namespace: ns} - GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, NewGatewayRef(gwNN), tcpRouteNN, tcpRouteFqdnNN) + GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, []GatewayRef{NewGatewayRef(gwNN)}, tcpRouteNN, tcpRouteFqdnNN) // Test the blocked route (ip section) ipSection := gwapiv1.SectionName("ip") @@ -74,10 +74,10 @@ func testTCPRouteWithBackendBlocked(t *testing.T, suite *suite.ConformanceTestSu ns := "gateway-conformance-infra" routeNN := types.NamespacedName{Name: routeName, Namespace: ns} gwNN := types.NamespacedName{Name: gwName, Namespace: ns} - gwAddr := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, NewGatewayRef(gwNN), routeNN) + gwAddr := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, []GatewayRef{NewGatewayRef(gwNN)}, routeNN) BackendMustBeAccepted(t, suite.Client, types.NamespacedName{Name: backendName, Namespace: ns}) - testTCPConnectionBlocked(t, gwAddr) + testTCPConnectionBlocked(t, gwAddr[0]) } func testTCPConnectionBlocked(t *testing.T, gwAddr string) { diff --git a/test/e2e/tests/tcproute_with_backend.go b/test/e2e/tests/tcproute_with_backend.go index f7f68db6b2b..ebd0c759be9 100644 --- a/test/e2e/tests/tcproute_with_backend.go +++ b/test/e2e/tests/tcproute_with_backend.go @@ -12,6 +12,7 @@ package tests import ( "testing" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/conformance/utils/http" "sigs.k8s.io/gateway-api/conformance/utils/suite" @@ -61,7 +62,7 @@ func testTCPRouteWithBackend(t *testing.T, suite *suite.ConformanceTestSuite, gw ns := "gateway-conformance-infra" routeNN := types.NamespacedName{Name: routeName, Namespace: ns} gwNN := types.NamespacedName{Name: gwName, Namespace: ns} - gwAddr := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, NewGatewayRef(gwNN), routeNN) + gwAddrs := GatewayAndTCPRoutesMustBeAccepted(t, suite.Client, &suite.TimeoutConfig, suite.ControllerName, []GatewayRef{NewGatewayRef(gwNN)}, routeNN) BackendMustBeAccepted(t, suite.Client, types.NamespacedName{Name: backendName, Namespace: ns}) OkResp := http.ExpectedResponse{ Request: http.Request{ @@ -74,5 +75,6 @@ func testTCPRouteWithBackend(t *testing.T, suite *suite.ConformanceTestSuite, gw } // Send a request to a valid path and expect a successful response - http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, OkResp) + require.Len(t, gwAddrs, 1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddrs[0], OkResp) } diff --git a/test/e2e/tests/udproute.go b/test/e2e/tests/udproute.go index 6174917d5a2..32bfd802000 100644 --- a/test/e2e/tests/udproute.go +++ b/test/e2e/tests/udproute.go @@ -136,13 +136,13 @@ func GatewayAndUDPRoutesMustBeAccepted(t *testing.T, c client.Client, timeoutCon }, }) } - UDPRouteMustHaveParents(t, c, timeoutConfig, routeNN, parents, namespaceRequired) + UDPRouteMustHaveParents(t, c, timeoutConfig, routeNN, parents, []bool{namespaceRequired}) } return gwAddr } -func UDPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig *config.TimeoutConfig, routeName types.NamespacedName, parents []gwapiv1.RouteParentStatus, namespaceRequired bool) { +func UDPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig *config.TimeoutConfig, routeName types.NamespacedName, parents []gwapiv1.RouteParentStatus, namespaceRequired []bool) { t.Helper() if timeoutConfig == nil { @@ -163,7 +163,7 @@ func UDPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig * require.NoErrorf(t, waitErr, "error waiting for UDPRoute to have parents matching expectations") } -func parentsForRouteMatch(t *testing.T, routeName types.NamespacedName, expected, actual []gwapiv1.RouteParentStatus, namespaceRequired bool) bool { +func parentsForRouteMatch(t *testing.T, routeName types.NamespacedName, expected, actual []gwapiv1.RouteParentStatus, namespaceRequired []bool) bool { t.Helper() if len(expected) != len(actual) { @@ -190,7 +190,7 @@ func parentsForRouteMatch(t *testing.T, routeName types.NamespacedName, expected return false } if !reflect.DeepEqual(actualParent.ParentRef.Namespace, expectedParent.ParentRef.Namespace) { - if namespaceRequired || actualParent.ParentRef.Namespace != nil { + if namespaceRequired[i] || actualParent.ParentRef.Namespace != nil { tlog.Logf(t, "Route %s/%s expected ParentReference.Namespace to be %v, got %v", routeName.Namespace, routeName.Name, expectedParent.ParentRef.Namespace, actualParent.ParentRef.Namespace) return false } diff --git a/test/e2e/tests/utils.go b/test/e2e/tests/utils.go index f0b8dc4fdfc..d160700134e 100644 --- a/test/e2e/tests/utils.go +++ b/test/e2e/tests/utils.go @@ -188,6 +188,31 @@ func BackendTrafficPolicyMustBeAccepted(t *testing.T, client client.Client, poli require.NoErrorf(t, waitErr, "error waiting for BackendTrafficPolicy to be accepted") } +// BackendTrafficPolicyMustBeAccepted waits for the specified BackendTrafficPolicy to be accepted. +func BackendTrafficPolicyMustBeAcceptedByAllAncestors( + t *testing.T, client client.Client, policyName types.NamespacedName, + controllerName string, ancestorRefs ...gwapiv1.ParentReference, +) { + t.Helper() + + waitErr := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 60*time.Second, true, func(ctx context.Context) (bool, error) { + policy := &egv1a1.BackendTrafficPolicy{} + err := client.Get(ctx, policyName, policy) + if err != nil { + return false, fmt.Errorf("error fetching BackendTrafficPolicy: %w", err) + } + + if ancestorsForPolicyMatch(t, policyName, ancestorRefs, policy.Status.Ancestors, controllerName) { + return true, nil + } + + tlog.Logf(t, "BackendTrafficPolicy not yet accepted: %v", policy) + return false, nil + }) + + require.NoErrorf(t, waitErr, "error waiting for BackendTrafficPolicy to be accepted") +} + // BackendTrafficPolicyMustFail waits for an BackendTrafficPolicy to fail with the specified reason. func BackendTrafficPolicyMustFail( t *testing.T, client client.Client, policyName types.NamespacedName, @@ -314,6 +339,36 @@ func policyAcceptedByAncestor(ancestors []gwapiv1.PolicyAncestorStatus, controll return false } +func ancestorsForPolicyMatch(t *testing.T, policyName types.NamespacedName, expected []gwapiv1.ParentReference, actual []gwapiv1.PolicyAncestorStatus, controllerName string) bool { + t.Helper() + + if len(expected) != len(actual) { + tlog.Logf(t, "Policy %s/%s expected %d ancestors got %d", policyName.Namespace, policyName.Name, len(expected), len(actual)) + return false + } + + for i, expectedAncestor := range expected { + actualAncestor := actual[i] + accepted := false + if string(actualAncestor.ControllerName) == controllerName && cmp.Equal(actualAncestor.AncestorRef, expectedAncestor) { + for _, condition := range actualAncestor.Conditions { + if condition.Type == string(gwapiv1.PolicyConditionAccepted) && condition.Status == metav1.ConditionTrue { + accepted = true + break + } + } + if !accepted { + tlog.Logf(t, "Policy %s/%s expected Accepted condition on ancestor %s", policyName.Namespace, policyName.Name, actualAncestor.AncestorRef.Name) + return false + } + } else { + tlog.Logf(t, "Policy %s/%s expected Ancestor %s", policyName.Namespace, policyName.Name, actualAncestor.AncestorRef.Name) + return false + } + } + return true +} + // EnvoyExtensionPolicyMustFail waits for an EnvoyExtensionPolicy to fail with the specified reason. func EnvoyExtensionPolicyMustFail( t *testing.T, client client.Client, policyName types.NamespacedName,