router: Create InternalRedirectPolicy in side RouteAction and extend it with pluggable predicates by penguingao · Pull Request #10908 · envoyproxy/envoy

penguingao · 2020-04-22T22:02:20Z

Description: router: Create InternalRedirectPolicy to capture all internal redirect related options and extend it with pluggable predicates similar to retry plugins. The previous_routes and whitelisted_routes predicate allow creating a DAG of routes for internal redirects. Each node in the DAG is a route. whitelisted_routes defines the edges of each node. previous_routes serves as visited status keeper for each of the edge. This prevents infinite loop, while allowing loop to exist in the DAG.
Risk Level: Medium
Testing: Unit tests. Integration tests.
Docs Changes: Updated HCM architecture overview page. Added toctree for the predicates.
Release Notes: Updated version history.

pluggable predicates. Signed-off-by: pengg <pengg@google.com>

Signed-off-by: pengg <pengg@google.com>

repokitteh-read-only · 2020-04-22T22:02:25Z

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

🐱

Caused by: #10908 was opened by penguingao.

see: more, trace.

penguingao · 2020-04-22T22:06:59Z

/assign @alyssawilk

Signed-off-by: pengg <pengg@google.com>

penguingao · 2020-04-23T14:04:17Z

/retest

repokitteh-read-only · 2020-04-23T14:07:45Z

🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: api (failed build)
🔨 rebuilding ci/circleci: go_control_plane_mirror (failed build)
🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #10908 (comment) was created by @penguingao.

see: more, trace.

penguingao · 2020-04-23T14:12:41Z

/retest

repokitteh-read-only · 2020-04-23T14:12:47Z

