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

Support Route.spec.subdomain #27

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

jacksgt
Copy link
Contributor

@jacksgt jacksgt commented May 3, 2023

Hi there!

As discussed at KubeCon, I had this patch sitting on my machine for quite a while - sorry for the delay!

In summary, this patch implements support for the OpenShift Route "subdomain" enhancement (added in OpenShift 4.11).
For reference, this is the commit that introduces the change: openshift/router@6f730c7

And here are the release notes for 4.11: https://docs.openshift.com/container-platform/4.11/release_notes/ocp-4-11-release-notes.html#ocp-4-11-routes-spec-subdomain-field

Previously, you could not specify the subdomain of a route, and the spec.host field was required to set the host name. You can now specify the spec.subdomain field and omit the spec.host field of a route. The router deployment that exposes the route will use the spec.subdomain value to determine the host name.

Closes #10


Status of this PR:

The basic functionality should work, but I haven't added any tests yet because I would first like to know if the maintainers are comfortable with the direction of the code. :-)
I will add tests once we have an agreement.

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2023
@maelvls
Copy link
Member

maelvls commented May 9, 2023

Hi! Thank you for submitting this PR! I'll take a look shortly (hopefully this afternoon).

Copy link
Member

@maelvls maelvls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, these changes look great. Thanks for the well-written comments in code, that helped a lot.

I wasn't able to test yet because the image openshift/origin-haproxy-router:v4.11.0 has not been published yet. Also, I don't know how to build that image, the upstream Dockerfile refers to a registry for which I don't have access 😬 The image is available at: quay.io/openshift/origin-haproxy-router:4.11.

For reference, here are the steps I wanted to take to test the feature. I wish we had some automated tests, but I don't really have the time to write some at the moment:

  1. Install OpenShift Route CRDs and controller:

    kind create cluster
    kubectl apply -f https://raw.githubusercontent.com/openshift/router/master/deploy/route_crd.yaml
    kubectl apply -f https://raw.githubusercontent.com/openshift/router/master/deploy/router_rbac.yaml
    kubectl apply -f https://raw.githubusercontent.com/openshift/router/master/deploy/router.yaml
    kubectl set image -n openshift-ingress deploy/ingress-router router=quay.io/openshift/origin-haproxy-router:4.11
    kubectl set env -n openshift-ingress deploy/ingress-router ROUTER_DOMAIN=domain.com
    kubectl apply -f https://github.com/jetstack/cert-manager/releases/download/v1.13.0/cert-manager.yaml
  2. Run openshift-routes:

    go run ./internal/cmd/ -v=5
  3. Create a cert-manager ClusterIssuer:

    kubectl apply -f- <<EOF
    apiVersion: cert-manager.io/v1
    kind: Issuer
    metadata:
      name: self-signed
      namespace: cert-manager
    spec:
      selfSigned: {}
    ---
    apiVersion: cert-manager.io/v1
    kind: Certificate
    metadata:
      name: example
      namespace: cert-manager
    spec:
      isCA: true
      privateKey:
        algorithm: ECDSA
        size: 256
      secretName: example
      commonName: example root
      duration: 262800h
      issuerRef:
        name: self-signed
        kind: Issuer
    ---
    apiVersion: cert-manager.io/v1
    kind: ClusterIssuer
    metadata:
      name: example
      namespace: cert-manager
    spec:
      ca:
        secretName: example
    EOF
  4. Create a Route with the subdomain foo. Since openshift-ingress has been
    configured with ROUTER_DOMAIN=domain.com, we don't need to specify the full
    domain. The actual hostname will be foo.domain.com.

    kubectl apply -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
    spec:
      subdomain: foo
      wildcardPolicy: None
      to:
        name: httpbin
    EOF
  5. Get the IP of the node (WARNING: this part only works from the Docker host; it likely won't work on macOS):

    IP=$(kubectl get nodes -ojson | jq '.items[].status.addresses[] | select(.type == "InternalIP").address' -r)
  6. Confirm that the issued cert's SAN is foo.domain.com:

    openssl s_client -connect $IP:443 -servername foo.domain.com 2>/dev/null <<<"" \
      | openssl x509 -noout -text \
      |  awk '/Subject: C=/{printf $NF"\n"} /DNS:/{x=gsub(/ *DNS:/, ""); printf "SANS=" $0"\n"}'

    It should show:

    SANS=foo.domain.com
    

internal/controller/sync.go Outdated Show resolved Hide resolved
internal/controller/sync.go Outdated Show resolved Hide resolved
internal/controller/sync.go Show resolved Hide resolved
@jacksgt
Copy link
Contributor Author

jacksgt commented May 9, 2023

Thanks for taking the time to review!
The image for the OpenShift router is available on Quay: https://quay.io/repository/openshift/origin-haproxy-router?tab=tags&tag=latest
quay.io/openshift/origin-haproxy-router:4.11

I'll follow your detailed testing steps and report back.

@jacksgt
Copy link
Contributor Author

jacksgt commented May 15, 2023

OpenShift 4.11 cluster

# oc version
Client Version: 4.11.13
Kustomize Version: v4.5.4
Server Version: 4.11.0-0.okd-2022-11-05-030711
Kubernetes Version: v1.24.0-2566+5157800f2a3bc3-dirty

Create ClusterIssuers

(selfsigned issuer already exists)

# 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    13s
letsencrypt   True    115d
selfsigned    True    115d

Create Route using subdomain

# oc apply -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:
  subdomain: foo
  wildcardPolicy: None
  to:
    name: httpbin
EOF
# oc get route example-route -o yaml
apiVersion: route.openshift.io/v1
kind: Route
metadata:
  annotations:
    cert-manager.io/certificate-revision: "1"
    cert-manager.io/issuer-kind: ClusterIssuer
    cert-manager.io/issuer-name: example
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"route.openshift.io/v1","kind":"Route","metadata":{"annotations":{"cert-manager.io/issuer-kind":"ClusterIssuer","cert-manager.io/issuer-name":"example"},"name":"example-route","namespace":"default"},"spec":{"subdomain":"foo","to":{"name":"httpbin"},"wildcardPolicy":"None"}}
  creationTimestamp: "2023-05-15T13:41:48Z"
  name: example-route
  namespace: default
  resourceVersion: "126414555"
  uid: 97b465c0-0a60-46b5-8260-5984e17dccf8
spec:
  subdomain: foo
  tls:
    certificate: |
      -----BEGIN CERTIFICATE-----
      MIICXjCCAgSgAwIBAgIQUXdQ5eOIgk2qAkvAEeDB5zAKBggqhkjOPQQDAjAXMRUw
      EwYDVQQDEwxleGFtcGxlIHJvb3QwHhcNMjMwNTE1MTM0MTQ5WhcNMjMwODEzMTM0
      MTQ5WjAAMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxQMJFxi2hH1/
      lOpfiWLN/E4JVLOvJxbjiIdod9sbXe0jgRcIWlXtRVaS1tgk9hscI2QoVbkQVEJy
      a6q3WOlIySxiMV1CYmtmHAuwb19QE8ca0vPSopyc3Gr54SJ5Q4sk8PDHqp7QL76l
      SYhfUvp+xyZ0QiMssWPQ1ZZixXMPnp+dIhtvqHW+8grxoNw+3+nAScjOBdg9PNQP
      zpKIGZvkyzMykqO5cJhgPLgrqdnDHsNR9CBkNRJwa1gstvB3loy3ukkJ3uK1uki2
      P3tCEDkvwX0mHse5KsKB1mhsWNYewm+rsjgddjlcVEC37Q3q49sUX9AVG9WiXd4+
      lUeLq/FKlQIDAQABo34wfDAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYIKwYB
      BQUHAwEwDAYDVR0TAQH/BAIwADAfBgNVHSMEGDAWgBQxTapkQk5YhD3LbNYc0obA
      FFo8gzAmBgNVHREBAf8EHDAaghhmb28uY2x1LWphY2stZGV2LmNlcm4uY2gwCgYI
      KoZIzj0EAwIDSAAwRQIhAPzp//LtQgx0gAh4ygAVU5+id1YgyuvXtvhSuaXv6wBE
      AiBml84l3Jbtt819E7LHfSAeUZFx0s/CJpaIpoiIoIo1JA==
      -----END CERTIFICATE-----
    insecureEdgeTerminationPolicy: Redirect
    key: |
      -----BEGIN RSA PRIVATE KEY-----
      MIIEowIBAAKCAQEAxQMJFxi2hH1/lOpfiWLN/E4JVLOvJxbjiIdod9sbXe0jgRcI
      WlXtRVaS1tgk9hscI2QoVbkQVEJya6q3WOlIySxiMV1CYmtmHAuwb19QE8ca0vPS
      opyc3Gr54SJ5Q4sk8PDHqp7QL76lSYhfUvp+xyZ0QiMssWPQ1ZZixXMPnp+dIhtv
      qHW+8grxoNw+3+nAScjOBdg9PNQPzpKIGZvkyzMykqO5cJhgPLgrqdnDHsNR9CBk
      NRJwa1gstvB3loy3ukkJ3uK1uki2P3tCEDkvwX0mHse5KsKB1mhsWNYewm+rsjgd
      djlcVEC37Q3q49sUX9AVG9WiXd4+lUeLq/FKlQIDAQABAoIBAA8DNjQeW/oongo4
      +eK8NrodMDTWJGi0I86JkvpfJey4X1Y87RQFLDl2aWYZvmdKlZBU14YAvi/NiG6P
      6bzKuhMqYKkmVCKv0G4erekuuClpqK+eiNR/XqylMjlnqRnuhngdwPlNdMvOmUXL
      MIhgMjz2vzEzAPrbglRkS06EgBI0MT8X8d9Ix9wiOOsNqS5/LKx4/9mCNff+rA/B
      rkGU2ziyrJ47Gl5buxs82jPhx3pKqEmXyC5jC89UOgRHxG02b5pW25L9TqhDG3+p
      YpmZ7ZkOFUSGmHxDf7qOn54CZfzisMo+GVICnaDm+fmY6TYofdUrQc4LVW+VTx98
      UVRD+AECgYEA65JhD/4LQeUwyqhNZ1m+Ks7H9q7uIhyUvStKMBU0ykb64kK+KsYI
      EB/nQUd5gCcZIDXpm1AhTS0z4ifCHO/bjHi8NleVpBhT6+2PhB55Tj+GopGHpiA7
      5riIf0HfDNJJj2e/R5J+dBroJjJ/bKiBYtolObozJjKAF7EZojnj8wECgYEA1hij
      Qap8KpMnMhSenk7kX3gL/+mfMDw0CtmHzg8cUrjR02SEpG3DMRnYU27IQLVTU97Q
      EXjR26mVgp0IYmSFP0+SeDA2vKnCkPhfeiBEljSTDMyDldEC3T29xusv3jNeg8qU
      igRV8MIUp28oIjbAhkZa1s289m6v7dkIcwBj25UCgYBtALWjBcVFv3x9ObVfTjpd
      aoNClR0hcaItijtw1k8wDfoG+iUue6W9eFW0chM5hl3s9qVatZBDOthYHFeItkAb
      1r4YUxyt6ofiknEnDEbLOz/cN4TSDPZKwzZ91AgW0bMyBSqQPqv+o3iqPBGUQMBl
      8lTpSkxir1lwHQWkeC8PAQKBgAF/BpUM5fU+NgXpkvo0PvBT3HGbZltl8E8rGgHc
      pSwG0qCRkUO86KJNNe/PVGLvYXeylO2qVH0egEb7ZfWaEjxRCAsC+z7ySMPOmIw3
      8YLDN0vdmgXQAh8dsVfUiO8amMx+++7C/P7DvHU3F6a1jz7g+v2JTorCV0RoQeDb
      3dbJAoGBAIdxn9k4NB20pi3p1lZ8GIYku9SrGu03BU+inDYfhmJMVzzjiJuJTbSK
      xVwEh6IAtfurIoG8jlbhvTK6Ma3vncTowuGConspXBA1gTDZRnI7Reb5xG7whYNs
      WjCEp9bdErD/YeNaL6ysWYA7fKVPnUNQZb32X5JbnelF1P/Lryls
      -----END RSA PRIVATE KEY-----
    termination: edge
  to:
    kind: Service
    name: httpbin
    weight: 100
  wildcardPolicy: None
status:
  ingress:
  - conditions:
    - lastTransitionTime: "2023-05-15T13:41:48Z"
      status: "True"
      type: Admitted
    host: foo.okd.example.com
    routerCanonicalHostname: router-default.okd.example.com
    routerName: default
    wildcardPolicy: None

Connect to website to check certificate

openssl s_client -connect $IP:443 -servername foo.okd.example.com 2>/dev/null <<<"" \
  | openssl x509 -noout -text \
  |  awk '/Subject: C=/{printf $NF"\n"} /DNS:/{x=gsub(/ *DNS:/, ""); printf "SANS=" $0"\n"}'

SANS=foo.okd.example.com

Reconfigure namespace so route gets admitted by multiple ingress controllers

note that this is custom label and depends on how OpenShift ingress controllers are set up

# oc label namespace/default okd.example.com/router-shard=all
namespace/default labeled

# oc -n openshift-ingress-operator get ingresscontroller
NAME           AGE
apps-lb-1      4d6h
apps-shard-1   115d
default        107d

# oc annotate route example-route "dummy-update=$(date)"
route.route.openshift.io/example-route annotated

Check new route status

oc get route example-route -o yaml
apiVersion: route.openshift.io/v1
kind: Route
metadata:
  annotations:
    cert-manager.io/certificate-revision: "2"
    cert-manager.io/issuer-kind: ClusterIssuer
    cert-manager.io/issuer-name: example
    dummy-update: Mon May 15 04:03:40 PM CEST 2023
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"route.openshift.io/v1","kind":"Route","metadata":{"annotations":{"cert-manager.io/issuer-kind":"ClusterIssuer","cert-manager.io/issuer-name":"example"},"name":"example-route","namespace":"default"},"spec":{"subdomain":"foo","to":{"name":"httpbin"},"wildcardPolicy":"None"}}
  creationTimestamp: "2023-05-15T13:41:48Z"
  name: example-route
  namespace: default
  resourceVersion: "126434457"
  uid: 97b465c0-0a60-46b5-8260-5984e17dccf8
spec:
  subdomain: foo
  tls:
    certificate: |
      -----BEGIN CERTIFICATE-----
      MIICrDCCAlKgAwIBAgIRAJILfLgHUN1kxPPJkv+fbaQwCgYIKoZIzj0EAwIwFzEV
      MBMGA1UEAxMMZXhhbXBsZSByb290MB4XDTIzMDUxNTE0MDMwN1oXDTIzMDgxMzE0
      MDMwN1owADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALvdsxFyREZH
      9fkgacj1DbQ5HjyW1I20YO3LWwUr0MumGjdT6OZMSx+/Yx0dNfNlnu+syHwkawPl
      PbZ3n0FgYEWGWraawlYMlr57TI9PATk9OepD/P787/qgwMYErH0fpDdpThv+DJwC
      DOuo4pxvfCIhiGqNphLmep+bCpso0euvFLtPhwhRNmF44w2f2oHffybmma44Duo0
      PzhGUP6Dtzml4qJgjpH0fb1bVpZCiUjrhuDknq86H8lCnsoCilOew7s1reCmHh4n
      RhHqaVs+jdXwaz1ineSVUk8OZ4gMEaMMDQlg5q8HRfyqJVjmCZRmul1jNj1G57T1
      /BuhsOeFueMCAwEAAaOByjCBxzAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYI
      KwYBBQUHAwEwDAYDVR0TAQH/BAIwADAfBgNVHSMEGDAWgBQxTapkQk5YhD3LbNYc
      0obAFFo8gzBxBgNVHREBAf8EZzBlghhmb28uY2x1LWphY2stZGV2LmNlcm4uY2iC
      ImZvby5jbHUtamFjay1kZXYtYXBwcy1sYi0xLmNlcm4uY2iCJWZvby5jbHUtamFj
      ay1kZXYtYXBwcy1zaGFyZC0xLmNlcm4uY2gwCgYIKoZIzj0EAwIDSAAwRQIhAP5r
      jJSR66ynMDc7AYEkGE8TQUQa4sMk4a+yLboOMwEBAiAtu+JyV+UaboAG3P6PuIME
      BTOWLbDG779uBscbdZCI2A==
      -----END CERTIFICATE-----
    insecureEdgeTerminationPolicy: Redirect
    key: |
      -----BEGIN RSA PRIVATE KEY-----
      MIIEowIBAAKCAQEAu92zEXJERkf1+SBpyPUNtDkePJbUjbRg7ctbBSvQy6YaN1Po
      5kxLH79jHR0182We76zIfCRrA+U9tnefQWBgRYZatprCVgyWvntMj08BOT056kP8
      /vzv+qDAxgSsfR+kN2lOG/4MnAIM66jinG98IiGIao2mEuZ6n5sKmyjR668Uu0+H
      CFE2YXjjDZ/agd9/JuaZrjgO6jQ/OEZQ/oO3OaXiomCOkfR9vVtWlkKJSOuG4OSe
      rzofyUKeygKKU57DuzWt4KYeHidGEeppWz6N1fBrPWKd5JVSTw5niAwRowwNCWDm
      rwdF/KolWOYJlGa6XWM2PUbntPX8G6Gw54W54wIDAQABAoIBAGlX5sp4pZo3TdFV
      gJwD9ZxCjxbwiN2w8M1Gw5JBwIIBcR/nOGizDUkPG7e+onsKV7YT0BP0o+F6UUGZ
      ED8rmLBDp2hPnXt88aZ8IzEU3x3GnEwltZ8SHEsQiyg+hK5g6Md9kCQYL9/nMnGO
      sQuBZD0LSqBdL2vXu5j16luY5yVdgEj+pRI35OxziDGCiaXl6rgMZQBjCmCOWya2
      YrBoVF3WVokFa6Mq0PEMXXuHQTyW0mJ6pLvl4MoIfyvzRx7rGL3yVwJPi4Xpm6Jj
      dCX9LWF1PLWeE0CEdi/Fxa5IbIUG6keQ3IEmXDmoRxz4cZ5GbgcwYfJ9AjmecTVV
      9Ko/o3kCgYEA6G1trh4SUP8rTd+9bVPOO1FI2chocSUZPgY8M6O2UuFxQDeryiyf
      qxRxYmKe/2YSB3BPqNdopbI9TC1Xc/9+RfFzO7P7JAp7bnInyR4ziiMUGNXA5zdt
      YJVJMQKp7sDHyrir4K6EIV8i10wKRch07K9g+MaMIW0V4CR4lz0RAecCgYEAzutP
      sk1uKuIUmEy/G1vzIAAr+khh0StQqQKaY2o7dYkxnZDcNH+XNbSy0eTfxRJksjUI
      6Sy9GJeJq/Fvw0UAeGAGv+mX+zPBrWxJXKnYb/vDqtzBaRxhRyZ2wbnKBjNyiklU
      Nsk6yiqY2OmJO4Fg1c/Y+n3rQth18ifxi4UEgKUCgYAYCclAKsgGLH3UDgHPXs2D
      gQKh04JJwWZ87bQoOxROOha2Z4uS206gKPsZC84Z5/qRXmI+uhiOmoKQcFgNHS14
      GmKqmBCvR45Ae/n1aPQ3oy0e7GyI/UiIpqftM7NTiAihxLux/xqXQPmffrPJR9Qf
      7nt+/zna8ydCCUOXkK9DnQKBgB6p4HWb4+eW+VZYiTmUtsLXQ60jbNuCf8GMETUK
      WSVh32hqPzfIcLAUxyszr7WUtDd0hI2Jg7xROKWycc2OPDOah2WJSGyBjwIUOgrx
      YJG3zZdUf5UED6ZrnM24qqegmCjGFSTJTV0IUv5SHXQkCCWnV5BHeMW/Ljtkj/cN
      D03BAoGBAIhZU4ZlS4jQ8Gr1dNKXecDiaXlH6I0q9UPrMfY5E8+yyhwboDZuR/st
      v9jy/xqenUSeQ+M2OFIRZvm5kV13xRqSu/PvH55txatf3geBV9rg17J4iugt/ujN
      RVcbBnlRaza526toRaXpPj2qZF8GmDx9eHTWJz196MYxYiwTE2FM
      -----END RSA PRIVATE KEY-----
    termination: edge
  to:
    kind: Service
    name: httpbin
    weight: 100
  wildcardPolicy: None
status:
  ingress:
  - conditions:
    - lastTransitionTime: "2023-05-15T13:41:48Z"
      status: "True"
      type: Admitted
    host: foo.okd.example.com
    routerCanonicalHostname: router-default.okd.example.com
    routerName: default
    wildcardPolicy: None
  - conditions:
    - lastTransitionTime: "2023-05-15T14:03:05Z"
      status: "True"
      type: Admitted
    host: foo.clu-jack-dev-apps-lb-1.example.com
    routerCanonicalHostname: router-apps-lb-1.clu-jack-dev-apps-lb-1.example.com
    routerName: apps-lb-1
    wildcardPolicy: None
  - conditions:
    - lastTransitionTime: "2023-05-15T14:03:05Z"
      status: "True"
      type: Admitted
    host: foo.clu-jack-dev-apps-shard-1.example.com
    routerCanonicalHostname: router-apps-shard-1.clu-jack-dev-apps-shard-1.example.com
    routerName: apps-shard-1
    wildcardPolicy: None

Check route certificate (should be updated)

openssl s_client -connect $IP:443 -servername foo.okd.example.com 2>/dev/null <<<"" \
   | openssl x509 -noout -text \
   |  awk '/Subject: C=/{printf $NF"\n"} /DNS:/{x=gsub(/ *DNS:/, ""); printf "SANS=" $0"\n"}'

