-
Notifications
You must be signed in to change notification settings - Fork 390
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
Remove ingress.https with unused kube-lego references, rely on ingress.tls and ingress.annotations #1813
Conversation
[kube-lego](https://github.com/jetstack/kube-lego) was the precursor to cert-manager, and has been dead for years and years now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup of ingress.https
ingress.https
as a whole is soleley used for ingress.https.type=kube-lego
. If we remove type
from the schema, it will error as much as if we remove all of ingress.https
. Due to this, I've added a commit cleaning it up fully.
Note that mybinder.org-deploy makes use of this configuration. In practice the annotation kubernetes.io/tls-acme
is also respected by cert-manager as a signal that it should be active, but it requires some configuration to specify default issuer etc alongside it - see https://cert-manager.io/docs/usage/ingress/.
I think removing this is a fine call, but it requires adjustments to deployments having it explicitly enabled (the default is not).
Config migration
Deployments with ingress.https.enabled=true
need to set ingress.annotations
and ingress.tls -> .hosts / .secretName
going onwards instead I think.
In practice for mybinder.org-deploy, examplifying with the prod config, I think the following changes would need to be made.
- Let the mybinder chart remove the
binderhub.ingress.https
section - Let the mybinder chart have
binderhub.ingress.annotations
includingkubernetes.io/tls-acme=true
- Let
binderhub.ingress.tls
look likebinderhub.jupyterhub.ingress.tls
configured here, declaring a secret namekubelego-tls-binder-prod
to avoid making a change of name.
Like that, I think mybinder.org will render like it did before this change as well.
Config migration example that verified PR
I did some chart rendering and concluded that the before / after config below led to the same content (besides key ordering and indentation).
# possible use before this config
# helm template . --values before-config.yaml --show-only templates/ingress.yaml > before-ingress.yaml
ingress:
enabled: true
hosts:
- example.com
https:
enabled: true
type: kube-lego
# equivalent config after this PR
# helm template . --values after-config.yaml --show-only templates/ingress.yaml > after-ingress.yaml
ingress:
enabled: true
hosts:
- example.com
annotations:
kubernetes.io/tls-acme: "true"
tls:
- secretName: binderhub-tls-binder-release-name
hosts:
- example.com
git diff --no-index -- before-ingress.yaml after-ingress.yaml
Thanks @consideRatio. I am going to wait to make sure I have access to the curvenote member as well (jupyterhub/mybinder.org-deploy#2920) before merging this and deploying it on mybinder.org |
Closing in favor of plan in #1862 (comment) |
kube-lego was the precursor to cert-manager, and has been dead for years and years now