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

Additional support for subject annotations #29

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

ctrought
Copy link
Contributor

@ctrought ctrought commented Jun 12, 2023

Mainly adds support for additional subject annotations now supported in v1.12 of cert-manager, also upgrades cert-manager to v1.12.1 (though I see another PR for that already).

Allow use of the cert-manager.io/issuer annotation, in addition to the existing cert-manager.io/issuer-name. This ingress annotation is more familiar for people using cert-manager on ingress already and makes switching over a bit simpler.
https://cert-manager.io/docs/usage/ingress/#supported-annotations

Ignore routes that are owned by ingress objects as cert-manager may already be providing certificates via ingresses that openshift-controller-manager then creates and route for. This helps users like myself already using cert-manager on OpenShift via OCM ingress -> route method to slowly migrate to using openshift-routes on routes without the two potentially conflicting.

  • feat: support subject annotations on route
  • fix: ignore routes owned by ingress
  • fix: support ingress issuer name annotation
  • chore: upgrade cert-manager to v1.12.1

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2023
@ctrought ctrought force-pushed the feat-subject-annotations branch from 69a3613 to 7420e75 Compare June 12, 2023 03:16
@maelvls maelvls self-requested a review June 23, 2023 17:57
@maelvls
Copy link
Member

maelvls commented Jun 23, 2023

Hey! Thank you for the great work. I will take a look at the PR on Monday. Thanks!

Note: I'll be on PTO from Tue 27 June to Sun 23 July. Please ping another maintainer (such as @inteon) if you need a quicker review.

@maelvls
Copy link
Member

maelvls commented Jul 31, 2023

Hey, I just realized that the project doesn't have unit tests for createNextCR, which makes me a bit nervous given how much additional logic is added in this PR. I shared the same worry in #28 (comment). I'll try to email you and @vinny-sabatini so that we can coordinate (I couldn't find you on the Kubernetes Slack).

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2023
@ctrought ctrought force-pushed the feat-subject-annotations branch from 7420e75 to f7289ab Compare August 14, 2023 22:38
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2023
@ctrought
Copy link
Contributor Author

ctrought commented Aug 14, 2023

@maelvls no rush, but have rebased and fixed merge conflicts. 🙏

Edit: Sorry looks like I missed your reply above! I've joined the cert-manager-dev slack channel

@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 14, 2023
@ctrought
Copy link
Contributor Author

@maelvls have updated PR with some tests, no rush but have a look when you get a chance! :)

@maelvls
Copy link
Member

maelvls commented Oct 5, 2023

Hi! Let me know if I can be of any help 👍

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2023
@ctrought
Copy link
Contributor Author

Hi! Let me know if I can be of any help 👍

Sorry I haven't had much time to look at this recently, I've addressed pretty much all points except the reconcile test. Since the reconcile logic lives in the controller.go were you wanting to see a new test suite specifically there (ie. controller_test.go) or just include it in the sync test suite? Or if you have time and want to add the test feel free! Thanks

@maelvls
Copy link
Member

maelvls commented Dec 15, 2023

Hey. As suggested, I added a test in 6f0eddb. Do you want to rebase or would you like me to rebase?

Update: I'll rebase if that's OK for you.

@maelvls maelvls force-pushed the feat-subject-annotations branch from 6f0eddb to a02b621 Compare December 15, 2023 10:56
@jetstack-bot jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 15, 2023
@maelvls
Copy link
Member

maelvls commented Dec 15, 2023

/approve
/lgtm

I should ask for another pair of eyes to look at the new test I have added, but I decided to go ahead an LGTM anyways because why not!

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2023
@maelvls
Copy link
Member

maelvls commented Dec 15, 2023

I'll release your changes in the next release (v0.5.0). Since I already release v0.4.0 yesterday, I am unsure whether I should be releasing v0.5.0 right now or not...

For later: here is the release notes I'll use when releasing v0.5.0:


You can now use more annotations! Thanks to @ctrought's work, you can now use the annotations cert-manager.io/email-sans, cert-manager.io/subject-organizations, cert-manager.io/subject-organizationalunits, cert-manager.io/subject-countries, cert-manager.io/subject-provinces, cert-manager.io/subject-localities, cert-manager.io/subject-postalcodes, cert-manager.io/subject-streetaddresses, and cert-manager.io/subject-serialnumber. The documentation for the annotations is identical to the Ingress annotations and can be read in the Ingress Usage page.

To help migrating the annotations from Ingresses to Routes, the annotation cert-manager.io/issuer can now be used as an alternative to cert-manager.io/issuer-name. Both annotations offer the same functionality. Note that the annotation cert-manager.io/cluster-issuer has not been brought to openshift-routes.

@ctrought
Copy link
Contributor Author

That's great, thank you for finishing the PR up @maelvls !!

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2024
@ctrought ctrought force-pushed the feat-subject-annotations branch from a02b621 to e1cc7e6 Compare January 24, 2024 17:04
@jetstack-bot jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 24, 2024
@ctrought
Copy link
Contributor Author

@maelvls anything else required before this gets merged? Just rebased after a different PR was merged. 🙏 Ty!

@maelvls
Copy link
Member

maelvls commented Jan 25, 2024

It seems like everything is good to go!

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2024
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

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 merged commit 5b9f2b3 into cert-manager:main Jan 25, 2024
2 of 3 checks passed
@maelvls
Copy link
Member

maelvls commented Jan 26, 2024

Hey, I'll cut v0.5.0 with your changes today.

Update: done!

@ctrought
Copy link
Contributor Author

@maelvls , amazing & thank you!!

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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. 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.

How to populate certificate metadata i.e. subject details e.g. OU, Organization etc
3 participants