bug: prune stale HTTPRoutes when tags are removed (#901)

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

Author: knative-prow-robot

pkg/reconciler/ingress/fixtures_test.go

@@ -57,6 +57,7 @@ func (r HTTPRoute) Build() *gatewayapi.HTTPRoute {
 				networking.IngressClassAnnotationKey: gatewayAPIIngressClassName,
 			},
 			Labels: map[string]string{
+				networking.IngressLabelKey:    "name",
 				networking.VisibilityLabelKey: "",
 			},
 			OwnerReferences: []metav1.OwnerReference{{

pkg/reconciler/ingress/ingress.go

@@ -23,9 +23,13 @@ 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"
 	"knative.dev/networking/pkg/apis/networking/v1alpha1"
 	ingressreconciler "knative.dev/networking/pkg/client/injection/reconciler/networking/v1alpha1/ingress"
 	"knative.dev/networking/pkg/ingress"
@@ -102,7 +106,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 +130,25 @@ 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)
+	{
+		selector := labels.SelectorFromSet(labels.Set{
+			networking.IngressLabelKey: ing.Name,
+		})
+		existingRoutes, err := c.httprouteLister.HTTPRoutes(ing.Namespace).List(selector)
+		if err != nil {
+			return fmt.Errorf("failed to list HTTPRoutes: %w", err)
+		}
+		for _, r := range existingRoutes {
+			// 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,10 @@ 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/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	clientgotesting "k8s.io/client-go/testing"
 	"k8s.io/utils/ptr"
@@ -196,6 +198,86 @@ 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 controller ownership and the ingress label so this HTTPRoute
+			// is neither controlled by nor labeled for this Ingress. It should
+			// therefore be ignored by the prune logic.
+			route.OwnerReferences = nil
+			delete(route.Labels, networking.IngressLabelKey)
+			return []runtime.Object{
+				ing(withBasicSpec, withGatewayAPIclass, withFinalizer, makeItReady),
+				httpRoute(t, ing(withBasicSpec, withGatewayAPIclass), httpRouteReady),
+				route,
+			}
+		}(), servicesAndEndpoints...),
+		// No deletes expected
+		WantDeletes: []clientgotesting.DeleteActionImpl{},
+	}, {
+		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 {

pkg/reconciler/ingress/resources/httproute.go

@@ -204,6 +204,7 @@ func MakeHTTPRoute(
 			Name:      LongestHost(rule.Hosts),
 			Namespace: ing.Namespace,
 			Labels: kmeta.UnionMaps(ing.Labels, map[string]string{
+				networking.IngressLabelKey:    ing.Name,
 				networking.VisibilityLabelKey: visibility,
 			}),
 			Annotations: kmeta.FilterMap(ing.GetAnnotations(), func(key string) bool {