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

ckcd · 2024-02-29T06:21:29Z

Proposed Changes

KCert Status Message gets bubbled up to its parent Route when it's failed

Release Note

Certificate generation errors are bubbled up to its parent Route.

knative-prow · 2024-02-29T06:21:39Z

Welcome @ckcd! It looks like this is your first PR to knative/serving 🎉

knative-prow · 2024-02-29T06:21:39Z

Hi @ckcd. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ckcd · 2024-02-29T06:29:39Z

@ReToCode @gabo1208 PTAL, thanks.

izabelacg · 2024-03-04T17:38:41Z

/ok-to-test

Cali0707 · 2024-03-05T16:14:26Z

/ok-to-test

codecov · 2024-03-05T16:19:58Z

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 84.95%. Comparing base (c2d0af1) to head (8d86ca3).
Report is 62 commits behind head on main.

Files	Patch %	Lines
pkg/reconciler/route/route.go	50.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14962      +/-   ##
==========================================
+ Coverage   84.11%   84.95%   +0.84%     
==========================================
  Files         213      213              
  Lines       16783    13115    -3668     
==========================================
- Hits        14117    11142    -2975     
+ Misses       2315     1618     -697     
- Partials      351      355       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ckcd · 2024-03-11T08:20:03Z

@izabelacg @Cali0707 @ReToCode Could please help to look at this PR? thanks.

ReToCode · 2024-03-11T10:33:46Z

pkg/apis/serving/v1/route_lifecycle_test.go

-						Status: "False",
-						Reason: "CommonName Too Long",
+						Status: "Unknown",
+						Reason: "The ready condition of Cert Manager Certificate does not exist.",


This message does not say much (not saying the old one was much better).

ReToCode · 2024-03-11T10:33:53Z

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",


Why unknown instead of false?

Just want to set up different tests for different states.

Ok, I think I was also confused because the reason message here was moved to the other tests. What about extracting the message outside of the test and re-use it in both tests? It seems like you want to test for the actual states (failed/ready) and the message just has to be there.

@ReToCode sure, will do this change. Thanks for your idea.

Awesome, way better now.

ReToCode · 2024-03-11T10:35:43Z

pkg/apis/serving/v1/route_lifecycle_test.go


 	apistest.CheckConditionFailed(r, RouteConditionCertificateProvisioned, t)
 }

+func TestCertificateFailedWithBubbledUpMessage(t *testing.T) {


I don't think I get the difference between these two tests. We mark the certificate provisioning as failed in both cases. Could you please elaborate the idea behind it?

As @gabo1208 said in #14530, the difference is Status=Unkown is a status in progress which means it's just not ready for now, and the Status=False is a final state with failure.

ReToCode · 2024-03-11T10:38:23Z

As API is also part of this:

/cc @dprotaso

ReToCode

/lgtm
/approve
/hold for @dprotaso

ckcd · 2024-03-12T06:57:37Z

Seems test failed because of data race for some irrelevant tests.
https://github.com/knative/serving/actions/runs/8244156246/job/22547213240?pr=14962

I will rebase code and re-trigger the tests.

--

Now all successful.

ckcd · 2024-03-21T02:16:29Z

@dprotaso could you please take a look at this? thanks.

dprotaso · 2024-03-26T21:21:35Z

closing/reopening so that the PR runs against the latest commit in main (not this is only for github actions - prow is smart about this)

ckcd · 2024-03-27T05:07:29Z

@dprotaso seems all ut flow passed now. please check it, thanks.

ReToCode · 2024-03-27T07:40:51Z

/assign @dprotaso

ckcd · 2024-03-27T08:28:14Z

/retest

dprotaso

changes look good - just a few minor things

dprotaso · 2024-03-27T21:24:59Z

pkg/apis/serving/v1/route_lifecycle_test.go

+	}{
+		{
+			name:        "Ready with empty message",
+			cert:        &netv1alpha1.Certificate{},
+			status:      corev1.ConditionTrue,
+			wantMessage: "",
+		},


Do you mind compacting this to match the list style we use in other parts of the repo

see:

serving/pkg/apis/serving/v1/route_lifecycle_test.go

Lines 78 to 83 in 315a94c

isReady bool

}{{

name: "empty status should not be ready",

status: RouteStatus{},

isReady: false,

}, {

Sure, thanks for your suggestion. will do this change.

pkg/reconciler/route/route.go

ckcd · 2024-03-28T14:40:11Z

@dprotaso Thanks for your reply! I made some changes according to your idea, please take a look. Thx~

dprotaso

thanks for the test - some comments

dprotaso · 2024-03-30T17:16:17Z

pkg/reconciler/route/table_test.go

+		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"),
+			),
+		},


I wouldn't expect creates in this scenario since we just want to test route propagation. I'd suggest moving these to the Objects list

dprotaso · 2024-03-30T17:27:34Z

pkg/reconciler/route/table_test.go

+			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()),
+		}},


Certificate shouldn't be updated. What's in Objects should be the updated certificate

dprotaso · 2024-03-30T17:30:36Z

pkg/reconciler/route/table_test.go

+			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),


The route should be in failed state but I don't see that here

dprotaso · 2024-03-30T17:31:24Z

pkg/reconciler/route/table_test.go

+			// 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"),


This action (deleting the certificate seems unexpected) I'm wondering if we are setting up the test correctly. I wouldn't expect the certificate to be deleted.

@dprotaso actually for this check that Route is correctly updated when Certificate is not ready test case, it also has above WantUpdates, WantDeletes and so on.

And for both not ready and failed in this table_test.go it's hard to check the route status = notReady/failed because in this line it will override the condition with HTTPDowngrade info. That's because we use HTTPProtocol=HTTPEnabled when init NewTestReconciler.

actually for this check that Route is correctly updated when Certificate is not ready test case, it also has above WantUpdates, WantDeletes and so on.

Yeah two things - some of the fixtures are off. Meaning they don't really reflect the state a resource can be in. Secondly, each test case varies from whichi point in time a reconciliation occurs so the starting fixtures can be different.

In the scenario where the certificate provisioning has failed. I would expect we were already in a a 'steady state' where all the objects have been created and updated but the only the certificate has been marked as 'failed'

Thus to patch your code the fixtures for this scenario would be

diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go index 52080a089..809690182 100644 --- a/pkg/reconciler/route/table_test.go +++ b/pkg/reconciler/route/table_test.go @@ -3130,10 +3130,47 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { }, { 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)), + 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, + MarkCertificateNotReady, + 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{ @@ -3151,64 +3188,35 @@ func TestReconcileEnableExternalDomainTLS(t *testing.T) { }, }, Spec: netv1alpha1.CertificateSpec{ - DNSNames: []string{"abc.test.example.com"}, + Domain: "example.com", + DNSNames: []string{"becomes-ready.default.example.com"}, + SecretName: "route-12-34", }, 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( + WithAddress, + WithRouteConditionsHTTPDowngrade, + MarkTrafficAssigned, + 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", + WithInitRouteConditions, + MarkTrafficAssigned, + WithRouteObservedGeneration, + WithRouteConditionsHTTPDowngrade, + WithAddress, + MarkIngressReady, + WithURL, + ), }}, Key: "default/becomes-ready", }, {

Here we have no updates to resources and nothing being created - but we see the route's status having the HTTPDowngrade.

Unsure if you wanted to disable the HTTPDowngrade option.

Also I wouldn't expect the certificate to be deleted - I think that's because the fixture wasn't accurate.

Hey @ckcd just following up here

@dprotaso Thanks for your analysis! And sorry for my late reply.

I made some changes for the test case according to your suggestions. And also add a MarkCertificateProvisionFailed for testing. Please check it. Thanks.

Signed-off-by: ckcd <curtis@mail.ustc.edu.cn>

dprotaso · 2024-04-23T01:59:13Z

/lgtm
/approve

thanks @ckcd

knative-prow · 2024-04-23T01:59:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckcd, dprotaso, ReToCode

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ReToCode,dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dprotaso · 2024-04-23T02:09:06Z

/hold cancel

dprotaso · 2024-04-23T02:10:17Z

/override "style / suggester / shell"
/override "style / suggester / yaml"
/override "style / suggester / github_actions"

knative-prow · 2024-04-23T02:10:21Z

@dprotaso: Overrode contexts on behalf of dprotaso: style / suggester / github_actions, style / suggester / shell, style / suggester / yaml

In response to this:

/override "style / suggester / shell"
/override "style / suggester / yaml"
/override "style / suggester / github_actions"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ckcd · 2024-04-23T06:19:10Z

@dprotaso Thank you for your patience and help!!

dprotaso · 2024-04-23T18:36:44Z

likewise 🎉

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 29, 2024

knative-prow bot requested review from izabelacg and ReToCode February 29, 2024 06:21

knative-prow bot added area/API API objects and controllers area/networking labels Feb 29, 2024

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from 0ea19a9 to 790f52d Compare February 29, 2024 06:27

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch 2 times, most recently from 29ca43c to 2fc98c9 Compare February 29, 2024 07:52

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 5, 2024

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from 2fc98c9 to fefef66 Compare March 6, 2024 15:35

ReToCode reviewed Mar 11, 2024

View reviewed changes

knative-prow bot requested a review from dprotaso March 11, 2024 10:38

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from fefef66 to 24f0f45 Compare March 12, 2024 05:42

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 12, 2024

ReToCode approved these changes Mar 12, 2024

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2024

knative-prow bot assigned ReToCode Mar 12, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2024

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from 24f0f45 to 95adad8 Compare March 12, 2024 06:55

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2024

dprotaso closed this Mar 26, 2024

dprotaso reopened this Mar 26, 2024

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from 95adad8 to e7288b1 Compare March 27, 2024 01:27

knative-prow bot assigned dprotaso Mar 27, 2024

dprotaso reviewed Mar 27, 2024

View reviewed changes

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from e7288b1 to 250ab9c Compare March 28, 2024 10:14

dprotaso reviewed Mar 30, 2024

View reviewed changes

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

8d86ca3

Signed-off-by: ckcd <curtis@mail.ustc.edu.cn>

ckcd force-pushed the ck2402_bubble_up_kcert_failed branch from 250ab9c to 8d86ca3 Compare April 15, 2024 15:50

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2024

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2024

knative-prow bot merged commit f65df07 into knative:main Apr 23, 2024
64 checks passed

	isReady bool
	}{{
	name: "empty status should not be ready",
	status: RouteStatus{},
	isReady: false,
	}, {

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

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

Conversation

ckcd commented Feb 29, 2024

Proposed Changes

knative-prow bot commented Feb 29, 2024

knative-prow bot commented Feb 29, 2024

ckcd commented Feb 29, 2024

izabelacg commented Mar 4, 2024

Cali0707 commented Mar 5, 2024

codecov bot commented Mar 5, 2024 • edited Loading

Codecov Report

ckcd commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReToCode commented Mar 11, 2024

ReToCode left a comment

Choose a reason for hiding this comment

ckcd commented Mar 12, 2024 • edited Loading

ckcd commented Mar 21, 2024

dprotaso commented Mar 26, 2024 • edited Loading

ckcd commented Mar 27, 2024

ReToCode commented Mar 27, 2024

ckcd commented Mar 27, 2024

dprotaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckcd commented Mar 28, 2024

dprotaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckcd Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

dprotaso commented Apr 23, 2024

knative-prow bot commented Apr 23, 2024

dprotaso commented Apr 23, 2024

dprotaso commented Apr 23, 2024

knative-prow bot commented Apr 23, 2024

ckcd commented Apr 23, 2024

dprotaso commented Apr 23, 2024

codecov bot commented Mar 5, 2024 •

edited

Loading

ckcd commented Mar 12, 2024 •

edited

Loading

dprotaso commented Mar 26, 2024 •

edited

Loading

dprotaso Apr 4, 2024 •

edited

Loading

ckcd Apr 15, 2024 •

edited

Loading