SANS=foo.okd.example.com,foo.clu-jack-dev-apps-lb-1.example.com,foo.clu-jack-dev-apps-shard-1.example.com

@maelvls maelvls self-requested a review May 15, 2023 19:24
@maelvls maelvls self-assigned this May 15, 2023
@maelvls
Copy link
Member

maelvls commented May 16, 2023

Hey, thanks for testing the PR, I didn't realize the image was readily available.

I haven't had time to read your comments today, but I will be able to review on Friday. I'll keep you updated!

@maelvls
Copy link
Member

maelvls commented May 23, 2023

I was able to reproduce your tests successfully, well done! I confirm that am comfortable with the direction of the code.

I wish we had some end-to-end tests in place to check this new behaviour but unfortunately we don't have any in place. 😞 I will open an issue to mention the lack of automated tests.

I think this PR is good to go (unless you thought of other unit tests that I may not have thought of?). Let me know if the PR is good on your side and I'll LGTM.

Thanks!

/approve

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2023
@jacksgt
Copy link
Contributor Author

jacksgt commented May 25, 2023

I was able to reproduce your tests successfully, well done! I confirm that am comfortable with the direction of the code.

I wish we had some end-to-end tests in place to check this new behaviour but unfortunately we don't have any in place. disappointed I will open an issue to mention the lack of automated tests.

I think this PR is good to go (unless you thought of other unit tests that I may not have thought of?). Let me know if the PR is good on your side and I'll LGTM.

Thanks for taking another look. Yes, I will add some unit tests for this new feature, then the PR should be good to go.

@jacksgt
Copy link
Contributor Author

jacksgt commented May 30, 2023

Hi @maelvls ,

I added some unit tests, for which I also needed to perform some refactoring on the creatNextCR and sync function. Please take a look. I hope the PR is not getting too complicated, otherwise I can also submit the tests in separate PR.

Copy link
Member

@maelvls maelvls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for unit testing buildNextCR, that's great work! The refactoring is welcome too. I had not paid attention to the fact that there were some missing unit tests in my previous review. "1337" as the revision number is a nice touch 😅

@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jacksgt, 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

@jacksgt jacksgt force-pushed the support-subdomains branch from 025c302 to 10d5cc7 Compare June 14, 2023 11:01
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jun 14, 2023
@jacksgt jacksgt force-pushed the support-subdomains branch from 10d5cc7 to e7e27c3 Compare June 14, 2023 11:02
@jacksgt
Copy link
Contributor Author

jacksgt commented Jun 14, 2023

Hi,
I squashed the commits and added the DCO / Signed-off-by.
I also performed a final validation according to the tests you outlined above.
From my side this is ready.

@jacksgt jacksgt changed the title [wip] Support Route.spec.subdomain Support Route.spec.subdomain Jun 14, 2023
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2023
@jacksgt
Copy link
Contributor Author

jacksgt commented Jul 24, 2023

ping @maelvls

@maelvls
Copy link
Member

maelvls commented Aug 1, 2023

Hey, sorry for the delay! I came back from vacation last week but had not got around to review your PR. 😅 Thank you so much for adding the missing unit tests on buildNextCR (which was called createNextCR previously and wasn't unit tested).

I think it is good to go!

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2023
@jetstack-bot jetstack-bot merged commit 22dbff3 into cert-manager:main Aug 1, 2023
@maelvls
Copy link
Member

maelvls commented Aug 1, 2023

Let's wait for #28, #29, and #32 to get merged and let's cut a new version of openshift-routes!

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for the route subdomain enhancement
3 participants