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

Moving from CertificateRequests to Certificates: A Survey #49

Closed
maelvls opened this issue Dec 15, 2023 · 29 comments
Closed

Moving from CertificateRequests to Certificates: A Survey #49

maelvls opened this issue Dec 15, 2023 · 29 comments

Comments

@maelvls
Copy link
Member

maelvls commented Dec 15, 2023

Dear users of cert-manager/openshift-routes,

I'd like to hear your use-cases of cert-manager/openshift-routes. The initial reason openshift-routes was created was because an internal customer was using cert-utils-operator and didn't like the idea of having the private key in Secret resources. Also, cert-utils-operator doesn't create the certificate out of annotations like what's possible with the Ingress and Gateway resources. This is why cert-manager/openshift-routes doesn't create a Certificate resources.

In #34 and #42, I learned that this choice makes it hard for people to adopt this tool. For example, the project https://github.com/its4u/cert-manager-routes-controller/ does the same thing except it creates a Certificate resource rather than a CertificateRequest.

If you are using cert-manager/openshift-routes, why do you use it? Do you use it as a way to request certificates right from the Route resource?

@maelvls maelvls pinned this issue Dec 15, 2023
@nate-duke
Copy link
Contributor

We use it to allow certificate-manager to manage certificates for Routes. We do not use redhat's cert-utils-operator.

We use an in-house ACME system to issue our certs.

@ctml91
Copy link

ctml91 commented Jan 15, 2024

We use it as a way to request certs directly from the route resource, as using ingress adds a bit of unnecessary complexity (OCP users now need to create Ingress for the purpose of creating a Route, when many are only familiar with Routes).

Personally I'd prefer if openshift-routes used Certificate resources, and allow cert-manager to handle using all the logic built into it already. IMO there is no difference in having the PK storage directly in the Route or Secret. Either way, it is being stored in etcd. By default cluster role definitions secrets are more secure than routes, as default cluster roles do not permit users with view privileges in a namespace to view secrets, however the view role permits users to view routes which gives them access to the PK. Either way, the PK will end up viewable in the Route so it's somewhat a moot point I think.

I suppose you could create custom roles to restrict access to the routes if you wanted to, while granting access to secrets, but it seems a bit backwards. But you could also create custom roles to restrict access to both secrets & routes if you truly wanted to restrict a user with access to a namespace from viewing PK's.

I think the benefits of using a certificate resource outweigh any pros of embedding directly in routes to avoid storing PK in Routes. I think if cert-manager is okay with storing PK in secrets, then openshift-routes should too.

@jacksgt
Copy link
Contributor

jacksgt commented Jan 29, 2024

Hello,

we use openshift-routes as part of our internal PaaS together with Let's Encrypt (for a couple hundred websites).
As @ctml91 already pointed out, it's important for us that users can keep using the native OpenShift Route resources (and maybe need to add a couple of annotation), but don't need to rewrite their deployment manifests to use Ingress.

We were previously using the https://github.com/tnozicka/openshift-acme project (long deprecated) for the same purpose, but have switched to openshift-routes because it's an official cert-manager project.

I'd prefer if openshift-routes used Certificate resources, and allow cert-manager to handle using all the logic built into it already. IMO there is no difference in having the PK storage directly in the Route or Secret

I fully agree with this and am heavily in favor of switching to Certificates. As I have already pointed out in #42 , we are currently a bit blind what's going on with the CertfificateRequests created by openshift-routes in our clusters simply because there are no Prometheus metrics available for it. This lead to some expired certificates in the past.

@jacksgt
Copy link
Contributor

jacksgt commented Feb 7, 2024

I'll put together a PR that provisions Certificates instead of CertificateRequest in the next couple of days.
Then we can discuss there what the implications of that are.

jacksgt added a commit to jacksgt/openshift-routes that referenced this issue Feb 13, 2024
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]>
@maelvls maelvls changed the title What do you use Openshift-routes for? What do you use Openshift-routes for? And why would you prefer Certificates to be created rather than CRs? Mar 1, 2024
@maelvls
Copy link
Member Author

maelvls commented Mar 1, 2024

I mentioned #55 during this morning's cert-manager open standup. I thought I understood the benefits of migrating to creating Certificates, but I struggled explaining the reasons. Would it be possible to meet at the next cert-manager community meeting (Thu 7 March 18:00 Paris time)?

Before then, Ill try to gather the pros and cons of each approach.

@ctrought
Copy link
Contributor

ctrought commented Mar 1, 2024

Some benefits of moving to Certificates

  1. increasing observability Monitoring observability for "CertificateRequests" #42
  2. the certificate will be stored in standard kubernetes secrets, allowing one to use elsewhere if desired. For example, for re-encrypt Routes one may also want to mount that certificate in the pod for the application to use as well. This cannot be done currently with openshift-routes because the cert is being stored directly in the Route object.
    • If using a re-encrypt route right now, one would probably have to generate another cert managed by a different process such as a separate Certificate resource in cert-manager or the built in service certificates managed by OpenShift which will both result in a cert that lives in a secret and can be used by the underlying application as a volume mount
  3. it will be simpler to implement sharing of certificates between routes Same certificate in path based Routes #54, similar to how ingress objects can share the underlying cert-manager generated secret whether the certificate being configured using a Certificate or through annotations on one ingress object. Briefly mentioned in the proposed PR Draft: Use Certificates instead of CertificateRequests #55 (comment)
  4. if 3) is implemented and a route can be pointed at a Certificate object, this will enable the use of all fields in a Certificate even if there are not supported annotations to configure those fields and makes the adoption of any new features much simpler as the logic doesn't need to be duplicated into openshift-routes.
    For example, I may want to have a JKS keystore generated for the backend of my application if using a re-encrypt route along with the PEM format used in the Route, leveraging a Certificate I'll be able to get both formats output.
    Another example is Ability to configure CertificateRequest revision history limit #46 where the revision history is not currently supported in openshift-routes but it could still be configured if directly using a Certificate object and linking the Route to it, or using a mutation policy to add it to the Certificate resource when generated by openshift-routes using a tool like Gatekeeper/Kyverno. The option of supporting these features in cert-manager routes would still be possible via annotations the same way it is for cert-manager and ingress objects, but only as a means to configure the Certificate resource and any complex logic can be offloaded to cert-manager.
  5. it will consistent, native ingress objects and routes are arguably the same thing so why should they not function the same way?

I understand initially there was one internal customer who didn't like the idea of having the private key in Secret resources which was a driver for using CertificateRequests, but as the adoption grows I think this is most likely not a popular opinion of users of openshift-routes. The default aggregated ClusterRole view in Kubernetes does not provide GET permissions to Secrets while in OpenShift it does provide GET access to Routes making it less secure by RBAC standards so there should imo not be any negative of using Secrets.

Personally I think there are some clear benefits of using Certificates to manage the lifecycle of the cert. On the other hand, the benefits of configuring CertificateRequests directly are not very clear imo.

I tried to summarize as best as I could, but please add to this if there are any other use cases I missed.

@maelvls
Copy link
Member Author

maelvls commented Mar 7, 2024

Thank you @ctrought, that is super useful, you perfectly laid out the pros and cons. @jacksgt Do you think you can join the dev meeting this afternoon? Thanks!

@maelvls
Copy link
Member Author

maelvls commented Mar 7, 2024

Increasing observability Monitoring observability for "CertificateRequests"

Fair enough, that's a good point. But if this is the primary reason for creating Certificates, wouldn't it make more sense for openshift-routes to expose its own Routes metrics that mimic cert-manager's metrics to get the same result?

Sharing of certificates between routes

[Attach multiple routes to the same Certificate] should be possible with an annotation like cert-manager.io/certificate-name, which would solve #54 (#55 (comment))

How big is this need? Is that something folks do often? I've read that it is something people do with Ingresses (cert-manager/cert-manager#841 (comment)) by pre-creating the Certificate resource instead of letting it be created by the ingress shim.

Enable the use of all fields in a Certificate even if there are not supported annotations

Ah, that's an excellent point. Ingresses and Gateways give you the opportunity to use a pre-existing Certificate resource, letting you fine-tune the Certificate, and among others letting you configure the encoding of the certificate.

I also understand for need for the new annotation cert-manager.io/certificate-name. For Ingress and Gateway resources, you just have to reference the Secret in the Ingress. But for Routes, something needs to copy the Secret into the Route's spec.key and spec.certificate. Which could be done by cert-utils-operator I guess.

If using a re-encrypt route along with the PEM format used in the Route.

Ingress and Gateway resources both let you re-encrypt traffic, so I do understand the need here. I can't see how people would be able to decrypt at the backend's site by reading the Route's spec.key and spec.certificate. Reading a Secret is much more convenient.

The option of supporting these features in cert-manager routes would still be possible via annotations the same way it is for cert-manager and ingress objects, but only as a means to configure the Certificate resource and any complex logic can be offloaded to cert-manager.

Just to understand things better, I compared the annotations supported by each:

csi-driver Ingress, Gateway Route
csi.cert-manager.io/issuer-name cert-manager.io/issuer-name
cert-manager.io/issuer cert-manager.io/issuer
cert-manager.io/cluster-issuer cert-manager.io/cluster-issuer
csi.cert-manager.io/issuer-kind cert-manager.io/issuer-kind cert-manager.io/issuer-kind
csi.cert-manager.io/issuer-group cert-manager.io/issuer-group cert-manager.io/issuer-group
csi.cert-manager.io/common-name cert-manager.io/common-name cert-manager.io/common-name
csi.cert-manager.io/dns-names ? cert-manager.io/alt-names
csi.cert-manager.io/ip-sans cert-manager.io/ip-sans cert-manager.io/ip-sans
csi.cert-manager.io/uri-sans cert-manager.io/uri-sans cert-manager.io/uri-sans
cert-manager.io/email-sans cert-manager.io/email-sans
csi.cert-manager.io/duration cert-manager.io/duration cert-manager.io/duration
csi.cert-manager.io/is-ca
csi.cert-manager.io/certificate-file
csi.cert-manager.io/ca-file
csi.cert-manager.io/privatekey-file
csi.cert-manager.io/fs-group
csi.cert-manager.io/renew-before cert-manager.io/renew-before cert-manager.io/renew-before
csi.cert-manager.io/pkcs12-enable
csi.cert-manager.io/pkcs12-filename
csi.cert-manager.io/pkcs12-password
kubernetes.io/tls-acme
acme.cert-manager.io/http01-ingress-class
acme.cert-manager.io/http01-edit-in-place
cert-manager.io/subject-organizations cert-manager.io/subject-organizations
cert-manager.io/subject-organizationalunits cert-manager.io/subject-organizationalunits
cert-manager.io/subject-countries cert-manager.io/subject-countries
cert-manager.io/subject-provinces cert-manager.io/subject-provinces
cert-manager.io/subject-localities cert-manager.io/subject-localities
cert-manager.io/subject-postalcodes cert-manager.io/subject-postalcodes
cert-manager.io/subject-streetaddresses cert-manager.io/subject-streetaddresses
cert-manager.io/subject-serialnumber cert-manager.io/subject-serialnumber
csi.cert-manager.io/key-usages cert-manager.io/usages
cert-manager.io/revision-history-limit
cert-manager.io/private-key-algorithm cert-manager.io/private-key-algorithm
csi.cert-manager.io/key-encoding cert-manager.io/private-key-encoding
cert-manager.io/private-key-size cert-manager.io/private-key-size
csi.cert-manager.io/reuse-private-key cert-manager.io/private-key-rotation-policy

In terms of annotations, there are only missing:

  • revision history limit,
  • key usages.

@maelvls
Copy link
Member Author

maelvls commented Mar 7, 2024

I think [the duplicate logic for handling the creation of certificates] is the case for multiple cert-manager projects: csi-driver, csi-driver-spiffe, istio-csr, ... (#55 (comment))

Routes are meant to behave like Ingress and Gateway resource, aren't they? Ingress and Gateway resources create Certificate resources, it would thus make sense that Routes create Certificates.

As Craig pointed out, Ingress and Gateway resources support re-encryption and sharing certificates across resources. For the sake of consistency, I'd suggest we go with Certificates so that the experience between Ingresses, Gateways, and Routes are similar.

@maelvls
Copy link
Member Author

maelvls commented May 16, 2024

On 7 March 2024, during the dev biweekly meetings (meeting notes), Trilok Geer – from Red hat – talked about OpenShift 4.16 (unreleased) and the new field externalCertificate. This ties well with the idea of creating Certificates instead of CertificateRequests in openshift-routes. Right now, externalCertificate's API isn't clear to us (is it meant to target a Secret resource)? Trilok told me that he will get back to us soon.

On 16 May 2024, we talked a bit more on that topic (meeting notes):

  • On the topic of bringing support for the Routes CRD to cert-manager "core", the status quo hasn't changed: although technically acceptable as long as it stays in a separate binary with its own go.mod, none of us have motivation implementing that. That said, there were concerns over the fact that only OpenShift 4.16 users would be able to benefit this.
  • Red Hat needs to be more involved with these discussions. I'll try to get someone from Red Hat for the next dev biweekly meeting.
  • There needs to be a discussion around how Red Hat contributes to cert-manager, in particular the openshift-routes project. Right now, openshift-routes doesn't have end-to-end tests because OpenShift costs a fortune to run.

@maelvls maelvls changed the title What do you use Openshift-routes for? And why would you prefer Certificates to be created rather than CRs? Moving from Certificates to CertificateRequests: A Survey May 24, 2024
@hawksight
Copy link
Member

Moving from Certificates to CertificateRequests: A Survey

@maelvls is the title the wrong way round? And should be:

Moving from CertificateRequests to Certificates: A Survey

@TrilokGeer
Copy link

@maelvls , thanks for sharing the collective date on annotations. This gives a better insight about compatibility with configurations supported by annotations.

externalCertificate's API isn't clear to us (is it meant to target a Secret resource)?

Yes, the externalCertificate targets a secret resource. The secret is generated and managed by cert-manager via the usual flow of Certificate resource allowing integration with cert-manager. This approach overcomes the problem of private key in route configuration.

@mf-of
Copy link

mf-of commented Jun 12, 2024

Just to chime in here: I fully agree with @jacksgt here, for exactly the same reasons as he points out. We've seen certificates that weren't able to be renewed at all unless manual action was taken, which kind of defeats the purpose of cert-manager altogether (removing Order and CertificateRequest was required in all cases we've seen). I'm hoping that using Certificates instead of CertificateRequests will improve this.

We haven't been using it all that long yet, but the combination of cert-manager, openshift-routes addition, and Let's Encrypt has been problematic so far (curiously, no issues when Let's Encrypt is replaced with a local Step CA, with renewals every 30m).

@maelvls maelvls changed the title Moving from Certificates to CertificateRequests: A Survey Moving from CertificateRequests to Certificates: A Survey Jul 18, 2024
SgtCoDFish pushed a commit to SgtCoDFish/cert-manager-openshift-routes that referenced this issue Sep 30, 2024
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 pushed a commit to SgtCoDFish/cert-manager-openshift-routes that referenced this issue Oct 2, 2024
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
Copy link
Member

We've now merged #101 which makes the switch to using Certificates unilaterally. It'll be included in the next release which should be coming soon!

@Bengrunt
Copy link

Bengrunt commented Oct 3, 2024

We've now merged #101 which makes the switch to using Certificates unilaterally. It'll be included in the next release which should be coming soon!

Hello, thanks everyone for that piece of work!
For us this will solve the issue of having concurrent certificate requests failing randomly in situations with multiple routes using the same domain but different subpaths.

Any insight on how transition will occur from the CertificateRequests to the Certificates mode ? Will existing requests be picked up and certificates moved to new Certificates ? I guess I'll have to try :)

Thanks !

@SgtCoDFish
Copy link
Member

Any insight on how transition will occur from the CertificateRequests to the Certificates mode ? Will existing requests be picked up and certificates moved to new Certificates ? I guess I'll have to try :)

First of all: please do try and report back! If there's anything we need to tweak now's the time.

To answer your question: after upgrading, the same renewal logic as before will be used in that if a cert is found on the route and it's valid and doesn't need to be renewed, it'll be left in place.

When the cert is renewed, it'll transparently be renewed just with a Certificate rather than a CertificateRequest.

I triggered a renewal locally for testing by manually deleting the certificate from the Route, which immediately triggered a reissuance.

@SgtCoDFish SgtCoDFish unpinned this issue Oct 3, 2024
@jacksgt
Copy link
Contributor

jacksgt commented Oct 4, 2024

Thanks for your input and considerations everyone, I'm really happy to see this has been released. :-)

@mf-of
Copy link

mf-of commented Oct 9, 2024

It looks good, certificates get created on (forced) renewal. I did notice that one of the annotations (cert-manager.io/private-key-rotation-policy: always) now requires a capital to work (so 'Always' instead of 'always'). It complained about that in the logging. Did I miss some notice about that, or am I doing something wrong? It would be awkward to have to replace that everywhere, but doable, I suppose.

@SgtCoDFish
Copy link
Member

I did notice that one of the annotations (cert-manager.io/private-key-rotation-policy: always) now requires a capital to work (so 'Always' instead of 'always').

Well, I've got good and bad news.

The bad is you probably have to update it everywhere, sorry.

The good is that it never worked anyway - I don't think the old CertificateRequest approach supported that key at all, so it was being ignored entirely before! I added support for it while finishing up the Certificate work 😁

@mf-of
Copy link

mf-of commented Oct 10, 2024

Well, then I hope everyone listens when I say they'll need to use a capital letter there, or drops the annotation entirely. I can't say I verified that it worked with CertificateRequests, so I'll take your word for it not working then. I'll definitely check now though 😄

@Bengrunt
Copy link

Bengrunt commented Oct 15, 2024

Hello, I deployed the new release (v0.7.0) using Helm on our preproduction cluster (OKD 4.15) and we encountered an issue : there seem to be a new default renew-before setting with a value of 0s on every existing route that prevents cert-manager to provision the new certificate.

On the openshift-routes controller logs we see:

I1015 20:41:45.067239 1 sync.go:81] "msg"="Route has no matching certificate" "logger"="cert-manager-openshift-routes.route.sync" "name"="website-dev" "namespace"="hosted-website-dev" "resourceVersion"="1079372362" "route"={"Namespace":"hosted-website-dev","Name":"website-dev"}
E1015 20:41:45.070834 1 sync.go:62] "msg"="error while reconciling" "error"="admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "logger"="cert-manager-openshift-routes.route" "object"={"Namespace":"hosted-website-dev","Name":"website-dev"}
E1015 20:41:45.070885 1 controller.go:316] "msg"="Reconciler error" "error"="admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "Route"={"name":"website-dev","namespace":"hosted-website-dev"} "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "logger"="cert-manager-openshift-routes.controller-manager" "name"="website-dev" "namespace"="hosted-website-dev" "reconcileID"="d8e44752-f08d-4f31-a2c1-2d41af711260"

I tried to set the cert-manager.io/renew-before annotation on one of the Routes and it allowed for the generation of the Certificate, ensuring proper operation of the deployment.

Additionally all existing certificates were removed from the Routes data, hence all deployments using cert-manager are now running without certificate.

This is not a smooth transition :D This behavior was clearly not expected, did I miss something ?
Is cert-manager.io/renew-before now mandatory ? Is it configurable at the Issuer or ClusterIssuer level ? Why does this annotation not support d, y as units for "days" and "years".

Thanks !

EDIT: The ClusterIssuer used in this scenario is using letsencrypt to issue certificates.

@ctml91
Copy link

ctml91 commented Oct 15, 2024

Hello, I deployed the new release (v0.7.0) using Helm on our preproduction cluster (OKD 4.15) and we encountered an issue : there seem to be a new default renew-before setting with a value of 0s on every existing route that prevents cert-manager to provision the new certificate.

On the openshift-routes controller logs we see:

I1015 20:41:45.067239 1 sync.go:81] "msg"="Route has no matching certificate" "logger"="cert-manager-openshift-routes.route.sync" "name"="website-dev" "namespace"="hosted-website-dev" "resourceVersion"="1079372362" "route"={"Namespace":"hosted-website-dev","Name":"website-dev"}
E1015 20:41:45.070834 1 sync.go:62] "msg"="error while reconciling" "error"="admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "logger"="cert-manager-openshift-routes.route" "object"={"Namespace":"hosted-website-dev","Name":"website-dev"}
E1015 20:41:45.070885 1 controller.go:316] "msg"="Reconciler error" "error"="admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "Route"={"name":"website-dev","namespace":"hosted-website-dev"} "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "logger"="cert-manager-openshift-routes.controller-manager" "name"="website-dev" "namespace"="hosted-website-dev" "reconcileID"="d8e44752-f08d-4f31-a2c1-2d41af711260"

I tried to set the cert-manager.io/renew-before annotation on one of the Routes and it allowed for the generation of the Certificate, ensuring proper operation of the deployment.

Additionally all existing certificates were removed from the Routes data, hence all deployments using cert-manager are now running without certificate.

This is not a smooth transition :D This behavior was clearly not expected, did I miss something ? Is cert-manager.io/renew-before now mandatory ? Is it configurable at the Issuer or ClusterIssuer level ? Why does this annotation not support d, y as units for "days" and "years".

Thanks !

EDIT: The ClusterIssuer used in this scenario is using letsencrypt to issue certificates.

I noticed the same as you. Looks like a bug, the default value on Certificate resources should be empty if no annotation is provided which would then rely on cert-manager defaults of 2/3 duration.

@Bengrunt
Copy link

I gathered a little bit more intel on the side effect of this change, I believe the cert-manager admission webhook denies requests made by openshift-routes controller when the renewBefore parameter is unset.

Should I create a new issue @maelvls ?

@SgtCoDFish
Copy link
Member

Thanks for the reports! I can confirm that cert-manager will reject a cert with a renewBefore of zero, but I don't know why that value is getting set for you in your specific use case. I can't see anywhere where we'd set that value and you mentioned "there seem to be a new default renew-before setting with a value of 0s on every existing route that prevents cert-manager to provision the new certificate."

I don't know where that value would be set. But I do think I can defend against it. I'll look into releasing an alpha release soon with a patch to omit the renewBefore if it's 0. That still doesn't explain where it comes from, but it might help with the problem at hand!

@Bengrunt
Copy link

Bengrunt commented Oct 17, 2024

Thanks for the work on this @SgtCoDFish !

Here are the relevant logs we get when this error occurs:

I1017 08:53:41.645407 1 controller.go:302] "msg"="Reconciling" "Route"={"name":"mysite-staging-nginx","namespace":"mysite"} "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "logger"="cert-manager-openshift-routes.controller-manager" "name"="mysite-staging-nginx" "namespace"="mysite" "reconcileID"="6ccfb347-338f-44b3-b3d5-6165d621c53b"
I1017 08:53:41.645443 1 controller.go:75] "msg"="started reconciling" "logger"="cert-manager-openshift-routes.route" "object"={"Namespace":"mysite","Name":"mysite-staging-nginx"}
I1017 08:53:41.651285 1 controller.go:83] "msg"="retrieved route" "logger"="cert-manager-openshift-routes.route" "object"={"Namespace":"mysite","Name":"mysite-staging-nginx"}
I1017 08:53:41.651308 1 controller.go:60] "msg"="Route has the annotation" "annotation-key"="cert-manager.io/issuer-name" "annotation-value"="acme-letsencrypt-prod" "logger"="cert-manager-openshift-routes.route" "object"={"Namespace":"mysite","Name":"mysite-staging-nginx"}
I1017 08:53:41.651664 1 app.go:93] "msg"="Event(v1.ObjectReference{Kind:\"Route\", Namespace:\"mysite\", Name:\"mysite-staging-nginx\", UID:\"989dcc15-254a-427d-aea2-0fda7ea3f98f\", APIVersion:\"route.openshift.io/v1\", ResourceVersion:\"1613290889\", FieldPath:\"\"}): type: 'Normal' reason: 'Issuing' Issuing cert as the existing cert is more than 2/3 through its validity period" "logger"="cert-manager-openshift-routes.controller-manager"
I1017 08:53:41.653895 1 sync.go:81] "msg"="Route has no matching certificate" "logger"="cert-manager-openshift-routes.route.sync" "name"="mysite-staging-nginx" "namespace"="mysite" "resourceVersion"="1613290889" "route"={"Namespace":"mysite","Name":"mysite-staging-nginx"}
E1017 08:53:41.676938 1 sync.go:62] "msg"="error while reconciling" "error"="admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "logger"="cert-manager-openshift-routes.route" "object"={"Namespace":"mysite","Name":"mysite-staging-nginx"}
E1017 08:53:41.676991 1 controller.go:316] "msg"="Reconciler error" "error"="admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "Route"={"name":"mysite-staging-nginx","namespace":"mysite"} "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "logger"="cert-manager-openshift-routes.controller-manager" "name"="mysite-staging-nginx" "namespace"="mysite" "reconcileID"="6ccfb347-338f-44b3-b3d5-6165d621c53b"
I1017 08:53:41.677062 1 app.go:93] "msg"="Event(v1.ObjectReference{Kind:\"Route\", Namespace:\"mysite\", Name:\"mysite-staging-nginx\", UID:\"989dcc15-254a-427d-aea2-0fda7ea3f98f\", APIVersion:\"route.openshift.io/v1\", ResourceVersion:\"1613290889\", FieldPath:\"\"}): type: 'Warning' reason: 'InternalReconcileError' error while reconciling: admission webhook \"validate.webhooks.cert-manager.io\" denied the request: spec.renewBefore: Invalid value: 0s: certificate renewBefore must be greater than 5m0s" "logger"="cert-manager-openshift-routes.controller-manager"

That's why I suspect an issue with either the value that is set by default for renewBefore when no annotation specifies it, or something not correctly handled by the cert-manager admission webhook.

FYI, the cert-manager operator on the cluster in running v1.15.2 and is handled by OLM.

Please tell me if I can help further!

@SgtCoDFish
Copy link
Member

@Bengrunt & @ctml91 - I've just released openshift-routes v0.7.1-beta.0 which includes a potential fix for this. Are you able to test and confirm if this helps?

@Bengrunt
Copy link

@SgtCoDFish I've just deployed it on our preproduction cluster instead of release 0.7.0 and it now works. I'm just going to do some more extensive tests before rolling it out on the production cluster but everything seems fine so far. Thanks a lot !

@inteon
Copy link
Member

inteon commented Oct 22, 2024

We switched openshift-routes to Certificates.
I'll close the issue, but feel free to post additional messages if you have more feedback.

@inteon inteon closed this as completed Oct 22, 2024
@SgtCoDFish
Copy link
Member

@Bengrunt thank you for testing! I'll release v0.7.1 proper later this week to give people a little more time to test 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests