network: add DownstreamTransportSocketConfigFactory and registry by lizan · Pull Request #2437 · envoyproxy/envoy

lizan · 2018-01-23T22:15:31Z

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

Description:
Add DownstreamTransportSocketConfigFactory and its implementations.

Risk Level: Medium

Testing:
unit test, integration tests

Docs Changes:

Release Notes:

Fixes #1840.

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

lizan · 2018-01-23T22:16:30Z

Will add changes to listeners after #2346, PTAL on the interface @mattklein123 @alyssawilk cc/ @PiotrSikora

…gistry

alyssawilk

Looks good - only a few minor nits. Hurrah for (bidirectional) pluggable crypto!

alyssawilk · 2018-01-24T14:28:57Z

include/envoy/server/transport_socket_config.h

+};
+
+/**
+ * Implemented by each transport socket and registered via class RegisterFactory for upstream


Do we have a requirement that each transport socket needs both ends or should this be "implemented by each transport socket which does upstream connections"?

GFE has some things it speaks upstream and not downstream. With Envoy I have a hard time figuring out how we'd do testing without doing both so it's quite likely this comment should stand, maybe with an additional comment somewhere that it's required we always do both.

No such requirement. Though I expect most implementations will have both upstream and downstream, nothing blocks an implementation with upstream only or downstream only, the interfaces and configuration support that. I will update the comments.

Tests could be done with external process, in some cases it could be done with existing other implementations, e.g. a transport socket is using other TLS implementation than BoringSSL.

alyssawilk · 2018-01-24T14:30:38Z

include/envoy/server/transport_socket_config.h

+   * Create a particular downstream transport socket factory implementation.
+   * TODO(lizan): Revisit the interface when TLS sniffing and filter chain match are implemented.
+   * @param listener_name const std::string& the name of the listener.
+   * @param server_names const std::vector<std::string>& the names of the server.


Mind adding a bit more detail here?
I assumed it was for SNI, then didn't know how SNI worked if you have a vector of server names. I was going to dig through code but I think our API comments should have enough data us non-crypto folk can puzzle it out. If it's blatantly obvious to crypto folk that may be enough =P

alyssawilk · 2018-01-24T14:31:02Z

include/envoy/server/transport_socket_config.h

+   * TODO(lizan): Revisit the interface when TLS sniffing and filter chain match are implemented.
+   * @param listener_name const std::string& the name of the listener.
+   * @param server_names const std::vector<std::string>& the names of the server.
+   * @param skip_ssl_context_update bool indicates whether the ssl context update should be skipped.


…gistry

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

…gistry

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

lizan · 2018-01-29T23:25:33Z

@alyssawilk @zuercher This is ready to be reviewed, PTAL.

mattklein123

LGTM, some random small comments. Thanks!

mattklein123 · 2018-01-30T20:58:33Z

include/envoy/event/dispatcher.h

-                                                        Ssl::Context* ssl_ctx) PURE;
+  virtual Network::ConnectionPtr
+  createServerConnection(Network::ConnectionSocketPtr&& socket,
+                         Network::TransportSocketPtr&& transport_socket) PURE;


Please fix doc comments

mattklein123 · 2018-01-30T20:59:28Z

include/envoy/server/transport_socket_config.h

+};

+/**
+ * Implemented by each transport socket which does downstream connection. Registered via class


nit: "used for downstream connections."

mattklein123 · 2018-01-30T21:01:23Z

include/envoy/server/transport_socket_config.h

+};
+
+/**
+ * Implemented by each transport socket which does upstream connection. Registered via class


nit: "used for upstream connections."

mattklein123 · 2018-01-30T21:02:06Z

source/common/ssl/ssl_socket.h

+  bool implementsSecureTransport() const override;
+
+private:
+  ServerContextPtr ssl_ctx_;


mattklein123 · 2018-01-30T21:04:05Z

source/server/connection_handler_impl.cc

 }

 void ConnectionHandlerImpl::ActiveListener::newConnection(Network::ConnectionSocketPtr&& socket) {
+  auto ts = config_.transportSocketFactory().createTransportSocket();


nit: local var not needed

mattklein123 · 2018-01-30T21:11:10Z

source/server/listener_manager_impl.cc

  }

+  ASSERT(!transport_socket_factories_.empty());
+  ASSERT(transport_socket_factories_[0]);


nit: != nullptr

mattklein123 · 2018-01-30T21:12:42Z

source/server/listener_manager_impl.cc

+        Server::Configuration::DownstreamTransportSocketConfigFactory>(transport_socket.name());
+    ProtobufTypes::MessagePtr message =
+        Config::Utility::translateToFactoryConfig(transport_socket, config_factory);
+    transport_socket_factories_.emplace_back(config_factory.createTransportSocketFactory(


Given that right now we effectively only support a single filter chain, I would recommend not using a vector here and just storing a single filter chain. The check at the end verifies that all filter chains are the same if they are implemented. All of this is going to have to get refactored when we do real matching. I would also add some TODO comments about the coming refactoring.

We do support multiple filter chain, only with same filters, to support SNI. There are tests here:
https://github.com/envoyproxy/envoy/blob/master/test/server/listener_manager_impl_test.cc#L842

Since TransportSocketFactory replaces ServerContext here, it has to be vector.

If you always return the 0th element of the vector, what is the point of the vector?

Inside the SslServerContext, it switches the the context to others based on ClientHello here, unless skip_context_update is true. So now the default is being used until SslServerContext process client hello, and then switch the context after. Since SslContextManager doesn't own the SslServerContexts, the transport socket factories owns them here.

This behavior should be updated once SNI based filter chain match is implemented correctly, right @PiotrSikora?

Ohh. OK I see. Now that factory holds context, you need to store them all. Hmm, yeah this is pretty hacky/ugly but I get why you did it until filter chain matching is implemented. Can you add a bunch of comments? This is really unclear from reading. Thank you.

Sure, done.

mattklein123 · 2018-01-30T21:13:35Z

test/test_common/network_utility.h

                                                                     Address::SocketType type);

+/**
+ * Create a transport socket for testing purpse.


typo: "purpse" same below. Also should be "testing purposes"

mattklein123 · 2018-01-30T21:15:24Z

@lizan does this complete #1840? If so can you add "Fixes" to the description?

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

…gistry

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

mattklein123 · 2018-01-30T21:57:02Z

test/integration/fake_upstream.cc

  server_initialized_.waitReady();
+
+  if (ssl_ctx_) {
+    throw EnvoyException("debug");


oops, done.

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

zuercher · 2018-01-30T23:02:37Z

source/server/listener_manager_impl.cc

+
+    // Each transport socket factory owns one SslServerContext, we need to store them all in a
+    // vector since Ssl::ContextManager doesn't owns SslServerContext. While
+    // transportSocketFacotry() always return the first element of transport_socket_factories_,


nit: "transportSocketFactory()" and "returns the first element"

zuercher · 2018-01-30T23:02:52Z

source/server/listener_manager_impl.cc

+    // Each transport socket factory owns one SslServerContext, we need to store them all in a
+    // vector since Ssl::ContextManager doesn't owns SslServerContext. While
+    // transportSocketFacotry() always return the first element of transport_socket_factories_,
+    // other transport socket facotires are needed when the default Ssl::ServerContext updates


nit: factories

zuercher · 2018-01-30T23:04:15Z

include/envoy/network/listener.h


  /**
-   * @return Ssl::ServerContext* the default SSL context.
+   * @return TransportSocketFacotry& the transport socket factory.


nit: TransportSocketFactory

zuercher · 2018-01-30T23:07:17Z

include/envoy/server/transport_socket_config.h

+};
+
+/**
+ * Implemented by each transport socket which used for upstream connections. Registered via class


nit: rm which (repeated below for DownstreamTransportSocketConfigFactory)

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

…gistry

lizan · 2018-01-31T07:36:29Z

Hmm, I ran coverage locally and it says coverage is 98.0%, and I double checked every file I changed that didn't lose coverage. Not sure why CircleCI complain it is 97.9%... The artifacts wasn't uploaded.

mattklein123 · 2018-01-31T18:37:46Z

@lizan sorry. You can either find some coverage to add, or wait until we do some coverage fixes.

zuercher

one last question, but otherwise LGTM

zuercher · 2018-01-31T20:57:48Z

source/server/listener_manager_impl.cc

  }

+  ASSERT(!transport_socket_factories_.empty());
+  ASSERT(transport_socket_factories_[0] != nullptr);


Question: should this be asserting that the just-added pointer is not null?

yeah that works too.

alyssawilk

One more nit on my end as well

alyssawilk · 2018-01-29T20:45:18Z

source/server/listener_manager_impl.cc

-        has_stk++;
+
+    auto transport_socket = filter_chain.transport_socket();
+    if (!filter_chain.has_transport_socket()) {


Snagging transport_socket before checking has_transport_socket is weird and could definitely use some commenting. Would it instead make more sense to have string name from transport_socket and if name were empty to override it based on has_tls_config?

Added a comment (also to upstream_impl). Checking the name does same thing. proto here is validated, so the name won't be empty if filter chain has transport socket.

mattklein123

small nit, LGTM other than remaining @zuercher and @alyssawilk comments.

mattklein123 · 2018-01-31T21:38:03Z

source/server/listener_manager_impl.cc

+        Config::Utility::translateToFactoryConfig(transport_socket, config_factory);
+
+    // Each transport socket factory owns one SslServerContext, we need to store them all in a
+    // vector since Ssl::ContextManager doesn't owns SslServerContext. While


typo: "doesn't owns". While you are here please reflow comment.

…gistry

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

mattklein123

@lizan small nit since you have to do a master merge anyway. Thanks!

mattklein123 · 2018-01-31T23:59:57Z

source/server/listener_manager_impl.cc

+    ProtobufTypes::MessagePtr message =
+        Config::Utility::translateToFactoryConfig(transport_socket, config_factory);
+
+    // Each transport socket factory owns one SslServerContext, we need to store them all in a


nit: Can you please reflow this comment out to 100 cols? I think the sentences got broken up when the formatter kicked in.

…gistry

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

alyssawilk

@mattklein123 @zuercher any final comments?

We were on a very old release. This is also going to be needed to update to the latest bazel 6.x release which we need to do to update Envoy. Signed-off-by: JP Simard <jp@jpsim.com>

add DownstreamTransportSocketConfigFactory and registry

48950a0

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

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

f34d885

…gistry

alyssawilk reviewed Jan 24, 2018

View reviewed changes

alyssawilk self-assigned this Jan 24, 2018

lizan added 5 commits January 26, 2018 15:11

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

8c4e39c

…gistry

Add transportSocketFactory to listener

b78b836

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

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

e843dee

delete defaultSslContext

deb5486

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

fix comments

1abb513

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

lizan force-pushed the downstream_tsf_registry branch from 6db3bac to 1abb513 Compare January 29, 2018 20:28

alyssawilk assigned zuercher Jan 29, 2018

lizan added 2 commits January 29, 2018 14:37

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

d5127bb

…gistry

add comment for utility functions

b98f539

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

lizan changed the title ~~[WIP] network: add DownstreamTransportSocketConfigFactory and registry~~ network: add DownstreamTransportSocketConfigFactory and registry Jan 29, 2018

mattklein123 reviewed Jan 30, 2018

View reviewed changes

lizan added 4 commits January 30, 2018 13:31

address comments

06dba15

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

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

bafd37a

…gistry

fix comments

813b77e

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

align with api package refactor

b250972

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

mattklein123 reviewed Jan 30, 2018

View reviewed changes

lizan added 3 commits January 30, 2018 14:00

remove unused variables

364d4fc

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

remove unused upstream_ssl_ctx_

d958983

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

add comments for SNI explanation

dbeec06

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

zuercher reviewed Jan 30, 2018

View reviewed changes

lizan added 3 commits January 30, 2018 17:04

fix typos

981ba1e

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

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

9375259

…gistry

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

a14e609

…gistry

zuercher reviewed Jan 31, 2018

View reviewed changes

alyssawilk reviewed Jan 31, 2018

View reviewed changes

mattklein123 reviewed Jan 31, 2018

View reviewed changes

lizan added 2 commits January 31, 2018 13:44

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

99cabb0

…gistry

address comments

9468ac7

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

mattklein123 reviewed Feb 1, 2018

View reviewed changes

lizan added 2 commits January 31, 2018 16:17

Merge remote-tracking branch 'upstream/master' into downstream_tsf_re…

97a8926

…gistry

merge upstream, reflow comment

e870f04

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

alyssawilk approved these changes Feb 1, 2018

View reviewed changes

mattklein123 approved these changes Feb 1, 2018

View reviewed changes

zuercher approved these changes Feb 1, 2018

View reviewed changes

alyssawilk merged commit 605fa42 into envoyproxy:master Feb 1, 2018

Conversation

lizan commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Jan 23, 2018

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!

lizan commented Jan 29, 2018

Uh oh!

mattklein123 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!

mattklein123 commented Jan 30, 2018

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 commented Jan 31, 2018

Uh oh!

mattklein123 commented Jan 31, 2018

Uh oh!

zuercher 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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

lizan commented Jan 23, 2018 •

edited

Loading

lizan Jan 31, 2018 •

edited

Loading