Skip to content

Commit

Permalink
feat: bubble up kcert status message when it's failed
Browse files Browse the repository at this point in the history 
Signed-off-by: ckcd <curtis@mail.ustc.edu.cn>
  • Loading branch information
@ckcd
ckcd committed Mar 28, 2024
1 parent 231db15 commit 250ab9c
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 45 deletions.
5 changes: 3 additions & 2 deletions pkg/apis/serving/v1/route_lifecycle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
111 changes: 71 additions & 40 deletions pkg/apis/serving/v1/route_lifecycle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 9 additions & 3 deletions pkg/reconciler/route/route.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
}
Expand Down
90 changes: 90 additions & 0 deletions pkg/reconciler/route/table_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3127,6 +3127,90 @@ 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)),
cfg("default", "config",
WithConfigGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001")),
rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001")),
// 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{
DNSNames: []string{"abc.test.example.com"},
},
Status: failedCertStatus(),
},
},
WantCreates: []runtime.Object{
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),
simpleK8sService(
Route("default", "becomes-ready", WithConfigTarget("config"), WithRouteUID("12-34")),
WithExternalName("becomes-ready.default.example.com"),
),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: certificateWithStatus(resources.MakeCertificates(Route("default", "becomes-ready", WithConfigTarget("config"), WithURL, WithRouteUID("12-34")),
map[string]string{"becomes-ready.default.example.com": ""}, netcfg.CertManagerCertificateClassName, "example.com")[0], 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, MarkIngressNotConfigured, WithStatusTraffic(
v1.TrafficTarget{
RevisionName: "config-00001",
Percent: ptr.Int64(100),
LatestRevision: ptr.Bool(true),
}), MarkIngressNotConfigured,
// The certificate is not ready. So we want to have HTTP URL.
WithURL),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "Created", "Created placeholder service %q", "becomes-ready"),
Eventf(corev1.EventTypeNormal, "Updated", "Updated Spec for Certificate %s/%s", "default", "route-12-34"),
Eventf(corev1.EventTypeNormal, "Deleted", "Deleted orphaned Knative Certificate %s/%s", "default", "route-12-34"),
Eventf(corev1.EventTypeNormal, "Created", "Created Ingress %q", "becomes-ready"),
},
WantDeletes: []clientgotesting.DeleteActionImpl{{
// This certificate's DNS name is not the host name needed by the input Route.
ActionImpl: clientgotesting.ActionImpl{
Namespace: "default",
Verb: "delete",
Resource: netv1alpha1.SchemeGroupVersion.WithResource("certificates"),
},
Name: "route-12-34",
}},
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",
Expand Down Expand Up @@ -3479,6 +3563,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
Expand Down

0 comments on commit 250ab9c

Please sign in to comment.