Switch to using Finalizers for ClusterIngress deletion.

This removes the OwnerReferences from ClusterIngress, which are buggy and unsupported anyways.

Instead, we will add a finalizer prior to creating ClusterIngress, which will enable us to hook into Route deletion to clean up the ClusterIngress resources manually.

When we see a Route with a DeletionTimestamp, we elide most processing.  When we are the first finalizer on the list, it is our turn and we delete the cluster ingresses matching our label selector and remove the finalizer to allow deletion to proceed.

Fixes: #2570

config/200-clusterrole.yaml

@@ -37,7 +37,7 @@ rules:
     verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
   - apiGroups: ["networking.internal.knative.dev"]
     resources: ["clusteringresses", "clusteringresses/status"]
-    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
+    verbs: ["get", "list", "create", "update", "delete", "deletecollection", "patch", "watch"]
   - apiGroups: ["build.knative.dev"]
     resources: ["builds"]
     verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]

pkg/reconciler/testing/actions.go

@@ -23,10 +23,11 @@ import (
 )
 
 type Actions struct {
-	Creates []clientgotesting.CreateAction
-	Updates []clientgotesting.UpdateAction
-	Deletes []clientgotesting.DeleteAction
-	Patches []clientgotesting.PatchAction
+	Creates           []clientgotesting.CreateAction
+	Updates           []clientgotesting.UpdateAction
+	Deletes           []clientgotesting.DeleteAction
+	DeleteCollections []clientgotesting.DeleteCollectionAction
+	Patches           []clientgotesting.PatchAction
 }
 
 type ActionRecorder interface {
@@ -50,6 +51,9 @@ func (l ActionRecorderList) ActionsByVerb() (Actions, error) {
 			case "delete":
 				a.Deletes = append(a.Deletes,
 					action.(clientgotesting.DeleteAction))
+			case "delete-collection":
+				a.DeleteCollections = append(a.DeleteCollections,
+					action.(clientgotesting.DeleteCollectionAction))
 			case "patch":
 				a.Patches = append(a.Patches,
 					action.(clientgotesting.PatchAction))

pkg/reconciler/testing/table.go

@@ -63,6 +63,9 @@ type TableRow struct {
 	// WantDeletes holds the set of Delete calls we expect during reconciliation.
 	WantDeletes []clientgotesting.DeleteActionImpl
 
+	// WantDeleteCollections holds the set of DeleteCollection calls we expect during reconciliation.
+	WantDeleteCollections []clientgotesting.DeleteCollectionActionImpl
+
 	// WantPatches holds the set of Patch calls we expect during reconciliation.
 	WantPatches []clientgotesting.PatchActionImpl
 
@@ -235,9 +238,32 @@ func (r *TableRow) Test(t *testing.T, factory Factory) {
 		}
 	}
 
+	for i, want := range r.WantDeleteCollections {
+		if i >= len(actions.DeleteCollections) {
+			t.Errorf("Missing delete-collection: %#v", want)
+			continue
+		}
+		got := actions.DeleteCollections[i]
+		if got, want := got.GetListRestrictions().Labels, want.GetListRestrictions().Labels; (got != nil) != (want != nil) || got.String() != want.String() {
+			t.Errorf("Unexpected delete-collection[%d].Labels = %v, wanted %v", i, got, want)
+		}
+		// TODO(mattmoor): Add this if/when we need support.
+		if got := got.GetListRestrictions().Fields; got.String() != "" {
+			t.Errorf("Unexpected delete-collection[%d].Fields = %v, wanted nil", i, got)
+		}
+		if !r.SkipNamespaceValidation && got.GetNamespace() != expectedNamespace {
+			t.Errorf("Unexpected delete-collection[%d]: %#v", i, got)
+		}
+	}
+	if got, want := len(actions.DeleteCollections), len(r.WantDeleteCollections); got > want {
+		for _, extra := range actions.DeleteCollections[want:] {
+			t.Errorf("Extra delete-collection: %#v", extra)
+		}
+	}
+
 	for i, want := range r.WantPatches {
 		if i >= len(actions.Patches) {
-			t.Errorf("Missing patch: %#v", want)
+			t.Errorf("Missing patch: %#v; raw: %s", want, string(want.GetPatch()))
 			continue
 		}
 
@@ -254,7 +280,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) {
 	}
 	if got, want := len(actions.Patches), len(r.WantPatches); got > want {
 		for _, extra := range actions.Patches[want:] {
-			t.Errorf("Extra patch: %#v", extra)
+			t.Errorf("Extra patch: %#v; raw: %s", extra, string(extra.GetPatch()))
 		}
 	}
 

pkg/reconciler/v1alpha1/route/reconcile_resources.go

@@ -49,10 +49,7 @@ func (c *Reconciler) getClusterIngressForRoute(route *v1alpha1.Route) (*netv1alp
 	}
 
 	// If that isn't found, then fallback on the legacy selector-based approach.
-	selector := labels.Set(map[string]string{
-		serving.RouteLabelKey:          route.Name,
-		serving.RouteNamespaceLabelKey: route.Namespace,
-	}).AsSelector()
+	selector := routeOwnerLabelSelector(route)
 	ingresses, err := c.clusterIngressLister.List(selector)
 	if err != nil {
 		return nil, err
@@ -70,6 +67,22 @@ func (c *Reconciler) getClusterIngressForRoute(route *v1alpha1.Route) (*netv1alp
 	return ingresses[0], nil
 }
 
+func routeOwnerLabelSelector(route *v1alpha1.Route) labels.Selector {
+	return labels.Set(map[string]string{
+		serving.RouteLabelKey:          route.Name,
+		serving.RouteNamespaceLabelKey: route.Namespace,
+	}).AsSelector()
+}
+
+func (c *Reconciler) deleteClusterIngressForRoute(route *v1alpha1.Route) error {
+	selector := routeOwnerLabelSelector(route)
+
+	// We always use DeleteCollection because even with a fixed name, we apply the labels.
+	return c.ServingClientSet.NetworkingV1alpha1().ClusterIngresses().DeleteCollection(
+		nil, metav1.ListOptions{LabelSelector: selector.String()},
+	)
+}
+
 func (c *Reconciler) reconcileClusterIngress(
 	ctx context.Context, r *v1alpha1.Route, desired *netv1alpha1.ClusterIngress) (*netv1alpha1.ClusterIngress, error) {
 	logger := logging.FromContext(ctx)

pkg/reconciler/v1alpha1/route/resources/cluster_ingress.go

@@ -24,7 +24,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 
-	"github.com/knative/pkg/kmeta"
 	"github.com/knative/serving/pkg/activator"
 	"github.com/knative/serving/pkg/apis/networking/v1alpha1"
 	"github.com/knative/serving/pkg/apis/serving"
@@ -53,8 +52,7 @@ func MakeClusterIngress(r *servingv1alpha1.Route, tc *traffic.Config) *v1alpha1.
 				serving.RouteLabelKey:          r.Name,
 				serving.RouteNamespaceLabelKey: r.Namespace,
 			},
-			OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)},
-			Annotations:     r.ObjectMeta.Annotations,
+			Annotations: r.ObjectMeta.Annotations,
 		},
 		Spec: makeClusterIngressSpec(r, tc.Targets),
 	}

pkg/reconciler/v1alpha1/route/resources/cluster_ingress_test.go

@@ -20,7 +20,6 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/knative/pkg/kmeta"
 	"github.com/knative/serving/pkg/apis/networking"
 	netv1alpha1 "github.com/knative/serving/pkg/apis/networking/v1alpha1"
 	"github.com/knative/serving/pkg/apis/serving"
@@ -55,9 +54,6 @@ func TestMakeClusterIngress_CorrectMetadata(t *testing.T) {
 		Annotations: map[string]string{
 			networking.IngressClassAnnotationKey: clusteringress.IstioIngressClassName,
 		},
-		OwnerReferences: []metav1.OwnerReference{
-			*kmeta.NewControllerRef(r),
-		},
 	}
 	meta := MakeClusterIngress(r, &traffic.Config{Targets: targets}).ObjectMeta
 	if diff := cmp.Diff(expected, meta); diff != "" {

pkg/reconciler/v1alpha1/route/route.go

@@ -18,13 +18,16 @@ package route
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 
 	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	corev1informers "k8s.io/client-go/informers/core/v1"
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
@@ -52,6 +55,11 @@ const (
 	controllerAgentName = "route-controller"
 )
 
+var (
+	routeResource  = v1alpha1.Resource("routes")
+	routeFinalizer = routeResource.String()
+)
+
 type configStore interface {
 	ToContext(ctx context.Context) context.Context
 	WatchConfigs(w configmap.Watcher)
@@ -132,13 +140,10 @@ func NewControllerWithClock(
 		},
 	})
 
-	clusterIngressInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
-		FilterFunc: controller.Filter(v1alpha1.SchemeGroupVersion.WithKind("Route")),
-		Handler: cache.ResourceEventHandlerFuncs{
-			AddFunc:    impl.EnqueueLabelOfNamespaceScopedResource(serving.RouteNamespaceLabelKey, serving.RouteLabelKey),
-			UpdateFunc: controller.PassNew(impl.EnqueueLabelOfNamespaceScopedResource(serving.RouteNamespaceLabelKey, serving.RouteLabelKey)),
-			DeleteFunc: impl.EnqueueLabelOfNamespaceScopedResource(serving.RouteNamespaceLabelKey, serving.RouteLabelKey),
-		},
+	clusterIngressInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    impl.EnqueueLabelOfNamespaceScopedResource(serving.RouteNamespaceLabelKey, serving.RouteLabelKey),
+		UpdateFunc: controller.PassNew(impl.EnqueueLabelOfNamespaceScopedResource(serving.RouteNamespaceLabelKey, serving.RouteLabelKey)),
+		DeleteFunc: impl.EnqueueLabelOfNamespaceScopedResource(serving.RouteNamespaceLabelKey, serving.RouteLabelKey),
 	})
 
 	c.tracker = tracker.New(impl.EnqueueKey, opt.GetTrackerLease())
@@ -222,6 +227,13 @@ func (c *Reconciler) reconcile(ctx context.Context, r *v1alpha1.Route) error {
 
 	r.Status.InitializeConditions()
 
+	if r.GetDeletionTimestamp() != nil {
+		// Check for a DeletionTimestamp.  If present, elide the normal reconcile logic.
+		// Instead, if our Finalizer is first, delete the ClusterIngress for this Route
+		// and remove the finalizer.
+		return c.reconcileDeletion(ctx, r)
+	}
+
 	logger.Infof("Reconciling route: %v", r)
 	// Configure traffic based on the RouteSpec.
 	traffic, err := c.configureTraffic(ctx, r)
@@ -244,6 +256,11 @@ func (c *Reconciler) reconcile(ctx context.Context, r *v1alpha1.Route) error {
 		Hostname: resourcenames.K8sServiceFullname(r),
 	}
 
+	// Add the finalizer before creating the ClusterIngress so that we can be sure it gets cleaned up.
+	if err := c.ensureFinalizer(r); err != nil {
+		return err
+	}
+
 	logger.Info("Creating ClusterIngress.")
 	clusterIngress, err := c.reconcileClusterIngress(ctx, r, resources.MakeClusterIngress(r, traffic))
 	if err != nil {
@@ -260,6 +277,26 @@ func (c *Reconciler) reconcile(ctx context.Context, r *v1alpha1.Route) error {
 	return nil
 }
 
+func (c *Reconciler) reconcileDeletion(ctx context.Context, r *v1alpha1.Route) error {
+	logger := logging.FromContext(ctx)
+
+	if len(r.Finalizers) == 0 || r.Finalizers[0] != routeFinalizer {
+		return nil
+	}
+
+	// Delete the ClusterIngress resources for this Route.
+	logger.Info("Cleaning up ClusterIngress")
+	if err := c.deleteClusterIngressForRoute(r); err != nil {
+		return err
+	}
+
+	// Update the Route to remove the Finalizer.
+	logger.Info("Removing Finalizer")
+	r.Finalizers = r.Finalizers[1:]
+	_, err := c.ServingClientSet.ServingV1alpha1().Routes(r.Namespace).Update(r)
+	return err
+}
+
 // configureTraffic attempts to configure traffic based on the RouteSpec.  If there are missing
 // targets (e.g. Configurations without a Ready Revision, or Revision that isn't Ready or Inactive),
 // no traffic will be configured.
@@ -311,6 +348,29 @@ func (c *Reconciler) configureTraffic(ctx context.Context, r *v1alpha1.Route) (*
 	return t, nil
 }
 
+func (c *Reconciler) ensureFinalizer(route *v1alpha1.Route) error {
+	finalizers := sets.NewString(route.Finalizers...)
+	if finalizers.Has(routeFinalizer) {
+		return nil
+	}
+	finalizers.Insert(routeFinalizer)
+
+	mergePatch := map[string]interface{}{
+		"metadata": map[string]interface{}{
+			"finalizers":      finalizers.List(),
+			"resourceVersion": route.ResourceVersion,
+		},
+	}
+
+	patch, err := json.Marshal(mergePatch)
+	if err != nil {
+		return err
+	}
+
+	_, err = c.ServingClientSet.ServingV1alpha1().Routes(route.Namespace).Patch(route.Name, types.MergePatchType, patch)
+	return err
+}
+
 /////////////////////////////////////////
 // Misc helpers.
 /////////////////////////////////////////

pkg/reconciler/v1alpha1/route/route_test.go

@@ -24,7 +24,6 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
 	duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1"
 	"github.com/knative/pkg/apis/istio/v1alpha3"
 	"github.com/knative/pkg/configmap"
@@ -305,16 +304,6 @@ func TestCreateRouteForOneReserveRevision(t *testing.T) {
 		t.Errorf("Unexpected label diff (-want +got): %v", diff)
 	}
 
-	// Check owner refs
-	expectedRefs := []metav1.OwnerReference{{
-		APIVersion: "serving.knative.dev/v1alpha1",
-		Kind:       "Route",
-		Name:       route.Name,
-	}}
-
-	if diff := cmp.Diff(expectedRefs, ci.OwnerReferences, cmpopts.IgnoreFields(expectedRefs[0], "Controller", "BlockOwnerDeletion")); diff != "" {
-		t.Errorf("Unexpected rule owner refs diff (-want +got): %v", diff)
-	}
 	domain := strings.Join([]string{route.Name, route.Namespace, defaultDomainSuffix}, ".")
 	expectedSpec := netv1alpha1.IngressSpec{
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,