quiche: implement QUIC specific TransportSocketFactory for TLS context by danzh2010 · Pull Request #8091 · envoyproxy/envoy

danzh2010 · 2019-08-29T20:06:21Z

Implement TransportSocketFactory for QUIC which carries Ssl::ContextConfig. The Ssl::ContextConfig instance is created based on envoy::api::v2::auth::DownstreamTlsContext in envoy::api::v2::listener::FilterChain::transport_socket.

QuicServer|ClientTransportSocketFactory has new interface server|clientContextConfig() to provide access toTLS context in addition to inherited TransportSocketFactory interface. QuicTransportSocketFactory::createTransportSocket() shouldn't be called in current implementation because QUICHE stack already does all the transport layer work.

When listener config socket type is Datagram and udp_listener_config.udp_listener_name is "quiche_quic_listener", envoy::api::v2::listener::FilterChain::transport_socket has to be configured with envoy::api::v2::auth::DownstreamTlsContext or envoy::api::v2::auth::UpstreamTlsContext.

ListenerImpl() will create a QuicTransportSocketFactory instance as part of filter chain in the same way as creating other transport socket factories.

Risk Level: medium, a bad config can hit RELEASE_ASSERT.
Testing: added unit tests for ListenerManagerImpl
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-08-29T20:25:44Z

/assign @alyssawilk

Signed-off-by: Dan Zhang <danzh@google.com>

lambdai

It seems the quic filter chain is quite different...

lambdai · 2019-08-30T04:19:03Z

source/server/listener_manager_impl.cc

-    } else {
-      transport_socket.set_name(
-          Extensions::TransportSockets::TransportSocketNames::get().RawBuffer);
+  std::vector<std::string> server_names(filter_chain.filter_chain_match().server_names().begin(),


nit: move into if (!is_quic_)

lambdai · 2019-08-30T04:23:32Z

source/server/listener_manager_impl.h

  ListenerFilterChainFactoryBuilder(
-      ListenerImpl& listener, Configuration::TransportSocketFactoryContextImpl& factory_context);
+      ListenerImpl& listener, Configuration::TransportSocketFactoryContextImpl& factory_context,
+      bool is_quic);


My impression is that TCPListenerFilterChainFactoryBuilder and QUIC one has nothing shared... Why not derived QUIC one from FilterChainFactoryBuilder?

both of them access ListenerImpl and TransportSocketFactoryContextImpl. I made QuicListenerFilterChainFactoryBuilder a stand alone class deriving from ListenerFilterChainFactoryBuilder.

lambdai · 2019-08-30T04:42:48Z

source/server/listener_manager_impl.cc

    addListenSocketOptions(Network::SocketOptionFactory::buildRxQueueOverFlowOptions());
    std::string listener_name =
        config.has_udp_listener_config() ? config.udp_listener_config().udp_listener_name() : "";
+    is_quic = listener_name == UdpListenerNames::get().Quic;


nit: is_quic = config.has_udp_listener_config() && config.udp_listener_config().udp_listener_name() == UdpListenerNames::get().Quic;

Also you can create const ref of udp_listener_config here and declare listener_name const

const auto & udp_config = config.has_udp_listener_config() ? config.udp_listener_config() : envoy::api::v2::listener::UdpListenerConfig();
const auto listener_name = udp_config.udp_listener_name().empty() ? udp_config.udp_listener_name() : "";

The idea is to figure out the final listener_name at one place

lambdai · 2019-08-30T04:57:24Z

source/server/filter_chain_manager_impl.h

+  // Network::FilterChain
+  const Network::TransportSocketFactory* transportSocketFactory() const override { return nullptr; }
+
+  Ssl::ContextConfig* tlsContextConfig() const { return tls_context_config_.get(); }


Where is this one used?

not used yet. Will be used to create EnvoyQuicProofSource which is a fake one now.

Can this return a const reference or just a reference?

mattklein123 · 2019-09-01T19:34:52Z

@danzh2010 friendly request to please update the PR description to something more descriptive. I will take a look when the review is a bit further along. Thank you!

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-09-05T15:07:30Z

/retest

repokitteh-read-only · 2019-09-05T15:07:34Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #8091 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-09-09T18:47:34Z

I updated PR description. It's good for another around of review.

lambdai

Thank you for the update!

danzh2010 · 2019-09-09T22:04:51Z

/assign @goaway

alyssawilk · 2019-09-10T14:58:09Z

include/envoy/network/filter.h

+   * @return a transport socket factory to be used by the new connection.
+   *         nullptr if the connection doesn't need a transport socket.
   */
-  virtual const TransportSocketFactory& transportSocketFactory() const PURE;


I think envoy standard might be using optional, but I'm a fan of pointers so we'll see what Matt says ;-)

an optional reference wrapper is "nicer" but I'm fine either way.

alyssawilk · 2019-09-10T14:58:58Z

source/server/connection_handler_impl.cc

    ASSERT(socket_type == Network::Address::SocketType::Datagram,
           "Only datagram/stream listener supported");
+    RELEASE_ASSERT(config.udpListenerFactory() != nullptr,
+                   "UDP listner factory is not initialized.");


Can you add a TODO to make this a proper config check?

This won't be a config error, we will create UdpListenerFactory anyway in ListenerImpl(). If not specified in config, we create the raw UDP listener factory.

I think you can remove this assert then? It would crash in an obvious way on the next line.

I think you didn't remove it, just converted it. Would it actually fail under createActiveUdpListener or is this assert required?

ListenerConfig.udpListenerFactory() is populated to default RawUdp listener if not specified in config. So this ASSERT is to check we actually have logic to do this before reaching here. Otherwise below config.udpListenerFactory()->createActiveUdpListener(*this, dispatcher_, logger_, config); would crash because of accessing members of nullptr.

Gotcha. If we plan to keep it in, let's at least change
listner -> listener

alyssawilk · 2019-09-10T15:00:04Z

source/common/network/address_impl.cc

  if (result.rc_ >= 0) {
-    RELEASE_ASSERT(os_sys_calls.close(result.rc_).rc_ == 0, "");
+    RELEASE_ASSERT(os_sys_calls.close(result.rc_).rc_ == 0,
+                   absl::StrCat("fail to close fd ", result.rc_));


Thanks for adding details here.

maybe -> "failed to close fd: response code " ?
since right now it looks like you're printing the fd, not the error code.

alyssawilk · 2019-09-10T15:07:48Z

source/server/listener_manager_impl.cc

+      parent_.server_.random(), parent_.server_.stats(), parent_.server_.singletonManager(),
+      parent_.server_.threadLocal(), validation_visitor, parent_.server_.api());
+  factory_context.setInitManager(initManager());
+  ListenerFilterChainFactoryBuilderPtr builder =


if I'm not lost in braces, we'll now do work for raw UDP that we didn't used to. Is that correct, and is it intentional?

Yes, we are initializing network filters for raw UDP too. I think it doesn't hurt as long as they don't config any network filters but leave them unused.

mattklein123

Flushing out a few questions/comments from a first pass. Thank you!

/wait

mattklein123 · 2019-09-10T21:46:52Z

include/envoy/network/filter.h

+   * @return a transport socket factory to be used by the new connection.
+   *         nullptr if the connection doesn't need a transport socket.
   */
-  virtual const TransportSocketFactory& transportSocketFactory() const PURE;


an optional reference wrapper is "nicer" but I'm fine either way.

mattklein123 · 2019-09-10T21:48:14Z

include/envoy/ssl/context_config.h

+
+  virtual ContextConfigPtr
+  createSslContextConfig(const Protobuf::Message& config,
+                         Server::Configuration::TransportSocketFactoryContext& context) PURE;


It's a little strange to create a transport socket factory context when making an SSL context. Should we just define whatever context we need in this file and then have TransportSocketFactoryContext derive from that if needed?

mattklein123 · 2019-09-10T21:49:59Z

source/extensions/transport_sockets/tls/downstream_context_config_factory.h

+namespace Tls {
+
+/**
+ * A factory to create server side Tls context config from a protobuf.


nit: Tls/TLS here and elsewhere in comments.

mattklein123 · 2019-09-10T21:50:51Z

source/extensions/transport_sockets/tls/downstream_context_config_factory.cc

+    const Protobuf::Message& config,
+    Server::Configuration::TransportSocketFactoryContext& context) {
+  return std::make_unique<ServerContextConfigImpl>(
+      dynamic_cast<const envoy::api::v2::auth::DownstreamTlsContext&>(config), context);


If we universally do a dynamic_cast here why not just pass this type as a parameter?

createSslContextConfig() is public interface which will also be used on client side which should downcast message to UpstreamTlsContext.

@lizan any thoughts on this one? I would need to sit down and page in all of this code but something seems off here. Maybe this is related to your other comment.

at interface level this seems fine, though at higher level I think this should just be a TransportSocketConfigFactory

mattklein123 · 2019-09-10T21:51:27Z

source/server/connection_handler_impl.cc

    ASSERT(socket_type == Network::Address::SocketType::Datagram,
           "Only datagram/stream listener supported");
+    RELEASE_ASSERT(config.udpListenerFactory() != nullptr,
+                   "UDP listner factory is not initialized.");


Can you add a TODO to make this a proper config check?

mattklein123 · 2019-09-10T21:52:34Z

source/server/filter_chain_manager_impl.h

+  // Network::FilterChain
+  const Network::TransportSocketFactory* transportSocketFactory() const override { return nullptr; }
+
+  Ssl::ContextConfig* tlsContextConfig() const { return tls_context_config_.get(); }


Can this return a const reference or just a reference?

mattklein123 · 2019-09-10T21:53:25Z

source/server/listener_manager_impl.cc

+              .createActiveUdpListenerFactory(config.has_udp_listener_config()
+                                                  ? config.udp_listener_config()
+                                                  : envoy::api::v2::listener::UdpListenerConfig());
+    } catch (EnvoyException ex) {


const EnvoyException&

mattklein123 · 2019-09-10T21:54:16Z

source/server/listener_manager_impl.cc

+                                                  ? config.udp_listener_config()
+                                                  : envoy::api::v2::listener::UdpListenerConfig());
+    } catch (EnvoyException ex) {
+      // TODO(danzh): add implementation for quic_listener and do not catch


I'm confused. Why do we need/want this fallback?

Just because we don't have QuicListenerFactoryConfig registered yet. Once PR #7896 gets in, this is not needed.

Can we just assert this then or wait until the other one is merged?

I'll wait for the other PR merge first and change this line. It is as such now to make listener mgr test pass in a meaningful way.

mattklein123 · 2019-09-10T21:55:21Z

source/server/listener_manager_impl.cc

+    const ::envoy::api::v2::listener::FilterChain& filter_chain) const {
+  // Initialize filter chain for QUIC.
+  if (!filter_chain.has_tls_context()) {
+    return nullptr;


Should this be a config error?

Yes. Is it okay to lead to connection close? Or what's the best way of handling such error?

We should check this during config load and fail the config, it should be an assert at this point IMO.

With removal of QuicFilterChain, this builder can be unified with tcp one too. This check no longer apply.

mattklein123 · 2019-09-10T21:56:55Z

source/server/listener_manager_impl.cc

+    return nullptr;
+  }
+  auto& tls_context_factory =
+      Config::Utility::getAndCheckFactory<Ssl::ContextConfigFactory>("downstream_tls_context");


"downstream_tls_context" should be a constant somewhere.

Can you talk a bit more about why we need to load the factory on this way vs. just directly instantiating it? It's a little confusing. I suppose this is due to pluggable TLS implementations?

Yes, all the registered class work in this PR is just to make envoy core code not depends on TLS extensions.

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk · 2019-09-16T16:00:22Z

include/envoy/network/connection_handler.h

  createActiveUdpListener(ConnectionHandler& parent, Event::Dispatcher& disptacher,
                          spdlog::logger& logger, Network::ListenerConfig& config) const PURE;
+
+  // @return true if the protocol above UDP doesn't form a connection.


super nitty but /** .. */ style comments
Also maybe
"if the protocol above UDP doesn't form a connection." -> "If the UDP passing through listener doesn't form stateful connections" ?

might even be nicer to invert and have isStatefulConnection() but maybe that's just me.

alyssawilk · 2019-09-16T16:01:26Z

source/server/connection_handler_impl.cc

    ASSERT(socket_type == Network::Address::SocketType::Datagram,
           "Only datagram/stream listener supported");
+    RELEASE_ASSERT(config.udpListenerFactory() != nullptr,
+                   "UDP listner factory is not initialized.");


I think you didn't remove it, just converted it. Would it actually fail under createActiveUdpListener or is this assert required?

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 · 2019-09-16T18:41:10Z

Let's merge master and then we can get back to reviewing this one.

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-09-16T21:00:05Z

Let's merge master and then we can get back to reviewing this one.

/wait

Done. PTAL

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

Looking good. just one nit left on my end.

alyssawilk · 2019-09-17T15:18:05Z

source/server/connection_handler_impl.cc

    ASSERT(socket_type == Network::Address::SocketType::Datagram,
           "Only datagram/stream listener supported");
+    RELEASE_ASSERT(config.udpListenerFactory() != nullptr,
+                   "UDP listner factory is not initialized.");


Gotcha. If we plan to keep it in, let's at least change
listner -> listener

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Thanks, LGTM with some small comments.

/wait

mattklein123 · 2019-09-17T17:37:10Z

source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h

+// server and client.
+class QuicTransportSocketFactoryBase : public Network::TransportSocketFactory {
+public:
+  ~QuicTransportSocketFactoryBase() override = default;


nit: delete

mattklein123 · 2019-09-17T17:37:30Z

source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h

+  Ssl::ServerContextConfig& serverContextConfig() const { return *config_; }
+
+private:
+  Ssl::ServerContextConfigPtr config_;


nit: const here and below

mattklein123 · 2019-09-17T17:37:45Z

source/extensions/quic_listeners/quiche/quic_transport_socket_factory.h

+class QuicTransportSocketConfigFactory
+    : public virtual Server::Configuration::TransportSocketConfigFactory {
+public:
+  ~QuicTransportSocketConfigFactory() override = default;


nit: delete

mattklein123 · 2019-09-17T17:38:35Z

source/server/connection_handler_impl.cc

  } else {
-    ASSERT(socket_type == Network::Address::SocketType::Datagram,
-           "Only datagram/stream listener supported");
+    RELEASE_ASSERT(config.udpListenerFactory() != nullptr,


The previous conversation was lost, but this should never happen due to being checked via config elsewhere, right? If so can we make this a normal ASSERT?

Early on ListenerImpl() will always create some sort of ActiveUdpListener, if not configured, ActiveRawUdpListener will be created. So yes here ASSERT is sufficient.

mattklein123 · 2019-09-17T17:40:27Z

source/server/listener_manager_impl.cc

+  } else if (udp_listener_factory_ != nullptr &&
+             !udp_listener_factory_->isTransportConnectionless()) {
+    for (auto& filter_chain : config.filter_chains()) {
+      // Early fail if any filter chain doesn't have transport socket configured.


I'm a little confused by this. If the underlying filter chain builder implementation requires a transport socket (like for QUIC) can't it just fail in that code? IMO that would be more clear? WDYT?

filter chain builder doesn't know if this listener is UDP or TCP, nor does it know about if it's connectionless or not. For non-Quic listener, it's empty transport_socket is allowed, and in that case a factory for RawBufferSocket will be created..

mattklein123 · 2019-09-17T17:41:35Z

source/server/well_known_names.h

 class UdpListenerNameValues {
 public:
  const std::string RawUdp = "raw_udp_listener";
+  const std::string Quic = "quic_listener";


alyssawilk

LGTM, modulo Matt's comments

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-09-18T16:55:14Z

Ping?

mattklein123

Let's ship and iterate! Well done. :)

envoyproxy#8091) Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 6 commits August 21, 2019 17:25

build FilterChain

33a71d8

Signed-off-by: Dan Zhang <danzh@google.com>

add test

24692ce

Signed-off-by: Dan Zhang <danzh@google.com>

format

bfeff99

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchain

bd688c2

Signed-off-by: Dan Zhang <danzh@google.com>

test quic

2a53122

Signed-off-by: Dan Zhang <danzh@google.com>

remove debug log

31ad1f8

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested a review from lizan as a code owner August 29, 2019 20:06

fix compile error

81aa31d

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned alyssawilk Aug 29, 2019

mattklein123 self-assigned this Aug 29, 2019

fix listener manager

8cce689

Signed-off-by: Dan Zhang <danzh@google.com>

lambdai reviewed Aug 30, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 1, 2019

fix linux test error

e37f233

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 4, 2019

danzh1989 added 2 commits September 4, 2019 14:49

more logging

e01047d

Signed-off-by: Dan Zhang <danzh@google.com>

fix socket close error

8049fd1

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk added the waiting label Sep 5, 2019

danzh2010 changed the title ~~Filterchain~~ quiche: implement QUIC specific FilterChain Sep 9, 2019

lambdai previously approved these changes Sep 9, 2019

View reviewed changes

repokitteh-read-only bot assigned goaway Sep 9, 2019

alyssawilk reviewed Sep 10, 2019

View reviewed changes

mattklein123 removed the waiting label Sep 10, 2019

mattklein123 requested changes Sep 10, 2019

View reviewed changes

danzh1989 added 5 commits September 12, 2019 19:02

add transport socket factory for quic

877ae42

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchain

05bb8c2

Signed-off-by: Dan Zhang <danzh@google.com>

fix test

2f6788a

Signed-off-by: Dan Zhang <danzh@google.com>

revert tls_context factory

8a7337e

Signed-off-by: Dan Zhang <danzh@google.com>

more comment

2ae5458

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 added the waiting label Sep 14, 2019

alyssawilk reviewed Sep 16, 2019

View reviewed changes

comment

f3d426f

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 16, 2019

repokitteh-read-only bot added the waiting label Sep 16, 2019

Merge branch 'master' into filterchain

f0ac1f1

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 16, 2019

danzh2010 changed the title ~~quiche: implement QUIC specific FilterChain~~ quiche: implement QUIC specific TransportSocketFactory for TLS context Sep 16, 2019

fix clang_tidy

76ba786

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Sep 17, 2019

View reviewed changes

typo

4bbdcb0

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 requested changes Sep 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 17, 2019

alyssawilk previously approved these changes Sep 17, 2019

View reviewed changes

nits

9395ff7

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed alyssawilk’s stale review via 9395ff7 September 17, 2019 23:31

repokitteh-read-only bot removed the waiting label Sep 17, 2019

mattklein123 approved these changes Sep 19, 2019

View reviewed changes

mattklein123 merged commit 272ee70 into envoyproxy:master Sep 19, 2019

danzh2010 added a commit to danzh2010/envoy that referenced this pull request Sep 24, 2019

quiche: implement QUIC specific TransportSocketFactory for TLS context (

0ae0551

envoyproxy#8091) Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 added a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

quiche: implement QUIC specific TransportSocketFactory for TLS context (

f63052f

envoyproxy#8091) Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 added a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

quiche: implement QUIC specific TransportSocketFactory for TLS context (

04a4104

envoyproxy#8091) Signed-off-by: Dan Zhang <danzh@google.com>

Conversation

danzh2010 commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Aug 29, 2019

Uh oh!

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

mattklein123 commented Sep 1, 2019

Uh oh!

danzh2010 commented Sep 5, 2019

Uh oh!

repokitteh-read-only bot commented Sep 5, 2019

Uh oh!

danzh2010 commented Sep 9, 2019

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Sep 9, 2019

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!

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

danzh2010 commented Aug 29, 2019 •

edited

Loading