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

knative · Feb 29, 2024 · 2fc98c9 · 2fc98c9
1 parent a1ad60a
commit 2fc98c9
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 9 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
@@ -451,8 +451,8 @@ func TestCertificateNotReadyWithBubbledUpMessage(t *testing.T) {
 				Conditions: duckv1.Conditions{
 					{
 						Type:   "Ready",
-						Status: "False",
-						Reason: "CommonName Too Long",
+						Status: "Unknown",
+						Reason: "The ready condition of Cert Manager Certificate does not exist.",
 					},
 				},
 			},
@@ -463,7 +463,7 @@ func TestCertificateNotReadyWithBubbledUpMessage(t *testing.T) {
 	r.InitializeConditions()
 	r.MarkCertificateNotReady(cert)
 
-	expectedCertMessage := "CommonName Too Long"
+	expectedCertMessage := "The ready condition of Cert Manager Certificate does not exist."
 	certMessage := r.Status.GetCondition("Ready").Message
 	if !strings.Contains(certMessage, expectedCertMessage) {
 		t.Errorf("Literal %q not found in status message: %q", expectedCertMessage, certMessage)
@@ -473,11 +473,37 @@ func TestCertificateNotReadyWithBubbledUpMessage(t *testing.T) {
 func TestCertificateProvisionFailed(t *testing.T) {
 	r := &RouteStatus{}
 	r.InitializeConditions()
-	r.MarkCertificateProvisionFailed("cert")
+	r.MarkCertificateProvisionFailed(&netv1alpha1.Certificate{})
 
 	apistest.CheckConditionFailed(r, RouteConditionCertificateProvisioned, t)
 }
 
+func TestCertificateFailedWithBubbledUpMessage(t *testing.T) {
+	cert := &netv1alpha1.Certificate{
+		Status: netv1alpha1.CertificateStatus{
+			Status: duckv1.Status{
+				Conditions: duckv1.Conditions{
+					{
+						Type:   "Ready",
+						Status: "False",
+						Reason: "CommonName Too Long",
+					},
+				},
+			},
+		},
+	}
+
+	r := &RouteStatus{}
+	r.InitializeConditions()
+	r.MarkCertificateProvisionFailed(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)
+	}
+}
+
 func TestRouteNotOwnCertificate(t *testing.T) {
 	r := &RouteStatus{}
 	r.InitializeConditions()

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)
 		}