Bubble up KCertificate Status Message when its not ready (#14496)

gabo1208 · dprotaso · web-flow · commit f69766c10adc · 2023-10-20T17:38:11.000Z
* basic change done kcert error bubbling up correctly, still need to fail when url is too long

* Update pkg/apis/serving/v1/route_lifecycle.go

Co-authored-by: Dave Protasowski &lt;dprotaso@gmail.com&gt;

---------

Co-authored-by: Dave Protasowski &lt;dprotaso@gmail.com&gt;
diff --git a/pkg/apis/serving/v1/route_lifecycle.go b/pkg/apis/serving/v1/route_lifecycle.go
@@ -174,10 +174,11 @@ func (rs *RouteStatus) MarkCertificateReady(name string) {
 
 // MarkCertificateNotReady marks the RouteConditionCertificateProvisioned
 // condition to indicate that the Certificate is not ready.
-func (rs *RouteStatus) MarkCertificateNotReady(name string) {
+func (rs *RouteStatus) MarkCertificateNotReady(c *v1alpha1.Certificate) {
+	certificateCondition := c.Status.GetCondition("Ready")
 	routeCondSet.Manage(rs).MarkUnknown(RouteConditionCertificateProvisioned,
 		"CertificateNotReady",
-		"Certificate %s is not ready.", name)
+		"Certificate %s is not ready: %s", c.Name, certificateCondition.GetReason())
 }
 
 // MarkCertificateNotOwned changes the RouteConditionCertificateProvisioned
diff --git a/pkg/apis/serving/v1/route_lifecycle_test.go b/pkg/apis/serving/v1/route_lifecycle_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package v1
 
 import (
+	"strings"
 	"testing"
 	"time"
 
@@ -438,11 +439,37 @@ func TestCertificateReady(t *testing.T) {
 func TestCertificateNotReady(t *testing.T) {
 	r := &RouteStatus{}
 	r.InitializeConditions()
-	r.MarkCertificateNotReady("cert")
+	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",
+					},
+				},
+			},
+		},
+	}
+
+	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)
+	}
+}
+
 func TestCertificateProvisionFailed(t *testing.T) {
 	r := &RouteStatus{}
 	r.InitializeConditions()
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
@@ -265,7 +265,7 @@ func (c *Reconciler) tls(ctx context.Context, host string, r *v1.Route, traffic
 			tls = append(tls, resources.MakeIngressTLS(cert, dnsNames.List()))
 		} else {
 			acmeChallenges = append(acmeChallenges, cert.Status.HTTP01Challenges...)
-			r.Status.MarkCertificateNotReady(cert.Name)
+			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.
diff --git a/pkg/testing/v1/route.go b/pkg/testing/v1/route.go
@@ -208,7 +208,7 @@ func MarkUnknownTrafficError(msg string) RouteOption {
 
 // MarkCertificateNotReady calls the method of the same name on .Status
 func MarkCertificateNotReady(r *v1.Route) {
-	r.Status.MarkCertificateNotReady(routenames.Certificate(r))
+	r.Status.MarkCertificateNotReady(&netv1alpha1.Certificate{})
 }
 
 // MarkCertificateNotOwned calls the method of the same name on .Status

Original file line number	Diff line number	Diff line change
`@@ -208,7 +208,7 @@ func MarkUnknownTrafficError(msg string) RouteOption {`
`208`	`208`
`209`	`209`	`// MarkCertificateNotReady calls the method of the same name on .Status`
`210`	`210`	`func MarkCertificateNotReady(r *v1.Route) {`
`211`		`- r.Status.MarkCertificateNotReady(routenames.Certificate(r))`
	`211`	`+ r.Status.MarkCertificateNotReady(&netv1alpha1.Certificate{})`
`212`	`212`	`}`
`213`	`213`
`214`	`214`	`// MarkCertificateNotOwned calls the method of the same name on .Status`