🐛 prune stale HTTPRoutes when tags are removed

Signed-off-by: kahirokunn <[email protected]>

Author: kahirokunn

pkg/reconciler/ingress/ingress.go

@@ -23,8 +23,11 @@ import (
 
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	"knative.dev/net-gateway-api/pkg/reconciler/ingress/config"
+	"knative.dev/net-gateway-api/pkg/reconciler/ingress/resources"
 	"knative.dev/net-gateway-api/pkg/status"
 	"knative.dev/networking/pkg/apis/networking/v1alpha1"
 	ingressreconciler "knative.dev/networking/pkg/client/injection/reconciler/networking/v1alpha1/ingress"
@@ -102,7 +105,10 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
 
 	routesReady := true
 
+	desiredRouteNames := sets.New[string]()
 	for _, rule := range ing.Spec.Rules {
+		desiredRouteNames.Insert(resources.LongestHost(rule.Hosts))
+
 		httproute, probeTargets, err := c.reconcileHTTPRoute(ctx, ingressHash, ing, &rule)
 		if err != nil {
 			return err
@@ -123,6 +129,26 @@ func (c *Reconciler) reconcileIngress(ctx context.Context, ing *v1alpha1.Ingress
 		}
 	}
 
+	// Delete HTTPRoutes that don't exist in the current Spec (i.e., tags removed and no longer referenced)
+	{
+		existingRoutes, err := c.httprouteLister.HTTPRoutes(ing.Namespace).List(labels.Everything())
+		if err != nil {
+			return fmt.Errorf("failed to list HTTPRoutes: %w", err)
+		}
+		for _, r := range existingRoutes {
+			// Don't touch routes not owned by this Ingress
+			if !metav1.IsControlledBy(r, ing) {
+				continue
+			}
+			// Not in the desired set = unnecessary
+			if !desiredRouteNames.Has(r.Name) {
+				if err := c.gwapiclient.GatewayV1().HTTPRoutes(r.Namespace).Delete(ctx, r.Name, metav1.DeleteOptions{}); err != nil && !apierrs.IsNotFound(err) {
+					return fmt.Errorf("failed to delete stale HTTPRoute %s/%s: %w", r.Namespace, r.Name, err)
+				}
+			}
+		}
+	}
+
 	externalIngressTLS := ing.GetIngressTLSForVisibility(v1alpha1.IngressVisibilityExternalIP)
 	listeners := make([]*gatewayapi.Listener, 0, len(externalIngressTLS))
 	for _, tls := range externalIngressTLS {

pkg/reconciler/ingress/ingress_test.go

@@ -23,8 +23,11 @@ import (
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	clientgotesting "k8s.io/client-go/testing"
 	"k8s.io/utils/ptr"
@@ -47,6 +50,7 @@ import (
 
 	gatewayapi "sigs.k8s.io/gateway-api/apis/v1"
 	gatewayapiv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
+	gatewaylisters "sigs.k8s.io/gateway-api/pkg/client/listers/apis/v1"
 
 	. "knative.dev/net-gateway-api/pkg/reconciler/testing"
 	. "knative.dev/pkg/reconciler/testing"
@@ -196,6 +200,103 @@ func TestReconcile(t *testing.T) {
 			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
 		}, servicesAndEndpoints...),
 		// no extra update
+	}, {
+		Name: "prune stale HTTPRoute when rule removed",
+		Key:  "ns/name",
+		Objects: append([]runtime.Object{
+			ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReady),
+			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
+			HTTPRoute{
+				Name:      "stale.example.com",
+				Namespace: "ns",
+				Hostname:  "stale.example.com",
+			}.Build(),
+		}, servicesAndEndpoints...),
+		WantDeletes: []clientgotesting.DeleteActionImpl{
+			clientgotesting.NewDeleteAction(
+				schema.GroupVersionResource{
+					Group:    "gateway.networking.k8s.io",
+					Version:  "v1",
+					Resource: "httproutes",
+				},
+				"ns",
+				"stale.example.com",
+			),
+		},
+	}, {
+		Name: "prune skips non-owned HTTPRoute",
+		Key:  "ns/name",
+		Objects: append(func() []runtime.Object {
+			route := HTTPRoute{
+				Name:      "stale.example.com",
+				Namespace: "ns",
+				Hostname:  "stale.example.com",
+			}.Build()
+			// Remove owner so it is not controlled by this Ingress
+			route.OwnerReferences = nil
+			return []runtime.Object{
+				ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReady),
+				httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
+				route,
+			}
+		}(), servicesAndEndpoints...),
+		// No deletes expected
+		WantDeletes: []clientgotesting.DeleteActionImpl{},
+	}, {
+		Name: "prune list error bubbles up",
+		Key:  "ns/name",
+		Ctx:  withHTTPRouteListError(),
+		Objects: append([]runtime.Object{
+			ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReady),
+		}, servicesAndEndpoints...),
+		WantCreates: []runtime.Object{
+			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass)),
+		},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReady, func(i *v1alpha1.Ingress) {
+				i.Status.MarkIngressNotReady("ReconcileIngressFailed", "Ingress reconciliation failed")
+			}),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Created", "Created HTTPRoute \"example.com\""),
+			Eventf(corev1.EventTypeWarning, "InternalError", `failed to list HTTPRoutes: forced list error`),
+		},
+		WantErr: true,
+	}, {
+		Name: "prune delete NotFound tolerated",
+		Key:  "ns/name",
+		WithReactors: []clientgotesting.ReactionFunc{
+			func(a clientgotesting.Action) (bool, runtime.Object, error) {
+				if a.GetVerb() == "delete" && a.GetResource().Resource == "httproutes" {
+					name := a.(clientgotesting.DeleteActionImpl).Name
+					return true, nil, apierrs.NewNotFound(
+						schema.GroupResource{Group: "gateway.networking.k8s.io", Resource: "httproutes"},
+						name,
+					)
+				}
+				return false, nil, nil
+			},
+		},
+		Objects: append([]runtime.Object{
+			ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReady),
+			httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
+			HTTPRoute{
+				Name:      "stale.example.com",
+				Namespace: "ns",
+				Hostname:  "stale.example.com",
+			}.Build(),
+		}, servicesAndEndpoints...),
+		WantDeletes: []clientgotesting.DeleteActionImpl{
+			clientgotesting.NewDeleteAction(
+				schema.GroupVersionResource{
+					Group:    "gateway.networking.k8s.io",
+					Version:  "v1",
+					Resource: "httproutes",
+				},
+				"ns",
+				"stale.example.com",
+			),
+		},
 	}}
 
 	table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {
@@ -214,6 +315,11 @@ func TestReconcile(t *testing.T) {
 			},
 		}
 
+		// Force HTTPRoute List error when test context requests it.
+		if ctx.Value(httpRouteListErrorKey{}) == true {
+			r.httprouteLister = errorWrapHTTPRouteLister{inner: listers.GetHTTPRouteLister()}
+		}
+
 		ingr := ingressreconciler.NewReconciler(ctx, logging.FromContext(ctx), fakeingressclient.Get(ctx),
 			listers.GetIngressLister(), controller.GetEventRecorder(ctx), r, gatewayAPIIngressClassName,
 			controller.Options{
@@ -226,6 +332,38 @@ func TestReconcile(t *testing.T) {
 	}))
 }
 
+// --- helpers for forcing lister errors in specific tests ---
+
+type httpRouteListErrorKey struct{}
+
+func withHTTPRouteListError() context.Context {
+	return context.WithValue(context.Background(), httpRouteListErrorKey{}, true)
+}
+
+type errorWrapHTTPRouteNamespaceLister struct {
+	inner gatewaylisters.HTTPRouteNamespaceLister
+}
+
+func (e errorWrapHTTPRouteNamespaceLister) List(selector labels.Selector) ([]*gatewayapi.HTTPRoute, error) {
+	return nil, fmt.Errorf("forced list error")
+}
+
+func (e errorWrapHTTPRouteNamespaceLister) Get(name string) (*gatewayapi.HTTPRoute, error) {
+	return e.inner.Get(name)
+}
+
+type errorWrapHTTPRouteLister struct {
+	inner gatewaylisters.HTTPRouteLister
+}
+
+func (e errorWrapHTTPRouteLister) List(selector labels.Selector) ([]*gatewayapi.HTTPRoute, error) {
+	return e.inner.List(selector)
+}
+
+func (e errorWrapHTTPRouteLister) HTTPRoutes(namespace string) gatewaylisters.HTTPRouteNamespaceLister {
+	return errorWrapHTTPRouteNamespaceLister{inner: e.inner.HTTPRoutes(namespace)}
+}
+
 func TestReconcileTLS(t *testing.T) {
 	// The gateway API annoyingly has a number of
 	secretName := "name-WE-STICK-A-LONG-UID-HERE"