Propagate status from KCert to Route #7163

nak3 · 2020-03-06T09:35:53Z

Proposed Changes

When autoTLS is enabled, KCert is one of the critical resources so
Route and KSvc should not be Ready status until Cert is deployed
correctly.

This patch changes to propagate status from KCert to Route.

/lint

Fixes #7162

Release Note

Route does not become Ready if KCert is not Ready on autoTLS enabled env

knative-prow-robot

@nak3: 2 warnings.

In response to this:

Proposed Changes

When autoTLS is enabled, KCert is one of the critical resources. This
patch changes changes to propagate status status from KCert to Route.

/lint

Fixes #7162

Release Note
Route does not become Ready if KCert is not Ready on autoTLS enabled env

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.

pkg/apis/serving/v1/route_lifecycle.go

pkg/apis/serving/v1alpha1/route_lifecycle.go

vagababov · 2020-03-06T17:21:03Z

/retry

pkg/apis/serving/v1/route_lifecycle.go

nak3 · 2020-03-07T08:56:54Z

/hold

I have modified files under vendor directory until knative/pkg#1148 is accepted.

markusthoemmes · 2020-03-20T08:36:32Z

@nak3 seems like the hold can be removed?

nak3 · 2020-03-20T09:07:28Z

/unhold

Yes, it is ready for review now. Thank you.

markusthoemmes · 2020-03-20T09:17:43Z

/assign @ZhiminXiang

ZhiminXiang · 2020-03-20T16:46:31Z

pkg/apis/serving/v1/route_lifecycle.go

@@ -29,6 +29,9 @@ import (
 var routeCondSet = apis.NewLivingConditionSet(
 	RouteConditionAllTrafficAssigned,
 	RouteConditionIngressReady,
+
+// TODO(nak3):  Add RouteConditionCertificateProvisioned in the next release.


If we don't plan to do this in this release (I assume 0.14), could you remove the "Release Note" from the description of this PR?

Sure, done.

Why in the next release?

The downgrade test TestServicePostDowngrade failed if we added the new condition in routeCondSet. The Route became Unknown status. (I have confirmed that it does not happen if previous version already has the condition.)
Here is the failure log

ZhiminXiang · 2020-03-20T23:23:15Z

/lgtm

nak3 · 2020-03-24T23:05:02Z

/assign @tcnghia

tcnghia · 2020-03-24T23:14:00Z

pkg/apis/serving/v1/route_lifecycle.go

@@ -29,6 +29,9 @@ import (
 var routeCondSet = apis.NewLivingConditionSet(
 	RouteConditionAllTrafficAssigned,
 	RouteConditionIngressReady,
+
+// TODO(nak3):  Add RouteConditionCertificateProvisioned in the next release.


File an issue to track?

Yes, I do. Thank you!

tcnghia · 2020-03-24T23:14:53Z

/approve

ZhiminXiang · 2020-03-26T23:40:28Z

/assgin @mattmoor

mattmoor

I'm cool with this change, I would just like to understand the rationale behind the release staging.

thanks!

If NewLivingConditionSet is added new condition, it fails when downgrade test. Add RouteConditionCertificateProvisioned in the next release

nak3 · 2020-04-21T01:56:38Z

I think both are good. Making minimal impact to UX is better so we should change it to "only when plain HTTP has been disabled"?

I have added MarkHTTPDownward and set CertificateProvisioned to Ready with MarkHTTPDownward when HTTP is enabled. Otherwise, it is same with original change.

Here is the example output:

$ kubectl get rt -n foo hello-example  -o yaml
apiVersion: serving.knative.dev/v1
kind: Route
  ...
status:
  address:
    url: http://hello-example.foo.svc.cluster.local
  conditions:
  - lastTransitionTime: "2020-04-21T01:48:00Z"
    status: "True"
    type: AllTrafficAssigned
  - lastTransitionTime: "2020-04-21T01:48:02Z"
    message: Certificate foo.example.com is not ready downard HTTP.
    reason: HTTPDownward
    status: "True"
    type: CertificateProvisioned
  - lastTransitionTime: "2020-04-21T01:48:02Z"
    status: "True"
    type: IngressReady
  - lastTransitionTime: "2020-04-21T01:48:02Z"
    status: "True"
    type: Ready
  observedGeneration: 1
  traffic:
  - latestRevision: true
    percent: 100
    revisionName: hello-example-phkdh-1
  url: http://hello-example.foo.example.com

pkg/apis/serving/v1/route_lifecycle.go

knative-metrics-robot · 2020-04-22T06:16:45Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/route/route.go	84.6%	84.8%	0.2

nak3 · 2020-04-22T07:46:08Z

/retest

nak3 · 2020-04-22T08:44:12Z

/retest

@knative-test-reporter-robot is not correct.. The test failed only TestUpdate and it seems flakiness.

ZhiminXiang · 2020-05-06T18:18:43Z

/lgtm
/approve

ZhiminXiang · 2020-05-06T18:25:28Z

When autoTLS is enabled, KCert is one of the critical resources so
Route and KSvc should not be Ready status until Cert is deployed
correctly.

I have been wondering whether we want this behavior in general, or only when plain HTTP has been disabled. What do folks think? 🤔

cc @ZhiminXiang @tcnghia

I think we only want this behavior when plain HTTP has been disabled.

ZhiminXiang · 2020-05-19T06:20:40Z

@mattmoor could you take another look? It needs your approval :)

mattmoor

/lgtm
/approve

Sorry for the delay, feel free to ping me more aggressively when things are blocked on me because I get a TON of notifications, and I'd love to have had this bake more.

knative-prow-robot · 2020-05-20T01:29:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, nak3, tcnghia, ZhiminXiang

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:

~~pkg/apis/OWNERS~~ [mattmoor]
~~pkg/reconciler/route/OWNERS~~ [ZhiminXiang,mattmoor,nak3,tcnghia]
~~pkg/testing/OWNERS~~ [ZhiminXiang,mattmoor,nak3,tcnghia]

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

nak3 · 2020-05-20T01:41:31Z

Thank you! 😄

nak3 · 2020-05-20T02:31:56Z

/retest

knative-test-reporter-robot · 2020-05-20T03:41:19Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	pull-knative-serving-integration-tests pull-knative-serving-integration-tests	2/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

This reverts commit 455185d.

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 6, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 6, 2020

knative-prow-robot reviewed Mar 6, 2020

View reviewed changes

pkg/apis/serving/v1/route_lifecycle.go Outdated Show resolved Hide resolved

pkg/apis/serving/v1alpha1/route_lifecycle.go Outdated Show resolved Hide resolved

knative-prow-robot added area/API API objects and controllers area/networking labels Mar 6, 2020

knative-prow-robot requested review from markusthoemmes and vagababov March 6, 2020 09:36

ZhiminXiang reviewed Mar 6, 2020

View reviewed changes

pkg/apis/serving/v1/route_lifecycle.go Outdated Show resolved Hide resolved

pkg/apis/serving/v1/route_lifecycle.go Outdated Show resolved Hide resolved

nak3 mentioned this pull request Mar 7, 2020

Add MarkTrueWithReason function to ConditionManager knative/pkg#1148

Merged

nak3 force-pushed the fix-cert-status branch from ffe1170 to 0bd7c11 Compare March 7, 2020 08:56

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

nak3 force-pushed the fix-cert-status branch from 0bd7c11 to 7f018d0 Compare March 20, 2020 02:02

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Mar 20, 2020

nak3 force-pushed the fix-cert-status branch from f0a4005 to 81e0c00 Compare March 20, 2020 02:19

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2020

knative-prow-robot assigned ZhiminXiang Mar 20, 2020

ZhiminXiang reviewed Mar 20, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2020

knative-prow-robot assigned tcnghia Mar 24, 2020

tcnghia reviewed Mar 24, 2020

View reviewed changes

mattmoor reviewed Mar 27, 2020

View reviewed changes

nak3 added 7 commits April 21, 2020 10:28

Add unit test for MarkAutoTLSNotEnabled

b14d176

Fix unit test

6f71b22

Do not add RouteConditionCertificateProvisioned to NewLivingConditionSet

4e3f3e2

If NewLivingConditionSet is added new condition, it fails when downgrade test. Add RouteConditionCertificateProvisioned in the next release

Remove TODO

b4210e9

Update unit test

e591fa7

Remove unused func

410aaf6

Add MarkHTTPDownward

6a2d8fb

nak3 force-pushed the fix-cert-status branch from fb55200 to 6a2d8fb Compare April 21, 2020 01:49

mattmoor reviewed Apr 22, 2020

View reviewed changes

pkg/apis/serving/v1/route_lifecycle.go Outdated Show resolved Hide resolved

Fix downward to downgrade

8e05e2c

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

mattmoor reviewed May 20, 2020

View reviewed changes

knative-prow-robot assigned mattmoor May 20, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2020

knative-prow-robot merged commit 455185d into knative:master May 20, 2020

nak3 deleted the fix-cert-status branch May 20, 2020 05:08

nak3 mentioned this pull request May 22, 2020

Call MarkTLSNotEnabled for private cluster-local service #8043

Merged

tcnghia pushed a commit that referenced this pull request May 22, 2020

Revert "Propagate status from KCert to Route (#7163)"

e1c99eb

This reverts commit 455185d.

tcnghia pushed a commit that referenced this pull request May 22, 2020

Revert "Propagate status from KCert to Route (#7163)"

ea161f5

This reverts commit 455185d.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate status from KCert to Route #7163

Propagate status from KCert to Route #7163

nak3 commented Mar 6, 2020 •

edited

Loading

knative-prow-robot left a comment

vagababov commented Mar 6, 2020

nak3 commented Mar 7, 2020

markusthoemmes commented Mar 20, 2020

nak3 commented Mar 20, 2020

markusthoemmes commented Mar 20, 2020

ZhiminXiang Mar 20, 2020

nak3 Mar 20, 2020

mattmoor Mar 27, 2020

nak3 Mar 27, 2020

ZhiminXiang commented Mar 20, 2020

nak3 commented Mar 24, 2020

tcnghia Mar 24, 2020

nak3 Mar 24, 2020

tcnghia commented Mar 24, 2020

ZhiminXiang commented Mar 26, 2020

mattmoor left a comment

nak3 commented Apr 21, 2020

knative-metrics-robot commented Apr 22, 2020

nak3 commented Apr 22, 2020

nak3 commented Apr 22, 2020

ZhiminXiang commented May 6, 2020

ZhiminXiang commented May 6, 2020

ZhiminXiang commented May 19, 2020

mattmoor left a comment

knative-prow-robot commented May 20, 2020

nak3 commented May 20, 2020

nak3 commented May 20, 2020

knative-test-reporter-robot commented May 20, 2020

Propagate status from KCert to Route #7163

Propagate status from KCert to Route #7163

Conversation

nak3 commented Mar 6, 2020 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

vagababov commented Mar 6, 2020

nak3 commented Mar 7, 2020

markusthoemmes commented Mar 20, 2020

nak3 commented Mar 20, 2020

markusthoemmes commented Mar 20, 2020

ZhiminXiang Mar 20, 2020

Choose a reason for hiding this comment

nak3 Mar 20, 2020

Choose a reason for hiding this comment

mattmoor Mar 27, 2020

Choose a reason for hiding this comment

nak3 Mar 27, 2020

Choose a reason for hiding this comment

ZhiminXiang commented Mar 20, 2020

nak3 commented Mar 24, 2020

tcnghia Mar 24, 2020

Choose a reason for hiding this comment

nak3 Mar 24, 2020

Choose a reason for hiding this comment

tcnghia commented Mar 24, 2020

ZhiminXiang commented Mar 26, 2020

mattmoor left a comment

Choose a reason for hiding this comment

nak3 commented Apr 21, 2020

knative-metrics-robot commented Apr 22, 2020

nak3 commented Apr 22, 2020

nak3 commented Apr 22, 2020

ZhiminXiang commented May 6, 2020

ZhiminXiang commented May 6, 2020

ZhiminXiang commented May 19, 2020

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 20, 2020

nak3 commented May 20, 2020

nak3 commented May 20, 2020

knative-test-reporter-robot commented May 20, 2020

nak3 commented Mar 6, 2020 •

edited

Loading