Skip to content

http: support route mutability#15266

Merged
snowp merged 86 commits intoenvoyproxy:mainfrom
mengmichael1:mmeng-patch
Apr 9, 2021
Merged

http: support route mutability#15266
snowp merged 86 commits intoenvoyproxy:mainfrom
mengmichael1:mmeng-patch

Conversation

@mengmichael1
Copy link
Copy Markdown
Contributor

@mengmichael1 mengmichael1 commented Mar 2, 2021 •
edited
Loading

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.

@mengmichael1
init
102f429 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
testing clang pre push
d0d0dc7 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
Merge remote-tracking branch 'upstream/main' into mmeng-patch
b6c915e 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
make sharedptr RouteImpl instead of casting
afff58a 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
spelling prepush lint
ae46a70 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add setRoute impl for conn manager impl
47ff663 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
setRoute impl for async client
828f622 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
check_format lint
183c9db 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add setRoute into filter_manager ActiveStreamFilterBase
a0482e1 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add FilterSetRoute unit test, fix setRoute in conn manager impl
5a47c50 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
spelling
f9dd462 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add DelegatingRoute class
72149ba 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
Merge remote-tracking branch 'upstream/main' into mmeng-patch
6ce6f25 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add DelegatingRouteEntry class
125be46 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
use setRoute as helper to refreshCachedRoute
347ccb6 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add DelegatingRoute child classes for testing
75d2b9f 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
fix import
6adc0be 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
lint
529ee21 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add FilterSetDelegatingRoute test
528baf0 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
standardize eq checks in unit test
22f36bb 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
use make_shared instead of shared_ptr to new obj
4362081 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
rm setRoute impl in async client
fbfe11a 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
first attempt at integration test
f990290 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
lint
d801fdb 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add setRoute info into Life of a Request docs
2f2b53e 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
fixing up set route filter int test
8c716e5 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
int test: don't wait for next upstream rq
06f36a1 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
spell prepush lint
6b2144b 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
Merge remote-tracking branch 'upstream/main' into mmeng-patch
19f4936 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
clean up int test
ffe478e 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Apr 1, 2021

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

@mengmichael1
rm reference to lyft
b5fc3ed 
Signed-off-by: Michael Meng <mmeng@lyft.com>
alyssawilk
alyssawilk reviewed Apr 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

@alyssawilk alyssawilk self-assigned this Apr 5, 2021
@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Apr 5, 2021

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

@mengmichael1
start on DelegatingRouteEntryAllCalls unit test: mocks setup
cbdea10 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add unit test coverage for all routeEntry delegated methods
54c15eb 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
int test: use access log to check upstream cluster, use localAddress …
5fa8e6f 
…asString helper

Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
move int test from separate file into integration_test
574a90c 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
Merge remote-tracking branch 'upstream/main' into mmeng-patch
0b44130 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
Copy link
Copy Markdown
Contributor Author

mengmichael1 commented Apr 7, 2021 •
edited
Loading

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.

@mengmichael1 mengmichael1 changed the title filter: support route mutability http: support route mutability Apr 8, 2021
@mengmichael1
fix routeEntry virtualHost assert
e8e6867 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add docs about route mutation into arch_overview http filters
07ac1b9 
Signed-off-by: Michael Meng <mmeng@lyft.com>
alyssawilk
alyssawilk reviewed Apr 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@mengmichael1
Merge remote-tracking branch 'upstream/main' into mmeng-patch
fe30a8e 
Signed-off-by: Michael Meng <mmeng@lyft.com>
@mengmichael1
add release notes into 1.18.0
d730810 
Signed-off-by: Michael Meng <mmeng@lyft.com>
alyssawilk
alyssawilk previously approved these changes Apr 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@snowp
Copy link
Copy Markdown
Contributor

snowp commented Apr 8, 2021

@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
Copy link
Copy Markdown
Member

davinci26 commented Apr 8, 2021

I have some time now, I can take a look

@mengmichael1
ensure orig dst ip addr is localhost and not 0.0.0.0 for windows
032792b 
Signed-off-by: Michael Meng <mmeng@lyft.com>
davinci26
davinci26 reviewed Apr 9, 2021
View reviewed changes
@mengmichael1
getLoopbackAddressUrlString
92f250d 
Signed-off-by: Michael Meng <mmeng@lyft.com>
snowp
snowp approved these changes Apr 9, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Happy to see this land!

@snowp snowp merged commit ead18ed into envoyproxy:main Apr 9, 2021
@mengmichael1 mengmichael1 mentioned this pull request Apr 13, 2021
Closed
Monkeyanator pushed a commit to Monkeyanator/envoy that referenced this pull request Apr 20, 2021
@mengmichael1
http: support route mutability (envoyproxy#15266)
7b441b4 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

+3 more reviewers

@davinci26 davinci26 davinci26 left review comments

@snowp snowp snowp approved these changes

@alyssawilk alyssawilk alyssawilk left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@snowp snowp

@alyssawilk alyssawilk

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mengmichael1 @snowp @alyssawilk @davinci26 @mattklein123