network: add transport socket interface by lizan · Pull Request #2116 · envoyproxy/envoy

lizan · 2017-11-28T09:45:33Z

network: add transport socket interface

Description:
Add transport socket interface and refactors out RawBufferSocket from Network::ConnectionImpl and SslSocket from Ssl::ConnectionImpl.

A step towards #1840, factory for register and instantiate transport sockets will be in next PRs.

API Changes:
envoyproxy/data-plane-api#218

Risk Level: Medium

Testing:
unit test and integration test.

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2017-11-29T18:08:07Z

This is ready to review, PTAL @mattklein123 @htuch. cc @PiotrSikora

alyssawilk

This looks great overall - the SSL logic came out really cleanly and I think the read/write semantics work better with a "socket" interface than a "crypto" interface.

I have a bunch of style nits but no major structural changes. Let's get this cleaned up and in! :-)

alyssawilk · 2017-11-30T19:07:08Z

include/envoy/network/transport_socket.h

+  virtual ~TransportSocketCallbacks() {}
+
+  /**
+   * @return the file descriptor associated with the connection.


We're not terribly consistent about it, but new Envoy style has the return type specified after @return
This should be applied to all @returns in this file

Also mild preference for Connection or Network::Connection where we're actually talking about a Network::Connection

alyssawilk · 2017-11-30T19:10:48Z

include/envoy/network/transport_socket.h

+  virtual Connection& connection() PURE;
+
+  /**
+   * @return return whether the read buffer should be drained


A bit more context on this would be nice now that it has moved away from where the code is using it.
You could maybe mention the example that this can be used to enforce yielding for configured read limits or some such

alyssawilk · 2017-11-30T19:13:51Z

include/envoy/network/transport_socket.h

+   * Raise a connection event to the connection.
+   * @param event supplies the connection event
+   */
+  virtual void raiseEvent(ConnectionEvent event) PURE;


If this is done on the connection, I'd lean towards exposing it there rather than having a wrapper function here. I guess since this is not accessible to all entities that have a connection it could be useful to scope it? Your call.

Either way you could doc up a bit more what this does. It basically fakes the connection getting the specific event from the kernel, no?

alyssawilk · 2017-11-30T19:15:26Z

include/envoy/network/transport_socket.h

+  virtual ~TransportSocket() {}
+
+  /**
+   * Called by connection once to intiialize the transport socket callbacks that the transport


alyssawilk · 2017-11-30T19:16:08Z

include/envoy/network/transport_socket.h

+
+  /**
+   * Closes the transport socket.
+   * @param event suuplies the connection event that closing the socket.


that -> that is?

alyssawilk · 2017-11-30T19:18:47Z

source/common/network/connection_impl.h

+                 Address::InstanceConstSharedPtr remote_address,
+                 Address::InstanceConstSharedPtr local_address,
+                 Address::InstanceConstSharedPtr bind_to_address,
+                 TransportSocketPtr transport_socket, bool using_original_dst, bool connected);


TransportSocketPtr&& transport_socket for ownership clarity?

alyssawilk · 2017-11-30T19:25:58Z

source/common/network/raw_buffer_socket.h

+  void setTransportSocketCallbacks(TransportSocketCallbacks& callbacks) override;
+  std::string protocol() const override;
+  bool canFlushClose() override { return true; }
+  void closeSocket(Network::ConnectionEvent) override {}


Doesn't this need to close the socket?

No, the raw buffer socket doesn't have to do anything other than the one does in ConnectionImpl.

alyssawilk · 2017-11-30T19:31:55Z

source/common/network/connection_impl.cc

      ENVOY_CONN_LOG(debug, "connected", *this);
      state_ &= ~InternalState::Connecting;
-      onConnected();
+      transport_socket_->onConnected();


This seems a little weird - the Network::Connection tells the socket it's connected, and then the socket calls raiseEvents back on the connection.

I suspect there's a good reason to pass through the socket but I'm missing it :-)

The main purpose of this is for SSL handshake

Connection calls TransportSocket::onConnected then transport socket starts the handshake.

transport socket call raiseEvents when handshake is complete.

Cool, that's a level of detail which might be worth documenting somewhere (the API file?) that onConnected is called on the transport when the underlying transport is connected, and raiseEvent(connected) is called when any transport-level handshakes are completed.

alyssawilk · 2017-11-30T19:40:41Z

source/common/ssl/connection_impl.h

+  ~ConnectionImpl();
+
+  // Network::Connection
+  Ssl::Connection* ssl() override { return dynamic_cast<SslSocket*>(transport_socket_.get()); }


Hm. If ssl() is a weird function on the base connection class (which returns nullptr) what do you think of making it an additional weird function on the transport socket? It'd let us avoid the dynamic cast, and I think do away with the ConnectionImpl entirely (as we could create the base connection class with a new SslSocket explicitly, and the regular connection class would call the null-returning ssl() for the raw socket.

Yes, I'm planning to remove the whole Ssl::ConnectionImpl at the end. The ssl() is kept here for compatibility with others, as it is used widely in filters, etc. That refactor will involve many classes, can be done in a separate PR.

Cool, let's TODO that cleanup just so it's clear what the plan is. I'm fine with a follow-up to keep this clean and simple

alyssawilk · 2017-11-30T19:44:06Z

include/envoy/network/transport_socket.h

+  virtual void raiseEvent(ConnectionEvent event) PURE;
+};
+
+class TransportSocket {


Class comments?

I think we could have a bit more detail. What is a transport socket vs a socket? We expect it's doing transformations on the data (TLS/LOAS) which might be worth calling out as part of the "transport" nature?

Signed-off-by: Lizan Zhou <zlizan@google.com>

alyssawilk

Sorry, just got out of a meeting so I don't have time for another full pass tonight. Looking better, here's what I had left so far!

alyssawilk · 2017-11-30T22:06:35Z

include/envoy/network/transport_socket.h


  /**
-   * @return return whether the read buffer should be drained
+   * @return whether the read buffer should be drained. This is used to enforce yielding for


@return bool

still missing types on a few others as well

alyssawilk · 2017-11-30T22:08:18Z

include/envoy/network/transport_socket.h

+  virtual void raiseEvent(ConnectionEvent event) PURE;
+};
+
+class TransportSocket {


I think we could have a bit more detail. What is a transport socket vs a socket? We expect it's doing transformations on the data (TLS/LOAS) which might be worth calling out as part of the "transport" nature?

alyssawilk · 2017-11-30T22:09:23Z

include/envoy/network/transport_socket.h

  /**
   * Closes the transport socket.
-   * @param event suuplies the connection event that closing the socket.
+   * @param event suuplies the connection event that is closing the socket.


Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2017-12-01T23:36:49Z

@alyssawilk ping.

mattklein123 · 2017-12-02T00:34:48Z

@lizan @alyssawilk is out on Fridays. I will take a look at this also this weekend.

mattklein123

At a high level this LGTM, other than my thinking out loud about FD, which I don't think we can do anything about easily right now.

@lizan I would prefer to hold this PR until we tag 1.5.0. So if possible can you just start working on your next PR on top of this one and we can get this merged later next week?

mattklein123 · 2017-12-02T18:25:34Z

include/envoy/network/transport_socket.h

+};
+
+/**
+ * Callbacks used by transport socket instances to communicate with connection


nit: period end of sentence.

mattklein123 · 2017-12-02T18:28:07Z

include/envoy/network/transport_socket.h

+  /**
+   * @return int the file descriptor associated with the connection.
+   */
+  virtual int fd() PURE;


Not all transport sockets are going to have a fd. Can this be removed from the interface somehow? (I haven't yet looked in detail at how the rest of the code is factored).

In now reading more of the PR, I guess this is due to the fact that fundamentally we are passing FDs around from listeners. I suppose at some point we may need to turn this into some type of generic handle but this is probably ok for now. Just thinking out loud.

Yeah I thought we need some type of generic handle but it is not easy and out of scope of this PR. We can refactor when we have something other than FDs.

I had the same concern but the SSL stack at least needs this.

We never found a good workaround in the GFE - just made -1 a permissible value which allows all sorts of bad feature creep :-(

Signed-off-by: Lizan Zhou <zlizan@google.com>

alyssawilk

@mattklein123 if we are waiting for 1.5 should this include release notes?

It's not a functional change, but it allows a feature we and others might care about so I assume it's worth doccing up.

Lizan, I'd suggest adding a few lines about the new API and what it allows to RAW_RELEASE_NOTES.md

alyssawilk · 2017-12-04T15:49:01Z

include/envoy/network/transport_socket.h

+  /**
+   * @return int the file descriptor associated with the connection.
+   */
+  virtual int fd() PURE;


I had the same concern but the SSL stack at least needs this.

We never found a good workaround in the GFE - just made -1 a permissible value which allows all sorts of bad feature creep :-(

alyssawilk · 2017-12-04T15:51:51Z

include/envoy/network/transport_socket.h

+  virtual std::string protocol() const PURE;
+
+  /**
+   * @return bool whether the socket can be flush closed.


"can be flushed and closed" since I'm not sure flush closed is a thing :-)

I'm kind of bummed we can't infer this from Connected. It'd work for TLS since we always raiseEvent Connected() right after the handshake is complete, but it might cause problems for TCP, as the socket isn't flagged connected until it processes the initial event. Matt, think this is worth fixing?

@lizan the question is for @mattklein123 but can you still update the comment in the interim?

mattklein123 · 2017-12-04T16:04:34Z

if we are waiting for 1.5 should this include release notes?

sorry I meant I would like to wait until after 1.5 is tagged. Either today or tomorrow. We can merge this later in the week? (For 1.6 cycle).

mattklein123 · 2017-12-04T17:07:14Z

Oops nevermind re: my release notes comment, we were saying the same thing. @alyssawilk yeah if we are waiting for 1.6.0 we should add a comment to raw release notes. Thank you!

lizan · 2017-12-04T21:52:35Z

Sure, let's merge this in 1.6 cycle, and I'll start the rest of work. Hopefully there will be no merge conflicts by then :)

Signed-off-by: Lizan Zhou <zlizan@google.com>

alyssawilk

Thanks for adding release notes :-)
LGTM pending that one last comment nit.

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123

LGTM. Nice job @lizan!

…to envoy logs. (envoyproxy#2116) * Added simple logging abstraction so mixer client logs can be relayed to envoy logs when running inside envoy, stderr when running standalone. * Log threshold guards that prevent needless serialization of logging arguments are now embedded in the log macros. * Format * Added do/while guards around logging statements.

Description: `TARGET_OS_MAC` is defined on iOS, but is not truth-y, so the previous checks were not sufficient. Risk Level: Low, re-enabling something that used to be enabled on iOS. Testing: Verified in our sample apps that WLAN/WWAN interfaces are properly logged when reachability changes. Docs Changes: Release Notes: [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>

lizan added 7 commits November 28, 2017 01:35

Move PostIoAction and IoResult to new header

e4642fe

Signed-off-by: Lizan Zhou <zlizan@google.com>

Pull out RawBufferSocket from ConnectionImpl

3488154

Signed-off-by: Lizan Zhou <zlizan@google.com>

Pull out SslSocket (integration test works)

f59be25

Signed-off-by: Lizan Zhou <zlizan@google.com>

Fix unit tests, add comments

1cc36e9

Signed-off-by: Lizan Zhou <zlizan@google.com>

fix format

de8cfca

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into transport_socket

3dda4e5

fix unit test merge conflict

f133571

Signed-off-by: Lizan Zhou <zlizan@google.com>

alyssawilk reviewed Nov 30, 2017

View reviewed changes

lizan added 2 commits November 30, 2017 13:48

address comments

f09ae3e

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into transport_socket

f1d6911

alyssawilk reviewed Nov 30, 2017

View reviewed changes

lizan added 2 commits November 30, 2017 14:35

address comments

a0545f1

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into transport_socket

9b56203

mattklein123 reviewed Dec 2, 2017

View reviewed changes

lizan added 2 commits December 2, 2017 16:40

fix comment

8676f5f

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into transport_socket

8d8fddd

alyssawilk reviewed Dec 4, 2017

View reviewed changes

lizan added 3 commits December 4, 2017 15:22

Merge remote-tracking branch 'upstream/master' into transport_socket

990275c

Add raw release notes

22c659a

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into transport_socket

dd7e6b1

alyssawilk reviewed Dec 5, 2017

View reviewed changes

lizan added 2 commits December 5, 2017 08:20

fix comment

7238121

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into transport_socket

7331555

alyssawilk approved these changes Dec 5, 2017

View reviewed changes

mattklein123 approved these changes Dec 5, 2017

View reviewed changes

mattklein123 merged commit 8d25070 into envoyproxy:master Dec 5, 2017

sbelair2 mentioned this pull request Sep 27, 2018

Refactor fd() callback in transport sockets to a generalized I/O handle. #4546

Closed

Conversation

lizan commented Nov 28, 2017

Uh oh!

lizan commented Nov 29, 2017

Uh oh!

alyssawilk left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan Nov 30, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

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!

lizan commented Dec 1, 2017

Uh oh!

mattklein123 commented Dec 2, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

lizan Nov 30, 2017 •

edited

Loading