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

Add support for Path/PathPrefix rules #26

Merged
merged 3 commits into from
Feb 11, 2023
Merged

Conversation

Sammy1Am
Copy link
Contributor

@Sammy1Am Sammy1Am commented Feb 9, 2023

Rather than a separate sub-domain for each service, I'm using paths for each one (e.g. my.domain.com/grafana), so I added support for rules that include paths. As written it requires hosts and paths to use backticks (`) instead of escaped quotes, but this seems to be the recommended practice anyway. It also will only select the first host or path defined, but that was already sort of the case. While I was in there I replaced the two regexReplaceAll lines with a single line that uses groups (go templates do it sort of weird, and it took me a while to figure it out).

These changes are working fine for me on my setup, but I'm not doing anything crazy beyond Host and PathPrefix, so additional testing might be warranted if there are any weird edge cases.

Changes:

  • Adding support for rules that contain Path or PathPrefix by appending the path after the hostname if present.
  • Resolved regexReplaceAll group matching when finding service name.
  • Append trailing slash to the href (most services are fine, but Traefik itself wants a trailing slash for the dashboard so it seemed worth including).

- Adding support for rules that contain Path or PathPrefix by appending the path after the hostname if present.
- Resolved regexReplaceAll group matching when finding service name.
@santimar
Copy link
Owner

santimar commented Feb 9, 2023

Hi @Sammy1Am and thanks for the PR.

Regarding the changes:

Adding support for rules that contain Path or PathPrefix by appending the path after the hostname if present.
Implementation looks good.
Also, reading your message I realized I didn't specify the fact that only the first rule is used if multiple are declared or that use of && and || may not work
I'll update the readme.

Resolved regexReplaceAll group matching when finding service name
Much cleaner this way.
I think that the majority of the users will use the backtick instead of the escaped double quote since it's more clean, easy to write and requires has only one character instead of two, so i would leave the code as is

Append trailing slash
I think this one is wrong.
This way if I set a rule like Host(`example.org`) && Path(`/path`) I would get a link to https://example.org/path/ which is wrong, isn't it?
I use traefik-home to serve the traefik dashboard without problems using the following configuration

labels:
      traefik.enable: "true"
      traefik.http.routers.traefik.rule: "Host(`dashboard.my.domain`)"
      traefik.http.routers.traefik.entrypoints: "web"
      traefik.http.routers.traefik.service: "api@internal"

@santimar santimar requested review from santimar and removed request for santimar February 9, 2023 19:21
@Sammy1Am
Copy link
Contributor Author

Sammy1Am commented Feb 9, 2023

Append trailing slash
I think this one is wrong.
This way if I set a rule like Host(example.org) && Path(/path) I would get a link to https://example.org/path/ which is wrong, isn't it?

For what it's worth, I agree. 😉 However Traefik does (for some reason) require a trailing slash at the end of https://my.domain.tld/dashboard/. There was an attempt to "fix" this, but it's apparently a "technical requirement" documented here.

Because I'm trying to keep my services under a single domain/cert, I'm using a path for the dashboard like this:

labels:
      - "traefik.enable=true"

      - "traefik.http.routers.dashboard.tls=true"
      - "traefik.http.routers.dashboard.rule=Host(`my.domain.tld`) && (PathPrefix(`/dashboard`) || PathPrefix(`/api`))"
      - "traefik.http.routers.dashboard.entrypoints=websecure"
      - "traefik.http.routers.dashboard.service=api@internal"

This works, but only if I put the trailing slash in the URL.

Now, all that being said, Traefik seems to be the exception here. I'm not really aware of anything else that requires the slash, though so far none of my services complain about it either. 🤷‍♂️ I figured since 100% of people installing Traefik Home will also have Traefik installed that it might make sense to cater to its weirdness, but I'd be (almost) equally happy to stick with the more-expected no-slash links and have Traefik's dashboard just broken for me.

I'd also maybe be in favor of some sort of flag (or if we can reliably detect when the service is Traefik, even a hard-coded Traefik exception to add a slash), since that would allow the Traefik dashboard to work with a Path rule but keep other services slash-free. This does add additional code/logic though, so it's a tradeoff.

Happy to do the legwork and make whatever changes you think make sense though. Also, thanks for this project; as soon as I got Traefik setup I wanted some kind of home page, hoped one existed, and here it was!

@Sammy1Am
Copy link
Contributor Author

Sammy1Am commented Feb 9, 2023

Actually, there might be a way to redirect/add the slash using middleware; maybe I'll look into that later today. That way Traefik Home could just do regular paths for all the normal services and Traefik could get its own extra config.

@santimar
Copy link
Owner

santimar commented Feb 9, 2023

Does it works if you change your rule to Host(`my.domain.tld`) && (PathPrefix(`/dashboard/`) || PathPrefix(`/api`)) ?
this way the PR code should produce https://my.domain.tld/dashboard/ (with a trailing slash just for the traefik container, since it's on the PathPrefix) and traefik should properly serve the dashboard.
can you give a try?

@Sammy1Am
Copy link
Contributor Author

Sammy1Am commented Feb 9, 2023

Well using PathPrefix(`/dashboard/`) works perfectly, great call. I think I avoided that because I was worried about when it removed the /dashboard/ from a path like my.domain.tld/dashboard/http/routers before sending it to the service that I would end up with something like my.domain.tldhttp/routers. But I'm not actually removing the prefix for Traefik 😅 so that's a non-issue.

Additional commit pushed to remove trailing slash.

@santimar
Copy link
Owner

Perfect.
Let me do some local test and then I'll merge

@santimar
Copy link
Owner

Everything looks good on my side
I just spotter a typo on comment at line 123, there's a missing space in word incase
Fix it and I'll make the release v1.3.0
Thank you! @Sammy1Am

@Sammy1Am
Copy link
Contributor Author

Done 😄 . Thanks for the quick response, it'll be nice to point my compose file back to the official package instead of my own.

@santimar santimar merged commit 8665213 into santimar:master Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants