filters: http original src filter #6790

klarose · 2019-05-02T20:39:16Z

Description: Add an http filter to extract the original source on a per-http-request basis
A while back I added the original src listener filter. That filter would use the downstream connection's source address to build a socket option that would then be applied to upstream connections, thereby forcing their source address to mirror that of the downstream connection.

This works fine for cases where Envoy is directly exposed to the internet, or a device in front of it is placing a proxy protocol header on it. However, if proxy protocol is not an option and something (e.g. another Envoy) is between Envoy and the client, then the listener filter is insufficient for two reasons:

The source address of the connection will be the source address of the "last hop", not the client.
Multiple http requests from different sources may be carried over the same tcp connection, meaning that we must create new options for each request, not for each connection.

The http original_src filter solves these two problems by using the http stack's knowledge of the request to determine the source address (e.g. via the XFF), and by setting the options on a per-request basis. It reuses the vast majority of the work done for the listener filter -- the connection pooling, etc, as well as some of the code build to create the options.

In short, this PR carries the entirety of the work to enable the http original src filter, which can be summarized as:

Refactoring some bits of the original_src listener filter out into a common library
Adding the concept of upstream connection options to the StreamDecoderFilterCallbacks and the LoadBalancerContext
Using the upstream connection options when creating the http connection pool in conjunction with the existing downstream connection options to build the full set of options to be used when creating upstream connections.
Adding the protobuf config for the original_src http filter
Writing the simple filter which creates the original_src socket options based on the downstreamRemoteAddress
Documenting the new filter

Risk Level: Medium
Testing: UT + Manual. I tried writing an integration test, but it needs a bit of infrastructure work to support running it, since it needs CAP_NET_ADMIN permissions. I started that a while back, but I didn't have time to complete it.
Docs Changes: Added docs for the filter
Release Notes: Will add

Other Notes:
This is quite large. Let me know if you want me to split it up somehow.

We'll want to use this option for both the http and the listener filter. Bring it out into a common library. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

The original source filter is now all hooked up, but it has some problems -- namely, it sets options on the downstream connection's socket, which is wrong. Signed-off-by: Kyle Larose <kyle@agilicus.com>

We need these to configure per-request socket options from the http filters. Signed-off-by: Kyle Larose <kyle@agilicus.com>

- Hook in to new callback options mechanism - add ut Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

We use the existing mechanism used to pass socket options to the connection pool creation logic to pass the upstream socket options. We do this by creating a new socket options object, then placing both sets of socket options into it. Signed-off-by: Kyle Larose <kyle@agilicus.com>

The active stream is what's shared between the various filters. Push the options up to it so they're actually shared! Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

- build - docs Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

zuercher · 2019-05-03T16:21:23Z

@snowp can you have a look?

snowp

Looked over the source/ bits, looks good for the most part.

I think it would be good to split up the changes made to the core code (ie the new filter callbacks) and the changes made to extensions if possible, should make this a bit easier to review

docs/root/intro/arch_overview/ip_transparency.rst

docs/root/configuration/http_filters/original_src_filter.rst

source/common/upstream/cluster_manager_impl.cc

source/extensions/filters/http/original_src/original_src.h

klarose · 2019-05-03T20:06:45Z

@snowp Thanks for the review. Would you suggest I close this, then post two new ones broken up as you suggested?

The extension code will depend on the new filter code. I'm guessing for things to work out I'll likely need to wait for the first PR (the new filter code) to finish before posting the second (the extension code)?

snowp · 2019-05-03T20:13:28Z

Given the dependency it might make sense to keep this up and just put up a smaller PR with the router changes. You can then merge in the changes once they land on master into this PR (this is how changes were split out and merged back into #6228 for example).

klarose · 2019-05-03T20:17:11Z

Got it. I'll do that. Thanks for the suggestion.

repokitteh-read-only · 2019-05-05T00:00:39Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6790 (comment) was created by @cpakulski.

see: more, trace.

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Cleaned up docs and a comment Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-05-08T21:13:24Z

@snowp I've merged in the changes we split out, and addressed your remaining comments.

Signed-off-by: Kyle Larose <kyle@agilicus.com>

The test didn't like tests being named the same across modules. Signed-off-by: Kyle Larose <kyle@agilicus.com>

snowp

Looks solid, just a few minor comments

Have these original src destinations got a maintainer sponsor sorted out as per https://github.com/envoyproxy/envoy/blob/3573b07af1ab5c4cf687ced0f80e2ccc0a0b7ec2/EXTENSION_POLICY.md#adding-new-extensions ? If not I don't mind taking it. Make sure to encode this into CODEOWNERS as outlined in the doc I linked

source/extensions/filters/http/original_src/original_src.cc

source/extensions/filters/http/original_src/original_src.h

test/extensions/filters/http/original_src/original_src_test.cc

Comments + style issues Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-05-09T18:22:56Z

Sorry -- I didn't notice the extension policy!

We definitely don't have a maintainer sorted out. Whoever does may also want to take on filters/listener/original_src as well, since it's very similar, and serves the same purpose.

snowp

Made another pass, looks pretty good. Feel free to put me down as the sponsor for this and the other original src filter

source/extensions/filters/common/original_src/socket_option_factory.cc

source/extensions/filters/http/original_src/config.h

source/extensions/filters/http/original_src/original_src.cc

test/extensions/filters/http/original_src/config_test.cc

test/extensions/filters/http/original_src/original_src_config_factory_test.cc

test/extensions/filters/http/original_src/original_src_test.cc

snowp · 2019-05-13T15:40:41Z

test/extensions/filters/http/original_src/original_src_test.cc

+  ASSERT_FALSE(mark_option.has_value());
+}
+
+TEST_F(OriginalSrcHttpTest, DecodeDataDoesNothing) {


These tests that just verify nothing seem a bit strange to me because they're exercising call orders that will never happen: decodeData will never be called before decodeHeaders, so normally I see tests run through the expected order of of decode* calls.

It's common to maintain state between the callbacks, e.g. for decodeHeaders to decide whether something happens in decodeData, which means that calling decodeData on its own often doesn't prove much. This test in particular that calls decodeData(, true) followed by decodeData(, false) especially stands out as a bit strange.

I don't feel super strongly about it in the context of this filter due to its simplicity, just wanted to point it out.

I'm actually implicitly asserting on nothing being invoked on callbacks -- it's a strict mock with no expectations set.

Any thoughts on how to make that more obvious? Some way of telling the mock that it should expect no calls at all (without needing to enumerate the disallowed calls)? I agree that it does look odd.

What I've done in the past is run through the entire filter lifecycle in a single test: decodeHeaders/decodeData/decodeTrailers, using strict mocks to verify that nothing happens on data/trailers. I don't really feel strongly about this so up to you if you want to change this

That's reasonable. I'll do that.

Signed-off-by: Kyle Larose <kyle@agilicus.com>

- const nits - explicit constructor - capitalization - simplified a few tests Also added myself as a reviewer. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-05-15T12:40:22Z

I think I got the sponsorship right -- the extension policy didn't explicitly call out what it meant to be a sponsor, only how to add reviewers for an extension. :)

Either way, I added you as the first reviewer, then figured that since two are required, and contributors are allowed, I could take that on as well. Let me know if I misinterpreted something there.

snowp

LGTM just one unresolved question on one of the comments (wether to rework the unit tests a bit)

test/extensions/filters/http/original_src/config_test.cc

snowp · 2019-05-20T20:19:38Z

test/extensions/filters/http/original_src/original_src_test.cc

+  ASSERT_FALSE(mark_option.has_value());
+}
+
+TEST_F(OriginalSrcHttpTest, DecodeDataDoesNothing) {


What I've done in the past is run through the entire filter lifecycle in a single test: decodeHeaders/decodeData/decodeTrailers, using strict mocks to verify that nothing happens on data/trailers. I don't really feel strongly about this so up to you if you want to change this

Signed-off-by: Kyle Larose <kyle@agilicus.com>

For the trailers and decodedata tests, run through the normal flow taken by an http filter. Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-05-21T22:18:22Z

/retest

repokitteh-read-only · 2019-05-21T22:18:26Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6790 (comment) was created by @klarose.

see: more, trace.

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-05-22T18:44:07Z

/retest

repokitteh-read-only · 2019-05-22T19:22:15Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #6790 (comment) was created by @klarose.

see: more, trace.

klarose · 2019-05-28T13:37:05Z

@snowp What's next on this? (Aside from fixing the merge conflict). I think I've addressed all your issues.

snowp

Sorry for the delay. This LGTM but it seems like you have a merge conflict, could you resolve and then we'll merge?

Signed-off-by: Kyle Larose <kyle@agilicus.com>

snowp

LGTM, thanks!

klarose added 11 commits April 29, 2019 16:34

refactor original_src_socket_option into common

c4de56e

We'll want to use this option for both the http and the listener filter. Bring it out into a common library. Signed-off-by: Kyle Larose <kyle@agilicus.com>

refactor creation of socket options to helper

03100a4

Signed-off-by: Kyle Larose <kyle@agilicus.com>

finish up skeleton of original source filter

d13f905

The original source filter is now all hooked up, but it has some problems -- namely, it sets options on the downstream connection's socket, which is wrong. Signed-off-by: Kyle Larose <kyle@agilicus.com>

add upstreamSocketOptions

ae0a630

We need these to configure per-request socket options from the http filters. Signed-off-by: Kyle Larose <kyle@agilicus.com>

finish up http original src filter

78cb8e8

- Hook in to new callback options mechanism - add ut Signed-off-by: Kyle Larose <kyle@agilicus.com>

fixup rebase previous original_src_filter

3ce2987

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Move upstream options to ActiveStream

818e307

The active stream is what's shared between the various filters. Push the options up to it so they're actually shared! Signed-off-by: Kyle Larose <kyle@agilicus.com>

document original_src http filter

1be9292

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into http_original_src_filter

9822df4

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Various fixes

918a5c6

- build - docs Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose mentioned this pull request May 2, 2019

End-to-End IP transparency using Proxy Protocol #4128

Closed

klarose added 2 commits May 2, 2019 16:50

add release note

04480a0

Signed-off-by: Kyle Larose <kyle@agilicus.com>

fix compile issue not found by gcc

b289bb6

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose changed the title ~~[filters] http original src filter~~ filters: http original src filter May 3, 2019

zuercher assigned snowp May 3, 2019

snowp suggested changes May 3, 2019

View reviewed changes

klarose mentioned this pull request May 3, 2019

[http] add upstreamSocketOptions and hook them into http connection pool creation #6804

Merged

klarose added 3 commits May 8, 2019 16:19

Merge branch 'master' into http_original_src_filter

81a54e4

Signed-off-by: Kyle Larose <kyle@agilicus.com>

review fixes

0c294dd

Cleaned up docs and a comment Signed-off-by: Kyle Larose <kyle@agilicus.com>

fix spelling

2df6143

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose added 2 commits May 9, 2019 08:56

Merge branch 'master' into http_original_src_filter

9766731

Signed-off-by: Kyle Larose <kyle@agilicus.com>

fix failing coverage test

b48b79a

The test didn't like tests being named the same across modules. Signed-off-by: Kyle Larose <kyle@agilicus.com>

snowp suggested changes May 9, 2019

View reviewed changes

code review fixes

cecf5e3

Comments + style issues Signed-off-by: Kyle Larose <kyle@agilicus.com>

snowp suggested changes May 13, 2019

View reviewed changes

klarose added 3 commits May 13, 2019 13:36

add snowp as the original_src sponsor.

db489b3

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Code review fixes

af1c9bc

- const nits - explicit constructor - capitalization - simplified a few tests Also added myself as a reviewer. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into http_original_src_filter

9f93270

Signed-off-by: Kyle Larose <kyle@agilicus.com>

snowp reviewed May 20, 2019

View reviewed changes

klarose added 2 commits May 21, 2019 11:36

Merge branch 'master' into http_original_src_filter

594edef

Signed-off-by: Kyle Larose <kyle@agilicus.com>

Rework UT a bit

befc157

For the trailers and decodedata tests, run through the normal flow taken by an http filter. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into http_original_src_filter

b69c87f

Signed-off-by: Kyle Larose <kyle@agilicus.com>

snowp previously approved these changes May 28, 2019

View reviewed changes

Merge branch 'master' into http_original_src_filter

6a7ed5e

Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose dismissed snowp’s stale review via 6a7ed5e May 29, 2019 12:32

snowp approved these changes May 29, 2019

View reviewed changes

snowp merged commit 712a72a into envoyproxy:master May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filters: http original src filter #6790

filters: http original src filter #6790

klarose commented May 2, 2019

zuercher commented May 3, 2019

snowp left a comment

klarose commented May 3, 2019

snowp commented May 3, 2019

klarose commented May 3, 2019

repokitteh-read-only bot commented May 5, 2019

klarose commented May 8, 2019

snowp left a comment

klarose commented May 9, 2019

snowp left a comment

snowp May 13, 2019

klarose May 14, 2019

snowp May 20, 2019

klarose May 21, 2019

klarose commented May 15, 2019

snowp left a comment

snowp May 20, 2019

klarose commented May 21, 2019

repokitteh-read-only bot commented May 21, 2019

klarose commented May 22, 2019

repokitteh-read-only bot commented May 22, 2019

klarose commented May 28, 2019

snowp left a comment

snowp left a comment

filters: http original src filter #6790

filters: http original src filter #6790

Conversation

klarose commented May 2, 2019

zuercher commented May 3, 2019

snowp left a comment

Choose a reason for hiding this comment

klarose commented May 3, 2019

snowp commented May 3, 2019

klarose commented May 3, 2019

repokitteh-read-only bot commented May 5, 2019

klarose commented May 8, 2019

snowp left a comment

Choose a reason for hiding this comment

klarose commented May 9, 2019

snowp left a comment

Choose a reason for hiding this comment

snowp May 13, 2019

Choose a reason for hiding this comment

klarose May 14, 2019

Choose a reason for hiding this comment

snowp May 20, 2019

Choose a reason for hiding this comment

klarose May 21, 2019

Choose a reason for hiding this comment

klarose commented May 15, 2019

snowp left a comment

Choose a reason for hiding this comment

snowp May 20, 2019

Choose a reason for hiding this comment

klarose commented May 21, 2019

repokitteh-read-only bot commented May 21, 2019

klarose commented May 22, 2019

repokitteh-read-only bot commented May 22, 2019

klarose commented May 28, 2019

snowp left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment