https: Only expose port 443 if we really have HTTPS on #1758
Conversation
Currently, port 443 is exposed by default. This causes issues when cloud providers (like AWS) health check all exposed ports. Since nothing is listening to port 443, it fails & causes general madness. This requires users explicitly set https to enabled, but explicit is better than implicit in this case I think. We have too many HTTPS options to try and auto-detect that, and currently the option we have basically fails HTTPS on AWS Fixes jupyterhub#1637
For HTTPS, we want to expose port 443 on our service, but only if it is 'turned on'. Unfortunately, in v 0.9, the default was set to 'enabled' but it didn't act as enabled unless hostnames were actually set. This exposed port 443 in proxy-public regardless of wether we were actually set up for HTTPS or not. This caused weird issues with AWS, since it was health checking all exposed ports. Since we didn't have automatic HTTPS setup (since that needs hostnames), this broke all installs on AWS! This tries to be backwards compatible change, exposing port 443 only if we *really* have https running. This should fix issues with AWS, and be more 'correct'. Manual HTTPS doesn't actually need hosts lists to be set, so we remove that from our documentation. Fixes jupyterhub#1637
yuvipanda
changed the title
| @@ -182,7 +182,7 @@ proxy: | |||
| enabled: true | |||
| minAvailable: 1 | |||
| https: | |||
| enabled: true | |||
| enabled: false | |||
There was a problem hiding this comment.
I think this makes sense as a default, but is something that probably breaks peoples configurations which left it out given our past documentation did so.
Hmmm, should we perhaps mitigate this? We could for example make template rendering fail using Helm's fail function given a certain condition, or we could provide a warning in NOTES.txt. For example, that hosts is specified and https.enabled is false, that would be a typical indication of an issue. Another one would be to not set a default value for this at all in values.yaml, and if it isn't rendering to be true or false but instead to be a blank string we assume it to be true...
Hmm... I'm not sure. What do you think about this potential need to help users transition?
There was a problem hiding this comment.
I think it would be fine to make it a breaking change, where we notify users of in the release notes, saying they need to avtively set it to enabled if they had it implicitly set enabled before.
I think it would also be fine to let one z2jh release look for a dependency on the default truthiness and actively fail template rendering if we think it would crash some configurations.
- It wasn't allowed to have a YAML comment before apiVersion, so I removed them. An alternative would have been to make the comments Helm template comments. - I updated a comment to be more explicit about port references etc. - I ensured that we provide the 443 port if we use a k8s secret provided to the configurable-http-proxy (proxy pod), and not only if we provide certificates. So "type: manual" and "type: secret" will both ensure we expose port 443 on the proxy-public service in other words.
|
I created a PR to merge into this PR, see yuvipanda#1. |
Fix syntax errors and avoided a potential bug.
|
Thanks folks! |
jupyterhub/zero-to-jupyterhub-k8s#1758 wasn't bug free after all
jupyterhub/zero-to-jupyterhub-k8s#1758 wasn't actually backwards compatible - we need to explicitly set this now.
|
Turns out this wasn't actually backwards compatible somehow. If I don't set |
|
@yuvipanda yes =/ I considered that in #1758 (comment) |
For HTTPS, we want to expose port 443 on our service, but
only if it is 'turned on'. Unfortunately, in v 0.9, the
default was set to 'enabled' but it didn't act as enabled
unless hostnames were actually set. This exposed port 443
in proxy-public regardless of wether we were actually
set up for HTTPS or not. This caused weird issues with
AWS, since it was health checking all exposed ports. Since
we didn't have automatic HTTPS setup (since that needs
hostnames), this broke all installs on AWS!
This tries to be backwards compatible change, exposing
port 443 only if we really have https running. This
should fix issues with AWS, and be more 'correct'.
Manual HTTPS doesn't actually need hosts lists to be set,
so we remove that from our documentation.
We also re-introduce 'https.enabled' in our documentation -
manual action (DNS) is needed to enable HTTPS, so this
being explicit is IMO a good thing.
Fixes #1637