Skip to content

caddyconfig: Add new handle_path directive for easier subpath grouping #3281

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

Merged
merged 4 commits into from
May 26, 2020

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Apr 19, 2020

Implementation of #3266

This is incomplete/non-functional, still has debug prints etc. The general idea is there but it's not working as expected.

Edit: Figured it out! Didn't realize I should be using h.NewRoute() which does the trick.

Caddyfile:

:80

handle_path /api/v1 {
    respond "API v1"
}

file_server

JSON:

{
  "apps": {
    "http": {
      "servers": {
        "srv0": {
          "listen": [
            ":80"
          ],
          "routes": [
            {
              "match": [
                {
                  "path": [
                    "/api/v1*"
                  ]
                }
              ],
              "handle": [
                {
                  "handler": "subroute",
                  "routes": [
                    {
                      "handle": [
                        {
                          "handler": "rewrite",
                          "strip_path_prefix": "/api/v1"
                        }
                      ]
                    },
                    {
                      "handle": [
                        {
                          "body": "API v1",
                          "handler": "static_response"
                        }
                      ]
                    }
                  ]
                }
              ]
            },
            {
              "handle": [
                {
                  "handler": "file_server",
                  "hide": [
                    "../../../../caddy/Caddyfile-handle-path"
                  ]
                }
              ]
            }
          ]
        }
      }
    }
  }
}

@francislavoie francislavoie marked this pull request as ready for review April 19, 2020 07:24
@francislavoie francislavoie changed the title caddyconfig: WIP implementation of handle_path caddyconfig: Implement handle_path directive for easier subpath grouping Apr 19, 2020
@francislavoie francislavoie requested a review from mholt April 19, 2020 07:27
@francislavoie francislavoie force-pushed the handle-path-directive branch from 58d6130 to a731cf3 Compare April 19, 2020 07:33
Comment on lines +132 to +155
// the ParseSegmentAsSubroute function expects the cursor
// to be at the token just before the block opening,
// so we need to rewind because we already read past it
h.Reset()
h.Next()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit funky, but aside from changing how ParseSegmentAsSubroute works to avoid the extra h.Next(), I'm not sure what to do.

@francislavoie francislavoie added this to the 2.1 milestone Apr 19, 2020
@francislavoie francislavoie linked an issue Apr 19, 2020 that may be closed by this pull request
@francislavoie francislavoie changed the title caddyconfig: Implement handle_path directive for easier subpath grouping caddyconfig: Add new handle_path directive for easier subpath grouping Apr 19, 2020
@zakcutner
Copy link

zakcutner commented Apr 19, 2020

This is super cool, thanks! My only concern is that I actually find the name handle_path slightly confusing as it doesn't express that it matches a prefix (rather than an exact match as usual) and that it strips the prefix rather than simply handling it — which I think is the killer behaviour here 😎

I don't have an amazing understanding of all the directives so perhaps this wouldn't make sense, but how about splitting up this functionality into two options which could be added to the existing handle directive?

  1. handle /foo prefix { ... } would cause /foo* to be matched rather than /foo.
  2. handle /foo strip { ... } would cause the first argument, /foo, to be stripped after matching.

Then, the functionality of handle_path /foo { ... } that you have implemented could be expressed using these options as handle /foo prefix strip { ... }.

Alternatively, maybe it would be cleaner to add the prefix option to the path matcher in a similar way to how NGINX does in location blocks (the syntax location /foo { ... } vs location = /foo { ... }). I think it also makes more sense for 'prefix mode' to actually be enabled by default as (just from my intuition 😬) I imagine this is the more common use case.

@mholt mholt added the under review 🧐 Review is pending before merging label Apr 20, 2020
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks again @francislavoie -- I like where this is going. I left a few comments about how we handle and what we should expect of the matcher token.

@mholt
Copy link
Member

mholt commented May 18, 2020

@zakcutner I propose we update this PR to accept a first argument that looks and acts like a standard path matcher token, i.e. handle_path /foo/* ... where we trim the * if present. Omitting the * would still be an exact match, and it would still strip it, essentially turning the path into /. This way it should act like all other path matchers, just with prefix stripping. How's that?

@francislavoie
Copy link
Member Author

I agree with that - I'll try to finish this one off soon.

@zakcutner
Copy link

Sound good 😃 It looks like this will solve a pretty specific use case so I agree there's not much point generalising it further like I was suggesting, particularly as it seems like that would be a lot of work. Thanks again to both of you for all your work on this!

@francislavoie francislavoie force-pushed the handle-path-directive branch from a731cf3 to 2c77d66 Compare May 23, 2020 02:27
@francislavoie
Copy link
Member Author

Alright, cleaned this up. I think this is done?

@francislavoie francislavoie requested a review from mholt May 23, 2020 02:28
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Great, this LGTM!

@zakcutner can you try this out? And then we will merge it if it works for you.

@mholt mholt removed the under review 🧐 Review is pending before merging label May 26, 2020
@zakcutner
Copy link

Just tried this out and it works great, thank you both 😃

@mholt mholt merged commit 8c5d00b into caddyserver:master May 26, 2020
@mholt
Copy link
Member

mholt commented May 26, 2020

Nice work on this one @francislavoie

@francislavoie francislavoie deleted the handle-path-directive branch May 26, 2020 23:29
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.

Strip path for reverse proxied requests
3 participants