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

pkg/apis/serving/v1/route_lifecycle.go

@@ -166,10 +166,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

pkg/apis/serving/v1/route_lifecycle_test.go

@@ -428,54 +428,85 @@ 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)
-}
+	cases := []struct {
+		name   string
+		cert   *netv1alpha1.Certificate
+		status corev1.ConditionStatus
 
-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",
-					},
+		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,
+					}},
 				},
 			},
 		},
-	}
-
-	r := &RouteStatus{}
-	r.InitializeConditions()
-	r.MarkCertificateNotReady(cert)
+		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,
+	}}
 
-	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) {

pkg/reconciler/route/route.go

@@ -245,7 +245,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
 			}
@@ -280,7 +280,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.
@@ -326,7 +330,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
 		}
@@ -342,6 +346,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)
 		}

pkg/reconciler/route/table_test.go

@@ -3128,6 +3128,98 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) {
 			Name: "route-12-34",
 		}},
 		Key: "default/becomes-ready",
+	}, {
+		Name: "check that Route is correctly updated when Certificate is failed",
+		Objects: []runtime.Object{
+			Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34"), WithRouteGeneration(1),
+				// Populated by reconciliation when all traffic has been assigned.
+				WithAddress,
+				MarkTrafficAssigned,
+				WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00001",
+						Percent:        ptr.Int64(100),
+						LatestRevision: ptr.Bool(true),
+					}),
+				// The certificate is not ready. So we want to have HTTP URL.
+				WithInitRouteConditions,
+				MarkTrafficAssigned,
+				WithRouteObservedGeneration,
+				MarkCertificateProvisionFailed,
+				WithAddress,
+				MarkIngressReady,
+				WithURL,
+			),
+			cfg("default", "config",
+				WithConfigGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001")),
+			rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001")),
+			simpleK8sService(
+				Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")),
+				WithExternalName(pkgnet.GetServiceHostname("private-istio-ingressgateway", "istio-system")),
+			),
+			ingressWithTLS(
+				Route("default", "becomes-ready", WithConfigTarget("config"), WithURL,
+					WithRouteUID("12-34"), WithRouteGeneration(1)),
+				&traffic.Config{
+					Targets: map[string]traffic.RevisionTargets{
+						traffic.DefaultTarget: {{
+							TrafficTarget: v1.TrafficTarget{
+								LatestRevision:    ptr.Bool(true),
+								ConfigurationName: "config",
+								RevisionName:      "config-00001",
+								Percent:           ptr.Int64(100),
+							},
+						}},
+					},
+				}, nil, nil, withReadyIngress),
+			// MakeCertificates will create a certificate with DNS name "*.test-ns.example.com" which is not the host name
+			// needed by the input Route.
+			&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",
+						netapi.CertificateTypeLabelKey: string(netcfg.CertificateExternalDomain),
+					},
+					Annotations: map[string]string{
+						netapi.CertificateClassAnnotationKey: netcfg.CertManagerCertificateClassName,
+					},
+				},
+				Spec: netv1alpha1.CertificateSpec{
+					Domain:     "example.com",
+					DNSNames:   []string{"becomes-ready.default.example.com"},
+					SecretName: "route-12-34",
+				},
+				Status: failedCertStatus(),
+			},
+		},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: Route("default", "becomes-ready", WithConfigTarget("config"),
+				WithRouteUID("12-34"), WithRouteGeneration(1), WithRouteObservedGeneration,
+				// Populated by reconciliation when all traffic has been assigned.
+				WithAddress,
+				WithRouteConditionsHTTPDowngrade,
+				MarkTrafficAssigned,
+				WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00001",
+						Percent:        ptr.Int64(100),
+						LatestRevision: ptr.Bool(true),
+					}),
+				// The certificate is failed. So we want to have HTTP URL.
+				WithInitRouteConditions,
+				MarkTrafficAssigned,
+				WithRouteObservedGeneration,
+				WithRouteConditionsHTTPDowngrade,
+				WithAddress,
+				MarkIngressReady,
+				WithURL,
+			),
+		}},
+		Key: "default/becomes-ready",
 	}, {
 		// This test is a same with "public becomes cluster local" above, but confirm it does not create certs with external-domain-tls for cluster-local.
 		Name: "public becomes cluster local w/ external-domain-tls",
@@ -3480,6 +3572,12 @@ func notReadyCertStatus() netv1alpha1.CertificateStatus {
 	return *certStatus
 }
 
+func failedCertStatus() netv1alpha1.CertificateStatus {
+	certStatus := &netv1alpha1.CertificateStatus{}
+	certStatus.MarkFailed("failed", "failed")
+	return *certStatus
+}
+
 func certificateWithStatus(cert *netv1alpha1.Certificate, status netv1alpha1.CertificateStatus) *netv1alpha1.Certificate {
 	cert.Status = status
 	return cert

pkg/testing/v1/route.go

@@ -203,6 +203,11 @@ func MarkCertificateNotReady(r *v1.Route) {
 	r.Status.MarkCertificateNotReady(&netv1alpha1.Certificate{})
 }
 
+// MarkCertificateProvisionFailed calls the method of the same name on .Status
+func MarkCertificateProvisionFailed(r *v1.Route) {
+	r.Status.MarkCertificateProvisionFailed(&netv1alpha1.Certificate{})
+}
+
 // MarkCertificateNotOwned calls the method of the same name on .Status
 func MarkCertificateNotOwned(r *v1.Route) {
 	r.Status.MarkCertificateNotOwned(routenames.Certificate(r))