diff --git a/internal/provider/kubernetes/status.go b/internal/provider/kubernetes/status.go index 03ff8f1120..7d554e2a1d 100644 --- a/internal/provider/kubernetes/status.go +++ b/internal/provider/kubernetes/status.go @@ -501,12 +501,34 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext // mergeRouteParentStatus merges the old and new RouteParentStatus. // This is needed because the RouteParentStatus doesn't support strategic merge patch yet. +// It also removes any status.parents entries that are no longer referenced in the route's spec.parentRefs. func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []gwapiv1.RouteParentStatus { - merged := make([]gwapiv1.RouteParentStatus, len(old)) - _ = copy(merged, old) + // First, create a map of all parentRefs in the new status + // These represent the current valid parentRefs from spec.parentRefs + newParentRefs := make(map[string]bool) + for _, parent := range new { + // Create a unique key for each parentRef + key := parentRefToString(parent.ParentRef, ns) + newParentRefs[key] = true + } + + // Filter out old entries that are no longer referenced in spec.parentRefs + var filteredOld []gwapiv1.RouteParentStatus + for _, existing := range old { + key := parentRefToString(existing.ParentRef, ns) + if newParentRefs[key] { + // Keep this entry as it's still referenced + filteredOld = append(filteredOld, existing) + } + // Skip entries that are no longer referenced + } + + // Now merge the filtered old entries with the new ones + merged := make([]gwapiv1.RouteParentStatus, len(filteredOld)) + _ = copy(merged, filteredOld) for _, parent := range new { found := -1 - for i, existing := range old { + for i, existing := range filteredOld { if isParentRefEqual(parent.ParentRef, existing.ParentRef, ns) { found = i break @@ -521,6 +543,30 @@ func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []g return merged } +// parentRefToString creates a unique string representation of a ParentReference +func parentRefToString(ref gwapiv1.ParentReference, routeNS string) string { + defaultGroup := (*gwapiv1.Group)(&gwapiv1.GroupVersion.Group) + group := defaultGroup + if ref.Group != nil { + group = ref.Group + } + + defaultKind := gwapiv1.Kind(resource.KindGateway) + kind := &defaultKind + if ref.Kind != nil { + kind = ref.Kind + } + + // If the parent's namespace is not set, default to the namespace of the Route. + defaultNS := gwapiv1.Namespace(routeNS) + namespace := &defaultNS + if ref.Namespace != nil { + namespace = ref.Namespace + } + + return fmt.Sprintf("%s/%s/%s/%s", *group, *kind, *namespace, ref.Name) +} + func isParentRefEqual(ref1, ref2 gwapiv1.ParentReference, routeNS string) bool { defaultGroup := (*gwapiv1.Group)(&gwapiv1.GroupVersion.Group) if ref1.Group == nil { diff --git a/internal/provider/kubernetes/status_cleanup_test.go b/internal/provider/kubernetes/status_cleanup_test.go new file mode 100644 index 0000000000..18f42a6637 --- /dev/null +++ b/internal/provider/kubernetes/status_cleanup_test.go @@ -0,0 +1,174 @@ +// 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. + +package kubernetes + +import ( + "reflect" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" +) + +func Test_mergeRouteParentStatusCleanup(t *testing.T) { + type args struct { + ns string + old []gwapiv1.RouteParentStatus + new []gwapiv1.RouteParentStatus + } + tests := []struct { + name string + args args + want []gwapiv1.RouteParentStatus + }{ + { + name: "remove old entries that are no longer referenced", + args: args{ + ns: "default", + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "OldReason", + }, + }, + }, + }, + new: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + }, + }, + // gateway2 is removed from spec.parentRefs, so it should not appear in the merged result + }, + }, + want: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + }, + }, + }, + }, + { + name: "update existing entry and keep only referenced entries", + args: args{ + ns: "default", + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "OldReason", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "OldReason", + }, + }, + }, + }, + new: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "NewReason", + }, + }, + }, + // gateway2 is removed from spec.parentRefs, so it should not appear in the merged result + }, + }, + want: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "NewReason", + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := mergeRouteParentStatus(tt.args.ns, tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { + t.Errorf("mergeRouteParentStatus() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/provider/kubernetes/status_test.go b/internal/provider/kubernetes/status_test.go index 5e81c46135..7b1cebe378 100644 --- a/internal/provider/kubernetes/status_test.go +++ b/internal/provider/kubernetes/status_test.go @@ -16,6 +16,7 @@ import ( func Test_mergeRouteParentStatus(t *testing.T) { type args struct { + ns string old []gwapiv1.RouteParentStatus new []gwapiv1.RouteParentStatus } @@ -27,6 +28,7 @@ func Test_mergeRouteParentStatus(t *testing.T) { { name: "merge old and new", args: args{ + ns: "default", old: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", @@ -49,12 +51,27 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "OldReason", + }, + }, + }, }, new: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { @@ -64,6 +81,27 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: 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{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, }, }, want: []gwapiv1.RouteParentStatus{ @@ -91,7 +129,8 @@ func Test_mergeRouteParentStatus(t *testing.T) { { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { @@ -107,6 +146,7 @@ func Test_mergeRouteParentStatus(t *testing.T) { { name: "override an existing parent", args: args{ + ns: "default", old: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", @@ -150,7 +190,8 @@ func Test_mergeRouteParentStatus(t *testing.T) { { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { @@ -166,25 +207,8 @@ func Test_mergeRouteParentStatus(t *testing.T) { { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", - }, - 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{ - Name: "gateway2", + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { @@ -200,6 +224,7 @@ func Test_mergeRouteParentStatus(t *testing.T) { { name: "nothing changed", args: args{ + ns: "default", old: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", @@ -237,7 +262,8 @@ func Test_mergeRouteParentStatus(t *testing.T) { { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { @@ -253,31 +279,83 @@ func Test_mergeRouteParentStatus(t *testing.T) { { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionTrue, - Reason: "Accepted", + Status: metav1.ConditionFalse, + Reason: "SomeReason", }, - { - Type: string(gwapiv1.RouteConditionResolvedRefs), - Status: metav1.ConditionTrue, - Reason: "ResolvedRefs", + }, + }, + }, + }, + { + name: "remove old entries that are no longer referenced", + args: args{ + ns: "default", + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + }, + }, + }, + new: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, }, }, + // gateway2 is removed from spec.parentRefs, so it should not appear in the merged result }, + }, + want: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), }, Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionFalse, - Reason: "SomeReason", + Status: metav1.ConditionTrue, + Reason: "Accepted", }, }, }, @@ -286,7 +364,7 @@ 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) { + if got := mergeRouteParentStatus(tt.args.ns, tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { t.Errorf("mergeRouteParentStatus() = %v, want %v", got, tt.want) } }) diff --git a/test/e2e/testdata/httproute-parents-cleanup.yaml b/test/e2e/testdata/httproute-parents-cleanup.yaml new file mode 100644 index 0000000000..d5110a4391 --- /dev/null +++ b/test/e2e/testdata/httproute-parents-cleanup.yaml @@ -0,0 +1,41 @@ +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway-1 + namespace: gateway-conformance-infra +spec: + gatewayClassName: envoy-gateway + listeners: + - name: http + port: 80 + protocol: HTTP +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: gateway-2 + namespace: gateway-conformance-infra +spec: + gatewayClassName: envoy-gateway + listeners: + - name: http + port: 81 + protocol: HTTP +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: httproute-parents-cleanup + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: gateway-1 + - name: gateway-2 + rules: + - matches: + - path: + type: PathPrefix + value: /test + backendRefs: + - name: infra-backend-v1 + port: 8080 diff --git a/test/e2e/tests/httproute_parents_cleanup.go b/test/e2e/tests/httproute_parents_cleanup.go new file mode 100644 index 0000000000..3fb298bbc5 --- /dev/null +++ b/test/e2e/tests/httproute_parents_cleanup.go @@ -0,0 +1,94 @@ +// 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" + + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + "sigs.k8s.io/gateway-api/conformance/utils/kubernetes" + "sigs.k8s.io/gateway-api/conformance/utils/suite" +) + +func init() { + ConformanceTests = append(ConformanceTests, HTTPRouteParentsCleanupTest) +} + +// HTTPRouteParentsCleanupTest tests that when a parentRef is removed from HTTPRoute spec.parentRefs, +// the corresponding entry in status.parents is also removed. +var HTTPRouteParentsCleanupTest = suite.ConformanceTest{ + ShortName: "HTTPRouteParentsCleanup", + Description: "Test HTTPRoute status.parents cleanup when parentRefs are removed", + Manifests: []string{"testdata/httproute-parents-cleanup.yaml"}, + Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { + t.Run("HTTPRoute status.parents should be cleaned up when parentRefs are removed", func(t *testing.T) { + ns := "gateway-conformance-infra" + routeNN := types.NamespacedName{Name: "httproute-parents-cleanup", Namespace: ns} + gw1NN := types.NamespacedName{Name: "gateway-1", Namespace: ns} + gw2NN := types.NamespacedName{Name: "gateway-2", Namespace: ns} + + // Wait for the HTTPRoute to be accepted by both gateways + kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gw1NN), routeNN) + kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gw2NN), routeNN) + + // Verify that the HTTPRoute has two entries in status.parents + route := &gwapiv1.HTTPRoute{} + err := suite.Client.Get(context.Background(), routeNN, route) + if err != nil { + t.Fatalf("Failed to get HTTPRoute: %v", err) + } + + if len(route.Status.Parents) != 2 { + t.Fatalf("Expected HTTPRoute to have 2 parents in status, got %d", len(route.Status.Parents)) + } + + // Update the HTTPRoute to remove the second parentRef + route.Spec.ParentRefs = route.Spec.ParentRefs[:1] // Keep only the first parentRef + err = suite.Client.Update(context.Background(), route) + if err != nil { + t.Fatalf("Failed to update HTTPRoute: %v", err) + } + + // Wait for the HTTPRoute status to be updated + err = wait.PollUntilContextTimeout(context.Background(), 1*time.Second, suite.TimeoutConfig.RouteMustHaveParents, true, func(ctx context.Context) (bool, error) { + updatedRoute := &gwapiv1.HTTPRoute{} + err := suite.Client.Get(ctx, routeNN, updatedRoute) + if err != nil { + return false, err + } + + // Check if status.parents has been cleaned up + return len(updatedRoute.Status.Parents) == 1, nil + }) + if err != nil { + t.Fatalf("Timed out waiting for HTTPRoute status.parents to be cleaned up: %v", err) + } + + // Get the updated HTTPRoute + updatedRoute := &gwapiv1.HTTPRoute{} + err = suite.Client.Get(context.Background(), routeNN, updatedRoute) + if err != nil { + t.Fatalf("Failed to get updated HTTPRoute: %v", err) + } + + // Verify that the remaining parent is for gateway-1 + if len(updatedRoute.Status.Parents) != 1 { + t.Fatalf("Expected HTTPRoute to have 1 parent in status, got %d", len(updatedRoute.Status.Parents)) + } + + parent := updatedRoute.Status.Parents[0] + if string(parent.ParentRef.Name) != "gateway-1" { + t.Fatalf("Expected remaining parent to be gateway-1, got %s", parent.ParentRef.Name) + } + }) + }, +}