From 8d86ca385b0076981fd9ebe1abb7157d009080c8 Mon Sep 17 00:00:00 2001 From: ckcd Date: Tue, 12 Mar 2024 13:41:58 +0800 Subject: [PATCH] feat: bubble up kcert status message when it's failed Signed-off-by: ckcd --- pkg/apis/serving/v1/route_lifecycle.go | 5 +- pkg/apis/serving/v1/route_lifecycle_test.go | 111 +++++++++++++------- pkg/reconciler/route/route.go | 12 ++- pkg/reconciler/route/table_test.go | 98 +++++++++++++++++ pkg/testing/v1/route.go | 5 + 5 files changed, 186 insertions(+), 45 deletions(-) diff --git a/pkg/apis/serving/v1/route_lifecycle.go b/pkg/apis/serving/v1/route_lifecycle.go index b577fca0d41a..35492cd27f98 100644 --- a/pkg/apis/serving/v1/route_lifecycle.go +++ b/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 diff --git a/pkg/apis/serving/v1/route_lifecycle_test.go b/pkg/apis/serving/v1/route_lifecycle_test.go index 8e484da50311..6614a13a8658 100644 --- a/pkg/apis/serving/v1/route_lifecycle_test.go +++ b/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) { diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go index 3eee16f52019..ccff359b4ac2 100644 --- a/pkg/reconciler/route/route.go +++ b/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) } diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go index 5b62e7bab56b..9482b375d166 100644 --- a/pkg/reconciler/route/table_test.go +++ b/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 diff --git a/pkg/testing/v1/route.go b/pkg/testing/v1/route.go index 0b73d1c94142..f1d26d55b561 100644 --- a/pkg/testing/v1/route.go +++ b/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))