Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSR Duration KEP #2788

Merged
merged 2 commits into from
Jun 25, 2021
Merged

CSR Duration KEP #2788

merged 2 commits into from
Jun 25, 2021

Conversation

enj
Copy link
Member

@enj enj commented Jun 17, 2021

Signed-off-by: Monis Khan [email protected]

/sig auth
/assign @liggitt @smarterclayton @mikedanese

Signed-off-by: Monis Khan <[email protected]>
@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 17, 2021
@k8s-ci-robot k8s-ci-robot requested review from deads2k and liggitt June 17, 2021 18:54
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 17, 2021
@enj
Copy link
Member Author

enj commented Jun 17, 2021

/assign @deads2k

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 17, 2021
@enj enj mentioned this pull request Jun 17, 2021
17 tasks
keps/sig-auth/2784-csr-duration/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2784-csr-duration/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2784-csr-duration/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2784-csr-duration/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2784-csr-duration/README.md Outdated Show resolved Hide resolved
keps/sig-auth/2784-csr-duration/README.md Show resolved Hide resolved
@deads2k
Copy link
Contributor

deads2k commented Jun 22, 2021

The design and the PRR both lgtm. The exception request was approved.

/lgtm
/approve
/hold

placing a hold to allow additional reviewers this week.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2021
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm as well

Comment on lines +227 to +228
- Confirm with [cert-manager](https://github.com/jetstack/cert-manager/pull/3646) that the new functionality addresses their use case
- Confirm with [pinniped](https://pinniped.dev) that the new functionality addresses their use case
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking for merge of this doc, but can you confirm with them this addresses their use case ~now, not in a release?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from the cert-manager side!

We currently use an annotation to allow clients to set requested duration. We can fallback to checking this annotation if this field is nil for users running older versions of kube.

Once pre 1.22 Kubernetes versions are no longer supported by cert-manager, we can remove the annotation completely.

/cc @munnerz

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for the Pinniped use case as well. Our planned usage is for a credential exchange API just like the second user story:

  1. External user authenticates to a trusted service (using some custom application-specific credential).
  2. Trusted service creates a CSR with a short spec.expirationSeconds (probably the 10 minute minimum).
  3. Trusted service approves its own CSR and relies on a default signer to sign it.
  4. Trusted service retrieves the signed certificate and returns it to the authenticated external user.

We would quickly add support for new flow but keep some fallback code that uses a custom signer until we no longer support current Kubernetes versions. We can probe for this feature dynamically and select the modern or fallback implementation appropriately.

@k8s-ci-robot k8s-ci-robot requested a review from munnerz June 23, 2021 14:07
@smarterclayton
Copy link
Contributor

Sorry for the delay, I thought I dropped a "this looks good and Jordan and David hit all of my comments already", but it looks like I never pressed comment.

This looks good to me, no further comments above what was already landed.

@supriya-premkumar
Copy link
Contributor

Hi @enj 👋🏽. Supriya here, 1.22 Enhancements Shadow.
For the enhancement to be included in the 1.22 milestone, it must meet the following criteria:

  • Complete all the Beta release PRR questionnaire(upgrade/rollback test)
  • The PR must be approved and merged by EOD June 25, 2021

Thank you!

@enj
Copy link
Member Author

enj commented Jun 25, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 165d718 into kubernetes:master Jun 25, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants