openshift-routes icon indicating copy to clipboard operation
openshift-routes copied to clipboard

Published 20 hours ago • verified publisher icon cert-manager

Draft: Use Certificates instead of CertificateRequests

Open jacksgt opened this issue 1 year ago • 9 comments

As discussed in https://github.com/cert-manager/openshift-routes/issues/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.

jacksgt avatar Feb 13 '24 09:02 jacksgt

[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 assign wallrj for approval. 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

jetstack-bot avatar Feb 13 '24 09:02 jetstack-bot

I have not touched the tests yet as I first want to invite some discussion about this proposal. But I already gave it a try on my local cluster:

Create a local ClusterIssuer:

oc apply -f - <<EOF
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: example
  namespace: openshift-cern-cert-manager
spec:
  isCA: true
  privateKey:
    algorithm: ECDSA
    size: 256
  secretName: example
  commonName: example root
  duration: 262800h
  issuerRef:
    name: selfsigned
    kind: ClusterIssuer
---
apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: example
  namespace: openshift-cern-cert-manager
spec:
  ca:
    secretName: example
EOF

oc get clusterissuer
NAME               READY   AGE
example            True    32m

Create a Route with annotations for openshift-routes:

oc create -f - <<EOF
apiVersion: route.openshift.io/v1
kind: Route
metadata:
  name: example-route
  annotations:
    cert-manager.io/issuer-name: example
    cert-manager.io/issuer-kind: ClusterIssuer
  namespace: default
spec:
  wildcardPolicy: None
  to:
    name: httpbin
EOF

Start openshift-routes locally:

go run internal/cmd/main.go --namespace openshift-cern-cert-manager --enable-leader-election=false
I0213 10:32:31.652525   30565 internal.go:360] cert-manager-openshift-routes/controller-manager "msg"="Starting server" "addr"={"IP":"::","Port":6060,"Zone":""} "kind"="health probe"
I0213 10:32:31.652683   30565 controller.go:177] cert-manager-openshift-routes/controller-manager "msg"="Starting EventSource" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "source"="kind source: *v1.Route"
I0213 10:32:31.652714   30565 server.go:50] cert-manager-openshift-routes/controller-manager "msg"="starting server" "addr"={"IP":"::","Port":9402,"Zone":""} "kind"="metrics" "path"="/metrics"
I0213 10:32:31.652789   30565 controller.go:177] cert-manager-openshift-routes/controller-manager "msg"="Starting EventSource" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "source"="kind source: *v1.Certificate"
I0213 10:32:31.652861   30565 controller.go:177] cert-manager-openshift-routes/controller-manager "msg"="Starting EventSource" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "source"="kind source: *v1.Secret"
I0213 10:32:31.652909   30565 controller.go:185] cert-manager-openshift-routes/controller-manager "msg"="Starting Controller" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route"
I0213 10:32:32.162140   30565 controller.go:219] cert-manager-openshift-routes/controller-manager "msg"="Starting workers" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route" "worker count"=1
  C-c C-cI0213 10:32:55.003628   30565 internal.go:581] cert-manager-openshift-routes/controller-manager "msg"="Stopping and waiting for non leader election runnables" 
I0213 10:32:55.003776   30565 server.go:43] cert-manager-openshift-routes/controller-manager "msg"="shutting down server" "addr"={"IP":"::","Port":9402,"Zone":""} "kind"="metrics" "path"="/metrics"
I0213 10:32:55.003897   30565 internal.go:585] cert-manager-openshift-routes/controller-manager "msg"="Stopping and waiting for leader election runnables" 
I0213 10:32:55.003968   30565 controller.go:239] cert-manager-openshift-routes/controller-manager "msg"="Shutdown signal received, waiting for all workers to finish" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route"
I0213 10:32:55.004010   30565 controller.go:241] cert-manager-openshift-routes/controller-manager "msg"="All workers finished" "controller"="route" "controllerGroup"="route.openshift.io" "controllerKind"="Route"
I0213 10:32:55.004043   30565 internal.go:591] cert-manager-openshift-routes/controller-manager "msg"="Stopping and waiting for caches" 
I0213 10:32:55.004332   30565 internal.go:595] cert-manager-openshift-routes/controller-manager "msg"="Stopping and waiting for webhooks" 
I0213 10:32:55.004391   30565 internal.go:599] cert-manager-openshift-routes/controller-manager "msg"="Wait completed, proceeding to shutdown the manager"

Check resources:

$ oc -n default get route example-route -o yaml
apiVersion: route.openshift.io/v1
kind: Route
metadata:
  annotations:
    cert-manager.io/issuer-kind: ClusterIssuer
    cert-manager.io/issuer-name: example
    openshift.io/host.generated: "true"
  creationTimestamp: "2024-02-13T09:25:07Z"
  name: example-route
  namespace: default
  resourceVersion: "469150287"
  uid: d0600c4b-cb06-4fef-ba0b-73b1a0e10ee0
spec:
  host: example-route-default.example.com
  tls:
    certificate: |
      -----BEGIN CERTIFICATE-----
      MIICcjCCAhmgAwIBAgIRAOFkhOoNFxv59rrOVz7dkEswCgYIKoZIzj0EAwIwFzEV
      MBMGA1UEAxMMZXhhbXBsZSByb290MB4XDTI0MDIxMzA5Mjc0MVoXDTI0MDUxMzA5
      Mjc0MVowADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKJGBQ7ndoip
      nxY7/+8wT3rLrOHwMfaJ6m52dBVOBELW5rE3WBhrEfuMtISVJ8gfWn7VupT0LUi7
      pnYaFyf0TVf8DpSyL7OMtPKE/ZxFCKOrc5mQiSjWmwSEwQyMYS3cdvadDIzV7kbO
      2DFyvFcATt/yUVwyvNJsxr3oQ7pgQahJyp98RgAU6lCgJ7jfWyVGBinUFGx3yzfD
      fqK4wEKoHn27VKmHiQQB9TssQi9ZHE5Fm66wBCBUc6tIaQlOdpUnE/kHasnZ7LmC
      rITE0ARQcmKBq4dzeyhAV2VNBzDnMP0BOItObn4Sn7YT2/QTmU39tK7DzRxzsQY1
      xtpmL8Q1l0UCAwEAAaOBkTCBjjAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYI
      KwYBBQUHAwEwDAYDVR0TAQH/BAIwADAfBgNVHSMEGDAWgBQxTapkQk5YhD3LbNYc
      0obAFFo8gzA4BgNVHREBAf8ELjAsgipleGFtcGxlLXJvdXRlLWRlZmF1bHQuY2x1
      LWphY2stZGV2LmNlcm4uY2gwCgYIKoZIzj0EAwIDRwAwRAIgXmvW5M340E8eK0pX
      MNX+GnUQTs8qE2jdiPEUfGS2LvsCIAS/qFPaaKj4ARAQpT4Dd7IM2yVK6dDFFvms
      SpVAUfC6
      -----END CERTIFICATE-----
    insecureEdgeTerminationPolicy: Redirect
    key: |
      -----BEGIN RSA PRIVATE KEY-----
      MIIEowIBAAKCAQEAokYFDud2iKmfFjv/7zBPesus4fAx9onqbnZ0FU4EQtbmsTdY
      GGsR+4y0hJUnyB9aftW6lPQtSLumdhoXJ/RNV/wOlLIvs4y08oT9nEUIo6tzmZCJ
      KNabBITBDIxhLdx29p0MjNXuRs7YMXK8VwBO3/JRXDK80mzGvehDumBBqEnKn3xG
      ABTqUKAnuN9bJUYGKdQUbHfLN8N+orjAQqgefbtUqYeJBAH1OyxCL1kcTkWbrrAE
      IFRzq0hpCU52lScT+QdqydnsuYKshMTQBFByYoGrh3N7KEBXZU0HMOcw/QE4i05u
      fhKfthPb9BOZTf20rsPNHHOxBjXG2mYvxDWXRQIDAQABAoIBADWOAEtb7o3J1Twk
      TyIkgoaXQ5ZJjGO+PoV4SHVjixp4DCi+iC9+3q9zT3xWMYvldRtY9DwGng9cBuMB
      V1UTVpdME4/VgtKyBGHprD1vtxs1EXDD99Bniz+hhIjcp5HYKdbYG/U7AWmTCFzB
      bhEUg/N66IkSIakcxzaTug5/iAu+0DoMVvKhiLkCSH6QYmi16Fxic0eGu7OlMVHu
      2wmQmMnu/5ExOj4RRYIzSqm9y4fNyW2Z7hWg/9Gx+ATw+ZdulUhMxJHjGxvAGR+3
      6GXoQ2qUNaYvbe5x2yiQbQNShxfuMnwdSpX0VCXCz6C1z5w2S/osmMEWCB2W+smz
      wzcQGd0CgYEAznQRlAnJUHOxeugOUxKq3gdKHbqSEiEPzMcRboqhu4fU2g5tKq8m
      VsHBCp/2wIfgpGmfSOpX67EDVWgQBTTq8NYZ9hpKyAep3OkKXR1b9ILoW0A1Gz0c
      VVHGZv2jmoK0pSR3i1QeGq9RtHitwwNrik+VlM9c3hzZ9JzlZ0oSrNsCgYEAyTep
      CQQUGRfaBvXOKuSYkFjbO7v+R6kB60XU+IqvirwksdSIlG4fH+ITIqtS0I2mJ9cI
      tdGj4uezo8f1nCJp4vmelR7SLqmMJqPrHe8aKWbsgX/XoBUvO7lpa20660DYC7v3
      XmipwCttPQp9R8MrLbc+wW+zNcUdp4EdiiCX9l8CgYBdJp2vz+KXfDv+Gqor7WZP
      G7bjRwUVTPmWCdPhrode1+DAKnYzJigESRPSuW5aXHSNemK2QZY97/ZzGKrxznib
      Bd9c3WwUaPDJjhRxAwg0gMRaN9Q+YApirKz6V0L0OjlLsfKGWQPkQmp5JWIxdV+W
      XmY9aHqcdSQabJhNTGy0tQKBgQCwW3hrzodO9vjA4O+x6GlPGpIL6NkVNavY6Xuf
      2u3ASuZedki+z0W4TA05da8/2uamRHH96aAaX7my8q7yCbeEmAPF7x2IiFGuDD0m
      H0puvybK2aHDTM35Kqia30Gkr1Cr+DL3LASbyXQU6/yhyQ0vJEx8fco0dm9nQGMD
      jU2jQQKBgBqSwHhguULjemX42K/FUt2eFZzsNfuAvxiyKdC+HUcMWiiWb14dTnwZ
      Ftg0b/LaIyackfKH80dQ+sDquXOj94tH6knaDo4GZMjp0Rk8fkZqy97XOe4miaHE
      PJpxIVeJD2wX27I/lPHcGu2JHMJ+mI8Qn2+a5XZwWwX3vdSXrJ1i
      -----END RSA PRIVATE KEY-----
    termination: edge
  to:
    kind: Service
    name: httpbin
    weight: 100
  wildcardPolicy: None
status:
  ingress:
  - conditions:
    - lastTransitionTime: "2024-02-13T09:25:07Z"
      status: "True"
      type: Admitted
    host: example-route-default.example.com
    routerCanonicalHostname: router-apps-shard-1.example.com
    routerName: apps-shard-1
    wildcardPolicy: None

$ oc -n default get certificate
NAME                  READY   SECRET                   AGE
example-route-xs7vt   True    example-route-tls-cert   84s

$ oc -n default get secret | grep route
example-route-tls-cert     kubernetes.io/tls                     3      99s

TODOs:

  • how to handle deletion of secrets / secrets that already exist?
  • update tests (after first code review)
  • update RBAC in deployment manifests / Helm chart (after first code review)
  • edge cases?

jacksgt avatar Feb 13 '24 09:02 jacksgt

Looking forward to this!

It would be great if we could also attach multiple routes to the same Certificate, this makes most sense for path based routes or any specifying alt names via cert-manager.io/alt-names. Could have one of the routes being the manager for the certificate via annotations, and other routes specify the exact Certificate to use via an annotation. That would require the Certificate name to be predictable (e.g. match name of owner route), alternatively point directly to the secret (via annotation) similar to how ingress works today where the configured cert does not necessarily need to be configured via ingress annotations.

ctrought avatar Feb 13 '24 19:02 ctrought

It would be great if we could also attach multiple routes to the same Certificate

I agree, this should be easily possible with an annotation like cert-manager.io/certificate-name, which would solve https://github.com/cert-manager/openshift-routes/issues/54 This would also allow the use of wildcard certificates, as requested here: https://github.com/cert-manager/openshift-routes/issues/13

But first this initial draft I didn't want to overcomplicate and introduce new features. :-)

jacksgt avatar Feb 14 '24 08:02 jacksgt

Hi @maelvls , would you mind taking a look at this?

jacksgt avatar Feb 26 '24 12:02 jacksgt

this project (openshift-routes) has to duplicate logic for handling the creation of certificates that already exists in cert-manager.

I think this is the case for multiple cert-manager projects: csi-driver, csi-driver-spiffe, istio-csr, ... See https://cert-manager.io/docs/usage/ for an overview of the different ways CRs are created.

inteon avatar Mar 01 '24 11:03 inteon

I responded to Tim's comment in https://github.com/cert-manager/openshift-routes/issues/49#issuecomment-1983836956.

Just wondering, how much effort would it be able to use one or the other (either create a CR or create a Certificate)? @jacksgt

maelvls avatar Mar 07 '24 16:03 maelvls

Just wondering, how much effort would it be able to use one or the other (either create a CR or create a Certificate)?

I think the initial implementation would not be an issue. In that case I'd suggest having two separate sync functions (one for each "mode"). However I'm not sure how maintainable and understandable this code will be over time ...

jacksgt avatar Mar 08 '24 16:03 jacksgt

PR needs rebase.

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.

cert-manager-prow[bot] avatar Aug 22 '24 12:08 cert-manager-prow[bot]

Thanks so much for the initial work on this @jacksgt - really appreciate it and sorry it took a while to get it merged.

We've now merged #101 which got this merged. I preserved your original commit so you'll get credit for the contribution.

I hope you're able to test this out when we release it soon!

I'll close this now since it's merged.

SgtCoDFish avatar Oct 03 '24 12:10 SgtCoDFish