uri_template: Add support for the "*" character matching in pattern rewrite and matching #39169

barchw · 2025-04-18T14:10:07Z

Commit Message: uri_template: Add support for the "*" character matching in pattern rewrite and matching
Additional Description:
Adds support for matching requests that include the * character in the path. A relevant issue was created: #39168
Risk Level:
Testing: Updated unit tests
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A
Runtime guard: envoy.reloadable_features.uri_template_match_on_asterisk

Resolves: #39168

repokitteh-read-only · 2025-04-18T14:10:13Z

Hi @barchw, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #39169 was opened by barchw.

see: more, trace.

mathetake · 2025-04-18T16:27:29Z

source/extensions/path/uri_template_lib/uri_template_internal.cc

-                                       "="; // user included "=" allowed
+                                       "=*"; // restricted characters


cc @RyanTheOptimist as a reviewer of this exclusion of * reserved character. do you have any context why it's the only char excluded?

Using the literal * in a path seems really a bad practice to me however (e.g. google search doesn't support non encoded reserved chars, not limited to *), so i am not sure if we want to expand this direction.

yanavlasov

/wait-any

yanavlasov · 2025-04-21T21:19:07Z

source/extensions/path/uri_template_lib/uri_template_internal.cc

@@ -44,7 +44,7 @@ constexpr absl::string_view kLiteral = "a-zA-Z0-9-._~" // Unreserved
                                       "%"             // pct-encoded
                                       "!$&'()+,;"     // sub-delims excluding *=
                                       ":@"
-                                       "="; // user included "=" allowed


I do not know why * was excluded as a match character. But for a change like this a runtime guard is required as a minimum, since it changes the existing behavior of the matcher.

Hi @yanavlasov, thanks for looking into this, I have moved the feature behind a runtime guard.

repokitteh-read-only · 2025-04-22T11:42:33Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #39169 was synchronize by barchw.

see: more, trace.

…ewrite and matching Signed-off-by: Chwila <bartosz.chwila@sap.com>

Signed-off-by: Chwila <bartosz.chwila@sap.com>

RyanTheOptimist

LGTM, modulo a small nit

RyanTheOptimist · 2025-04-30T17:14:25Z

source/extensions/path/uri_template_lib/uri_template_internal.cc

@@ -46,6 +47,14 @@ constexpr absl::string_view kLiteral = "a-zA-Z0-9-._~" // Unreserved
                                       ":@"
                                       "="; // user included "=" allowed

+// Additional literal that allows "*" in the pattern.
+// Valid pchar from https://datatracker.ietf.org/doc/html/rfc3986#appendix-A
+constexpr absl::string_view kLiteralAsterisk = "a-zA-Z0-9-._~" // Unreserved


nit: This should probably be "LiteralWithAsterisk". The current name makes it sound like it would be just a literal asterisk. (And perhaps add a comment about removing when we remove the runtime feature)

Thanks for the review, adapted

Signed-off-by: Chwila <bartosz.chwila@sap.com>

phlax · 2025-05-05T14:55:49Z

/backport

…ewrite and matching (envoyproxy#39169) Adds support for matching requests that include the `*` character in the path. A relevant issue was created: envoyproxy#39168 Risk Level: Testing: Updated unit tests Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Runtime guard: envoy.reloadable_features.uri_template_match_on_asterisk Resolves: envoyproxy#39168 --------- Signed-off-by: Chwila <bartosz.chwila@sap.com>

…ewrite and matching (envoyproxy#39169) Adds support for matching requests that include the `*` character in the path. A relevant issue was created: envoyproxy#39168 Risk Level: Testing: Updated unit tests Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Runtime guard: envoy.reloadable_features.uri_template_match_on_asterisk Resolves: envoyproxy#39168 --------- Signed-off-by: Chwila <bartosz.chwila@sap.com> Signed-off-by: Yan Avlasov <yavlasov@google.com>

…ewrite and matching (#39169) Adds support for matching requests that include the `*` character in the path. A relevant issue was created: #39168 Risk Level: Testing: Updated unit tests Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Runtime guard: envoy.reloadable_features.uri_template_match_on_asterisk Resolves: #39168 --------- Signed-off-by: Chwila <bartosz.chwila@sap.com> Signed-off-by: Yan Avlasov <yavlasov@google.com>

barchw requested review from wbpcode and ravenblackx as code owners April 18, 2025 14:10

barchw force-pushed the fix-uri-template-asterisk-sign branch 2 times, most recently from f4ae9e2 to f453fe9 Compare April 18, 2025 14:12

This was referenced Apr 18, 2025

RBAC access denied on path containing * kyma-project/api-gateway#1839

Open

URI template operators * and ** do not match the * character #39168

Closed

barchw force-pushed the fix-uri-template-asterisk-sign branch from 5cfc62e to bf77bcd Compare April 18, 2025 15:03

mathetake reviewed Apr 18, 2025

View reviewed changes

ravenblackx assigned RyanTheOptimist Apr 21, 2025

yanavlasov reviewed Apr 21, 2025

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Apr 21, 2025

barchw added 3 commits April 22, 2025 13:42

uri_template: Add support for the "*" character matching in pattern r…

776d95c

…ewrite and matching Signed-off-by: Chwila <bartosz.chwila@sap.com>

fix format issue

be6db60

Signed-off-by: Chwila <bartosz.chwila@sap.com>

Add missing words to dictionary

3d236f8

Signed-off-by: Chwila <bartosz.chwila@sap.com>

barchw force-pushed the fix-uri-template-asterisk-sign branch from 412071e to 8998e86 Compare April 22, 2025 11:44

Put the feature behind runtime guard

7d607ee

Signed-off-by: Chwila <bartosz.chwila@sap.com>

barchw force-pushed the fix-uri-template-asterisk-sign branch 2 times, most recently from afee755 to 45fa9bc Compare April 22, 2025 12:45

Keep the static variables

e3e313e

Signed-off-by: Chwila <bartosz.chwila@sap.com>

barchw force-pushed the fix-uri-template-asterisk-sign branch from 45fa9bc to e3e313e Compare April 22, 2025 13:11

Update literal comment

f6dd87f

Signed-off-by: Chwila <bartosz.chwila@sap.com>

barchw force-pushed the fix-uri-template-asterisk-sign branch from 870e754 to f6dd87f Compare April 22, 2025 13:58

RyanTheOptimist assigned yanavlasov and unassigned RyanTheOptimist Apr 24, 2025

RyanTheOptimist previously approved these changes Apr 30, 2025

View reviewed changes

barchw dismissed RyanTheOptimist’s stale review via 9bb1efd April 30, 2025 19:21

barchw force-pushed the fix-uri-template-asterisk-sign branch from 9bb1efd to 26f07e8 Compare April 30, 2025 19:22

barchw requested a review from RyanTheOptimist April 30, 2025 19:24

barchw force-pushed the fix-uri-template-asterisk-sign branch from 26f07e8 to f4900c0 Compare April 30, 2025 19:32

Switch the constexpr name from kLiteralAsterisk to kLiteralWithAsterisk

9716900

Signed-off-by: Chwila <bartosz.chwila@sap.com>

barchw force-pushed the fix-uri-template-asterisk-sign branch from f4900c0 to 9716900 Compare April 30, 2025 19:54

yanavlasov approved these changes May 2, 2025

View reviewed changes

yanavlasov merged commit 59c6863 into envoyproxy:main May 2, 2025
24 checks passed

repokitteh-read-only bot added the backport/review label May 5, 2025

This was referenced May 8, 2025

trigger 1.24.6 build istio/release-builder#2105

Merged

trigger 1.25.3 build istio/release-builder#2104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

uri_template: Add support for the "*" character matching in pattern rewrite and matching #39169

uri_template: Add support for the "*" character matching in pattern rewrite and matching #39169

Uh oh!

barchw commented Apr 18, 2025 •

edited

Loading

Uh oh!

repokitteh-read-only bot commented Apr 18, 2025

Uh oh!

mathetake Apr 18, 2025 •

edited

Loading

Uh oh!

yanavlasov left a comment

Uh oh!

yanavlasov Apr 21, 2025

Uh oh!

barchw Apr 22, 2025

Uh oh!

repokitteh-read-only bot commented Apr 22, 2025

Uh oh!

RyanTheOptimist left a comment

Uh oh!

RyanTheOptimist Apr 30, 2025

Uh oh!

barchw Apr 30, 2025

Uh oh!

Uh oh!

phlax commented May 5, 2025

Uh oh!

Uh oh!

		"="; // user included "=" allowed
		"=*"; // restricted characters

uri_template: Add support for the "*" character matching in pattern rewrite and matching #39169

uri_template: Add support for the "*" character matching in pattern rewrite and matching #39169

Uh oh!

Conversation

barchw commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 18, 2025

Uh oh!

mathetake Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

barchw Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Apr 22, 2025

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

barchw Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phlax commented May 5, 2025

Uh oh!

Uh oh!

barchw commented Apr 18, 2025 •

edited

Loading

mathetake Apr 18, 2025 •

edited

Loading