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

ctl experimental create certificatesigningrequest #4106

Merged

Conversation

JoshVanL
Copy link
Collaborator

@JoshVanL JoshVanL commented Jun 15, 2021

This PR adds the command:

ctl experimental create certificatesigningrequest
ctl x create csr

This behaves the same as the ctl create certificaterequest command, expect that it creates Kubernetes CertificateSigningRequest resources, rather than CertificateRequest resources.

Note that the signerName is built using the discovery API to determine whether the signer is namespaced or not.

Documentation: cert-manager/website#615

Adds CLI command: `ctl experimental create certificatesigningrequest` for creating a Kuberenetes CertificateSigningRequest based upon a cert-manager Certificate manifest file

/milestone v1.5

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
usages

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
@jetstack-bot jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 15, 2021
@jetstack-bot jetstack-bot added this to the v1.5 milestone Jun 15, 2021
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 15, 2021
@JoshVanL JoshVanL added area/ctl Indicates a PR or issue relates to the cert-manager-ctl CLI component and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 15, 2021
@JoshVanL JoshVanL force-pushed the ctl-experimental-create-csr branch from 1e652aa to 331e178 Compare June 15, 2021 14:36
@jetstack-bot jetstack-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 15, 2021
Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
Copy link
Collaborator

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Looks good to me and seems like a really useful command

I've read through the code and also tested the new command by:

  1. Building the plugin with bazel build //cmd/ctl and adding to path
  2. Deploying cert-manager v1.4 with the experimental csr feature gate enabled
  3. Creating a csr with the kubectl plugin using the example commands
  4. Manually approving the csr (Do we intend to have a default approver for CSRs like we do with CRs? There may have been a discussion about this that I've forgotten about)
  5. Observing the X.509 certificate added to csr's status/written to a file

I had a couple of issues along the way, so added a few minor suggestions.

@JoshVanL
Copy link
Collaborator Author

Manually approving the csr (Do we intend to have a default approver for CSRs like we do with CRs? There may have been a discussion about this that I've forgotten about)

Not today we don't. It would be a nice chance for us to not have a blanket auto-approver.

I've mentioned before that it does make me nervous that we have an internal auto-approver which is simple to forget to disable. Not disabling that auto-approver makes any other approver pointless.

removing error wraping

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>
Copy link
Collaborator

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Thanks for the changes, I've tested that namespace is now defaulted if not provided and panic no longer thrown.

I realized that we don't apply defaults or validate the certificate template, specifically the duration (i.e I could issue myself an X.509 cert with a duration 1s with the CA issuer).
I guess it might be ok for this, since the certs aren't going to be auto-renewed.

Going to lgtm with a hold in case @maelvls too wants to leave a review

/hold
/lgtm

@jetstack-bot jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 18, 2021
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, JoshVanL

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

@maelvls
Copy link
Member

maelvls commented Jun 18, 2021

I would love to have an option -oyaml to see the manifest itself 😅

@maelvls
Copy link
Member

maelvls commented Jun 18, 2021

/unhold

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2021
@irbekrm
Copy link
Collaborator

irbekrm commented Jun 18, 2021

/kind feature

@jetstack-bot jetstack-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 18, 2021
@jetstack-bot jetstack-bot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jun 18, 2021
@jetstack-bot jetstack-bot merged commit 67c8176 into cert-manager:master Jun 18, 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. area/ctl Indicates a PR or issue relates to the cert-manager-ctl CLI component dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants