Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

@@ -18,9 +18,11 @@ package clusteringress
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
 
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/knative/pkg/apis/istio/v1alpha3"
@@ -50,6 +52,15 @@ const (
 	controllerAgentName = "clusteringress-controller"
 )
 
+// clusterIngressFinalizer is the name that we put into the resource finalizer list, e.g.
+//  metadata:
+//    finalizers:
+//    - clusteringresses.networking.internal.knative.dev
+var (
+	clusterIngressResource  = v1alpha1.Resource("clusteringresses")
+	clusterIngressFinalizer = clusterIngressResource.String()
+)
+
 type configStore interface {
 	ToContext(ctx context.Context) context.Context
 	WatchConfigs(w configmap.Watcher)
@@ -184,7 +195,7 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.ClusterIngress) (*v1alpha1.C
 func (c *Reconciler) reconcile(ctx context.Context, ci *v1alpha1.ClusterIngress) error {
 	logger := logging.FromContext(ctx)
 	if ci.GetDeletionTimestamp() != nil {
-		return nil
+		return c.reconcileDeletion(ctx, ci)
 	}
 
 	// We may be reading a version of the object that was stored at an older version
@@ -219,7 +230,14 @@ func (c *Reconciler) reconcile(ctx context.Context, ci *v1alpha1.ClusterIngress)
 	// TODO(zhiminx): currently we turn off Gateway reconciliation as it relies
 	// on Istio 1.1, which is not ready.
 	// We should eventually use a feature flag (in ConfigMap) to turn this on/off.
+
 	if c.enableReconcilingGateway {
+		// Add the finalizer before adding `Servers` into Gateway so that we can be sure
+		// the `Servers` get cleaned up from Gateway.
+		if err := c.ensureFinalizer(ci); err != nil {
+			return err
+		}
+
 		desiredServers := resources.MakeServers(ci)
 		if err := c.reconcileGateways(ctx, ci, gatewayNames, desiredServers); err != nil {
 			return err
@@ -325,6 +343,51 @@ func (c *Reconciler) reconcileVirtualService(ctx context.Context, ci *v1alpha1.C
 	return nil
 }
 
+func (c *Reconciler) ensureFinalizer(ci *v1alpha1.ClusterIngress) error {
+	finalizers := sets.NewString(ci.Finalizers...)
+	if finalizers.Has(clusterIngressFinalizer) {
+		return nil
+	}
+
+	mergePatch := map[string]interface{}{
+		"metadata": map[string]interface{}{
+			"finalizers":      append(ci.Finalizers, clusterIngressFinalizer),
+			"resourceVersion": ci.ResourceVersion,
+		},
+	}
+
+	patch, err := json.Marshal(mergePatch)
+	if err != nil {
+		return err
+	}
+
+	_, err = c.ServingClientSet.NetworkingV1alpha1().ClusterIngresses().Patch(ci.Name, types.MergePatchType, patch)
+	return err
+}
+
+func (c *Reconciler) reconcileDeletion(ctx context.Context, ci *v1alpha1.ClusterIngress) error {
+	logger := logging.FromContext(ctx)
+
+	// If our Finalizer is first, delete the `Servers` from Gateway for this ClusterIngress,
+	// and remove the finalizer.
+	if len(ci.Finalizers) == 0 || ci.Finalizers[0] != clusterIngressFinalizer {
+		return nil
+	}
+
+	gatewayNames := gatewayNamesFromContext(ctx, ci)
+	logger.Infof("Cleaning up Gateway Servers for ClusterIngress %s", ci.Name)
+	// No desired Servers means deleting all of the existing Servers associated with the CI.
+	if err := c.reconcileGateways(ctx, ci, gatewayNames, []v1alpha3.Server{}); err != nil {
+		return err
+	}
+
+	// Update the ClusterIngress to remove the Finalizer.
+	logger.Info("Removing Finalizer")
+	ci.Finalizers = ci.Finalizers[1:]
+	_, err := c.ServingClientSet.NetworkingV1alpha1().ClusterIngresses().Update(ci)
+	return err
+}
+
 func (c *Reconciler) reconcileGateways(ctx context.Context, ci *v1alpha1.ClusterIngress, gatewayNames []string, desiredServers []v1alpha3.Server) error {
 	for _, gatewayName := range gatewayNames {
 		if err := c.reconcileGateway(ctx, ci, gatewayName, desiredServers); err != nil {
@@ -337,7 +400,6 @@ func (c *Reconciler) reconcileGateways(ctx context.Context, ci *v1alpha1.Cluster
 func (c *Reconciler) reconcileGateway(ctx context.Context, ci *v1alpha1.ClusterIngress, gatewayName string, desired []v1alpha3.Server) error {
 	// TODO(zhiminx): Need to handle the scenario when deleting ClusterIngress. In this scenario,
 	// the Gateway servers of the ClusterIngress need also be removed from Gateway.
-
 	logger := logging.FromContext(ctx)
 	gateway, err := c.gatewayLister.Gateways(system.Namespace()).Get(gatewayName)
 	if err != nil {

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

@@ -18,6 +18,7 @@ package clusteringress
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -104,7 +105,7 @@ var (
 	ingressTLSServer = v1alpha3.Server{
 		Hosts: []string{"host-tls.example.com"},
 		Port: v1alpha3.Port{
-			Name:     "new-created-clusteringress:0",
+			Name:     "reconciling-clusteringress:0",
 			Number:   443,
 			Protocol: v1alpha3.ProtocolHTTPS,
 		},
@@ -264,23 +265,26 @@ func TestReconcile_Gateway(t *testing.T) {
 		Name:                    "update Gateway to match newly created ClusterIngress",
 		SkipNamespaceValidation: true,
 		Objects: []runtime.Object{
-			ingressWithTLS("new-created-clusteringress", 1234, ingressTLS),
+			ingressWithTLS("reconciling-clusteringress", 1234, ingressTLS),
 			// No Gateway servers match the given TLS of ClusterIngress.
 			gateway("knative-ingress-gateway", system.Namespace(), []v1alpha3.Server{irrelevanteServer}),
 		},
 		WantCreates: []metav1.Object{
 			// The creation of gateways are triggered when setting up the test.
 			gateway("knative-ingress-gateway", system.Namespace(), []v1alpha3.Server{irrelevanteServer}),
 
-			resources.MakeVirtualService(ingress("new-created-clusteringress", 1234),
+			resources.MakeVirtualService(ingress("reconciling-clusteringress", 1234),
 				[]string{"knative-ingress-gateway"}),
 		},
 		WantUpdates: []clientgotesting.UpdateActionImpl{{
 			// ingressTLSServer needs to be added into Gateway.
 			Object: gateway("knative-ingress-gateway", system.Namespace(), []v1alpha3.Server{ingressTLSServer, irrelevanteServer}),
 		}},
+		WantPatches: []clientgotesting.PatchActionImpl{
+			patchAddFinalizerAction("reconciling-clusteringress", clusterIngressFinalizer),
+		},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
-			Object: ingressWithTLSAndStatus("new-created-clusteringress", 1234,
+			Object: ingressWithTLSAndStatus("reconciling-clusteringress", 1234,
 				ingressTLS,
 				v1alpha1.IngressStatus{
 					LoadBalancer: &v1alpha1.LoadBalancerStatus{
@@ -307,22 +311,25 @@ func TestReconcile_Gateway(t *testing.T) {
 			),
 		}},
 		WantEvents: []string{
-			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "new-created-clusteringress"),
+			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "reconciling-clusteringress"),
 			Eventf(corev1.EventTypeNormal, "Updated", "Updated Gateway %q/%q", system.Namespace(), "knative-ingress-gateway"),
 		},
-		Key: "new-created-clusteringress",
+		Key: "reconciling-clusteringress",
 	}, {
 		Name:                    "No preinstalled Gateways",
 		SkipNamespaceValidation: true,
 		Objects: []runtime.Object{
-			ingressWithTLS("new-created-clusteringress", 1234, ingressTLS),
+			ingressWithTLS("reconciling-clusteringress", 1234, ingressTLS),
 		},
 		WantCreates: []metav1.Object{
-			resources.MakeVirtualService(ingress("new-created-clusteringress", 1234),
+			resources.MakeVirtualService(ingress("reconciling-clusteringress", 1234),
 				[]string{"knative-ingress-gateway"}),
 		},
+		WantPatches: []clientgotesting.PatchActionImpl{
+			patchAddFinalizerAction("reconciling-clusteringress", clusterIngressFinalizer),
+		},
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
-			Object: ingressWithTLSAndStatus("new-created-clusteringress", 1234,
+			Object: ingressWithTLSAndStatus("reconciling-clusteringress", 1234,
 				ingressTLS,
 				v1alpha1.IngressStatus{
 					LoadBalancer: &v1alpha1.LoadBalancerStatus{
@@ -349,12 +356,33 @@ func TestReconcile_Gateway(t *testing.T) {
 			),
 		}},
 		WantEvents: []string{
-			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "new-created-clusteringress"),
+			Eventf(corev1.EventTypeNormal, "Created", "Created VirtualService %q", "reconciling-clusteringress"),
 			Eventf(corev1.EventTypeWarning, "InternalError", `gateway.networking.istio.io "knative-ingress-gateway" not found`),
 		},
 		// Error should be returned when there is no preinstalled gateways.
 		WantErr: true,
-		Key:     "new-created-clusteringress",
+		Key:     "reconciling-clusteringress",
+	}, {
+		Name:                    "delete ClusterIngress",
+		SkipNamespaceValidation: true,
+		Objects: []runtime.Object{
+			ingressWithFinalizers("reconciling-clusteringress", 1234, ingressTLS, []string{clusterIngressFinalizer}),
+			gateway("knative-ingress-gateway", system.Namespace(), []v1alpha3.Server{irrelevanteServer, ingressTLSServer}),
+		},
+		WantCreates: []metav1.Object{
+			// The creation of gateways are triggered when setting up the test.
+			gateway("knative-ingress-gateway", system.Namespace(), []v1alpha3.Server{irrelevanteServer, ingressTLSServer}),
+		},
+		WantUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: gateway("knative-ingress-gateway", system.Namespace(), []v1alpha3.Server{irrelevanteServer}),
+		}, {
+			// Finalizer should be removed.
+			Object: ingressWithFinalizers("reconciling-clusteringress", 1234, ingressTLS, []string{}),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated Gateway %q/%q", system.Namespace(), "knative-ingress-gateway"),
+		},
+		Key: "reconciling-clusteringress",
 	}}
 	table.Test(t, MakeFactory(func(listers *Listers, opt reconciler.Options) controller.Reconciler {
 
@@ -410,6 +438,15 @@ func gateway(name, namespace string, servers []v1alpha3.Server) *v1alpha3.Gatewa
 	}
 }
 
+func patchAddFinalizerAction(ingressName, finalizer string) clientgotesting.PatchActionImpl {
+	action := clientgotesting.PatchActionImpl{
+		Name: ingressName,
+	}
+	patch := fmt.Sprintf(`{"metadata":{"finalizers":["%s"],"resourceVersion":"v1"}}`, finalizer)
+	action.Patch = []byte(patch)
+	return action
+}
+
 func addAnnotations(ing *v1alpha1.ClusterIngress, annos map[string]string) *v1alpha1.ClusterIngress {
 	if ing.ObjectMeta.Annotations == nil {
 		ing.ObjectMeta.Annotations = make(map[string]string)
@@ -455,6 +492,7 @@ func ingressWithStatus(name string, generation int64, status v1alpha1.IngressSta
 				serving.RouteLabelKey:          "test-route",
 				serving.RouteNamespaceLabelKey: "test-ns",
 			},
+			ResourceVersion: "v1",
 		},
 		Spec: v1alpha1.IngressSpec{
 			DeprecatedGeneration: generation,
@@ -468,6 +506,13 @@ func ingress(name string, generation int64) *v1alpha1.ClusterIngress {
 	return ingressWithStatus(name, generation, v1alpha1.IngressStatus{})
 }
 
+func ingressWithFinalizers(name string, generation int64, tls []v1alpha1.ClusterIngressTLS, finalizers []string) *v1alpha1.ClusterIngress {
+	ingress := ingressWithTLS(name, generation, tls)
+	ingress.ObjectMeta.Finalizers = finalizers
+	t := metav1.NewTime(time.Unix(1e9, 0))
+	ingress.ObjectMeta.DeletionTimestamp = &t
+	return ingress
+}
 func ingressWithTLS(name string, generation int64, tls []v1alpha1.ClusterIngressTLS) *v1alpha1.ClusterIngress {
 	return ingressWithTLSAndStatus(name, generation, tls, v1alpha1.IngressStatus{})
 }

pkg/reconciler/v1alpha1/clusteringress/resources/gateway.go

@@ -23,9 +23,21 @@ import (
 
 	"github.com/knative/pkg/apis/istio/v1alpha3"
 	"github.com/knative/serving/pkg/apis/networking/v1alpha1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
+// Istio Gateway requires to have at least one server. This placeholderServer is used when
+// all of the real servers are deleted.
+var placeholderServer = v1alpha3.Server{
+	Hosts: []string{"place-holder.place-holder"},
+	Port: v1alpha3.Port{
+		Name:     "place-holder",
+		Number:   9999,
+		Protocol: v1alpha3.ProtocolHTTP,
+	},
+}
+
 // GetServers gets the `Servers` from `Gateway` that belongs to the given ClusterIngress.
 func GetServers(gateway *v1alpha3.Gateway, ci *v1alpha1.ClusterIngress) []v1alpha3.Server {
 	servers := []v1alpha3.Server{}
@@ -92,12 +104,20 @@ func UpdateGateway(gateway *v1alpha3.Gateway, want []v1alpha3.Server, existing [
 		// We remove
 		//  1) the existing servers
 		//  2) the default HTTP server and HTTPS server in the gateway because they are only used for the scenario of not reconciling gateway.
-		if existingServers.Has(server.Port.Name) || isDefaultServer(&server) {
+		//  3) the placeholder servers.
+		if existingServers.Has(server.Port.Name) || isDefaultServer(&server) || isPlaceHolderServer(&server) {
 			continue
 		}
 		servers = append(servers, server)
 	}
 	servers = append(servers, want...)
+
+	// Istio Gateway requires to have at least one server. So if the final gateway does not have any server,
+	// we add "placeholder" server back.
+	if len(servers) == 0 {
+		servers = append(servers, placeholderServer)
+	}
+
 	sortServers(servers)
 	gateway.Spec.Servers = servers
 	return gateway
@@ -106,3 +126,7 @@ func UpdateGateway(gateway *v1alpha3.Gateway, want []v1alpha3.Server, existing [
 func isDefaultServer(server *v1alpha3.Server) bool {
 	return server.Port.Name == "http" || server.Port.Name == "https"
 }
+
+func isPlaceHolderServer(server *v1alpha3.Server) bool {
+	return equality.Semantic.DeepEqual(server, &placeholderServer)
+}