envoy icon indicating copy to clipboard operation
envoy copied to clipboard

uri_template: Allow uri_template variables in literal expression

Open DieBauer opened this issue 1 year ago • 7 comments

Commit Message: Allow uri_template variables in literal expression Additional Description: UriTemplateMatch now allows for variables to be defined in a literal. Before this change, all variables had to be clamped by path separators (/).

This is not according RFC6570.

Even though the spec is not implemented by Envoy (*, ** are not a glob in the file system sense), it is good to have more feature parity with URI Templates.

The change is to treat path separators as literals. The restriction can then be easily lifted to allow variables inside literals.

2 checks have been rewritten, double slashes are not allowed, /abc//d and /abc/{var=/d}, and that a PathGlob can only exist between two path separators. Risk Level: Low Testing: Tests added and adjusted where necessary Docs Changes: Release Notes: Optional Fixes #Issue: #34507

DieBauer avatar Jun 26 '24 13:06 DieBauer

@yanavlasov friendly ping.

htuch avatar Jul 04 '24 03:07 htuch

I've asked the original author @silverstar194 to review as well, as I do not fully understand the consequences of this change.

/wait-any

yanavlasov avatar Jul 09 '24 20:07 yanavlasov

Hi @yanavlasov @silverstar194 is there anything I can do to move this PR forward? Let me know!

DieBauer avatar Jul 15 '24 07:07 DieBauer

This changes some validation logic which we don't know whether is currently safe or not in production with existing configs, so can you add a knob to the code to have the option to preserve the current behavior? Ideally in a way that keeping the knob around won't lead to duplicated code.

Also please add test cases for the things changed.

Thanks for the review! I'll need some time to process and work on. Do you have a similar set-up in Envoy for this 'preserve' current behavior? It sounds like a feature-flag. Any hints would be appreciated.

DieBauer avatar Jul 19 '24 11:07 DieBauer

#34974 is an example of using feature-flag for a breaking change.

/wait-any

yanavlasov avatar Jul 19 '24 12:07 yanavlasov

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions[bot] avatar Aug 18 '24 16:08 github-actions[bot]

I still need to go through the pathrewrite implications. With holidays ongoing it's slower than expected. I'll mark this as draft, or when autoclose kicks in ask for reopening when done.

DieBauer avatar Aug 25 '24 08:08 DieBauer

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions[bot] avatar Sep 24 '24 12:09 github-actions[bot]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions[bot] avatar Oct 01 '24 12:10 github-actions[bot]