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

Cleanup CSR util/pki code #6019

Merged
merged 4 commits into from
May 11, 2023
Merged

Cleanup CSR util/pki code #6019

merged 4 commits into from
May 11, 2023

Conversation

inteon
Copy link
Member

@inteon inteon commented May 4, 2023 •
edited

Pull Request Motivation

This PR removes some duplicate logic and centralises some marshalling/ unmarshalling logic.
The PR contains only one breaking change: MarshalBasicConstraints's signature changed.
The PR does however deprecate a lot of functions.

Important: projects that use these deprecated functions should switch to the advised (see deprecation comments) alternative function ASAP.

/kind cleanup

NOTE FOR REVIEWER: the first commit contains the important changes, the second commit switches all our source code to the non-deprecated functions.

Release Note

NONE

@jetstack-bot jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. release-note-none Denotes a PR that doesn't merit a release note. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 4, 2023
inteon
inteon commented May 4, 2023
View reviewed changes
pkg/util/pki/csr.go Outdated Show resolved Hide resolved
@inteon inteon force-pushed the improve_pki branch 2 times, most recently from 06ab314 to 2b6a38e Compare May 4, 2023 20:29
@inteon inteon changed the title Cleanup CSR util and validation code Cleanup CSR util/pki and validation code May 4, 2023
@inteon inteon changed the title Cleanup CSR util/pki and validation code Cleanup CSR util/pki and related webhook validation code May 4, 2023
@inteon inteon force-pushed the improve_pki branch 2 times, most recently from 1fca651 to 72d6525 Compare May 5, 2023 07:55
@jetstack-bot jetstack-bot added the area/testing Issues relating to testing label May 5, 2023
@inteon inteon requested a review from SgtCoDFish May 5, 2023 08:27
@inteon inteon changed the title Cleanup CSR util/pki and related webhook validation code Cleanup CSR util/pki code May 9, 2023
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 9, 2023
SgtCoDFish
SgtCoDFish reviewed May 9, 2023
View reviewed changes
pkg/util/pki/csr.go Outdated Show resolved Hide resolved
pkg/util/pki/csr.go Show resolved Hide resolved
pkg/util/pki/csr.go Outdated Show resolved Hide resolved
pkg/util/pki/csr.go Show resolved Hide resolved
@jetstack-bot jetstack-bot 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 May 9, 2023
@inteon inteon force-pushed the improve_pki branch 2 times, most recently from e9ec676 to 0602179 Compare May 9, 2023 13:48
SgtCoDFish
SgtCoDFish requested changes May 9, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

Few more comments / questions - I think this is close!

pkg/util/pki/basicconstraints.go Show resolved Hide resolved
pkg/util/pki/certificatetemplate.go Outdated Show resolved Hide resolved
pkg/util/pki/csr.go Show resolved Hide resolved
@inteon inteon requested a review from SgtCoDFish May 9, 2023 16:09
SgtCoDFish
SgtCoDFish approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

I think this looks like an improvement 👍

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2023
@inteon
cleanup CSR & CertificateTemplate util code
1c2662a 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
switch to non-deprecated functions in source code
0cf0f80 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
remove CertificateTemplateAddKeyUsages
20599d1 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
keep using deprecated functions in ctl binary
6580975 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label May 10, 2023
wallrj
wallrj approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, SgtCoDFish, wallrj

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:
  • OWNERS [SgtCoDFish,inteon,wallrj]

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 merged commit bf5a482 into cert-manager:master May 11, 2023
6 checks passed
@jetstack-bot jetstack-bot added this to the v1.12 milestone May 11, 2023
@inteon inteon mentioned this pull request Jun 21, 2023
Merged
@wallrj wallrj mentioned this pull request Feb 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallrj wallrj wallrj approved these changes

@SgtCoDFish SgtCoDFish SgtCoDFish approved these changes

Assignees

@wallrj wallrj

@SgtCoDFish SgtCoDFish

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@inteon @jetstack-bot @wallrj @SgtCoDFish