WIP router: implement path rewrite with regex by cmluciano · Pull Request #8462 · envoyproxy/envoy

cmluciano · 2019-10-02T16:48:26Z

Co-Authored-By: Gabriel Taylor Russ gabriel.taylor.russ@gmail.com

Signed-off-by: Christopher M. Luciano cmluciano@us.ibm.com

Description: Support regex path rewrites
Risk Level: Medium, modifies an existing rewrite action
Testing: bazel test //test/common/router:config_impl_test
Docs Changes: TODO
Release Notes: TODO
Fixes #2092

TODO:

Add a few base tests for regex path
Add a test specifically showing rewrite happening after the RBAC filter

repokitteh-read-only · 2019-10-02T16:48:30Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8462 was opened by cmluciano.

see: more, trace.

cmluciano

For reviewers: I'm still trying to figure out why the prefix ends up being wrong once it hits the PrefixPathRewriter.rewrite function. Any help would be greatly appreciated.

source/common/router/config_impl.h

htuch

Thanks, some API and structural comments. Is this PR still WiP? If so, please reflect in the title.
/wait

api/envoy/api/v2/route/route.proto

source/common/json/config_schemas.cc

source/common/router/config_impl.h

htuch · 2019-10-04T03:30:38Z

source/common/router/config_impl.h

+                      bool case_sensitive) const {
+    ASSERT(case_sensitive ? absl::StartsWith(path, matched_path)
+                          : absl::StartsWithIgnoreCase(path, matched_path));
+    return std::regex_replace(matched_path, pattern_, substitution_);


We don't allow std::regex to be used going forward, see #7878.

It it enough to be passing in a type of RE2 or is std::regex_replace itself part of the problem?

Yes, please no std::regex of any kind on the data path.

stale · 2019-10-11T16:24:13Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

cmluciano · 2019-10-18T14:42:56Z

@htuch I believe the only remaining error failure is

[ RUN      ] RouteConfigurationV2.RegexPrefixWithNoRewriteWorksWhenPathChanged
[2019-10-18 14:32:29.254][1060923][critical][assert] [source/common/router/config_impl.cc:790] assert failure: regex_->match(pathOnly(headers)).
[2019-10-18 14:32:29.254][1060923][critical][backtrace] [bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:83] Caught Abort trap: 6, suspect faulting address 0x7fff6eaa72c6
[2019-10-18 14:32:29.254][1060923][critical][backtrace] [bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:70] Backtrace (use tools/stack_decode.py to get line numbers):
[2019-10-18 14:32:29.254][1060923][critical][backtrace] [bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:71] Envoy version: 0/1.12.0-dev/redacted/DEBUG/BoringSSL
external/bazel_tools/tools/test/test-setup.sh: line 310: 70995 Abort trap: 6           "${TEST_PATH}" "$@" 2>&1

This ASSERT has a comment that I'm wondering about here.

Do you think this is a signal that it should be revisited as to whether this ASSERT is necessary or does it indicate something wrong within the current PR?

htuch · 2019-10-18T21:02:30Z

@cmluciano are you clearing the route cache as the comment indicates? You need to do this if you modify path (or any header).

htuch

I think we're still waiting for the switch of the pattern match to be based on RegexMatcher and RE2.
/wait

stale · 2019-11-04T03:09:45Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

cmluciano · 2019-11-04T20:42:52Z

Not stale- I had some temporary compilation problems and now I'm working through basing this off of the RE2 library

stale · 2019-11-11T20:48:53Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

stale · 2019-11-18T22:17:00Z

This pull request has been automatically closed because it has not had activity in the last 14 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!

DevZenFlow · 2019-11-22T11:38:57Z

Can we re-open this please?

htuch · 2019-11-22T19:28:18Z

@vikpck reopened, but not would be good to hear from @cmluciano on the plan. I've heard at least 2 folks in the past week in various conversations ask for this :)

cmluciano · 2019-11-25T16:18:09Z

Thanks for re-opening. I'll be getting back to working on this today.

Co-Authored-By: Gabriel Taylor Russ <gabriel.taylor.russ@gmail.com> Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

stale · 2019-12-02T22:01:46Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

lotte-mollie · 2019-12-04T10:35:06Z

We would really like this functionality as well! Is there an ETA?

dio · 2019-12-08T23:09:10Z

We hope to see some TODOs be implemented soon. Meanwhile, seems like you need to merge master? And this: #8462 (review). Thanks!

stale · 2019-12-15T23:45:12Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

dio · 2019-12-16T02:19:02Z

No stale according to my exchange with @cmluciano.

stale · 2019-12-23T03:36:09Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

cmluciano · 2019-12-23T19:59:08Z

Not stale

stale · 2019-12-30T20:36:58Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

stale · 2020-01-06T21:14:44Z

This pull request has been automatically closed because it has not had activity in the last 14 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!

kenske · 2020-01-15T16:12:23Z

@cmluciano have you been able to make any progress on this?

jmarantz · 2020-01-15T19:48:32Z

I'm not sure who can take it over, but I would recommend using RE2 for regexes for this rather than std::regex. I think going forward we want to use that wherever possible in Envoy due to better performance and predictability.

Support path rewriting using regular expressions and optionally capture groups. This PR is like #8462, but using the safe regular expression support. Risk Level: Medium, since it slightly modifies the existing `prefix_rewrite` code. Testing: Unit tests are added to `test/common/router/config_impl_test.cc`, runnable with `bazel test //test/common/router:config_impl_test` Docs Changes: Any doc that references `prefix_rewrite` has been changed to reference `regex_rewrite` as well, if appropriate. Release Notes: A bullet is added to `docs/root/intro/version_history.rst` mentioning the new support. Signed-off-by: James Hennessy <jph@us.ibm.com>

cmluciano commented Oct 2, 2019

View reviewed changes

source/common/router/config_impl.h Outdated Show resolved Hide resolved

source/common/router/config_impl.h Show resolved Hide resolved

htuch suggested changes Oct 4, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 4, 2019

cmluciano changed the title ~~router: implement path rewrite with regex~~ WIP router: implement path rewrite with regex Oct 4, 2019

zuercher mentioned this pull request Oct 10, 2019

Feature Request: Support nginx style rewrite rules via rds.RedirectAction #2092

Closed

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Oct 11, 2019

repokitteh-read-only bot removed the waiting label Oct 14, 2019

repokitteh-read-only bot added the api label Oct 18, 2019

mattklein123 assigned htuch Oct 26, 2019

htuch suggested changes Oct 28, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 28, 2019

zuercher mentioned this pull request Nov 1, 2019

Support prefix rewrites with after string matches #8833

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 4, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 4, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 11, 2019

stale bot closed this Nov 18, 2019

htuch reopened this Nov 22, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2019

Christopher M. Luciano added 5 commits November 25, 2019 16:17

router: implement path rewrite with regex

1d0c788

Co-Authored-By: Gabriel Taylor Russ <gabriel.taylor.russ@gmail.com> Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

router: move regex_rewrite to separate field and fix tests

cd64d21

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

router: only rewrite if path_rewriter is not empty

ff3d72f

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

router: remove json golden routes

e69e5d6

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

router: use new setPath for header rewrite

63b28e8

Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

cmluciano force-pushed the cml/rewriteregex branch from 56235c1 to 63b28e8 Compare November 25, 2019 21:49

repokitteh-read-only bot removed the waiting label Nov 25, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 2, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 4, 2019

htuch added the waiting label Dec 4, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 15, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 16, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 23, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 23, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 30, 2019

stale bot closed this Jan 6, 2020

jphx mentioned this pull request Feb 13, 2020

router: implement new regex_rewrite route action #10050

Merged

Conversation

cmluciano commented Oct 2, 2019

Uh oh!

repokitteh-read-only bot commented Oct 2, 2019

Uh oh!

cmluciano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

cmluciano Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Oct 18, 2019

Choose a reason for hiding this comment

Uh oh!

stale bot commented Oct 11, 2019

Uh oh!

cmluciano commented Oct 18, 2019

Uh oh!

htuch commented Oct 18, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

stale bot commented Nov 4, 2019

Uh oh!

cmluciano commented Nov 4, 2019

Uh oh!

stale bot commented Nov 11, 2019

Uh oh!

stale bot commented Nov 18, 2019

Uh oh!

DevZenFlow commented Nov 22, 2019

Uh oh!

htuch commented Nov 22, 2019

Uh oh!

cmluciano commented Nov 25, 2019

Uh oh!

stale bot commented Dec 2, 2019

Uh oh!

lotte-mollie commented Dec 4, 2019

Uh oh!

dio commented Dec 8, 2019

Uh oh!

stale bot commented Dec 15, 2019

Uh oh!

dio commented Dec 16, 2019

Uh oh!

stale bot commented Dec 23, 2019

Uh oh!

cmluciano commented Dec 23, 2019

Uh oh!

stale bot commented Dec 30, 2019

Uh oh!

stale bot commented Jan 6, 2020

Uh oh!

kenske commented Jan 15, 2020

Uh oh!

jmarantz commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants