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

Add option to switch between using certificates and CRs #102

Closed
wants to merge 9 commits into from

Conversation

SgtCoDFish
Copy link
Member

In #101 (a followup to #55) we unilaterally switch to using certificates for issuing certs for routes. This PR presents an alternative whereby we allow users to opt in to the old behavior.

We can pick between #101 and this PR.

This PR is WIP until we decide on a path to take. In both cases (i.e. #101 and this PR) the Certificate approach needs further testing, but that can also be done when a path is chosen!

jacksgt and others added 7 commits September 30, 2024 11:56
As discussed in
cert-manager#49 ,
using `CertificateRequest` resources for interacting with cert-manager
has several drawbacks:

- this project (openshift-routes) has to duplicate logic for handling
the creation of certificates that already exists in cert-manager.
- observability is impaired because cert-manager does not offer
metrics for `CertificateRequest` resources, only for `Certificates`.

Therefore, this patch replaces the management of `CertificateRequests` with
`Certificates` (and `Secrets`). In return, it requires slightly higher
privileges because openshift-routes needs to be able to
create and read `Certificates`.
At the same time, the code complexity and LOC has been reduced.

Signed-off-by: Jack Henschel <[email protected]>
Signed-off-by: Ashley Davis <[email protected]>
@cert-manager-prow cert-manager-prow bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Sep 30, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sgtcodfish. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@SgtCoDFish
Copy link
Member Author

/hold

Until we've decided an approach!

@cert-manager-prow cert-manager-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 30, 2024
@cert-manager-prow
Copy link
Contributor

@SgtCoDFish: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cert-manager-openshift-routes-verify 6d684f7 link true /test pull-cert-manager-openshift-routes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@SgtCoDFish
Copy link
Member Author

Going to close this PR since we decided in standup this morning to proceed with just supporting Certificates for now. If we have to re-add CRs in the future this will be useful, but for now we're not doing that.

@SgtCoDFish SgtCoDFish closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants