[filters] original src so_mark and ip_transparent by klarose · Pull Request #5525 · envoyproxy/envoy

klarose · 2019-01-07T20:56:06Z

Description:
This completes the primary functionality of the original src listener filter by adding SO_MARK and IP_TRANSPARENT options to the upstream connection. With these, any IP address can now connect upstream. This is one of the tasks mentioned in #5337

My original plan for the options was to have the OriginalSrcSocketOption recurse into them. However, I realized that this was somewhat sketchy when doing the UT -- there was not clean mapping between a socket option and multiple "sub-options". Rather than introduce such a comment, I simply leveraged the fact that we are already adding an option to the socket -- now we just add three instead of one!

Risk Level: Low. Existing filter; minor changes to code used in testing.
Testing: UT + some testing of ipv4 and ipv6 traffic using a container.
Docs Changes: See #5539.

We need to set SO_MARK and IP_TRANSPARENT. Do so. This needs some further work to UT it. Signed-off-by: Kyle Larose <kyle@agilicus.com>

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

Rather than relying on the original_src option encapsulating many options, just have the filter add multiple options. Simpler. Signed-off-by: Kyle Larose <kyle@agilicus.com>

If the platform (i.e. mac) doesn't support a given option, short circuit testing of it by just returning. Also fix a typo in a comment. Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose · 2019-01-08T13:20:16Z

/retest

repokitteh-read-only · 2019-01-08T13:20:19Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

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

see: more, trace.

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

This field didn't make sense. We take a different approach for testing options which don't have details that make sense. Signed-off-by: Kyle Larose <kyle@agilicus.com>

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

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

klarose · 2019-01-14T14:02:23Z

@lizan Thanks for the first pass. How is this looking now?

klarose · 2019-01-17T21:21:44Z

@lizan Any chance of looking at this in the next few days, please? Thanks. :)

lizan

LGTM modulo nits.

/wait

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

klarose · 2019-01-21T15:53:11Z

@lizan Looking good now?

mattklein123

Thanks 1 question.

mattklein123 · 2019-01-23T18:04:16Z

+  // Sets the SO_MARK option on the upstream connection's socket to the provided value. Used to
+  // ensure that non-local addresses may be routed back through envoy when binding to the original
+  // source address.
+  // [#proto-status: experimental]


I realize this is not this PR, but does a mark of 0 make sense? Shouldn't this be an optional uint32 WKT wrapper? If a mark of 0 means, no mark, and this is OK, can you document that?

A mark of 0 is equivalent to not marking it at all. I could document that for sure, though if I were to do that, I would probably want to make the mark option not set at all if it's zero. I can definitely do that as part of this PR. I'll update the docs while I'm at it too. Now that they're merged, it's not a big deal. :)

OK thank you. Either option is fine with me.

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

klarose · 2019-01-23T21:38:51Z

/retest

repokitteh-read-only · 2019-01-23T21:38:56Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

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

see: more, trace.

mattklein123

Thanks!

Signed-off-by: Kyle Larose <kyle@agilicus.com> Signed-off-by: Dan Zhang <danzh@google.com>

jrajahalme · 2019-01-25T12:57:19Z

@klarose Could you briefly elaborate how you use the SO_MARK set on the upstream connection to ensure the return traffic is routed back to Envoy? AFAIK only the outgoing packets will have the skb->mark set, return packets will not (unless marked by the destination in the same network namespace).

klarose · 2019-01-25T13:27:54Z

@jrajahalme TL;DR: conntrack

Basically, we save the mark in the conntrack record on egress from envoy. When return packets come back, we use conntrack to restore the mark, then force it through the a different routing table. You can see some example config here: https://www.envoyproxy.io/docs/envoy/latest/configuration/listener_filters/original_src_filter#extra-setup

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

Signed-off-by: Kyle Larose <kyle@agilicus.com> Signed-off-by: Fred Douglas <fredlas@google.com>

klarose added 5 commits January 7, 2019 15:16

Set socket options for original_src

d558ac1

We need to set SO_MARK and IP_TRANSPARENT. Do so. This needs some further work to UT it. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Update protobuf comment to show mark as implemented

e08782b

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

Add description_ to Socket::Option::Details

71030a3

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

Refactor sub-options and test

86c4c07

Rather than relying on the original_src option encapsulating many options, just have the filter add multiple options. Simpler. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Short circuit unsupported tests

065cf23

If the platform (i.e. mac) doesn't support a given option, short circuit testing of it by just returning. Also fix a typo in a comment. Signed-off-by: Kyle Larose <kyle@agilicus.com>

klarose changed the title ~~Original src extra options~~ [filters] original src extra options Jan 7, 2019

klarose changed the title ~~[filters] original src extra options~~ [filters] original src so_mark and ip_transparent Jan 7, 2019

klarose commented Jan 7, 2019

View reviewed changes

Comment thread include/envoy/network/listen_socket.h Outdated

mattklein123 assigned lizan Jan 9, 2019

lizan reviewed Jan 9, 2019

View reviewed changes

klarose added 4 commits January 10, 2019 08:27

Merge branch 'master' into original_src_extra_options

ea45cdb

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

Remove option details description field

ea21e76

This field didn't make sense. We take a different approach for testing options which don't have details that make sense. Signed-off-by: Kyle Larose <kyle@agilicus.com>

Code review nits

b20b0ac

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

Remove unnecessary conversions.

9317368

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

cmluciano mentioned this pull request Jan 10, 2019

Add ability to use non-local source IP for outgoing connections. #2659

Closed

lizan reviewed Jan 17, 2019

View reviewed changes

Comment thread source/extensions/filters/listener/original_src/original_src_socket_option.h

Comment thread source/extensions/filters/listener/original_src/original_src_socket_option.cc Outdated

klarose added 2 commits January 18, 2019 08:39

Merge branch 'master' into original_src_extra_options

b8088ce

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

Code review fixes

d3b5dd1

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

lizan previously approved these changes Jan 23, 2019

View reviewed changes

mattklein123 self-assigned this Jan 23, 2019

mattklein123 reviewed Jan 23, 2019

View reviewed changes

mattklein123 added the waiting label Jan 23, 2019

klarose added 2 commits January 23, 2019 14:43

Merge branch 'master' into original_src_extra_options

30332ec

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

Do not apply mark if 0

c12dad4

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

klarose dismissed lizan’s stale review via c12dad4 January 23, 2019 20:02

repokitteh-read-only Bot removed the waiting label Jan 23, 2019

mattklein123 approved these changes Jan 23, 2019

View reviewed changes

mattklein123 merged commit 2b9aef1 into envoyproxy:master Jan 23, 2019

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 24, 2019

[filters] original src so_mark and ip_transparent (envoyproxy#5525)

9849055

Signed-off-by: Kyle Larose <kyle@agilicus.com> Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 31, 2019

[filters] original src so_mark and ip_transparent (envoyproxy#5525)

4c2b415

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

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019

[filters] original src so_mark and ip_transparent (envoyproxy#5525)

d6cde3c

Signed-off-by: Kyle Larose <kyle@agilicus.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Conversation

klarose commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

klarose commented Jan 8, 2019

Uh oh!

repokitteh-read-only Bot commented Jan 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klarose commented Jan 14, 2019

Uh oh!

klarose commented Jan 17, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

klarose commented Jan 21, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

klarose Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

klarose commented Jan 23, 2019

Uh oh!

repokitteh-read-only Bot commented Jan 23, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jrajahalme commented Jan 25, 2019

Uh oh!

klarose commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

klarose commented Jan 7, 2019 •

edited

Loading

klarose commented Jan 25, 2019 •

edited

Loading