CSR condition status, lastTransitionTime, versioned validation #90191

liggitt · 2020-04-16T03:26:53Z

What type of PR is this?
/kind api-change
/kind feature

What this PR does / why we need it:

Implements v1beta1 portions of the CSR KEP (kubernetes/enhancements#1513)

Best reviewed commit-by-commit

Adds a well-known condition type of Failed for use by signers that want to indicate permanent failure
- Updates the signer controller to record a Failed condition when a request does not pass signerName validation
- Updates printers/describers to surface failed status
- Updates cert manager to stop waiting for failed CSRs
- Updates cert cleaner to clean up stale failed CSRs
- Allows conditions other than Approved and Denied to be added via /status
Brings CSR conditions in line with recommended condition schema:
- Adds a status field (for compatibility, defaults to True if unspecified, may only be True for Approved, Denied, and Failed conditions)
- Adds a lastTransitionTime field
Plumbs API version information to CSR validation
Partitions validation functions for the main resource, status, and approval subresources
In all API versions:
- Approved and Denied conditions are mutually exclusive (fixes csr approved than denied ，can not be approved again #90510)
- Approved, Denied, and Failed conditions cannot be removed once set
In versions other than v1beta1:
- status.certificate is required to be well-formed
- status.certificate is immutable once set
- duplicate condition types are not allowed
- empty condition type is not allowed
- legacy-unknown signer is not allowed

Does this PR introduce a user-facing change?:

CertificateSigningRequest API conditions were updated:
* a `status` field was added; this field defaults to `True`, and may only be set to `True` for `Approved`, `Denied`, and `Failed` conditions
* a `lastTransitionTime` field was added
* a `Failed` condition type was added to allow signers to indicate permanent failure; this condition can be added via the `certificatesigningrequests/status` subresource.
* `Approved` and `Denied` conditions are mutually exclusive
* `Approved`, `Denied`, and `Failed` conditions can no longer be removed from a CSR

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/20190607-certificates-api.md

/sig auth
/milestone v1.19
/priority important-soon
/cc @deads2k @munnerz

pkg/apis/certificates/types.go

pkg/apis/certificates/v1beta1/defaults.go

pkg/apis/certificates/validation/validation.go

pkg/apis/certificates/v1beta1/defaults.go

pkg/apis/certificates/validation/validation.go

pkg/apis/certificates/validation/validation_test.go

staging/src/k8s.io/api/certificates/v1beta1/types.go

liggitt · 2020-05-28T20:53:40Z

regenerated openapi - https://github.com/kubernetes/kubernetes/compare/01a835ae5da08076c0e0cbf2c32c798884cf2d2d..7049149181189eae304246cc49de78d08eef4f80

k8s-ci-robot · 2020-05-28T20:53:44Z

New changes are detected. LGTM label has been removed.

liggitt · 2020-05-29T02:56:41Z

/retest

smarterclayton · 2020-06-01T21:36:18Z

pkg/apis/certificates/v1beta1/defaults.go

@@ -59,18 +67,34 @@ func DefaultSignerNameFromSpec(obj *certificatesv1beta1.CertificateSigningReques
 	}
 }

+var (


So much better

smarterclayton · 2020-06-01T21:44:24Z

API changes look fine to me, lgtm from that perspective.

liggitt · 2020-06-01T21:47:57Z

thanks

/hold cancel

pkg/controller/certificates/signer/signer.go

smarterclayton · 2020-06-01T22:00:36Z

Only one question, I had to read the KEP again but I didn't see anything really crazy.

smarterclayton · 2020-06-01T23:51:12Z

/approve
/lgtm

k8s-ci-robot · 2020-06-01T23:51:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton

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:

~~api/OWNERS~~ [liggitt,smarterclayton]
~~pkg/apis/OWNERS~~ [liggitt,smarterclayton]
~~pkg/controller/certificates/OWNERS~~ [liggitt,smarterclayton]
~~pkg/kubelet/certificate/OWNERS~~ [liggitt,smarterclayton]
~~pkg/printers/OWNERS~~ [liggitt,smarterclayton]
~~pkg/registry/certificates/OWNERS~~ [liggitt,smarterclayton]
~~plugin/pkg/admission/certificates/OWNERS~~ [liggitt,smarterclayton]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt,smarterclayton]
~~staging/src/k8s.io/client-go/util/certificate/OWNERS~~ [liggitt,smarterclayton]
~~staging/src/k8s.io/kubectl/OWNERS~~ [liggitt,smarterclayton]

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

liggitt · 2020-06-02T00:51:34Z

/retest

liggitt · 2020-06-02T02:44:10Z

/retest

liggitt · 2020-06-02T03:42:05Z

/retest

k8s-ci-robot requested review from deads2k and munnerz April 16, 2020 03:26

k8s-ci-robot added this to the v1.19 milestone Apr 16, 2020

liggitt force-pushed the csr-status branch from e14dee3 to c46aaea Compare April 16, 2020 04:01

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 16, 2020

liggitt force-pushed the csr-status branch from c46aaea to bb2ff96 Compare April 16, 2020 04:05

liggitt changed the title ~~WIP - CSR conditions, versioned validation~~ WIP - CSR condition status, versioned validation Apr 16, 2020

liggitt force-pushed the csr-status branch from bb2ff96 to edfcd15 Compare April 16, 2020 04:52

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Apr 16, 2020

liggitt force-pushed the csr-status branch from edfcd15 to fded1fd Compare April 16, 2020 05:06

k8s-ci-robot added area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Apr 16, 2020

liggitt mentioned this pull request Apr 20, 2020

CertificateSigningRequest API kubernetes/enhancements#1513

Closed

liggitt added this to Assigned in API Reviews Apr 23, 2020

liggitt assigned smarterclayton Apr 23, 2020

munnerz reviewed May 5, 2020

View reviewed changes

liggitt added this to In progress in SIG-Auth: CertificateSigningRequests May 6, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2020

liggitt force-pushed the csr-status branch from fded1fd to c6fbbb0 Compare May 27, 2020 15:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2020

Generated files

7049149

liggitt force-pushed the csr-status branch from 01a835a to 7049149 Compare May 28, 2020 20:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2020

liggitt added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels May 28, 2020

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels May 28, 2020

This was referenced May 29, 2020

Add e2e coverage for the CertificateSigningRequest API, enable patch support for approval subresource #91558

Merged

csr approved than denied ，can not be approved again #90510

Closed

smarterclayton reviewed Jun 1, 2020

View reviewed changes

liggitt removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2020

smarterclayton reviewed Jun 1, 2020

View reviewed changes

pkg/controller/certificates/signer/signer.go Show resolved Hide resolved

smarterclayton reviewed Jun 1, 2020

View reviewed changes

pkg/controller/certificates/signer/signer.go Show resolved Hide resolved

k8s-ci-robot merged commit 5fb9e35 into kubernetes:master Jun 2, 2020

liggitt moved this from In progress to Done in SIG-Auth: CertificateSigningRequests Jun 2, 2020

liggitt deleted the csr-status branch June 2, 2020 15:02

github-actions bot mentioned this pull request Jun 9, 2020

Week Ending June 7, 2020 dev-obs/actus#175

Open

liggitt moved this from Assigned to API review completed, 1.19 in API Reviews Jun 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSR condition status, lastTransitionTime, versioned validation #90191

CSR condition status, lastTransitionTime, versioned validation #90191

liggitt commented Apr 16, 2020 •

edited

Loading

liggitt commented May 28, 2020 •

edited

Loading

k8s-ci-robot commented May 28, 2020

liggitt commented May 29, 2020

smarterclayton Jun 1, 2020

smarterclayton commented Jun 1, 2020

liggitt commented Jun 1, 2020 •

edited

Loading

smarterclayton commented Jun 1, 2020

smarterclayton commented Jun 1, 2020

k8s-ci-robot commented Jun 1, 2020

liggitt commented Jun 2, 2020

liggitt commented Jun 2, 2020

liggitt commented Jun 2, 2020

               	}
               }
+              var (

CSR condition status, lastTransitionTime, versioned validation #90191

CSR condition status, lastTransitionTime, versioned validation #90191

Conversation

liggitt commented Apr 16, 2020 • edited Loading

liggitt commented May 28, 2020 • edited Loading

k8s-ci-robot commented May 28, 2020

liggitt commented May 29, 2020

smarterclayton Jun 1, 2020

Choose a reason for hiding this comment

smarterclayton commented Jun 1, 2020

liggitt commented Jun 1, 2020 • edited Loading

smarterclayton commented Jun 1, 2020

smarterclayton commented Jun 1, 2020

k8s-ci-robot commented Jun 1, 2020

liggitt commented Jun 2, 2020

liggitt commented Jun 2, 2020

liggitt commented Jun 2, 2020

liggitt commented Apr 16, 2020 •

edited

Loading

liggitt commented May 28, 2020 •

edited

Loading

liggitt commented Jun 1, 2020 •

edited

Loading