Bootstrap and configuration of monolithic QuicListener. by mpwarres · Pull Request #5549 · envoyproxy/envoy

mpwarres · 2019-01-09T07:04:15Z

Description:

Plumbing for configuration and bootstrap of (not yet filled-in) QuicListener. Attempts to satisfy the following design constraints:

Accomodate multiple possible implementations of QuicListener (e.g. QUICHE as well as ngtcp2 or others).
Keep most of the QuicListener implementation as an extension outside of core, that can be omitted from the build.
Avoid overlap with UDP proxying interfaces being hammered out in PR Introducing UDP listener. #5473.

While unit tests are included and this code is (I think) in a state ready for review, it may make sense to wait until there is a full integration test with a minimal (faked-out) QuicListener implementation before reviewing this in depth. (Hopefully that should be within the next couple of days.) I'm posting this now primarily to provide some context for sibling PR #5548, and to get high-level feedback if this is heading in the wrong direction.

Risk Level: low, not used yet.
Testing: unit tests.
Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Michael Warres <mpw@google.com>

Along with this, add Server::ProdListenerComponentFactory implementation, and Server::ListenerImpl use of it. Signed-off-by: Michael Warres <mpw@google.com>

Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-01-09T07:05:15Z

FYI @mattklein123 @alyssawilk @conqerAtapple

alyssawilk

Very cool!

alyssawilk · 2019-01-09T14:37:44Z

api/envoy/api/v2/listener/listener.proto

+  // supported listener. Future listeners may include:
+  //
+  // * envoy.quic.quiche
+  // * envoy.quic.ngtcp2


I'd be inclined to skip examples in favor of a TODO to comment where supported (upstream) listeners can be found.

Also maybe QuicListenerConfig? Having a listener have a QuicListener seems a bit circular but saying this is where we stick quic-specific-config makes more sense to me.

alyssawilk · 2019-01-09T14:42:36Z

api/envoy/config/quic_listener/quiche/v2alpha/quiche.proto

+  // QUIC traffic should be specified in the nested :ref:`HttpConnectionManager
+  // <envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager>` config
+  // protobuf.
+  envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager http_config = 1


As said elsewhere I would have leaned towards configuring the HCM config in the listener filter config a we do for TCP but that may be nonsensical for generic UDP. Blergh.

+1. I think we can make this happen within the main listener config and runtime checks, with the caveat that I think we will need to pivot between static and connection oriented UDP like I mentioned above.

alyssawilk · 2019-01-09T14:51:14Z

api/envoy/api/v2/lds.proto

+  // QUIC listener for handing UDP packets. Only used when :ref:`address
+  // <envoy_api_field_Listener.address>` specifies a UDP :ref:`SocketAddress
+  // <envoy_api_msg_core_SocketAddress>`.
+  listener.QuicListener quic_listener = 16;


If we're using this config for factory registration of "what listener do I want to create" I think we may want to make it generic so we can reuse for generic UdpListenerImpls, WDYT?

Also at a glance I may have a slightly different idea of how we're planning to do the QUIC monolith - I'd assumed we'd have code which factory created a QUICListener, which was a subclass of a regular listener (or if we stick with @conqerAtapple 's APIs would be a UdpListener), which would have a similar idea of filter chains (once we have n-connection emulation but would check-fail for listener filters and any non-HCM filters today) and the HCM config would be a normal filter off the QuicListener. That said that may not make sense for generic UDP.... Let's sync up today in person or on chat.

Unfortunately we are in a bit of an API bind here. If I were starting from scratch, I think I would have different listener types with possible a CommonListener section shared by all the types, so I think this is going to get ugly no matter what we do, as we are going to have a lot of special cases that look like "if you have this listener type, these fields don't apply."

With that said, I don't think it's worth going through the deprecation dance to fix this as it would be a huge change. If people think we should do that though, I think we should discuss it sooner rather than later (cc @htuch for API thoughts). For the rest of this review I will assume we aren't going to do a fundamental refactoring.

On this particular topic, IMO, I think we can do basic UDP without a factory. If it's connection oriented most of the fields apply, and if it's not connection oriented, we can likely have a new type of filter chain that instantiates static filters (maybe the existing filter chain config can still be used w/ runtime checks for filter types). So I guess we will need some way for basic UDP of pivoting between connection oriented and static at the config level. @conqerAtapple thoughts here?