🔨 rebuilding ci/circleci: go_control_plane_mirror (failed build)
🔨 rebuilding ci/circleci: docs (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: api (failed build)

🐱

Caused by: a #10908 (comment) was created by @penguingao.

see: more, trace.

penguingao · 2020-04-23T15:08:54Z

/retest

repokitteh-read-only · 2020-04-23T15:08:59Z

🔨 rebuilding ci/circleci: go_control_plane_mirror (failed build)
🔨 rebuilding ci/circleci: api (failed build)
🔨 rebuilding ci/circleci: docs (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10908 (comment) was created by @penguingao.

see: more, trace.

Signed-off-by: pengg <pengg@google.com>

htuch

API looks reasonable, a few comments..

api/envoy/config/route/v3/route_components.proto

htuch · 2020-04-27T01:05:03Z

api/envoy/config/route/v3/route_components.proto

+
+  // HTTP scheme combinations that are allowed to be handled as internal
+  // redirect. Downstream scheme comes first and redirect target URL scheme
+  // follows.


This seems pretty strange to be including the target URL scheme; why isn't is sufficient to include just the downstream?

For that matter, why do we even need this, given that the route match that leads to the redirect itself has a scheme matcher?

This was from the preexisting behavior: it allows a HTTP downstream to be redirected to only HTTP targets; it allows HTTPS downstream to be redirected to any targets.

My understanding is that, if an origin redirect an HTTP request to an HTTPS target, there is a chance that the origin would like to move the downstream to a secure transport for sensitive payload.

While this is not always the case, certainly not our use case, I decided to make this configurable but preserving the existing behavior.

By "scheme matcher" did you mean VirtualHost. require_tls?

Scheme seems an odd one here IMO.
upstream scheme is generally just based on "is the connection to upstream secure or not"
I think in general we want to make sure that x-forwarded-proto matches the scheme of the redirect URI. If we want to say we only redirect https:// requests or http:// requests that'd be the request x-forwarded-proto header not scheme

Ah, thanks @alyssawilk and @htuch!

I think I had misunderstood the purpose of the existing check on x-forwarded-proto vs redirect URI scheme. Our use case led me to think about the redirect URL more as a way to define the desired upstream for a request, but the redirect URL should really be treated purely as a property of the downstream, which makes the existing check make perfect sense, and indeed makes the scheme option here very odd. In other words, Upstream's scheme is completely orthogonal to what the downstream's scheme is.

I'll remove this option completely.

This is now done. I removed this option.

I added an option to allow cross scheme redirect - I think this should be fine since as @hutch said, there are scheme match in routes, we can selectively allow certain route to be redirected to, across scheme boundary if we know this is safe.

Signed-off-by: pengg <pengg@google.com>

api/envoy/config/route/v3/route_components.proto

Signed-off-by: pengg <pengg@google.com>

…-more-options Signed-off-by: pengg <pengg@google.com>

Signed-off-by: pengg <pengg@google.com>

behavior change is documented in the release note. Signed-off-by: pengg <pengg@google.com>

Signed-off-by: pengg <pengg@google.com>

penguingao · 2020-05-13T02:10:35Z

/retest

repokitteh-read-only · 2020-05-13T02:10:39Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10908 (comment) was created by @penguingao.

see: more, trace.

Signed-off-by: pengg <pengg@google.com>

alyssawilk

Almost there!

alyssawilk · 2020-05-13T12:59:06Z

api/envoy/config/route/v3/route_components.proto

+  repeated google.protobuf.Any predicates = 3;
+
+  // Disallow internal redirect to follow a target URI with a different scheme than the value of
+  // x-forwarded-proto. Default behavior is to follow such redirects.


Ugh, sorry I didn't comment earlier, but I don't think I like the default behavior being IMO sketchy (forwarding https content over http connections)
How about we either switch the default (last time I swear!) which means the behavioral change between the old and the new is the new is more strict, and folks can flip it and apply your predicate if they want old behavior?

alyssawilk · 2020-05-13T13:03:13Z

docs/root/version_history/current.rst

+* router: internal redirect configs are moved to the :ref`internal_redirect_policy
+  <envoy_api_field_router.RouterAction.internal_redirect_policy>` field, which defines more fine
+  grained control of the internal redirect behavior. This changes the default cross scheme redirect behavior.
+  All cross scheme redirect is allowed by default, but http downstream to https target is disallowed. To restore


I think you also need to doc up the deprecated field in the Deprecated section and maybe move comments on how to preserve behavior there?

penguingao · 2020-05-13T13:07:23Z

It looks like I ran into windows 259 path length limit with the long predicate name.

windows path length limitation. Change default behavior to be disallowing cross scheme redirect. Rename the option to allow_cross_scheme (default to false). Update tests. Document implemeneted default behavior change. Signed-off-by: pengg <pengg@google.com>

penguingao · 2020-05-13T14:05:57Z

I renamed the predicate with a shorter name to get around window bazel path length limit as well. Thanks!

alyssawilk · 2020-05-13T14:06:43Z

cc @wrowe anything we can do about the path limits? Given the directory names are fairly long I'm hoping we can find a workaround to not have to reduce readability

alyssawilk

LGTM modulo a few tiny nits. Huzzah!

api/envoy/config/route/v3/route_components.proto

include/envoy/router/internal_redirect.h

source/common/router/router.cc

api/envoy/extensions/internal_redirect/allow_listed_routes/v3/allow_listed_routes_config.proto

AllowListed -> Allow listed. Signed-off-by: pengg <pengg@google.com>

penguingao

Thanks!

alyssawilk

@mattklein123 or @htuch for final API changes?

mattklein123

API LGTM!

htuch

/lgtm api

penguingao added 4 commits April 22, 2020 17:12

Move internal redirect configs into InternalRedirectPolicy and implement

2d1206f

pluggable predicates. Signed-off-by: pengg <pengg@google.com>

Generate protobuf.

15f1510

Signed-off-by: pengg <pengg@google.com>

Fix typos.

81debc8

Signed-off-by: pengg <pengg@google.com>

Fix more typo in the comments.

e3eee18

Signed-off-by: pengg <pengg@google.com>

repokitteh-read-only bot added the api label Apr 22, 2020

repokitteh-read-only bot assigned alyssawilk Apr 22, 2020

penguingao added 11 commits April 22, 2020 18:22

Fix ref links.

c89a6b4

Signed-off-by: pengg <pengg@google.com>

Fix link in hcm arch overview.

0f77ae8

Signed-off-by: pengg <pengg@google.com>

Fix doc links. For real.

985819a

Signed-off-by: pengg <pengg@google.com>

And fix the formats.

8199c60

Signed-off-by: pengg <pengg@google.com>

Fix typos.

fac46f2

Signed-off-by: pengg <pengg@google.com>

More typo correction.

fe9e5c9

Signed-off-by: pengg <pengg@google.com>

More typos.

179e5a9

Signed-off-by: pengg <pengg@google.com>

fix clang-tidy findings.

1c7ada2

Signed-off-by: pengg <pengg@google.com>

ConfigTest -> PreviousRoutesTest.

925950c

Signed-off-by: pengg <pengg@google.com>

Format.

61e15f7

Signed-off-by: pengg <pengg@google.com>

Kick CI

a806956

Signed-off-by: pengg <pengg@google.com>

penguingao added 2 commits April 23, 2020 11:38

Kick CI

5711900

Signed-off-by: pengg <pengg@google.com>

Avoid using newly deprecated fields in the integration test.

684ba65

Signed-off-by: pengg <pengg@google.com>

htuch reviewed Apr 27, 2020

View reviewed changes

penguingao added 2 commits April 27, 2020 10:16

Add comments InternalRedirectAction enum values.

ad05c9b

Signed-off-by: pengg <pengg@google.com>

Run proto_format on proto comment change.

c606a1e

Signed-off-by: pengg <pengg@google.com>

htuch reviewed May 5, 2020

View reviewed changes

api/envoy/config/route/v3/route_components.proto Outdated Show resolved Hide resolved

penguingao added 2 commits May 5, 2020 10:31

Adjust stats_integration expectation according to CI.

5be8361

Signed-off-by: pengg <pengg@google.com>

Fix router_test.

5c1e199

Signed-off-by: pengg <pengg@google.com>

htuch mentioned this pull request May 7, 2020

compressor: expose generic compressor filter to users #10553

Merged

penguingao added 6 commits May 12, 2020 15:49

Merge remote-tracking branch 'upstream/master' into internal-redirect…

8138621

…-more-options Signed-off-by: pengg <pengg@google.com>

Move predicate config to use TypedExtensionConfig.

8461c5f

Signed-off-by: pengg <pengg@google.com>

Implete OnlyAllowSafeCrossSchemeRedirect internal redirect predicate.

56b0188

Signed-off-by: pengg <pengg@google.com>

Fix format.

15a4d3f

Signed-off-by: pengg <pengg@google.com>

Add OnlyAllowSafeCrossSchemeRedirect predicate to docs. Make sure the

948ca63

behavior change is documented in the release note. Signed-off-by: pengg <pengg@google.com>

Fix previous_route_test.

93cd0e6

Signed-off-by: pengg <pengg@google.com>

Fix clang tidy finding.

c62f9da

Signed-off-by: pengg <pengg@google.com>

alyssawilk reviewed May 13, 2020

View reviewed changes

API comment fix. const vars. Add @param to interface documentation.

416e79e

AllowListed -> Allow listed. Signed-off-by: pengg <pengg@google.com>

penguingao commented May 13, 2020

View reviewed changes

alyssawilk approved these changes May 13, 2020

View reviewed changes

mattklein123 self-assigned this May 13, 2020

mattklein123 approved these changes May 13, 2020

View reviewed changes

repokitteh-read-only bot removed the api label May 13, 2020

htuch reviewed May 14, 2020

View reviewed changes

alyssawilk merged commit 1ce0109 into envoyproxy:master May 14, 2020

penguingao deleted the internal-redirect-more-options branch May 14, 2020 18:48

KBaichoo mentioned this pull request Aug 12, 2020

WatchDog Extension hook #12416

Merged

rgs1 mentioned this pull request Jan 4, 2021

access log: add support for command formatter extensions #14512

Merged

Conversation

penguingao commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 22, 2020

Uh oh!

penguingao commented Apr 22, 2020

Uh oh!

penguingao commented Apr 23, 2020

Uh oh!

repokitteh-read-only bot commented Apr 23, 2020

Uh oh!

penguingao commented Apr 23, 2020

Uh oh!

repokitteh-read-only bot commented Apr 23, 2020

Uh oh!

penguingao commented Apr 23, 2020

Uh oh!

repokitteh-read-only bot commented Apr 23, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

penguingao Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

penguingao Apr 28, 2020 • 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!

penguingao Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

penguingao commented May 13, 2020

Uh oh!

repokitteh-read-only bot commented May 13, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk May 13, 2020

Choose a reason for hiding this comment

Uh oh!

alyssawilk May 13, 2020

Choose a reason for hiding this comment

Uh oh!

penguingao May 13, 2020

Choose a reason for hiding this comment

Uh oh!

penguingao commented May 13, 2020

Uh oh!

penguingao commented May 13, 2020

Uh oh!

alyssawilk commented May 13, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

penguingao left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

penguingao commented Apr 22, 2020 •

edited

Loading

penguingao Apr 28, 2020 •

edited

Loading