transport socket: Add proxy proto transport socket. by wez470 · Pull Request #11584 · envoyproxy/envoy

wez470 · 2020-06-14T05:46:28Z

Signed-off-by: Weston Carlson wez470@gmail.com

Commit Message: Add proxy proto transport socket

Additional Description: This is the part 1 PR described in #10682. It adds the transports socket / unit tests, a transport socket options struct for the proxy proto header, and does a refactor to make the listener filter use the common proxy proto constants (potentially want to move these now since the proxy proto config api type is not in extensions?)

Risk Level: Small
Testing: Unit
Docs Changes: None
Release Notes: None
Part Of: #1031

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 · 2020-06-15T16:13:25Z

Posting question I had in slack here. https://envoyproxy.slack.com/archives/C78HA81DH/p1592235285286200. Essentially wondering if there's a way for me to get docs passing with the current state or if I need to add the API / config factories, etc all in this PR.

ggreenway · 2020-06-15T19:04:56Z

source/extensions/transport_sockets/proxy_protocol/BUILD

+envoy_package()
+
+envoy_cc_extension(
+    name = "proxy_protocol",


Should this be named upstream_proxy_protocol or similar, so it is clear that it is only for use in an upstream context?

Yes, very good suggestion

ggreenway · 2020-06-15T19:08:34Z

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

+
+Network::IoResult ProxyProtocolSocket::doWrite(Buffer::Instance& buffer, bool end_stream) {
+  if (!generated_header_) {
+    generateHeader();


Can this be done in the constructor or onConnected() or somewhere earlier?

I've moved to onConnected for now. Do you know if that method gets called after setTransportSocketCallbacks?

actually, could probably just move the generation to setTransportSocketCallbacks

ggreenway · 2020-06-15T19:14:12Z

include/envoy/network/proxy_protocol.h

+namespace Envoy {
+namespace Network {
+
+struct ProxyProtocolHeader {


Rename ProxyProtocolOptions? When I think of header, I think of something in wire-format.

ggreenway · 2020-06-15T19:16:33Z

source/common/network/transport_socket_options_impl.h

+   * @returns TransportSocketOptionsSharedPtr a shared pointer to the transport socket options,
+   * nullptr if nothing is in the filter state or no PROXY protocol info is supplied.
+   */
+  static TransportSocketOptionsSharedPtr fromFilterStateWithProxyProtocolHeader(


Is this function needed? Couldn't the proxy protocol data be part of the FilterState?

I'll double check this 👍

Yeah, definitely can just add the proxy proto data to the FilterState.

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 · 2020-06-18T20:42:32Z

@cpakulski looked into the docs stuff a bit for me (thanks!). Essentially it seems like having an API is assumed. i.e. needing a proto with // [#extension: envoy.transport_sockets.upstream_proxy_protocol]. The generated security posture docs link to it.

Seems like I'm going to have to add the API, docs, TCP Proxy changes, e2e tests, etc all to this branch and just have one big PR. Not sure what the best course of action is. Maybe close this for now while I add all that stuff? It's definitely going to take me more than a few days to add it all.

Signed-off-by: Weston Carlson <wez470@gmail.com>

alyssawilk

Very cool to see this going out (and thank you for the manageable chunks! =P)
First pass thoughts below!

alyssawilk · 2020-06-22T14:47:38Z

include/envoy/network/proxy_protocol.h

+namespace Envoy {
+namespace Network {
+
+struct ProxyProtocolOptions {


optional rename, maybe ProxyProtocolData ?

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

alyssawilk · 2020-06-22T14:56:41Z

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h

+namespace TransportSockets {
+namespace ProxyProtocol {
+
+class ProxyProtocolSocket : public Network::TransportSocket,


totally optional, but what do you think of a base class that's a pure wrapper, and subclassing that to make it clear what's being overridden and what's passthrough.

Yeah, seems like a good idea. Could potentially be used for the tap socket as well. I'll look into it.

Signed-off-by: Weston Carlson <wez470@gmail.com>

repokitteh-read-only · 2020-07-04T21:01:47Z

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: #11584 was synchronize by wez470.

see: more, trace.

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 · 2020-07-13T19:44:38Z

hmm, now ssl integration tests are failing 😕

alyssawilk · 2020-07-13T20:52:08Z

@lizan I'm pretty good with this PR modulo CI failures. Would you take second pass as Greg is out?

source/common/network/transport_socket_options_impl.cc

Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Weston Carlson <wez470@gmail.com>

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 · 2020-07-21T01:21:21Z

FYI, CI is happy and this should be good to look at again

lizan

Can we add integration tests?

lizan · 2020-07-21T03:51:37Z

include/envoy/network/proxy_protocol.h

+namespace Network {
+
+struct ProxyProtocolData {
+  const Network::Address::InstanceConstSharedPtr& src_addr_;


This has a high chance to be dangling reference when it is passed around. I'm not sure why our tests didn't catch, do we have integration test for the whole setup?

No full setup integration tests at this point. The API (marked as unimplemented) / config factory / TCP Proxy / Doc / etc changes aren't in this PR to try and reduce scope. Those changes + more unit / integration tests would come in a follow up PR. Do you want to change this data type now or in the future PR? This won't be used at all right now.

I could also add a TODO to check / fix this for now?

At least make this non-reference?

Yes I can do that 👍

Signed-off-by: Weston Carlson <wez470@gmail.com>

lizan

@ggreenway ?

I think issues are resolved

wez470 · 2020-07-28T14:10:18Z

🎉. Thanks for the feedback, everyone!

Commit Message: Add proxy proto transport socket Additional Description: This is the part 1 PR described in envoyproxy#10682. It adds the transports socket / unit tests, a transport socket options struct for the proxy proto header, and does a refactor to make the listener filter use the common proxy proto constants (potentially want to move these now since the proxy proto config api type is not in extensions?) Risk Level: Small Testing: Unit Docs Changes: None Release Notes: None Part Of: #1031 Signed-off-by: Weston Carlson <wez470@gmail.com> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Commit Message: Add proxy proto transport socket Additional Description: This is the part 1 PR described in envoyproxy#10682. It adds the transports socket / unit tests, a transport socket options struct for the proxy proto header, and does a refactor to make the listener filter use the common proxy proto constants (potentially want to move these now since the proxy proto config api type is not in extensions?) Risk Level: Small Testing: Unit Docs Changes: None Release Notes: None Part Of: envoyproxy#1031 Signed-off-by: Weston Carlson <wez470@gmail.com> Co-authored-by: Lizan Zhou <lizan@tetrate.io>

Commit Message: Add proxy proto transport socket Additional Description: This is the part 1 PR described in envoyproxy#10682. It adds the transports socket / unit tests, a transport socket options struct for the proxy proto header, and does a refactor to make the listener filter use the common proxy proto constants (potentially want to move these now since the proxy proto config api type is not in extensions?) Risk Level: Small Testing: Unit Docs Changes: None Release Notes: None Part Of: envoyproxy#1031 Signed-off-by: Weston Carlson <wez470@gmail.com> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: chaoqinli <chaoqinli@google.com>

wez470 added 4 commits June 13, 2020 23:34

Add proxy proto transport socket.

c5fb7ff

Signed-off-by: Weston Carlson <wez470@gmail.com>

Add security posture.

7feb936

Signed-off-by: Weston Carlson <wez470@gmail.com>

Fix build file.

b34c37e

Signed-off-by: Weston Carlson <wez470@gmail.com>

Add tests for passthrough methods.

2095a6a

Signed-off-by: Weston Carlson <wez470@gmail.com>

htuch requested a review from ggreenway June 15, 2020 15:56

htuch assigned ggreenway Jun 15, 2020

ggreenway previously requested changes Jun 15, 2020

View reviewed changes

ggreenway assigned alyssawilk Jun 15, 2020

alyssawilk added the waiting label Jun 16, 2020

PR feedback

4f6e2d7

Signed-off-by: Weston Carlson <wez470@gmail.com>

repokitteh-read-only bot removed the waiting label Jun 17, 2020

Move header generation to onConnected.

49029ca

Signed-off-by: Weston Carlson <wez470@gmail.com>

wez470 added 4 commits June 20, 2020 13:09

Merge branch 'master' into proxy-proto-transport-socket

9c8d946

Signed-off-by: Weston Carlson <wez470@gmail.com>

Format code.

6bf44d3

Signed-off-by: Weston Carlson <wez470@gmail.com>

Fix issues from merge.

75cb061

Signed-off-by: Weston Carlson <wez470@gmail.com>

Format code.

285ead7

Signed-off-by: Weston Carlson <wez470@gmail.com>

alyssawilk reviewed Jun 22, 2020

View reviewed changes

wez470 added 4 commits June 28, 2020 15:03

Instantiate socket options properly in tests.

5860b44

Signed-off-by: Weston Carlson <wez470@gmail.com>

Merge branch 'master' into proxy-proto-transport-socket

01a21fb

Signed-off-by: Weston Carlson <wez470@gmail.com>

Add initial proxy proto socket API.

351f819

Signed-off-by: Weston Carlson <wez470@gmail.com>

Format protos.

3544fbb

Signed-off-by: Weston Carlson <wez470@gmail.com>

repokitteh-read-only bot added the api label Jul 4, 2020

wez470 added 5 commits July 4, 2020 19:32

Add socket config factory.

2f5848c

Signed-off-by: Weston Carlson <wez470@gmail.com>

Fix tests to use new class name.

8a9e7cc

Signed-off-by: Weston Carlson <wez470@gmail.com>

Add passthrough socket base class.

b23001c

Signed-off-by: Weston Carlson <wez470@gmail.com>

Use passthrough base class in tap socket.

c53f216

Signed-off-by: Weston Carlson <wez470@gmail.com>

Add passthrough socket unit tests.

637a1bd

Signed-off-by: Weston Carlson <wez470@gmail.com>

alyssawilk assigned lizan Jul 13, 2020

lizan reviewed Jul 13, 2020

View reviewed changes

source/common/network/transport_socket_options_impl.cc Outdated Show resolved Hide resolved

source/common/network/transport_socket_options_impl.cc Show resolved Hide resolved

wez470 and others added 6 commits July 18, 2020 13:33

Use default initialization.

b9ec60f

Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Weston Carlson <wez470@gmail.com>

Add proxy proto socket option test coverage.

8be7155

Signed-off-by: Weston Carlson <wez470@gmail.com>

Merge branch 'master' into proxy-proto-transport-socket

7b3d120

Signed-off-by: Weston Carlson <wez470@gmail.com>

Remove transport_socket member var from tap socket.

3eb7610

Signed-off-by: Weston Carlson <wez470@gmail.com>

Remove unused config factory.

e3aa964

Signed-off-by: Weston Carlson <wez470@gmail.com>

clang-tidy

19b5c3d

Signed-off-by: Weston Carlson <wez470@gmail.com>

lizan reviewed Jul 21, 2020

View reviewed changes

lizan added the waiting label Jul 21, 2020

lambdai mentioned this pull request Jul 21, 2020

mde: set additional remote port istio/proxy#2917

Closed

Make proxy proto data a non reference.

c4fe257

Signed-off-by: Weston Carlson <wez470@gmail.com>

repokitteh-read-only bot removed the waiting label Jul 24, 2020

Kick CI

f63d0a8

Signed-off-by: Weston Carlson <wez470@gmail.com>

lizan approved these changes Jul 25, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jul 25, 2020

alyssawilk approved these changes Jul 28, 2020

View reviewed changes

alyssawilk merged commit 8972b47 into envoyproxy:master Jul 28, 2020

wez470 mentioned this pull request Aug 31, 2020

transport sockets: expose proxy protocol socket #12762

Merged

Conversation

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

Uh oh!

Uh oh!

wez470 commented Jun 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wez470 commented Jun 18, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 4, 2020

Uh oh!

wez470 commented Jul 13, 2020

Uh oh!

alyssawilk commented Jul 13, 2020

Uh oh!

Uh oh!

Uh oh!

wez470 commented Jul 21, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wez470 Jul 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

wez470 commented Jul 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wez470 commented Jun 14, 2020 •

edited

Loading

wez470 Jul 21, 2020 •

edited

Loading