Traefik v2 file provider #135
Conversation
|
Hi @GeorgianaElena, hope this works as a starting point. I'm sure the commit history is not as tidy as it could have been, maybe the rebasing didn't go exactly as planned(!), as I think it includes the tidying commits you already made to the Cheers, |
- Renamed frontends and backedns to routers and services, respectively.
- Traefik API paths at /api/providers/{provider_name} no longer work, so
search /api/http/{services|routers} instead
- Traefik file provider doesn't like arbitrary data (any more?), so have put
JupyterHub's 'data' object into the dynamic configuration file in separate
root keys.
To Do:
- Haven't touched the consul or etcd providers, other than to rename
'frontends' and 'backends', as above.
- Test, test, test.
Additionally, have renamed TraefikTomlProxy to TraefikFileProviderProxy, and
edited relevant traefik_utils functions so the proxy provider should now work
with either yaml or toml files. (need to test).
- jupyterhub_config.py will now need to reference the proxy class
'traefik_file', e.g.:-
c.JupyterHub.proxy_class = 'traefik_file'
c.TraefikFileProviderProxy = '/path/to/rules.toml'
documentation, examples and performance test suite. These are replaced by `traefik_file` and `TraefikFileProviderProxy`, respectively.
This is a re-work of original commit faa2832 to try and make it a bit more manageable. In this commit, work specifically on the file provider proxy. - Relevant documentation has been updated in README.md, docs/source/file.md (renamed from toml.md) - requirements.txt removes toml a forced dependency as ruamel.yaml could be used instead. This latter module is required by jupyterhub anyway, so should already be present on a system running JupyterHub. - The external file provider now watches a directory instead of a file, so have added a pre-baked dynamic_config directory (and file), where the rules.toml file will be saved and managed by the TraefikFileProviderProxy. - Have duplicated the toml_proxy and external_toml_proxy pytest fixtures to test both toml and yaml files. (Would have preferred to parametrize the existing fixtures to avoid duplicating them, but couldn't figure out how). - `tests/test_traefik_api_auth.py` - Have had to make the test give traefik more of a chance to read its dynamic config before running the test. Previously, in traefik v1, the api authentication would be configured in the static config. Now the api 'middleware' is configured in the dynamic config, the previous wait condition of just waiting for the port to come up didn't give traefik enough time to set up the API authentication on that entrypoint / router.
|
Hey @alexleach! This a great starting point, indeed 🎉
Rebasing is difficult, especially for a PR this size, so don't worry! I gave it a shot too and rebased it against the I could push those changes on your branch and update this PR, but I would really appreciate it if before, you could take a look at the changes to make sure I didn't remove any useful info while solving merge conflicts. (The tests pass locally btw, so this is a good sign) |
|
Hi @GeorgianaElena, thanks for the kind words and for tidying it up to how it should be! I think this looks much better, but I would like to do a git / vim diff between your repository and mine to check how the final working copies compare. Pushing to my branch sounds like a plan, not sure how else to go about it, but maybe now is a good point to raise a couple of points / considerations:-
I'll have a more thorough comparison when I can and will get back to you on that anyway, but would be glad to hear your thoughts on the above in the meantime 🙂 KR, Alex |
|
@alexleach, let me know when you feel comfortable with me pushing the rebased version of these changes on your branch. Some ideas below about some of the points in the previous comment:
I think the shorter version makes sense, as you said they are all
It should be fairly easy to drop some of the commits, once we've rebased the PR against the latest version of
I believe it would make our life much easier if both of them are listed as explicit requirements. At least until we find a reason against it, as it happened for the |
|
Hi @GeorgianaElena , Thanks for the responses to my comments. I've had a chance to have a look this morning and to compare the HEAD's of our branches both locally and then on github. The default compare view (Ref: 1) shows all the commit history, which actually makes it really difficult to compare! On github, just comparing the HEAD's by commit ID (Ref: 2), we can see that there are just some minor differences to various version numbers in I think that only one difference in I haven't narrowed down which commit(s) actually introduced these differences, but I suppose that isn't relevant. Now we have found these differences, we can quite easily patch them in a new commit. The patch needs to be made locally it seems, showing the raw patch on github (Ref: 4) shows the full commit history again, which is overkill... Okay, so a to-do list (please let me know your comments):-
Please let me know your thoughts and how I can next help! KR, Alex 🙂 |
- fileprovider.py - Reintroduce _start_traefik, removed in commit
05e7c07
- install.py - Revert some changes made in commit
05e7c07.
- proxy.py - Revert some API changes on the method signatures, which
broke compatibility with the TKvProxy subclasses.
- test_install.py - Make sure download traefik v2, as the download URLs have
changed since v1 (and is reflected in install.py).
- test_traefik_api_auth.py - No longer test TKvProxy subclasses, they don't
work
- test_traefik_utils.py - Remove reference to traefik_utils.persist_routes,
which no longer exists.
Traefik v2 is now distributed as a compressed archive, not the raw binary, so needs to be uncompressed before it can be used.
Correct traefik-v2 documentation URLs in install.md. Reduce line-lengths of f-strings in proxy.py and traefik_utils.py. Co-authored-by: GeorgianaElena <[email protected]>
|
Hey @alexleach! Sorry for the long silence, but I had some days off and tried to stay away from the keyboard. I followed your to-do list above and updated my copy of your branch (great catch of the default Do you think you can add me as a contributor to your fork? The option should be under https://github.com/alexleach/traefik-proxy, |
|
Hi @GeorgianaElena , Thanks for getting back to me, I was wondering about the long silence, but no worries! Hope you had an enjoyable and well-deserved break from the keyboard! 😄 🌞 🏖️ You've probably got the invitation to be a collaborator by now. I imagine that this is a better idea, as I think it will allow you to tidy up the commit history (i.e. to delete / edit past commits), whereas opening a PR I think would probably allow you to add commits. Not sure tbh, but either way, you should be able to edit my fork as you like now! Thanks for going through the check-list. Let me know how I can help next! KR, Alex |
GeorgianaElena
force-pushed
the
traefik-v2-file
branch
from
23cc536 to
8d5e290
Compare
There was a problem hiding this comment.
Thanks for your patience @alexleach and sorry again for taking so long for a follow up.
I left a few comments and would love for your feedback on some of the questions there.
In the meanwhile, I'll do some more cleanup and address some of suggestions there.
(Also, I renamed the file proxy file back to toml.py for a better diff, so the tests are failing.)
| "rules.toml", config=True, help="""traefik's dynamic configuration file""" | ||
| ) | ||
|
|
||
| dynamic_config_handler = Any() |
There was a problem hiding this comment.
We should provide a short description about the dynamic_config_handler usage.
There was a problem hiding this comment.
Suggesting this edit in my working tree:-
dynamic_config_handler = Any(help="""An instance of traefik_utils.TraefikConfigFileHandler""")
| self.dynamic_config["jupyter"]["routers"][router_alias] = { | ||
| "data": data | ||
| } |
There was a problem hiding this comment.
Was the jupyter entrypoint created just to have a place to store data?
Add the data node to a separate top-level node, so traefik doesn't complain
@alexleach, do you remember what kind of error did traefik threw when trying to store data under http.routers.router_alias?
There was a problem hiding this comment.
Yes, I've just tested this, by adding the data node instead to http/routers/${router_alias}/data, and this error is produced:-
ERRO[2021-10-26T10:54:15Z] Error occurred during watcher callback: field not found, node: data providerName=file
I don't remember testing out different options thoroughly, but have just spent loads of time trying to add the data node to http/jupyter/routers/${router_alias}/data, and this also fails, with:-
ERRO[2021-10-26T13:17:10Z] Cannot start the provider *file.Provider: field not found, node: jupyter
So yes, the data node needs a separate top-level place to go, at which point traefik does not raise an error.
| # If empty, delete the keys | ||
| if not self.dynamic_config["http"]["routers"]: | ||
| self.dynamic_config["http"].pop("routers") | ||
| if not self.dynamic_config["http"]["services"]: | ||
| self.dynamic_config["http"].pop("services") | ||
| if not self.dynamic_config["http"]: | ||
| self.dynamic_config.pop("http") | ||
| if not self.dynamic_config["jupyter"]["routers"]: | ||
| self.dynamic_config["jupyter"].pop("routers") |
There was a problem hiding this comment.
Is traefik complaining if the keys don't have a value?
|
|
||
| return backend_entry | ||
| def generate_service_weight_entry( proxy, service_alias, separator="/"): |
There was a problem hiding this comment.
| def generate_service_weight_entry( proxy, service_alias, separator="/"): | |
| def generate_service_weight_entry(proxy, service_alias, separator="/"): |
| class TraefikConfigFileHandler(object): | ||
| """Handles reading and writing Traefik config files. Can operate | ||
| on both toml and yaml files""" |
There was a problem hiding this comment.
Thanks for your work on offering support for both file formats @alexleach!
I'm wondering if this is something that's sustainable in the long run (from the support perspective) 👀
| def dump(self, data): | ||
| with open(self.file_path, "w") as f: | ||
| self._dump(data, f) |
There was a problem hiding this comment.
Just for consistency:
| def dump(self, data): | |
| with open(self.file_path, "w") as f: | |
| self._dump(data, f) | |
| def dump(self, data): | |
| """Depending on self.file_path, call either yaml.dump or toml.dump""" | |
| with open(self.file_path, "w") as fd: | |
| self._dump(data, fd) |
| # toml is now optional, as can use yaml configuration files instead now... | ||
| #toml | ||
| #ruamel.yaml |
There was a problem hiding this comment.
Maybe choose to go with a default configuration file type and install that? Not installing either of them gives me the impression that we're providing an incomplete package... Not sure what's the standard practice for this though.
| @@ -36,14 +36,40 @@ def pytest_configure(config): | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
| # There must be a way to parameterise this to run on both yaml and toml files? | |||
There was a problem hiding this comment.
Hmm, I think so, yes! Maybe this helps https://docs.pytest.org/en/stable/fixture.html#parametrizing-fixtures?
|
|
||
| rc = await api_login() |
There was a problem hiding this comment.
I believe this and the assert below are not needed anymore since we're already checking the return code in cmp_api_login.
|
Hi @GeorgianaElena , I'm having a look at this now. I had to make sure of course that I'm looking at the same commit, and will start running some tests to give accurate answers to some of these questions. Of course the tests don't work as is, as Cheers, |
|
closed by #145 |
Hello,
As discussed in #133 and #97, I have done some work getting jupyterhub-traefik-proxy working with traefik v2. This PR should allow jupyterhub to work with the Traefik v2 file provider, but comes with some caveats (as mentioned in #133 ):-
Fixes are available for these (in https://github.com/alexleach/traefik-proxy/tree/Traefik_v2), but this PR aims to minimise the size of the patches. Features can then be introduced incrementally to ease reviewing.
Hope this helps as a starting point for getting traefik v2 support functional anyway! 🙂
fixes 2i2c-org/upstream#29