-
Notifications
You must be signed in to change notification settings - Fork 5.5k
network: add DownstreamTransportSocketConfigFactory and registry #2437
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 7 commits
48950a0
f34d885
8c4e39c
b78b836
e843dee
deb5486
1abb513
d5127bb
b98f539
06dba15
bafd37a
813b77e
b250972
364d4fc
d958983
dbeec06
981ba1e
9375259
a14e609
99cabb0
9468ac7
97a8926
e870f04
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 |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| #include "envoy/common/exception.h" | ||
| #include "envoy/network/connection.h" | ||
| #include "envoy/network/listen_socket.h" | ||
| #include "envoy/network/transport_socket.h" | ||
| #include "envoy/ssl/context.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -32,9 +33,9 @@ class ListenerConfig { | |
| virtual ListenSocket& socket() PURE; | ||
|
|
||
| /** | ||
| * @return Ssl::ServerContext* the default SSL context. | ||
| * @return TransportSocketFacotry& the transport socket factory. | ||
|
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. nit: TransportSocketFactory |
||
| */ | ||
| virtual Ssl::ServerContext* defaultSslContext() PURE; | ||
| virtual TransportSocketFactory& transportSocketFactory() PURE; | ||
|
|
||
| /** | ||
| * @return bool specifies whether the listener should actually listen on the port. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,11 +33,31 @@ class TransportSocketConfigFactory { | |
| public: | ||
| virtual ~TransportSocketConfigFactory() {} | ||
|
|
||
| /** | ||
| * @return ProtobufTypes::MessagePtr create empty config proto message. The transport socket | ||
| * config, which arrives in an opaque google.protobuf.Struct message, will be converted | ||
| * to JSON and then parsed into this empty proto. | ||
| */ | ||
| virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE; | ||
|
|
||
| /** | ||
| * @return std::string the identifying name for a particular TransportSocketFactoryPtr | ||
| * implementation produced by the factory. | ||
| */ | ||
| virtual std::string name() const PURE; | ||
| }; | ||
|
|
||
| /** | ||
| * Implemented by each transport socket which does upstream connection. Registered via class | ||
|
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. nit: "used for upstream connections." |
||
| * RegisterFactory. | ||
| */ | ||
| class UpstreamTransportSocketConfigFactory : public virtual TransportSocketConfigFactory { | ||
| public: | ||
| /** | ||
| * Create a particular transport socket factory implementation. | ||
| * @param config const Protobuf::Message& supplies the config message for the transport socket | ||
| * implementation. | ||
| * @param context TransportSocketFactoryContext& supplies the transport socket's context. | ||
| * @param context TransportSocketFactoryContext& supplies the transport socket's context. | ||
| * @return Network::TransportSocketFactoryPtr the transport socket factory instance. The returned | ||
| * TransportSocketFactoryPtr should not be nullptr. | ||
| * | ||
|
|
@@ -47,23 +67,40 @@ class TransportSocketConfigFactory { | |
| virtual Network::TransportSocketFactoryPtr | ||
| createTransportSocketFactory(const Protobuf::Message& config, | ||
| TransportSocketFactoryContext& context) PURE; | ||
| }; | ||
|
|
||
| /** | ||
| * Implemented by each transport socket which does downstream connection. Registered via class | ||
|
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. nit: "used for downstream connections." |
||
| * RegisterFactory. | ||
| */ | ||
| class DownstreamTransportSocketConfigFactory : public virtual TransportSocketConfigFactory { | ||
| public: | ||
| /** | ||
| * @return ProtobufTypes::MessagePtr create empty config proto message. The transport socket | ||
| * config, which arrives in an opaque google.protobuf.Struct message, will be converted | ||
| * to JSON and then parsed into this empty proto. | ||
| */ | ||
| virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE; | ||
|
|
||
| /** | ||
| * @return std::string the identifying name for a particular TransportSocketFactoryPtr | ||
| * implementation produced by the factory. | ||
| * Create a particular downstream transport socket factory implementation. | ||
| * TODO(lizan): Revisit the parameters for SNI below 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. This parameter is | ||
| * currently used by SNI implementation to know the expected server names. | ||
| * @param skip_ssl_context_update bool indicates whether the ssl context update should be skipped. | ||
|
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. Ditto |
||
| * This parameter is currently used by SNI implementation to know whether it should perform | ||
| * certificate selection. | ||
| * @param config const Protobuf::Message& supplies the config message for the transport socket | ||
| * implementation. | ||
| * @param context TransportSocketFactoryContext& supplies the transport socket's context. | ||
| * @return Network::TransportSocketFactoryPtr the transport socket factory instance. The returned | ||
| * TransportSocketFactoryPtr should not be nullptr. | ||
| * | ||
| * @throw EnvoyException if the implementation is unable to produce a factory with the provided | ||
| * parameters. | ||
| */ | ||
| virtual std::string name() const PURE; | ||
| virtual Network::TransportSocketFactoryPtr | ||
| createTransportSocketFactory(const std::string& listener_name, | ||
| const std::vector<std::string>& server_names, | ||
| bool skip_ssl_context_update, const Protobuf::Message& config, | ||
| TransportSocketFactoryContext& context) PURE; | ||
| }; | ||
|
|
||
| class UpstreamTransportSocketConfigFactory : public virtual TransportSocketConfigFactory {}; | ||
|
|
||
| } // namespace Configuration | ||
| } // namespace Server | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,5 +65,17 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory { | |
| ClientContextPtr ssl_ctx_; | ||
| }; | ||
|
|
||
| class ServerSslSocketFactory : public Network::TransportSocketFactory { | ||
| public: | ||
| ServerSslSocketFactory(const ServerContextConfig& config, const std::string& listener_name, | ||
| const std::vector<std::string>& server_names, bool skip_context_update, | ||
| Ssl::ContextManager& manager, Stats::Scope& stats_scope); | ||
| Network::TransportSocketPtr createTransportSocket() const override; | ||
| bool implementsSecureTransport() const override; | ||
|
|
||
| private: | ||
| ServerContextPtr ssl_ctx_; | ||
|
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. nit: const |
||
| }; | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,8 +184,9 @@ void ConnectionHandlerImpl::ActiveListener::onAccept( | |
| } | ||
|
|
||
| void ConnectionHandlerImpl::ActiveListener::newConnection(Network::ConnectionSocketPtr&& socket) { | ||
| auto ts = config_.transportSocketFactory().createTransportSocket(); | ||
|
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. nit: local var not needed |
||
| Network::ConnectionPtr new_connection = | ||
| parent_.dispatcher_.createServerConnection(std::move(socket), config_.defaultSslContext()); | ||
| parent_.dispatcher_.createServerConnection(std::move(socket), std::move(ts)); | ||
| new_connection->setBufferLimits(config_.perConnectionBufferLimitBytes()); | ||
| onNewConnection(std::move(new_connection)); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,14 @@ | ||
| #include "server/listener_manager_impl.h" | ||
|
|
||
| #include "envoy/registry/registry.h" | ||
| #include "envoy/server/transport_socket_config.h" | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/config/utility.h" | ||
| #include "common/config/well_known_names.h" | ||
| #include "common/network/listen_socket_impl.h" | ||
| #include "common/network/utility.h" | ||
| #include "common/protobuf/utility.h" | ||
| #include "common/ssl/context_config_impl.h" | ||
|
|
||
| #include "server/configuration_impl.h" | ||
| #include "server/drain_manager_impl.h" | ||
|
|
@@ -167,17 +167,33 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManag | |
| "is currently not supported", | ||
| address_->asString())); | ||
| } | ||
| if (filter_chain.has_tls_context()) { | ||
| Ssl::ServerContextConfigImpl context_config(filter_chain.tls_context()); | ||
| tls_contexts_.emplace_back(parent_.server_.sslContextManager().createSslServerContext( | ||
| name_, sni_domains, *listener_scope_, context_config, skip_context_update)); | ||
| has_tls++; | ||
| if (filter_chain.tls_context().has_session_ticket_keys()) { | ||
| has_stk++; | ||
|
|
||
| auto transport_socket = filter_chain.transport_socket(); | ||
| if (!filter_chain.has_transport_socket()) { | ||
|
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. 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?
Member
Author
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. 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. |
||
| if (filter_chain.has_tls_context()) { | ||
| transport_socket.set_name(Config::TransportSocketNames::get().SSL); | ||
| MessageUtil::jsonConvert(filter_chain.tls_context(), *transport_socket.mutable_config()); | ||
|
|
||
| has_tls++; | ||
| if (filter_chain.tls_context().has_session_ticket_keys()) { | ||
| has_stk++; | ||
| } | ||
| } else { | ||
| transport_socket.set_name(Config::TransportSocketNames::get().RAW_BUFFER); | ||
| } | ||
| } | ||
|
|
||
| auto& config_factory = Config::Utility::getAndCheckFactory< | ||
| Server::Configuration::DownstreamTransportSocketConfigFactory>(transport_socket.name()); | ||
| ProtobufTypes::MessagePtr message = | ||
| Config::Utility::translateToFactoryConfig(transport_socket, config_factory); | ||
| transport_socket_factories_.emplace_back(config_factory.createTransportSocketFactory( | ||
|
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. 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.
Member
Author
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. We do support multiple filter chain, only with same filters, to support SNI. There are tests here: Since TransportSocketFactory replaces ServerContext here, it has to be vector.
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. If you always return the 0th element of the vector, what is the point of the vector?
Member
Author
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. 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?
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. 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.
Member
Author
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. Sure, done. |
||
| name_, sni_domains, skip_context_update, *message, *this)); | ||
| } | ||
|
|
||
| ASSERT(!transport_socket_factories_.empty()); | ||
| ASSERT(transport_socket_factories_[0]); | ||
|
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. nit: |
||
|
|
||
| // TODO(PiotrSikora): allow filter chains with mixed use of Session Ticket Keys. | ||
| // This doesn't work right now, because BoringSSL uses "session context" (initial SSL_CTX that | ||
| // accepted connection, before SNI update) for session related stuff, including Session Ticket | ||
|
|
||
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.
Please fix doc comments