-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Bootstrap and configuration of monolithic QuicListener. #5549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2c2e3a8
7ec46e0
a5b9636
40a8abf
c2331db
aa6188c
056f0cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -215,3 +215,22 @@ message ListenerFilter { | |
| google.protobuf.Any typed_config = 3; | ||
| } | ||
| } | ||
|
|
||
| // [#proto-status: experimental] | ||
| message QuicListener { | ||
| // The name of the QUIC listener to instantiate. The name must match a | ||
| // supported listener. Future listeners may include: | ||
| // | ||
| // * envoy.quic.quiche | ||
| // * envoy.quic.ngtcp2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| string name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
|
||
| // Filter specific configuration which depends on the filter being instantiated. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer "listener extension" to "filter" terminology here I think. |
||
| // See the supported filters for further documentation. | ||
| oneof config_type { | ||
| google.protobuf.Struct config = 2; | ||
|
|
||
| // [#not-implemented-hide:] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @lizan for whichever PR merges first.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think we can go straight to |
||
| google.protobuf.Any typed_config = 3; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| load("//bazel:api_build_system.bzl", "api_proto_library") | ||
|
|
||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| api_proto_library( | ||
| name = "quiche", | ||
| srcs = ["quiche.proto"], | ||
| deps = ["//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.config.quic_listener.quiche.v2alpha; | ||
| option java_package = "io.envoyproxy.envoy.config.quic_listener.quiche.v2alpha"; | ||
| option java_multiple_files = true; | ||
| option go_package = "v2"; | ||
|
|
||
| import "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto"; | ||
|
|
||
| import "validate/validate.proto"; | ||
| import "gogoproto/gogo.proto"; | ||
|
|
||
| // [#protodoc-title: QUICHE] | ||
|
|
||
| // Configuration for QUICHE-based QUIC listener. This will provide QUIC support to Envoy. | ||
| // https://quiche.googlesource.com/quiche/ | ||
| message Quiche { | ||
|
|
||
| // HTTP-level configuration for the QUIC listener. In particular, HTTP filters to be applied to | ||
| // 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +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. |
||
| [(validate.rules).message.required = true, (gogoproto.nullable) = false]; | ||
|
|
||
| // QUIC-specific config will be added as fields here. | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| #include "envoy/common/exception.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/network/listen_socket.h" | ||
| #include "envoy/quic/listener.h" | ||
| #include "envoy/stats/scope.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -31,6 +32,12 @@ class ListenerConfig { | |
| */ | ||
| virtual FilterChainFactory& filterChainFactory() PURE; | ||
|
|
||
| /** | ||
| * @return QuicListenerFactory* the factory for creating QUIC listeners, or nullptr if none is | ||
| * configured. | ||
| */ | ||
| virtual Quic::QuicListenerFactory* quicListenerFactory() PURE; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the ownership model for this pointer?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's owned internally. |
||
|
|
||
| /** | ||
| * @return Socket& the actual listen socket. The address of this socket may be | ||
| * different from configured if for example the configured address binds to port zero. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "config_interface", | ||
| hdrs = ["config.h"], | ||
| deps = [ | ||
| ":listener_interface", | ||
| "//include/envoy/server:filter_config_interface", | ||
| "//source/common/protobuf", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "listener_interface", | ||
| hdrs = ["listener.h"], | ||
| deps = [ | ||
| "//include/envoy/network:listen_socket_interface", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| #pragma once | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "envoy/quic/listener.h" | ||
| #include "envoy/server/filter_config.h" | ||
|
|
||
| #include "common/protobuf/protobuf.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Quic { | ||
|
|
||
| /** | ||
| * Implemented by the QUIC listener and registered via RegisterFactory. | ||
| */ | ||
| class QuicListenerConfigFactory { | ||
| public: | ||
| virtual ~QuicListenerConfigFactory() {} | ||
|
|
||
| /** | ||
| * Create a particular QUIC listener factory implementation. If the implementation is unable to | ||
| * produce a factory with the provided parameters, it should throw an EnvoyException. | ||
| * @param config the protobuf configuration for the listener. | ||
| * @param context the listener's context. | ||
| */ | ||
| virtual QuicListenerFactoryPtr | ||
| createListenerFactoryFromProto(const Protobuf::Message& config, | ||
| Server::Configuration::ListenerFactoryContext& context) PURE; | ||
|
|
||
| /** | ||
| * @return ProtobufTypes::MessagePtr create empty config proto message. The QUIC listener | ||
| * config, which arrives in an opaque message, will be parsed into this empty proto. | ||
| */ | ||
| virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE; | ||
|
|
||
| /** | ||
| * @return std::string the identifying name for a particular implementation of a QUIC listener | ||
| * produced by the factory. | ||
| */ | ||
| virtual std::string name() PURE; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about |
||
| }; | ||
|
|
||
| } // namespace Quic | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| #pragma once | ||
|
|
||
| #include <memory> | ||
|
|
||
| #include "envoy/network/listen_socket.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Event { | ||
| class Dispatcher; | ||
| } | ||
|
|
||
| namespace Quic { | ||
|
|
||
| /** | ||
| * Callbacks used by QuicListener instances to communicate with their execution | ||
| * environment. | ||
| */ | ||
| class QuicListenerCallbacks { | ||
| public: | ||
| virtual ~QuicListenerCallbacks() {} | ||
|
|
||
| /** | ||
| * @return the socket via which the listener can send and receive datagrams. | ||
| */ | ||
| virtual Network::Socket& socket() PURE; | ||
|
|
||
| /** | ||
| * @return the dispatcher via which the listener can register for events. | ||
| */ | ||
| virtual Event::Dispatcher& dispatcher() PURE; | ||
| }; | ||
|
|
||
| /** | ||
| * Abstract interface for QUIC listeners. | ||
| */ | ||
| // Ideally, this class would extend Network::Listener, or Network::Listener | ||
| // 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just fix that and break them into a separate header file? |
||
| // (//include/envoy/network:listener_interface). | ||
| class QuicListener { | ||
| public: | ||
| virtual ~QuicListener() {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not |
||
|
|
||
| /** | ||
| * Temporarily disable accepting new connections. | ||
| */ | ||
| virtual void disable() PURE; | ||
|
|
||
| /** | ||
| * Enable accepting new connections. | ||
| */ | ||
| virtual void enable() PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<QuicListener> QuicListenerPtr; | ||
|
|
||
| /** | ||
| * Factory for creating QUIC listeners. | ||
| */ | ||
| class QuicListenerFactory { | ||
| public: | ||
| virtual ~QuicListenerFactory() {} | ||
|
|
||
| /** | ||
| * Create a QUIC listener. The listener is responsible for registering for | ||
| * socket events as needed; both socket and dispatcher are accessible via the | ||
| * callbacks provided. | ||
| * @return the newly created listener. | ||
| */ | ||
| virtual QuicListenerPtr createQuicListener(QuicListenerCallbacks& callbacks) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<QuicListenerFactory> QuicListenerFactoryPtr; | ||
|
|
||
| } // namespace Quic | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "well_known_names", | ||
| hdrs = ["well_known_names.h"], | ||
| deps = [ | ||
| "//source/common/singleton:const_singleton", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| # QUICHE-based QUIC listener implementation | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "config", | ||
| srcs = ["config.cc"], | ||
| deps = [ | ||
| "//include/envoy/quic:config_interface", | ||
| "//include/envoy/registry", | ||
| "//source/common/protobuf:utility_lib", | ||
| "//source/extensions/quic_listeners:well_known_names", | ||
| "@envoy_api//envoy/config/quic_listener/quiche/v2alpha:quiche_cc", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
QuicListenerConfigeven if a bit redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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,
¯\_(ツ)_/¯There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
UDPlistener would make more sense here and would work well with #5473