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

Traefik v2, TraefikProxy v1 #861

Merged
merged 12 commits into from May 18, 2023
Merged

Traefik v2, TraefikProxy v1 #861

merged 12 commits into from May 18, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Mar 23, 2023

Updates:

  • jupyterhub-traefik-proxy to 1.0
  • traefik to 2.10

traefik v2 totally changed their config file format, so several config options needed to move around, but ultimately the behavior is unchanged. jupyterhub-traefik-proxy 1.0 is required to use traefik v2 due to the config and API changes. Some configuration is no longer required as jupyterhub-traefik-proxy writes initial dynamic config (api route, auth) during its startup, instead of requiring to duplicate that info to the traefik config file.

This is a breaking change for the rare cases where users configured traefik or TraefikProxy directly, since both are major changes, but for most users the change should not be noticed (🤞) except as a performance boost when there are many users.

closes #513

- traefik releases are tarballs now
- move traefik platform check to download function
  for easier unit testing on unsupported platforms
@minrk minrk force-pushed the traefik-v2 branch 4 times, most recently from ca9944d to 5015f4f Compare May 16, 2023 09:37
- tls config is no longer allowed in static config file, add separate dynamic config
- no longer need to persist auth config ourselves (TraefikProxy handles this)
- make sure to reload proxy before reloading hub in tests
@minrk minrk force-pushed the traefik-v2 branch 2 times, most recently from 0ded20b to d3aea5b Compare May 16, 2023 11:14
@consideRatio
Copy link
Member

Wieee tests are passing, ping me for review when its ready for review @minrk!

@minrk minrk marked this pull request as ready for review May 16, 2023 12:34
@minrk minrk changed the title [WIP] Traefik v2 Traefik v2 May 16, 2023
@minrk
Copy link
Member Author

minrk commented May 16, 2023

Tests are passing and I did a test deployment to verify that the letsencrypt config works. Good to go from me!

@consideRatio
Copy link
Member

@minrk can you update title, PR description, and labels?

If there is breaking changes, can you clarify them in the PR description in a dedicated section as well?

My rough understanding is that we are updating the traefik-proxy, and setting it up to work against traefik v2 instead of v1.7.33. As part of this, we update misc config of traefik-proxy as well that ships with the TLJH distribution.

tljh/traefik.py Outdated Show resolved Hide resolved
@minrk minrk added the breaking label May 16, 2023
@minrk minrk changed the title Traefik v2 Traefik v2, TraefikProxy v1 May 16, 2023
minrk and others added 2 commits May 16, 2023 14:53
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
@minrk
Copy link
Member Author

minrk commented May 16, 2023

It's possible the 0.2.0 upgrade test failed because of #895. I don't understand the error message well enough to be sure, but I ran a test deployment and can confirm that without the PATH change upgrading from 0.2.0 to jupyterhub 4 does not work, so I don't really understand why the test are passing most of the time. It could be that the initial 0.2.0 setup doesn't let old jupyterhub get fully set up enough.

@minrk minrk reopened this May 16, 2023
@minrk
Copy link
Member Author

minrk commented May 18, 2023

All green!

The intermittent failure in the integration test seems to be increasing in frequency. I still don't really understand the cause.

tljh/traefik-dynamic.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik-dynamic.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik-dynamic.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik-dynamic.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
Always chomp left, never right. This is what we do in the helm chart templates and has been very easy to be consistent with for a good result with little to no issues.
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Wieee! This LGTM @minrk - if you agree on 324ded0 that I committed, then let's go for a merge!

@minrk
Copy link
Member Author

minrk commented May 18, 2023

Yes, thanks! I still don't have good intuition for chomping.

@consideRatio consideRatio merged commit 814c6da into jupyterhub:main May 18, 2023
12 checks passed
@consideRatio
Copy link
Member

❤️ 🎉 wieeeeeeeeeeeeeeeee!!!!

Thank you @minrk for your efforts into this!!!!

@minrk minrk deleted the traefik-v2 branch May 18, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify ciphersuites in TLS config
3 participants