-
Notifications
You must be signed in to change notification settings - Fork 797
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
Deprecate kube-lego and document cert-manager usage #755
Conversation
Woooooo!! I'm enthusiastic about this :D |
doc/source/advanced.md
Outdated
2. Install & configure cert-manager using the | ||
[cert-manager helm-chart](https://github.com/kubernetes/charts/tree/master/stable/cert-manager) with ingressShim enabled: `--set ingressShim.defaultIssuerName=letsencrypt-prod --set ingressShim.defaultIssuerKind=ClusterIssuer`. | ||
|
||
3. Create the default ClusterIssuer: |
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.
Can you add the command that people should run with this yaml? Can't we integrate this into the z2jh chart?
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.
We can I think - we can declare it as a dependency.
I've done that for the z2jh-chart itself and built on top of it.
For reference: https://github.com/consideRatio/z2jh-extended/blob/master/z2jh-extended/requirements.yaml
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.
It is very likely the users already run or will run cert-manager cluster-wide, if using it. having it as a hard dependency and the issuer managed by us seems to complicate things.
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.
@betatim just added the full command
|
||
2. Apply the config changes by running `helm upgrade ...` | ||
3. Wait for about a minute, now your hub should be HTTPS enabled! | ||
Refer to the Advanced Topics section for Automatic HTTPS with cert-manager and [Let's Encrypt](https://letsencrypt.org/). |
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.
I want to make sure that enabling letsencrypt remains the simplest and first recommended way to enable HTTPS. It shouldn't be moved to advanced topics. This https-enabling config that faces the users should ideally stay the same and the kube-lego->certmanager transition should be hidden from the user as much as possible.
What's the impediment to doing this 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.
Addressing this is mandatory and in my eyes what needs to be done before we can merge this.
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.
Happy to add the simple command back to the security section.
Is there a specific blocker to moving this PR forward? We're running into some related challenges that we'd like to use cert-manager for. |
kubucations's videos about cert-manager + lets encrypt was great for me to watch, that made me grasp a lot of things about cert-manager. I have now tried cert-manager, and will summarize what I've picked up:
Conclusion:
Suggestions:
@clkao @betatim @minrk @yuvipanda, what do you think? Especially curious regarding the optional chart dependency solution. |
An optional dependency makes it easier for people to get started, by just editing a single values file, but that does mean we have to support it and follow upstream. IMO this is more worthwhile when the dependencies are very coupled, such as a database that you want to share secrets with. For cert manager i think asking users to run an additional Another scenario: if we make this an optional dependency and do not support all options, and the users wish to migrate to a standalone cert-manager (for example if users decide to switch from http01 to dns01 domain validation), we'll need to figure out how to disconnect the helm dependency and preventing the old tls secret being deleted in the process. |
@clkao it will be fine to loose the tls secret though right? It will just force a renewal ahead of time i think. Adding cert-manager as an optional dependency, makes the users able to control it directly through provided Example of proxy:
secretToken: asdfasdfasdf
cert-manager:
someCertManagerKey: andValue |
I think an additional
I'd really like to avoid any What about:
Alternately, and I don't know what the up/downsides are, ship a cert-manager in our chart. What are the downsides, for instance, of having multiple cert-manager instances in a cluster? Is it a problem? If not, having the simples autohttps config deploy cert-manager, but make sure the chart works with a central cert-manager might be ideal. |
@minrk to make cert-manager get a secret with TLS certificates etc in it for a certain domain, you need both an Issuer (or ClusterIssuer) and a Certificate resource You can automate the creation of a Certificate resource if you have an Ingress resource with a tls field in its spec though, because cert-manager will find it and use it to create the Certificate. To conclude so far, we need an Issuer of assume the usage of a predefined ClusterIssuer, and we need a Certificate or a Ingress resource with tls fields in its spec. Then we get ourselves a TLS-secret automatically created for us to consume. If we declare an ingress object though, that specific cloud provider needs to create an associated LoadBalancer and be able to terminate TLS etc. These things are currently in flux, and many different cloud providers have implemented this to varying degree. The GitLab helm chart deploys their own ingress controller (pods are spawned and running) in the namespace along with their deployment, that makes them work better on all kinds of cloud provider. My ideas right now.
Hmmm... Something like this. |
My general hope is to keep stringing kube-lego around until we get the traefik based proxy going - although I'll defer to others wether this is tenable. That should simplify everything here. In the meantime, additional docs on using cert-manager instead of our automatic issuer seems like a good thing to do? |
https://github.com/jupyterhub/outreachy/blob/master/ideas/traefik-jupyterhub-proxy.rst has info on the traefik proxy. |
I'm currently having issues with |
The blocker is mentioned in this comment #755 (comment). The user facing config should remain as simple as it is today and be unchanged for the "just use letsencrypt" use case. |
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/server-becomes-stale-in-a-matter-of-days-even-hours/1920/7 |
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/how-to-get-csr-for-ssl-cert/2505/2 |
1 similar comment
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/how-to-get-csr-for-ssl-cert/2505/2 |
I just wanted to point out that running
deploys but also warns,
|
I tried the instructions in As I read the instructions, all that is needed in config.yaml is
I tried that. I also tried adding
And I tried adding
in the ingress section. |
Add |
It should be noted that the |
We no longer use kube-lego, with #1539. I've personally never been able to get cert-manager to work despite trying several times - it feels very heavy for our common use case. How do people think we should proceed with this PR now? |
I use cert-manager in production across a number of sites and it's been great, especially in the last few releases. |
While I think cert-manager is a great utility, I'm less happy about embedding a dependency on it in this helm chart, this is an evolved opinion. I have learned from experience that it is hard to install/upgrade CRDs and we would run into that using cert-manager as a dependency. @yuvipanda have implemented a great replacent of kube-lego that is lightweight and suitable for anyone wanting to enable HTTPs quickly, that also doesn't exclude use of cert-manager if one have that utility in the cluster. @clkao do you agree to close this PR? I'm very thankful for your work on it, and it made us have relevant discussion and helped me learn a lot more about this topic. |
Closing as 0.9.0-beta.3 has the new autohttps with a traefik pod that acquires a cert and syncs it to a k8s secret. |
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/jupyterhub-upgrade-steps-for-https-acmev2/4029/1 |
Note that gce ingress is currently having a regression preventing first-time cert-manager request fail (when the tls secret is unset): kubernetes/ingress-gce#388, we might want to hold off until gce is fixed to avoid confusion.
Closes #475