DRAFT: Proxy Protocol Transport Socket #10682

wez470 · 2020-04-07T17:57:53Z

I opened this because I wanted to discuss / get some feedback on a few things. I am thinking I can split the rest of this work into 2 PRs (Maybe 3?). Contents would be as follows:

PR 1:

Transport socket class
Transport socket unit tests / test utils
DownstreamAddresses transport socket option struct
Refactor of listener filter to use common constants

PR 2:

Transport socket config / config factory
Transport socket API
Transport socket Integration tests (Not in this PR yet)
TCP Proxy API update for option to set down addrs socket option (Not in this PR yet)
TCP Proxy update to set downstream addr option (Just hacked in for now so I could test things out)
Any needed TCP Proxy integration / unit tests (Not in this PR yet)
Documentation (Not in this PR yet)

PR 3:

Performance pass?

This branch currently has PR 1 and some parts of what would be PR 2 in it. I'd just move the PR 1 pieces to a separate branch. Thoughts? I've also left a comment with a question on some code.

@alyssawilk @mattklein123

edit - Just noticed that v2 xDS is now frozen. So I'll have to update that.

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

repokitteh-read-only · 2020-04-07T17:57:58Z

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

🐱

Caused by: #10682 was opened by wez470.

see: more, trace.

wez470 · 2020-04-07T18:11:44Z

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

+  Buffer::OwnedImpl buff{};
+  Common::ProxyProtocol::generateV1Header(src_address, dst_address, src_port, dst_port, ip_version, buff);
+  auto res = buff.write(callbacks_->ioHandle());
+  if (!res.ok()) {


Looking at the raw buffer socket, it also checks if the res.err is an Api::IoError::IoErrorCode::Again, and returns KeepOpen if it is. I wasn't sure if when receiving the Again error, it was a possibility that the header was partially sent. Handling that would complicate the logic so for now I'm just returning Close on any error. Let me know what you think is the right thing to do here is.

@lizan, do you know? Should this do a similar thing to the raw buffer socket, or is it fine to Close on any error?

alyssawilk

I'm inclined to have @lizan lead first pass on this one as I think he's done more of the socket level API work, but here's some thoughts in passing :-)

api/envoy/config/transport_socket/proxy_protocol/v2/proxy_protocol.proto

include/envoy/network/transport_socket.h

alyssawilk · 2020-04-08T19:03:38Z

include/envoy/network/transport_socket.h

+   * @return the optional downstream address info. Can be used in upstream sockets that
+   *         need awareness of downstream info e.g. proxy_protocol.
+   */
+  virtual const absl::optional<DownstreamAddresses> downstreamAddresses() const PURE;


@envoyproxy/maintainers for future-proofing sanity check
We use our own custom proxy proto header equivalent in-house as we add some non-standard fields IMO are fairly generically useful. Do y'all know if anyone else does this? I already have some ideas of how we can do our custom stuff with stock Envoy but if we're not the only ones who play around this way I'd be inclined to do a review pass with generic extensibility in mind.

proxy protocol v2 has support to extensions. Can your internal version be mapped on this?

https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

If the length specified in the PROXY protocol header indicates that additional bytes are part of the header beyond the address information, a receiver may choose to skip over and ignore those bytes, or attempt to interpret those bytes. The information in those bytes will be arranged in Type-Length-Value (TLV vectors) in the following format. The first byte is the Type of the vector. The second two bytes represent the length in bytes of the value (not included the Type and Length bytes), and following the length field is the number of bytes specified by the length. struct pp2_tlv { uint8_t type; uint8_t length_hi; uint8_t length_lo; uint8_t value[0]; }; A receiver may choose to skip over and ignore the TLVs he is not interested in or he does not understand. Senders can generate the TLVs only for the information they choose to publish. The following types have already been registered for the <type> field : #define PP2_TYPE_ALPN 0x01 #define PP2_TYPE_AUTHORITY 0x02 #define PP2_TYPE_CRC32C 0x03 #define PP2_TYPE_NOOP 0x04 #define PP2_TYPE_UNIQUE_ID 0x05 #define PP2_TYPE_SSL 0x20 #define PP2_SUBTYPE_SSL_VERSION 0x21 #define PP2_SUBTYPE_SSL_CN 0x22 #define PP2_SUBTYPE_SSL_CIPHER 0x23 #define PP2_SUBTYPE_SSL_SIG_ALG 0x24 #define PP2_SUBTYPE_SSL_KEY_ALG 0x25 #define PP2_TYPE_NETNS 0x30

Possibly, but if other folks are likely to use the extension I think that answers my question - that should be good enough for the common case. Thanks!

alyssawilk · 2020-04-14T14:56:05Z

Actually can we do one more split, which is trying to land the APIs first?

I've got a WIP patch using your utilities (yay!) and I think I have a handle on API work now:
https://github.com/envoyproxy/envoy/compare/master...alyssawilk:shinkansen_next?expand=1

I think basically it'd be nice to have a standalone message for proxy proto config which has the version, and can optionally be extended later for extensions. Then in my PR for translating CONNECT I can just pull that whole proto in the ProxyConfig. Alternately I can land the API with my PR, but I figure you've done all the hard work, might as well get the git blame credit :-P

wez470 · 2020-04-15T03:00:03Z

Yeah, that sounds good to me. I'm fine if that change happens in your PR. If you need it right away, it's probably best. If it can wait a couple days, I'm happy to do it.

stale · 2020-04-23T04:07:11Z

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!

azure-pipelines · 2020-04-25T15:53:58Z

Command 'rerun' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

Description: This PR creates a common PROXY protocol config API message. It will be used for CONNECT work as well as in the transport socket for my upstream proxy proto work. This message could be extended to include TLVs in the future. Risk Level: Low Testing: None Docs Changes: None Release Notes: None Discussed in: #10682 (my draft PR to discuss the upstream implementation) Signed-off-by: Weston Carlson <[email protected]>

wez470 · 2020-05-09T23:26:07Z

Okay, thanks for the initial feedback here, everyone 🙂. Since there were no objections, I'm going to split out the first PR (as mentioned in the description).

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

stale · 2020-05-18T03:28:02Z

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!

wez470 · 2020-05-18T16:54:48Z

Closing as I'm just going to work on making PRs for the individual pieces

surki · 2020-06-01T13:18:58Z

@wez470 wondering if you have other PRs opened, if so please share them

wez470 · 2020-06-11T02:39:17Z

Hi, sorry for the delay. I plan to have PR 1 out this weekend.

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 <[email protected]> Co-authored-by: Lizan Zhou <[email protected]>

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 <[email protected]> Co-authored-by: Lizan Zhou <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>

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 <[email protected]> Co-authored-by: Lizan Zhou <[email protected]>

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 <[email protected]> Co-authored-by: Lizan Zhou <[email protected]> Signed-off-by: chaoqinli <[email protected]>

wez470 added 2 commits April 7, 2020 11:21

Add proxy proto transport socket.

1cd35fa

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

Move quic socket name to be alphabetical order.

b0dfb46

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

repokitteh-read-only bot added the api label Apr 7, 2020

wez470 commented Apr 7, 2020

View reviewed changes

mattklein123 assigned mattklein123, alyssawilk and lizan Apr 8, 2020

alyssawilk reviewed Apr 8, 2020

View reviewed changes

alyssawilk added the waiting label Apr 13, 2020

This was referenced Apr 19, 2020

Filter: Add PROXY Protocol Header Generation Upstream Filter #10177

Closed

Add proxy protocol config api message. #10845

Merged

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Apr 23, 2020

Merge branch 'master' into proxy-proto-all

bcee656

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

repokitteh-read-only bot removed the waiting label May 11, 2020

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

wez470 closed this May 18, 2020

wez470 mentioned this pull request Jun 14, 2020

transport socket: Add proxy proto transport socket. #11584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRAFT: Proxy Protocol Transport Socket #10682

DRAFT: Proxy Protocol Transport Socket #10682

wez470 commented Apr 7, 2020 •

edited

Loading

repokitteh-read-only bot commented Apr 7, 2020

wez470 Apr 7, 2020 •

edited

Loading

wez470 May 3, 2020

alyssawilk left a comment

alyssawilk Apr 8, 2020

ggreenway Apr 8, 2020

alyssawilk Apr 13, 2020

alyssawilk commented Apr 14, 2020

wez470 commented Apr 15, 2020

stale bot commented Apr 23, 2020

azure-pipelines bot commented Apr 25, 2020

wez470 commented May 9, 2020

stale bot commented May 18, 2020

wez470 commented May 18, 2020 •

edited

Loading

surki commented Jun 1, 2020

wez470 commented Jun 11, 2020 •

edited

Loading

DRAFT: Proxy Protocol Transport Socket #10682

DRAFT: Proxy Protocol Transport Socket #10682

Conversation

wez470 commented Apr 7, 2020 • edited Loading

repokitteh-read-only bot commented Apr 7, 2020

wez470 Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

wez470 May 3, 2020

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Apr 8, 2020

Choose a reason for hiding this comment

ggreenway Apr 8, 2020

Choose a reason for hiding this comment

alyssawilk Apr 13, 2020

Choose a reason for hiding this comment

alyssawilk commented Apr 14, 2020

wez470 commented Apr 15, 2020

stale bot commented Apr 23, 2020

azure-pipelines bot commented Apr 25, 2020

wez470 commented May 9, 2020

stale bot commented May 18, 2020

wez470 commented May 18, 2020 • edited Loading

surki commented Jun 1, 2020

wez470 commented Jun 11, 2020 • edited Loading

wez470 commented Apr 7, 2020 •

edited

Loading

wez470 Apr 7, 2020 •

edited

Loading

wez470 commented May 18, 2020 •

edited

Loading

wez470 commented Jun 11, 2020 •

edited

Loading