So on the QUIC topic, if we are committed to supporting runtime QUIC listener loading, then this approach seems fine to me. I agree with @alyssawilk that I would call this QuicListenerConfig even if a bit redundant.

I guess there are two separate considerations; from an implementation and API perspective. Given that what is proposed in this PR is just to add a new kind of opaque config for pluggable listener variants, it seems we could easily reuse this for UDP etc if we name it correctly. How this manifest in the concrete implementation, ¯\_(ツ)_/¯

I think UDP listener would make more sense here and would work well with #5473

conqerAtapple

Excited to see this. Some high level comments at this point. I think we still have the layering being figured out with UdpListener. Hopefully things will be more clear soon with that.

conqerAtapple · 2019-01-09T18:08:25Z

include/envoy/quic/listener.h

+// (//include/envoy/network:listener_interface).
+class QuicListener {
+public:
+  virtual ~QuicListener() {}


Why not virtual ~QuicListener() = default ? Here and other places.

conqerAtapple · 2019-01-09T18:15:53Z

include/envoy/network/listener.h

+   * @return QuicListenerFactory* the factory for creating QUIC listeners, or nullptr if none is
+   *         configured.
+   */
+  virtual Quic::QuicListenerFactory* quicListenerFactory() PURE;


What is the ownership model for this pointer?

It's owned internally.

conqerAtapple · 2019-01-09T18:17:35Z

include/envoy/quic/config.h

+   * @return std::string the identifying name for a particular implementation of a QUIC listener
+   * produced by the factory.
+   */
+  virtual std::string name() PURE;


what about const qualifying the method? I guess you left it out as this is an interface ?

mattklein123

Thanks for working on this. Super exciting. Flushing some high level API comments which we should discuss before the impl.

mattklein123 · 2019-01-09T20:48:37Z

api/envoy/api/v2/lds.proto

+  // QUIC listener for handing UDP packets. Only used when :ref:`address
+  // <envoy_api_field_Listener.address>` specifies a UDP :ref:`SocketAddress
+  // <envoy_api_msg_core_SocketAddress>`.
+  listener.QuicListener quic_listener = 16;


Unfortunately we are in a bit of an API bind here. If I were starting from scratch, I think I would have different listener types with possible a CommonListener section shared by all the types, so I think this is going to get ugly no matter what we do, as we are going to have a lot of special cases that look like "if you have this listener type, these fields don't apply."

With that said, I don't think it's worth going through the deprecation dance to fix this as it would be a huge change. If people think we should do that though, I think we should discuss it sooner rather than later (cc @htuch for API thoughts). For the rest of this review I will assume we aren't going to do a fundamental refactoring.

On this particular topic, IMO, I think we can do basic UDP without a factory. If it's connection oriented most of the fields apply, and if it's not connection oriented, we can likely have a new type of filter chain that instantiates static filters (maybe the existing filter chain config can still be used w/ runtime checks for filter types). So I guess we will need some way for basic UDP of pivoting between connection oriented and static at the config level. @conqerAtapple thoughts here?

So on the QUIC topic, if we are committed to supporting runtime QUIC listener loading, then this approach seems fine to me. I agree with @alyssawilk that I would call this QuicListenerConfig even if a bit redundant.

mattklein123 · 2019-01-09T20:49:17Z

api/envoy/api/v2/listener/listener.proto

+  // supported listener. Future listeners may include:
+  //
+  // * envoy.quic.quiche
+  // * envoy.quic.ngtcp2


mattklein123 · 2019-01-09T20:49:42Z

api/envoy/api/v2/listener/listener.proto

+  oneof config_type {
+    google.protobuf.Struct config = 2;
+
+    // [#not-implemented-hide:]


cc @lizan for whichever PR merges first.

Yeah, I think we can go straight to Any and skip the Struct in this PR.

mattklein123 · 2019-01-09T20:50:29Z

api/envoy/config/quic_listener/quiche/v2alpha/quiche.proto

+  // QUIC traffic should be specified in the nested :ref:`HttpConnectionManager
+  // <envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager>` config
+  // protobuf.
+  envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager http_config = 1


+1. I think we can make this happen within the main listener config and runtime checks, with the caveat that I think we will need to pivot between static and connection oriented UDP like I mentioned above.

mattklein123 · 2019-01-09T20:51:01Z

include/envoy/network/listener.h

+   * @return QuicListenerFactory* the factory for creating QUIC listeners, or nullptr if none is
+   *         configured.
+   */
+  virtual Quic::QuicListenerFactory* quicListenerFactory() PURE;


It's owned internally.

mattklein123 · 2019-01-09T20:51:45Z

include/envoy/quic/listener.h

+// could even be used directly instead of it. However, as currently stands, this
+// would introduce a circular dependency, since Network::ListenerConfig (which
+// refers to QuicListenerFactory) and Network::Listener (to which
+// QuicListenerFactory would refer) are defined in the same build target


Could we just fix that and break them into a separate header file?

htuch · 2019-01-09T23:47:50Z

api/envoy/api/v2/lds.proto

+  // QUIC listener for handing UDP packets. Only used when :ref:`address
+  // <envoy_api_field_Listener.address>` specifies a UDP :ref:`SocketAddress
+  // <envoy_api_msg_core_SocketAddress>`.
+  listener.QuicListener quic_listener = 16;


I guess there are two separate considerations; from an implementation and API perspective. Given that what is proposed in this PR is just to add a new kind of opaque config for pluggable listener variants, it seems we could easily reuse this for UDP etc if we name it correctly. How this manifest in the concrete implementation, ¯\_(ツ)_/¯

htuch · 2019-01-09T23:48:36Z

api/envoy/api/v2/listener/listener.proto

+  // * envoy.quic.ngtcp2
+  string name = 1 [(validate.rules).string.min_bytes = 1];
+
+  // Filter specific configuration which depends on the filter being instantiated.


Prefer "listener extension" to "filter" terminology here I think.

htuch · 2019-01-09T23:49:05Z

api/envoy/api/v2/listener/listener.proto

+  oneof config_type {
+    google.protobuf.Struct config = 2;
+
+    // [#not-implemented-hide:]


Yeah, I think we can go straight to Any and skip the Struct in this PR.

stale · 2019-01-21T02:02:26Z

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!

mpwarres · 2019-01-21T15:15:45Z

I'm resuming work on this now that #5548 is in.

stale · 2019-01-28T20:55:57Z

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!

mpwarres · 2019-01-28T21:30:41Z

Tagging associated issue #2557

stale · 2019-02-05T14:18:49Z

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!

stale · 2019-02-12T14:56:54Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

mpwarres added 7 commits January 9, 2019 00:01

Add QuicListenerFactory and QuicListenerConfigFactory interfaces.

2c2e3a8

Signed-off-by: Michael Warres <mpw@google.com>

Add quic_listener field to Listener config protobuf.

7ec46e0

Signed-off-by: Michael Warres <mpw@google.com>

Add Network::ListenerConfig::quicListenerFactory() method.

a5b9636

Signed-off-by: Michael Warres <mpw@google.com>

Add Server::ListenerComponentFactory::createQuicListenerFactory().

40a8abf

Along with this, add Server::ProdListenerComponentFactory implementation, and Server::ListenerImpl use of it. Signed-off-by: Michael Warres <mpw@google.com>

Add envoy.quic_listeners.quiche extension.

c2331db

Signed-off-by: Michael Warres <mpw@google.com>

Add HttpConnectionManager field to Quiche config proto.

aa6188c

Signed-off-by: Michael Warres <mpw@google.com>

Add validation of Quiche config proto.

056f0cf

Signed-off-by: Michael Warres <mpw@google.com>

alyssawilk self-assigned this Jan 9, 2019

alyssawilk reviewed Jan 9, 2019

View reviewed changes

mattklein123 self-assigned this Jan 9, 2019

conqerAtapple reviewed Jan 9, 2019

View reviewed changes

mattklein123 reviewed Jan 9, 2019

View reviewed changes

htuch suggested changes Jan 9, 2019

View reviewed changes

mattklein123 added the waiting label Jan 13, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 21, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 21, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 28, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 28, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 5, 2019

stale bot closed this Feb 12, 2019

Conversation

mpwarres commented Jan 9, 2019

Uh oh!

mpwarres commented Jan 9, 2019

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!

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

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!

stale bot commented Jan 21, 2019

Uh oh!

mpwarres commented Jan 21, 2019

Uh oh!

stale bot commented Jan 28, 2019

Uh oh!

mpwarres commented Jan 28, 2019

Uh oh!

stale bot commented Feb 5, 2019

Uh oh!

stale bot commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants