Skip to content

fix: cleanup dangling parents in route and policy statuses #7307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: cleanup dangling parents in route and policy statuses #7307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1e4590
fix: cleanup dangling parents in route and policy statuses
y-rabie Oct 28, 2025
67a46b9
fix: only process security/backend/envoyextension policies of our con…
y-rabie Oct 28, 2025
6ad68a8
fix and add unit test for merging ancestors of ExtensionServerPolicies
y-rabie Nov 4, 2025
3cbb371
Adjust and add e2e tests for status cleanup
y-rabie Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions internal/gatewayapi/backendtrafficpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,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)
}
}
}
Expand All @@ -126,7 +126,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)
}
}
}
Expand Down Expand Up @@ -225,6 +225,7 @@ func (t *Translator) buildGatewayPolicyMap(
func (t *Translator) processBackendTrafficPolicyForRoute(
resources *resource.Resources,
xdsIR resource.XdsIRMap,
gatewayMap map[types.NamespacedName]*policyGatewayTargetContext,
Copy link
Member

@zhaohuabing zhaohuabing Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we break this up into smaller problems to avoid such a large change ? and start with the core issue of merging status relevant across gateway classes in the provider status updater layer ?

@y-rabie To address @arkodg 's comment, we probably could move this fix out to a separate PR?

Copy link
Member

@zhaohuabing zhaohuabing Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is no longer needed? Looks like it's been fixed in #7536.

Copy link
Contributor Author

@y-rabie y-rabie Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it does the same thing. I'll not include it in the split PRs 👍

routeMap map[policyTargetRouteKey]*policyRouteTargetContext,
gatewayRouteMap *GatewayPolicyRouteMap,
gatewayPolicyMergedMap *GatewayPolicyRouteMap,
Expand Down Expand Up @@ -254,12 +255,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),
Expand Down
38 changes: 26 additions & 12 deletions internal/gatewayapi/envoyextensionpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (t *Translator) ProcessEnvoyExtensionPolicies(envoyExtensionPolicies []*egv
}

t.processEnvoyExtensionPolicyForRoute(resources, xdsIR,
routeMap, gatewayRouteMap, policy, currTarget)
routeMap, gatewayMap, gatewayRouteMap, policy, currTarget)
}
}
}
Expand All @@ -106,7 +106,7 @@ func (t *Translator) ProcessEnvoyExtensionPolicies(envoyExtensionPolicies []*egv
}

t.processEnvoyExtensionPolicyForRoute(resources, xdsIR,
routeMap, gatewayRouteMap, policy, currTarget)
routeMap, gatewayMap, gatewayRouteMap, policy, currTarget)
}
}
}
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
54 changes: 45 additions & 9 deletions internal/gatewayapi/extensionserverpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand All @@ -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{
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}
112 changes: 112 additions & 0 deletions internal/gatewayapi/extensionserverpolicy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Loading
Loading