Repository: knative/serving (go)

Issue:
Dropping a tag in a route does not delete cert
<!-- If you need to report a security issue with Knative, send an email to knative-security@googlegroups.com. -->

<!--
## In what area(s)?
Remove the '> ' to select
> /area API
> /area autoscale
> /area build
> /area monitoring
/area networking
> /area test-and-release

Other classifications:
> /kind good-first-issue
> /kind process
> /kind spec
-->



## Expected Behavior

When auto TLS is enabled and user adds a tag to a revision and then drops the tag, the certificate for the tag revision should be created first and then deleted.

## Actual Behavior

After dropping the tag, the certificate is not deleted.

/cc @mattmoor 


Pull request description:
Delete orphaned tag certificates after tag deletion
Fixes #10873

<!-- Please include the 'why' behind your changes if no issue exists -->
## Proposed Changes

* Delete orphaned tag certificates after deleting the corresponding tag.
When using AutoTLS mode, Knative creates certificates for each tag but does not delete them when the corresponding tag gets deleted. This feature only deletes certificates that belong to a route. It will not delete certificates with valid tags if they have a wildcard certificate.

The function works, but as mentioned in the relevant issue, it might be nice to implement a grace period before deletion.

**Release Note**

<!-- Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE". -->

```release-note
NONE
```

Patch:
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index edf5675de..f522e5b49 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -24,6 +24,8 @@ import (
 	"strings"
 	"time"
 
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kubelabels "k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/clock"
@@ -38,9 +40,11 @@ import (
 	networkinglisters "knative.dev/networking/pkg/client/listers/networking/v1alpha1"
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
+	"knative.dev/pkg/controller"
 	"knative.dev/pkg/logging"
 	pkgreconciler "knative.dev/pkg/reconciler"
 	"knative.dev/pkg/tracker"
+	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	clientset "knative.dev/serving/pkg/client/clientset/versioned"
 	routereconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/route"
@@ -259,9 +263,55 @@ func (c *Reconciler) tls(ctx context.Context, host string, r *v1.Route, traffic
 	sort.Slice(acmeChallenges, func(i, j int) bool {
 		return acmeChallenges[i].URL.String() < acmeChallenges[j].URL.String()
 	})
+
+	orphanCerts, err := c.getOrphanRouteCerts(r, domainToTagMap)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	recorder := controller.GetEventRecorder(ctx)
+	for _, cert := range orphanCerts {
+		err = c.GetNetworkingClient().NetworkingV1alpha1().Certificates(cert.Namespace).Delete(ctx, cert.Name, metav1.DeleteOptions{})
+		if err != nil {
+			recorder.Eventf(cert, corev1.EventTypeNormal, "DeleteFailed",
+				"Failed to delete orphaned Knative Certificate %s/%s: %v", cert.Namespace, cert.Name, err)
+		} else {
+			recorder.Eventf(cert, corev1.EventTypeNormal, "Deleted",
+				"Deleted orphaned Knative Certificate %s/%s", cert.Namespace, cert.Name)
+		}
+	}
+
 	return tls, acmeChallenges, nil
 }
 
+// Returns a slice of certificates that used to belong route's old tags and are currently not in use.
+func (c *Reconciler) getOrphanRouteCerts(r *v1.Route, domainToTagMap map[string]string) ([]*netv1alpha1.Certificate, error) {
+	labelSelector := kubelabels.SelectorFromSet(kubelabels.Set{
+		serving.RouteLabelKey: r.Name,
+	})
+
+	certs, err := c.certificateLister.Certificates(r.Namespace).List(labelSelector)
+	if err != nil {
+		return nil, err
+	}
+
+	var unusedCerts []*netv1alpha1.Certificate
+	for _, cert := range certs {
+		var shouldKeepCert bool
+		for _, dn := range cert.Spec.DNSNames {
+			if _, used := domainToTagMap[dn]; used {
+				shouldKeepCert = true
+			}
+		}
+
+		if !shouldKeepCert {
+			unusedCerts = append(unusedCerts, cert)
+		}
+	}
+
+	return unusedCerts, nil
+}
+
 // 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.


Test patch:
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 7b88f0873..44b8756da 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -2583,8 +2583,145 @@ func TestReconcileEnableAutoTLS(t *testing.T) {
 		WantEvents: []string{
 			Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"),
 			Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"),
+			Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34"),
 			Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"),
 		},
+		WantDeletes: []clientgotesting.DeleteActionImpl{{
+			// This certificate's DNS name is not the host name needed by the input Route.
+			ActionImpl: clientgotesting.ActionImpl{
+				Namespace: "default",
+				Verb:      "delete",
+				Resource:  netv1alpha1.SchemeGroupVersion.WithResource("certificates"),
+			},
+			Name: "route-12-34",
+		}},
+		Key: "default/becomes-ready",
+	}, {
+		Name: "check that unused certificate removal works as intended",
+		Objects: []runtime.Object{
+			Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")),
+			cfg("default", "config",
+				WithConfigGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001")),
+			rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001")),
+			&netv1alpha1.Certificate{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "route-12-34",
+					Namespace: "default",
+					OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(
+						Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")))},
+					Labels: map[string]string{
+						serving.RouteLabelKey: "becomes-ready",
+					},
+					Annotations: map[string]string{
+						networking.CertificateClassAnnotationKey: network.CertManagerCertificateClassName,
+					},
+				},
+				Spec: netv1alpha1.CertificateSpec{
+					DNSNames: []string{"abc.test.example.com", "becomes-ready.default.example.com"},
+				},
+				Status: readyCertStatus(),
+			},
+			// This certificate is not required; the route will use the certificate named "route-12-34".
+			// This certificate has a different DNSNames order to test the unused certificate deletion.
+			&netv1alpha1.Certificate{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "route-12-34-keep",
+					Namespace: "default",
+					OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(
+						Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")))},
+					Labels: map[string]string{
+						serving.RouteLabelKey: "becomes-ready",
+					},
+					Annotations: map[string]string{
+						networking.CertificateClassAnnotationKey: network.CertManagerCertificateClassName,
+					},
+				},
+				Spec: netv1alpha1.CertificateSpec{
+					DNSNames: []string{"becomes-ready.default.example.com", "abc.test.example.com"},
+				},
+				Status: readyCertStatus(),
+			},
+			// MakeCertificates will create a certificate with DNS name "abc.test.example.com" which is not the host name
+			// needed by the input Route.
+			&netv1alpha1.Certificate{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "route-12-34-unused",
+					Namespace: "default",
+					OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(
+						Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")))},
+					Labels: map[string]string{
+						serving.RouteLabelKey: "becomes-ready",
+					},
+					Annotations: map[string]string{
+						networking.CertificateClassAnnotationKey: network.CertManagerCertificateClassName,
+					},
+				},
+				Spec: netv1alpha1.CertificateSpec{
+					DNSNames: []string{"abc.test.example.com"},
+				},
+				Status: readyCertStatus(),
+			},
+		},
+		WantCreates: []runtime.Object{
+			ingressWithTLS(
+				Route("default", "becomes-ready", WithConfigTarget("config"), WithURL,
+					WithRouteUID("12-34")),
+				&traffic.Config{
+					Targets: map[string]traffic.RevisionTargets{
+						traffic.DefaultTarget: {{
+							TrafficTarget: v1.TrafficTarget{
+								ConfigurationName: "config",
+								LatestRevision:    ptr.Bool(true),
+								RevisionName:      "config-00001",
+								Percent:           ptr.Int64(100),
+							},
+						}},
+					},
+				},
+				[]netv1alpha1.IngressTLS{{
+					Hosts:           []string{"becomes-ready.default.example.com"},
+					SecretName:      "route-12-34",
+					SecretNamespace: "default",
+				}},
+				nil,
+			),
+			simpleK8sService(
+				Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")),
+				WithExternalName("becomes-ready.default.example.com"),
+			),
+		},
+		WantUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: certificateWithStatus(resources.MakeCertificates(Route("default", "becomes-ready", WithConfigTarget("config"), WithURL, WithRouteUID("12-34")),
+				map[string]string{"becomes-ready.default.example.com": ""}, network.CertManagerCertificateClassName)[0], readyCertStatus()),
+		}},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: Route("default", "becomes-ready", WithConfigTarget("config"),
+				WithRouteUID("12-34"),
+				WithAddress, WithInitRouteConditions,
+				MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00001",
+						Percent:        ptr.Int64(100),
+						LatestRevision: ptr.Bool(true),
+					}), MarkCertificateReady,
+				// The certificate is ready. So we want to have HTTPS URL.
+				WithHTTPSDomain),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"),
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"),
+			Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34-unused"),
+			Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"),
+		},
+		WantDeletes: []clientgotesting.DeleteActionImpl{{
+			// This certificate's DNS name is not the host name needed by the input Route.
+			ActionImpl: clientgotesting.ActionImpl{
+				Namespace: "default",
+				Verb:      "delete",
+				Resource:  netv1alpha1.SchemeGroupVersion.WithResource("certificates"),
+			},
+			Name: "route-12-34-unused",
+		}},
 		Key: "default/becomes-ready",
 	}, {
 		Name: "verify ingress rules created for http01 challenges",
@@ -2796,8 +2933,18 @@ func TestReconcileEnableAutoTLS(t *testing.T) {
 		WantEvents: []string{
 			Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"),
 			Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"),
+			Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34"),
 			Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"),
 		},
+		WantDeletes: []clientgotesting.DeleteActionImpl{{
+			// This certificate's DNS name is not the host name needed by the input Route.
+			ActionImpl: clientgotesting.ActionImpl{
+				Namespace: "default",
+				Verb:      "delete",
+				Resource:  netv1alpha1.SchemeGroupVersion.WithResource("certificates"),
+			},
+			Name: "route-12-34",
+		}},
 		Key: "default/becomes-ready",
 	}, {
 		// This test is a same with "public becomes cluster local" above, but confirm it does not create certs with autoTLS for cluster-local.