From d182cbe242fa06111d1612fb319923f86329b612 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 23 Dec 2019 14:53:56 -0800 Subject: [PATCH 01/44] wip: get an api listener at all costs Signed-off-by: Jose Nino --- include/envoy/http/codec.h | 2 + include/envoy/server/BUILD | 1 + include/envoy/server/instance.h | 4 +- include/envoy/server/listener_manager.h | 2 + source/common/http/conn_manager_impl.cc | 41 ++--- .../network/http_connection_manager/BUILD | 1 + .../network/http_connection_manager/config.cc | 69 ++++++++- .../network/http_connection_manager/config.h | 8 + source/server/BUILD | 6 + source/server/config_validation/server.h | 1 + source/server/listener_impl.cc | 68 +++++++++ source/server/listener_impl.h | 140 +++++++++++++++++- source/server/listener_manager_impl.cc | 11 ++ source/server/listener_manager_impl.h | 4 + source/server/server.h | 3 + 15 files changed, 337 insertions(+), 24 deletions(-) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index ca092f037434..386f333e8c2f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -420,6 +420,8 @@ class ServerConnectionCallbacks : public virtual ConnectionCallbacks { bool is_internally_created = false) PURE; }; +using ServerConnectionCallbacksPtr = std::unique_ptr; + /** * A server side HTTP connection. */ diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 657643e74a86..3a53e80ef912 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -102,6 +102,7 @@ envoy_cc_library( "//include/envoy/api:api_interface", "//include/envoy/common:mutex_tracer", "//include/envoy/event:timer_interface", + "//include/envoy/http:codec_interface", "//include/envoy/http:context_interface", "//include/envoy/http:query_params_interface", "//include/envoy/init:manager_interface", diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 59e3a9a8b93e..93d932856da3 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -8,7 +8,7 @@ #include "envoy/api/api.h" #include "envoy/common/mutex_tracer.h" #include "envoy/event/timer.h" -#include "envoy/grpc/context.h" +#include "envoy/http/codec.h" #include "envoy/http/context.h" #include "envoy/init/manager.h" #include "envoy/local_info/local_info.h" @@ -229,6 +229,8 @@ class Instance { * @return Configuration::ServerFactoryContext& factory context for filters. */ virtual Configuration::ServerFactoryContext& serverFactoryContext() PURE; + + virtual Http::ServerConnectionCallbacks* apiListener() PURE; }; } // namespace Server diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 6709adcfc875..5a71a53693e6 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -211,6 +211,8 @@ class ListenerManager { */ using FailureStates = std::vector>; virtual void endListenerUpdate(FailureStates&& failure_states) PURE; + + virtual Http::ServerConnectionCallbacks* apiListener() PURE; }; } // namespace Server diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 3061e0daca2b..3828eef47870 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -118,7 +118,9 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, overload_manager ? overload_manager->getThreadLocalOverloadState().getState( Server::OverloadActionNames::get().DisableHttpKeepAlive) : Server::OverloadManager::getInactiveState()), - time_source_(time_source) {} + time_source_(time_source) { + ENVOY_LOG(error, "HCM constructor"); +} const HeaderMapImpl& ConnectionManagerImpl::continueHeader() { CONSTRUCT_ON_FIRST_USE(HeaderMapImpl, @@ -258,7 +260,8 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder, if (connection_idle_timer_) { connection_idle_timer_->disableTimer(); } - + ENVOY_LOG(error, "we should crash here"); + ENVOY_CONN_LOG(error, "new stream", read_callbacks_->connection()); ENVOY_CONN_LOG(debug, "new stream", read_callbacks_->connection()); ActiveStreamPtr new_stream(new ActiveStream(*this)); new_stream->state_.is_internally_created_ = is_internally_created; @@ -1382,23 +1385,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply( - is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply(is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, + FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD index bcc676feed09..9287ebac6793 100644 --- a/source/extensions/filters/network/http_connection_manager/BUILD +++ b/source/extensions/filters/network/http_connection_manager/BUILD @@ -20,6 +20,7 @@ envoy_cc_extension( deps = [ "//include/envoy/config:config_provider_manager_interface", "//include/envoy/filesystem:filesystem_interface", + "//include/envoy/http:codec_interface", "//include/envoy/http:filter_interface", "//include/envoy/registry", "//include/envoy/router:route_config_provider_manager_interface", diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index f5662dda813e..da4c63d18ff5 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -6,8 +6,6 @@ #include #include "envoy/api/v2/core/base.pb.h" -#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" -#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h" #include "envoy/filesystem/filesystem.h" #include "envoy/server/admin.h" #include "envoy/tracing/http_tracer.h" @@ -489,6 +487,73 @@ const Network::Address::Instance& HttpConnectionManagerConfig::localAddress() { return *context_.localInfo().address(); } +// Singleton registration via macro defined in envoy/singleton/manager.h +SINGLETON_MANAGER_REGISTRATION(hcm_date_provider); +SINGLETON_MANAGER_REGISTRATION(hcm_route_config_provider_manager); +SINGLETON_MANAGER_REGISTRATION(hcm_scoped_routes_config_provider_manager); + +std::function +HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( + const ProtobufWkt::Any& proto_config, Server::Configuration::FactoryContext& context) { + ENVOY_LOG_MISC(error, "In factory"); + auto typed_config = MessageUtil::anyConvert< + envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager>( + proto_config); + ENVOY_LOG_MISC(error, "Downcasted proto successfully"); + + std::shared_ptr hcm_date_provider = + context.singletonManager().getTyped( + SINGLETON_MANAGER_REGISTERED_NAME(hcm_date_provider), [&context] { + return std::make_shared(context.dispatcher(), + context.threadLocal()); + }); + ENVOY_LOG_MISC(error, "Built date provider"); + + std::shared_ptr hcm_route_config_provider_manager = + context.singletonManager().getTyped( + SINGLETON_MANAGER_REGISTERED_NAME(hcm_route_config_provider_manager), [&context] { + return std::make_shared(context.admin()); + }); + + ENVOY_LOG_MISC(error, "Built route config provider"); + + std::shared_ptr + hcm_scoped_routes_config_provider_manager = + context.singletonManager().getTyped( + SINGLETON_MANAGER_REGISTERED_NAME(hcm_scoped_routes_config_provider_manager), + [&context, hcm_route_config_provider_manager] { + return std::make_shared( + context.admin(), *hcm_route_config_provider_manager); + }); + ENVOY_LOG_MISC(error, "built scoped route provider"); + + std::shared_ptr filter_config(new HttpConnectionManagerConfig( + typed_config, context, *hcm_date_provider, *hcm_route_config_provider_manager, + *hcm_scoped_routes_config_provider_manager)); + ENVOY_LOG_MISC(error, "built config"); + + // This lambda captures the shared_ptrs created above, thus preserving the + // reference count. + // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine + // as these captured objects are also global singletons. + ENVOY_LOG_MISC(error, "about to return lambda"); + + return [hcm_scoped_routes_config_provider_manager, hcm_route_config_provider_manager, + hcm_date_provider, filter_config, &context]( + Network::ReadFilterCallbacks& read_callbacks) -> Http::ServerConnectionCallbacksPtr { + ENVOY_LOG_MISC(error, "Inside of factory lambda"); + + auto conn_manager = std::make_unique( + *filter_config, context.drainDecision(), context.random(), context.httpContext(), + context.runtime(), context.localInfo(), context.clusterManager(), + &context.overloadManager(), context.dispatcher().timeSource()); + + conn_manager->initializeReadFilterCallbacks(read_callbacks); + + return conn_manager; + }; +} + } // namespace HttpConnectionManager } // namespace NetworkFilters } // namespace Extensions diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 0eaa73b9e6a8..258e2d29166a 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -11,6 +11,7 @@ #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h" #include "envoy/http/filter.h" +#include "envoy/http/codec.h" #include "envoy/router/route_config_provider_manager.h" #include "common/common/logger.h" @@ -199,6 +200,13 @@ class HttpConnectionManagerConfig : Logger::Loggable, static const uint64_t RequestTimeoutMs = 0; }; +class HttpConnectionManagerFactory { +public: + static std::function + createHttpConnectionManagerFactoryFromProto(const ProtobufWkt::Any& proto_config, + Server::Configuration::FactoryContext& context); +}; + } // namespace HttpConnectionManager } // namespace NetworkFilters } // namespace Extensions diff --git a/source/server/BUILD b/source/server/BUILD index 5e4cc5a66719..f2d679694d33 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -278,10 +278,12 @@ envoy_cc_library( ":listener_manager_impl", ":transport_socket_config_lib", ":well_known_names_lib", + "//include/envoy/network:connection_interface", "//include/envoy/server:active_udp_listener_config_interface", "//include/envoy/server:filter_config_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", + "//source/common/common:empty_string", "//source/common/config:utility_lib", "//source/common/init:manager_lib", "//source/common/network:connection_balancer_lib", @@ -289,7 +291,9 @@ envoy_cc_library( "//source/common/network:socket_option_factory_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", + "//source/common/stream_info:stream_info_lib", "//source/extensions/filters/listener:well_known_names", + "//source/extensions/filters/network/http_connection_manager:config", "//source/extensions/transport_sockets:well_known_names", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/api/v2/core:pkg_cc_proto", @@ -319,12 +323,14 @@ envoy_cc_library( "//include/envoy/server:transport_socket_config_interface", "//include/envoy/server:worker_interface", "//source/common/config:utility_lib", + "//source/common/http:conn_manager_lib", "//source/common/init:manager_lib", "//source/common/network:listen_socket_lib", "//source/common/network:socket_option_factory_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", "//source/extensions/filters/listener:well_known_names", + "//source/extensions/filters/network/http_connection_manager:config", "//source/extensions/transport_sockets:well_known_names", "@envoy_api//envoy/admin/v2alpha:pkg_cc_proto", "@envoy_api//envoy/api/v2:pkg_cc_proto", diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 71483c76ce55..890cd5b70750 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -110,6 +110,7 @@ class ValidationInstance final : Logger::Loggable, return validation_context_; } Configuration::ServerFactoryContext& serverFactoryContext() override { return server_context_; } + Http::ServerConnectionCallbacks* apiListener() override { return nullptr; } // Server::ListenerComponentFactory LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index c9b7da94f87d..dc5d13d74357 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -10,6 +10,7 @@ #include "common/common/assert.h" #include "common/config/utility.h" +#include "common/http/conn_manager_impl.h" #include "common/network/connection_balancer_impl.h" #include "common/network/resolver_impl.h" #include "common/network/socket_option_factory.h" @@ -25,6 +26,7 @@ #include "extensions/filters/listener/well_known_names.h" #include "extensions/transport_sockets/well_known_names.h" +#include "extensions/filters/network/http_connection_manager/config.h" namespace Envoy { namespace Server { @@ -397,5 +399,71 @@ void ListenerImpl::setSocketFactory(const Network::ListenSocketFactorySharedPtr& socket_factory_ = socket_factory; } +////////////////// API Listener ////////////////////////// +ApiListenerImpl::ApiListenerImpl(const envoy::api::v2::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor) + : config_(config), parent_(parent), name_(name), + address_(Network::Address::resolveProtoAddress(config.address())), + validation_visitor_(validation_visitor), + global_scope_(parent_.server_.stats().createScope("")), + listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), + read_callbacks_(SyntheticReadCallbacks(*this)) { + ENVOY_LOG(error, "In API listener constructor"); + http_connection_manager_factory_ = + Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: + createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), *this); + ENVOY_LOG(error, "Created lambda"); +} + +AccessLog::AccessLogManager& ApiListenerImpl::accessLogManager() { + return parent_.server_.accessLogManager(); +} +Upstream::ClusterManager& ApiListenerImpl::clusterManager() { + return parent_.server_.clusterManager(); +} +Event::Dispatcher& ApiListenerImpl::dispatcher() { return parent_.server_.dispatcher(); } +Network::DrainDecision& ApiListenerImpl::drainDecision() { return *this; } +Grpc::Context& ApiListenerImpl::grpcContext() { return parent_.server_.grpcContext(); } +bool ApiListenerImpl::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } +Tracing::HttpTracer& ApiListenerImpl::httpTracer() { return httpContext().tracer(); } +Http::Context& ApiListenerImpl::httpContext() { return parent_.server_.httpContext(); } +Init::Manager& ApiListenerImpl::initManager() { return parent_.server_.initManager(); } +const LocalInfo::LocalInfo& ApiListenerImpl::localInfo() const { + return parent_.server_.localInfo(); +} +Envoy::Runtime::RandomGenerator& ApiListenerImpl::random() { return parent_.server_.random(); } +Envoy::Runtime::Loader& ApiListenerImpl::runtime() { return parent_.server_.runtime(); } +Stats::Scope& ApiListenerImpl::scope() { return *global_scope_; } +Singleton::Manager& ApiListenerImpl::singletonManager() { + return parent_.server_.singletonManager(); +} +OverloadManager& ApiListenerImpl::overloadManager() { return parent_.server_.overloadManager(); } +ThreadLocal::Instance& ApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } +Admin& ApiListenerImpl::admin() { return parent_.server_.admin(); } +const envoy::api::v2::core::Metadata& ApiListenerImpl::listenerMetadata() const { + return config_.metadata(); +}; +envoy::api::v2::core::TrafficDirection ApiListenerImpl::direction() const { + return config_.traffic_direction(); +}; +TimeSource& ApiListenerImpl::timeSource() { return api().timeSource(); } + +ProtobufMessage::ValidationVisitor& ApiListenerImpl::messageValidationVisitor() { + return validation_visitor_; +} +Api::Api& ApiListenerImpl::api() { return parent_.server_.api(); } +ServerLifecycleNotifier& ApiListenerImpl::lifecycleNotifier() { + return parent_.server_.lifecycleNotifier(); +} +OptProcessContextRef ApiListenerImpl::processContext() { return parent_.server_.processContext(); } +Configuration::ServerFactoryContext& ApiListenerImpl::getServerFactoryContext() const { + return parent_.server_.serverFactoryContext(); +} +Stats::Scope& ApiListenerImpl::listenerScope() { return *listener_scope_; } + +// FIXME hook the api listener into listener draining in the manager. +bool ApiListenerImpl::drainClose() const { return false; } + } // namespace Server } // namespace Envoy diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 85130c3ced5e..942bdc6b9ee3 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -4,6 +4,7 @@ #include "envoy/api/v2/core/base.pb.h" #include "envoy/api/v2/lds.pb.h" +#include "envoy/network/connection.h" #include "envoy/network/filter.h" #include "envoy/server/drain_manager.h" #include "envoy/server/filter_config.h" @@ -12,6 +13,8 @@ #include "common/common/logger.h" #include "common/init/manager_impl.h" +#include "common/common/empty_string.h" +#include "common/stream_info/stream_info_impl.h" #include "server/filter_chain_manager_impl.h" @@ -247,5 +250,138 @@ class ListenerImpl : public Network::ListenerConfig, friend class ListenerFilterChainFactoryBuilder; }; -} // namespace Server -} // namespace Envoy +class ApiListenerImpl : public Configuration::FactoryContext, + public Network::DrainDecision, + Logger::Loggable { +public: + ApiListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, + const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor); + + Http::ServerConnectionCallbacks* apiHandle() { + if (!http_connection_manager_) { + ENVOY_LOG_MISC(error, "There was no hcm creating one"); + http_connection_manager_ = http_connection_manager_factory_(read_callbacks_); + ENVOY_LOG(error, "Exercised lambda"); + } + return http_connection_manager_.get(); + } + + const Network::Address::InstanceConstSharedPtr& address() const { return address_; } + + AccessLog::AccessLogManager& accessLogManager() override; + Upstream::ClusterManager& clusterManager() override; + Event::Dispatcher& dispatcher() override; + Network::DrainDecision& drainDecision() override; + Grpc::Context& grpcContext() override; + bool healthCheckFailed() override; + Tracing::HttpTracer& httpTracer() override; + Http::Context& httpContext() override; + Init::Manager& initManager() override; + const LocalInfo::LocalInfo& localInfo() const override; + Envoy::Runtime::RandomGenerator& random() override; + Envoy::Runtime::Loader& runtime() override; + Stats::Scope& scope() override; + Singleton::Manager& singletonManager() override; + OverloadManager& overloadManager() override; + ThreadLocal::Instance& threadLocal() override; + Admin& admin() override; + const envoy::api::v2::core::Metadata& listenerMetadata() const override; + envoy::api::v2::core::TrafficDirection direction() const override; + TimeSource& timeSource() override; + ProtobufMessage::ValidationVisitor& messageValidationVisitor() override; + Api::Api& api() override; + ServerLifecycleNotifier& lifecycleNotifier() override; + OptProcessContextRef processContext() override; + Configuration::ServerFactoryContext& getServerFactoryContext() const override; + Stats::Scope& listenerScope() override; + + bool drainClose() const override; + +private: + class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { + public: + SyntheticReadCallbacks(ApiListenerImpl& parent) + : parent_(parent), connection_(SyntheticConnection(*this)) {} + + // Network::ReadFilterCallbacks + void continueReading() override {} + void injectReadDataToFilterChain(Buffer::Instance&, bool) override {} + Upstream::HostDescriptionConstSharedPtr upstreamHost() override { return nullptr; } + void upstreamHost(Upstream::HostDescriptionConstSharedPtr) override {} + Network::Connection& connection() override { return connection_; } + + class SyntheticConnection : public Network::Connection { + public: + SyntheticConnection(SyntheticReadCallbacks& parent) + : parent_(parent), stream_info_(parent_.parent_.timeSource()), + options_(std::make_shared>()) {} + + // Network::FilterManager + void addWriteFilter(Network::WriteFilterSharedPtr) override {} + void addFilter(Network::FilterSharedPtr) override {} + void addReadFilter(Network::ReadFilterSharedPtr) override {} + bool initializeReadFilters() override { return true; } + + // Network::Connection + void addConnectionCallbacks(Network::ConnectionCallbacks&) override {} + void addBytesSentCallback(Network::Connection::BytesSentCb) override {} + void enableHalfClose(bool) override {} + void close(Network::ConnectionCloseType) override {} + Event::Dispatcher& dispatcher() override { return parent_.parent_.dispatcher(); } + uint64_t id() const override { return 12345; } + std::string nextProtocol() const override { return EMPTY_STRING; } + void noDelay(bool) override {} + void readDisable(bool) override {} + void detectEarlyCloseWhenReadDisabled(bool) override {} + bool readEnabled() const override { return true; } + const Network::Address::InstanceConstSharedPtr& remoteAddress() const override { + return parent_.parent_.address(); + } + absl::optional + unixSocketPeerCredentials() const override { + return absl::nullopt; + } + const Network::Address::InstanceConstSharedPtr& localAddress() const override { + return parent_.parent_.address(); + } + void setConnectionStats(const Network::Connection::ConnectionStats&) override {} + Ssl::ConnectionInfoConstSharedPtr ssl() const override { return nullptr; } + absl::string_view requestedServerName() const override { return EMPTY_STRING; } + State state() const override { return Network::Connection::State::Open; } + void write(Buffer::Instance&, bool) override {} + void setBufferLimits(uint32_t) override {} + uint32_t bufferLimit() const override { return 65000; } + bool localAddressRestored() const override { return false; } + bool aboveHighWatermark() const override { return false; } + const Network::ConnectionSocket::OptionsSharedPtr& socketOptions() const override { + return options_; + } + StreamInfo::StreamInfo& streamInfo() override { return stream_info_; } + const StreamInfo::StreamInfo& streamInfo() const override { return stream_info_; } + void setDelayedCloseTimeout(std::chrono::milliseconds) override {} + absl::string_view transportFailureReason() const override { return EMPTY_STRING; } + + SyntheticReadCallbacks& parent_; + StreamInfo::StreamInfoImpl stream_info_; + Network::ConnectionSocket::OptionsSharedPtr options_; + }; + + ApiListenerImpl& parent_; + SyntheticConnection connection_; + }; + + const envoy::api::v2::Listener& config_; + ListenerManagerImpl& parent_; + const std::string name_; + Network::Address::InstanceConstSharedPtr address_; + ProtobufMessage::ValidationVisitor& validation_visitor_; + Stats::ScopePtr global_scope_; + Stats::ScopePtr listener_scope_; + SyntheticReadCallbacks read_callbacks_; + std::function + http_connection_manager_factory_; + Http::ServerConnectionCallbacksPtr http_connection_manager_; +}; // namespace Server +}; // namespace Server + + ; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index ef03a703e843..beae286ec439 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -29,6 +29,7 @@ #include "extensions/filters/listener/well_known_names.h" #include "extensions/transport_sockets/well_known_names.h" +#include "extensions/filters/network/http_connection_manager/config.h" namespace Envoy { namespace Server { @@ -300,6 +301,16 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& config, const std::string& version_info, bool added_via_api) { + + // FIXME initial experimentation where we only allow one API listener to exist and to come from + // bootstrap + if (!api_listener_ && config.has_api_listener()) { + ENVOY_LOG(error, "initializing API listener"); + api_listener_ = std::make_unique( + config, *this, config.name(), server_.messageValidationContext().staticValidationVisitor()); + return true; + } + std::string name; if (!config.name().empty()) { name = config.name(); diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 3c1f8f3067f5..11523fab1641 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -145,6 +145,9 @@ class ListenerManagerImpl : public ListenerManager, Logger::LoggableapiHandle() : nullptr; + } Instance& server_; ListenerComponentFactory& factory_; @@ -214,6 +217,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable api_listener_; // Active listeners are listeners that are currently accepting new connections on the workers. ListenerList active_listeners_; // Warming listeners are listeners that may need further initialization via the listener's init diff --git a/source/server/server.h b/source/server/server.h index 13acc78b04eb..d4ec7aeca437 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -223,6 +223,9 @@ class InstanceImpl final : Logger::Loggable, ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() const override { return *local_info_; } TimeSource& timeSource() override { return time_source_; } + Http::ServerConnectionCallbacks* apiListener() override { + return listenerManager().apiListener(); + } Configuration::ServerFactoryContext& serverFactoryContext() override { return server_context_; } From 3c36ef8289cf101dc63361cc95aa28d2cf8ad2e1 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 23 Dec 2019 15:46:36 -0800 Subject: [PATCH 02/44] fmt build Signed-off-by: Jose Nino --- include/envoy/server/instance.h | 1 + source/common/http/conn_manager_impl.cc | 23 +++++++++++++++++++++-- source/server/listener_impl.cc | 2 +- source/server/listener_impl.h | 8 ++++---- source/server/listener_manager_impl.cc | 2 +- 5 files changed, 28 insertions(+), 8 deletions(-) diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 93d932856da3..4254fb6fee29 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -8,6 +8,7 @@ #include "envoy/api/api.h" #include "envoy/common/mutex_tracer.h" #include "envoy/event/timer.h" +#include "envoy/grpc/context.h" #include "envoy/http/codec.h" #include "envoy/http/context.h" #include "envoy/init/manager.h" diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 3828eef47870..2a4019e83f08 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -260,19 +260,38 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder, if (connection_idle_timer_) { connection_idle_timer_->disableTimer(); } - ENVOY_LOG(error, "we should crash here"); + ENVOY_CONN_LOG(error, "new stream", read_callbacks_->connection()); - ENVOY_CONN_LOG(debug, "new stream", read_callbacks_->connection()); + ENVOY_CONN_LOG(error, "new stream2", read_callbacks_->connection()); + + if (!read_callbacks_) { + ENVOY_LOG_MISC(error, "read_callbacks is null"); + } + ENVOY_CONN_LOG(debug, "new stream3", read_callbacks_->connection()); ActiveStreamPtr new_stream(new ActiveStream(*this)); + ENVOY_CONN_LOG(error, "new stream4", read_callbacks_->connection()); + new_stream->state_.is_internally_created_ = is_internally_created; + ENVOY_CONN_LOG(error, "new stream5", read_callbacks_->connection()); + new_stream->response_encoder_ = &response_encoder; + ENVOY_CONN_LOG(error, "new stream6", read_callbacks_->connection()); + new_stream->response_encoder_->getStream().addCallbacks(*new_stream); + ENVOY_CONN_LOG(error, "new stream7", read_callbacks_->connection()); + new_stream->buffer_limit_ = new_stream->response_encoder_->getStream().bufferLimit(); + ENVOY_CONN_LOG(error, "new stream8", read_callbacks_->connection()); + // If the network connection is backed up, the stream should be made aware of it on creation. // Both HTTP/1.x and HTTP/2 codecs handle this in StreamCallbackHelper::addCallbacks_. ASSERT(read_callbacks_->connection().aboveHighWatermark() == false || new_stream->high_watermark_count_ > 0); + ENVOY_CONN_LOG(error, "new stream9", read_callbacks_->connection()); + new_stream->moveIntoList(std::move(new_stream), streams_); + ENVOY_CONN_LOG(error, "new stream10", read_callbacks_->connection()); + return **streams_.begin(); } diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index dc5d13d74357..fdfd700d3331 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -25,8 +25,8 @@ #include "server/well_known_names.h" #include "extensions/filters/listener/well_known_names.h" -#include "extensions/transport_sockets/well_known_names.h" #include "extensions/filters/network/http_connection_manager/config.h" +#include "extensions/transport_sockets/well_known_names.h" namespace Envoy { namespace Server { diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 942bdc6b9ee3..a9cd0ed92df0 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -11,9 +11,9 @@ #include "envoy/server/listener_manager.h" #include "envoy/stats/scope.h" +#include "common/common/empty_string.h" #include "common/common/logger.h" #include "common/init/manager_impl.h" -#include "common/common/empty_string.h" #include "common/stream_info/stream_info_impl.h" #include "server/filter_chain_manager_impl.h" @@ -381,7 +381,7 @@ class ApiListenerImpl : public Configuration::FactoryContext, std::function http_connection_manager_factory_; Http::ServerConnectionCallbacksPtr http_connection_manager_; -}; // namespace Server -}; // namespace Server +}; - ; +} // namespace Server +} // namespace Envoy diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index beae286ec439..d9b79a505d8d 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -28,8 +28,8 @@ #include "server/well_known_names.h" #include "extensions/filters/listener/well_known_names.h" -#include "extensions/transport_sockets/well_known_names.h" #include "extensions/filters/network/http_connection_manager/config.h" +#include "extensions/transport_sockets/well_known_names.h" namespace Envoy { namespace Server { From d91b112ccaf866c1bedc806bea389d5ba580382a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 23 Dec 2019 18:00:16 -0800 Subject: [PATCH 03/44] building and worksgit add source/! :tada: Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 23 ++----------------- source/common/http/conn_manager_impl.h | 5 ++++ .../network/http_connection_manager/config.cc | 5 ++++ 3 files changed, 12 insertions(+), 21 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2a4019e83f08..a83a430d0200 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -261,37 +261,17 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder, connection_idle_timer_->disableTimer(); } - ENVOY_CONN_LOG(error, "new stream", read_callbacks_->connection()); - ENVOY_CONN_LOG(error, "new stream2", read_callbacks_->connection()); - - if (!read_callbacks_) { - ENVOY_LOG_MISC(error, "read_callbacks is null"); - } - ENVOY_CONN_LOG(debug, "new stream3", read_callbacks_->connection()); + ENVOY_CONN_LOG(debug, "new stream", read_callbacks_->connection()); ActiveStreamPtr new_stream(new ActiveStream(*this)); - ENVOY_CONN_LOG(error, "new stream4", read_callbacks_->connection()); - new_stream->state_.is_internally_created_ = is_internally_created; - ENVOY_CONN_LOG(error, "new stream5", read_callbacks_->connection()); - new_stream->response_encoder_ = &response_encoder; - ENVOY_CONN_LOG(error, "new stream6", read_callbacks_->connection()); - new_stream->response_encoder_->getStream().addCallbacks(*new_stream); - ENVOY_CONN_LOG(error, "new stream7", read_callbacks_->connection()); - new_stream->buffer_limit_ = new_stream->response_encoder_->getStream().bufferLimit(); - ENVOY_CONN_LOG(error, "new stream8", read_callbacks_->connection()); - // If the network connection is backed up, the stream should be made aware of it on creation. // Both HTTP/1.x and HTTP/2 codecs handle this in StreamCallbackHelper::addCallbacks_. ASSERT(read_callbacks_->connection().aboveHighWatermark() == false || new_stream->high_watermark_count_ > 0); - ENVOY_CONN_LOG(error, "new stream9", read_callbacks_->connection()); - new_stream->moveIntoList(std::move(new_stream), streams_); - ENVOY_CONN_LOG(error, "new stream10", read_callbacks_->connection()); - return **streams_.begin(); } @@ -517,6 +497,7 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource(), connection_manager.filterState()), upstream_options_(std::make_shared()) { + ENVOY_LOG_MISC(error, "Creating ActiveStream"); ASSERT(!connection_manager.config_.isRoutable() || ((connection_manager.config_.routeConfigProvider() == nullptr && connection_manager.config_.scopedRouteConfigProvider() != nullptr) || diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index ec9f58326661..8ba5d65c07d8 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -66,6 +66,11 @@ class ConnectionManagerImpl : Logger::Loggable, Stats::Scope& scope); static const HeaderMapImpl& continueHeader(); + void forceCodecCreation() { + ASSERT(!codec_); + codec_ = config_.createCodec(read_callbacks_->connection(), Buffer::OwnedImpl(), *this); + } + // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; Network::FilterStatus onNewConnection() override; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index da4c63d18ff5..e7f1ed7bcbeb 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -548,7 +548,12 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( context.runtime(), context.localInfo(), context.clusterManager(), &context.overloadManager(), context.dispatcher().timeSource()); + // Additional actions taken in the normal path. + // When a new connection is creating its filter chain it hydrates the factory with a filter + // manager which provides the hcm with the "read_callbacks". conn_manager->initializeReadFilterCallbacks(read_callbacks); + // When the connection first sends data through the hcm, the hcm creates a codec. + conn_manager->forceCodecCreation(); return conn_manager; }; From c6f2d8a5b0c5a1677ea07c07759282b94ed0093a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 26 Dec 2019 15:34:29 -0800 Subject: [PATCH 04/44] initial clean up. Builds, tested Signed-off-by: Jose Nino --- include/envoy/server/BUILD | 1 - include/envoy/server/instance.h | 3 - include/envoy/server/listener_manager.h | 14 +- source/server/BUILD | 34 ++++- source/server/api_listener.cc | 90 +++++++++++++ source/server/api_listener.h | 162 +++++++++++++++++++++++ source/server/config_validation/server.h | 1 - source/server/listener_impl.cc | 68 ---------- source/server/listener_impl.h | 135 ------------------- source/server/listener_manager_impl.cc | 47 ++++++- source/server/listener_manager_impl.h | 8 +- source/server/server.h | 3 - 12 files changed, 339 insertions(+), 227 deletions(-) create mode 100644 source/server/api_listener.cc create mode 100644 source/server/api_listener.h diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 3a53e80ef912..657643e74a86 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -102,7 +102,6 @@ envoy_cc_library( "//include/envoy/api:api_interface", "//include/envoy/common:mutex_tracer", "//include/envoy/event:timer_interface", - "//include/envoy/http:codec_interface", "//include/envoy/http:context_interface", "//include/envoy/http:query_params_interface", "//include/envoy/init:manager_interface", diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 4254fb6fee29..59e3a9a8b93e 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -9,7 +9,6 @@ #include "envoy/common/mutex_tracer.h" #include "envoy/event/timer.h" #include "envoy/grpc/context.h" -#include "envoy/http/codec.h" #include "envoy/http/context.h" #include "envoy/init/manager.h" #include "envoy/local_info/local_info.h" @@ -230,8 +229,6 @@ class Instance { * @return Configuration::ServerFactoryContext& factory context for filters. */ virtual Configuration::ServerFactoryContext& serverFactoryContext() PURE; - - virtual Http::ServerConnectionCallbacks* apiListener() PURE; }; } // namespace Server diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 5a71a53693e6..571fdc6301cb 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -212,7 +212,19 @@ class ListenerManager { using FailureStates = std::vector>; virtual void endListenerUpdate(FailureStates&& failure_states) PURE; - virtual Http::ServerConnectionCallbacks* apiListener() PURE; + // FIXME figure out what type to return here. An idea I had is to provide a handle that connects + // to Envoy's owned listener via a weak ptr -- similar to the init manager handles. This way API + // calls can be safe without owning the underlying listener. Moreover, it might be the case that + // while this function returns a handle, the caller needs prior knowledge about the API surface + // that the handle provides so they can downcast to the actual "API listener" surface. This could + // also help with the type that the listener manager stores, i.e it stores "ApiListener" and the + // only thing that that interface knows how to do is to return a safe handle. + /** + * @name name of the API Listener to search for. + * @return Http::ServerConnectionCallbacks* a handle to the API Listener if it exists, nullptr if + * it does not. + */ + virtual Http::ServerConnectionCallbacks* apiListener(const std::string& name) PURE; }; } // namespace Server diff --git a/source/server/BUILD b/source/server/BUILD index f2d679694d33..f938bd39c8cf 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -263,6 +263,31 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "api_listener_lib", + srcs = [ + "api_listener.cc", + ], + hdrs = [ + "api_listener.h", + ], + deps = [ + ":drain_manager_lib", + ":listener_manager_impl", + "//include/envoy/network:connection_interface", + "//include/envoy/server:filter_config_interface", + "//include/envoy/server:listener_manager_interface", + "//source/common/common:empty_string", + "//source/common/http:conn_manager_lib", + "//source/common/init:manager_lib", + "//source/common/network:resolver_lib", + "//source/common/stream_info:stream_info_lib", + "//source/extensions/filters/network/http_connection_manager:config", + "@envoy_api//envoy/api/v2:pkg_cc_proto", + "@envoy_api//envoy/api/v2/listener:pkg_cc_proto", + ], +) + envoy_cc_library( name = "listener_lib", srcs = [ @@ -278,12 +303,10 @@ envoy_cc_library( ":listener_manager_impl", ":transport_socket_config_lib", ":well_known_names_lib", - "//include/envoy/network:connection_interface", "//include/envoy/server:active_udp_listener_config_interface", "//include/envoy/server:filter_config_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", - "//source/common/common:empty_string", "//source/common/config:utility_lib", "//source/common/init:manager_lib", "//source/common/network:connection_balancer_lib", @@ -291,9 +314,7 @@ envoy_cc_library( "//source/common/network:socket_option_factory_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", - "//source/common/stream_info:stream_info_lib", "//source/extensions/filters/listener:well_known_names", - "//source/extensions/filters/network/http_connection_manager:config", "//source/extensions/transport_sockets:well_known_names", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/api/v2/core:pkg_cc_proto", @@ -301,12 +322,16 @@ envoy_cc_library( ], ) +# TODO(junr03): actually separate this lib from the listener and api listener lib. +# this can be done if the parent_ in the listener and the api listener becomes the ListenerManager interface. +# the issue right now is that the listener's reach into the listener manager's server_ instance variable. envoy_cc_library( name = "listener_manager_impl", srcs = [ "listener_manager_impl.cc", ], hdrs = [ + "api_listener.h", "listener_impl.h", "listener_manager_impl.h", ], @@ -390,6 +415,7 @@ envoy_cc_library( ], deps = [ ":active_raw_udp_listener_config", + ":api_listener_lib", ":configuration_lib", ":connection_handler_lib", ":guarddog_lib", diff --git a/source/server/api_listener.cc b/source/server/api_listener.cc new file mode 100644 index 000000000000..358845288032 --- /dev/null +++ b/source/server/api_listener.cc @@ -0,0 +1,90 @@ +#include "server/api_listener.h" + +#include "envoy/api/v2/lds.pb.h" +#include "envoy/api/v2/listener/listener.pb.h" +#include "envoy/stats/scope.h" + +#include "common/http/conn_manager_impl.h" +#include "common/protobuf/utility.h" +#include "common/network/resolver_impl.h" + +#include "server/drain_manager_impl.h" +#include "server/listener_manager_impl.h" + +#include "extensions/filters/network/http_connection_manager/config.h" + +namespace Envoy { +namespace Server { + +HttpApiListener::HttpApiListener(const envoy::api::v2::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor) + : config_(config), parent_(parent), name_(name), + address_(Network::Address::resolveProtoAddress(config.address())), + validation_visitor_(validation_visitor), + global_scope_(parent_.server_.stats().createScope("")), + listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), + read_callbacks_(SyntheticReadCallbacks(*this)) { + ENVOY_LOG(error, "In API listener constructor"); + http_connection_manager_factory_ = + Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: + createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), *this); + ENVOY_LOG(error, "Created lambda"); +} + +Http::ServerConnectionCallbacks* HttpApiListener::apiHandle() { + if (!http_connection_manager_) { + http_connection_manager_ = http_connection_manager_factory_(read_callbacks_); + } + return http_connection_manager_.get(); +} + +AccessLog::AccessLogManager& HttpApiListener::accessLogManager() { + return parent_.server_.accessLogManager(); +} +Upstream::ClusterManager& HttpApiListener::clusterManager() { + return parent_.server_.clusterManager(); +} +Event::Dispatcher& HttpApiListener::dispatcher() { return parent_.server_.dispatcher(); } +Network::DrainDecision& HttpApiListener::drainDecision() { return *this; } +Grpc::Context& HttpApiListener::grpcContext() { return parent_.server_.grpcContext(); } +bool HttpApiListener::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } +Tracing::HttpTracer& HttpApiListener::httpTracer() { return httpContext().tracer(); } +Http::Context& HttpApiListener::httpContext() { return parent_.server_.httpContext(); } +Init::Manager& HttpApiListener::initManager() { return parent_.server_.initManager(); } +const LocalInfo::LocalInfo& HttpApiListener::localInfo() const { + return parent_.server_.localInfo(); +} +Envoy::Runtime::RandomGenerator& HttpApiListener::random() { return parent_.server_.random(); } +Envoy::Runtime::Loader& HttpApiListener::runtime() { return parent_.server_.runtime(); } +Stats::Scope& HttpApiListener::scope() { return *global_scope_; } +Singleton::Manager& HttpApiListener::singletonManager() { + return parent_.server_.singletonManager(); +} +OverloadManager& HttpApiListener::overloadManager() { return parent_.server_.overloadManager(); } +ThreadLocal::Instance& HttpApiListener::threadLocal() { return parent_.server_.threadLocal(); } +Admin& HttpApiListener::admin() { return parent_.server_.admin(); } +const envoy::api::v2::core::Metadata& HttpApiListener::listenerMetadata() const { + return config_.metadata(); +}; +envoy::api::v2::core::TrafficDirection HttpApiListener::direction() const { + return config_.traffic_direction(); +}; +TimeSource& HttpApiListener::timeSource() { return api().timeSource(); } +ProtobufMessage::ValidationVisitor& HttpApiListener::messageValidationVisitor() { + return validation_visitor_; +} +Api::Api& HttpApiListener::api() { return parent_.server_.api(); } +ServerLifecycleNotifier& HttpApiListener::lifecycleNotifier() { + return parent_.server_.lifecycleNotifier(); +} +OptProcessContextRef HttpApiListener::processContext() { return parent_.server_.processContext(); } +Configuration::ServerFactoryContext& HttpApiListener::getServerFactoryContext() const { + return parent_.server_.serverFactoryContext(); +} +Stats::Scope& HttpApiListener::listenerScope() { return *listener_scope_; } +// FIXME hook the api listener into listener draining in the manager. +bool HttpApiListener::drainClose() const { return false; } + +} // namespace Server +} // namespace Envoy diff --git a/source/server/api_listener.h b/source/server/api_listener.h new file mode 100644 index 000000000000..9718bb43c366 --- /dev/null +++ b/source/server/api_listener.h @@ -0,0 +1,162 @@ +#pragma once + +#include + +#include "envoy/api/v2/lds.pb.h" +#include "envoy/network/connection.h" +#include "envoy/network/filter.h" +#include "envoy/server/drain_manager.h" +#include "envoy/server/filter_config.h" +#include "envoy/server/listener_manager.h" +#include "envoy/stats/scope.h" + +#include "common/http/conn_manager_impl.h" +#include "common/common/empty_string.h" +#include "common/common/logger.h" +#include "common/init/manager_impl.h" +#include "common/stream_info/stream_info_impl.h" + +#include "extensions/filters/network/http_connection_manager/config.h" + +namespace Envoy { +namespace Server { + +class ListenerManagerImpl; + +/** + * Listener that provides a handle to inject HTTP calls into envoy via the traditional HCM path. + * Thus it provides full access to Envoy's L7 features, e.g HTTP filters. + */ +class HttpApiListener : public Configuration::FactoryContext, + public Network::DrainDecision, + Logger::Loggable { +public: + HttpApiListener(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, + const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor); + + // see thoughts on what this API handle could be at server/listener_manager.h + Http::ServerConnectionCallbacks* apiHandle(); + const Network::Address::InstanceConstSharedPtr& address() const { return address_; } + + // TODO(junr03): the majority of this surface could be moved out of the listener via some sort of + // base class context. + // Server::Configuration::FactoryContext + AccessLog::AccessLogManager& accessLogManager() override; + Upstream::ClusterManager& clusterManager() override; + Event::Dispatcher& dispatcher() override; + Network::DrainDecision& drainDecision() override; + Grpc::Context& grpcContext() override; + bool healthCheckFailed() override; + Tracing::HttpTracer& httpTracer() override; + Http::Context& httpContext() override; + Init::Manager& initManager() override; + const LocalInfo::LocalInfo& localInfo() const override; + Envoy::Runtime::RandomGenerator& random() override; + Envoy::Runtime::Loader& runtime() override; + Stats::Scope& scope() override; + Singleton::Manager& singletonManager() override; + OverloadManager& overloadManager() override; + ThreadLocal::Instance& threadLocal() override; + Admin& admin() override; + const envoy::api::v2::core::Metadata& listenerMetadata() const override; + envoy::api::v2::core::TrafficDirection direction() const override; + TimeSource& timeSource() override; + ProtobufMessage::ValidationVisitor& messageValidationVisitor() override; + Api::Api& api() override; + ServerLifecycleNotifier& lifecycleNotifier() override; + OptProcessContextRef processContext() override; + Configuration::ServerFactoryContext& getServerFactoryContext() const override; + Stats::Scope& listenerScope() override; + + bool drainClose() const override; + +private: + class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { + public: + SyntheticReadCallbacks(HttpApiListener& parent) + : parent_(parent), connection_(SyntheticConnection(*this)) {} + + // Network::ReadFilterCallbacks + void continueReading() override {} + void injectReadDataToFilterChain(Buffer::Instance&, bool) override {} + Upstream::HostDescriptionConstSharedPtr upstreamHost() override { return nullptr; } + void upstreamHost(Upstream::HostDescriptionConstSharedPtr) override {} + Network::Connection& connection() override { return connection_; } + + class SyntheticConnection : public Network::Connection { + public: + SyntheticConnection(SyntheticReadCallbacks& parent) + : parent_(parent), stream_info_(parent_.parent_.timeSource()), + options_(std::make_shared>()) {} + + // Network::FilterManager + void addWriteFilter(Network::WriteFilterSharedPtr) override {} + void addFilter(Network::FilterSharedPtr) override {} + void addReadFilter(Network::ReadFilterSharedPtr) override {} + bool initializeReadFilters() override { return true; } + + // Network::Connection + void addConnectionCallbacks(Network::ConnectionCallbacks&) override {} + void addBytesSentCallback(Network::Connection::BytesSentCb) override {} + void enableHalfClose(bool) override {} + void close(Network::ConnectionCloseType) override {} + Event::Dispatcher& dispatcher() override { return parent_.parent_.dispatcher(); } + uint64_t id() const override { return 12345; } + std::string nextProtocol() const override { return EMPTY_STRING; } + void noDelay(bool) override {} + void readDisable(bool) override {} + void detectEarlyCloseWhenReadDisabled(bool) override {} + bool readEnabled() const override { return true; } + const Network::Address::InstanceConstSharedPtr& remoteAddress() const override { + return parent_.parent_.address(); + } + absl::optional + unixSocketPeerCredentials() const override { + return absl::nullopt; + } + const Network::Address::InstanceConstSharedPtr& localAddress() const override { + return parent_.parent_.address(); + } + void setConnectionStats(const Network::Connection::ConnectionStats&) override {} + Ssl::ConnectionInfoConstSharedPtr ssl() const override { return nullptr; } + absl::string_view requestedServerName() const override { return EMPTY_STRING; } + State state() const override { return Network::Connection::State::Open; } + void write(Buffer::Instance&, bool) override {} + void setBufferLimits(uint32_t) override {} + uint32_t bufferLimit() const override { return 65000; } + bool localAddressRestored() const override { return false; } + bool aboveHighWatermark() const override { return false; } + const Network::ConnectionSocket::OptionsSharedPtr& socketOptions() const override { + return options_; + } + StreamInfo::StreamInfo& streamInfo() override { return stream_info_; } + const StreamInfo::StreamInfo& streamInfo() const override { return stream_info_; } + void setDelayedCloseTimeout(std::chrono::milliseconds) override {} + absl::string_view transportFailureReason() const override { return EMPTY_STRING; } + + SyntheticReadCallbacks& parent_; + StreamInfo::StreamInfoImpl stream_info_; + Network::ConnectionSocket::OptionsSharedPtr options_; + }; + + HttpApiListener& parent_; + SyntheticConnection connection_; + }; + + const envoy::api::v2::Listener& config_; + ListenerManagerImpl& parent_; + const std::string name_; + Network::Address::InstanceConstSharedPtr address_; + ProtobufMessage::ValidationVisitor& validation_visitor_; + Stats::ScopePtr global_scope_; + Stats::ScopePtr listener_scope_; + SyntheticReadCallbacks read_callbacks_; + // Need to store the factory due to the shared_ptrs we need to keep alive: date provider, route + // config manager, scoped route config manager. + std::function + http_connection_manager_factory_; + Http::ServerConnectionCallbacksPtr http_connection_manager_; +}; + +} // namespace Server +} // namespace Envoy diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 890cd5b70750..71483c76ce55 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -110,7 +110,6 @@ class ValidationInstance final : Logger::Loggable, return validation_context_; } Configuration::ServerFactoryContext& serverFactoryContext() override { return server_context_; } - Http::ServerConnectionCallbacks* apiListener() override { return nullptr; } // Server::ListenerComponentFactory LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index fdfd700d3331..c9b7da94f87d 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -10,7 +10,6 @@ #include "common/common/assert.h" #include "common/config/utility.h" -#include "common/http/conn_manager_impl.h" #include "common/network/connection_balancer_impl.h" #include "common/network/resolver_impl.h" #include "common/network/socket_option_factory.h" @@ -25,7 +24,6 @@ #include "server/well_known_names.h" #include "extensions/filters/listener/well_known_names.h" -#include "extensions/filters/network/http_connection_manager/config.h" #include "extensions/transport_sockets/well_known_names.h" namespace Envoy { @@ -399,71 +397,5 @@ void ListenerImpl::setSocketFactory(const Network::ListenSocketFactorySharedPtr& socket_factory_ = socket_factory; } -////////////////// API Listener ////////////////////////// -ApiListenerImpl::ApiListenerImpl(const envoy::api::v2::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor) - : config_(config), parent_(parent), name_(name), - address_(Network::Address::resolveProtoAddress(config.address())), - validation_visitor_(validation_visitor), - global_scope_(parent_.server_.stats().createScope("")), - listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), - read_callbacks_(SyntheticReadCallbacks(*this)) { - ENVOY_LOG(error, "In API listener constructor"); - http_connection_manager_factory_ = - Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: - createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), *this); - ENVOY_LOG(error, "Created lambda"); -} - -AccessLog::AccessLogManager& ApiListenerImpl::accessLogManager() { - return parent_.server_.accessLogManager(); -} -Upstream::ClusterManager& ApiListenerImpl::clusterManager() { - return parent_.server_.clusterManager(); -} -Event::Dispatcher& ApiListenerImpl::dispatcher() { return parent_.server_.dispatcher(); } -Network::DrainDecision& ApiListenerImpl::drainDecision() { return *this; } -Grpc::Context& ApiListenerImpl::grpcContext() { return parent_.server_.grpcContext(); } -bool ApiListenerImpl::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } -Tracing::HttpTracer& ApiListenerImpl::httpTracer() { return httpContext().tracer(); } -Http::Context& ApiListenerImpl::httpContext() { return parent_.server_.httpContext(); } -Init::Manager& ApiListenerImpl::initManager() { return parent_.server_.initManager(); } -const LocalInfo::LocalInfo& ApiListenerImpl::localInfo() const { - return parent_.server_.localInfo(); -} -Envoy::Runtime::RandomGenerator& ApiListenerImpl::random() { return parent_.server_.random(); } -Envoy::Runtime::Loader& ApiListenerImpl::runtime() { return parent_.server_.runtime(); } -Stats::Scope& ApiListenerImpl::scope() { return *global_scope_; } -Singleton::Manager& ApiListenerImpl::singletonManager() { - return parent_.server_.singletonManager(); -} -OverloadManager& ApiListenerImpl::overloadManager() { return parent_.server_.overloadManager(); } -ThreadLocal::Instance& ApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } -Admin& ApiListenerImpl::admin() { return parent_.server_.admin(); } -const envoy::api::v2::core::Metadata& ApiListenerImpl::listenerMetadata() const { - return config_.metadata(); -}; -envoy::api::v2::core::TrafficDirection ApiListenerImpl::direction() const { - return config_.traffic_direction(); -}; -TimeSource& ApiListenerImpl::timeSource() { return api().timeSource(); } - -ProtobufMessage::ValidationVisitor& ApiListenerImpl::messageValidationVisitor() { - return validation_visitor_; -} -Api::Api& ApiListenerImpl::api() { return parent_.server_.api(); } -ServerLifecycleNotifier& ApiListenerImpl::lifecycleNotifier() { - return parent_.server_.lifecycleNotifier(); -} -OptProcessContextRef ApiListenerImpl::processContext() { return parent_.server_.processContext(); } -Configuration::ServerFactoryContext& ApiListenerImpl::getServerFactoryContext() const { - return parent_.server_.serverFactoryContext(); -} -Stats::Scope& ApiListenerImpl::listenerScope() { return *listener_scope_; } - -// FIXME hook the api listener into listener draining in the manager. -bool ApiListenerImpl::drainClose() const { return false; } - } // namespace Server } // namespace Envoy diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index a9cd0ed92df0..115b5bba945f 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -11,10 +11,8 @@ #include "envoy/server/listener_manager.h" #include "envoy/stats/scope.h" -#include "common/common/empty_string.h" #include "common/common/logger.h" #include "common/init/manager_impl.h" -#include "common/stream_info/stream_info_impl.h" #include "server/filter_chain_manager_impl.h" @@ -250,138 +248,5 @@ class ListenerImpl : public Network::ListenerConfig, friend class ListenerFilterChainFactoryBuilder; }; -class ApiListenerImpl : public Configuration::FactoryContext, - public Network::DrainDecision, - Logger::Loggable { -public: - ApiListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, - const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor); - - Http::ServerConnectionCallbacks* apiHandle() { - if (!http_connection_manager_) { - ENVOY_LOG_MISC(error, "There was no hcm creating one"); - http_connection_manager_ = http_connection_manager_factory_(read_callbacks_); - ENVOY_LOG(error, "Exercised lambda"); - } - return http_connection_manager_.get(); - } - - const Network::Address::InstanceConstSharedPtr& address() const { return address_; } - - AccessLog::AccessLogManager& accessLogManager() override; - Upstream::ClusterManager& clusterManager() override; - Event::Dispatcher& dispatcher() override; - Network::DrainDecision& drainDecision() override; - Grpc::Context& grpcContext() override; - bool healthCheckFailed() override; - Tracing::HttpTracer& httpTracer() override; - Http::Context& httpContext() override; - Init::Manager& initManager() override; - const LocalInfo::LocalInfo& localInfo() const override; - Envoy::Runtime::RandomGenerator& random() override; - Envoy::Runtime::Loader& runtime() override; - Stats::Scope& scope() override; - Singleton::Manager& singletonManager() override; - OverloadManager& overloadManager() override; - ThreadLocal::Instance& threadLocal() override; - Admin& admin() override; - const envoy::api::v2::core::Metadata& listenerMetadata() const override; - envoy::api::v2::core::TrafficDirection direction() const override; - TimeSource& timeSource() override; - ProtobufMessage::ValidationVisitor& messageValidationVisitor() override; - Api::Api& api() override; - ServerLifecycleNotifier& lifecycleNotifier() override; - OptProcessContextRef processContext() override; - Configuration::ServerFactoryContext& getServerFactoryContext() const override; - Stats::Scope& listenerScope() override; - - bool drainClose() const override; - -private: - class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { - public: - SyntheticReadCallbacks(ApiListenerImpl& parent) - : parent_(parent), connection_(SyntheticConnection(*this)) {} - - // Network::ReadFilterCallbacks - void continueReading() override {} - void injectReadDataToFilterChain(Buffer::Instance&, bool) override {} - Upstream::HostDescriptionConstSharedPtr upstreamHost() override { return nullptr; } - void upstreamHost(Upstream::HostDescriptionConstSharedPtr) override {} - Network::Connection& connection() override { return connection_; } - - class SyntheticConnection : public Network::Connection { - public: - SyntheticConnection(SyntheticReadCallbacks& parent) - : parent_(parent), stream_info_(parent_.parent_.timeSource()), - options_(std::make_shared>()) {} - - // Network::FilterManager - void addWriteFilter(Network::WriteFilterSharedPtr) override {} - void addFilter(Network::FilterSharedPtr) override {} - void addReadFilter(Network::ReadFilterSharedPtr) override {} - bool initializeReadFilters() override { return true; } - - // Network::Connection - void addConnectionCallbacks(Network::ConnectionCallbacks&) override {} - void addBytesSentCallback(Network::Connection::BytesSentCb) override {} - void enableHalfClose(bool) override {} - void close(Network::ConnectionCloseType) override {} - Event::Dispatcher& dispatcher() override { return parent_.parent_.dispatcher(); } - uint64_t id() const override { return 12345; } - std::string nextProtocol() const override { return EMPTY_STRING; } - void noDelay(bool) override {} - void readDisable(bool) override {} - void detectEarlyCloseWhenReadDisabled(bool) override {} - bool readEnabled() const override { return true; } - const Network::Address::InstanceConstSharedPtr& remoteAddress() const override { - return parent_.parent_.address(); - } - absl::optional - unixSocketPeerCredentials() const override { - return absl::nullopt; - } - const Network::Address::InstanceConstSharedPtr& localAddress() const override { - return parent_.parent_.address(); - } - void setConnectionStats(const Network::Connection::ConnectionStats&) override {} - Ssl::ConnectionInfoConstSharedPtr ssl() const override { return nullptr; } - absl::string_view requestedServerName() const override { return EMPTY_STRING; } - State state() const override { return Network::Connection::State::Open; } - void write(Buffer::Instance&, bool) override {} - void setBufferLimits(uint32_t) override {} - uint32_t bufferLimit() const override { return 65000; } - bool localAddressRestored() const override { return false; } - bool aboveHighWatermark() const override { return false; } - const Network::ConnectionSocket::OptionsSharedPtr& socketOptions() const override { - return options_; - } - StreamInfo::StreamInfo& streamInfo() override { return stream_info_; } - const StreamInfo::StreamInfo& streamInfo() const override { return stream_info_; } - void setDelayedCloseTimeout(std::chrono::milliseconds) override {} - absl::string_view transportFailureReason() const override { return EMPTY_STRING; } - - SyntheticReadCallbacks& parent_; - StreamInfo::StreamInfoImpl stream_info_; - Network::ConnectionSocket::OptionsSharedPtr options_; - }; - - ApiListenerImpl& parent_; - SyntheticConnection connection_; - }; - - const envoy::api::v2::Listener& config_; - ListenerManagerImpl& parent_; - const std::string name_; - Network::Address::InstanceConstSharedPtr address_; - ProtobufMessage::ValidationVisitor& validation_visitor_; - Stats::ScopePtr global_scope_; - Stats::ScopePtr listener_scope_; - SyntheticReadCallbacks read_callbacks_; - std::function - http_connection_manager_factory_; - Http::ServerConnectionCallbacksPtr http_connection_manager_; -}; - } // namespace Server } // namespace Envoy diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index d9b79a505d8d..306d604d9309 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -28,7 +28,6 @@ #include "server/well_known_names.h" #include "extensions/filters/listener/well_known_names.h" -#include "extensions/filters/network/http_connection_manager/config.h" #include "extensions/transport_sockets/well_known_names.h" namespace Envoy { @@ -302,12 +301,35 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& config, const std::string& version_info, bool added_via_api) { - // FIXME initial experimentation where we only allow one API listener to exist and to come from - // bootstrap - if (!api_listener_ && config.has_api_listener()) { - ENVOY_LOG(error, "initializing API listener"); - api_listener_ = std::make_unique( - config, *this, config.name(), server_.messageValidationContext().staticValidationVisitor()); + // FIXME HttpApiListener is very prescriptive about "what" and API Listener is. + // Whereas the config is very non-prescriptive -- through the use of an Any. + // Perhaps this path should similar to several parts in the codebase that have registered + // factories for all the different types. The problem is that I can't quite see what the abstract + // type that we store is; it entirely depends on what the API surface of the API listener is. + if (config.has_api_listener()) { + ENVOY_LOG(error, "has api listener"); + // FIXME I propose that name should be required and unique for API listeners. Much like how + // address is required for a socket based listener. This is how Envoy can find different API + // listeners. + if (config.name().empty()) { + ENVOY_LOG(error, "the listener name is empty"); + // FIXME throwing exception is ok here? Yes for bootstrap. What happens to the exception on an + // LDS update? + } else { + // FIXME if the name is already there and the listener was added via API, then update. + auto it = api_listeners_.find(config.name()); + if (it != api_listeners_.end()) { + ENVOY_LOG(error, "this listener name already exists {}", config.name()); + // Update if updatable + } else { + // Add! + ENVOY_LOG(error, "Adding brand new listener {}", config.name()); + api_listeners_.emplace(config.name(), + std::make_unique( + config, *this, config.name(), + server_.messageValidationContext().staticValidationVisitor())); + } + } return true; } @@ -793,5 +815,16 @@ ListenerManagerImpl::createListenSocketFactory(const envoy::api::v2::core::Addre listener.bindToPort(), listener.name(), reuse_port); } +Http::ServerConnectionCallbacks* ListenerManagerImpl::apiListener(const std::string& name) { + auto it = api_listeners_.find(name); + if (it != api_listeners_.end()) { + ENVOY_LOG(error, "returning non-null"); + return it->second->apiHandle(); + } else { + ENVOY_LOG(error, "returning null"); + return nullptr; + } +} + } // namespace Server } // namespace Envoy diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 11523fab1641..f7a00ed30f0d 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -20,6 +20,8 @@ #include "server/filter_chain_manager_impl.h" #include "server/lds_api.h" #include "server/listener_impl.h" +#include "server/api_listener.h" + namespace Envoy { namespace Server { @@ -145,9 +147,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::LoggableapiHandle() : nullptr; - } + Http::ServerConnectionCallbacks* apiListener(const std::string& name) override; Instance& server_; ListenerComponentFactory& factory_; @@ -217,7 +217,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable api_listener_; + std::unordered_map> api_listeners_; // Active listeners are listeners that are currently accepting new connections on the workers. ListenerList active_listeners_; // Warming listeners are listeners that may need further initialization via the listener's init diff --git a/source/server/server.h b/source/server/server.h index d4ec7aeca437..13acc78b04eb 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -223,9 +223,6 @@ class InstanceImpl final : Logger::Loggable, ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() const override { return *local_info_; } TimeSource& timeSource() override { return time_source_; } - Http::ServerConnectionCallbacks* apiListener() override { - return listenerManager().apiListener(); - } Configuration::ServerFactoryContext& serverFactoryContext() override { return server_context_; } From cfa81d85d4d6801ccc06173d15a536ef142d3e1f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 26 Dec 2019 16:07:07 -0800 Subject: [PATCH 05/44] fmt Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 34 +++++++++---------- .../network/common/redis/codec_impl.cc | 4 +-- .../network/http_connection_manager/config.h | 2 +- source/server/api_listener.cc | 2 +- source/server/api_listener.h | 2 +- source/server/listener_manager_impl.h | 3 +- 6 files changed, 23 insertions(+), 24 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index a83a430d0200..c06c9c0428a8 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1385,23 +1385,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply(is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, - FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply( + is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 6b62e228bfa6..c7ebc51efa4b 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -315,8 +315,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( - const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator:: +operator!=(const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 258e2d29166a..6e6737873d15 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -10,8 +10,8 @@ #include "envoy/config/config_provider_manager.h" #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h" -#include "envoy/http/filter.h" #include "envoy/http/codec.h" +#include "envoy/http/filter.h" #include "envoy/router/route_config_provider_manager.h" #include "common/common/logger.h" diff --git a/source/server/api_listener.cc b/source/server/api_listener.cc index 358845288032..97afab465fec 100644 --- a/source/server/api_listener.cc +++ b/source/server/api_listener.cc @@ -5,8 +5,8 @@ #include "envoy/stats/scope.h" #include "common/http/conn_manager_impl.h" -#include "common/protobuf/utility.h" #include "common/network/resolver_impl.h" +#include "common/protobuf/utility.h" #include "server/drain_manager_impl.h" #include "server/listener_manager_impl.h" diff --git a/source/server/api_listener.h b/source/server/api_listener.h index 9718bb43c366..51c609282fd9 100644 --- a/source/server/api_listener.h +++ b/source/server/api_listener.h @@ -10,9 +10,9 @@ #include "envoy/server/listener_manager.h" #include "envoy/stats/scope.h" -#include "common/http/conn_manager_impl.h" #include "common/common/empty_string.h" #include "common/common/logger.h" +#include "common/http/conn_manager_impl.h" #include "common/init/manager_impl.h" #include "common/stream_info/stream_info_impl.h" diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index f7a00ed30f0d..e99cbe6eeb69 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -17,11 +17,10 @@ #include "envoy/server/worker.h" #include "envoy/stats/scope.h" +#include "server/api_listener.h" #include "server/filter_chain_manager_impl.h" #include "server/lds_api.h" #include "server/listener_impl.h" -#include "server/api_listener.h" - namespace Envoy { namespace Server { From 538d73c9ec1a183ce9d48d59920662b95b3e32ad Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 27 Dec 2019 15:43:30 -0800 Subject: [PATCH 06/44] back to singleton. ApiListener and ApiListenerHandle interfaces Signed-off-by: Jose Nino --- include/envoy/http/BUILD | 1 + include/envoy/http/codec.h | 4 +- include/envoy/server/BUILD | 5 + include/envoy/server/api_listener.h | 29 ++++++ include/envoy/server/listener_manager.h | 17 ++-- source/server/BUILD | 7 +- source/server/api_listener.cc | 90 ------------------ source/server/api_listener_impl.cc | 95 +++++++++++++++++++ .../{api_listener.h => api_listener_impl.h} | 25 ++--- source/server/listener_manager_impl.cc | 48 +++------- source/server/listener_manager_impl.h | 6 +- 11 files changed, 174 insertions(+), 153 deletions(-) create mode 100644 include/envoy/server/api_listener.h delete mode 100644 source/server/api_listener.cc create mode 100644 source/server/api_listener_impl.cc rename source/server/{api_listener.h => api_listener_impl.h} (90%) diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index e722ab74d3b7..8e1a616f896a 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -30,6 +30,7 @@ envoy_cc_library( ":protocol_interface", "//include/envoy/buffer:buffer_interface", "//include/envoy/network:address_interface", + "//include/envoy/server:api_listener_interface", ], ) diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 1ef5b3ff441c..ff3a3e9a87d2 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -10,6 +10,7 @@ #include "envoy/http/metadata_interface.h" #include "envoy/http/protocol.h" #include "envoy/network/address.h" +#include "envoy/server/api_listener.h" namespace Envoy { namespace Http { @@ -413,7 +414,8 @@ class DownstreamWatermarkCallbacks { /** * Callbacks for server connections. */ -class ServerConnectionCallbacks : public virtual ConnectionCallbacks { +class ServerConnectionCallbacks : public virtual ConnectionCallbacks, + public virtual Server::ApiListenerHandle { public: /** * Invoked when a new request stream is initiated by the remote. diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 657643e74a86..32ad5820d503 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -31,6 +31,11 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "api_listener_interface", + hdrs = ["api_listener.h"], +) + envoy_cc_library( name = "configuration_interface", hdrs = ["configuration.h"], diff --git a/include/envoy/server/api_listener.h b/include/envoy/server/api_listener.h new file mode 100644 index 000000000000..9728d39da2d1 --- /dev/null +++ b/include/envoy/server/api_listener.h @@ -0,0 +1,29 @@ +#pragma once + +namespace Envoy { +namespace Server { + +/** + * Handle provided to users to interact with Envoy via a particular API. + */ +class ApiListenerHandle { +public: + virtual ~ApiListenerHandle() = default; +}; + +/** + * Listener that provides an API to interact with Envoy via a handle. + */ +class ApiListener { +public: + virtual ~ApiListener() = default; + + virtual absl::string_view name() const PURE; + + virtual ApiListenerHandle* handle() PURE; +}; + +using ApiListenerPtr = std::unique_ptr; + +} // namespace Server +} // namespace Envoy \ No newline at end of file diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 571fdc6301cb..4204d7171df7 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -212,19 +212,14 @@ class ListenerManager { using FailureStates = std::vector>; virtual void endListenerUpdate(FailureStates&& failure_states) PURE; - // FIXME figure out what type to return here. An idea I had is to provide a handle that connects - // to Envoy's owned listener via a weak ptr -- similar to the init manager handles. This way API - // calls can be safe without owning the underlying listener. Moreover, it might be the case that - // while this function returns a handle, the caller needs prior knowledge about the API surface - // that the handle provides so they can downcast to the actual "API listener" surface. This could - // also help with the type that the listener manager stores, i.e it stores "ApiListener" and the - // only thing that that interface knows how to do is to return a safe handle. - /** - * @name name of the API Listener to search for. - * @return Http::ServerConnectionCallbacks* a handle to the API Listener if it exists, nullptr if + // TODO(junr03): once Api Listeners support warming and draining, this function should return a + // weak ptr to its caller. This would allow the caller to verify is the listener is + // available to receive API calls on it. + /** + * @return ApiListenerHandle* a handle to the API Listener if it exists, nullptr if * it does not. */ - virtual Http::ServerConnectionCallbacks* apiListener(const std::string& name) PURE; + virtual ApiListenerHandle* apiListener() PURE; }; } // namespace Server diff --git a/source/server/BUILD b/source/server/BUILD index 57a02f023ccd..a296dc97d81b 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -267,15 +267,16 @@ envoy_cc_library( envoy_cc_library( name = "api_listener_lib", srcs = [ - "api_listener.cc", + "api_listener_impl.cc", ], hdrs = [ - "api_listener.h", + "api_listener_impl.h", ], deps = [ ":drain_manager_lib", ":listener_manager_impl", "//include/envoy/network:connection_interface", + "//include/envoy/server:api_listener_interface", "//include/envoy/server:filter_config_interface", "//include/envoy/server:listener_manager_interface", "//source/common/common:empty_string", @@ -332,7 +333,7 @@ envoy_cc_library( "listener_manager_impl.cc", ], hdrs = [ - "api_listener.h", + "api_listener_impl.h", "listener_impl.h", "listener_manager_impl.h", ], diff --git a/source/server/api_listener.cc b/source/server/api_listener.cc deleted file mode 100644 index 97afab465fec..000000000000 --- a/source/server/api_listener.cc +++ /dev/null @@ -1,90 +0,0 @@ -#include "server/api_listener.h" - -#include "envoy/api/v2/lds.pb.h" -#include "envoy/api/v2/listener/listener.pb.h" -#include "envoy/stats/scope.h" - -#include "common/http/conn_manager_impl.h" -#include "common/network/resolver_impl.h" -#include "common/protobuf/utility.h" - -#include "server/drain_manager_impl.h" -#include "server/listener_manager_impl.h" - -#include "extensions/filters/network/http_connection_manager/config.h" - -namespace Envoy { -namespace Server { - -HttpApiListener::HttpApiListener(const envoy::api::v2::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor) - : config_(config), parent_(parent), name_(name), - address_(Network::Address::resolveProtoAddress(config.address())), - validation_visitor_(validation_visitor), - global_scope_(parent_.server_.stats().createScope("")), - listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), - read_callbacks_(SyntheticReadCallbacks(*this)) { - ENVOY_LOG(error, "In API listener constructor"); - http_connection_manager_factory_ = - Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: - createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), *this); - ENVOY_LOG(error, "Created lambda"); -} - -Http::ServerConnectionCallbacks* HttpApiListener::apiHandle() { - if (!http_connection_manager_) { - http_connection_manager_ = http_connection_manager_factory_(read_callbacks_); - } - return http_connection_manager_.get(); -} - -AccessLog::AccessLogManager& HttpApiListener::accessLogManager() { - return parent_.server_.accessLogManager(); -} -Upstream::ClusterManager& HttpApiListener::clusterManager() { - return parent_.server_.clusterManager(); -} -Event::Dispatcher& HttpApiListener::dispatcher() { return parent_.server_.dispatcher(); } -Network::DrainDecision& HttpApiListener::drainDecision() { return *this; } -Grpc::Context& HttpApiListener::grpcContext() { return parent_.server_.grpcContext(); } -bool HttpApiListener::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } -Tracing::HttpTracer& HttpApiListener::httpTracer() { return httpContext().tracer(); } -Http::Context& HttpApiListener::httpContext() { return parent_.server_.httpContext(); } -Init::Manager& HttpApiListener::initManager() { return parent_.server_.initManager(); } -const LocalInfo::LocalInfo& HttpApiListener::localInfo() const { - return parent_.server_.localInfo(); -} -Envoy::Runtime::RandomGenerator& HttpApiListener::random() { return parent_.server_.random(); } -Envoy::Runtime::Loader& HttpApiListener::runtime() { return parent_.server_.runtime(); } -Stats::Scope& HttpApiListener::scope() { return *global_scope_; } -Singleton::Manager& HttpApiListener::singletonManager() { - return parent_.server_.singletonManager(); -} -OverloadManager& HttpApiListener::overloadManager() { return parent_.server_.overloadManager(); } -ThreadLocal::Instance& HttpApiListener::threadLocal() { return parent_.server_.threadLocal(); } -Admin& HttpApiListener::admin() { return parent_.server_.admin(); } -const envoy::api::v2::core::Metadata& HttpApiListener::listenerMetadata() const { - return config_.metadata(); -}; -envoy::api::v2::core::TrafficDirection HttpApiListener::direction() const { - return config_.traffic_direction(); -}; -TimeSource& HttpApiListener::timeSource() { return api().timeSource(); } -ProtobufMessage::ValidationVisitor& HttpApiListener::messageValidationVisitor() { - return validation_visitor_; -} -Api::Api& HttpApiListener::api() { return parent_.server_.api(); } -ServerLifecycleNotifier& HttpApiListener::lifecycleNotifier() { - return parent_.server_.lifecycleNotifier(); -} -OptProcessContextRef HttpApiListener::processContext() { return parent_.server_.processContext(); } -Configuration::ServerFactoryContext& HttpApiListener::getServerFactoryContext() const { - return parent_.server_.serverFactoryContext(); -} -Stats::Scope& HttpApiListener::listenerScope() { return *listener_scope_; } -// FIXME hook the api listener into listener draining in the manager. -bool HttpApiListener::drainClose() const { return false; } - -} // namespace Server -} // namespace Envoy diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc new file mode 100644 index 000000000000..2c04c75bf0b5 --- /dev/null +++ b/source/server/api_listener_impl.cc @@ -0,0 +1,95 @@ +#include "server/api_listener_impl.h" + +#include "envoy/api/v2/lds.pb.h" +#include "envoy/api/v2/listener/listener.pb.h" +#include "envoy/stats/scope.h" + +#include "common/http/conn_manager_impl.h" +#include "common/network/resolver_impl.h" +#include "common/protobuf/utility.h" + +#include "server/drain_manager_impl.h" +#include "server/listener_manager_impl.h" + +#include "extensions/filters/network/http_connection_manager/config.h" + +namespace Envoy { +namespace Server { + +HttpApiListenerImpl::HttpApiListenerImpl(const envoy::api::v2::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor) + : config_(config), parent_(parent), name_(name), + address_(Network::Address::resolveProtoAddress( + config.address())), // TODO(junr03): consider moving the SyntheticAddressImpl from Envoy + // Mobile to Envoy. + validation_visitor_(validation_visitor), + global_scope_(parent_.server_.stats().createScope("")), + listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), + read_callbacks_(SyntheticReadCallbacks(*this)) { + ENVOY_LOG(error, "In API listener constructor"); + http_connection_manager_factory_ = + Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: + createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), *this); + ENVOY_LOG(error, "Created lambda"); +} + +ApiListenerHandle* HttpApiListenerImpl::handle() { + if (!http_connection_manager_) { + http_connection_manager_ = http_connection_manager_factory_(read_callbacks_); + } + return http_connection_manager_.get(); +} + +AccessLog::AccessLogManager& HttpApiListenerImpl::accessLogManager() { + return parent_.server_.accessLogManager(); +} +Upstream::ClusterManager& HttpApiListenerImpl::clusterManager() { + return parent_.server_.clusterManager(); +} +Event::Dispatcher& HttpApiListenerImpl::dispatcher() { return parent_.server_.dispatcher(); } +Network::DrainDecision& HttpApiListenerImpl::drainDecision() { return *this; } +Grpc::Context& HttpApiListenerImpl::grpcContext() { return parent_.server_.grpcContext(); } +bool HttpApiListenerImpl::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } +Tracing::HttpTracer& HttpApiListenerImpl::httpTracer() { return httpContext().tracer(); } +Http::Context& HttpApiListenerImpl::httpContext() { return parent_.server_.httpContext(); } +Init::Manager& HttpApiListenerImpl::initManager() { return parent_.server_.initManager(); } +const LocalInfo::LocalInfo& HttpApiListenerImpl::localInfo() const { + return parent_.server_.localInfo(); +} +Envoy::Runtime::RandomGenerator& HttpApiListenerImpl::random() { return parent_.server_.random(); } +Envoy::Runtime::Loader& HttpApiListenerImpl::runtime() { return parent_.server_.runtime(); } +Stats::Scope& HttpApiListenerImpl::scope() { return *global_scope_; } +Singleton::Manager& HttpApiListenerImpl::singletonManager() { + return parent_.server_.singletonManager(); +} +OverloadManager& HttpApiListenerImpl::overloadManager() { + return parent_.server_.overloadManager(); +} +ThreadLocal::Instance& HttpApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } +Admin& HttpApiListenerImpl::admin() { return parent_.server_.admin(); } +const envoy::api::v2::core::Metadata& HttpApiListenerImpl::listenerMetadata() const { + return config_.metadata(); +}; +envoy::api::v2::core::TrafficDirection HttpApiListenerImpl::direction() const { + return config_.traffic_direction(); +}; +TimeSource& HttpApiListenerImpl::timeSource() { return api().timeSource(); } +ProtobufMessage::ValidationVisitor& HttpApiListenerImpl::messageValidationVisitor() { + return validation_visitor_; +} +Api::Api& HttpApiListenerImpl::api() { return parent_.server_.api(); } +ServerLifecycleNotifier& HttpApiListenerImpl::lifecycleNotifier() { + return parent_.server_.lifecycleNotifier(); +} +OptProcessContextRef HttpApiListenerImpl::processContext() { + return parent_.server_.processContext(); +} +Configuration::ServerFactoryContext& HttpApiListenerImpl::getServerFactoryContext() const { + return parent_.server_.serverFactoryContext(); +} +Stats::Scope& HttpApiListenerImpl::listenerScope() { return *listener_scope_; } +bool HttpApiListenerImpl::drainClose() const { return false; } + +} // namespace Server +} // namespace Envoy diff --git a/source/server/api_listener.h b/source/server/api_listener_impl.h similarity index 90% rename from source/server/api_listener.h rename to source/server/api_listener_impl.h index 51c609282fd9..39f49b675135 100644 --- a/source/server/api_listener.h +++ b/source/server/api_listener_impl.h @@ -5,6 +5,7 @@ #include "envoy/api/v2/lds.pb.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" +#include "envoy/server/api_listener.h" #include "envoy/server/drain_manager.h" #include "envoy/server/filter_config.h" #include "envoy/server/listener_manager.h" @@ -16,8 +17,6 @@ #include "common/init/manager_impl.h" #include "common/stream_info/stream_info_impl.h" -#include "extensions/filters/network/http_connection_manager/config.h" - namespace Envoy { namespace Server { @@ -27,17 +26,21 @@ class ListenerManagerImpl; * Listener that provides a handle to inject HTTP calls into envoy via the traditional HCM path. * Thus it provides full access to Envoy's L7 features, e.g HTTP filters. */ -class HttpApiListener : public Configuration::FactoryContext, - public Network::DrainDecision, - Logger::Loggable { +class HttpApiListenerImpl : public ApiListener, + public Configuration::FactoryContext, + public Network::DrainDecision, + Logger::Loggable { public: - HttpApiListener(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, - const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor); + HttpApiListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, + const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor); - // see thoughts on what this API handle could be at server/listener_manager.h - Http::ServerConnectionCallbacks* apiHandle(); const Network::Address::InstanceConstSharedPtr& address() const { return address_; } + // ApiListener + absl::string_view name() const override { return name_; } + ApiListenerHandle* handle() override; + // TODO(junr03): the majority of this surface could be moved out of the listener via some sort of // base class context. // Server::Configuration::FactoryContext @@ -73,7 +76,7 @@ class HttpApiListener : public Configuration::FactoryContext, private: class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { public: - SyntheticReadCallbacks(HttpApiListener& parent) + SyntheticReadCallbacks(HttpApiListenerImpl& parent) : parent_(parent), connection_(SyntheticConnection(*this)) {} // Network::ReadFilterCallbacks @@ -139,7 +142,7 @@ class HttpApiListener : public Configuration::FactoryContext, Network::ConnectionSocket::OptionsSharedPtr options_; }; - HttpApiListener& parent_; + HttpApiListenerImpl& parent_; SyntheticConnection connection_; }; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 45b874055895..161ab5b6b715 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -26,6 +26,7 @@ #include "server/filter_chain_manager_impl.h" #include "server/transport_socket_config_impl.h" #include "server/well_known_names.h" +#include "server/api_listener_impl.h" #include "extensions/filters/listener/well_known_names.h" #include "extensions/transport_sockets/well_known_names.h" @@ -306,36 +307,22 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& config, const std::string& version_info, bool added_via_api) { - // FIXME HttpApiListener is very prescriptive about "what" and API Listener is. + // FIXME HttpApiListenerImpl is very prescriptive about "what" and API Listener is. // Whereas the config is very non-prescriptive -- through the use of an Any. // Perhaps this path should similar to several parts in the codebase that have registered // factories for all the different types. The problem is that I can't quite see what the abstract // type that we store is; it entirely depends on what the API surface of the API listener is. - if (config.has_api_listener()) { - ENVOY_LOG(error, "has api listener"); - // FIXME I propose that name should be required and unique for API listeners. Much like how - // address is required for a socket based listener. This is how Envoy can find different API - // listeners. - if (config.name().empty()) { - ENVOY_LOG(error, "the listener name is empty"); - // FIXME throwing exception is ok here? Yes for bootstrap. What happens to the exception on an - // LDS update? - } else { - // FIXME if the name is already there and the listener was added via API, then update. - auto it = api_listeners_.find(config.name()); - if (it != api_listeners_.end()) { - ENVOY_LOG(error, "this listener name already exists {}", config.name()); - // Update if updatable - } else { - // Add! - ENVOY_LOG(error, "Adding brand new listener {}", config.name()); - api_listeners_.emplace(config.name(), - std::make_unique( - config, *this, config.name(), - server_.messageValidationContext().staticValidationVisitor())); - } - } + // TODO(junr03): currently only one ApiListener can be installed via bootstrap to avoid having to + // build a collection of listeners, and to have to be able to warm and drain the listeners. In the + // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. + if (!api_listener_ && !added_via_api && config.has_api_listener()) { + api_listener_ = std::make_unique( + config, *this, config.name(), server_.messageValidationContext().staticValidationVisitor()); return true; + } else { + ENVOY_LOG(debug, "listener {} can not be added because currently only one ApiListener is " + "allowed, and it can only be added via bootstrap configuration"); + return false; } std::string name; @@ -820,15 +807,8 @@ ListenerManagerImpl::createListenSocketFactory(const envoy::api::v2::core::Addre listener.bindToPort(), listener.name(), reuse_port); } -Http::ServerConnectionCallbacks* ListenerManagerImpl::apiListener(const std::string& name) { - auto it = api_listeners_.find(name); - if (it != api_listeners_.end()) { - ENVOY_LOG(error, "returning non-null"); - return it->second->apiHandle(); - } else { - ENVOY_LOG(error, "returning null"); - return nullptr; - } +ApiListenerHandle* ListenerManagerImpl::apiListener() { + return api_listener_ ? api_listener_->handle() : nullptr; } } // namespace Server diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index e99cbe6eeb69..edb31e16159a 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -10,6 +10,7 @@ #include "envoy/api/v2/listener/listener.pb.h" #include "envoy/network/filter.h" #include "envoy/network/listen_socket.h" +#include "envoy/server/api_listener.h" #include "envoy/server/filter_config.h" #include "envoy/server/instance.h" #include "envoy/server/listener_manager.h" @@ -17,7 +18,6 @@ #include "envoy/server/worker.h" #include "envoy/stats/scope.h" -#include "server/api_listener.h" #include "server/filter_chain_manager_impl.h" #include "server/lds_api.h" #include "server/listener_impl.h" @@ -146,7 +146,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable> api_listeners_; + ApiListenerPtr api_listener_; // Active listeners are listeners that are currently accepting new connections on the workers. ListenerList active_listeners_; // Warming listeners are listeners that may need further initialization via the listener's init From ad8134859b2d2940bc25517559fd22e909ad6456 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 27 Dec 2019 15:51:42 -0800 Subject: [PATCH 07/44] fmt Signed-off-by: Jose Nino --- source/server/listener_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 161ab5b6b715..b458a3b12244 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -21,12 +21,12 @@ #include "common/network/utility.h" #include "common/protobuf/utility.h" +#include "server/api_listener_impl.h" #include "server/configuration_impl.h" #include "server/drain_manager_impl.h" #include "server/filter_chain_manager_impl.h" #include "server/transport_socket_config_impl.h" #include "server/well_known_names.h" -#include "server/api_listener_impl.h" #include "extensions/filters/listener/well_known_names.h" #include "extensions/transport_sockets/well_known_names.h" From e6179183052e9efdfd069231accbd70804c2ae19 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 27 Dec 2019 17:07:45 -0800 Subject: [PATCH 08/44] comments Signed-off-by: Jose Nino --- include/envoy/server/api_listener.h | 10 +++++ include/envoy/server/listener_manager.h | 6 +-- source/common/http/conn_manager_impl.cc | 44 ++++++++++--------- source/common/http/conn_manager_impl.h | 12 +++-- .../network/common/redis/codec_impl.cc | 8 ++-- 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/include/envoy/server/api_listener.h b/include/envoy/server/api_listener.h index 9728d39da2d1..2ca0e055b264 100644 --- a/include/envoy/server/api_listener.h +++ b/include/envoy/server/api_listener.h @@ -5,6 +5,8 @@ namespace Server { /** * Handle provided to users to interact with Envoy via a particular API. + * Given the flexibility of the api_listener config, this interface is also maximally flexible. + * Consumers of this interface have to cast this handle to narrower types. */ class ApiListenerHandle { public: @@ -18,8 +20,16 @@ class ApiListener { public: virtual ~ApiListener() = default; + /** + * An ApiListener is uniquely identified by its name. + * + * @return the name of the ApiListener. + */ virtual absl::string_view name() const PURE; + /** + * @return a handle for interaction, nullptr if the Listener can not construct one. + */ virtual ApiListenerHandle* handle() PURE; }; diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 4204d7171df7..7e92ef7f9d52 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -212,9 +212,9 @@ class ListenerManager { using FailureStates = std::vector>; virtual void endListenerUpdate(FailureStates&& failure_states) PURE; - // TODO(junr03): once Api Listeners support warming and draining, this function should return a - // weak ptr to its caller. This would allow the caller to verify is the listener is - // available to receive API calls on it. + // TODO(junr03): once ApiListeners support warming and draining, this function should return a + // weak_ptr of the ApiListenerHandle to its caller. This would allow the caller to verify if the + // ApiListener that backs the handle is available to receive API calls on it. /** * @return ApiListenerHandle* a handle to the API Listener if it exists, nullptr if * it does not. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index c06c9c0428a8..510f6aa0920d 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -118,9 +118,7 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, overload_manager ? overload_manager->getThreadLocalOverloadState().getState( Server::OverloadActionNames::get().DisableHttpKeepAlive) : Server::OverloadManager::getInactiveState()), - time_source_(time_source) { - ENVOY_LOG(error, "HCM constructor"); -} + time_source_(time_source) {} const HeaderMapImpl& ConnectionManagerImpl::continueHeader() { CONSTRUCT_ON_FIRST_USE(HeaderMapImpl, @@ -287,6 +285,11 @@ void ConnectionManagerImpl::handleCodecException(const char* error) { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWriteAndDelay); } +void ConnectionManagerImpl::forceCodecCreation() { + ASSERT(!codec_); + codec_ = config_.createCodec(read_callbacks_->connection(), Buffer::OwnedImpl(), *this); +} + Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. @@ -497,7 +500,6 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource(), connection_manager.filterState()), upstream_options_(std::make_shared()) { - ENVOY_LOG_MISC(error, "Creating ActiveStream"); ASSERT(!connection_manager.config_.isRoutable() || ((connection_manager.config_.routeConfigProvider() == nullptr && connection_manager.config_.scopedRouteConfigProvider() != nullptr) || @@ -1385,23 +1387,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply( - is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply(is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, + FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 8ba5d65c07d8..e3bb2a1ee633 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -66,10 +66,14 @@ class ConnectionManagerImpl : Logger::Loggable, Stats::Scope& scope); static const HeaderMapImpl& continueHeader(); - void forceCodecCreation() { - ASSERT(!codec_); - codec_ = config_.createCodec(read_callbacks_->connection(), Buffer::OwnedImpl(), *this); - } + // Currently the ConnectionManager creates a codec lazily when either: + // a) onConnection for H3. + // b) onData for H1 and H2. + // With the introduction of ApiListeners, neither even occurs. This function allows us to force + // create a codec. + // TODO(junr03): consider passing a synthetic codec instead of creating once. The codec in the + // ApiListener case is solely used to determine the protocol version. + void forceCodecCreation(); // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index c7ebc51efa4b..221a5ac87ad1 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -315,8 +315,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator:: -operator!=(const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( + const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } @@ -383,9 +383,7 @@ void DecoderImpl::parseSlice(const Buffer::RawSlice& slice) { pending_value_stack_.front().value_->type(RespType::Integer); break; } - default: { - throw ProtocolError("invalid value type"); - } + default: { throw ProtocolError("invalid value type"); } } remaining--; From efe5cffa29485444c6591382c4c231b95cb83c0f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 27 Dec 2019 17:15:51 -0800 Subject: [PATCH 09/44] comments split up Signed-off-by: Jose Nino --- .../network/http_connection_manager/config.cc | 20 +++++++++---------- source/server/api_listener_impl.cc | 5 +---- source/server/api_listener_impl.h | 14 +++++++++++-- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index e7f1ed7bcbeb..ce7e506fcf9c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -6,6 +6,8 @@ #include #include "envoy/api/v2/core/base.pb.h" +#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" +#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h" #include "envoy/filesystem/filesystem.h" #include "envoy/server/admin.h" #include "envoy/tracing/http_tracer.h" @@ -499,7 +501,6 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( auto typed_config = MessageUtil::anyConvert< envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager>( proto_config); - ENVOY_LOG_MISC(error, "Downcasted proto successfully"); std::shared_ptr hcm_date_provider = context.singletonManager().getTyped( @@ -507,7 +508,6 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( return std::make_shared(context.dispatcher(), context.threadLocal()); }); - ENVOY_LOG_MISC(error, "Built date provider"); std::shared_ptr hcm_route_config_provider_manager = context.singletonManager().getTyped( @@ -515,8 +515,6 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( return std::make_shared(context.admin()); }); - ENVOY_LOG_MISC(error, "Built route config provider"); - std::shared_ptr hcm_scoped_routes_config_provider_manager = context.singletonManager().getTyped( @@ -525,19 +523,15 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( return std::make_shared( context.admin(), *hcm_route_config_provider_manager); }); - ENVOY_LOG_MISC(error, "built scoped route provider"); std::shared_ptr filter_config(new HttpConnectionManagerConfig( typed_config, context, *hcm_date_provider, *hcm_route_config_provider_manager, *hcm_scoped_routes_config_provider_manager)); - ENVOY_LOG_MISC(error, "built config"); // This lambda captures the shared_ptrs created above, thus preserving the // reference count. // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine // as these captured objects are also global singletons. - ENVOY_LOG_MISC(error, "about to return lambda"); - return [hcm_scoped_routes_config_provider_manager, hcm_route_config_provider_manager, hcm_date_provider, filter_config, &context]( Network::ReadFilterCallbacks& read_callbacks) -> Http::ServerConnectionCallbacksPtr { @@ -548,11 +542,15 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( context.runtime(), context.localInfo(), context.clusterManager(), &context.overloadManager(), context.dispatcher().timeSource()); - // Additional actions taken in the normal path. + // This factory creates a new ConnectionManagerImpl in the absence of its usual environment as + // an L4 filter, so this factory needs to take a few actions. // When a new connection is creating its filter chain it hydrates the factory with a filter - // manager which provides the hcm with the "read_callbacks". + // manager which provides the ConnectionManager with its "read_callbacks". conn_manager->initializeReadFilterCallbacks(read_callbacks); - // When the connection first sends data through the hcm, the hcm creates a codec. + ENVOY_LOG_MISC(error, "initialized"); + + // When the connection first calls onData on the ConnectionManager, the ConnectionManager + // creates a codec. Here we force create a codec as onData will not be called. conn_manager->forceCodecCreation(); return conn_manager; diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index 2c04c75bf0b5..f718f507cb7a 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -20,9 +20,7 @@ HttpApiListenerImpl::HttpApiListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor) : config_(config), parent_(parent), name_(name), - address_(Network::Address::resolveProtoAddress( - config.address())), // TODO(junr03): consider moving the SyntheticAddressImpl from Envoy - // Mobile to Envoy. + address_(Network::Address::resolveProtoAddress(config.address())), validation_visitor_(validation_visitor), global_scope_(parent_.server_.stats().createScope("")), listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), @@ -89,7 +87,6 @@ Configuration::ServerFactoryContext& HttpApiListenerImpl::getServerFactoryContex return parent_.server_.serverFactoryContext(); } Stats::Scope& HttpApiListenerImpl::listenerScope() { return *listener_scope_; } -bool HttpApiListenerImpl::drainClose() const { return false; } } // namespace Server } // namespace Envoy diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index 39f49b675135..c8f3e6b7dced 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -23,7 +23,7 @@ namespace Server { class ListenerManagerImpl; /** - * Listener that provides a handle to inject HTTP calls into envoy via the traditional HCM path. + * Listener that provides a handle to inject HTTP calls into envoy via an Http::ConnectionManager. * Thus it provides full access to Envoy's L7 features, e.g HTTP filters. */ class HttpApiListenerImpl : public ApiListener, @@ -35,6 +35,8 @@ class HttpApiListenerImpl : public ApiListener, const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor); + // TODO(junr03): consider moving Envoy Mobile's SyntheticAddressImpl to Envoy in order to return + // that rather than this semi-real one. const Network::Address::InstanceConstSharedPtr& address() const { return address_; } // ApiListener @@ -71,9 +73,15 @@ class HttpApiListenerImpl : public ApiListener, Configuration::ServerFactoryContext& getServerFactoryContext() const override; Stats::Scope& listenerScope() override; - bool drainClose() const override; + // Network::DrainDecision + // TODO(junr03): hook up draining to listener state management. + bool drainClose() const override { return false; } private: + // Synthetic class that acts as a stub Network::ReadFilterCallbacks. + // TODO(junr03): if we are able to separate the Network Filter aspects of the + // Http::ConnectionManagerImpl from the http management aspects of it, it is possible we would not + // need this and the SyntheticConnection stub anymore. class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { public: SyntheticReadCallbacks(HttpApiListenerImpl& parent) @@ -86,6 +94,8 @@ class HttpApiListenerImpl : public ApiListener, void upstreamHost(Upstream::HostDescriptionConstSharedPtr) override {} Network::Connection& connection() override { return connection_; } + // Synthetic class that acts as a stub for the connection backing the + // Network::ReadFilterCallbacks. class SyntheticConnection : public Network::Connection { public: SyntheticConnection(SyntheticReadCallbacks& parent) From 2760a218e8b20d6b3f96067959c1fb5896f8a941 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 27 Dec 2019 17:24:12 -0800 Subject: [PATCH 10/44] move read callbacks outside of the lambda Signed-off-by: Jose Nino --- .../network/http_connection_manager/config.cc | 16 ++++++++-------- .../network/http_connection_manager/config.h | 8 ++++++-- source/server/api_listener_impl.cc | 14 ++++++-------- source/server/api_listener_impl.h | 8 ++++---- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index ce7e506fcf9c..891cd8b2868e 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -494,10 +494,12 @@ SINGLETON_MANAGER_REGISTRATION(hcm_date_provider); SINGLETON_MANAGER_REGISTRATION(hcm_route_config_provider_manager); SINGLETON_MANAGER_REGISTRATION(hcm_scoped_routes_config_provider_manager); -std::function +// TODO(junr03): some of this code can be DRYed up and shared with the other factory code. Clean up +// if this factory approach is well received by reviewers. +std::function HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( - const ProtobufWkt::Any& proto_config, Server::Configuration::FactoryContext& context) { - ENVOY_LOG_MISC(error, "In factory"); + const ProtobufWkt::Any& proto_config, Server::Configuration::FactoryContext& context, + Network::ReadFilterCallbacks& read_callbacks) { auto typed_config = MessageUtil::anyConvert< envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager>( proto_config); @@ -533,10 +535,8 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine // as these captured objects are also global singletons. return [hcm_scoped_routes_config_provider_manager, hcm_route_config_provider_manager, - hcm_date_provider, filter_config, &context]( - Network::ReadFilterCallbacks& read_callbacks) -> Http::ServerConnectionCallbacksPtr { - ENVOY_LOG_MISC(error, "Inside of factory lambda"); - + hcm_date_provider, filter_config, &context, + &read_callbacks]() -> Http::ServerConnectionCallbacksPtr { auto conn_manager = std::make_unique( *filter_config, context.drainDecision(), context.random(), context.httpContext(), context.runtime(), context.localInfo(), context.clusterManager(), @@ -544,10 +544,10 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( // This factory creates a new ConnectionManagerImpl in the absence of its usual environment as // an L4 filter, so this factory needs to take a few actions. + // When a new connection is creating its filter chain it hydrates the factory with a filter // manager which provides the ConnectionManager with its "read_callbacks". conn_manager->initializeReadFilterCallbacks(read_callbacks); - ENVOY_LOG_MISC(error, "initialized"); // When the connection first calls onData on the ConnectionManager, the ConnectionManager // creates a codec. Here we force create a codec as onData will not be called. diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 6e6737873d15..b53df749a3ab 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -200,11 +200,15 @@ class HttpConnectionManagerConfig : Logger::Loggable, static const uint64_t RequestTimeoutMs = 0; }; +/** + * Factory to create an HttpConnectionManager outside of a Network Filter Chain. + */ class HttpConnectionManagerFactory { public: - static std::function + static std::function createHttpConnectionManagerFactoryFromProto(const ProtobufWkt::Any& proto_config, - Server::Configuration::FactoryContext& context); + Server::Configuration::FactoryContext& context, + Network::ReadFilterCallbacks& read_callbacks); }; } // namespace HttpConnectionManager diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index f718f507cb7a..93a444601da2 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -24,17 +24,15 @@ HttpApiListenerImpl::HttpApiListenerImpl(const envoy::api::v2::Listener& config, validation_visitor_(validation_visitor), global_scope_(parent_.server_.stats().createScope("")), listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), - read_callbacks_(SyntheticReadCallbacks(*this)) { - ENVOY_LOG(error, "In API listener constructor"); - http_connection_manager_factory_ = - Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: - createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), *this); - ENVOY_LOG(error, "Created lambda"); -} + read_callbacks_(SyntheticReadCallbacks(*this)), + http_connection_manager_factory_( + Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: + createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), + *this, read_callbacks_)) {} ApiListenerHandle* HttpApiListenerImpl::handle() { if (!http_connection_manager_) { - http_connection_manager_ = http_connection_manager_factory_(read_callbacks_); + http_connection_manager_ = http_connection_manager_factory_(); } return http_connection_manager_.get(); } diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index c8f3e6b7dced..ed01fbc4a84c 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -164,10 +164,10 @@ class HttpApiListenerImpl : public ApiListener, Stats::ScopePtr global_scope_; Stats::ScopePtr listener_scope_; SyntheticReadCallbacks read_callbacks_; - // Need to store the factory due to the shared_ptrs we need to keep alive: date provider, route - // config manager, scoped route config manager. - std::function - http_connection_manager_factory_; + // Need to store the factory due to the shared_ptrs that need to be kept alive: date provider, + // route config manager, scoped route config manager. + std::function http_connection_manager_factory_; + // Http::ServerConnectionCallbacks is the API surface that this class provides via its handle(). Http::ServerConnectionCallbacksPtr http_connection_manager_; }; From 1a57cf0bf2f6bfc62081c0992bcbcb962a88a2f0 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 28 Dec 2019 13:07:51 -0800 Subject: [PATCH 11/44] comment Signed-off-by: Jose Nino --- source/server/listener_manager_impl.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index b458a3b12244..dddf3237b86c 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -307,11 +307,6 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& config, const std::string& version_info, bool added_via_api) { - // FIXME HttpApiListenerImpl is very prescriptive about "what" and API Listener is. - // Whereas the config is very non-prescriptive -- through the use of an Any. - // Perhaps this path should similar to several parts in the codebase that have registered - // factories for all the different types. The problem is that I can't quite see what the abstract - // type that we store is; it entirely depends on what the API surface of the API listener is. // TODO(junr03): currently only one ApiListener can be installed via bootstrap to avoid having to // build a collection of listeners, and to have to be able to warm and drain the listeners. In the // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. From d74abb6604b783e411e8248392369831b5cc888f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 28 Dec 2019 13:15:05 -0800 Subject: [PATCH 12/44] fmt Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 34 ++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 510f6aa0920d..d3ad48824b60 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1387,23 +1387,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply(is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, - FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply( + is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( From 5684186c79889a747927fed9a61f7bd746857d5d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 28 Dec 2019 13:17:40 -0800 Subject: [PATCH 13/44] change api listener to implemented Signed-off-by: Jose Nino --- api/envoy/config/listener/v2/api_listener.proto | 1 - 1 file changed, 1 deletion(-) diff --git a/api/envoy/config/listener/v2/api_listener.proto b/api/envoy/config/listener/v2/api_listener.proto index 65da5efa56c5..af0d0a3dbcb8 100644 --- a/api/envoy/config/listener/v2/api_listener.proto +++ b/api/envoy/config/listener/v2/api_listener.proto @@ -8,7 +8,6 @@ option java_package = "io.envoyproxy.envoy.config.listener.v2"; option java_outer_classname = "ApiListenerProto"; option java_multiple_files = true; -// [#not-implemented-hide:] // Describes a type of API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. message ApiListener { From 1427f3ed9ab1dc940a3c92b17743cea85dae0a1d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 28 Dec 2019 16:09:24 -0800 Subject: [PATCH 14/44] fixes Signed-off-by: Jose Nino --- api/envoy/config/listener/v2/api_listener.proto | 3 +-- test/mocks/server/mocks.h | 1 + test/server/BUILD | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/listener/v2/api_listener.proto b/api/envoy/config/listener/v2/api_listener.proto index af0d0a3dbcb8..16adb4473c80 100644 --- a/api/envoy/config/listener/v2/api_listener.proto +++ b/api/envoy/config/listener/v2/api_listener.proto @@ -12,8 +12,7 @@ option java_multiple_files = true; // exposed to the non-proxy application depends on the type of API listener. message ApiListener { // The type in this field determines the type of API listener. At present, the following - // types are supported: - // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) + // types are supported: envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) // [#next-major-version: In the v3 API, replace this Any field with a oneof containing the // specific config message for each type of API listener. We could not do this in v2 because // it would have caused circular dependencies for go protos: lds.proto depends on this file, diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 93d9c60e58c8..15c70b7cc531 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -288,6 +288,7 @@ class MockListenerManager : public ListenerManager { MOCK_METHOD0(stopWorkers, void()); MOCK_METHOD0(beginListenerUpdate, void()); MOCK_METHOD1(endListenerUpdate, void(ListenerManager::FailureStates&&)); + MOCK_METHOD0(apiListener, ApiListenerHandle*()); }; class MockServerLifecycleNotifier : public ServerLifecycleNotifier { diff --git a/test/server/BUILD b/test/server/BUILD index 8c4c13f19ea3..4df16e95a28f 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -180,6 +180,7 @@ envoy_cc_test_library( hdrs = ["listener_manager_impl_test.h"], data = ["//test/extensions/transport_sockets/tls/test_data:certs"], deps = [ + "//source/server:api_listener_lib", "//source/server:listener_lib", "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", From 3dd0b138dc2666e0ee66fb21a2269327901aeef3 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 2 Jan 2020 11:27:17 -0800 Subject: [PATCH 15/44] fmt Signed-off-by: Jose Nino --- api/envoy/config/listener/v2/api_listener.proto | 3 ++- .../extensions/filters/network/common/redis/codec_impl.cc | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/listener/v2/api_listener.proto b/api/envoy/config/listener/v2/api_listener.proto index 16adb4473c80..44ef190600a5 100644 --- a/api/envoy/config/listener/v2/api_listener.proto +++ b/api/envoy/config/listener/v2/api_listener.proto @@ -12,7 +12,8 @@ option java_multiple_files = true; // exposed to the non-proxy application depends on the type of API listener. message ApiListener { // The type in this field determines the type of API listener. At present, the following - // types are supported: envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) + // types are supported: + // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) // [#next-major-version: In the v3 API, replace this Any field with a oneof containing the // specific config message for each type of API listener. We could not do this in v2 because // it would have caused circular dependencies for go protos: lds.proto depends on this file, diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 221a5ac87ad1..c7ebc51efa4b 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -315,8 +315,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( - const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator:: +operator!=(const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } @@ -383,7 +383,9 @@ void DecoderImpl::parseSlice(const Buffer::RawSlice& slice) { pending_value_stack_.front().value_->type(RespType::Integer); break; } - default: { throw ProtocolError("invalid value type"); } + default: { + throw ProtocolError("invalid value type"); + } } remaining--; From 9f6094bbcd0459c650b105fd1062ff75c9f66a20 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 2 Jan 2020 12:50:05 -0800 Subject: [PATCH 16/44] fmt, with clang-format 9 Signed-off-by: Jose Nino --- source/extensions/filters/network/common/redis/codec_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index c7ebc51efa4b..6b62e228bfa6 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -315,8 +315,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator:: -operator!=(const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( + const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } From 65e3a71e09ab64e21125167f1f912cc63aeddd7e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 2 Jan 2020 13:31:48 -0800 Subject: [PATCH 17/44] does this work with proto formatting? Signed-off-by: Jose Nino --- .../envoy/config/listener/v2/api_listener.proto | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/generated_api_shadow/envoy/config/listener/v2/api_listener.proto b/generated_api_shadow/envoy/config/listener/v2/api_listener.proto index 65da5efa56c5..44ef190600a5 100644 --- a/generated_api_shadow/envoy/config/listener/v2/api_listener.proto +++ b/generated_api_shadow/envoy/config/listener/v2/api_listener.proto @@ -8,13 +8,12 @@ option java_package = "io.envoyproxy.envoy.config.listener.v2"; option java_outer_classname = "ApiListenerProto"; option java_multiple_files = true; -// [#not-implemented-hide:] // Describes a type of API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. message ApiListener { // The type in this field determines the type of API listener. At present, the following // types are supported: - // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) + // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) // [#next-major-version: In the v3 API, replace this Any field with a oneof containing the // specific config message for each type of API listener. We could not do this in v2 because // it would have caused circular dependencies for go protos: lds.proto depends on this file, From c9f429b6d31376c82dd50e2836c529a5e7c68a04 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 2 Jan 2020 14:12:44 -0800 Subject: [PATCH 18/44] change to implemented Signed-off-by: Jose Nino --- api/envoy/api/v2/lds.proto | 1 - api/envoy/api/v3alpha/lds.proto | 1 - source/server/listener_manager_impl.cc | 19 +++++++++++-------- test/server/listener_manager_impl_test.cc | 2 +- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/api/envoy/api/v2/lds.proto b/api/envoy/api/v2/lds.proto index 1fdf25616dbf..6c9b4a887a94 100644 --- a/api/envoy/api/v2/lds.proto +++ b/api/envoy/api/v2/lds.proto @@ -219,7 +219,6 @@ message Listener { // creating a packet-oriented UDP listener. If not present, treat it as "raw_udp_listener". listener.UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for :ref:`name` diff --git a/api/envoy/api/v3alpha/lds.proto b/api/envoy/api/v3alpha/lds.proto index b0ca49b15534..1abd5d7a716c 100644 --- a/api/envoy/api/v3alpha/lds.proto +++ b/api/envoy/api/v3alpha/lds.proto @@ -216,7 +216,6 @@ message Listener { // "raw_udp_listener". listener.UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index b7c98536e3a7..c477375748b8 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -310,14 +310,17 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& co // TODO(junr03): currently only one ApiListener can be installed via bootstrap to avoid having to // build a collection of listeners, and to have to be able to warm and drain the listeners. In the // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. - if (!api_listener_ && !added_via_api && config.has_api_listener()) { - api_listener_ = std::make_unique( - config, *this, config.name(), server_.messageValidationContext().staticValidationVisitor()); - return true; - } else { - ENVOY_LOG(debug, "listener {} can not be added because currently only one ApiListener is " - "allowed, and it can only be added via bootstrap configuration"); - return false; + if (config.has_api_listener()) { + if (!api_listener_ && !added_via_api) { + api_listener_ = std::make_unique( + config, *this, config.name(), + server_.messageValidationContext().staticValidationVisitor()); + return true; + } else { + ENVOY_LOG(debug, "listener {} can not be added because currently only one ApiListener is " + "allowed, and it can only be added via bootstrap configuration"); + return false; + } } std::string name; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 18722fdeab6b..35af26bfdfc0 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -3565,7 +3565,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, VerifySanWithNoCA) { - certificate_chain: { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem" } private_key: { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_key.pem" } validation_context: - match_subject_alt_names: + match_subject_alt_names: exact: "spiffe://lyft.com/testclient" )EOF", Network::Address::IpVersion::v4); From dad95f69a8198397c014c6a88cf6374010d9c34c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 3 Jan 2020 09:44:15 -0800 Subject: [PATCH 19/44] remove acronym Signed-off-by: Jose Nino --- .../filters/network/http_connection_manager/config.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 891cd8b2868e..135dd2c127a5 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -494,7 +494,7 @@ SINGLETON_MANAGER_REGISTRATION(hcm_date_provider); SINGLETON_MANAGER_REGISTRATION(hcm_route_config_provider_manager); SINGLETON_MANAGER_REGISTRATION(hcm_scoped_routes_config_provider_manager); -// TODO(junr03): some of this code can be DRYed up and shared with the other factory code. Clean up +// TODO(junr03): some of this code can be shared with the other factory code. Clean up // if this factory approach is well received by reviewers. std::function HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( From 25d949448399dfb8f334c7f2052708f0cea9a432 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 3 Jan 2020 09:51:19 -0800 Subject: [PATCH 20/44] generated Signed-off-by: Jose Nino --- generated_api_shadow/envoy/api/v2/lds.proto | 1 - generated_api_shadow/envoy/service/listener/v3alpha/lds.proto | 1 - 2 files changed, 2 deletions(-) diff --git a/generated_api_shadow/envoy/api/v2/lds.proto b/generated_api_shadow/envoy/api/v2/lds.proto index 2a4e30d3f481..2dbc00c227da 100644 --- a/generated_api_shadow/envoy/api/v2/lds.proto +++ b/generated_api_shadow/envoy/api/v2/lds.proto @@ -221,7 +221,6 @@ message Listener { // creating a packet-oriented UDP listener. If not present, treat it as "raw_udp_listener". listener.UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for :ref:`name` diff --git a/generated_api_shadow/envoy/service/listener/v3alpha/lds.proto b/generated_api_shadow/envoy/service/listener/v3alpha/lds.proto index 5d758ea7c7d1..69e27b2ed060 100644 --- a/generated_api_shadow/envoy/service/listener/v3alpha/lds.proto +++ b/generated_api_shadow/envoy/service/listener/v3alpha/lds.proto @@ -235,7 +235,6 @@ message Listener { // "raw_udp_listener". config.listener.v3alpha.UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for From 542ceda5cfa3043dd44ba9a7052974465c5fbd4d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 6 Jan 2020 11:03:06 -0800 Subject: [PATCH 21/44] missing bazel deps Signed-off-by: Jose Nino --- source/server/BUILD | 1 + test/server/BUILD | 1 + 2 files changed, 2 insertions(+) diff --git a/source/server/BUILD b/source/server/BUILD index de0f45b911e9..f28043226e1a 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -156,6 +156,7 @@ envoy_cc_library( srcs = envoy_select_hot_restart(["hot_restarting_parent.cc"]), hdrs = envoy_select_hot_restart(["hot_restarting_parent.h"]), deps = [ + ":api_listener_lib", ":hot_restarting_base", ":listener_lib", "//source/common/memory:stats_lib", diff --git a/test/server/BUILD b/test/server/BUILD index 8efe239daa7f..65a05daf325c 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -259,6 +259,7 @@ envoy_cc_test( "//source/extensions/transport_sockets/raw_buffer:config", "//source/extensions/transport_sockets/tls:config", "//source/extensions/transport_sockets/tls:ssl_socket_lib", + "//source/server:api_listener_lib", "//source/server:filter_chain_manager_lib", "//source/server:listener_lib", "//test/mocks/network:network_mocks", From ae1442c2d5afe577a233172fc29cda98e9441173 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 6 Jan 2020 12:07:13 -0800 Subject: [PATCH 22/44] finish updating from merge Signed-off-by: Jose Nino --- api/envoy/api/v2/listener.proto | 1 - api/envoy/config/listener/v3alpha/listener.proto | 1 - generated_api_shadow/envoy/api/v2/listener.proto | 1 - .../envoy/config/listener/v3alpha/listener.proto | 1 - .../network/http_connection_manager/config.cc | 2 +- source/server/BUILD | 2 ++ source/server/api_listener_impl.cc | 6 +++--- source/server/api_listener_impl.h | 13 +++++++------ source/server/listener_manager_impl.cc | 6 +++--- 9 files changed, 16 insertions(+), 17 deletions(-) diff --git a/api/envoy/api/v2/listener.proto b/api/envoy/api/v2/listener.proto index e2dbdee72178..094e61e0e666 100644 --- a/api/envoy/api/v2/listener.proto +++ b/api/envoy/api/v2/listener.proto @@ -202,7 +202,6 @@ message Listener { // creating a packet-oriented UDP listener. If not present, treat it as "raw_udp_listener". listener.UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for :ref:`name` diff --git a/api/envoy/config/listener/v3alpha/listener.proto b/api/envoy/config/listener/v3alpha/listener.proto index 5e36b5557ec3..73cf80ab6994 100644 --- a/api/envoy/config/listener/v3alpha/listener.proto +++ b/api/envoy/config/listener/v3alpha/listener.proto @@ -197,7 +197,6 @@ message Listener { // "raw_udp_listener". UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for diff --git a/generated_api_shadow/envoy/api/v2/listener.proto b/generated_api_shadow/envoy/api/v2/listener.proto index e2dbdee72178..094e61e0e666 100644 --- a/generated_api_shadow/envoy/api/v2/listener.proto +++ b/generated_api_shadow/envoy/api/v2/listener.proto @@ -202,7 +202,6 @@ message Listener { // creating a packet-oriented UDP listener. If not present, treat it as "raw_udp_listener". listener.UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for :ref:`name` diff --git a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto index 14cce83bd7e9..3b7dfe4bbff5 100644 --- a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto @@ -213,7 +213,6 @@ message Listener { // "raw_udp_listener". UdpListenerConfig udp_listener_config = 18; - // [#not-implemented-hide:] // Used to represent an API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index ac87d7505883..6e5a8dc6a2ab 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -510,7 +510,7 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( const ProtobufWkt::Any& proto_config, Server::Configuration::FactoryContext& context, Network::ReadFilterCallbacks& read_callbacks) { auto typed_config = MessageUtil::anyConvert< - envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager>( + envoy::extensions::filters::network::http_connection_manager::v3alpha::HttpConnectionManager>( proto_config); std::shared_ptr hcm_date_provider = diff --git a/source/server/BUILD b/source/server/BUILD index f28043226e1a..417f002ae976 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -290,6 +290,8 @@ envoy_cc_library( "//source/extensions/filters/network/http_connection_manager:config", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/api/v2/listener:pkg_cc_proto", + "@envoy_api//envoy/config/core/v3alpha:pkg_cc_proto", + "@envoy_api//envoy/config/listener/v3alpha:pkg_cc_proto", ], ) diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index 93a444601da2..504cb0a91d3e 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -16,7 +16,7 @@ namespace Envoy { namespace Server { -HttpApiListenerImpl::HttpApiListenerImpl(const envoy::api::v2::Listener& config, +HttpApiListenerImpl::HttpApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, ListenerManagerImpl& parent, const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor) : config_(config), parent_(parent), name_(name), @@ -64,10 +64,10 @@ OverloadManager& HttpApiListenerImpl::overloadManager() { } ThreadLocal::Instance& HttpApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } Admin& HttpApiListenerImpl::admin() { return parent_.server_.admin(); } -const envoy::api::v2::core::Metadata& HttpApiListenerImpl::listenerMetadata() const { +const envoy::config::core::v3alpha::Metadata& HttpApiListenerImpl::listenerMetadata() const { return config_.metadata(); }; -envoy::api::v2::core::TrafficDirection HttpApiListenerImpl::direction() const { +envoy::config::core::v3alpha::TrafficDirection HttpApiListenerImpl::direction() const { return config_.traffic_direction(); }; TimeSource& HttpApiListenerImpl::timeSource() { return api().timeSource(); } diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index ed01fbc4a84c..9edbddf4faa0 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -2,7 +2,8 @@ #include -#include "envoy/api/v2/lds.pb.h" +#include "envoy/config/core/v3alpha/base.pb.h" +#include "envoy/config/listener/v3alpha/listener.pb.h" #include "envoy/network/connection.h" #include "envoy/network/filter.h" #include "envoy/server/api_listener.h" @@ -31,8 +32,8 @@ class HttpApiListenerImpl : public ApiListener, public Network::DrainDecision, Logger::Loggable { public: - HttpApiListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent, - const std::string& name, + HttpApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, + ListenerManagerImpl& parent, const std::string& name, ProtobufMessage::ValidationVisitor& validation_visitor); // TODO(junr03): consider moving Envoy Mobile's SyntheticAddressImpl to Envoy in order to return @@ -63,8 +64,8 @@ class HttpApiListenerImpl : public ApiListener, OverloadManager& overloadManager() override; ThreadLocal::Instance& threadLocal() override; Admin& admin() override; - const envoy::api::v2::core::Metadata& listenerMetadata() const override; - envoy::api::v2::core::TrafficDirection direction() const override; + const envoy::config::core::v3alpha::Metadata& listenerMetadata() const override; + envoy::config::core::v3alpha::TrafficDirection direction() const override; TimeSource& timeSource() override; ProtobufMessage::ValidationVisitor& messageValidationVisitor() override; Api::Api& api() override; @@ -156,7 +157,7 @@ class HttpApiListenerImpl : public ApiListener, SyntheticConnection connection_; }; - const envoy::api::v2::Listener& config_; + const envoy::config::listener::v3alpha::Listener& config_; ListenerManagerImpl& parent_; const std::string name_; Network::Address::InstanceConstSharedPtr address_; diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index b528cab5d7ae..37ee53a7bb5b 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -307,9 +307,9 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { return {ALL_LISTENER_MANAGER_STATS(POOL_COUNTER(scope), POOL_GAUGE(scope))}; } -<<<<<<< HEAD -bool ListenerManagerImpl::addOrUpdateListener(envoy::config::listener::v3alpha::Listener& config, - const std::string& version_info, bool added_via_api) { +bool ListenerManagerImpl::addOrUpdateListener( + const envoy::config::listener::v3alpha::Listener& config, const std::string& version_info, + bool added_via_api) { // TODO(junr03): currently only one ApiListener can be installed via bootstrap to avoid having to // build a collection of listeners, and to have to be able to warm and drain the listeners. In the From 1df8a3e50dd48fb1f47a774a8a556960d37c601b Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 7 Jan 2020 09:53:23 -0800 Subject: [PATCH 23/44] comments Signed-off-by: Jose Nino --- api/envoy/api/v2/listener.proto | 5 + .../config/listener/v3alpha/listener.proto | 5 + include/envoy/http/BUILD | 7 +- include/envoy/http/api_listener.h | 30 ++++++ include/envoy/http/codec.h | 6 +- include/envoy/server/BUILD | 2 + include/envoy/server/api_listener.h | 25 +++-- include/envoy/server/listener_manager.h | 10 +- source/common/http/BUILD | 1 + source/common/http/conn_manager_impl.cc | 8 +- source/common/http/conn_manager_impl.h | 8 +- .../network/http_connection_manager/config.cc | 102 +++++++++--------- .../network/http_connection_manager/config.h | 35 +++++- source/server/api_listener_impl.cc | 95 ++++++++-------- source/server/api_listener_impl.h | 72 ++++++++----- source/server/listener_impl.h | 1 - source/server/listener_manager_impl.cc | 8 +- source/server/listener_manager_impl.h | 2 +- test/mocks/http/BUILD | 9 ++ test/mocks/http/api_listener.cc | 10 ++ test/mocks/http/api_listener.h | 20 ++++ test/mocks/server/mocks.h | 2 +- tools/proto_format.sh | 2 +- 23 files changed, 298 insertions(+), 167 deletions(-) create mode 100644 include/envoy/http/api_listener.h create mode 100644 test/mocks/http/api_listener.cc create mode 100644 test/mocks/http/api_listener.h diff --git a/api/envoy/api/v2/listener.proto b/api/envoy/api/v2/listener.proto index 094e61e0e666..826cd0ad9666 100644 --- a/api/envoy/api/v2/listener.proto +++ b/api/envoy/api/v2/listener.proto @@ -206,6 +206,11 @@ message Listener { // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for :ref:`name` // should be set. + // + // .. note:: + // + // Currently only one ApiListener can be installed via bootstrap + // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types // are in the ApiListener message, the socket listener messages should be in their own message, diff --git a/api/envoy/config/listener/v3alpha/listener.proto b/api/envoy/config/listener/v3alpha/listener.proto index 73cf80ab6994..8336d21e887c 100644 --- a/api/envoy/config/listener/v3alpha/listener.proto +++ b/api/envoy/config/listener/v3alpha/listener.proto @@ -201,6 +201,11 @@ message Listener { // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for // :ref:`name` should be set. + // + // .. note:: + // + // Currently only one ApiListener can be installed via bootstrap + // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types // are in the ApiListener message, the socket listener messages should be in their own message, diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index e9098257901d..0fbff9d5eee1 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -8,6 +8,12 @@ load( envoy_package() +envoy_cc_library( + name = "api_listener_interface", + hdrs = ["api_listener.h"], + deps = [":codec_interface"], +) + envoy_cc_library( name = "async_client_interface", hdrs = ["async_client.h"], @@ -30,7 +36,6 @@ envoy_cc_library( ":protocol_interface", "//include/envoy/buffer:buffer_interface", "//include/envoy/network:address_interface", - "//include/envoy/server:api_listener_interface", ], ) diff --git a/include/envoy/http/api_listener.h b/include/envoy/http/api_listener.h new file mode 100644 index 000000000000..01c7059da317 --- /dev/null +++ b/include/envoy/http/api_listener.h @@ -0,0 +1,30 @@ +#pragma once + +#include "envoy/http/codec.h" + +namespace Envoy { +namespace Http { + +/** + * ApiListener that allows consumers to interact with HTTP streams via API calls. + */ +class ApiListener { +public: + virtual ~ApiListener() = default; + + /** + * Invoked when a new request stream is initiated by the remote. + * @param response_encoder supplies the encoder to use for creating the response. The request and + * response are backed by the same Stream object. + * @param is_internally_created indicates if this stream was originated by a + * client, or was created by Envoy, by example as part of an internal redirect. + * @return StreamDecoder& supplies the decoder callbacks to fire into for stream decoding events. + */ + virtual StreamDecoder& newStream(StreamEncoder& response_encoder, + bool is_internally_created = false) PURE; +}; + +using ApiListenerPtr = std::unique_ptr; + +} // namespace Http +} // namespace Envoy \ No newline at end of file diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index ff3a3e9a87d2..55841b7e1425 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -10,7 +10,6 @@ #include "envoy/http/metadata_interface.h" #include "envoy/http/protocol.h" #include "envoy/network/address.h" -#include "envoy/server/api_listener.h" namespace Envoy { namespace Http { @@ -414,8 +413,7 @@ class DownstreamWatermarkCallbacks { /** * Callbacks for server connections. */ -class ServerConnectionCallbacks : public virtual ConnectionCallbacks, - public virtual Server::ApiListenerHandle { +class ServerConnectionCallbacks : public virtual ConnectionCallbacks { public: /** * Invoked when a new request stream is initiated by the remote. @@ -429,8 +427,6 @@ class ServerConnectionCallbacks : public virtual ConnectionCallbacks, bool is_internally_created = false) PURE; }; -using ServerConnectionCallbacksPtr = std::unique_ptr; - /** * A server side HTTP connection. */ diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 746d20636104..4faf0dd3b738 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -34,6 +34,7 @@ envoy_cc_library( envoy_cc_library( name = "api_listener_interface", hdrs = ["api_listener.h"], + deps = ["//include/envoy/http:api_listener_interface"], ) envoy_cc_library( @@ -192,6 +193,7 @@ envoy_cc_library( name = "listener_manager_interface", hdrs = ["listener_manager.h"], deps = [ + ":api_listener_interface", ":drain_manager_interface", ":filter_config_interface", ":guarddog_interface", diff --git a/include/envoy/server/api_listener.h b/include/envoy/server/api_listener.h index 2ca0e055b264..4a3d8139ac61 100644 --- a/include/envoy/server/api_listener.h +++ b/include/envoy/server/api_listener.h @@ -1,23 +1,17 @@ #pragma once +#include "envoy/http/api_listener.h" + namespace Envoy { namespace Server { /** - * Handle provided to users to interact with Envoy via a particular API. - * Given the flexibility of the api_listener config, this interface is also maximally flexible. - * Consumers of this interface have to cast this handle to narrower types. - */ -class ApiListenerHandle { -public: - virtual ~ApiListenerHandle() = default; -}; - -/** - * Listener that provides an API to interact with Envoy via a handle. + * Listener that allows consumer to interact with Envoy via a designated API. */ class ApiListener { public: + enum class Type { HttpApiListener }; + virtual ~ApiListener() = default; /** @@ -28,9 +22,14 @@ class ApiListener { virtual absl::string_view name() const PURE; /** - * @return a handle for interaction, nullptr if the Listener can not construct one. + * @return the Type of the ApiListener. + */ + virtual Type type() const PURE; + + /** + * @return Http::ApiListener IFF type() == Type::HttpApiListener, otherwise nullptr. */ - virtual ApiListenerHandle* handle() PURE; + virtual Http::ApiListener* http() PURE; }; using ApiListenerPtr = std::unique_ptr; diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 1d0f586a140f..d7746c9753fe 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -9,6 +9,7 @@ #include "envoy/network/filter.h" #include "envoy/network/listen_socket.h" #include "envoy/network/listener.h" +#include "envoy/server/api_listener.h" #include "envoy/server/drain_manager.h" #include "envoy/server/filter_config.h" #include "envoy/server/guarddog.h" @@ -214,13 +215,12 @@ class ListenerManager { virtual void endListenerUpdate(FailureStates&& failure_states) PURE; // TODO(junr03): once ApiListeners support warming and draining, this function should return a - // weak_ptr of the ApiListenerHandle to its caller. This would allow the caller to verify if the - // ApiListener that backs the handle is available to receive API calls on it. + // weak_ptr to its caller. This would allow the caller to verify if the + // ApiListener is available to receive API calls on it. /** - * @return ApiListenerHandle* a handle to the API Listener if it exists, nullptr if - * it does not. + * @return ApiListener* the server's API Listener if it exists, nullptr if it does not. */ - virtual ApiListenerHandle* apiListener() PURE; + virtual ApiListener* apiListener() PURE; }; } // namespace Server diff --git a/source/common/http/BUILD b/source/common/http/BUILD index ac0e8960f5c6..bccdd7c76269 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -160,6 +160,7 @@ envoy_cc_library( "//include/envoy/common:time_interface", "//include/envoy/event:deferred_deletable", "//include/envoy/event:dispatcher_interface", + "//include/envoy/http:api_listener_interface", "//include/envoy/http:codec_interface", "//include/envoy/http:context_interface", "//include/envoy/http:filter_interface", diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index e459580e3f80..0e38abd9f886 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -285,15 +285,15 @@ void ConnectionManagerImpl::handleCodecException(const char* error) { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWriteAndDelay); } -void ConnectionManagerImpl::forceCodecCreation() { +void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { ASSERT(!codec_); - codec_ = config_.createCodec(read_callbacks_->connection(), Buffer::OwnedImpl(), *this); + codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); } Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. - codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); + createCodec(data); if (codec_->protocol() == Protocol::Http2) { stats_.named_.downstream_cx_http2_total_.inc(); stats_.named_.downstream_cx_http2_active_.inc(); @@ -361,7 +361,7 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { } // Only QUIC connection's stream_info_ specifies protocol. Buffer::OwnedImpl dummy; - codec_ = config_.createCodec(read_callbacks_->connection(), dummy, *this); + createCodec(dummy); ASSERT(codec_->protocol() == Protocol::Http3); stats_.named_.downstream_cx_http3_total_.inc(); stats_.named_.downstream_cx_http3_active_.inc(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index e3bb2a1ee633..9b38ac614124 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -11,6 +11,7 @@ #include "envoy/access_log/access_log.h" #include "envoy/common/scope_tracker.h" #include "envoy/event/deferred_deletable.h" +#include "envoy/http/api_listener.h" #include "envoy/http/codec.h" #include "envoy/http/codes.h" #include "envoy/http/context.h" @@ -48,7 +49,8 @@ namespace Http { class ConnectionManagerImpl : Logger::Loggable, public Network::ReadFilter, public ServerConnectionCallbacks, - public Network::ConnectionCallbacks { + public Network::ConnectionCallbacks, + public Http::ApiListener { public: ConnectionManagerImpl(ConnectionManagerConfig& config, const Network::DrainDecision& drain_close, Runtime::RandomGenerator& random_generator, Http::Context& http_context, @@ -69,11 +71,11 @@ class ConnectionManagerImpl : Logger::Loggable, // Currently the ConnectionManager creates a codec lazily when either: // a) onConnection for H3. // b) onData for H1 and H2. - // With the introduction of ApiListeners, neither even occurs. This function allows us to force + // With the introduction of ApiListeners, neither event occurs. This function allows us to force // create a codec. // TODO(junr03): consider passing a synthetic codec instead of creating once. The codec in the // ApiListener case is solely used to determine the protocol version. - void forceCodecCreation(); + void createCodec(Buffer::Instance& data); // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 6e5a8dc6a2ab..9ae9fa929f4f 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -18,7 +18,6 @@ #include "common/common/fmt.h" #include "common/config/utility.h" #include "common/http/conn_manager_utility.h" -#include "common/http/date_provider_impl.h" #include "common/http/default_server_string.h" #include "common/http/http1/codec_impl.h" #include "common/http/http2/codec_impl.h" @@ -26,8 +25,6 @@ #include "common/http/http3/well_known_names.h" #include "common/http/utility.h" #include "common/protobuf/utility.h" -#include "common/router/rds_impl.h" -#include "common/router/scoped_rds.h" #include "common/runtime/runtime_impl.h" #include "common/tracing/http_tracer_impl.h" @@ -78,11 +75,10 @@ SINGLETON_MANAGER_REGISTRATION(date_provider); SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager); SINGLETON_MANAGER_REGISTRATION(scoped_routes_config_provider_manager); -Network::FilterFactoryCb -HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( - const envoy::extensions::filters::network::http_connection_manager::v3alpha:: - HttpConnectionManager& proto_config, - Server::Configuration::FactoryContext& context) { +std::tuple, + std::shared_ptr, + std::shared_ptr> +Utility::createSingletons(Server::Configuration::FactoryContext& context) { std::shared_ptr date_provider = context.singletonManager().getTyped( SINGLETON_MANAGER_REGISTERED_NAME(date_provider), [&context] { @@ -104,9 +100,36 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( context.admin(), *route_config_provider_manager); }); - std::shared_ptr filter_config(new HttpConnectionManagerConfig( - proto_config, context, *date_provider, *route_config_provider_manager, - *scoped_routes_config_provider_manager)); + return std::make_tuple(date_provider, route_config_provider_manager, + scoped_routes_config_provider_manager); +} + +std::shared_ptr +Utility::createConfig(const envoy::extensions::filters::network::http_connection_manager::v3alpha:: + HttpConnectionManager& proto_config, + Server::Configuration::FactoryContext& context, + Http::DateProvider& date_provider, + Router::RouteConfigProviderManager& route_config_provider_manager, + Config::ConfigProviderManager& scoped_routes_config_provider_manager) { + return std::make_shared(proto_config, context, date_provider, + route_config_provider_manager, + scoped_routes_config_provider_manager); +} + +Network::FilterFactoryCb +HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( + const envoy::extensions::filters::network::http_connection_manager::v3alpha:: + HttpConnectionManager& proto_config, + Server::Configuration::FactoryContext& context) { + std::shared_ptr date_provider; + std::shared_ptr route_config_provider_manager; + std::shared_ptr scoped_routes_config_provider_manager; + std::tie(date_provider, route_config_provider_manager, scoped_routes_config_provider_manager) = + Utility::createSingletons(context); + + auto filter_config = + Utility::createConfig(proto_config, context, *date_provider, *route_config_provider_manager, + *scoped_routes_config_provider_manager); // This lambda captures the shared_ptrs created above, thus preserving the // reference count. @@ -498,54 +521,28 @@ const Network::Address::Instance& HttpConnectionManagerConfig::localAddress() { return *context_.localInfo().address(); } -// Singleton registration via macro defined in envoy/singleton/manager.h -SINGLETON_MANAGER_REGISTRATION(hcm_date_provider); -SINGLETON_MANAGER_REGISTRATION(hcm_route_config_provider_manager); -SINGLETON_MANAGER_REGISTRATION(hcm_scoped_routes_config_provider_manager); - -// TODO(junr03): some of this code can be shared with the other factory code. Clean up -// if this factory approach is well received by reviewers. -std::function +std::function HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( - const ProtobufWkt::Any& proto_config, Server::Configuration::FactoryContext& context, - Network::ReadFilterCallbacks& read_callbacks) { - auto typed_config = MessageUtil::anyConvert< - envoy::extensions::filters::network::http_connection_manager::v3alpha::HttpConnectionManager>( - proto_config); - - std::shared_ptr hcm_date_provider = - context.singletonManager().getTyped( - SINGLETON_MANAGER_REGISTERED_NAME(hcm_date_provider), [&context] { - return std::make_shared(context.dispatcher(), - context.threadLocal()); - }); - - std::shared_ptr hcm_route_config_provider_manager = - context.singletonManager().getTyped( - SINGLETON_MANAGER_REGISTERED_NAME(hcm_route_config_provider_manager), [&context] { - return std::make_shared(context.admin()); - }); + const envoy::extensions::filters::network::http_connection_manager::v3alpha:: + HttpConnectionManager& proto_config, + Server::Configuration::FactoryContext& context, Network::ReadFilterCallbacks& read_callbacks) { - std::shared_ptr - hcm_scoped_routes_config_provider_manager = - context.singletonManager().getTyped( - SINGLETON_MANAGER_REGISTERED_NAME(hcm_scoped_routes_config_provider_manager), - [&context, hcm_route_config_provider_manager] { - return std::make_shared( - context.admin(), *hcm_route_config_provider_manager); - }); + std::shared_ptr date_provider; + std::shared_ptr route_config_provider_manager; + std::shared_ptr scoped_routes_config_provider_manager; + std::tie(date_provider, route_config_provider_manager, scoped_routes_config_provider_manager) = + Utility::createSingletons(context); - std::shared_ptr filter_config(new HttpConnectionManagerConfig( - typed_config, context, *hcm_date_provider, *hcm_route_config_provider_manager, - *hcm_scoped_routes_config_provider_manager)); + auto filter_config = + Utility::createConfig(proto_config, context, *date_provider, *route_config_provider_manager, + *scoped_routes_config_provider_manager); // This lambda captures the shared_ptrs created above, thus preserving the // reference count. // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine // as these captured objects are also global singletons. - return [hcm_scoped_routes_config_provider_manager, hcm_route_config_provider_manager, - hcm_date_provider, filter_config, &context, - &read_callbacks]() -> Http::ServerConnectionCallbacksPtr { + return [scoped_routes_config_provider_manager, route_config_provider_manager, + date_provider, filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { auto conn_manager = std::make_unique( *filter_config, context.drainDecision(), context.random(), context.httpContext(), context.runtime(), context.localInfo(), context.clusterManager(), @@ -560,7 +557,8 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( // When the connection first calls onData on the ConnectionManager, the ConnectionManager // creates a codec. Here we force create a codec as onData will not be called. - conn_manager->forceCodecCreation(); + Buffer::OwnedImpl dummy; + conn_manager->createCodec(dummy); return conn_manager; }; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 4a8923a3c43a..a640979a231c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -14,8 +14,11 @@ #include "envoy/router/route_config_provider_manager.h" #include "common/common/logger.h" +#include "common/http/date_provider_impl.h" #include "common/http/conn_manager_impl.h" #include "common/json/json_loader.h" +#include "common/router/rds_impl.h" +#include "common/router/scoped_rds.h" #include "extensions/filters/network/common/factory_base.h" #include "extensions/filters/network/well_known_names.h" @@ -205,10 +208,34 @@ class HttpConnectionManagerConfig : Logger::Loggable, */ class HttpConnectionManagerFactory { public: - static std::function - createHttpConnectionManagerFactoryFromProto(const ProtobufWkt::Any& proto_config, - Server::Configuration::FactoryContext& context, - Network::ReadFilterCallbacks& read_callbacks); + static std::function createHttpConnectionManagerFactoryFromProto( + const envoy::extensions::filters::network::http_connection_manager::v3alpha:: + HttpConnectionManager& proto_config, + Server::Configuration::FactoryContext& context, Network::ReadFilterCallbacks& read_callbacks); +}; + +/** + * Utility class for shared logic between HTTP connection manager factories. + */ +class Utility { +public: + /** + * + */ + static std::tuple, + std::shared_ptr, + std::shared_ptr> + createSingletons(Server::Configuration::FactoryContext& context); + + /** + * + */ + static std::shared_ptr + createConfig(const envoy::extensions::filters::network::http_connection_manager::v3alpha:: + HttpConnectionManager& proto_config, + Server::Configuration::FactoryContext& context, Http::DateProvider& date_provider, + Router::RouteConfigProviderManager& route_config_provider_manager, + Config::ConfigProviderManager& scoped_routes_config_provider_manager); }; } // namespace HttpConnectionManager diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index 504cb0a91d3e..7fdfe82821ea 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -16,75 +16,80 @@ namespace Envoy { namespace Server { -HttpApiListenerImpl::HttpApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor) +ApiListenerImpl::ApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor) : config_(config), parent_(parent), name_(name), address_(Network::Address::resolveProtoAddress(config.address())), validation_visitor_(validation_visitor), global_scope_(parent_.server_.stats().createScope("")), listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), - read_callbacks_(SyntheticReadCallbacks(*this)), - http_connection_manager_factory_( - Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: - createHttpConnectionManagerFactoryFromProto(config.api_listener().api_listener(), - *this, read_callbacks_)) {} + read_callbacks_(SyntheticReadCallbacks(*this)) {} -ApiListenerHandle* HttpApiListenerImpl::handle() { - if (!http_connection_manager_) { - http_connection_manager_ = http_connection_manager_factory_(); - } - return http_connection_manager_.get(); -} - -AccessLog::AccessLogManager& HttpApiListenerImpl::accessLogManager() { +AccessLog::AccessLogManager& ApiListenerImpl::accessLogManager() { return parent_.server_.accessLogManager(); } -Upstream::ClusterManager& HttpApiListenerImpl::clusterManager() { +Upstream::ClusterManager& ApiListenerImpl::clusterManager() { return parent_.server_.clusterManager(); } -Event::Dispatcher& HttpApiListenerImpl::dispatcher() { return parent_.server_.dispatcher(); } -Network::DrainDecision& HttpApiListenerImpl::drainDecision() { return *this; } -Grpc::Context& HttpApiListenerImpl::grpcContext() { return parent_.server_.grpcContext(); } -bool HttpApiListenerImpl::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } -Tracing::HttpTracer& HttpApiListenerImpl::httpTracer() { return httpContext().tracer(); } -Http::Context& HttpApiListenerImpl::httpContext() { return parent_.server_.httpContext(); } -Init::Manager& HttpApiListenerImpl::initManager() { return parent_.server_.initManager(); } -const LocalInfo::LocalInfo& HttpApiListenerImpl::localInfo() const { +Event::Dispatcher& ApiListenerImpl::dispatcher() { return parent_.server_.dispatcher(); } +Network::DrainDecision& ApiListenerImpl::drainDecision() { return *this; } +Grpc::Context& ApiListenerImpl::grpcContext() { return parent_.server_.grpcContext(); } +bool ApiListenerImpl::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } +Tracing::HttpTracer& ApiListenerImpl::httpTracer() { return httpContext().tracer(); } +Http::Context& ApiListenerImpl::httpContext() { return parent_.server_.httpContext(); } +Init::Manager& ApiListenerImpl::initManager() { return parent_.server_.initManager(); } +const LocalInfo::LocalInfo& ApiListenerImpl::localInfo() const { return parent_.server_.localInfo(); } -Envoy::Runtime::RandomGenerator& HttpApiListenerImpl::random() { return parent_.server_.random(); } -Envoy::Runtime::Loader& HttpApiListenerImpl::runtime() { return parent_.server_.runtime(); } -Stats::Scope& HttpApiListenerImpl::scope() { return *global_scope_; } -Singleton::Manager& HttpApiListenerImpl::singletonManager() { +Envoy::Runtime::RandomGenerator& ApiListenerImpl::random() { return parent_.server_.random(); } +Envoy::Runtime::Loader& ApiListenerImpl::runtime() { return parent_.server_.runtime(); } +Stats::Scope& ApiListenerImpl::scope() { return *global_scope_; } +Singleton::Manager& ApiListenerImpl::singletonManager() { return parent_.server_.singletonManager(); } -OverloadManager& HttpApiListenerImpl::overloadManager() { - return parent_.server_.overloadManager(); -} -ThreadLocal::Instance& HttpApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } -Admin& HttpApiListenerImpl::admin() { return parent_.server_.admin(); } -const envoy::config::core::v3alpha::Metadata& HttpApiListenerImpl::listenerMetadata() const { +OverloadManager& ApiListenerImpl::overloadManager() { return parent_.server_.overloadManager(); } +ThreadLocal::Instance& ApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } +Admin& ApiListenerImpl::admin() { return parent_.server_.admin(); } +const envoy::config::core::v3alpha::Metadata& ApiListenerImpl::listenerMetadata() const { return config_.metadata(); }; -envoy::config::core::v3alpha::TrafficDirection HttpApiListenerImpl::direction() const { +envoy::config::core::v3alpha::TrafficDirection ApiListenerImpl::direction() const { return config_.traffic_direction(); }; -TimeSource& HttpApiListenerImpl::timeSource() { return api().timeSource(); } -ProtobufMessage::ValidationVisitor& HttpApiListenerImpl::messageValidationVisitor() { +TimeSource& ApiListenerImpl::timeSource() { return api().timeSource(); } +ProtobufMessage::ValidationVisitor& ApiListenerImpl::messageValidationVisitor() { return validation_visitor_; } -Api::Api& HttpApiListenerImpl::api() { return parent_.server_.api(); } -ServerLifecycleNotifier& HttpApiListenerImpl::lifecycleNotifier() { +Api::Api& ApiListenerImpl::api() { return parent_.server_.api(); } +ServerLifecycleNotifier& ApiListenerImpl::lifecycleNotifier() { return parent_.server_.lifecycleNotifier(); } -OptProcessContextRef HttpApiListenerImpl::processContext() { - return parent_.server_.processContext(); -} -Configuration::ServerFactoryContext& HttpApiListenerImpl::getServerFactoryContext() const { +OptProcessContextRef ApiListenerImpl::processContext() { return parent_.server_.processContext(); } +Configuration::ServerFactoryContext& ApiListenerImpl::getServerFactoryContext() const { return parent_.server_.serverFactoryContext(); } -Stats::Scope& HttpApiListenerImpl::listenerScope() { return *listener_scope_; } +Stats::Scope& ApiListenerImpl::listenerScope() { return *listener_scope_; } + +HttpApiListener::HttpApiListener(const envoy::config::listener::v3alpha::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor) + : ApiListenerImpl(config, parent, name, validation_visitor) { + auto typed_config = MessageUtil::anyConvert< + envoy::extensions::filters::network::http_connection_manager::v3alpha::HttpConnectionManager>( + config.api_listener().api_listener()); + + http_connection_manager_factory_ = + Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: + createHttpConnectionManagerFactoryFromProto(typed_config, *this, read_callbacks_); +} + +Http::ApiListener* HttpApiListener::http() { + if (!http_connection_manager_) { + http_connection_manager_ = http_connection_manager_factory_(); + } + return http_connection_manager_.get(); +} } // namespace Server } // namespace Envoy diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index 9edbddf4faa0..156d67ed712e 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -27,22 +27,17 @@ class ListenerManagerImpl; * Listener that provides a handle to inject HTTP calls into envoy via an Http::ConnectionManager. * Thus it provides full access to Envoy's L7 features, e.g HTTP filters. */ -class HttpApiListenerImpl : public ApiListener, - public Configuration::FactoryContext, - public Network::DrainDecision, - Logger::Loggable { +class ApiListenerImpl : public ApiListener, + public Configuration::FactoryContext, + public Network::DrainDecision, + Logger::Loggable { public: - HttpApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor); - // TODO(junr03): consider moving Envoy Mobile's SyntheticAddressImpl to Envoy in order to return // that rather than this semi-real one. const Network::Address::InstanceConstSharedPtr& address() const { return address_; } // ApiListener absl::string_view name() const override { return name_; } - ApiListenerHandle* handle() override; // TODO(junr03): the majority of this surface could be moved out of the listener via some sort of // base class context. @@ -78,21 +73,29 @@ class HttpApiListenerImpl : public ApiListener, // TODO(junr03): hook up draining to listener state management. bool drainClose() const override { return false; } -private: +protected: + ApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor); + // Synthetic class that acts as a stub Network::ReadFilterCallbacks. // TODO(junr03): if we are able to separate the Network Filter aspects of the // Http::ConnectionManagerImpl from the http management aspects of it, it is possible we would not // need this and the SyntheticConnection stub anymore. class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { public: - SyntheticReadCallbacks(HttpApiListenerImpl& parent) + SyntheticReadCallbacks(ApiListenerImpl& parent) : parent_(parent), connection_(SyntheticConnection(*this)) {} // Network::ReadFilterCallbacks - void continueReading() override {} - void injectReadDataToFilterChain(Buffer::Instance&, bool) override {} + void continueReading() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + void injectReadDataToFilterChain(Buffer::Instance&, bool) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } Upstream::HostDescriptionConstSharedPtr upstreamHost() override { return nullptr; } - void upstreamHost(Upstream::HostDescriptionConstSharedPtr) override {} + void upstreamHost(Upstream::HostDescriptionConstSharedPtr) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } Network::Connection& connection() override { return connection_; } // Synthetic class that acts as a stub for the connection backing the @@ -104,22 +107,26 @@ class HttpApiListenerImpl : public ApiListener, options_(std::make_shared>()) {} // Network::FilterManager - void addWriteFilter(Network::WriteFilterSharedPtr) override {} - void addFilter(Network::FilterSharedPtr) override {} - void addReadFilter(Network::ReadFilterSharedPtr) override {} + void addWriteFilter(Network::WriteFilterSharedPtr) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void addFilter(Network::FilterSharedPtr) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + void addReadFilter(Network::ReadFilterSharedPtr) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } bool initializeReadFilters() override { return true; } // Network::Connection void addConnectionCallbacks(Network::ConnectionCallbacks&) override {} - void addBytesSentCallback(Network::Connection::BytesSentCb) override {} - void enableHalfClose(bool) override {} + void addBytesSentCallback(Network::Connection::BytesSentCb) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void enableHalfClose(bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void close(Network::ConnectionCloseType) override {} Event::Dispatcher& dispatcher() override { return parent_.parent_.dispatcher(); } uint64_t id() const override { return 12345; } std::string nextProtocol() const override { return EMPTY_STRING; } - void noDelay(bool) override {} + void noDelay(bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void readDisable(bool) override {} - void detectEarlyCloseWhenReadDisabled(bool) override {} + void detectEarlyCloseWhenReadDisabled(bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } bool readEnabled() const override { return true; } const Network::Address::InstanceConstSharedPtr& remoteAddress() const override { return parent_.parent_.address(); @@ -135,8 +142,8 @@ class HttpApiListenerImpl : public ApiListener, Ssl::ConnectionInfoConstSharedPtr ssl() const override { return nullptr; } absl::string_view requestedServerName() const override { return EMPTY_STRING; } State state() const override { return Network::Connection::State::Open; } - void write(Buffer::Instance&, bool) override {} - void setBufferLimits(uint32_t) override {} + void write(Buffer::Instance&, bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + void setBufferLimits(uint32_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } uint32_t bufferLimit() const override { return 65000; } bool localAddressRestored() const override { return false; } bool aboveHighWatermark() const override { return false; } @@ -153,7 +160,7 @@ class HttpApiListenerImpl : public ApiListener, Network::ConnectionSocket::OptionsSharedPtr options_; }; - HttpApiListenerImpl& parent_; + ApiListenerImpl& parent_; SyntheticConnection connection_; }; @@ -165,11 +172,24 @@ class HttpApiListenerImpl : public ApiListener, Stats::ScopePtr global_scope_; Stats::ScopePtr listener_scope_; SyntheticReadCallbacks read_callbacks_; +}; + +class HttpApiListener : public ApiListenerImpl { +public: + HttpApiListener(const envoy::config::listener::v3alpha::Listener& config, + ListenerManagerImpl& parent, const std::string& name, + ProtobufMessage::ValidationVisitor& validation_visitor); + + // ApiListener + ApiListener::Type type() const override { return ApiListener::Type::HttpApiListener; } + Http::ApiListener* http() override; + +private: // Need to store the factory due to the shared_ptrs that need to be kept alive: date provider, // route config manager, scoped route config manager. - std::function http_connection_manager_factory_; + std::function http_connection_manager_factory_; // Http::ServerConnectionCallbacks is the API surface that this class provides via its handle(). - Http::ServerConnectionCallbacksPtr http_connection_manager_; + Http::ApiListenerPtr http_connection_manager_; }; } // namespace Server diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h index 0e19228ae8eb..3fb321dd2559 100644 --- a/source/server/listener_impl.h +++ b/source/server/listener_impl.h @@ -4,7 +4,6 @@ #include "envoy/config/core/v3alpha/base.pb.h" #include "envoy/config/listener/v3alpha/listener.pb.h" -#include "envoy/network/connection.h" #include "envoy/network/filter.h" #include "envoy/server/drain_manager.h" #include "envoy/server/filter_config.h" diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 37ee53a7bb5b..2761803d5a0d 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -316,7 +316,9 @@ bool ListenerManagerImpl::addOrUpdateListener( // future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap. if (config.has_api_listener()) { if (!api_listener_ && !added_via_api) { - api_listener_ = std::make_unique( + // TODO(junr03): dispatch to different concrete constructors when there are other + // ApiListenerImpl derived classes. + api_listener_ = std::make_unique( config, *this, config.name(), server_.messageValidationContext().staticValidationVisitor()); return true; @@ -811,9 +813,5 @@ Network::ListenSocketFactorySharedPtr ListenerManagerImpl::createListenSocketFac listener.bindToPort(), listener.name(), reuse_port); } -ApiListenerHandle* ListenerManagerImpl::apiListener() { - return api_listener_ ? api_listener_->handle() : nullptr; -} - } // namespace Server } // namespace Envoy diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 4ba2e8d17f2a..af83bf99fe9d 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -147,7 +147,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable Date: Tue, 7 Jan 2020 09:59:15 -0800 Subject: [PATCH 24/44] comments Signed-off-by: Jose Nino --- .../filters/network/http_connection_manager/config.cc | 4 ++-- .../filters/network/http_connection_manager/config.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 9ae9fa929f4f..d0c6508c5e88 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -541,8 +541,8 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( // reference count. // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine // as these captured objects are also global singletons. - return [scoped_routes_config_provider_manager, route_config_provider_manager, - date_provider, filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { + return [scoped_routes_config_provider_manager, route_config_provider_manager, date_provider, + filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { auto conn_manager = std::make_unique( *filter_config, context.drainDecision(), context.random(), context.httpContext(), context.runtime(), context.localInfo(), context.clusterManager(), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index a640979a231c..347b566e45b9 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -14,8 +14,8 @@ #include "envoy/router/route_config_provider_manager.h" #include "common/common/logger.h" -#include "common/http/date_provider_impl.h" #include "common/http/conn_manager_impl.h" +#include "common/http/date_provider_impl.h" #include "common/json/json_loader.h" #include "common/router/rds_impl.h" #include "common/router/scoped_rds.h" From 8624c14f32afa601d06a0646a85f11a3aaf8a543 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 7 Jan 2020 10:06:30 -0800 Subject: [PATCH 25/44] generated Signed-off-by: Jose Nino --- generated_api_shadow/envoy/api/v2/listener.proto | 5 +++++ .../envoy/config/listener/v3alpha/listener.proto | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/generated_api_shadow/envoy/api/v2/listener.proto b/generated_api_shadow/envoy/api/v2/listener.proto index 094e61e0e666..826cd0ad9666 100644 --- a/generated_api_shadow/envoy/api/v2/listener.proto +++ b/generated_api_shadow/envoy/api/v2/listener.proto @@ -206,6 +206,11 @@ message Listener { // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for :ref:`name` // should be set. + // + // .. note:: + // + // Currently only one ApiListener can be installed via bootstrap + // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types // are in the ApiListener message, the socket listener messages should be in their own message, diff --git a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto index 3b7dfe4bbff5..7ba116a05bcb 100644 --- a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto @@ -217,6 +217,11 @@ message Listener { // exposed to the non-proxy application depends on the type of API listener. // When this field is set, no other field except for // :ref:`name` should be set. + // + // .. note:: + // + // Currently only one ApiListener can be installed via bootstrap + // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types // are in the ApiListener message, the socket listener messages should be in their own message, From 6de2dbc26142f563726c75bb276fa5c2c2f5655f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 7 Jan 2020 10:18:21 -0800 Subject: [PATCH 26/44] comments Signed-off-by: Jose Nino --- api/envoy/api/v2/listener.proto | 3 ++- api/envoy/config/listener/v3alpha/listener.proto | 3 ++- generated_api_shadow/envoy/api/v2/listener.proto | 3 ++- .../envoy/config/listener/v3alpha/listener.proto | 3 ++- .../filters/network/http_connection_manager/config.h | 10 ++++++++++ 5 files changed, 18 insertions(+), 4 deletions(-) diff --git a/api/envoy/api/v2/listener.proto b/api/envoy/api/v2/listener.proto index 826cd0ad9666..2cc32a103d87 100644 --- a/api/envoy/api/v2/listener.proto +++ b/api/envoy/api/v2/listener.proto @@ -209,7 +209,8 @@ message Listener { // // .. note:: // - // Currently only one ApiListener can be installed via bootstrap + // Currently only one ApiListener can be installed; and it can only be done via bootstrap config, + // not LDS. // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types diff --git a/api/envoy/config/listener/v3alpha/listener.proto b/api/envoy/config/listener/v3alpha/listener.proto index 8336d21e887c..d14bfcc73da7 100644 --- a/api/envoy/config/listener/v3alpha/listener.proto +++ b/api/envoy/config/listener/v3alpha/listener.proto @@ -204,7 +204,8 @@ message Listener { // // .. note:: // - // Currently only one ApiListener can be installed via bootstrap + // Currently only one ApiListener can be installed; and it can only be done via bootstrap config, + // not LDS. // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types diff --git a/generated_api_shadow/envoy/api/v2/listener.proto b/generated_api_shadow/envoy/api/v2/listener.proto index 826cd0ad9666..2cc32a103d87 100644 --- a/generated_api_shadow/envoy/api/v2/listener.proto +++ b/generated_api_shadow/envoy/api/v2/listener.proto @@ -209,7 +209,8 @@ message Listener { // // .. note:: // - // Currently only one ApiListener can be installed via bootstrap + // Currently only one ApiListener can be installed; and it can only be done via bootstrap config, + // not LDS. // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types diff --git a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto index 7ba116a05bcb..47a9bd0d98a3 100644 --- a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto @@ -220,7 +220,8 @@ message Listener { // // .. note:: // - // Currently only one ApiListener can be installed via bootstrap + // Currently only one ApiListener can be installed; and it can only be done via bootstrap config, + // not LDS. // // [#next-major-version: In the v3 API, instead of this messy approach where the socket // listener fields are directly in the top-level Listener message and the API listener types diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 347b566e45b9..b9cdafc1a32e 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -220,7 +220,10 @@ class HttpConnectionManagerFactory { class Utility { public: /** + * Create/get singletons needed for config creation. * + * @param context supplies the context used to create the singletons. + * @return a tuple containing all the singletons. */ static std::tuple, std::shared_ptr, @@ -228,7 +231,14 @@ class Utility { createSingletons(Server::Configuration::FactoryContext& context); /** + * Create the HttpConnectionManagerConfig. * + * @param proto_config supplies the config to install. + * @param context supplies the context used to create the config. + * @param date_provider the singleton used in config creation. + * @param route_config_provider_manager the singleton used in config creation. + * @param scoped_routes_config_provider_manager the singleton used in config creation. + * @return a shared_ptr to the created config object. */ static std::shared_ptr createConfig(const envoy::extensions::filters::network::http_connection_manager::v3alpha:: From 1c68d73b4104566ff3b292b45e5030e0b75e78e1 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 7 Jan 2020 10:20:24 -0800 Subject: [PATCH 27/44] accidental commit Signed-off-by: Jose Nino --- tools/proto_format.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/proto_format.sh b/tools/proto_format.sh index 9f1d55fb48ff..fdc515fd108e 100755 --- a/tools/proto_format.sh +++ b/tools/proto_format.sh @@ -14,7 +14,7 @@ set -e declare -r PROTO_TARGETS=$(bazel query "labels(srcs, labels(deps, @envoy_api_canonical//docs:protos))") # This is for local RBE setup, should be no-op for builds without RBE setting in bazelrc files. -BAZEL_BUILD_OPTIONS+=" --remote_download_outputs=all --xcode_version=11.3.0.11C29" +BAZEL_BUILD_OPTIONS+=" --remote_download_outputs=all" # TODO(htuch): This script started life by cloning docs/build.sh. It depends on # the @envoy_api_canonical//docs:protos target in a few places as a result. This is not From b2c9ba5e5325065e6986dbce330cee2258294ae9 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 9 Jan 2020 14:50:06 -0800 Subject: [PATCH 28/44] fmt Signed-off-by: Jose Nino --- api/envoy/config/listener/v3alpha/api_listener.proto | 3 +-- .../envoy/config/listener/v2/api_listener.proto | 3 +++ .../envoy/config/listener/v3alpha/api_listener.proto | 3 +-- .../envoy/config/listener/v3alpha/listener.proto | 4 ++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/api/envoy/config/listener/v3alpha/api_listener.proto b/api/envoy/config/listener/v3alpha/api_listener.proto index 2d32354db829..2aa14219e5f8 100644 --- a/api/envoy/config/listener/v3alpha/api_listener.proto +++ b/api/envoy/config/listener/v3alpha/api_listener.proto @@ -10,7 +10,6 @@ option java_package = "io.envoyproxy.envoy.config.listener.v3alpha"; option java_outer_classname = "ApiListenerProto"; option java_multiple_files = true; -// [#not-implemented-hide:] // Describes a type of API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. message ApiListener { @@ -19,7 +18,7 @@ message ApiListener { // The type in this field determines the type of API listener. At present, the following // types are supported: - // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) + // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) // [#next-major-version: In the v3 API, replace this Any field with a oneof containing the // specific config message for each type of API listener. We could not do this in v2 because // it would have caused circular dependencies for go protos: lds.proto depends on this file, diff --git a/generated_api_shadow/envoy/config/listener/v2/api_listener.proto b/generated_api_shadow/envoy/config/listener/v2/api_listener.proto index 44ef190600a5..7fd3d56e7f39 100644 --- a/generated_api_shadow/envoy/config/listener/v2/api_listener.proto +++ b/generated_api_shadow/envoy/config/listener/v2/api_listener.proto @@ -4,9 +4,12 @@ package envoy.config.listener.v2; import "google/protobuf/any.proto"; +import "udpa/annotations/migrate.proto"; + option java_package = "io.envoyproxy.envoy.config.listener.v2"; option java_outer_classname = "ApiListenerProto"; option java_multiple_files = true; +option (udpa.annotations.file_migrate).move_to_package = "envoy.config.listener.v3alpha"; // Describes a type of API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. diff --git a/generated_api_shadow/envoy/config/listener/v3alpha/api_listener.proto b/generated_api_shadow/envoy/config/listener/v3alpha/api_listener.proto index 2d32354db829..2aa14219e5f8 100644 --- a/generated_api_shadow/envoy/config/listener/v3alpha/api_listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3alpha/api_listener.proto @@ -10,7 +10,6 @@ option java_package = "io.envoyproxy.envoy.config.listener.v3alpha"; option java_outer_classname = "ApiListenerProto"; option java_multiple_files = true; -// [#not-implemented-hide:] // Describes a type of API listener, which is used in non-proxy clients. The type of API // exposed to the non-proxy application depends on the type of API listener. message ApiListener { @@ -19,7 +18,7 @@ message ApiListener { // The type in this field determines the type of API listener. At present, the following // types are supported: - // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) + // envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager (HTTP) // [#next-major-version: In the v3 API, replace this Any field with a oneof containing the // specific config message for each type of API listener. We could not do this in v2 because // it would have caused circular dependencies for go protos: lds.proto depends on this file, diff --git a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto index 47a9bd0d98a3..f9d224aa6fe7 100644 --- a/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto +++ b/generated_api_shadow/envoy/config/listener/v3alpha/listener.proto @@ -4,7 +4,7 @@ package envoy.config.listener.v3alpha; import "envoy/config/core/v3alpha/address.proto"; import "envoy/config/core/v3alpha/base.proto"; -import "envoy/config/listener/v2/api_listener.proto"; +import "envoy/config/listener/v3alpha/api_listener.proto"; import "envoy/config/listener/v3alpha/listener_components.proto"; import "envoy/config/listener/v3alpha/udp_listener_config.proto"; @@ -229,7 +229,7 @@ message Listener { // and the top-level Listener should essentially be a oneof that selects between the // socket listener and the various types of API listener. That way, a given Listener message // can structurally only contain the fields of the relevant type.] - v2.ApiListener api_listener = 19; + ApiListener api_listener = 19; // The listener's connection balancer configuration, currently only applicable to TCP listeners. // If no configuration is specified, Envoy will not attempt to balance active connections between From ce5904385d0bb136dffa56f9508cfe41359301a6 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 10 Jan 2020 13:01:45 -0800 Subject: [PATCH 29/44] comments Signed-off-by: Jose Nino --- include/envoy/http/api_listener.h | 2 + source/common/http/conn_manager_impl.cc | 90 ++++++------- source/common/http/conn_manager_impl.h | 4 +- .../network/http_connection_manager/config.cc | 38 ++---- .../network/http_connection_manager/config.h | 14 ++- source/server/api_listener_impl.h | 6 +- test/server/BUILD | 13 ++ test/server/api_listener_test.cc | 87 +++++++++++++ test/server/listener_manager_impl_test.cc | 118 ++++++++++++++++++ 9 files changed, 294 insertions(+), 78 deletions(-) create mode 100644 test/server/api_listener_test.cc diff --git a/include/envoy/http/api_listener.h b/include/envoy/http/api_listener.h index 01c7059da317..deebe031c60a 100644 --- a/include/envoy/http/api_listener.h +++ b/include/envoy/http/api_listener.h @@ -8,6 +8,8 @@ namespace Http { /** * ApiListener that allows consumers to interact with HTTP streams via API calls. */ +// TODO(junr03): this is a replica of the functions in ServerConnectionCallbacks. It would be nice +// to not duplicate this interface layout. class ApiListener { public: virtual ~ApiListener() = default; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index b2ea0a99e477..181d0790fc86 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -288,20 +288,24 @@ void ConnectionManagerImpl::handleCodecException(const char* error) { void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { ASSERT(!codec_); codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); + + if (codec_->protocol() == Protocol::Http2) { + stats_.named_.downstream_cx_http2_total_.inc(); + stats_.named_.downstream_cx_http2_active_.inc(); + } else if (codec_->protocol() == Protocol::Http3) { + stats_.named_.downstream_cx_http3_total_.inc(); + stats_.named_.downstream_cx_http3_active_.inc(); + } else { + stats_.named_.downstream_cx_http1_total_.inc(); + stats_.named_.downstream_cx_http1_active_.inc(); + } } Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. + ASSERT(codec_->protocol() != Protocol::Http3); createCodec(data); - if (codec_->protocol() == Protocol::Http2) { - stats_.named_.downstream_cx_http2_total_.inc(); - stats_.named_.downstream_cx_http2_active_.inc(); - } else { - ASSERT(codec_->protocol() != Protocol::Http3); - stats_.named_.downstream_cx_http1_total_.inc(); - stats_.named_.downstream_cx_http1_active_.inc(); - } } bool redispatch; @@ -363,8 +367,6 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { Buffer::OwnedImpl dummy; createCodec(dummy); ASSERT(codec_->protocol() == Protocol::Http3); - stats_.named_.downstream_cx_http3_total_.inc(); - stats_.named_.downstream_cx_http3_active_.inc(); // Stop iterating through each filters for QUIC. Currently a QUIC connection // only supports one filter, HCM, and bypasses the onData() interface. Because // QUICHE already handles de-multiplexing. @@ -554,8 +556,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect ConnectionManagerImpl::ActiveStream::~ActiveStream() { stream_info_.onRequestComplete(); - // A downstream disconnect can be identified for HTTP requests when the upstream returns with a 0 - // response code and when no other response flags are set. + // A downstream disconnect can be identified for HTTP requests when the upstream returns with a + // 0 response code and when no other response flags are set. if (!stream_info_.hasAnyResponseFlag() && !stream_info_.responseCode()) { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamConnectionTermination); } @@ -782,9 +784,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Verify header sanity checks which should have been performed by the codec. ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); - // Currently we only support relative paths at the application layer. We expect the codec to have - // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this - // when the allow_absolute_url flag is enabled on the HCM. + // Currently we only support relative paths at the application layer. We expect the codec to + // have broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does + // this when the allow_absolute_url flag is enabled on the HCM. // https://tools.ietf.org/html/rfc7230#section-5.3 We also need to check for the existence of // :path because CONNECT does not have a path, and we don't support that currently. if (!request_headers_->Path() || request_headers_->Path()->value().getStringView().empty() || @@ -864,7 +866,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (route_entry != nullptr && route_entry->idleTimeout()) { idle_timeout_ms_ = route_entry->idleTimeout().value(); if (idle_timeout_ms_.count()) { - // If we have a route-level idle timeout but no global stream idle timeout, create a timer. + // If we have a route-level idle timeout but no global stream idle timeout, create a + // timer. if (stream_idle_timer_ == nullptr) { stream_idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( @@ -962,9 +965,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte static_cast((*entry).get()), static_cast(status)); const bool new_metadata_added = processNewlyAddedMetadata(); - // If end_stream is set in headers, and a filter adds new metadata, we need to delay end_stream - // in headers by inserting an empty data frame with end_stream set. The empty data frame is sent - // after the new metadata. + // If end_stream is set in headers, and a filter adds new metadata, we need to delay + // end_stream in headers by inserting an empty data frame with end_stream set. The empty data + // frame is sent after the new metadata. if ((*entry)->end_stream_ && new_metadata_added && !buffered_request_data_) { Buffer::OwnedImpl empty_data(""); ENVOY_STREAM_LOG( @@ -1079,8 +1082,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData( ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeData)); // We check the request_trailers_ pointer here in case addDecodedTrailers - // is called in decodeData during a previous filter invocation, at which point we communicate to - // the current and future filters that the stream has not yet ended. + // is called in decodeData during a previous filter invocation, at which point we communicate + // to the current and future filters that the stream has not yet ended. if (end_stream) { state_.filter_call_state_ |= FilterCallState::LastDataFrame; } @@ -1381,23 +1384,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply( - is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply(is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, + FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( @@ -1485,7 +1488,8 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte // Base headers. connection_manager_.config_.dateProvider().setDateHeader(headers); - // Following setReference() is safe because serverName() is constant for the life of the listener. + // Following setReference() is safe because serverName() is constant for the life of the + // listener. const auto transformation = connection_manager_.config_.serverHeaderTransformation(); if (transformation == ConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE || (transformation == ConnectionManagerConfig::HttpConnectionManagerProto::APPEND_IF_ABSENT && @@ -1687,8 +1691,8 @@ void ConnectionManagerImpl::ActiveStream::encodeData( ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeData)); // We check the response_trailers_ pointer here in case addEncodedTrailers - // is called in encodeData during a previous filter invocation, at which point we communicate to - // the current and future filters that the stream has not yet ended. + // is called in encodeData during a previous filter invocation, at which point we communicate + // to the current and future filters that the stream has not yet ended. state_.filter_call_state_ |= FilterCallState::EncodeData; if (end_stream) { state_.filter_call_state_ |= FilterCallState::LastDataFrame; @@ -1877,8 +1881,8 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() { if (upgrade != nullptr) { const Router::RouteEntry::UpgradeMap* upgrade_map = nullptr; - // We must check if the 'cached_route_' optional is populated since this function can be called - // early via sendLocalReply(), before the cached route is populated. + // We must check if the 'cached_route_' optional is populated since this function can be + // called early via sendLocalReply(), before the cached route is populated. if (hasCachedRoute() && cached_route_.value()->routeEntry()) { upgrade_map = &cached_route_.value()->routeEntry()->upgradeMap(); } diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 9b38ac614124..3b953c9ded15 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -71,8 +71,8 @@ class ConnectionManagerImpl : Logger::Loggable, // Currently the ConnectionManager creates a codec lazily when either: // a) onConnection for H3. // b) onData for H1 and H2. - // With the introduction of ApiListeners, neither event occurs. This function allows us to force - // create a codec. + // With the introduction of ApiListeners, neither event occurs. This function allows consumer code + // to manually create a codec. // TODO(junr03): consider passing a synthetic codec instead of creating once. The codec in the // ApiListener case is solely used to determine the protocol version. void createCodec(Buffer::Instance& data); diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 24f6e5e528bb..c880969bfcee 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -75,10 +75,7 @@ SINGLETON_MANAGER_REGISTRATION(date_provider); SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager); SINGLETON_MANAGER_REGISTRATION(scoped_routes_config_provider_manager); -std::tuple, - std::shared_ptr, - std::shared_ptr> -Utility::createSingletons(Server::Configuration::FactoryContext& context) { +Utility::Singletons Utility::createSingletons(Server::Configuration::FactoryContext& context) { std::shared_ptr date_provider = context.singletonManager().getTyped( SINGLETON_MANAGER_REGISTERED_NAME(date_provider), [&context] { @@ -100,8 +97,7 @@ Utility::createSingletons(Server::Configuration::FactoryContext& context) { context.admin(), *route_config_provider_manager); }); - return std::make_tuple(date_provider, route_config_provider_manager, - scoped_routes_config_provider_manager); + return {date_provider, route_config_provider_manager, scoped_routes_config_provider_manager}; } std::shared_ptr @@ -121,22 +117,17 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( const envoy::extensions::filters::network::http_connection_manager::v3alpha:: HttpConnectionManager& proto_config, Server::Configuration::FactoryContext& context) { - std::shared_ptr date_provider; - std::shared_ptr route_config_provider_manager; - std::shared_ptr scoped_routes_config_provider_manager; - std::tie(date_provider, route_config_provider_manager, scoped_routes_config_provider_manager) = - Utility::createSingletons(context); + Utility::Singletons singletons = Utility::createSingletons(context); - auto filter_config = - Utility::createConfig(proto_config, context, *date_provider, *route_config_provider_manager, - *scoped_routes_config_provider_manager); + auto filter_config = Utility::createConfig(proto_config, context, *singletons.date_provider_, + *singletons.route_config_provider_manager_, + *singletons.scoped_routes_config_provider_manager_); // This lambda captures the shared_ptrs created above, thus preserving the // reference count. // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine // as these captured objects are also global singletons. - return [scoped_routes_config_provider_manager, route_config_provider_manager, date_provider, - filter_config, &context](Network::FilterManager& filter_manager) -> void { + return [singletons, filter_config, &context](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(Network::ReadFilterSharedPtr{new Http::ConnectionManagerImpl( *filter_config, context.drainDecision(), context.random(), context.httpContext(), context.runtime(), context.localInfo(), context.clusterManager(), @@ -529,22 +520,17 @@ HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( HttpConnectionManager& proto_config, Server::Configuration::FactoryContext& context, Network::ReadFilterCallbacks& read_callbacks) { - std::shared_ptr date_provider; - std::shared_ptr route_config_provider_manager; - std::shared_ptr scoped_routes_config_provider_manager; - std::tie(date_provider, route_config_provider_manager, scoped_routes_config_provider_manager) = - Utility::createSingletons(context); + Utility::Singletons singletons = Utility::createSingletons(context); - auto filter_config = - Utility::createConfig(proto_config, context, *date_provider, *route_config_provider_manager, - *scoped_routes_config_provider_manager); + auto filter_config = Utility::createConfig(proto_config, context, *singletons.date_provider_, + *singletons.route_config_provider_manager_, + *singletons.scoped_routes_config_provider_manager_); // This lambda captures the shared_ptrs created above, thus preserving the // reference count. // Keep in mind the lambda capture list **doesn't** determine the destruction order, but it's fine // as these captured objects are also global singletons. - return [scoped_routes_config_provider_manager, route_config_provider_manager, date_provider, - filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { + return [singletons, filter_config, &context, &read_callbacks]() -> Http::ApiListenerPtr { auto conn_manager = std::make_unique( *filter_config, context.drainDecision(), context.random(), context.httpContext(), context.runtime(), context.localInfo(), context.clusterManager(), diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index b9cdafc1a32e..4a83a402516c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -219,16 +219,20 @@ class HttpConnectionManagerFactory { */ class Utility { public: + struct Singletons { + std::shared_ptr date_provider_; + std::shared_ptr route_config_provider_manager_; + std::shared_ptr + scoped_routes_config_provider_manager_; + }; + /** * Create/get singletons needed for config creation. * * @param context supplies the context used to create the singletons. - * @return a tuple containing all the singletons. + * @return Singletons struct containing all the singletons. */ - static std::tuple, - std::shared_ptr, - std::shared_ptr> - createSingletons(Server::Configuration::FactoryContext& context); + static Singletons createSingletons(Server::Configuration::FactoryContext& context); /** * Create the HttpConnectionManagerConfig. diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index 156d67ed712e..d4204605f294 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -39,8 +39,10 @@ class ApiListenerImpl : public ApiListener, // ApiListener absl::string_view name() const override { return name_; } - // TODO(junr03): the majority of this surface could be moved out of the listener via some sort of - // base class context. + // TODO(junr03): move these functions into a concrete Server:Configuration::FactoryContext that + // takes the server's context, and the listener config in the constructor. The only challenging + // part is implementing the initManager() call. The function depends on the Listener's state for + // ListenerImpl, and will do so once ApiListeners are dynamic. // Server::Configuration::FactoryContext AccessLog::AccessLogManager& accessLogManager() override; Upstream::ClusterManager& clusterManager() override; diff --git a/test/server/BUILD b/test/server/BUILD index 0176a61279c5..7bc1fb57a54e 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -16,6 +16,19 @@ load( envoy_package() +envoy_cc_test( + name = "api_listener_test", + srcs = ["api_listener_test.cc"], + deps = [ + ":utility_lib", + "//source/server:api_listener_lib", + "//source/server:listener_lib", + "//test/mocks/server:server_mocks", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/listener/v3alpha:pkg_cc_proto", + ], +) + envoy_cc_test( name = "backtrace_test", srcs = ["backtrace_test.cc"], diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc new file mode 100644 index 000000000000..6435950728ef --- /dev/null +++ b/test/server/api_listener_test.cc @@ -0,0 +1,87 @@ +#include + +#include "envoy/config/listener/v3alpha/listener.pb.h" + +#include "server/listener_manager_impl.h" +#include "server/api_listener_impl.h" + +#include "test/mocks/server/mocks.h" +#include "test/server/utility.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Server { + +class ApiListenerTest : public testing::Test { +protected: + ApiListenerTest() + : listener_manager_(std::make_unique(server_, listener_factory_, + worker_factory_, false)) {} + + NiceMock server_; + NiceMock listener_factory_; + NiceMock worker_factory_; + std::unique_ptr listener_manager_; + NiceMock validation_visitor_; +}; + +TEST_F(ApiListenerTest, HttpApiListener) { + const std::string yaml = R"EOF( +name: test_api_listener +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager + stat_prefix: hcm + route_config: + name: api_router + virtual_hosts: + - name: api + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: dynamic_forward_proxy_cluster + )EOF"; + + const envoy::config::listener::v3alpha::Listener config = parseListenerFromV2Yaml(yaml); + + auto http_api_listener = + HttpApiListener(config, *listener_manager_, config.name(), validation_visitor_); + + ASSERT_EQ("test_api_listener", http_api_listener.name()); + ASSERT_EQ(ApiListener::Type::HttpApiListener, http_api_listener.type()); + ASSERT_NE(nullptr, http_api_listener.http()); +} + +// TEST_F(ApiListenerTest, HttpApiListenerThrowsWithBadConfig) { +// const std::string yaml = R"EOF( +// name: test_api_listener +// address: +// socket_address: +// address: 127.0.0.1 +// port_value: 1234 +// api_listener: +// api_listener: +// "@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy +// stat_prefix: tcp_stats +// cluster: cluster_0 +// )EOF"; + +// const envoy::config::listener::v3alpha::Listener config = parseListenerFromV2Yaml(yaml); + +// EXPECT_THROW_WITH_MESSAGE( +// HttpApiListener(config, *listener_manager_, config.name(), validation_visitor_), +// EnvoyException, "message"); +// } + +} // namespace Server +} // namespace Envoy \ No newline at end of file diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index f6f875c659dc..dec21c11c072 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -3634,6 +3634,124 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, VerifyIgnoreExpirationWithCA) { EXPECT_NO_THROW(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true)); } +TEST_F(ListenerManagerImplWithRealFiltersTest, ApiListener) { + const std::string yaml = R"EOF( +name: test_api_listener +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager + stat_prefix: hcm + route_config: + name: api_router + virtual_hosts: + - name: api + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: dynamic_forward_proxy_cluster + )EOF"; + + ASSERT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", false)); + EXPECT_EQ(0U, manager_->listeners().size()); + EXPECT_NE(nullptr, manager_->apiListener()); +} + +TEST_F(ListenerManagerImplWithRealFiltersTest, ApiListenerNotAllowedAddedViaApi) { + const std::string yaml = R"EOF( +name: test_api_listener +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager + stat_prefix: hcm + route_config: + name: api_router + virtual_hosts: + - name: api + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: dynamic_forward_proxy_cluster + )EOF"; + + ASSERT_FALSE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true)); + EXPECT_EQ(0U, manager_->listeners().size()); + EXPECT_EQ(nullptr, manager_->apiListener()); +} + +TEST_F(ListenerManagerImplWithRealFiltersTest, ApiListenerOnlyOneApiListener) { + const std::string yaml = R"EOF( +name: test_api_listener +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager + stat_prefix: hcm + route_config: + name: api_router + virtual_hosts: + - name: api + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: dynamic_forward_proxy_cluster + )EOF"; + + const std::string yaml2 = R"EOF( +name: test_api_listener_2 +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager + stat_prefix: hcm + route_config: + name: api_router + virtual_hosts: + - name: api + domains: + - "*" + routes: + - match: + prefix: "/" + route: + cluster: dynamic_forward_proxy_cluster + )EOF"; + + ASSERT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", false)); + EXPECT_EQ(0U, manager_->listeners().size()); + EXPECT_NE(nullptr, manager_->apiListener()); + EXPECT_EQ("test_api_listener", manager_->apiListener()->name()); + + // Only one ApiListener is added. + ASSERT_FALSE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", false)); + EXPECT_EQ(0U, manager_->listeners().size()); + // The original ApiListener is there. + EXPECT_NE(nullptr, manager_->apiListener()); + EXPECT_EQ("test_api_listener", manager_->apiListener()->name()); +} + } // namespace } // namespace Server } // namespace Envoy From 7ce32a59b48331bfd322ded76d9056e23cca3aba Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 10 Jan 2020 13:13:19 -0800 Subject: [PATCH 30/44] fmt Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 34 +++++++++---------- source/extensions/transport_sockets/tls/BUILD | 1 - test/server/api_listener_test.cc | 2 +- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 181d0790fc86..a3b216ae8918 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1384,23 +1384,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply(is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, - FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply( + is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( diff --git a/source/extensions/transport_sockets/tls/BUILD b/source/extensions/transport_sockets/tls/BUILD index 9dab665c298b..c99762ffd8cd 100644 --- a/source/extensions/transport_sockets/tls/BUILD +++ b/source/extensions/transport_sockets/tls/BUILD @@ -22,7 +22,6 @@ envoy_cc_extension( "//include/envoy/registry", "//include/envoy/server:transport_socket_config_interface", "//source/extensions/transport_sockets:well_known_names", - "@envoy_api//envoy/extensions/transport_sockets/tls/v3alpha:pkg_cc_proto", ], ) diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index 6435950728ef..003e95110341 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -2,8 +2,8 @@ #include "envoy/config/listener/v3alpha/listener.pb.h" -#include "server/listener_manager_impl.h" #include "server/api_listener_impl.h" +#include "server/listener_manager_impl.h" #include "test/mocks/server/mocks.h" #include "test/server/utility.h" From 6e2ee131f99a17177ed41a8ece93b565fc2ee033 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 10 Jan 2020 15:01:07 -0800 Subject: [PATCH 31/44] bad merge Signed-off-by: Jose Nino --- source/extensions/transport_sockets/tls/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/transport_sockets/tls/BUILD b/source/extensions/transport_sockets/tls/BUILD index c99762ffd8cd..9dab665c298b 100644 --- a/source/extensions/transport_sockets/tls/BUILD +++ b/source/extensions/transport_sockets/tls/BUILD @@ -22,6 +22,7 @@ envoy_cc_extension( "//include/envoy/registry", "//include/envoy/server:transport_socket_config_interface", "//source/extensions/transport_sockets:well_known_names", + "@envoy_api//envoy/extensions/transport_sockets/tls/v3alpha:pkg_cc_proto", ], ) From df69e8c07a4877d581e23ec03c238bc602db0c5d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 10:10:45 -0800 Subject: [PATCH 32/44] move stats back. my test will fail Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 69 +++++++++++++------------ 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 9269b21e1949..0e38abd9f886 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -42,7 +42,6 @@ #include "absl/strings/escaping.h" #include "absl/strings/match.h" -#include "absl/strings/str_cat.h" namespace Envoy { namespace Http { @@ -289,24 +288,20 @@ void ConnectionManagerImpl::handleCodecException(const char* error) { void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { ASSERT(!codec_); codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); - - if (codec_->protocol() == Protocol::Http2) { - stats_.named_.downstream_cx_http2_total_.inc(); - stats_.named_.downstream_cx_http2_active_.inc(); - } else if (codec_->protocol() == Protocol::Http3) { - stats_.named_.downstream_cx_http3_total_.inc(); - stats_.named_.downstream_cx_http3_active_.inc(); - } else { - stats_.named_.downstream_cx_http1_total_.inc(); - stats_.named_.downstream_cx_http1_active_.inc(); - } } Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. - ASSERT(codec_->protocol() != Protocol::Http3); createCodec(data); + if (codec_->protocol() == Protocol::Http2) { + stats_.named_.downstream_cx_http2_total_.inc(); + stats_.named_.downstream_cx_http2_active_.inc(); + } else { + ASSERT(codec_->protocol() != Protocol::Http3); + stats_.named_.downstream_cx_http1_total_.inc(); + stats_.named_.downstream_cx_http1_active_.inc(); + } } bool redispatch; @@ -368,6 +363,8 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { Buffer::OwnedImpl dummy; createCodec(dummy); ASSERT(codec_->protocol() == Protocol::Http3); + stats_.named_.downstream_cx_http3_total_.inc(); + stats_.named_.downstream_cx_http3_active_.inc(); // Stop iterating through each filters for QUIC. Currently a QUIC connection // only supports one filter, HCM, and bypasses the onData() interface. Because // QUICHE already handles de-multiplexing. @@ -491,7 +488,7 @@ void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_re break; default: throw std::invalid_argument( - absl::StrCat("invalid tracing reason, value: ", static_cast(tracing_reason))); + fmt::format("invalid tracing reason, value: {}", static_cast(tracing_reason))); } } @@ -557,8 +554,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect ConnectionManagerImpl::ActiveStream::~ActiveStream() { stream_info_.onRequestComplete(); - // A downstream disconnect can be identified for HTTP requests when the upstream returns with a - // 0 response code and when no other response flags are set. + // A downstream disconnect can be identified for HTTP requests when the upstream returns with a 0 + // response code and when no other response flags are set. if (!stream_info_.hasAnyResponseFlag() && !stream_info_.responseCode()) { stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamConnectionTermination); } @@ -782,12 +779,18 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } } - // Verify header sanity checks which should have been performed by the codec. - ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); + // Make sure the host is valid. + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") && + !HeaderUtility::authorityIsValid(request_headers_->Host()->value().getStringView())) { + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", + nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().InvalidAuthority); + return; + } - // Currently we only support relative paths at the application layer. We expect the codec to - // have broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does - // this when the allow_absolute_url flag is enabled on the HCM. + // Currently we only support relative paths at the application layer. We expect the codec to have + // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this + // when the allow_absolute_url flag is enabled on the HCM. // https://tools.ietf.org/html/rfc7230#section-5.3 We also need to check for the existence of // :path because CONNECT does not have a path, and we don't support that currently. if (!request_headers_->Path() || request_headers_->Path()->value().getStringView().empty() || @@ -867,8 +870,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, if (route_entry != nullptr && route_entry->idleTimeout()) { idle_timeout_ms_ = route_entry->idleTimeout().value(); if (idle_timeout_ms_.count()) { - // If we have a route-level idle timeout but no global stream idle timeout, create a - // timer. + // If we have a route-level idle timeout but no global stream idle timeout, create a timer. if (stream_idle_timer_ == nullptr) { stream_idle_timer_ = connection_manager_.read_callbacks_->connection().dispatcher().createTimer( @@ -966,9 +968,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte static_cast((*entry).get()), static_cast(status)); const bool new_metadata_added = processNewlyAddedMetadata(); - // If end_stream is set in headers, and a filter adds new metadata, we need to delay - // end_stream in headers by inserting an empty data frame with end_stream set. The empty data - // frame is sent after the new metadata. + // If end_stream is set in headers, and a filter adds new metadata, we need to delay end_stream + // in headers by inserting an empty data frame with end_stream set. The empty data frame is sent + // after the new metadata. if ((*entry)->end_stream_ && new_metadata_added && !buffered_request_data_) { Buffer::OwnedImpl empty_data(""); ENVOY_STREAM_LOG( @@ -1083,8 +1085,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData( ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeData)); // We check the request_trailers_ pointer here in case addDecodedTrailers - // is called in decodeData during a previous filter invocation, at which point we communicate - // to the current and future filters that the stream has not yet ended. + // is called in decodeData during a previous filter invocation, at which point we communicate to + // the current and future filters that the stream has not yet ended. if (end_stream) { state_.filter_call_state_ |= FilterCallState::LastDataFrame; } @@ -1489,8 +1491,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte // Base headers. connection_manager_.config_.dateProvider().setDateHeader(headers); - // Following setReference() is safe because serverName() is constant for the life of the - // listener. + // Following setReference() is safe because serverName() is constant for the life of the listener. const auto transformation = connection_manager_.config_.serverHeaderTransformation(); if (transformation == ConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE || (transformation == ConnectionManagerConfig::HttpConnectionManagerProto::APPEND_IF_ABSENT && @@ -1692,8 +1693,8 @@ void ConnectionManagerImpl::ActiveStream::encodeData( ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeData)); // We check the response_trailers_ pointer here in case addEncodedTrailers - // is called in encodeData during a previous filter invocation, at which point we communicate - // to the current and future filters that the stream has not yet ended. + // is called in encodeData during a previous filter invocation, at which point we communicate to + // the current and future filters that the stream has not yet ended. state_.filter_call_state_ |= FilterCallState::EncodeData; if (end_stream) { state_.filter_call_state_ |= FilterCallState::LastDataFrame; @@ -1882,8 +1883,8 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() { if (upgrade != nullptr) { const Router::RouteEntry::UpgradeMap* upgrade_map = nullptr; - // We must check if the 'cached_route_' optional is populated since this function can be - // called early via sendLocalReply(), before the cached route is populated. + // We must check if the 'cached_route_' optional is populated since this function can be called + // early via sendLocalReply(), before the cached route is populated. if (hasCachedRoute() && cached_route_.value()->routeEntry()) { upgrade_map = &cached_route_.value()->routeEntry()->upgradeMap(); } From 0ce64d4c458ba062973cc51b262bfb52d0ca4d5e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 10:25:58 -0800 Subject: [PATCH 33/44] adjust Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 51 +++++++++++-------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 0e38abd9f886..f9d1ac3c7fe3 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -42,6 +42,7 @@ #include "absl/strings/escaping.h" #include "absl/strings/match.h" +#include "absl/strings/str_cat.h" namespace Envoy { namespace Http { @@ -293,7 +294,7 @@ void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. - createCodec(data); + codec_ = createCodec(data); if (codec_->protocol() == Protocol::Http2) { stats_.named_.downstream_cx_http2_total_.inc(); stats_.named_.downstream_cx_http2_active_.inc(); @@ -361,7 +362,7 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { } // Only QUIC connection's stream_info_ specifies protocol. Buffer::OwnedImpl dummy; - createCodec(dummy); + codec_ = createCodec(dummy); ASSERT(codec_->protocol() == Protocol::Http3); stats_.named_.downstream_cx_http3_total_.inc(); stats_.named_.downstream_cx_http3_active_.inc(); @@ -488,7 +489,7 @@ void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_re break; default: throw std::invalid_argument( - fmt::format("invalid tracing reason, value: {}", static_cast(tracing_reason))); + absl::StrCat("invalid tracing reason, value: ", static_cast(tracing_reason))); } } @@ -779,14 +780,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } } - // Make sure the host is valid. - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") && - !HeaderUtility::authorityIsValid(request_headers_->Host()->value().getStringView())) { - sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", - nullptr, is_head_request_, absl::nullopt, - StreamInfo::ResponseCodeDetails::get().InvalidAuthority); - return; - } + // Verify header sanity checks which should have been performed by the codec. + ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); // Currently we only support relative paths at the application layer. We expect the codec to have // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this @@ -1387,23 +1382,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply( - is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply(is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, + FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( From ed5e8e6ad18e0502b4a9a2faf62de41f758c2f4f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 10:30:09 -0800 Subject: [PATCH 34/44] type Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index f9d1ac3c7fe3..aec0c377161e 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -294,7 +294,7 @@ void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. - codec_ = createCodec(data); + createCodec(data); if (codec_->protocol() == Protocol::Http2) { stats_.named_.downstream_cx_http2_total_.inc(); stats_.named_.downstream_cx_http2_active_.inc(); @@ -362,7 +362,7 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { } // Only QUIC connection's stream_info_ specifies protocol. Buffer::OwnedImpl dummy; - codec_ = createCodec(dummy); + createCodec(dummy); ASSERT(codec_->protocol() == Protocol::Http3); stats_.named_.downstream_cx_http3_total_.inc(); stats_.named_.downstream_cx_http3_active_.inc(); From cacb66673458f159e0dca717baf722f8008eeb1c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 10:51:51 -0800 Subject: [PATCH 35/44] fmt Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 34 ++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index aec0c377161e..acae3fc18257 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1382,23 +1382,23 @@ void ConnectionManagerImpl::ActiveStream::sendLocalReply( createFilterChain(); } stream_info_.setResponseCodeDetails(details); - Utility::sendLocalReply(is_grpc_request, - [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { - if (modify_headers != nullptr) { - modify_headers(*headers); - } - response_headers_ = std::move(headers); - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeHeaders(nullptr, *response_headers_, end_stream); - }, - [this](Buffer::Instance& data, bool end_stream) -> void { - // TODO: Start encoding from the last decoder filter that saw the - // request instead. - encodeData(nullptr, data, end_stream, - FilterIterationStartState::CanStartFromCurrent); - }, - state_.destroyed_, code, body, grpc_status, is_head_request); + Utility::sendLocalReply( + is_grpc_request, + [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void { + if (modify_headers != nullptr) { + modify_headers(*headers); + } + response_headers_ = std::move(headers); + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeHeaders(nullptr, *response_headers_, end_stream); + }, + [this](Buffer::Instance& data, bool end_stream) -> void { + // TODO: Start encoding from the last decoder filter that saw the + // request instead. + encodeData(nullptr, data, end_stream, FilterIterationStartState::CanStartFromCurrent); + }, + state_.destroyed_, code, body, grpc_status, is_head_request); } void ConnectionManagerImpl::ActiveStream::encode100ContinueHeaders( From c5f629a83452fa6c03810bfa7cc9534fb5e6254a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 11:58:20 -0800 Subject: [PATCH 36/44] test Signed-off-by: Jose Nino --- test/server/api_listener_test.cc | 46 +++++++++++++++++++------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index 003e95110341..a7624ae1176d 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -59,29 +59,37 @@ name: test_api_listener ASSERT_EQ("test_api_listener", http_api_listener.name()); ASSERT_EQ(ApiListener::Type::HttpApiListener, http_api_listener.type()); - ASSERT_NE(nullptr, http_api_listener.http()); + // FIXME: fix stats + // ASSERT_NE(nullptr, http_api_listener.http()); } -// TEST_F(ApiListenerTest, HttpApiListenerThrowsWithBadConfig) { -// const std::string yaml = R"EOF( -// name: test_api_listener -// address: -// socket_address: -// address: 127.0.0.1 -// port_value: 1234 -// api_listener: -// api_listener: -// "@type": type.googleapis.com/envoy.config.filter.network.tcp_proxy.v2.TcpProxy -// stat_prefix: tcp_stats -// cluster: cluster_0 -// )EOF"; +TEST_F(ApiListenerTest, HttpApiListenerThrowsWithBadConfig) { + const std::string yaml = R"EOF( +name: test_api_listener +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.api.v2.Cluster + name: cluster1 + type: EDS + eds_cluster_config: + eds_config: + path: eds path + )EOF"; -// const envoy::config::listener::v3alpha::Listener config = parseListenerFromV2Yaml(yaml); + const envoy::config::listener::v3alpha::Listener config = parseListenerFromV2Yaml(yaml); -// EXPECT_THROW_WITH_MESSAGE( -// HttpApiListener(config, *listener_manager_, config.name(), validation_visitor_), -// EnvoyException, "message"); -// } + EXPECT_THROW_WITH_MESSAGE( + HttpApiListener(config, *listener_manager_, config.name(), validation_visitor_), + EnvoyException, + "Unable to unpack as " + "envoy.extensions.filters.network.http_connection_manager.v3alpha.HttpConnectionManager: " + "[type.googleapis.com/envoy.api.v2.Cluster] {\n name: \"cluster1\"\n type: EDS\n " + "eds_cluster_config {\n eds_config {\n path: \"eds path\"\n }\n }\n}\n"); +} } // namespace Server } // namespace Envoy \ No newline at end of file From ba3d861bf9654431f990215592f5247d49afa952 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 11:58:59 -0800 Subject: [PATCH 37/44] spell Signed-off-by: Jose Nino --- test/server/api_listener_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index a7624ae1176d..0c88fae08ac7 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -59,7 +59,7 @@ name: test_api_listener ASSERT_EQ("test_api_listener", http_api_listener.name()); ASSERT_EQ(ApiListener::Type::HttpApiListener, http_api_listener.type()); - // FIXME: fix stats + // fix me: fix stats // ASSERT_NE(nullptr, http_api_listener.http()); } From 2116afa4f4867c08a0671f7afdfd8aaf4d9fa183 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Sat, 11 Jan 2020 14:44:16 -0800 Subject: [PATCH 38/44] stats Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 21 +++++++++++---------- test/server/api_listener_test.cc | 3 +-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index acae3fc18257..b51fedb7477a 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -289,20 +289,23 @@ void ConnectionManagerImpl::handleCodecException(const char* error) { void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { ASSERT(!codec_); codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); + + if (codec_->protocol() == Protocol::Http3) { + stats_.named_.downstream_cx_http3_total_.inc(); + stats_.named_.downstream_cx_http3_active_.inc(); + } else if (codec_->protocol() == Protocol::Http2) { + stats_.named_.downstream_cx_http2_total_.inc(); + stats_.named_.downstream_cx_http2_active_.inc(); + } else { + stats_.named_.downstream_cx_http1_total_.inc(); + stats_.named_.downstream_cx_http1_active_.inc(); + } } Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { // Http3 codec should have been instantiated by now. createCodec(data); - if (codec_->protocol() == Protocol::Http2) { - stats_.named_.downstream_cx_http2_total_.inc(); - stats_.named_.downstream_cx_http2_active_.inc(); - } else { - ASSERT(codec_->protocol() != Protocol::Http3); - stats_.named_.downstream_cx_http1_total_.inc(); - stats_.named_.downstream_cx_http1_active_.inc(); - } } bool redispatch; @@ -364,8 +367,6 @@ Network::FilterStatus ConnectionManagerImpl::onNewConnection() { Buffer::OwnedImpl dummy; createCodec(dummy); ASSERT(codec_->protocol() == Protocol::Http3); - stats_.named_.downstream_cx_http3_total_.inc(); - stats_.named_.downstream_cx_http3_active_.inc(); // Stop iterating through each filters for QUIC. Currently a QUIC connection // only supports one filter, HCM, and bypasses the onData() interface. Because // QUICHE already handles de-multiplexing. diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index 0c88fae08ac7..fd25b25f7da2 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -59,8 +59,7 @@ name: test_api_listener ASSERT_EQ("test_api_listener", http_api_listener.name()); ASSERT_EQ(ApiListener::Type::HttpApiListener, http_api_listener.type()); - // fix me: fix stats - // ASSERT_NE(nullptr, http_api_listener.http()); + ASSERT_NE(nullptr, http_api_listener.http()); } TEST_F(ApiListenerTest, HttpApiListenerThrowsWithBadConfig) { From e1c789efcab23c18bf0b562237804305894f8d17 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 14 Jan 2020 23:42:22 -0800 Subject: [PATCH 39/44] comments and test Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 12 +- source/server/listener_manager_impl.cc | 4 +- test/integration/BUILD | 9 ++ .../api_listener_integration_test.cc | 115 ++++++++++++++++++ 4 files changed, 135 insertions(+), 5 deletions(-) create mode 100644 test/integration/api_listener_integration_test.cc diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 605c73303b2f..67749ddacf82 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -290,15 +290,21 @@ void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { ASSERT(!codec_); codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); - if (codec_->protocol() == Protocol::Http3) { + switch (codec_->protocol()) { + case Protocol::Http3: stats_.named_.downstream_cx_http3_total_.inc(); stats_.named_.downstream_cx_http3_active_.inc(); - } else if (codec_->protocol() == Protocol::Http2) { + break; + case Protocol::Http2: stats_.named_.downstream_cx_http2_total_.inc(); stats_.named_.downstream_cx_http2_active_.inc(); - } else { + case Protocol::Http11: + case Protocol::Http10: stats_.named_.downstream_cx_http1_total_.inc(); stats_.named_.downstream_cx_http1_active_.inc(); + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; } } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 567c6e6f2e28..6b3c789a527c 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -333,8 +333,8 @@ bool ListenerManagerImpl::addOrUpdateListener( server_.messageValidationContext().staticValidationVisitor()); return true; } else { - ENVOY_LOG(debug, "listener {} can not be added because currently only one ApiListener is " - "allowed, and it can only be added via bootstrap configuration"); + ENVOY_LOG(warn, "listener {} can not be added because currently only one ApiListener is " + "allowed, and it can only be added via bootstrap configuration"); return false; } } diff --git a/test/integration/BUILD b/test/integration/BUILD index 710fadc9c81d..aba1bf881200 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -66,6 +66,15 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "api_listener_integration_test", + srcs = ["api_listener_integration_test.cc"], + deps = [ + ":http_integration_lib", + "//test/mocks/http:stream_encoder_mock", + ], +) + envoy_cc_test( name = "api_version_integration_test", srcs = ["api_version_integration_test.cc"], diff --git a/test/integration/api_listener_integration_test.cc b/test/integration/api_listener_integration_test.cc new file mode 100644 index 000000000000..aac1bc63b2f6 --- /dev/null +++ b/test/integration/api_listener_integration_test.cc @@ -0,0 +1,115 @@ +#include "test/integration/integration.h" +#include "test/mocks/http/stream_encoder.h" +#include "test/test_common/environment.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::ReturnRef; + +namespace Envoy { +namespace { + +class ApiListenerIntegrationTest : public BaseIntegrationTest, + public testing::TestWithParam { +public: + ApiListenerIntegrationTest() : BaseIntegrationTest(GetParam(), config()) { + use_lds_ = false; + autonomous_upstream_ = true; + } + + void SetUp() override { BaseIntegrationTest::initialize(); } + + void TearDown() override { + test_server_.reset(); + fake_upstreams_.clear(); + } + + static std::string config() { + return R"EOF( +admin: + access_log_path: /dev/null + address: + socket_address: + address: 127.0.0.1 + port_value: 0 +static_resources: + clusters: + name: cluster_0 + hosts: + socket_address: + address: 127.0.0.1 + port_value: 0 + listeners: + - name: default_listener + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + filter_chains: + filters: + - name: api_listener + address: + socket_address: + address: 127.0.0.1 + port_value: 1 + api_listener: + api_listener: + "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3alpha.HttpConnectionManager + stat_prefix: hcm + route_config: + virtual_hosts: + name: integration + routes: + route: + cluster: cluster_0 + match: + prefix: "/" + domains: "*" + name: route_config_0 + http_filters: + - name: envoy.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3alpha.Router + + )EOF"; + } + + NiceMock stream_encoder_; +}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, ApiListenerIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +TEST_P(ApiListenerIntegrationTest, Basic) { + ConditionalInitializer test_ran; + test_server_->server().dispatcher().post([this, &test_ran]() -> void { + ASSERT_EQ("api_listener", test_server_->server().listenerManager().apiListener()->name()); + ASSERT_NE(nullptr, test_server_->server().listenerManager().apiListener()->http()); + auto* http_api_listener = test_server_->server().listenerManager().apiListener()->http(); + + ON_CALL(stream_encoder_, getStream()).WillByDefault(ReturnRef(stream_encoder_.stream_)); + auto& stream_decoder = http_api_listener->newStream(stream_encoder_); + + // The AutonomousUpstream responds with 200 OK and a body of 10 bytes. + // In the http1 codec the end stream is encoded with encodeData and 0 bytes. + Http::TestHeaderMapImpl expected_response_headers{{":status", "200"}}; + EXPECT_CALL(stream_encoder_, encodeHeaders(_, false)); + EXPECT_CALL(stream_encoder_, encodeData(_, false)); + EXPECT_CALL(stream_encoder_, encodeData(BufferStringEqual(""), true)); + + // Send a headers-only request + stream_decoder.decodeHeaders( + Http::HeaderMapPtr(new Http::TestHeaderMapImpl{ + {":method", "GET"}, {":path", "/api"}, {":scheme", "http"}, {":authority", "host"}}), + true); + + test_ran.setReady(); + }); + test_ran.waitReady(); +} + +} // namespace +} // namespace Envoy \ No newline at end of file From 9b99e589127d7c932697b53f0ad6510de33e2be2 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 15 Jan 2020 09:32:06 -0800 Subject: [PATCH 40/44] factory context Signed-off-by: Jose Nino --- source/server/BUILD | 2 + source/server/api_listener_impl.cc | 61 +++------------------- source/server/api_listener_impl.h | 49 ++++------------- source/server/filter_chain_manager_impl.cc | 46 ++++++++++++++++ source/server/filter_chain_manager_impl.h | 47 +++++++++++++++++ source/server/listener_manager_impl.cc | 4 +- test/server/api_listener_test.cc | 7 +-- 7 files changed, 114 insertions(+), 102 deletions(-) diff --git a/source/server/BUILD b/source/server/BUILD index 94cd10617b3a..5ffea1f9e5c5 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -280,6 +280,7 @@ envoy_cc_library( ], deps = [ ":drain_manager_lib", + ":filter_chain_manager_lib", ":listener_manager_impl", "//include/envoy/network:connection_interface", "//include/envoy/server:api_listener_interface", @@ -378,6 +379,7 @@ envoy_cc_library( hdrs = ["filter_chain_manager_impl.h"], deps = [ ":filter_chain_factory_context_callback", + "//include/envoy/server:instance_interface", "//include/envoy/server:listener_manager_interface", "//include/envoy/server:transport_socket_config_interface", "//source/common/common:empty_string", diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index 7fdfe82821ea..dc9f7057aa48 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -17,71 +17,24 @@ namespace Envoy { namespace Server { ApiListenerImpl::ApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor) + ListenerManagerImpl& parent, const std::string& name) : config_(config), parent_(parent), name_(name), address_(Network::Address::resolveProtoAddress(config.address())), - validation_visitor_(validation_visitor), global_scope_(parent_.server_.stats().createScope("")), listener_scope_(parent_.server_.stats().createScope(fmt::format("listener.api.{}.", name_))), + factory_context_(parent_.server_, config_, *this, *global_scope_, *listener_scope_), read_callbacks_(SyntheticReadCallbacks(*this)) {} -AccessLog::AccessLogManager& ApiListenerImpl::accessLogManager() { - return parent_.server_.accessLogManager(); -} -Upstream::ClusterManager& ApiListenerImpl::clusterManager() { - return parent_.server_.clusterManager(); -} -Event::Dispatcher& ApiListenerImpl::dispatcher() { return parent_.server_.dispatcher(); } -Network::DrainDecision& ApiListenerImpl::drainDecision() { return *this; } -Grpc::Context& ApiListenerImpl::grpcContext() { return parent_.server_.grpcContext(); } -bool ApiListenerImpl::healthCheckFailed() { return parent_.server_.healthCheckFailed(); } -Tracing::HttpTracer& ApiListenerImpl::httpTracer() { return httpContext().tracer(); } -Http::Context& ApiListenerImpl::httpContext() { return parent_.server_.httpContext(); } -Init::Manager& ApiListenerImpl::initManager() { return parent_.server_.initManager(); } -const LocalInfo::LocalInfo& ApiListenerImpl::localInfo() const { - return parent_.server_.localInfo(); -} -Envoy::Runtime::RandomGenerator& ApiListenerImpl::random() { return parent_.server_.random(); } -Envoy::Runtime::Loader& ApiListenerImpl::runtime() { return parent_.server_.runtime(); } -Stats::Scope& ApiListenerImpl::scope() { return *global_scope_; } -Singleton::Manager& ApiListenerImpl::singletonManager() { - return parent_.server_.singletonManager(); -} -OverloadManager& ApiListenerImpl::overloadManager() { return parent_.server_.overloadManager(); } -ThreadLocal::Instance& ApiListenerImpl::threadLocal() { return parent_.server_.threadLocal(); } -Admin& ApiListenerImpl::admin() { return parent_.server_.admin(); } -const envoy::config::core::v3alpha::Metadata& ApiListenerImpl::listenerMetadata() const { - return config_.metadata(); -}; -envoy::config::core::v3alpha::TrafficDirection ApiListenerImpl::direction() const { - return config_.traffic_direction(); -}; -TimeSource& ApiListenerImpl::timeSource() { return api().timeSource(); } -ProtobufMessage::ValidationVisitor& ApiListenerImpl::messageValidationVisitor() { - return validation_visitor_; -} -Api::Api& ApiListenerImpl::api() { return parent_.server_.api(); } -ServerLifecycleNotifier& ApiListenerImpl::lifecycleNotifier() { - return parent_.server_.lifecycleNotifier(); -} -OptProcessContextRef ApiListenerImpl::processContext() { return parent_.server_.processContext(); } -Configuration::ServerFactoryContext& ApiListenerImpl::getServerFactoryContext() const { - return parent_.server_.serverFactoryContext(); -} -Stats::Scope& ApiListenerImpl::listenerScope() { return *listener_scope_; } - HttpApiListener::HttpApiListener(const envoy::config::listener::v3alpha::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor) - : ApiListenerImpl(config, parent, name, validation_visitor) { + ListenerManagerImpl& parent, const std::string& name) + : ApiListenerImpl(config, parent, name) { auto typed_config = MessageUtil::anyConvert< envoy::extensions::filters::network::http_connection_manager::v3alpha::HttpConnectionManager>( config.api_listener().api_listener()); - http_connection_manager_factory_ = - Envoy::Extensions::NetworkFilters::HttpConnectionManager::HttpConnectionManagerFactory:: - createHttpConnectionManagerFactoryFromProto(typed_config, *this, read_callbacks_); + http_connection_manager_factory_ = Envoy::Extensions::NetworkFilters::HttpConnectionManager:: + HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( + typed_config, factory_context_, read_callbacks_); } Http::ApiListener* HttpApiListener::http() { diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index d4204605f294..39c3d2901c74 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -18,6 +18,8 @@ #include "common/init/manager_impl.h" #include "common/stream_info/stream_info_impl.h" +#include "server/filter_chain_manager_impl.h" + namespace Envoy { namespace Server { @@ -28,7 +30,6 @@ class ListenerManagerImpl; * Thus it provides full access to Envoy's L7 features, e.g HTTP filters. */ class ApiListenerImpl : public ApiListener, - public Configuration::FactoryContext, public Network::DrainDecision, Logger::Loggable { public: @@ -39,46 +40,13 @@ class ApiListenerImpl : public ApiListener, // ApiListener absl::string_view name() const override { return name_; } - // TODO(junr03): move these functions into a concrete Server:Configuration::FactoryContext that - // takes the server's context, and the listener config in the constructor. The only challenging - // part is implementing the initManager() call. The function depends on the Listener's state for - // ListenerImpl, and will do so once ApiListeners are dynamic. - // Server::Configuration::FactoryContext - AccessLog::AccessLogManager& accessLogManager() override; - Upstream::ClusterManager& clusterManager() override; - Event::Dispatcher& dispatcher() override; - Network::DrainDecision& drainDecision() override; - Grpc::Context& grpcContext() override; - bool healthCheckFailed() override; - Tracing::HttpTracer& httpTracer() override; - Http::Context& httpContext() override; - Init::Manager& initManager() override; - const LocalInfo::LocalInfo& localInfo() const override; - Envoy::Runtime::RandomGenerator& random() override; - Envoy::Runtime::Loader& runtime() override; - Stats::Scope& scope() override; - Singleton::Manager& singletonManager() override; - OverloadManager& overloadManager() override; - ThreadLocal::Instance& threadLocal() override; - Admin& admin() override; - const envoy::config::core::v3alpha::Metadata& listenerMetadata() const override; - envoy::config::core::v3alpha::TrafficDirection direction() const override; - TimeSource& timeSource() override; - ProtobufMessage::ValidationVisitor& messageValidationVisitor() override; - Api::Api& api() override; - ServerLifecycleNotifier& lifecycleNotifier() override; - OptProcessContextRef processContext() override; - Configuration::ServerFactoryContext& getServerFactoryContext() const override; - Stats::Scope& listenerScope() override; - // Network::DrainDecision // TODO(junr03): hook up draining to listener state management. bool drainClose() const override { return false; } protected: ApiListenerImpl(const envoy::config::listener::v3alpha::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor); + ListenerManagerImpl& parent, const std::string& name); // Synthetic class that acts as a stub Network::ReadFilterCallbacks. // TODO(junr03): if we are able to separate the Network Filter aspects of the @@ -105,7 +73,7 @@ class ApiListenerImpl : public ApiListener, class SyntheticConnection : public Network::Connection { public: SyntheticConnection(SyntheticReadCallbacks& parent) - : parent_(parent), stream_info_(parent_.parent_.timeSource()), + : parent_(parent), stream_info_(parent_.parent_.factory_context_.timeSource()), options_(std::make_shared>()) {} // Network::FilterManager @@ -123,7 +91,9 @@ class ApiListenerImpl : public ApiListener, } void enableHalfClose(bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void close(Network::ConnectionCloseType) override {} - Event::Dispatcher& dispatcher() override { return parent_.parent_.dispatcher(); } + Event::Dispatcher& dispatcher() override { + return parent_.parent_.factory_context_.dispatcher(); + } uint64_t id() const override { return 12345; } std::string nextProtocol() const override { return EMPTY_STRING; } void noDelay(bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } @@ -170,17 +140,16 @@ class ApiListenerImpl : public ApiListener, ListenerManagerImpl& parent_; const std::string name_; Network::Address::InstanceConstSharedPtr address_; - ProtobufMessage::ValidationVisitor& validation_visitor_; Stats::ScopePtr global_scope_; Stats::ScopePtr listener_scope_; + FactoryContextImpl factory_context_; SyntheticReadCallbacks read_callbacks_; }; class HttpApiListener : public ApiListenerImpl { public: HttpApiListener(const envoy::config::listener::v3alpha::Listener& config, - ListenerManagerImpl& parent, const std::string& name, - ProtobufMessage::ValidationVisitor& validation_visitor); + ListenerManagerImpl& parent, const std::string& name); // ApiListener ApiListener::Type type() const override { return ApiListener::Type::HttpApiListener; } diff --git a/source/server/filter_chain_manager_impl.cc b/source/server/filter_chain_manager_impl.cc index 0a0fcdb3ce0f..5ff110597d96 100644 --- a/source/server/filter_chain_manager_impl.cc +++ b/source/server/filter_chain_manager_impl.cc @@ -577,5 +577,51 @@ Configuration::FilterChainFactoryContext& FilterChainManagerImpl::createFilterCh factory_contexts_.push_back(std::make_unique(parent_context_)); return *factory_contexts_.back(); } + +FactoryContextImpl::FactoryContextImpl(Server::Instance& server, + const envoy::config::listener::v3alpha::Listener& config, + Network::DrainDecision& drain_decision, + Stats::Scope& global_scope, Stats::Scope& listener_scope) + : server_(server), config_(config), drain_decision_(drain_decision), + global_scope_(global_scope), listener_scope_(listener_scope) {} + +AccessLog::AccessLogManager& FactoryContextImpl::accessLogManager() { + return server_.accessLogManager(); +} +Upstream::ClusterManager& FactoryContextImpl::clusterManager() { return server_.clusterManager(); } +Event::Dispatcher& FactoryContextImpl::dispatcher() { return server_.dispatcher(); } +Grpc::Context& FactoryContextImpl::grpcContext() { return server_.grpcContext(); } +bool FactoryContextImpl::healthCheckFailed() { return server_.healthCheckFailed(); } +Tracing::HttpTracer& FactoryContextImpl::httpTracer() { return server_.httpContext().tracer(); } +Http::Context& FactoryContextImpl::httpContext() { return server_.httpContext(); } +Init::Manager& FactoryContextImpl::initManager() { return server_.initManager(); } +const LocalInfo::LocalInfo& FactoryContextImpl::localInfo() const { return server_.localInfo(); } +Envoy::Runtime::RandomGenerator& FactoryContextImpl::random() { return server_.random(); } +Envoy::Runtime::Loader& FactoryContextImpl::runtime() { return server_.runtime(); } +Stats::Scope& FactoryContextImpl::scope() { return global_scope_; } +Singleton::Manager& FactoryContextImpl::singletonManager() { return server_.singletonManager(); } +OverloadManager& FactoryContextImpl::overloadManager() { return server_.overloadManager(); } +ThreadLocal::SlotAllocator& FactoryContextImpl::threadLocal() { return server_.threadLocal(); } +Admin& FactoryContextImpl::admin() { return server_.admin(); } +TimeSource& FactoryContextImpl::timeSource() { return server_.timeSource(); } +ProtobufMessage::ValidationVisitor& FactoryContextImpl::messageValidationVisitor() { + return server_.messageValidationContext().staticValidationVisitor(); +} +Api::Api& FactoryContextImpl::api() { return server_.api(); } +ServerLifecycleNotifier& FactoryContextImpl::lifecycleNotifier() { + return server_.lifecycleNotifier(); +} +OptProcessContextRef FactoryContextImpl::processContext() { return server_.processContext(); } +Configuration::ServerFactoryContext& FactoryContextImpl::getServerFactoryContext() const { + return server_.serverFactoryContext(); +} +const envoy::config::core::v3alpha::Metadata& FactoryContextImpl::listenerMetadata() const { + return config_.metadata(); +} +envoy::config::core::v3alpha::TrafficDirection FactoryContextImpl::direction() const { + return config_.traffic_direction(); +} +Network::DrainDecision& FactoryContextImpl::drainDecision() { return drain_decision_; } +Stats::Scope& FactoryContextImpl::listenerScope() { return listener_scope_; } } // namespace Server } // namespace Envoy diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h index f23f9cbe5934..7e174eea5fd5 100644 --- a/source/server/filter_chain_manager_impl.h +++ b/source/server/filter_chain_manager_impl.h @@ -6,6 +6,7 @@ #include "envoy/config/listener/v3alpha/listener_components.pb.h" #include "envoy/network/drain_decision.h" #include "envoy/server/filter_config.h" +#include "envoy/server/instance.h" #include "envoy/server/transport_socket_config.h" #include "envoy/thread_local/thread_local.h" @@ -73,6 +74,52 @@ class FilterChainFactoryContextImpl : public Configuration::FilterChainFactoryCo Configuration::FactoryContext& parent_context_; }; +/** + * Implementation of FactoryContext wrapping a Server::Instance and some listener components. + */ +class FactoryContextImpl : public Configuration::FactoryContext { +public: + FactoryContextImpl(Server::Instance& server, + const envoy::config::listener::v3alpha::Listener& config, + Network::DrainDecision& drain_decision, Stats::Scope& global_scope, + Stats::Scope& listener_scope); + + // Configuration::FactoryContext + AccessLog::AccessLogManager& accessLogManager() override; + Upstream::ClusterManager& clusterManager() override; + Event::Dispatcher& dispatcher() override; + Grpc::Context& grpcContext() override; + bool healthCheckFailed() override; + Tracing::HttpTracer& httpTracer() override; + Http::Context& httpContext() override; + Init::Manager& initManager() override; + const LocalInfo::LocalInfo& localInfo() const override; + Envoy::Runtime::RandomGenerator& random() override; + Envoy::Runtime::Loader& runtime() override; + Stats::Scope& scope() override; + Singleton::Manager& singletonManager() override; + OverloadManager& overloadManager() override; + ThreadLocal::SlotAllocator& threadLocal() override; + Admin& admin() override; + TimeSource& timeSource() override; + ProtobufMessage::ValidationVisitor& messageValidationVisitor() override; + Api::Api& api() override; + ServerLifecycleNotifier& lifecycleNotifier() override; + OptProcessContextRef processContext() override; + Configuration::ServerFactoryContext& getServerFactoryContext() const override; + const envoy::config::core::v3alpha::Metadata& listenerMetadata() const override; + envoy::config::core::v3alpha::TrafficDirection direction() const override; + Network::DrainDecision& drainDecision() override; + Stats::Scope& listenerScope() override; + +private: + Server::Instance& server_; + const envoy::config::listener::v3alpha::Listener& config_; + Network::DrainDecision& drain_decision_; + Stats::Scope& global_scope_; + Stats::Scope& listener_scope_; +}; + /** * Implementation of FilterChainManager. */ diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 6b3c789a527c..0db97adff182 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -328,9 +328,7 @@ bool ListenerManagerImpl::addOrUpdateListener( if (!api_listener_ && !added_via_api) { // TODO(junr03): dispatch to different concrete constructors when there are other // ApiListenerImpl derived classes. - api_listener_ = std::make_unique( - config, *this, config.name(), - server_.messageValidationContext().staticValidationVisitor()); + api_listener_ = std::make_unique(config, *this, config.name()); return true; } else { ENVOY_LOG(warn, "listener {} can not be added because currently only one ApiListener is " diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index fd25b25f7da2..c82997c79aa5 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -25,7 +25,6 @@ class ApiListenerTest : public testing::Test { NiceMock listener_factory_; NiceMock worker_factory_; std::unique_ptr listener_manager_; - NiceMock validation_visitor_; }; TEST_F(ApiListenerTest, HttpApiListener) { @@ -54,8 +53,7 @@ name: test_api_listener const envoy::config::listener::v3alpha::Listener config = parseListenerFromV2Yaml(yaml); - auto http_api_listener = - HttpApiListener(config, *listener_manager_, config.name(), validation_visitor_); + auto http_api_listener = HttpApiListener(config, *listener_manager_, config.name()); ASSERT_EQ("test_api_listener", http_api_listener.name()); ASSERT_EQ(ApiListener::Type::HttpApiListener, http_api_listener.type()); @@ -82,8 +80,7 @@ name: test_api_listener const envoy::config::listener::v3alpha::Listener config = parseListenerFromV2Yaml(yaml); EXPECT_THROW_WITH_MESSAGE( - HttpApiListener(config, *listener_manager_, config.name(), validation_visitor_), - EnvoyException, + HttpApiListener(config, *listener_manager_, config.name()), EnvoyException, "Unable to unpack as " "envoy.extensions.filters.network.http_connection_manager.v3alpha.HttpConnectionManager: " "[type.googleapis.com/envoy.api.v2.Cluster] {\n name: \"cluster1\"\n type: EDS\n " From cd640b3d96155c67aedec7880a8a02ae23a79390 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 15 Jan 2020 12:21:55 -0800 Subject: [PATCH 41/44] hcm Signed-off-by: Jose Nino --- source/common/http/conn_manager_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 67749ddacf82..bfffbb6ef04f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -298,6 +298,7 @@ void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { case Protocol::Http2: stats_.named_.downstream_cx_http2_total_.inc(); stats_.named_.downstream_cx_http2_active_.inc(); + break; case Protocol::Http11: case Protocol::Http10: stats_.named_.downstream_cx_http1_total_.inc(); From 4d47fa5c6819cdd824b5b85503e5387b3eb54d26 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 15 Jan 2020 13:24:26 -0800 Subject: [PATCH 42/44] optional reference_wrapper Signed-off-by: Jose Nino --- include/envoy/server/api_listener.h | 1 + include/envoy/server/listener_manager.h | 2 +- source/server/listener_manager_impl.cc | 4 ++++ source/server/listener_manager_impl.h | 2 +- .../api_listener_integration_test.cc | 20 ++++++++++++------- test/mocks/server/mocks.h | 2 +- test/server/listener_manager_impl_test.cc | 12 +++++------ 7 files changed, 27 insertions(+), 16 deletions(-) diff --git a/include/envoy/server/api_listener.h b/include/envoy/server/api_listener.h index 4a3d8139ac61..30bce6674b1e 100644 --- a/include/envoy/server/api_listener.h +++ b/include/envoy/server/api_listener.h @@ -33,6 +33,7 @@ class ApiListener { }; using ApiListenerPtr = std::unique_ptr; +using ApiListenerOptRef = absl::optional>; } // namespace Server } // namespace Envoy \ No newline at end of file diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 42ea136d041b..b1b96f71e0ae 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -220,7 +220,7 @@ class ListenerManager { /** * @return ApiListener* the server's API Listener if it exists, nullptr if it does not. */ - virtual ApiListener* apiListener() PURE; + virtual ApiListenerOptRef apiListener() PURE; }; } // namespace Server diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index b4614714236c..48f413bf16ce 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -857,5 +857,9 @@ Network::ListenSocketFactorySharedPtr ListenerManagerImpl::createListenSocketFac listener.bindToPort(), listener.name(), reuse_port); } +ApiListenerOptRef ListenerManagerImpl::apiListener() { + return api_listener_ ? ApiListenerOptRef(std::ref(*api_listener_)) : absl::nullopt; +} + } // namespace Server } // namespace Envoy diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 5b5f32cd38da..f0ff37da50d9 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -150,7 +150,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggableserver().dispatcher().post([this, &test_ran]() -> void { - ASSERT_EQ("api_listener", test_server_->server().listenerManager().apiListener()->name()); - ASSERT_NE(nullptr, test_server_->server().listenerManager().apiListener()->http()); - auto* http_api_listener = test_server_->server().listenerManager().apiListener()->http(); + ASSERT_TRUE(test_server_->server().listenerManager().apiListener().has_value()); + ASSERT_EQ("api_listener", test_server_->server().listenerManager().apiListener()->get().name()); + ASSERT_NE(nullptr, test_server_->server().listenerManager().apiListener()->get().http()); + auto* http_api_listener = test_server_->server().listenerManager().apiListener()->get().http(); ON_CALL(stream_encoder_, getStream()).WillByDefault(ReturnRef(stream_encoder_.stream_)); auto& stream_decoder = http_api_listener->newStream(stream_encoder_); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index d43dfcc51d02..393edb319bb8 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -294,7 +294,7 @@ class MockListenerManager : public ListenerManager { MOCK_METHOD0(stopWorkers, void()); MOCK_METHOD0(beginListenerUpdate, void()); MOCK_METHOD1(endListenerUpdate, void(ListenerManager::FailureStates&&)); - MOCK_METHOD0(apiListener, ApiListener*()); + MOCK_METHOD0(apiListener, ApiListenerOptRef()); }; class MockServerLifecycleNotifier : public ServerLifecycleNotifier { diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index a130f6e311f0..af1e466e922c 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -3704,7 +3704,7 @@ name: test_api_listener ASSERT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", false)); EXPECT_EQ(0U, manager_->listeners().size()); - EXPECT_NE(nullptr, manager_->apiListener()); + ASSERT_TRUE(manager_->apiListener().has_value()); } TEST_F(ListenerManagerImplWithRealFiltersTest, ApiListenerNotAllowedAddedViaApi) { @@ -3733,7 +3733,7 @@ name: test_api_listener ASSERT_FALSE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", true)); EXPECT_EQ(0U, manager_->listeners().size()); - EXPECT_EQ(nullptr, manager_->apiListener()); + ASSERT_FALSE(manager_->apiListener().has_value()); } TEST_F(ListenerManagerImplWithRealFiltersTest, ApiListenerOnlyOneApiListener) { @@ -3785,15 +3785,15 @@ name: test_api_listener_2 ASSERT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", false)); EXPECT_EQ(0U, manager_->listeners().size()); - EXPECT_NE(nullptr, manager_->apiListener()); - EXPECT_EQ("test_api_listener", manager_->apiListener()->name()); + ASSERT_TRUE(manager_->apiListener().has_value()); + EXPECT_EQ("test_api_listener", manager_->apiListener()->get().name()); // Only one ApiListener is added. ASSERT_FALSE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), "", false)); EXPECT_EQ(0U, manager_->listeners().size()); // The original ApiListener is there. - EXPECT_NE(nullptr, manager_->apiListener()); - EXPECT_EQ("test_api_listener", manager_->apiListener()->name()); + ASSERT_TRUE(manager_->apiListener().has_value()); + EXPECT_EQ("test_api_listener", manager_->apiListener()->get().name()); } } // namespace From 835adbbe28de18152b2e78a8311506eb558f0a08 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 16 Jan 2020 18:03:05 -0800 Subject: [PATCH 43/44] comments Signed-off-by: Jose Nino --- include/envoy/http/api_listener.h | 1 + include/envoy/server/api_listener.h | 4 +- include/envoy/server/listener_manager.h | 2 +- source/common/http/conn_manager_impl.cc | 2 - source/common/protobuf/utility.h | 16 +++ source/server/api_listener_impl.cc | 8 +- source/server/api_listener_impl.h | 2 +- test/integration/BUILD | 1 + .../api_listener_integration_test.cc | 101 ++++++++---------- test/server/api_listener_test.cc | 2 +- 10 files changed, 74 insertions(+), 65 deletions(-) diff --git a/include/envoy/http/api_listener.h b/include/envoy/http/api_listener.h index deebe031c60a..59c83a5dd93f 100644 --- a/include/envoy/http/api_listener.h +++ b/include/envoy/http/api_listener.h @@ -27,6 +27,7 @@ class ApiListener { }; using ApiListenerPtr = std::unique_ptr; +using ApiListenerOptRef = absl::optional>; } // namespace Http } // namespace Envoy \ No newline at end of file diff --git a/include/envoy/server/api_listener.h b/include/envoy/server/api_listener.h index 30bce6674b1e..da5937f5d680 100644 --- a/include/envoy/server/api_listener.h +++ b/include/envoy/server/api_listener.h @@ -27,9 +27,9 @@ class ApiListener { virtual Type type() const PURE; /** - * @return Http::ApiListener IFF type() == Type::HttpApiListener, otherwise nullptr. + * @return valid ref IFF type() == Type::HttpApiListener, otherwise nullopt. */ - virtual Http::ApiListener* http() PURE; + virtual Http::ApiListenerOptRef http() PURE; }; using ApiListenerPtr = std::unique_ptr; diff --git a/include/envoy/server/listener_manager.h b/include/envoy/server/listener_manager.h index 3b9ae0e9fc27..57a7e97549a2 100644 --- a/include/envoy/server/listener_manager.h +++ b/include/envoy/server/listener_manager.h @@ -218,7 +218,7 @@ class ListenerManager { // weak_ptr to its caller. This would allow the caller to verify if the // ApiListener is available to receive API calls on it. /** - * @return ApiListener* the server's API Listener if it exists, nullptr if it does not. + * @return the server's API Listener if it exists, nullopt if it does not. */ virtual ApiListenerOptRef apiListener() PURE; }; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index cb51b7f5cb0a..232fe99d132f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -304,8 +304,6 @@ void ConnectionManagerImpl::createCodec(Buffer::Instance& data) { stats_.named_.downstream_cx_http1_total_.inc(); stats_.named_.downstream_cx_http1_active_.inc(); break; - default: - NOT_REACHED_GCOVR_EXCL_LINE; } } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 76bd9042f526..b2d4b828be1a 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -303,6 +303,22 @@ class MessageUtil { return typed_message; }; + /** + * Convert and validate from google.protobuf.Any to a typed message. + * @param message source google.protobuf.Any message. + * + * @return MessageType the typed message inside the Any. + * @throw ProtoValidationException if the message does not satisfy its type constraints. + */ + template + static inline MessageType + anyConvertAndValidate(const ProtobufWkt::Any& message, + ProtobufMessage::ValidationVisitor& validation_visitor) { + MessageType typed_message = anyConvert(message); + validate(typed_message, validation_visitor); + return typed_message; + }; + /** * Convert between two protobufs via a JSON round-trip. This is used to translate arbitrary * messages to/from google.protobuf.Struct. diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index 470007040197..e551e44dcfd9 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -28,20 +28,20 @@ ApiListenerImpl::ApiListenerImpl(const envoy::config::listener::v3::Listener& co HttpApiListener::HttpApiListener(const envoy::config::listener::v3::Listener& config, ListenerManagerImpl& parent, const std::string& name) : ApiListenerImpl(config, parent, name) { - auto typed_config = MessageUtil::anyConvert< + auto typed_config = MessageUtil::anyConvertAndValidate< envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager>( - config.api_listener().api_listener()); + config.api_listener().api_listener(), factory_context_.messageValidationVisitor()); http_connection_manager_factory_ = Envoy::Extensions::NetworkFilters::HttpConnectionManager:: HttpConnectionManagerFactory::createHttpConnectionManagerFactoryFromProto( typed_config, factory_context_, read_callbacks_); } -Http::ApiListener* HttpApiListener::http() { +Http::ApiListenerOptRef HttpApiListener::http() { if (!http_connection_manager_) { http_connection_manager_ = http_connection_manager_factory_(); } - return http_connection_manager_.get(); + return Http::ApiListenerOptRef(std::ref(*http_connection_manager_)); } } // namespace Server diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index d71df8e62932..8fc7e91d582c 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -153,7 +153,7 @@ class HttpApiListener : public ApiListenerImpl { // ApiListener ApiListener::Type type() const override { return ApiListener::Type::HttpApiListener; } - Http::ApiListener* http() override; + Http::ApiListenerOptRef http() override; private: // Need to store the factory due to the shared_ptrs that need to be kept alive: date provider, diff --git a/test/integration/BUILD b/test/integration/BUILD index 1c2c858f4fc9..e9a3ee8940cd 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -72,6 +72,7 @@ envoy_cc_test( deps = [ ":http_integration_lib", "//test/mocks/http:stream_encoder_mock", + "//test/server:utility_lib", ], ) diff --git a/test/integration/api_listener_integration_test.cc b/test/integration/api_listener_integration_test.cc index a6b2e0c5e6b1..0fa47eb8275c 100644 --- a/test/integration/api_listener_integration_test.cc +++ b/test/integration/api_listener_integration_test.cc @@ -1,5 +1,6 @@ #include "test/integration/integration.h" #include "test/mocks/http/stream_encoder.h" +#include "test/server/utility.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" @@ -14,70 +15,61 @@ namespace { class ApiListenerIntegrationTest : public BaseIntegrationTest, public testing::TestWithParam { public: - ApiListenerIntegrationTest() : BaseIntegrationTest(GetParam(), config()) { + ApiListenerIntegrationTest() : BaseIntegrationTest(GetParam(), bootstrap_config()) { use_lds_ = false; autonomous_upstream_ = true; } - void SetUp() override { BaseIntegrationTest::initialize(); } + void SetUp() override { + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + // currently ApiListener does not trigger this wait + // https://github.com/envoyproxy/envoy/blob/0b92c58d08d28ba7ef0ed5aaf44f90f0fccc5dce/test/integration/integration.cc#L454 + // Thus, the ApiListener has to be added in addition to the already existing listener in the + // config. + bootstrap.mutable_static_resources()->add_listeners()->MergeFrom( + Server::parseListenerFromV2Yaml(api_listener_config())); + }); + BaseIntegrationTest::initialize(); + } void TearDown() override { test_server_.reset(); fake_upstreams_.clear(); } - static std::string config() { - return R"EOF( -admin: - access_log_path: /dev/null - address: - socket_address: - address: 127.0.0.1 - port_value: 0 -static_resources: - clusters: - name: cluster_0 - load_assignment: - cluster_name: my_cds_cluster - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: 127.0.0.1 - port_value: 0 - listeners: - - name: default_listener - address: - socket_address: - address: 127.0.0.1 - port_value: 0 + static std::string bootstrap_config() { + // At least one empty filter chain needs to be specified. + return ConfigHelper::BASE_CONFIG + R"EOF( filter_chains: filters: - - name: api_listener - address: - socket_address: - address: 127.0.0.1 - port_value: 1 - api_listener: - api_listener: - "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager - stat_prefix: hcm - route_config: - virtual_hosts: - name: integration - routes: - route: - cluster: cluster_0 - match: - prefix: "/" - domains: "*" - name: route_config_0 - http_filters: - - name: envoy.router - typed_config: - "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + )EOF"; + } + static std::string api_listener_config() { + return R"EOF( +name: api_listener +address: + socket_address: + address: 127.0.0.1 + port_value: 1 +api_listener: + api_listener: + "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager + stat_prefix: hcm + route_config: + virtual_hosts: + name: integration + routes: + route: + cluster: cluster_0 + match: + prefix: "/" + domains: "*" + name: route_config_0 + http_filters: + - name: envoy.router + typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router )EOF"; } @@ -93,11 +85,12 @@ TEST_P(ApiListenerIntegrationTest, Basic) { test_server_->server().dispatcher().post([this, &test_ran]() -> void { ASSERT_TRUE(test_server_->server().listenerManager().apiListener().has_value()); ASSERT_EQ("api_listener", test_server_->server().listenerManager().apiListener()->get().name()); - ASSERT_NE(nullptr, test_server_->server().listenerManager().apiListener()->get().http()); - auto* http_api_listener = test_server_->server().listenerManager().apiListener()->get().http(); + ASSERT_TRUE(test_server_->server().listenerManager().apiListener()->get().http().has_value()); + auto& http_api_listener = + test_server_->server().listenerManager().apiListener()->get().http()->get(); ON_CALL(stream_encoder_, getStream()).WillByDefault(ReturnRef(stream_encoder_.stream_)); - auto& stream_decoder = http_api_listener->newStream(stream_encoder_); + auto& stream_decoder = http_api_listener.newStream(stream_encoder_); // The AutonomousUpstream responds with 200 OK and a body of 10 bytes. // In the http1 codec the end stream is encoded with encodeData and 0 bytes. diff --git a/test/server/api_listener_test.cc b/test/server/api_listener_test.cc index f476cf5f876c..ccf9c643ee55 100644 --- a/test/server/api_listener_test.cc +++ b/test/server/api_listener_test.cc @@ -57,7 +57,7 @@ name: test_api_listener ASSERT_EQ("test_api_listener", http_api_listener.name()); ASSERT_EQ(ApiListener::Type::HttpApiListener, http_api_listener.type()); - ASSERT_NE(nullptr, http_api_listener.http()); + ASSERT_TRUE(http_api_listener.http().has_value()); } TEST_F(ApiListenerTest, HttpApiListenerThrowsWithBadConfig) { From 77a5570f2be97ea781972b4ade80c7b08261257c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 16 Jan 2020 18:26:49 -0800 Subject: [PATCH 44/44] nits Signed-off-by: Jose Nino --- source/server/api_listener_impl.cc | 6 +++--- source/server/api_listener_impl.h | 23 +++++++++++++---------- source/server/listener_manager_impl.cc | 2 +- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/source/server/api_listener_impl.cc b/source/server/api_listener_impl.cc index e551e44dcfd9..f99025468381 100644 --- a/source/server/api_listener_impl.cc +++ b/source/server/api_listener_impl.cc @@ -16,8 +16,8 @@ namespace Envoy { namespace Server { -ApiListenerImpl::ApiListenerImpl(const envoy::config::listener::v3::Listener& config, - ListenerManagerImpl& parent, const std::string& name) +ApiListenerImplBase::ApiListenerImplBase(const envoy::config::listener::v3::Listener& config, + ListenerManagerImpl& parent, const std::string& name) : config_(config), parent_(parent), name_(name), address_(Network::Address::resolveProtoAddress(config.address())), global_scope_(parent_.server_.stats().createScope("")), @@ -27,7 +27,7 @@ ApiListenerImpl::ApiListenerImpl(const envoy::config::listener::v3::Listener& co HttpApiListener::HttpApiListener(const envoy::config::listener::v3::Listener& config, ListenerManagerImpl& parent, const std::string& name) - : ApiListenerImpl(config, parent, name) { + : ApiListenerImplBase(config, parent, name) { auto typed_config = MessageUtil::anyConvertAndValidate< envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager>( config.api_listener().api_listener(), factory_context_.messageValidationVisitor()); diff --git a/source/server/api_listener_impl.h b/source/server/api_listener_impl.h index 8fc7e91d582c..3b5dfdcded01 100644 --- a/source/server/api_listener_impl.h +++ b/source/server/api_listener_impl.h @@ -26,12 +26,11 @@ namespace Server { class ListenerManagerImpl; /** - * Listener that provides a handle to inject HTTP calls into envoy via an Http::ConnectionManager. - * Thus it provides full access to Envoy's L7 features, e.g HTTP filters. + * Base class all ApiListeners. */ -class ApiListenerImpl : public ApiListener, - public Network::DrainDecision, - Logger::Loggable { +class ApiListenerImplBase : public ApiListener, + public Network::DrainDecision, + Logger::Loggable { public: // TODO(junr03): consider moving Envoy Mobile's SyntheticAddressImpl to Envoy in order to return // that rather than this semi-real one. @@ -45,8 +44,8 @@ class ApiListenerImpl : public ApiListener, bool drainClose() const override { return false; } protected: - ApiListenerImpl(const envoy::config::listener::v3::Listener& config, ListenerManagerImpl& parent, - const std::string& name); + ApiListenerImplBase(const envoy::config::listener::v3::Listener& config, + ListenerManagerImpl& parent, const std::string& name); // Synthetic class that acts as a stub Network::ReadFilterCallbacks. // TODO(junr03): if we are able to separate the Network Filter aspects of the @@ -54,7 +53,7 @@ class ApiListenerImpl : public ApiListener, // need this and the SyntheticConnection stub anymore. class SyntheticReadCallbacks : public Network::ReadFilterCallbacks { public: - SyntheticReadCallbacks(ApiListenerImpl& parent) + SyntheticReadCallbacks(ApiListenerImplBase& parent) : parent_(parent), connection_(SyntheticConnection(*this)) {} // Network::ReadFilterCallbacks @@ -132,7 +131,7 @@ class ApiListenerImpl : public ApiListener, Network::ConnectionSocket::OptionsSharedPtr options_; }; - ApiListenerImpl& parent_; + ApiListenerImplBase& parent_; SyntheticConnection connection_; }; @@ -146,7 +145,11 @@ class ApiListenerImpl : public ApiListener, SyntheticReadCallbacks read_callbacks_; }; -class HttpApiListener : public ApiListenerImpl { +/** + * ApiListener that provides a handle to inject HTTP calls into Envoy via an + * Http::ConnectionManager. Thus, it provides full access to Envoy's L7 features, e.g HTTP filters. + */ +class HttpApiListener : public ApiListenerImplBase { public: HttpApiListener(const envoy::config::listener::v3::Listener& config, ListenerManagerImpl& parent, const std::string& name); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 66a82ef68060..c9e289291f1a 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -327,7 +327,7 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: if (config.has_api_listener()) { if (!api_listener_ && !added_via_api) { // TODO(junr03): dispatch to different concrete constructors when there are other - // ApiListenerImpl derived classes. + // ApiListenerImplBase derived classes. api_listener_ = std::make_unique(config, *this, config.name()); return true; } else {