transport sockets: expose proxy protocol socket by wez470 · Pull Request #12762 · envoyproxy/envoy

wez470 · 2020-08-20T22:10:21Z

Exposes proxy proto socket

Commit Message: Expose proxy protocol socket publicly.

Additional Description: Finishes off work for upstream proxy protocol header generation (possible performance pass still to come). Previous work so far includes #10548 (util funcs to generate header), #10845 (config api message), and #11584 (non-exposed transport socket). This PR hooks up tcp proxy, adds config factories to the socket, adds integration tests, and adds docs.

Risk Level: Medium

Testing: Unit + Integration + Functional

Docs Changes: IP transparency - Added to proxy protocol section to mention this socket for upstream generation, API docs
generated from proto

Release Notes: https://github.com/envoyproxy/envoy/pull/12762/files#diff-0ec5c857b4b6fbbb72b2ffc5275b15efR64

Fixes #1031

Signed-off-by: Weston Carlson <[email protected]>

repokitteh-read-only · 2020-08-20T22:10:26Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12762 was opened by wez470.

see: more, trace.

Signed-off-by: Weston Carlson <[email protected]>

alyssawilk · 2020-08-31T13:46:46Z

@antoniovicente would you be up for taking first pass?

antoniovicente · 2020-08-31T15:26:17Z

/assign @antoniovicente

Signed-off-by: Weston Carlson <[email protected]>

lizan · 2020-09-01T07:14:33Z

docs/root/intro/arch_overview/other_features/ip_transparency.rst

-:ref:`Original Src Listener Filter <arch_overview_ip_transparency_original_src_listener>`.
+:ref:`Original Src Listener Filter <arch_overview_ip_transparency_original_src_listener>`. Finally,
+Envoy supports generating this header using the :ref:`Proxy Protocol Transport Socket <extension_envoy.transport_sockets.upstream_proxy_protocol>`
+transport socket.


Can we have a full example config here?

Yeah, I was thinking about that, but was unsure where to add. We don't really have a designated socket spot for config examples. At one point I thought we could maybe add a sockets section here https://www.envoyproxy.io/docs/envoy/latest/configuration/configuration? wdyt? I'm fine with just adding in this file too.

The envoy.transport_sockets.tls example in docs/root/start/start.rst could be a source of inspiration.

antoniovicente

Thanks for implementing this. Looks pretty good. Sorry for the delays in code review.

source/extensions/transport_sockets/proxy_protocol/BUILD

test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc

source/extensions/transport_sockets/proxy_protocol/config.cc

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

rchernobelskiy · 2020-09-09T02:36:47Z

Thank you for adding this. Testing it locally works great 👍

wez470 · 2020-09-09T15:13:35Z

Thanks for trying it out! I actually was seeing some issues with health checks when I was going through functional tests again the other day. For some reason

envoy/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

Line 56 in 1df7242

auto src_addr = callbacks_->connection().localAddress();

was null. I'm trying to figure out what's going on there. This is also probably an indicator that we need an integration test to cover this.

Signed-off-by: Weston Carlson <[email protected]>

wez470 · 2020-09-22T05:25:48Z

@alyssawilk I think things are ready for another look 🙂

alyssawilk

Looking great! I think once we get the last tests sorted you're good to go.

alyssawilk · 2020-09-22T13:45:07Z

test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc

+}
+
+// Test header is sent unencrypted using a TLS inner socket
+TEST_P(ProxyProtocolIntegrationTest, TestTLSSocket) {


Try an -l trace and see where it hangs? Maybe it's hitting a timeout somewhere? If this is all raw TCP I think half close semantics apply, so if you close the client the connection may persist until the idle timeout kicks in.

if
ASSERT_TRUE(fake_upstream_connection->close());
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
tcp_client->waitForHalfClose();
tcp_client->close();
fixes, that's the problem, though I'm surprised there wouldn't be the same stall in other tests

alyssawilk · 2020-09-22T13:51:09Z

test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc

+        "envoy.transport_sockets.raw_buffer");
+  initialize();
+
+  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_));


again maybe -l trace or cerr logging? It may be easier to snag values from other health checking tests in tcp_proxy_integration_test.cc

Signed-off-by: Weston Carlson <[email protected]>

alyssawilk · 2020-09-23T13:17:49Z

LGTM modulo clang tidy fix :-)

Signed-off-by: Weston Carlson <[email protected]>

alyssawilk · 2020-09-23T17:42:50Z

@lizan can you take a look both as secondary reviewer and API shepherd?

stale · 2020-10-04T09:16:17Z

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!

alyssawilk · 2020-10-05T16:31:54Z

@lizan ping? Alternately @mattklein123 could do second and Api pass?

mattklein123 · 2020-10-05T17:46:29Z

I can take a look.

mattklein123

Exciting to see this land. One small question. Also, can you merge main just to make sure we aren't too out of date? Thanks.

/wait-any

mattklein123 · 2020-10-05T22:43:11Z

source/common/tcp_proxy/tcp_proxy.cc

+    if (!read_callbacks_->connection()
+             .streamInfo()
+             .filterState()
+             ->hasData<Network::ProxyProtocolFilterState>(
+                 Network::ProxyProtocolFilterState::key())) {
+      read_callbacks_->connection().streamInfo().filterState()->setData(
+          Network::ProxyProtocolFilterState::key(),
+          std::make_unique<Network::ProxyProtocolFilterState>(Network::ProxyProtocolData{
+              downstreamConnection()->remoteAddress(), downstreamConnection()->localAddress()}),
+          StreamInfo::FilterState::StateType::ReadOnly,
+          StreamInfo::FilterState::LifeSpan::Connection);
+    }


It seems odd to me that we jam this data into filter state only to read it back in the next function call. What is the use case for nested states here? Could we just pass the addresses directly into the fromFilterState() function?

It was like this at one point and changed from feedback #11584 (comment). I'm fine either way

If this has already been discussed it's fine with me. We can revisit later if needed.

Signed-off-by: Weston Carlson <[email protected]>

wez470 · 2020-10-06T19:51:12Z

Thanks all! 🎉

rchernobelskiy · 2020-10-20T04:00:46Z

Doing some more testing on this, there is a common case where it's not behaving as expected from what I can tell.
The first few requests always work correctly for me, or if I never change downstream/upstream IPs everything works correctly also.
But when changing the downstream remote IP, the remote IP inserted in the proxyproto header is randomly reused from previous connections.
Logs (%DOWNSTREAM_REMOTE_ADDRESS%) always shows the correct address though.

Rough steps to reproduce:
Have envoy listen on 127.0.0.1:9000 and tcp proxy to a cluster that listens for connections, reads the proxy protocol header, and then promptly closes the connection.

ip route add local 252.0.0.0/8 dev lo
# run this about 20 times:
curl --interface 252.2.2.0 127.0.0.1:9000
# then run this about 20 times
curl --interface 252.3.3.0 127.0.0.1:9000
# then run this about 20 times
curl --interface 252.4.4.0 127.0.0.1:9000

The expectation here and what the logs show is 20 connections each from the 3 separate addresses above. What the proxy protocol sends, ends up being a random mix.

Any ideas?
I tried setting connection_pool_per_downstream_connection: true, with no luck unfortunately.

rchernobelskiy · 2020-10-20T05:26:18Z

I wonder if this comment, suggesting existence of connection reuse scenarios, is relevant:
#4684 (comment)

Edit: filed this issue here: #13659

alyssawilk · 2020-10-26T17:04:11Z

Sadly it's not that simple but I've confirmed there's definitely a bug here. I'll pick up fixing.

wez470 added 5 commits August 2, 2020 19:09

Add proxy proto tcp proxy changes. Add config factory.

fc67cf5

Signed-off-by: Weston Carlson <[email protected]>

Fix format.

2323504

Signed-off-by: Weston Carlson <[email protected]>

Merge remote-tracking branch 'upstream/master'

f813691

Signed-off-by: Weston Carlson <[email protected]>

Add initial proxy proto integration tests.

0ee7c45

Signed-off-by: Weston Carlson <[email protected]>

Fix format.

71ba92d

Signed-off-by: Weston Carlson <[email protected]>

repokitteh-read-only bot added the api label Aug 20, 2020

wez470 added 3 commits August 20, 2020 20:01

Fix format.

1291529

Signed-off-by: Weston Carlson <[email protected]>

Fix test.

6c91f31

Signed-off-by: Weston Carlson <[email protected]>

Fix proto doc comment.

4825f05

Signed-off-by: Weston Carlson <[email protected]>

mattklein123 assigned ggreenway Aug 22, 2020

ggreenway assigned lizan and alyssawilk Aug 24, 2020

wez470 added 7 commits August 30, 2020 15:27

Only add proxy proto filter state data if it doesn't exist.

473cbbe

Signed-off-by: Weston Carlson <[email protected]>

Fix format.

00775ba

Signed-off-by: Weston Carlson <[email protected]>

Fix generated shadow api.

bf270c5

Signed-off-by: Weston Carlson <[email protected]>

Add ip transparency docs.

07a1022

Signed-off-by: Weston Carlson <[email protected]>

Remove regex test matcher that failed on windows.

7ebf550

Signed-off-by: Weston Carlson <[email protected]>

Remove whitespace.

bd93847

Signed-off-by: Weston Carlson <[email protected]>

Fix docs sentence.

f89950a

Signed-off-by: Weston Carlson <[email protected]>

wez470 marked this pull request as ready for review August 31, 2020 02:37

wez470 requested a review from alyssawilk as a code owner August 31, 2020 02:37

repokitteh-read-only bot assigned antoniovicente Aug 31, 2020

Merge remote-tracking branch 'upstream/master'

1df7242

Signed-off-by: Weston Carlson <[email protected]>

lizan reviewed Sep 1, 2020

View reviewed changes

antoniovicente reviewed Sep 3, 2020

View reviewed changes

Rename test.

3542aaa

Signed-off-by: Weston Carlson <[email protected]>

alyssawilk reviewed Sep 22, 2020

View reviewed changes

wez470 added 2 commits September 22, 2020 23:43

Verify header in health check test.

df53fe7

Signed-off-by: Weston Carlson <[email protected]>

Remove uneeded initializer.

defb718

Signed-off-by: Weston Carlson <[email protected]>

alyssawilk previously approved these changes Sep 23, 2020

View reviewed changes

Clang tidy proxy protocol tests.

55d11b3

Signed-off-by: Weston Carlson <[email protected]>

wez470 dismissed alyssawilk’s stale review via 55d11b3 September 23, 2020 14:35

alyssawilk approved these changes Sep 23, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 4, 2020

mattklein123 self-assigned this Oct 5, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 5, 2020

mattklein123 reviewed Oct 5, 2020

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Oct 5, 2020

Merge remote-tracking branch 'upstream/master'

4d9f748

Signed-off-by: Weston Carlson <[email protected]>

mattklein123 approved these changes Oct 6, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Oct 6, 2020

mattklein123 merged commit b18f57e into envoyproxy:master Oct 6, 2020

bgagnon mentioned this pull request Oct 26, 2020

Add upstream support for PROXYv2 projectcontour/contour#902

Closed

2 tasks

Conversation

wez470 commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 20, 2020

Uh oh!

alyssawilk commented Aug 31, 2020

Uh oh!

antoniovicente commented Aug 31, 2020

Uh oh!

lizan Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

wez470 Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wez470 Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

antoniovicente Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rchernobelskiy commented Sep 9, 2020

Uh oh!

wez470 commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wez470 commented Sep 22, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

alyssawilk Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Sep 23, 2020

Uh oh!

alyssawilk commented Sep 23, 2020

Uh oh!

stale bot commented Oct 4, 2020

Uh oh!

alyssawilk commented Oct 5, 2020

Uh oh!

mattklein123 commented Oct 5, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

wez470 Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

wez470 commented Oct 6, 2020

Uh oh!

rchernobelskiy commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchernobelskiy commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Oct 26, 2020

Uh oh!

Reviewers

wez470 commented Aug 20, 2020 •

edited

Loading

wez470 Sep 1, 2020 •

edited

Loading

wez470 commented Sep 9, 2020 •

edited

Loading

wez470 Oct 6, 2020 •

edited

Loading

rchernobelskiy commented Oct 20, 2020 •

edited

Loading

rchernobelskiy commented Oct 20, 2020 •

edited

Loading