feat(router): keep query string by default for redirects by knight42 · Pull Request #11493 · envoyproxy/envoy

knight42 · 2020-06-08T05:01:35Z

Signed-off-by: knight42 anonymousknight96@gmail.com

Commit Message: leave query string intact by default(strip_query: false) when performing path redirect.

Additional Description:
I think the current behavior(stripping query by default) is not aligned with the documentation. From the documentation of path_redirect and strip_query field, I am under the impression that query portion is not considered part of path portion. Additionally, even if we want to strip query for path redirects, we can simply set strip_query to true.

Risk Level:
Testing: unit test
Docs Changes:
Release Notes: query string is not stripped by default for path redirects

Fixes #11443

knight42 · 2020-06-08T05:02:13Z

/cc @snowp

zuercher

Thanks! I left a couple comments. Mostly I think we need to carefully consider how this impacts existing configurations.

Also, the release notes section of the pull request template is primarily a prompt to remind developers to consider whether release notes in docs/root/version_history/current.rst need to be updated. I do think we should have a release note for this change. More details are in the PULL_REQUESTS.md file.

zuercher · 2020-06-09T16:23:45Z

I think this would be a bit easier to follow if current_path was just declared and used within the !path_redirect_.empty() block.

if path_redirect_ is empty, we would need to set final_path to current_path.

I'm suggesting that in that final else block you can just use final_path = headers.getPathValue(); and then move the declaration of current_path to just before it's remaining uses.

zuercher

I think the changes in the router look good modulo some changes around comments.

We should document the relationship between path_redirect and strip_query in api/envoy/api/v2/route/route_components.proto and api/envoy/config/route/v3/route_components.proto. Basically that path_redirect with a query string overrides strip_query. I believe you'll need to run tools/proto_format/proto_format.sh fix to update some generated code or you'll get CI errors.

Please avoid force pushes unless you're fixing a DCO error -- it sometimes causes github to lose track of comments and makes changes harder to follow. We always squash commits to master so the final git history will be clean.

zuercher · 2020-06-11T17:04:49Z

Let's remove this comment.

I do think it's worth having a brief note near here that explains that the path_redirect_ query string, if any, takes precedence over the request's query string.

repokitteh-read-only · 2020-06-12T06:12:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11493 was synchronize by knight42.

see: more, trace.

knight42 · 2020-06-12T06:15:14Z

@zuercher really grateful to your detailed guidance! I have updated the comments, please check again.

zuercher

Sorry. Noticed one more thing. But we have to wait for api shepherds to tag this as approved, anyway.

zuercher · 2020-06-13T23:50:02Z

Last pass. I should have realized this earlier, but the value we compute for this variable never changes for a given instance of RouteEntryImplBase. I think this should become a const member variable of RouteEntryImplBase. It'll save us a string search on every request that hits a path redirect.

zuercher · 2020-06-13T23:50:05Z

If we have path_redirect_has_query_ we can also split this if statement and skip searching headers.getPathValue() for a ? when we know we're not going to use it.

knight42 · 2020-06-14T16:52:10Z

@zuercher that's ok. should I ping someone?

zuercher

cc @envoyproxy/api-shepherds this PR has some comment-only changes to v2 to clarify the change in behavior

htuch · 2020-06-16T15:30:35Z

/lgtm v2-freeze
/lgtm api

zuercher · 2020-06-16T23:11:24Z

@mattklein123 do you want a second look at this?

mattklein123

Thanks a few questions/comments.

/wait

mattklein123 · 2020-06-17T03:50:30Z

Can you add some examples here (make sure to add the examples to v3 also)? I've been looking at this for a few minutes and I'm a little confused as to what the current behavior is and what is changing.

Currently the request's query string will be stripped regardless of the value of strip_query.

With this patch, envoy would only strip the request's query string if path_redirect doesn't contain query string and strip_query is set to true.
If path_redirect contains query string, users will be redirected to the specified path with query string no matter what the value of strip_query is.

Thanks can you add some actual examples of real paths and what happens to them?

mattklein123 · 2020-06-17T03:51:19Z

I think even if we think this is the right thing to do, this needs to be runtime guarded (default can be on). Can you please add a runtime feature flag for this?

I am not sure how to add a runtime feature flag and what its name should be, could you please provide an example?

Take a look at some blame/commits that touch this array: https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.cc#L54

mattklein123 · 2020-06-17T03:52:06Z

If we have a runtime guard do we need this warning? Should this potentially just be a failure? Again I'm confused as to the behavior.

mattklein123 · 2020-06-17T03:53:03Z

Suggested change

// the path_redirect query string, if any, takes precedence over the request's query string,

// The path_redirect query string, if any, takes precedence over the request's query string,

mattklein123 · 2020-06-17T03:53:57Z

nit: const for all

knight42 · 2020-06-28T10:10:40Z

@mattklein123 do you think I should add a test for the previous behavior(always strip the request's query string)?

Signed-off-by: knight42 <anonymousknight96@gmail.com>

repokitteh-read-only · 2020-06-28T11:30:31Z

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

🐱

Caused by: #11493 was synchronize by knight42.

see: more, trace.

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 · 2020-06-28T11:32:35Z

force push to rebase and resolve conflicts.

mattklein123

Thank you makes total sense to me now. Just a few small comments. Also friendly request to please never force push as it breaks reviews. Please just merge main and add commits. Thanks!

/wait

mattklein123 · 2020-06-28T16:17:58Z

+  if (Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.preserve_query_string_in_redirects")) {


Is it possible to not check this twice since you check it during route load? It might not be consistent then. Can we latch the expected behavior in the constructor?

mattklein123 · 2020-06-28T16:18:16Z

        redirect: { host_redirect: new.lyft.com }
      - match: { path: "/path/redirect/"}
        redirect: { path_redirect: "/new/path-redirect/" }
+      - match: { path: "/path/redirect/strip-query/true"}


We should have tests for the runtime override set to false also.

Signed-off-by: knight42 <anonymousknight96@gmail.com>

Add tests for turning off feature `envoy.reloadable_features.preserve_query_string_in_path_redirects`. Signed-off-by: knight42 <anonymousknight96@gmail.com>

mattklein123

Thank you!

…11493) Signed-off-by: knight42 <anonymousknight96@gmail.com> Signed-off-by: Yifan Yang <needyyang@google.com>

knight42 force-pushed the feat/path-redirect-keep-query branch from f9511e0 to 92091ae Compare June 8, 2020 12:33

mattklein123 assigned zuercher Jun 8, 2020

zuercher requested changes Jun 9, 2020

View reviewed changes

knight42 force-pushed the feat/path-redirect-keep-query branch from 92091ae to f834250 Compare June 10, 2020 06:53

knight42 requested a review from zuercher June 10, 2020 06:57

knight42 force-pushed the feat/path-redirect-keep-query branch 2 times, most recently from a53dd16 to 2fd7dd3 Compare June 11, 2020 03:29

zuercher reviewed Jun 11, 2020

View reviewed changes

repokitteh-read-only Bot added v2-freeze api labels Jun 12, 2020

knight42 requested a review from zuercher June 12, 2020 06:13

zuercher reviewed Jun 13, 2020

View reviewed changes

knight42 force-pushed the feat/path-redirect-keep-query branch from a258584 to 2a867e5 Compare June 14, 2020 16:51

knight42 requested a review from zuercher June 14, 2020 16:51

zuercher previously approved these changes Jun 15, 2020

View reviewed changes

repokitteh-read-only Bot removed v2-freeze api labels Jun 16, 2020

zuercher assigned mattklein123 Jun 16, 2020

mattklein123 requested changes Jun 17, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 17, 2020

knight42 dismissed zuercher’s stale review via 5a556f4 June 17, 2020 10:12

repokitteh-read-only Bot added v2-freeze api and removed waiting labels Jun 17, 2020

knight42 added 8 commits June 28, 2020 19:27

feat(router): keep query string by default for redirects

4d1550a

Signed-off-by: knight42 <anonymousknight96@gmail.com>

doc(router): update comments about query string in path_redirect

de8cc9f

Signed-off-by: knight42 <anonymousknight96@gmail.com>

fix: cache path_redirect_has_query_

f990c73

Signed-off-by: knight42 <anonymousknight96@gmail.com>

fix nit

f23408e

Signed-off-by: knight42 <anonymousknight96@gmail.com>

update comments about path_redirect

8bc9daf

Signed-off-by: knight42 <anonymousknight96@gmail.com>

feat: add runtime feature

5a48fd5

Signed-off-by: knight42 <anonymousknight96@gmail.com>

doc(router): provide example for path_redirect

ba72789

Signed-off-by: knight42 <anonymousknight96@gmail.com>

fix: address format errors

eb2e0c9

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 force-pushed the feat/path-redirect-keep-query branch from d4ad4ec to 2c28b44 Compare June 28, 2020 11:30

repokitteh-read-only Bot removed the waiting label Jun 28, 2020

doc: resolve conflicts

73e3734

Signed-off-by: knight42 <anonymousknight96@gmail.com>

knight42 force-pushed the feat/path-redirect-keep-query branch from 2c28b44 to 73e3734 Compare June 28, 2020 11:30

mattklein123 requested changes Jun 28, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 28, 2020

knight42 added 2 commits June 29, 2020 16:54

fix: make feature name more accurate

d22e4bf

Signed-off-by: knight42 <anonymousknight96@gmail.com>

test(router): add missing tests

689621f

Add tests for turning off feature `envoy.reloadable_features.preserve_query_string_in_path_redirects`. Signed-off-by: knight42 <anonymousknight96@gmail.com>

repokitteh-read-only Bot removed the waiting label Jun 29, 2020

mattklein123 approved these changes Jun 29, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Jun 29, 2020

zuercher approved these changes Jun 29, 2020

View reviewed changes

mattklein123 merged commit 89d6c6c into envoyproxy:master Jun 29, 2020

knight42 deleted the feat/path-redirect-keep-query branch June 30, 2020 03:05

knight42 mentioned this pull request Jun 30, 2020

test(router): fix PathRedirectQueryNotPreserved test #11812

Merged

aimless404 pushed a commit to aimless404/envoy that referenced this pull request Jun 30, 2020

feat(router): keep query string by default for redirects (envoyproxy#…

6b486ca

…11493) Signed-off-by: knight42 <anonymousknight96@gmail.com> Signed-off-by: Yifan Yang <needyyang@google.com>

mattklein123 mentioned this pull request Jan 11, 2021

envoy.reloadable_features.preserve_query_string_in_path_redirects deprecation #14648

Closed

	// the path_redirect query string, if any, takes precedence over the request's query string,
	// The path_redirect query string, if any, takes precedence over the request's query string,

		if (Runtime::runtimeFeatureEnabled(
		"envoy.reloadable_features.preserve_query_string_in_redirects")) {

Conversation

knight42 commented Jun 8, 2020

Uh oh!

knight42 commented Jun 8, 2020

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only Bot commented Jun 12, 2020

Uh oh!

knight42 commented Jun 12, 2020

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knight42 commented Jun 14, 2020

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Jun 16, 2020

Uh oh!

zuercher commented Jun 16, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knight42 commented Jun 28, 2020

Uh oh!

repokitteh-read-only Bot commented Jun 28, 2020

Uh oh!

knight42 commented Jun 28, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels