upstream: add transparency options #5035

klarose · 2018-11-14T18:18:06Z

Description:
This adds two options to the upstream cluster definition:

mark
src_transparent

Mark is a 32-bit unsigned integer whose non-zero value indicates that all packets originating from envoy to the configured upstream cluster should be marked with the provided value.

src_transparent is a boolean where true indicates that L3/L4 source transparency should be enabled for this upstream cluster. That is, connections to the cluster will have the same IP address (and possibly port) as the original downstream connection.

Mark will just work as-is. However, src_transparent still needs implementation to work properly.

The options are implemented by adding socket options to the upstream cluster. Consequently, when an upstream connection is made, the socket options are applied. This infrastructure already existed.

The implementation applies SO_MARK in response to the mark configuration. This option seems to only exist on Linux. It requires CAP_NET_ADMIN privileges.

src_transparent applies IP_TRANSPARENT. The socket option code for this already exists. This change simply invokes it in the context of the upstream cluster. It also requires CAP_NET_ADMIN.

This is part of the infrastructure for #4128.

Risk Level: Low
This is an optional feature which is disabled by default. The only new logic is in choosing whether or not to add the socket options, called from existing code.

Testing:
Unit tests were added for the configuration options at every level. I ran the full unit test suite (which I think runs the integration tests.. If it doesn't, let me know, and I'll run them).

I manually tested that the mark socket option worked by running envoy in a container connected to a simple http upstream. With the required iptables rules in place, I could see the mark using conntrack -L.

Docs Changes:
I documented the API in the protobuf specification. The overall feature will need a proper blurb when it's done.

You can now set a "mark" unsigned integer value in a cluster. This will mark any upstream packets originating from envoy on this cluster with the given value. You can also configure a cluster to be "src_transparent". This will cause the cluster to use the downstream connection's remote address as the source address for the upstream connection. Signed-off-by: Kyle Larose <kyle@agilicus.com>

We can use this to control whether or not a packet is marked when emitted by Envoy. Signed-off-by: Kyle Larose <kyle@agilicus.com>

The cluster config supports the mark and src_transparent properties in the upstream cluster. The upstream cluster now reads these properties. It sets the corresponding socket options if the properties are present. Signed-off-by: Kyle Larose <kyle@agilicus.com>

api/envoy/api/v2/cds.proto

test/common/upstream/upstream_impl_test.cc

SO_MARK and IP_TRANSPARENT may not be supported on all platforms. Skip tests testing those if they aren't supported. Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2018-11-14T21:41:23Z

@lyft/network-team Can somebody take a look at this please?

Thanks!

Edit: I'm not sure the above actually worked... Are the contribution guidelines out of date?

mattklein123 · 2018-11-15T19:13:47Z

@zuercher do you mind taking a first pass on this?

stale · 2018-11-22T22:09:32Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

klarose · 2018-11-22T22:15:41Z

poke. Please review when back from holidays!

zuercher

Sorry it took so long for me to get to this. The overall approach seems solid.

You'll probably want to merge master as well, just to make sure things haven't drifted too far.

api/envoy/api/v2/cds.proto

source/common/config/cds_json.cc

test/common/upstream/upstream_impl_test.cc

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

- Removed json updates; json config is deprecated - Use scalar types for protobufs - Update new upstream_impl_tests to use yaml rather than json. Signed-off-by: Kyle Larose <kyle@agilicus.com>

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

api/envoy/api/v2/cds.proto

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

No need to pollute the already large Cluster definition when there's a perfectly appropriate message to hold the source transparency config! Signed-off-by: Kyle Larose <kyle@agilicus.com>

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

zuercher

This is looking good. At this point, the only thing left is resolving the complexity of the unit tests. My vote is to promote SocketOptionName to include and make it accessible so the test can just check the right options are set.

klarose · 2018-11-28T18:30:22Z

/retest

repokitteh-read-only · 2018-11-28T18:30:25Z

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

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

see: more, trace.

klarose · 2018-11-28T20:16:18Z

/retest

repokitteh-read-only · 2018-11-28T20:16:21Z

🔨 rebuilding ci/circleci: ipv6_tests (failed build)

🐱

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

see: more, trace.

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

We want to be able to introspect the options being passed around without needing to actually apply them. The simplest way to do this is to have them return the values they would set. Since the values may differ based on who they are visiting, this introspection takes into account the socket and state passed in. Signed-off-by: Kyle Larose <kyle@agilicus.com>

This changes some tests I added a while back for the source transparency socket options to use the new Option:getOptionInformation API. This is nicer since it decouples the higher level tests from the low level implementation of the socket API. Signed-off-by: Kyle Larose <kyle@agilicus.com>

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

Having some issues running some IPv6 option tests. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Ugh. Sucks to not be able to reproduce locally. Signed-off-by: Kyle Larose <kyle@agilicus.com>

zuercher

I think this looks good. Thanks for figuring out the testing!

include/envoy/network/listen_socket.h

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

zuercher · 2018-12-10T23:32:01Z

@mattklein123 or @htuch not sure about your availability this week, but I think this is ready for one of you to take a look

htuch · 2018-12-13T08:42:47Z

@zuercher will defer to @mattklein123 on this one, as I have limited bandwidth at present.

klarose · 2018-12-14T16:03:29Z

We may want to wait for the conversation in #5255 to be resolved before merging. We may end up changing the config model.

There is still some stuff in here I'd like -- namely, the socket option introspection. I can always do that in a separate PR if we decide to ditch this.

mattklein123 · 2018-12-16T05:58:00Z

We may want to wait for the conversation in #5255 to be resolved before merging. We may end up changing the config model.

I need to catch up on all of this. @klarose should I bother reviewing this now? Or just look at #5255 first?

klarose · 2018-12-16T14:07:06Z

@mattklein123 At least read the conversation we've been having in that PR. We've been discussing an alternative design. I'd like your feedback on it. TL;DR: try to use a filter rather than changing the core components.

klarose · 2018-12-20T13:22:08Z

Cancelling this as it has been subsumed by the work for another approach (#5337)

IP Source transparency involves non-local IP addresses being routed as though they were local. This requires some magic in the stack to ensure that those flows are sent back to envoy from the upstream host, rather than back to the original source IP address. We plan on using SO_MARK to do this. So, add it into the socket option factory. My intention is for it to be used by a follow-up PR to #5337. This was cherry-picked from PR #5035 where it was already reviewed. I plan on closing that PR. Risk Level: Low. No code invoked in production yet. Testing: Ran newly added UT and other UT in network. Docs Changes: None until we expose this through config. Release Notes: None Signed-off-by: Kyle Larose <kyle@agilicus.com>

IP Source transparency involves non-local IP addresses being routed as though they were local. This requires some magic in the stack to ensure that those flows are sent back to envoy from the upstream host, rather than back to the original source IP address. We plan on using SO_MARK to do this. So, add it into the socket option factory. My intention is for it to be used by a follow-up PR to envoyproxy#5337. This was cherry-picked from PR envoyproxy#5035 where it was already reviewed. I plan on closing that PR. Risk Level: Low. No code invoked in production yet. Testing: Ran newly added UT and other UT in network. Docs Changes: None until we expose this through config. Release Notes: None Signed-off-by: Kyle Larose <kyle@agilicus.com> Signed-off-by: Fred Douglas <fredlas@google.com>

klarose added 3 commits November 14, 2018 09:36

Add SO_MARK as a socket option

888ab7f

We can use this to control whether or not a packet is marked when emitted by Envoy. Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose commented Nov 14, 2018

View reviewed changes

api/envoy/api/v2/cds.proto Outdated Show resolved Hide resolved

klarose commented Nov 14, 2018

View reviewed changes

test/common/upstream/upstream_impl_test.cc Show resolved Hide resolved

upstream test: Check whether option are supported

08595b0

SO_MARK and IP_TRANSPARENT may not be supported on all platforms. Skip tests testing those if they aren't supported. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into add_transparency_options

3716573

mattklein123 assigned zuercher Nov 15, 2018

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Nov 22, 2018

zuercher reviewed Nov 26, 2018

View reviewed changes

klarose added 3 commits November 26, 2018 16:55

Merge branch 'master' into add_transparency_options

f6c7737

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

Code review fixes

fb2cb0a

- Removed json updates; json config is deprecated - Use scalar types for protobufs - Update new upstream_impl_tests to use yaml rather than json. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Merge branch 'master' into add_transparency_options

3f0c865

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

zuercher reviewed Nov 27, 2018

View reviewed changes

api/envoy/api/v2/cds.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/cds.proto Outdated Show resolved Hide resolved

klarose added 4 commits November 28, 2018 10:18

Merge branch 'master' into add_transparency_options

8df9d52

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

Move transparency config to UpstreamConnectionOptions

2c50e5e

No need to pollute the already large Cluster definition when there's a perfectly appropriate message to hold the source transparency config! Signed-off-by: Kyle Larose <kyle@agilicus.com>

Missed some comment fixes.

e5364a8

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

Fixup some unnecessary whitespace changes

ab4e27f

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

zuercher reviewed Nov 28, 2018

View reviewed changes

klarose added 2 commits December 6, 2018 08:33

Merge branch 'master' into add_transparency_options

0c5cc20

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

klarose added 4 commits December 6, 2018 14:49

Fix unused variable

e5f624d

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

Add back option supported checks

80bfed7

Having some issues running some IPv6 option tests. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Use correct option to check.

007b51d

Ugh. Sucks to not be able to reproduce locally. Signed-off-by: Kyle Larose <kyle@agilicus.com>

zuercher previously approved these changes Dec 8, 2018

View reviewed changes

include/envoy/network/listen_socket.h Outdated Show resolved Hide resolved

Rename Option::Information to Option::Details

6478f5a

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

klarose dismissed zuercher’s stale review via 6478f5a December 8, 2018 15:20

klarose mentioned this pull request Dec 10, 2018

http: add ip src transparency #5255

Closed

zuercher approved these changes Dec 10, 2018

View reviewed changes

zuercher assigned mattklein123 Dec 11, 2018

This was referenced Dec 19, 2018

[network] add socket option introspection #5351

Merged

[network] add so_mark as a socket option #5352

Merged

klarose closed this Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upstream: add transparency options #5035

upstream: add transparency options #5035

klarose commented Nov 14, 2018 •

edited

Loading

klarose commented Nov 14, 2018 •

edited

Loading

mattklein123 commented Nov 15, 2018

stale bot commented Nov 22, 2018

klarose commented Nov 22, 2018

zuercher left a comment

zuercher left a comment

klarose commented Nov 28, 2018

repokitteh-read-only bot commented Nov 28, 2018

klarose commented Nov 28, 2018

repokitteh-read-only bot commented Nov 28, 2018

zuercher left a comment

zuercher commented Dec 10, 2018

htuch commented Dec 13, 2018

klarose commented Dec 14, 2018

mattklein123 commented Dec 16, 2018

klarose commented Dec 16, 2018 •

edited

Loading

klarose commented Dec 20, 2018

upstream: add transparency options #5035

upstream: add transparency options #5035

Conversation

klarose commented Nov 14, 2018 • edited Loading

klarose commented Nov 14, 2018 • edited Loading

mattklein123 commented Nov 15, 2018

stale bot commented Nov 22, 2018

klarose commented Nov 22, 2018

zuercher left a comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

klarose commented Nov 28, 2018

repokitteh-read-only bot commented Nov 28, 2018

klarose commented Nov 28, 2018

repokitteh-read-only bot commented Nov 28, 2018

zuercher left a comment

Choose a reason for hiding this comment

zuercher commented Dec 10, 2018

htuch commented Dec 13, 2018

klarose commented Dec 14, 2018

mattklein123 commented Dec 16, 2018

klarose commented Dec 16, 2018 • edited Loading

klarose commented Dec 20, 2018

klarose commented Nov 14, 2018 •

edited

Loading

klarose commented Nov 14, 2018 •

edited

Loading

klarose commented Dec 16, 2018 •

edited

Loading