Caddy: v2: Discussion: `strip` directive to replace `strip_prefix` and `strip_suffix`

[whitestrake]

caddy version: v2.0.0-beta9.0.20200311221200-cfe85a9fe625 (built to latest commit as of this issue, no modules)

This Caddyfile works:

http:// {
  route /foo* {
    strip_prefix foo
  }
}

This Caddyfile does not:

http:// {
  route /foo* {
    strip_prefix /foo
  }
}

The error given is:

2020/03/13 00:23:51.326 INFO using adjacent Caddyfile run: adapting config using caddyfile: parsing caddyfile tokens for 'route': Caddyfile:3 - Error during parsing: parsing caddyfile tokens for 'strip_prefix': Caddyfile:3 - Error during parsing: Wrong argument count or unexpected line ending after 'strip_prefix'

[mholt]

Ah, just a hunch real quick, I bet that's because path matchers have to start with a forward slash... so it thinks the first (and only) argument is a path matcher, and not the prefix that should be stripped. When it then looks for the next argument to see what prefix should be stripped, it can't find any. See https://caddyserver.com/docs/caddyfile/concepts#path-matcher -- I think even the examples demonstrate this scenario: https://caddyserver.com/docs/caddyfile/directives/strip_prefix#examples

Let me know if there's any other questions!

[whitestrake]

That explains the cause, but do you not consider this a bug?

Providing one valid prefix works, a different valid prefix to strip does not work. The documentation specifically outlines it as valid syntax, and separately explicitly acknowledges a leading slash as the norm: "This value may omit the leading forward slash / and it will be assumed."

https://caddyserver.com/docs/caddyfile/directives/strip_prefix

At bare minimum, the Caddyfile parser error is misleading, if not outright incorrect. And If we want to keep this unexpected and unintuitive behaviour, shouldn't we at least update the documentation to advise as such?

I definitely don't feel that the example sufficiently demonstrates that a single argument to strip_prefix that contains a leading slash will generate this error. As far as I know, it's just showing off that you _may_ omit the slash, as mentioned above.

[mholt]

To clarify:

One prefix is a prefix to match (just like any other directive), another prefix is the prefix to strip. It's a bit confusing in this case since both arguments are about prefixes. :)

The strip_prefix directive does not _have_ to accept a matcher. We could remove that argument entirely, so that the prefix to match is always the same as the prefix to match. However, then it becomes impossible to filter requests by anything other than path, without using another directive like route or handle first, i.e. strip_prefix @post /foo becomes impossible...

So, matcher token or no matcher token? Those are our options at this point.

I think the current implementation is correct, if using matchers.

[francislavoie]

I think requiring strip_prefix * /foo is fine, but I think the Caddyfile parsing error messages need blanket improvements across the board to make it clearer to users. Ideally we have time to make a push before stable in that area. I would call it a nice-to-have, not a must-have though.

[whitestrake]

So after discussing further with @mholt I've come to understand that resolving this to work exactly how the current syntax specifies will come up is technically difficult. This is because the Caddyfile parser attempts to locate a matcher as the first argument before it takes the remaining arguments and passes them to the directive. This is uniform across the entire Caddyfile. So, altering this to handle this specific case is not really feasible.

Rather than the obvious options remaining:

• Document this edge case and leave it alone
• Remove the optionality of the matcher (i.e. always require one)
• Remove the matcher entirely (i.e. never accept a matcher)

Which all have their own downsides... I proposed to Matt that we flip the script on the strip_x directives and pick a new syntax that can never encounter this problem:

strip [<matcher>] <prefix|suffix> <string>

Matt immediately enumerated a few possible examples:

strip @post prefix /foo
strip suffix /my-suffix

The question was also raised as to whether strip_uri would be more appropriate. While it's good to specify URI here, just for clarity's sake, and it also becomes consistent with the replace_uri directive, I'm personally rooting for just strip because it's simpler, and it reads more like intuitive English when you're reading one of these directives in a Caddyfile.

Thoughts?

[francislavoie]

Oooh, I like that idea of a strip directive. Good idea!!

I don't think the name strip_uri is necessary. Most things in the Caddyfile deal with paths already, it feels pretty implicit to me.

[mholt]

Great idea @whitestrake

@francislavoie The only problem with using a generic name like strip is that no other directive -- even if it deals with something other than the path -- can be named that. For example, maybe a directive that strips content from the response body? That's why replace_uri directive is called what it is...

So I'm leaning toward strip_uri or actually probably strip_path since it only deals with the path portion of the URI.

[francislavoie]

How about strip [<matcher>] <path_prefix|path_suffix|content> <string>

Edit: We could also consider the uglier but more terse ^ and $ from regex to avoid having to specify the words prefix and suffix?

Like... strip [<matcher>] path ^/prefix or strip [<matcher>] path /suffix$

I don't love it but figured it worth bringing up the idea. Might be confusing for users, they might assume it supports full regex (which it could... but...)

[whitestrake]

While nothing _immediately_ comes to mind as being something else someone would want to _strip_ in the context of request handling, I suppose being specific with the directive name is the responsible course of action here in case something does pop up.

(Edit: stripping headers, but that's obviously covered elsewhere.)

It just irks me because strip is quite a short, neat word and I feel like having things be short, neat, and intuitive/readable is one of the main values of the Caddyfile.

I'd take something like that first example, @francislavoie - although just <path_prefix|path_suffix> would probably suffice because altering arbitrarily-placed content comes under the domain of replace_uri, which in fact covers all of these cases, just that stripping URI prefixes and suffixes are such a common task they deserved their own shorthand.

Definitely don't love borrowing regex anchors.

[francislavoie]

although just would probably suffice because altering arbitrarily-placed content comes under the domain of replace_uri

I agree, was just trying to address @mholt's point specifically about strip no longer being able to be used for other purposes

Definitely don't love borrowing regex anchors.

👍

[francislavoie]

I might take a shot at implementing this over the weekend if someone else doesn't do it first 😄

Seems easy enough, just some Caddyfile argument handling changes.

[mholt]

@francislavoie @whitestrake

How about a single uri directive that unifies:

• strip_prefix
• strip_suffix
• uri_replace

Proposed syntax:

uri [<matcher>] strip_prefix|strip_suffix|replace <needle> [<replacement>]

Examples:

# these first two are equivalent by chance,
# simply because a path that doesn't have
# that prefix won't be stripped of it!
uri /foo/* strip_prefix /foo/
uri strip_prefix /foo/

# only strips suffix if ends with /bar
uri strip_suffix /bar 

# only does replacement if matches @post and has "foo"
uri @post replace foo bar

Sound good? If so, I can get on that today.

[francislavoie]

Yep, sounds good to me. Sorry I didn't get around to this yet, wanted to try implementing but if you have the time, go for it! 😄

[mholt]

@francislavoie @whitestrake I've implemented these changes in #3157! Please take a look, and I think I can release beta 18 today.