diff --git a/CHANGELOG.md b/CHANGELOG.md index fb78680aa2..7b562bf131 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -198,6 +198,9 @@ Adding a new version? You'll need three changes: if the config was being updated while the HTTP endpoint was being hit at the same time. [#5474](https://github.com/Kong/kubernetes-ingress-controller/pull/5474) +- Stale `HTTPRoute`'s parent statuses are now removed when the `HTTPRoute` no longer + defines a parent `Gateway` in its `spec.parentRefs`. + [#5477](https://github.com/Kong/kubernetes-ingress-controller/pull/5477) ### Changed diff --git a/internal/controllers/gateway/httproute_controller.go b/internal/controllers/gateway/httproute_controller.go index def05ecc9c..f0a2b860fd 100644 --- a/internal/controllers/gateway/httproute_controller.go +++ b/internal/controllers/gateway/httproute_controller.go @@ -8,6 +8,7 @@ import ( "github.com/go-logr/logr" "github.com/samber/lo" + "golang.org/x/exp/slices" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -417,6 +418,14 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } + // Ensure we have no status for no-longer defined parentRefs. + if wasAnyStatusRemoved := ensureNoStaleParentStatus(httproute); wasAnyStatusRemoved { + if err := r.Status().Update(ctx, httproute); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + // the referenced gateway object(s) for the HTTPRoute needs to be ready // before we'll attempt any configurations of it. If it's not we'll // requeue the object and wait until all supported gateways are ready. @@ -752,3 +761,39 @@ func (r *HTTPRouteReconciler) getHTTPRouteRuleReason(ctx context.Context, httpRo func (r *HTTPRouteReconciler) SetLogger(l logr.Logger) { r.Log = l } + +// ensureNoStaleParentStatus removes any status references to Gateways that are no longer in the HTTPRoute's parentRefs +// and returns true if any status was removed. +func ensureNoStaleParentStatus(httproute *gatewayapi.HTTPRoute) (wasAnyStatusRemoved bool) { + // Create a map of currently defined parentRefs for fast lookup. + currentlyDefinedParentRefs := make(map[string]struct{}) + for _, parentRef := range httproute.Spec.ParentRefs { + currentlyDefinedParentRefs[parentReferenceKey(httproute.Namespace, parentRef)] = struct{}{} + } + + for parentIdx, parentStatus := range httproute.Status.Parents { + // Don't touch statuses from other controllers. + if parentStatus.ControllerName != GetControllerName() { + continue + } + // Remove the status if the parentRef is no longer defined. + if _, ok := currentlyDefinedParentRefs[parentReferenceKey(httproute.Namespace, parentStatus.ParentRef)]; !ok { + httproute.Status.Parents = slices.Delete(httproute.Status.Parents, parentIdx, parentIdx+1) + wasAnyStatusRemoved = true + } + } + return wasAnyStatusRemoved +} + +// parentReferenceKey returns a string key for a parentRef of a route. It can be used for indexing a map. +func parentReferenceKey(routeNamespace string, parentRef gatewayapi.ParentReference) string { + namespace := routeNamespace + if parentRef.Namespace != nil { + namespace = string(*parentRef.Namespace) + } + sectionName := "" + if parentRef.SectionName != nil { + sectionName = string(*parentRef.SectionName) + } + return fmt.Sprintf("%s/%s/%s", namespace, parentRef.Name, sectionName) +} diff --git a/internal/controllers/gateway/httproute_controller_test.go b/internal/controllers/gateway/httproute_controller_test.go new file mode 100644 index 0000000000..cfcb7c7545 --- /dev/null +++ b/internal/controllers/gateway/httproute_controller_test.go @@ -0,0 +1,160 @@ +package gateway + +import ( + "testing" + + "github.com/samber/lo" + "github.com/stretchr/testify/assert" + + "github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi" +) + +func TestEnsureNoStaleParentStatus(t *testing.T) { + testCases := []struct { + name string + httproute *gatewayapi.HTTPRoute + expectedAnyStatusRemoved bool + expectedStatusesForRefs []gatewayapi.ParentReference + }{ + { + name: "no stale status", + httproute: &gatewayapi.HTTPRoute{ + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + }, + }, + }, + expectedAnyStatusRemoved: false, + expectedStatusesForRefs: nil, // There was no status for `defined-in-spec` created yet. + }, + { + name: "no stale status with existing status for spec parent ref", + httproute: &gatewayapi.HTTPRoute{ + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + }, + }, + Status: gatewayapi.HTTPRouteStatus{ + RouteStatus: gatewayapi.RouteStatus{ + Parents: []gatewayapi.RouteParentStatus{ + { + ControllerName: GetControllerName(), + ParentRef: gatewayapi.ParentReference{Name: "defined-in-spec"}, + }, + }, + }, + }, + }, + expectedStatusesForRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + expectedAnyStatusRemoved: false, + }, + { + name: "stale status", + httproute: &gatewayapi.HTTPRoute{ + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + }, + }, + Status: gatewayapi.HTTPRouteStatus{ + RouteStatus: gatewayapi.RouteStatus{ + Parents: []gatewayapi.RouteParentStatus{ + { + ControllerName: GetControllerName(), + ParentRef: gatewayapi.ParentReference{Name: "not-defined-in-spec"}, + }, + }, + }, + }, + }, + expectedStatusesForRefs: nil, // There was no status for `defined-in-spec` created yet. + expectedAnyStatusRemoved: true, + }, + { + name: "stale status with existing status for spec parent ref", + httproute: &gatewayapi.HTTPRoute{ + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + }, + }, + Status: gatewayapi.HTTPRouteStatus{ + RouteStatus: gatewayapi.RouteStatus{ + Parents: []gatewayapi.RouteParentStatus{ + { + ControllerName: GetControllerName(), + ParentRef: gatewayapi.ParentReference{Name: "not-defined-in-spec"}, + }, + { + ControllerName: GetControllerName(), + ParentRef: gatewayapi.ParentReference{Name: "defined-in-spec"}, + }, + }, + }, + }, + }, + expectedStatusesForRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + expectedAnyStatusRemoved: true, + }, + { + name: "do not remove status for other controllers", + httproute: &gatewayapi.HTTPRoute{ + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{ + {Name: "defined-in-spec"}, + }, + }, + }, + Status: gatewayapi.HTTPRouteStatus{ + RouteStatus: gatewayapi.RouteStatus{ + Parents: []gatewayapi.RouteParentStatus{ + { + ControllerName: gatewayapi.GatewayController("another-controller"), + ParentRef: gatewayapi.ParentReference{Name: "not-defined-in-spec"}, + }, + { + ControllerName: GetControllerName(), + ParentRef: gatewayapi.ParentReference{Name: "defined-in-spec"}, + }, + }, + }, + }, + }, + expectedStatusesForRefs: []gatewayapi.ParentReference{ + {Name: "not-defined-in-spec"}, + {Name: "defined-in-spec"}, + }, + expectedAnyStatusRemoved: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + wasAnyStatusRemoved := ensureNoStaleParentStatus(tc.httproute) + assert.Equal(t, tc.expectedAnyStatusRemoved, wasAnyStatusRemoved) + + actualStatuses := lo.Map(tc.httproute.Status.Parents, func(status gatewayapi.RouteParentStatus, _ int) string { + return parentReferenceKey(tc.httproute.Namespace, status.ParentRef) + }) + expectedStatuses := lo.Map(tc.expectedStatusesForRefs, func(ref gatewayapi.ParentReference, _ int) string { + return parentReferenceKey(tc.httproute.Namespace, ref) + }) + assert.ElementsMatch(t, expectedStatuses, actualStatuses) + }) + } +} diff --git a/test/envtest/httproute_controller_test.go b/test/envtest/httproute_controller_test.go index 6f4dbb11b7..a8fb519c84 100644 --- a/test/envtest/httproute_controller_test.go +++ b/test/envtest/httproute_controller_test.go @@ -27,14 +27,14 @@ import ( "github.com/kong/kubernetes-ingress-controller/v3/test/mocks" ) +const ( + waitDuration = 5 * time.Second + tickDuration = 100 * time.Millisecond +) + func TestHTTPRouteReconcilerProperlyReactsToReferenceGrant(t *testing.T) { t.Parallel() - const ( - waitDuration = 5 * time.Second - tickDuration = 100 * time.Millisecond - ) - scheme := Scheme(t, WithGatewayAPI) cfg := Setup(t, scheme) client := NewControllerClient(t, scheme, cfg) @@ -265,6 +265,120 @@ func TestHTTPRouteReconcilerProperlyReactsToReferenceGrant(t *testing.T) { } } +func TestHTTPRouteReconciler_RemovesOutdatedParentStatuses(t *testing.T) { + t.Parallel() + + scheme := Scheme(t, WithGatewayAPI) + cfg := Setup(t, scheme) + client := NewControllerClient(t, scheme, cfg) + + reconciler := &gateway.HTTPRouteReconciler{ + Client: client, + DataplaneClient: mocks.Dataplane{}, + } + + // We use a deferred cancel to stop the manager and not wait for its timeout. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ns := CreateNamespace(ctx, t, client) + nsRoute := CreateNamespace(ctx, t, client) + + StartReconcilers(ctx, t, client.Scheme(), cfg, reconciler) + + gwc := gatewayapi.GatewayClass{ + Spec: gatewayapi.GatewayClassSpec{ + ControllerName: gateway.GetControllerName(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Annotations: map[string]string{ + "konghq.com/gatewayclass-unmanaged": "placeholder", + }, + }, + } + require.NoError(t, client.Create(ctx, &gwc)) + t.Cleanup(func() { _ = client.Delete(ctx, &gwc) }) + + gw := gatewayapi.Gateway{ + Spec: gatewayapi.GatewaySpec{ + GatewayClassName: gatewayapi.ObjectName(gwc.Name), + Listeners: []gatewayapi.Listener{ + { + Name: gatewayapi.SectionName("http"), + Port: gatewayapi.PortNumber(80), + Protocol: gatewayapi.HTTPProtocolType, + AllowedRoutes: &gatewayapi.AllowedRoutes{ + Namespaces: &gatewayapi.RouteNamespaces{ + From: lo.ToPtr(gatewayapi.NamespacesFromAll), + }, + }, + }, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + Name: uuid.NewString(), + }, + } + require.NoError(t, client.Create(ctx, &gw)) + + route := gatewayapi.HTTPRoute{ + TypeMeta: metav1.TypeMeta{ + Kind: "HTTPRoute", + APIVersion: "v1beta1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: nsRoute.Name, + Name: uuid.NewString(), + }, + Spec: gatewayapi.HTTPRouteSpec{ + CommonRouteSpec: gatewayapi.CommonRouteSpec{ + ParentRefs: []gatewayapi.ParentReference{{ + Name: gatewayapi.ObjectName(gw.Name), + Namespace: lo.ToPtr(gatewayapi.Namespace(ns.Name)), + }}, + }, + Rules: []gatewayapi.HTTPRouteRule{{ + BackendRefs: builder.NewHTTPBackendRef("backend-1").WithNamespace(ns.Name).ToSlice(), + }}, + }, + } + require.NoError(t, client.Create(ctx, &route)) + + // Status has to be updated separately. + route.Status = gatewayapi.HTTPRouteStatus{ + RouteStatus: gatewayapi.RouteStatus{ + Parents: []gatewayapi.RouteParentStatus{ + { + ParentRef: gatewayapi.ParentReference{ + Name: "other-gw-name", + }, + ControllerName: gateway.GetControllerName(), + }, + }, + }, + } + require.NoError(t, client.Status().Update(ctx, &route)) + + require.Eventually(t, func() bool { + err := client.Get(ctx, k8stypes.NamespacedName{Name: route.Name, Namespace: route.Namespace}, &route) + if err != nil { + t.Logf("failed to get HTTPRoute %s/%s: %v", route.Namespace, route.Name, err) + return false + } + + if staleStatusFound := lo.ContainsBy(route.Status.Parents, func(ps gatewayapi.RouteParentStatus) bool { + return ps.ParentRef.Name == "other-gw-name" + }); staleStatusFound { + t.Log("found stale status for parent other-gw-name") + return false + } + + return true + }, waitDuration, tickDuration, "expected stale status to be removed from HTTPRoute") +} + func printHTTPRoutesConditions(ctx context.Context, client ctrlclient.Client, nn k8stypes.NamespacedName) string { var route gatewayapi.HTTPRoute err := client.Get(ctx, ctrlclient.ObjectKey{Namespace: nn.Namespace, Name: nn.Name}, &route)