Use Certificates over CertificateRequests (#55 followup) #101
Conversation
cert-manager-prow
bot
added
the
dco-signoff: yes
SgtCoDFish
changed the title
cert-manager-prow
bot
added
the
size/XXL
|
/hold EDIT: This hold was while this was WIP. It's now ready for review so I'll unhold. |
cert-manager-prow
bot
added
the
do-not-merge/hold
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
f6482a7 to
c0b0dd6
Compare
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
fa0ac3f to
702e517
Compare
|
/unhold |
cert-manager-prow
bot
removed
the
do-not-merge/hold
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]>
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
9058138 to
d22d991
Compare
cert-manager-prow
bot
added
dco-signoff: no
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
d22d991 to
88600f5
Compare
cert-manager-prow
bot
added
dco-signoff: yes
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
39213bb to
5b2d427
Compare
| return strconv.Atoi(revision) | ||
| } | ||
| func (r *RouteController) getCertificateForRoute(ctx context.Context, route *routev1.Route) (*cmapi.Certificate, error) { | ||
| // Note: this could also implement logic to re-use an existing certificate: route.Annotations[cmapi.CertificateNameKey] |
There was a problem hiding this comment.
Does this mean that it isn't possible (yet) to pre-create a Certificate in order to use a Certificate field that isn't supported using Route annotations?
(mentioned by @ctrought in #49 (comment))
There was a problem hiding this comment.
That's correct as far as I know yeah. That sounds like a nice-to-have rather than a need-to-have; do you agree?
I think a more pressing need is what was briefly discussed in standup yesterday, which is that this logic mirrors so much about ingress-shim that we could expose the ingress-shim logic from cert-manager and re-use it here. But I didn't want to bite off a cert-manager change here.
Something like (pseudo function) cmingress.AnnotationsToCertificate(...).
There was a problem hiding this comment.
I agree, let's postpone this to later (if folks still need it).
this logic mirrors so much about ingress-shim that we could expose the ingress-shim
It would be great if we could do that, especially since we aim to support all the same annotations.
Is it possible do this this now (with no work needed on the cert-manager project), or does it require changes in the cert-manager project in which case we would probably be better off doing that in the next release of openshift-routes?
There was a problem hiding this comment.
Hey. I went over the code and found it well written and nothing stood out as an issue.
During my review, I haven't gone over the updated unit tests, I haven't run the ./test-smoke.sh myself, and haven't run openshift-routes locally to test it out... Hoping that it all works 😅
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
128c291 to
a994911
Compare
The main aim here is to improve testing and to add other annotations that weren't added in the open source PR. This is a squashed commit of several: - ensure logr calls have named args to prevent panics - update RBAC to use certs rather than certreqs - fix duration in smoke test, log when polling - don't try to create secret, clean up tests - fix lint error, allow more permissive algorithm specification - change check for CRs to certs - don't Own Secrets since we don't generate them - add support for IP addresses + URIs for certs - add most remaining annotations and improve integration/e2e tests - remove readme notice - remove create secrets permission - ensure deterministic cert name + check names aren't too long - Review comments from Mael: A grammar fix + context on IngressController Signed-off-by: Ashley Davis <[email protected]>
SgtCoDFish
force-pushed
the
jacksgt-use-certificates
branch
from
a994911 to
b55e587
Compare
|
[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 |
cert-manager-prow
bot
added
the
approved
|
/lgtm |
cert-manager-prow
bot
added
do-not-merge/hold
|
I reviewed the code and the smoke test is passing. |
cert-manager-prow
bot
removed
the
do-not-merge/hold
|
Hi @SgtCoDFish , thanks for picking this up and all of your work around getting it ready to be shipped - amazing progress! |
@jacksgt made an excellent start to this in #55 and I've preserved their authorship on the commit.
This PR adds a few followup changes to resolve merge conflicts, ensure tests pass and to improve behaviour locally.
From testing in Kind, I've confirmed that this PR does create and update certificates, and that if a secret already exists when a route is reconciled that secret will be picked up.
This is a WIP PR - to make the tests work I deleted a lot fo code, but there are probably useful tests we can perform for Certs which are similar to those we deleted.UPDATE: We've now decided that we'll take this approach over the approach in #102 so I'll fix up this PR to get it ready for review.