http: support route mutability #15266

mengmichael1 · 2021-03-02T21:54:04Z

Commit Message: http: support route mutability

Additional Description: Implements #14904.

Adds a setRoute method into the Filter API, adding setRoute into HCM ActiveStream where it is implemented as a StreamFilterCallbacks for filters to call
Adds a DelegatingRoute / DelegatingRouteEntry mechanism to easily create a route that mutates specific properties of an existing route.

Risk Level: Low, implemented as new feature (no behavior changes)

Testing:

Unit tests in HttpConnectionManagerImplTest: testing (1) happy path case of setting the route to a DelegatingRoute that overrides clusterName, (2) setRoute(nullptr), (3) all routeEntry calls are delegated correctly and exhibit the same behavior as the base route's routeEntry
Integration test with (IntegrationTest, SetRouteToDelegatingRouteWithClusterOverride)

Docs Changes: "Life of a Request" docs, HTTP filters arch docs

Release Notes: Added to 1.18.0

http: added support for stream filters to mutate the cached route set by HCM route resolution. Useful for filters in a filter chain that want to override specific methods/properties of a route. See :ref:`http route mutation <arch_overview_http_filters_route_mutation>` docs for more information.

Platform Specific Features: n/a

[Optional API Considerations:] Should be noted that all changes (e.g. addition of a new virtual method) to the Route / RouteEntry API (in include/envoy/router) must also be changed in the DelegatingRoute / DelegatingRouteEntry API. Developers will see build errors if there are new virtual methods not implemented, which will act as a forcing function to maintain parity b/w the APIs.

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk · 2021-04-01T21:10:13Z

I'm headed out (EoD EST) and out tomorrow but I'll check it out early Monday!

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk

Looks great!
I'd encourage adding this into the release notes as a new feature as I think it's useful enough we should call it out :-)

source/common/router/delegating_route_impl.cc

source/common/router/delegating_route_impl.h

test/integration/set_delegating_route_test.cc

alyssawilk · 2021-04-05T13:45:37Z

Oh also looks like CI is wedged -a new push should sort that out.

Signed-off-by: Michael Meng <mmeng@lyft.com>

…asString helper Signed-off-by: Michael Meng <mmeng@lyft.com>

Signed-off-by: Michael Meng <mmeng@lyft.com>

mengmichael1 · 2021-04-07T22:45:31Z

Thanks for the review @alyssawilk! I think this is ready for another pass now.

~~You mentioned adding this to the release notes, how do I do that?~~ Found it. Will add to https://github.com/envoyproxy/envoy/blob/main/docs/root/version_history/current.rst tomorrow!

On the topic of docs, I've added some notes about route mutation into the arch overview http filters docs. You think that's an appropriate place?

Evening update: I'm also noticing CI things are failing with windows, will dive into sometime tomorrow.

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk

LGTM modulo ci being sorted out and release notes (glad you found the right file, and the other additions are fantastic!)

Also as I'm out tomorrow feel free to ping snow for merge once those two are addressed!

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk

LGTM! Will defer merge to snow since I'm headed out for a long weekend shortly :-)

snowp · 2021-04-08T21:52:24Z

@envoyproxy/windows-dev There seems to be some Windows only (I think tsan is unrelated? but not 100% sure) on this PR, any chance anyone of you could help debugging? The error message is unfortunately not very helpful

davinci26 · 2021-04-08T21:57:10Z

I have some time now, I can take a look

Signed-off-by: Michael Meng <mmeng@lyft.com>

test/integration/integration_test.cc

Signed-off-by: Michael Meng <mmeng@lyft.com>

snowp

LGTM, thanks! Happy to see this land!

Adds a setRoute method into the Filter API, adding setRoute into HCM ActiveStream where it is implemented as a StreamFilterCallbacks for filters to call Adds a DelegatingRoute / DelegatingRouteEntry mechanism to easily create a route that mutates specific properties of an existing route. Signed-off-by: Michael Meng <mmeng@lyft.com>

mengmichael1 added 30 commits February 9, 2021 12:14

init

102f429

Signed-off-by: Michael Meng <mmeng@lyft.com>

testing clang pre push

d0d0dc7

Signed-off-by: Michael Meng <mmeng@lyft.com>

Merge remote-tracking branch 'upstream/main' into mmeng-patch

b6c915e

Signed-off-by: Michael Meng <mmeng@lyft.com>

make sharedptr RouteImpl instead of casting

afff58a

Signed-off-by: Michael Meng <mmeng@lyft.com>

spelling prepush lint

ae46a70

Signed-off-by: Michael Meng <mmeng@lyft.com>

add setRoute impl for conn manager impl

47ff663

Signed-off-by: Michael Meng <mmeng@lyft.com>

setRoute impl for async client

828f622

Signed-off-by: Michael Meng <mmeng@lyft.com>

check_format lint

183c9db

Signed-off-by: Michael Meng <mmeng@lyft.com>

add setRoute into filter_manager ActiveStreamFilterBase

a0482e1

Signed-off-by: Michael Meng <mmeng@lyft.com>

add FilterSetRoute unit test, fix setRoute in conn manager impl

5a47c50

Signed-off-by: Michael Meng <mmeng@lyft.com>

spelling

f9dd462

Signed-off-by: Michael Meng <mmeng@lyft.com>

add DelegatingRoute class

72149ba

Signed-off-by: Michael Meng <mmeng@lyft.com>

Merge remote-tracking branch 'upstream/main' into mmeng-patch

6ce6f25

Signed-off-by: Michael Meng <mmeng@lyft.com>

add DelegatingRouteEntry class

125be46

Signed-off-by: Michael Meng <mmeng@lyft.com>

use setRoute as helper to refreshCachedRoute

347ccb6

Signed-off-by: Michael Meng <mmeng@lyft.com>

add DelegatingRoute child classes for testing

75d2b9f

Signed-off-by: Michael Meng <mmeng@lyft.com>

fix import

6adc0be

Signed-off-by: Michael Meng <mmeng@lyft.com>

lint

529ee21

Signed-off-by: Michael Meng <mmeng@lyft.com>

add FilterSetDelegatingRoute test

528baf0

Signed-off-by: Michael Meng <mmeng@lyft.com>

standardize eq checks in unit test

22f36bb

Signed-off-by: Michael Meng <mmeng@lyft.com>

use make_shared instead of shared_ptr to new obj

4362081

Signed-off-by: Michael Meng <mmeng@lyft.com>

rm setRoute impl in async client

fbfe11a

Signed-off-by: Michael Meng <mmeng@lyft.com>

first attempt at integration test

f990290

Signed-off-by: Michael Meng <mmeng@lyft.com>

lint

d801fdb

Signed-off-by: Michael Meng <mmeng@lyft.com>

add setRoute info into Life of a Request docs

2f2b53e

Signed-off-by: Michael Meng <mmeng@lyft.com>

fixing up set route filter int test

8c716e5

Signed-off-by: Michael Meng <mmeng@lyft.com>

int test: don't wait for next upstream rq

06f36a1

Signed-off-by: Michael Meng <mmeng@lyft.com>

spell prepush lint

6b2144b

Signed-off-by: Michael Meng <mmeng@lyft.com>

Merge remote-tracking branch 'upstream/main' into mmeng-patch

19f4936

Signed-off-by: Michael Meng <mmeng@lyft.com>

clean up int test

ffe478e

Signed-off-by: Michael Meng <mmeng@lyft.com>

rm reference to lyft

b5fc3ed

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk reviewed Apr 5, 2021

View reviewed changes

alyssawilk self-assigned this Apr 5, 2021

alyssawilk added the waiting label Apr 6, 2021

mengmichael1 added 5 commits April 6, 2021 11:42

start on DelegatingRouteEntryAllCalls unit test: mocks setup

cbdea10

Signed-off-by: Michael Meng <mmeng@lyft.com>

add unit test coverage for all routeEntry delegated methods

54c15eb

Signed-off-by: Michael Meng <mmeng@lyft.com>

int test: use access log to check upstream cluster, use localAddress …

5fa8e6f

…asString helper Signed-off-by: Michael Meng <mmeng@lyft.com>

move int test from separate file into integration_test

574a90c

Signed-off-by: Michael Meng <mmeng@lyft.com>

Merge remote-tracking branch 'upstream/main' into mmeng-patch

0b44130

Signed-off-by: Michael Meng <mmeng@lyft.com>

repokitteh-read-only bot removed the waiting label Apr 7, 2021

mengmichael1 changed the title ~~filter: support route mutability~~ http: support route mutability Apr 8, 2021

mengmichael1 added 2 commits April 7, 2021 21:18

fix routeEntry virtualHost assert

e8e6867

Signed-off-by: Michael Meng <mmeng@lyft.com>

add docs about route mutation into arch_overview http filters

07ac1b9

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk reviewed Apr 8, 2021

View reviewed changes

mengmichael1 added 2 commits April 8, 2021 12:12

Merge remote-tracking branch 'upstream/main' into mmeng-patch

fe30a8e

Signed-off-by: Michael Meng <mmeng@lyft.com>

add release notes into 1.18.0

d730810

Signed-off-by: Michael Meng <mmeng@lyft.com>

alyssawilk previously approved these changes Apr 8, 2021

View reviewed changes

ensure orig dst ip addr is localhost and not 0.0.0.0 for windows

032792b

Signed-off-by: Michael Meng <mmeng@lyft.com>

mengmichael1 dismissed alyssawilk’s stale review via 032792b April 9, 2021 00:09

davinci26 reviewed Apr 9, 2021

View reviewed changes

test/integration/integration_test.cc Outdated Show resolved Hide resolved

getLoopbackAddressUrlString

92f250d

Signed-off-by: Michael Meng <mmeng@lyft.com>

snowp approved these changes Apr 9, 2021

View reviewed changes

snowp merged commit ead18ed into envoyproxy:main Apr 9, 2021

mengmichael1 mentioned this pull request Apr 13, 2021

http conn manager: Support Route Mutability #14904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: support route mutability #15266

http: support route mutability #15266

mengmichael1 commented Mar 2, 2021 •

edited

alyssawilk commented Apr 1, 2021

alyssawilk left a comment

alyssawilk commented Apr 5, 2021

mengmichael1 commented Apr 7, 2021 •

edited

alyssawilk left a comment •

edited

alyssawilk left a comment

snowp commented Apr 8, 2021

davinci26 commented Apr 8, 2021

snowp left a comment

http: support route mutability #15266

http: support route mutability #15266

Conversation

mengmichael1 commented Mar 2, 2021 • edited

alyssawilk commented Apr 1, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Apr 5, 2021

mengmichael1 commented Apr 7, 2021 • edited

alyssawilk left a comment • edited

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

snowp commented Apr 8, 2021

davinci26 commented Apr 8, 2021

snowp left a comment

Choose a reason for hiding this comment

mengmichael1 commented Mar 2, 2021 •

edited

mengmichael1 commented Apr 7, 2021 •

edited

alyssawilk left a comment •

edited