feat: bubble up kcert status message when it's failed

knative · Mar 12, 2024 · 24f0f45 · 24f0f45
1 parent d8b4c5c
commit 24f0f45
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 44 deletions.
diff --git a/pkg/apis/serving/v1/route_lifecycle.go b/pkg/apis/serving/v1/route_lifecycle.go
@@ -160,10 +160,11 @@ func (rs *RouteStatus) MarkMissingTrafficTarget(kind, name string) {
 // MarkCertificateProvisionFailed marks the
 // RouteConditionCertificateProvisioned condition to indicate that the
 // Certificate provisioning failed.
-func (rs *RouteStatus) MarkCertificateProvisionFailed(name string) {
+func (rs *RouteStatus) MarkCertificateProvisionFailed(c *v1alpha1.Certificate) {
+	certificateCondition := c.Status.GetCondition(apis.ConditionReady)
 	routeCondSet.Manage(rs).MarkFalse(RouteConditionCertificateProvisioned,
 		"CertificateProvisionFailed",
-		"Certificate %s fails to be provisioned.", name)
+		"Certificate %s fails to be provisioned: %s", c.Name, certificateCondition.GetReason())
 }
 
 // MarkCertificateReady marks the RouteConditionCertificateProvisioned

diff --git a/pkg/apis/serving/v1/route_lifecycle_test.go b/pkg/apis/serving/v1/route_lifecycle_test.go
@@ -428,54 +428,95 @@ func TestRouteNotOwnedStuff(t *testing.T) {
 	apistest.CheckConditionFailed(r, RouteConditionReady, t)
 }
 
-func TestCertificateReady(t *testing.T) {
-	r := &RouteStatus{}
-	r.InitializeConditions()
-	r.MarkCertificateReady("cert")
-
-	apistest.CheckConditionSucceeded(r, RouteConditionCertificateProvisioned, t)
-}
+func TestCertificateProvision(t *testing.T) {
+	message := "CommonName Too Long"
 
-func TestCertificateNotReady(t *testing.T) {
-	r := &RouteStatus{}
-	r.InitializeConditions()
-	r.MarkCertificateNotReady(&netv1alpha1.Certificate{})
-
-	apistest.CheckConditionOngoing(r, RouteConditionCertificateProvisioned, t)
-}
-
-func TestCertificateNotReadyWithBubbledUpMessage(t *testing.T) {
-	cert := &netv1alpha1.Certificate{
-		Status: netv1alpha1.CertificateStatus{
-			Status: duckv1.Status{
-				Conditions: duckv1.Conditions{
-					{
-						Type:   "Ready",
-						Status: "False",
-						Reason: "CommonName Too Long",
+	cases := []struct {
+		name   string
+		cert   *netv1alpha1.Certificate
+		status corev1.ConditionStatus
+
+		wantMessage string
+	}{
+		{
+			name:        "Ready with empty message",
+			cert:        &netv1alpha1.Certificate{},
+			status:      corev1.ConditionTrue,
+			wantMessage: "",
+		},
+		{
+			name:        "NotReady with empty message",
+			cert:        &netv1alpha1.Certificate{},
+			status:      corev1.ConditionUnknown,
+			wantMessage: "",
+		},
+		{
+			name: "NotReady with bubbled up message",
+			cert: &netv1alpha1.Certificate{
+				Status: netv1alpha1.CertificateStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{
+								Type:   apis.ConditionReady,
+								Status: corev1.ConditionUnknown,
+								Reason: message,
+							},
+						},
+					},
+				},
+			},
+			status:      corev1.ConditionUnknown,
+			wantMessage: message,
+		},
+		{
+			name:        "Failed with empty message",
+			cert:        &netv1alpha1.Certificate{},
+			status:      corev1.ConditionFalse,
+			wantMessage: "",
+		},
+		{
+			name: "Failed with bubbled up message",
+			cert: &netv1alpha1.Certificate{
+				Status: netv1alpha1.CertificateStatus{
+					Status: duckv1.Status{
+						Conditions: duckv1.Conditions{
+							{
+								Type:   apis.ConditionReady,
+								Status: corev1.ConditionFalse,
+								Reason: message,
+							},
+						},
 					},
 				},
 			},
+			status:      corev1.ConditionFalse,
+			wantMessage: message,
 		},
 	}
 
-	r := &RouteStatus{}
-	r.InitializeConditions()
-	r.MarkCertificateNotReady(cert)
-
-	expectedCertMessage := "CommonName Too Long"
-	certMessage := r.Status.GetCondition("Ready").Message
-	if !strings.Contains(certMessage, expectedCertMessage) {
-		t.Errorf("Literal %q not found in status message: %q", expectedCertMessage, certMessage)
-	}
-}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			r := &RouteStatus{}
+			r.InitializeConditions()
+
+			if tc.status == corev1.ConditionTrue {
+				r.MarkCertificateReady(tc.cert.Name)
+			} else if tc.status == corev1.ConditionFalse {
+				r.MarkCertificateProvisionFailed(tc.cert)
+			} else {
+				r.MarkCertificateNotReady(tc.cert)
+			}
 
-func TestCertificateProvisionFailed(t *testing.T) {
-	r := &RouteStatus{}
-	r.InitializeConditions()
-	r.MarkCertificateProvisionFailed("cert")
+			if err := apistest.CheckCondition(r, RouteConditionCertificateProvisioned, tc.status); err != nil {
+				t.Error(err)
+			}
 
-	apistest.CheckConditionFailed(r, RouteConditionCertificateProvisioned, t)
+			certMessage := r.Status.GetCondition(apis.ConditionReady).Message
+			if !strings.Contains(certMessage, tc.wantMessage) {
+				t.Errorf("Literal %q not found in status message: %q", tc.wantMessage, certMessage)
+			}
+		})
+	}
 }
 
 func TestRouteNotOwnCertificate(t *testing.T) {

diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
@@ -239,7 +239,7 @@ func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.R
 				if kaccessor.IsNotOwned(err) {
 					r.Status.MarkCertificateNotOwned(desiredCert.Name)
 				} else {
-					r.Status.MarkCertificateProvisionFailed(desiredCert.Name)
+					r.Status.MarkCertificateProvisionFailed(desiredCert)
 				}
 				return nil, nil, err
 			}
@@ -274,7 +274,11 @@ func (c *Reconciler) externalDomainTLS(ctx context.Context, host string, r *v1.R
 			tls = append(tls, resources.MakeIngressTLS(cert, sets.List(dnsNames)))
 		} else {
 			acmeChallenges = append(acmeChallenges, cert.Status.HTTP01Challenges...)
-			r.Status.MarkCertificateNotReady(cert)
+			if cert.IsFailed() {
+				r.Status.MarkCertificateProvisionFailed(cert)
+			} else {
+				r.Status.MarkCertificateNotReady(cert)
+			}
 			// When httpProtocol is enabled, downgrade http scheme.
 			// Explicitly not using the override settings here as to not to muck with
 			// external-domain-tls semantics.
@@ -320,7 +324,7 @@ func (c *Reconciler) clusterLocalDomainTLS(ctx context.Context, r *v1.Route, tc
 			if kaccessor.IsNotOwned(err) {
 				r.Status.MarkCertificateNotOwned(desiredCert.Name)
 			} else {
-				r.Status.MarkCertificateProvisionFailed(desiredCert.Name)
+				r.Status.MarkCertificateProvisionFailed(desiredCert)
 			}
 			return nil, err
 		}
@@ -336,6 +340,8 @@ func (c *Reconciler) clusterLocalDomainTLS(ctx context.Context, r *v1.Route, tc
 
 			r.Status.MarkCertificateReady(cert.Name)
 			tls = append(tls, resources.MakeIngressTLS(cert, sets.List(localDomains)))
+		} else if cert.IsFailed() {
+			r.Status.MarkCertificateProvisionFailed(cert)
 		} else {
 			r.Status.MarkCertificateNotReady(cert)
 		}