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.

Sign up for GitHub

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

Jump to bottom

Add encodeUsagesInRequest to Certificate spec (fix #3301) #3304

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Add encodeUsagesInRequest to Certificate spec (fix #3301) #3304

merged 2 commits into from
Oct 16, 2020

Conversation

raphink
Copy link
Contributor

@raphink raphink commented Sep 22, 2020 •
edited by meyskens
Loading

This PR intends to fix #3301 by adding a new encode_usages_in_request option to certificates.

Add encodeUsagesInRequest to Certificate spec to disable encoding usages in the CSR

@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 22, 2020
@jetstack-bot
Copy link
Contributor

Hi @raphink. Thanks for your PR.

I'm waiting for a jetstack or cert-manager 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.

@jetstack-bot jetstack-bot added the area/api Indicates a PR directly modifies the 'pkg/apis' directory label Sep 22, 2020
@raphink raphink marked this pull request as draft September 22, 2020 09:40
@jetstack-bot jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2020
@raphink raphink changed the title Add encore_usages_in_request to Certificate spec (fix #3301) Add encode_usages_in_request to Certificate spec (fix #3301) Sep 22, 2020
@jetstack-bot jetstack-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Sep 22, 2020
@munnerz
Copy link
Member

munnerz commented Sep 22, 2020

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 22, 2020
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. area/deploy Indicates a PR modifies deployment configuration size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 22, 2020
@raphink
Copy link
Contributor Author

raphink commented Sep 22, 2020

For some reason, with the current code, when I create a Certificate with encodeUsagesInRequest, the field gets removed from spec immediatly it seems. I can still see my value in the annotations.

@meyskens
Copy link
Contributor

@raphink It seems to keep the value for me

spec:
    commonName: test
    duration: 2160h0m0s
    encodeUsagesInRequest: false
    issuerRef:
      group: cert-manager.io
      kind: Issuer
      name: selfsigned-issuer

What are you seeing exactly?

@raphink
Copy link
Contributor Author

raphink commented Sep 30, 2020

@meyskens yes I'm seeing that too and I don't understand why 🤕

@meyskens
Copy link
Contributor

meyskens commented Oct 8, 2020

@raphink I think i am confused here... encodeUsagesInRequest: false should be kept right?

@meyskens
Copy link
Contributor

meyskens commented Oct 8, 2020

Feel free to ping me on Slack for this as that might be easier to debug there than on GitHub

@jetstack-bot jetstack-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 Oct 16, 2020
@raphink
Add encode_usages_in_request to Certificate spec (fix #3301)
b2d719d 
Signed-off-by: Raphaël Pinson <raphael.pinson@camptocamp.com>
@raphink
Regenerate CRDs
e1c8d3a 
Signed-off-by: Raphaël Pinson <raphael.pinson@camptocamp.com>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Oct 16, 2020
@raphink raphink marked this pull request as ready for review October 16, 2020 13:41
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2020
@meyskens meyskens changed the title Add encode_usages_in_request to Certificate spec (fix #3301) Add encodeUsagesInRequest to Certificate spec (fix #3301) Oct 16, 2020
@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 16, 2020
@meyskens
Copy link
Contributor

/kind feature

@jetstack-bot jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 16, 2020
@meyskens
Copy link
Contributor

/approve
/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2020
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: meyskens, raphink

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2020
@meyskens
Copy link
Contributor

flakie

/retest

@jetstack-bot jetstack-bot merged commit 1286d37 into cert-manager:master Oct 16, 2020
@jetstack-bot jetstack-bot added this to the v1.1 milestone Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meyskens meyskens meyskens requested changes

@JoshVanL JoshVanL JoshVanL left review comments

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@meyskens meyskens

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration 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. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

Option to not include key usage in CSR
5 participants
@raphink @jetstack-bot @munnerz @meyskens @JoshVanL