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

caddyfile: Populate regexp matcher names by default #6145

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Conversation

francislavoie
Copy link
Member

This is something I've been mulling over for months, I was trying to think of the right solution to solve this, and I think I finally just realized "duh, just add a context map to the dispenser".

The idea is that often users might define a named matcher with a regexp like this:

@oldposts path_regexp oldposts ^/post/(.*)$
redir @oldposts /posts/{re.oldposts.1}

We wrote oldposts a total of four times 😬 What if we instead we could omit having to name the regexp, when we've already named the named matcher? With this PR, now this works:

@oldposts path_regexp ^/post/(.*)$
redir @oldposts /posts/{re.oldposts.1}

Anyway, this is kinda complementary to #6113 but totally independent. With that one, this is possible, by not even using the matcher name at all:

@oldposts path_regexp ^/post/(.*)$
redir @oldposts /posts/{re.1}

Which is fine for simple cases where the regexp is used immediate after matching and there's no risk of another regexp matcher clobbering it.

Worth noting, because of how CEL matchers are compiled & provisioned at runtime, and not at Caddyfile-adapt time, this feature doesn't work for passing names into expression matchers right now. It might be possible to add the matcher name to the expression matcher's JSON as well, so that at runtime it can add it to the context when provisioning. Not sure if that's worth doing yet, but I might take a crack at it just for completion's sake.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Mar 3, 2024
@francislavoie francislavoie added this to the v2.8.0 milestone Mar 3, 2024
@francislavoie francislavoie requested a review from mholt March 3, 2024 08:58
@francislavoie
Copy link
Member Author

I implemented it for the expression matchers as well. It's kinda gnarly though:

  • MatchExpression has custom JSON marshaling. I don't know why we did that. But anyway if we want to pass through the Name, we need to marshal it as a whole object and not just the expression string. So I changed it accordingly. Ran into issues with infinite loops/stack overflows because lol JSON marshal/unmarshal is recursive, so we need to use separate types as the target to cancel the recursion. Thankfully, this is fully backwards compatible with old JSON configs (obviously Caddyfiles will adapt differently but that's fine).
  • Added a ctx.WithValue() helper to copy the caddy.Context, because the stdlib context.WithValue() returns a ctx.Context but we need a caddy.Context and type asserting gave me an error saying "it's not a caddy.Context, it's a ctx.valueCtx 😭
  • Tests broke because we're using context during expression provisioning, so I had to fix it to use a proper context. No biggie, just copying how we did it in other tests.

@mholt
Copy link
Member

mholt commented Mar 6, 2024

Thanks for all this work, wow.

I do have concerns that it feels quite complex (code logic) just to save 1 token (or possibly 2 tokens) in the config.

I also wonder what if the matcher has multiple regexes? (I think that's possible, IIRC. Maybe not. Haven't looked lol)

@mholt mholt added the under review 🧐 Review is pending before merging label Mar 6, 2024
@francislavoie
Copy link
Member Author

francislavoie commented Mar 6, 2024

I also wonder what if the matcher has multiple regexes?

It's only possible if you use header_regexp and path_regexp in the same matcher set, can't have multiple of the same. But obviously a CEL matcher can have many of each.

At that point, the user is expected to use a name to disambiguate them.

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.

Nice work -- thank you Francis for improving the Caddyfile!

@mholt mholt merged commit 9cd472c into master Apr 17, 2024
25 checks passed
@mholt mholt deleted the matcher-regexp-names branch April 17, 2024 18:19
@mholt mholt removed the under review 🧐 Review is pending before merging label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants