From cd9e7ae2fbd8c86888ad0fc7d0a276272c1549f9 Mon Sep 17 00:00:00 2001 From: Elisha Ziskind Date: Mon, 3 Jun 2019 18:38:56 -0400 Subject: [PATCH 01/13] Fix server lifecycle notifier when callback list is empty (#7103) Signed-off-by: Elisha Ziskind --- source/server/server.cc | 24 +++++++++++------------- test/server/server_test.cc | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/source/server/server.cc b/source/server/server.cc index f9a02669284f..903c4b3c9910 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -575,23 +575,21 @@ void InstanceImpl::notifyCallbacksForStage(Stage stage, Event::PostCb completion } } + // Wrap completion_cb so that it only gets invoked when all callbacks for this stage + // have finished their work. + std::shared_ptr cb_guard(new Event::PostCb([] {}), + [this, completion_cb](Event::PostCb* cb) { + ASSERT(std::this_thread::get_id() == main_thread_id_); + completion_cb(); + delete cb; + }); + auto it2 = stage_completable_callbacks_.find(stage); if (it2 != stage_completable_callbacks_.end()) { - ASSERT(!it2->second.empty()); - // Wrap completion_cb so that it only gets invoked when all callbacks for this stage - // have finished their work. - auto completion_cb_count = std::make_shared(it2->second.size()); - Event::PostCb wrapped_cb = [this, completion_cb, completion_cb_count] { - ASSERT(std::this_thread::get_id() == main_thread_id_); - if (--*completion_cb_count == 0) { - completion_cb(); - } - }; + ENVOY_LOG(info, "Notifying {} callback(s) with completion.", it2->second.size()); for (const StageCallbackWithCompletion& callback : it2->second) { - callback(wrapped_cb); + callback([cb_guard] { (*cb_guard)(); }); } - } else { - completion_cb(); } } diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 5e41e2016686..d18bf0a56c62 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -201,6 +201,27 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, ServerInstanceImplTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); +TEST_P(ServerInstanceImplTest, EmptyShutdownLifecycleNotifications) { + absl::Notification started; + + auto server_thread = Thread::threadFactoryForTest().createThread([&] { + initialize("test/server/node_bootstrap.yaml"); + auto startup_handle = server_->registerCallback(ServerLifecycleNotifier::Stage::Startup, + [&] { started.Notify(); }); + auto shutdown_handle = server_->registerCallback(ServerLifecycleNotifier::Stage::ShutdownExit, + [&](Event::PostCb) { FAIL(); }); + shutdown_handle = nullptr; // unregister callback + server_->run(); + startup_handle = nullptr; + server_ = nullptr; + thread_local_ = nullptr; + }); + + started.WaitForNotification(); + server_->dispatcher().post([&] { server_->shutdown(); }); + server_thread->join(); +} + TEST_P(ServerInstanceImplTest, LifecycleNotifications) { bool startup = false, shutdown = false, shutdown_with_completion = false; absl::Notification started, shutdown_begin, completion_block, completion_done; From 82ccd5572931ed6c735833212b0fdc09935b6f3d Mon Sep 17 00:00:00 2001 From: Fred Douglas <43351173+fredlas@users.noreply.github.com> Date: Mon, 3 Jun 2019 15:58:46 -0700 Subject: [PATCH 02/13] config: move SubscriptionCallbacks to ctor, pointers become refs (#7122) The Subscription interface was receiving its callbacks in start(). No usage actually needed the ability to defer providing callbacks until after construction. Furthermore, some Subscription implementations want to hold onto the callbacks, and had to use a pointer rather than a reference. Risk Level: low Signed-off-by: Fred Douglas --- include/envoy/config/subscription.h | 4 +-- .../common/config/delta_subscription_impl.cc | 17 +++++------ .../common/config/delta_subscription_impl.h | 6 ++-- .../config/filesystem_subscription_impl.cc | 25 ++++++---------- .../config/filesystem_subscription_impl.h | 12 ++++---- .../config/grpc_mux_subscription_impl.cc | 21 +++++++------ .../config/grpc_mux_subscription_impl.h | 7 +++-- source/common/config/grpc_subscription_impl.h | 17 ++++++----- .../common/config/http_subscription_impl.cc | 24 +++++++-------- source/common/config/http_subscription_impl.h | 8 ++--- source/common/config/subscription_factory.cc | 19 ++++++------ source/common/config/subscription_factory.h | 4 ++- source/common/router/rds_impl.cc | 4 +-- source/common/router/scoped_rds.cc | 2 +- source/common/router/scoped_rds.h | 2 +- source/common/router/vhds.cc | 4 +-- source/common/router/vhds.h | 2 +- source/common/secret/sds_api.cc | 4 +-- source/common/upstream/cds_api_impl.cc | 2 +- source/common/upstream/cds_api_impl.h | 2 +- source/common/upstream/eds.cc | 4 +-- source/server/lds_api.cc | 4 +-- .../config/delta_subscription_impl_test.cc | 2 +- .../config/delta_subscription_test_harness.h | 4 +-- .../filesystem_subscription_impl_test.cc | 7 +++-- .../filesystem_subscription_test_harness.h | 4 +-- .../config/grpc_subscription_impl_test.cc | 2 +- .../config/grpc_subscription_test_harness.h | 6 ++-- .../config/http_subscription_test_harness.h | 6 ++-- .../config/subscription_factory_test.cc | 30 +++++++++---------- test/common/router/vhds_test.cc | 16 +++++----- test/mocks/config/mocks.h | 5 ++-- 32 files changed, 134 insertions(+), 142 deletions(-) diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index a60a74e4d2f3..707fab98ea3b 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -68,10 +68,8 @@ class Subscription { * Start a configuration subscription asynchronously. This should be called once and will continue * to fetch throughout the lifetime of the Subscription object. * @param resources set of resource names to fetch. - * @param callbacks the callbacks to be notified of configuration updates. The callback must not - * result in the deletion of the Subscription object. */ - virtual void start(const std::set& resources, SubscriptionCallbacks& callbacks) PURE; + virtual void start(const std::set& resource_names) PURE; /** * Update the resources to fetch. diff --git a/source/common/config/delta_subscription_impl.cc b/source/common/config/delta_subscription_impl.cc index e3d285ec8010..d2ee8cc76f1f 100644 --- a/source/common/config/delta_subscription_impl.cc +++ b/source/common/config/delta_subscription_impl.cc @@ -14,12 +14,12 @@ DeltaSubscriptionImpl::DeltaSubscriptionImpl( const LocalInfo::LocalInfo& local_info, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, absl::string_view type_url, Runtime::RandomGenerator& random, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, SubscriptionStats stats, - std::chrono::milliseconds init_fetch_timeout) + const RateLimitSettings& rate_limit_settings, SubscriptionCallbacks& callbacks, + SubscriptionStats stats, std::chrono::milliseconds init_fetch_timeout) : grpc_stream_(this, std::move(async_client), service_method, random, dispatcher, scope, rate_limit_settings), - type_url_(type_url), local_info_(local_info), stats_(stats), dispatcher_(dispatcher), - init_fetch_timeout_(init_fetch_timeout) {} + type_url_(type_url), local_info_(local_info), callbacks_(callbacks), stats_(stats), + dispatcher_(dispatcher), init_fetch_timeout_(init_fetch_timeout) {} void DeltaSubscriptionImpl::pause() { state_->pause(); } void DeltaSubscriptionImpl::resume() { @@ -28,12 +28,11 @@ void DeltaSubscriptionImpl::resume() { } // Config::Subscription -void DeltaSubscriptionImpl::start(const std::set& resources, - SubscriptionCallbacks& callbacks) { - state_ = std::make_unique(type_url_, resources, callbacks, local_info_, - init_fetch_timeout_, dispatcher_, stats_); +void DeltaSubscriptionImpl::start(const std::set& resource_names) { + state_ = std::make_unique( + type_url_, resource_names, callbacks_, local_info_, init_fetch_timeout_, dispatcher_, stats_); grpc_stream_.establishNewStream(); - updateResources(resources); + updateResources(resource_names); } void DeltaSubscriptionImpl::updateResources(const std::set& update_to_these_names) { diff --git a/source/common/config/delta_subscription_impl.h b/source/common/config/delta_subscription_impl.h index 846de51d9eef..7389947b0aa1 100644 --- a/source/common/config/delta_subscription_impl.h +++ b/source/common/config/delta_subscription_impl.h @@ -30,13 +30,14 @@ class DeltaSubscriptionImpl : public Subscription, const Protobuf::MethodDescriptor& service_method, absl::string_view type_url, Runtime::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, - SubscriptionStats stats, std::chrono::milliseconds init_fetch_timeout); + SubscriptionCallbacks& callbacks, SubscriptionStats stats, + std::chrono::milliseconds init_fetch_timeout); void pause(); void resume(); // Config::Subscription - void start(const std::set& resources, SubscriptionCallbacks& callbacks) override; + void start(const std::set& resource_names) override; void updateResources(const std::set& update_to_these_names) override; // Config::GrpcStreamCallbacks @@ -80,6 +81,7 @@ class DeltaSubscriptionImpl : public Subscription, std::queue ack_queue_; const LocalInfo::LocalInfo& local_info_; + SubscriptionCallbacks& callbacks_; SubscriptionStats stats_; Event::Dispatcher& dispatcher_; std::chrono::milliseconds init_fetch_timeout_; diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index eb5c3db6d9bc..c9bcaedd3d7d 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -9,12 +9,11 @@ namespace Envoy { namespace Config { FilesystemSubscriptionImpl::FilesystemSubscriptionImpl( - Event::Dispatcher& dispatcher, absl::string_view path, SubscriptionStats stats, - ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) - : path_(path), watcher_(dispatcher.createFilesystemWatcher()), stats_(stats), api_(api), - validation_visitor_(validation_visitor) { - watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t events) { - UNREFERENCED_PARAMETER(events); + Event::Dispatcher& dispatcher, absl::string_view path, SubscriptionCallbacks& callbacks, + SubscriptionStats stats, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) + : path_(path), watcher_(dispatcher.createFilesystemWatcher()), callbacks_(callbacks), + stats_(stats), api_(api), validation_visitor_(validation_visitor) { + watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) { if (started_) { refresh(); } @@ -22,19 +21,13 @@ FilesystemSubscriptionImpl::FilesystemSubscriptionImpl( } // Config::Subscription -void FilesystemSubscriptionImpl::start(const std::set& resources, - Config::SubscriptionCallbacks& callbacks) { - // We report all discovered resources in the watched file. - UNREFERENCED_PARAMETER(resources); - callbacks_ = &callbacks; +void FilesystemSubscriptionImpl::start(const std::set&) { started_ = true; // Attempt to read in case there is a file there already. refresh(); } -void FilesystemSubscriptionImpl::updateResources(const std::set& resources) { - // We report all discovered resources in the watched file. - UNREFERENCED_PARAMETER(resources); +void FilesystemSubscriptionImpl::updateResources(const std::set&) { // Bump stats for consistence behavior with other xDS. stats_.update_attempt_.inc(); } @@ -47,7 +40,7 @@ void FilesystemSubscriptionImpl::refresh() { try { MessageUtil::loadFromFile(path_, message, validation_visitor_, api_); config_update_available = true; - callbacks_->onConfigUpdate(message.resources(), message.version_info()); + callbacks_.onConfigUpdate(message.resources(), message.version_info()); stats_.version_.set(HashUtil::xxHash64(message.version_info())); stats_.update_success_.inc(); ENVOY_LOG(debug, "Filesystem config update accepted for {}: {}", path_, message.DebugString()); @@ -60,7 +53,7 @@ void FilesystemSubscriptionImpl::refresh() { ENVOY_LOG(warn, "Filesystem config update failure: {}", e.what()); stats_.update_failure_.inc(); } - callbacks_->onConfigUpdateFailed(&e); + callbacks_.onConfigUpdateFailed(&e); } } diff --git a/source/common/config/filesystem_subscription_impl.h b/source/common/config/filesystem_subscription_impl.h index 1307b8508493..6ff089208dc8 100644 --- a/source/common/config/filesystem_subscription_impl.h +++ b/source/common/config/filesystem_subscription_impl.h @@ -21,14 +21,14 @@ class FilesystemSubscriptionImpl : public Config::Subscription, Logger::Loggable { public: FilesystemSubscriptionImpl(Event::Dispatcher& dispatcher, absl::string_view path, - SubscriptionStats stats, + SubscriptionCallbacks& callbacks, SubscriptionStats stats, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); // Config::Subscription - void start(const std::set& resources, - Config::SubscriptionCallbacks& callbacks) override; - - void updateResources(const std::set& resources) override; + // We report all discovered resources in the watched file, so the resource names arguments are + // unused, and updateResources is a no-op (other than updating a stat). + void start(const std::set&) override; + void updateResources(const std::set&) override; private: void refresh(); @@ -36,7 +36,7 @@ class FilesystemSubscriptionImpl : public Config::Subscription, bool started_{}; const std::string path_; std::unique_ptr watcher_; - SubscriptionCallbacks* callbacks_{}; + SubscriptionCallbacks& callbacks_; SubscriptionStats stats_; Api::Api& api_; ProtobufMessage::ValidationVisitor& validation_visitor_; diff --git a/source/common/config/grpc_mux_subscription_impl.cc b/source/common/config/grpc_mux_subscription_impl.cc index 5b7ffe2d084d..8383036d4e52 100644 --- a/source/common/config/grpc_mux_subscription_impl.cc +++ b/source/common/config/grpc_mux_subscription_impl.cc @@ -9,22 +9,21 @@ namespace Envoy { namespace Config { -GrpcMuxSubscriptionImpl::GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, SubscriptionStats stats, +GrpcMuxSubscriptionImpl::GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, + SubscriptionCallbacks& callbacks, + SubscriptionStats stats, absl::string_view type_url, Event::Dispatcher& dispatcher, std::chrono::milliseconds init_fetch_timeout) - : grpc_mux_(grpc_mux), stats_(stats), type_url_(type_url), dispatcher_(dispatcher), - init_fetch_timeout_(init_fetch_timeout) {} + : grpc_mux_(grpc_mux), callbacks_(callbacks), stats_(stats), type_url_(type_url), + dispatcher_(dispatcher), init_fetch_timeout_(init_fetch_timeout) {} // Config::Subscription -void GrpcMuxSubscriptionImpl::start(const std::set& resources, - SubscriptionCallbacks& callbacks) { - callbacks_ = &callbacks; - +void GrpcMuxSubscriptionImpl::start(const std::set& resources) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { ENVOY_LOG(warn, "gRPC config: initial fetch timed out for {}", type_url_); - callbacks_->onConfigUpdateFailed(nullptr); + callbacks_.onConfigUpdateFailed(nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } @@ -54,7 +53,7 @@ void GrpcMuxSubscriptionImpl::onConfigUpdate( // supply those versions to onConfigUpdate() along with the xDS response ("system") // version_info. This way, both types of versions can be tracked and exposed for debugging by // the configuration update targets. - callbacks_->onConfigUpdate(resources, version_info); + callbacks_.onConfigUpdate(resources, version_info); stats_.update_success_.inc(); stats_.update_attempt_.inc(); stats_.version_.set(HashUtil::xxHash64(version_info)); @@ -73,11 +72,11 @@ void GrpcMuxSubscriptionImpl::onConfigUpdateFailed(const EnvoyException* e) { ENVOY_LOG(warn, "gRPC config for {} rejected: {}", type_url_, e->what()); } stats_.update_attempt_.inc(); - callbacks_->onConfigUpdateFailed(e); + callbacks_.onConfigUpdateFailed(e); } std::string GrpcMuxSubscriptionImpl::resourceName(const ProtobufWkt::Any& resource) { - return callbacks_->resourceName(resource); + return callbacks_.resourceName(resource); } void GrpcMuxSubscriptionImpl::disableInitFetchTimeoutTimer() { diff --git a/source/common/config/grpc_mux_subscription_impl.h b/source/common/config/grpc_mux_subscription_impl.h index 933ddb8cff1d..10d08ff49f3c 100644 --- a/source/common/config/grpc_mux_subscription_impl.h +++ b/source/common/config/grpc_mux_subscription_impl.h @@ -17,12 +17,13 @@ class GrpcMuxSubscriptionImpl : public Subscription, GrpcMuxCallbacks, Logger::Loggable { public: - GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, SubscriptionStats stats, absl::string_view type_url, + GrpcMuxSubscriptionImpl(GrpcMux& grpc_mux, SubscriptionCallbacks& callbacks, + SubscriptionStats stats, absl::string_view type_url, Event::Dispatcher& dispatcher, std::chrono::milliseconds init_fetch_timeout); // Config::Subscription - void start(const std::set& resources, SubscriptionCallbacks& callbacks) override; + void start(const std::set& resource_names) override; void updateResources(const std::set& update_to_these_names) override; // Config::GrpcMuxCallbacks @@ -35,9 +36,9 @@ class GrpcMuxSubscriptionImpl : public Subscription, void disableInitFetchTimeoutTimer(); GrpcMux& grpc_mux_; + SubscriptionCallbacks& callbacks_; SubscriptionStats stats_; const std::string type_url_; - SubscriptionCallbacks* callbacks_{}; GrpcMuxWatchPtr watch_{}; Event::Dispatcher& dispatcher_; std::chrono::milliseconds init_fetch_timeout_; diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index 0c90b8a47f06..caace8dcf2cc 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -17,18 +17,18 @@ class GrpcSubscriptionImpl : public Config::Subscription { GrpcSubscriptionImpl(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, const Protobuf::MethodDescriptor& service_method, absl::string_view type_url, - SubscriptionStats stats, Stats::Scope& scope, - const RateLimitSettings& rate_limit_settings, + SubscriptionCallbacks& callbacks, SubscriptionStats stats, + Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, std::chrono::milliseconds init_fetch_timeout) - : grpc_mux_(local_info, std::move(async_client), dispatcher, service_method, random, scope, - rate_limit_settings), - grpc_mux_subscription_(grpc_mux_, stats, type_url, dispatcher, init_fetch_timeout) {} + : callbacks_(callbacks), grpc_mux_(local_info, std::move(async_client), dispatcher, + service_method, random, scope, rate_limit_settings), + grpc_mux_subscription_(grpc_mux_, callbacks_, stats, type_url, dispatcher, + init_fetch_timeout) {} // Config::Subscription - void start(const std::set& resources, - Config::SubscriptionCallbacks& callbacks) override { + void start(const std::set& resource_names) override { // Subscribe first, so we get failure callbacks if grpc_mux_.start() fails. - grpc_mux_subscription_.start(resources, callbacks); + grpc_mux_subscription_.start(resource_names); grpc_mux_.start(); } @@ -39,6 +39,7 @@ class GrpcSubscriptionImpl : public Config::Subscription { GrpcMuxImpl& grpcMux() { return grpc_mux_; } private: + Config::SubscriptionCallbacks& callbacks_; GrpcMuxImpl grpc_mux_; GrpcMuxSubscriptionImpl grpc_mux_subscription_; }; diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 41915a0561af..78a5e6caf9c0 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -20,12 +20,13 @@ HttpSubscriptionImpl::HttpSubscriptionImpl( const std::string& remote_cluster_name, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, std::chrono::milliseconds refresh_interval, std::chrono::milliseconds request_timeout, const Protobuf::MethodDescriptor& service_method, - SubscriptionStats stats, std::chrono::milliseconds init_fetch_timeout, + SubscriptionCallbacks& callbacks, SubscriptionStats stats, + std::chrono::milliseconds init_fetch_timeout, ProtobufMessage::ValidationVisitor& validation_visitor) : Http::RestApiFetcher(cm, remote_cluster_name, dispatcher, random, refresh_interval, request_timeout), - stats_(stats), dispatcher_(dispatcher), init_fetch_timeout_(init_fetch_timeout), - validation_visitor_(validation_visitor) { + callbacks_(callbacks), stats_(stats), dispatcher_(dispatcher), + init_fetch_timeout_(init_fetch_timeout), validation_visitor_(validation_visitor) { request_.mutable_node()->CopyFrom(local_info.node()); ASSERT(service_method.options().HasExtension(google::api::http)); const auto& http_rule = service_method.options().GetExtension(google::api::http); @@ -34,21 +35,18 @@ HttpSubscriptionImpl::HttpSubscriptionImpl( } // Config::Subscription -void HttpSubscriptionImpl::start(const std::set& resources, - Config::SubscriptionCallbacks& callbacks) { - ASSERT(callbacks_ == nullptr); - +void HttpSubscriptionImpl::start(const std::set& resource_names) { if (init_fetch_timeout_.count() > 0) { init_fetch_timeout_timer_ = dispatcher_.createTimer([this]() -> void { ENVOY_LOG(warn, "REST config: initial fetch timed out for", path_); - callbacks_->onConfigUpdateFailed(nullptr); + callbacks_.onConfigUpdateFailed(nullptr); }); init_fetch_timeout_timer_->enableTimer(init_fetch_timeout_); } - Protobuf::RepeatedPtrField resources_vector(resources.begin(), resources.end()); + Protobuf::RepeatedPtrField resources_vector(resource_names.begin(), + resource_names.end()); request_.mutable_resource_names()->Swap(&resources_vector); - callbacks_ = &callbacks; initialize(); } @@ -82,14 +80,14 @@ void HttpSubscriptionImpl::parseResponse(const Http::Message& response) { return; } try { - callbacks_->onConfigUpdate(message.resources(), message.version_info()); + callbacks_.onConfigUpdate(message.resources(), message.version_info()); request_.set_version_info(message.version_info()); stats_.version_.set(HashUtil::xxHash64(request_.version_info())); stats_.update_success_.inc(); } catch (const EnvoyException& e) { ENVOY_LOG(warn, "REST config update rejected: {}", e.what()); stats_.update_rejected_.inc(); - callbacks_->onConfigUpdateFailed(&e); + callbacks_.onConfigUpdateFailed(&e); } } @@ -103,7 +101,7 @@ void HttpSubscriptionImpl::onFetchFailure(const EnvoyException* e) { void HttpSubscriptionImpl::handleFailure(const EnvoyException* e) { stats_.update_failure_.inc(); - callbacks_->onConfigUpdateFailed(e); + callbacks_.onConfigUpdateFailed(e); } void HttpSubscriptionImpl::disableInitFetchTimeoutTimer() { diff --git a/source/common/config/http_subscription_impl.h b/source/common/config/http_subscription_impl.h index d8df983e7650..452ba132582a 100644 --- a/source/common/config/http_subscription_impl.h +++ b/source/common/config/http_subscription_impl.h @@ -24,13 +24,13 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, const std::string& remote_cluster_name, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, std::chrono::milliseconds refresh_interval, std::chrono::milliseconds request_timeout, - const Protobuf::MethodDescriptor& service_method, SubscriptionStats stats, + const Protobuf::MethodDescriptor& service_method, + SubscriptionCallbacks& callbacks, SubscriptionStats stats, std::chrono::milliseconds init_fetch_timeout, ProtobufMessage::ValidationVisitor& validation_visitor); // Config::Subscription - void start(const std::set& resources, - Config::SubscriptionCallbacks& callbacks) override; + void start(const std::set& resource_names) override; void updateResources(const std::set& update_to_these_names) override; // Http::RestApiFetcher @@ -45,8 +45,8 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, std::string path_; Protobuf::RepeatedPtrField resources_; - Config::SubscriptionCallbacks* callbacks_{}; envoy::api::v2::DiscoveryRequest request_; + Config::SubscriptionCallbacks& callbacks_; SubscriptionStats stats_; Event::Dispatcher& dispatcher_; std::chrono::milliseconds init_fetch_timeout_; diff --git a/source/common/config/subscription_factory.cc b/source/common/config/subscription_factory.cc index 65df13859192..84eda4fd253f 100644 --- a/source/common/config/subscription_factory.cc +++ b/source/common/config/subscription_factory.cc @@ -16,14 +16,14 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, Stats::Scope& scope, const std::string& rest_method, const std::string& grpc_method, absl::string_view type_url, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api) { + Api::Api& api, SubscriptionCallbacks& callbacks) { std::unique_ptr result; SubscriptionStats stats = Utility::generateStats(scope); switch (config.config_source_specifier_case()) { case envoy::api::v2::core::ConfigSource::kPath: { Utility::checkFilesystemSubscriptionBackingPath(config.path(), api); - result = std::make_unique(dispatcher, config.path(), stats, - validation_visitor, api); + result = std::make_unique( + dispatcher, config.path(), callbacks, stats, validation_visitor, api); break; } case envoy::api::v2::core::ConfigSource::kApiConfigSource: { @@ -40,8 +40,8 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( local_info, cm, api_config_source.cluster_names()[0], dispatcher, random, Utility::apiConfigSourceRefreshDelay(api_config_source), Utility::apiConfigSourceRequestTimeout(api_config_source), - *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), stats, - Utility::configSourceInitialFetchTimeout(config), validation_visitor); + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), callbacks, + stats, Utility::configSourceInitialFetchTimeout(config), validation_visitor); break; case envoy::api::v2::core::ApiConfigSource::GRPC: result = std::make_unique( @@ -51,7 +51,7 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( ->create(), dispatcher, random, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method), type_url, - stats, scope, Utility::parseRateLimitSettings(api_config_source), + callbacks, stats, scope, Utility::parseRateLimitSettings(api_config_source), Utility::configSourceInitialFetchTimeout(config)); break; case envoy::api::v2::core::ApiConfigSource::DELTA_GRPC: { @@ -62,8 +62,8 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( api_config_source, scope) ->create(), dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method), - type_url, random, scope, Utility::parseRateLimitSettings(api_config_source), stats, - Utility::configSourceInitialFetchTimeout(config)); + type_url, random, scope, Utility::parseRateLimitSettings(api_config_source), callbacks, + stats, Utility::configSourceInitialFetchTimeout(config)); break; } default: @@ -73,7 +73,8 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( } case envoy::api::v2::core::ConfigSource::kAds: { result = std::make_unique( - cm.adsMux(), stats, type_url, dispatcher, Utility::configSourceInitialFetchTimeout(config)); + cm.adsMux(), callbacks, stats, type_url, dispatcher, + Utility::configSourceInitialFetchTimeout(config)); break; } default: diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h index d0efdd08585f..1ce63fa85901 100644 --- a/source/common/config/subscription_factory.h +++ b/source/common/config/subscription_factory.h @@ -27,13 +27,15 @@ class SubscriptionFactory { * service description). * @param validation_visitor message validation visitor instance. * @param api reference to the Api object + * @param callbacks the callbacks needed by all Subscription objects, to deliver config updates. + * The callbacks must not result in the deletion of the Subscription object. */ static std::unique_ptr subscriptionFromConfigSource( const envoy::api::v2::core::ConfigSource& config, const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, Stats::Scope& scope, const std::string& rest_method, const std::string& grpc_method, absl::string_view type_url, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api); + Api::Api& api, SubscriptionCallbacks& callbacks); }; } // namespace Config diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 10541d9f697b..37dc4491db90 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -59,7 +59,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager) : route_config_name_(rds.route_config_name()), factory_context_(factory_context), init_target_(fmt::format("RdsRouteConfigSubscription {}", route_config_name_), - [this]() { subscription_->start({route_config_name_}, *this); }), + [this]() { subscription_->start({route_config_name_}); }), scope_(factory_context.scope().createScope(stat_prefix + "rds." + route_config_name_ + ".")), stat_prefix_(stat_prefix), stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}), route_config_provider_manager_(route_config_provider_manager), @@ -73,7 +73,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( "envoy.api.v2.RouteDiscoveryService.FetchRoutes", "envoy.api.v2.RouteDiscoveryService.StreamRoutes", Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), - factory_context.messageValidationVisitor(), factory_context.api()); + factory_context.messageValidationVisitor(), factory_context.api(), *this); config_update_info_ = std::make_unique( factory_context.timeSource(), factory_context.messageValidationVisitor()); diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index d79f98fc4cc6..9a1d5889c368 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -54,7 +54,7 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription( "envoy.api.v2.ScopedRoutesDiscoveryService.StreamScopedRoutes", Grpc::Common::typeUrl( envoy::api::v2::ScopedRouteConfiguration().GetDescriptor()->full_name()), - validation_visitor_, factory_context.api()); + validation_visitor_, factory_context.api(), *this); } void ScopedRdsConfigSubscription::onConfigUpdate( diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 1d666eee070d..020cdd54fa3f 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -82,7 +82,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio const std::string& name() const { return name_; } // Envoy::Config::ConfigSubscriptionCommonBase - void start() override { subscription_->start({}, *this); } + void start() override { subscription_->start({}); } // Envoy::Config::SubscriptionCallbacks void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index f856d0231072..eb39ec65b238 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -25,7 +25,7 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, SubscriptionFactoryFunction factory_function) : config_update_info_(config_update_info), init_target_(fmt::format("VhdsConfigSubscription {}", config_update_info_->routeConfigName()), - [this]() { subscription_->start({}, *this); }), + [this]() { subscription_->start({}); }), scope_(factory_context.scope().createScope(stat_prefix + "vhds." + config_update_info_->routeConfigName() + ".")), stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), @@ -46,7 +46,7 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), *scope_, "none", "envoy.api.v2.VirtualHostDiscoveryService.DeltaVirtualHosts", Grpc::Common::typeUrl(envoy::api::v2::route::VirtualHost().GetDescriptor()->full_name()), - factory_context.messageValidationVisitor(), factory_context.api()); + factory_context.messageValidationVisitor(), factory_context.api(), *this); } void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 4aa6e44e3afe..1767b98db720 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -42,7 +42,7 @@ typedef std::unique_ptr (*SubscriptionFactoryFuncti const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, Event::Dispatcher&, Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, Stats::Scope&, const std::string&, const std::string&, absl::string_view, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api&); + Api::Api&, Envoy::Config::SubscriptionCallbacks&); class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, Logger::Loggable { diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 71bc5f1c7bd4..4354866552d5 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -83,8 +83,8 @@ void SdsApi::initialize() { "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets", Grpc::Common::typeUrl(envoy::api::v2::auth::Secret().GetDescriptor()->full_name()), - validation_visitor_, api_); - subscription_->start({sds_config_name_}, *this); + validation_visitor_, api_, *this); + subscription_->start({sds_config_name_}); } } // namespace Secret diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index ba848552b942..580bd4f2912f 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -42,7 +42,7 @@ CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, Clu cds_config, local_info, dispatcher, cm, random, *scope_, "envoy.api.v2.ClusterDiscoveryService.FetchClusters", grpc_method, Grpc::Common::typeUrl(envoy::api::v2::Cluster().GetDescriptor()->full_name()), - validation_visitor, api); + validation_visitor, api, *this); } void CdsApiImpl::onConfigUpdate(const Protobuf::RepeatedPtrField& resources, diff --git a/source/common/upstream/cds_api_impl.h b/source/common/upstream/cds_api_impl.h index 5a94f35de52c..2dbed586e459 100644 --- a/source/common/upstream/cds_api_impl.h +++ b/source/common/upstream/cds_api_impl.h @@ -28,7 +28,7 @@ class CdsApiImpl : public CdsApi, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api); // Upstream::CdsApi - void initialize() override { subscription_->start({}, *this); } + void initialize() override { subscription_->start({}); } void setInitializedCb(std::function callback) override { initialize_callback_ = callback; } diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 0e91f11a91d9..37f133551be7 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -30,10 +30,10 @@ EdsClusterImpl::EdsClusterImpl( "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints", "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints", Grpc::Common::typeUrl(envoy::api::v2::ClusterLoadAssignment().GetDescriptor()->full_name()), - factory_context.messageValidationVisitor(), factory_context.api()); + factory_context.messageValidationVisitor(), factory_context.api(), *this); } -void EdsClusterImpl::startPreInit() { subscription_->start({cluster_name_}, *this); } +void EdsClusterImpl::startPreInit() { subscription_->start({cluster_name_}); } void EdsClusterImpl::BatchUpdateHelper::batchUpdate(PrioritySet::HostUpdateCb& host_update_cb) { std::unordered_map updated_hosts; diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 9a92bdb89f54..b0f297f49948 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -22,14 +22,14 @@ LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, ListenerManager& lm, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api) : listener_manager_(lm), scope_(scope.createScope("listener_manager.lds.")), cm_(cm), - init_target_("LDS", [this]() { subscription_->start({}, *this); }), + init_target_("LDS", [this]() { subscription_->start({}); }), validation_visitor_(validation_visitor) { subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( lds_config, local_info, dispatcher, cm, random, *scope_, "envoy.api.v2.ListenerDiscoveryService.FetchListeners", "envoy.api.v2.ListenerDiscoveryService.StreamListeners", Grpc::Common::typeUrl(envoy::api::v2::Listener().GetDescriptor()->full_name()), - validation_visitor_, api); + validation_visitor_, api, *this); Config::Utility::checkLocalInfo("lds", local_info); init_manager.add(init_target_); } diff --git a/test/common/config/delta_subscription_impl_test.cc b/test/common/config/delta_subscription_impl_test.cc index 2f8577a393c1..7ed82165a09b 100644 --- a/test/common/config/delta_subscription_impl_test.cc +++ b/test/common/config/delta_subscription_impl_test.cc @@ -108,7 +108,7 @@ TEST_F(DeltaSubscriptionImplTest, NoGrpcStream) { // start() also tries to start the GrpcStream. So, have that attempt return nullptr. EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(nullptr)); EXPECT_CALL(async_stream_, sendMessage(_, _)).Times(0); - subscription_->start({"name1"}, callbacks_); + subscription_->start({"name1"}); subscription_->updateResources({"name1", "name2"}); } diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index 61d40bb2f83a..12b6d5d8e4e6 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -34,7 +34,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { subscription_ = std::make_unique( local_info_, std::unique_ptr(async_client_), dispatcher_, *method_descriptor_, Config::TypeUrl::get().ClusterLoadAssignment, random_, stats_store_, - rate_limit_settings_, stats_, init_fetch_timeout); + rate_limit_settings_, callbacks_, stats_, init_fetch_timeout); } ~DeltaSubscriptionTestHarness() { @@ -51,7 +51,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); last_cluster_names_ = cluster_names; expectSendMessage(last_cluster_names_, ""); - subscription_->start(cluster_names, callbacks_); + subscription_->start(cluster_names); } void expectSendMessage(const std::set& cluster_names, diff --git a/test/common/config/filesystem_subscription_impl_test.cc b/test/common/config/filesystem_subscription_impl_test.cc index 22eb872cb164..51efbd9a9e00 100644 --- a/test/common/config/filesystem_subscription_impl_test.cc +++ b/test/common/config/filesystem_subscription_impl_test.cc @@ -43,9 +43,10 @@ TEST(MiscFilesystemSubscriptionImplTest, BadWatch) { auto* watcher = new Filesystem::MockWatcher(); EXPECT_CALL(dispatcher, createFilesystemWatcher_()).WillOnce(Return(watcher)); EXPECT_CALL(*watcher, addWatch(_, _, _)).WillOnce(Throw(EnvoyException("bad path"))); - EXPECT_THROW_WITH_MESSAGE( - FilesystemSubscriptionImpl(dispatcher, "##!@/dev/null", stats, validation_visitor, *api), - EnvoyException, "bad path"); + NiceMock> callbacks; + EXPECT_THROW_WITH_MESSAGE(FilesystemSubscriptionImpl(dispatcher, "##!@/dev/null", callbacks, + stats, validation_visitor, *api), + EnvoyException, "bad path"); } } // namespace diff --git a/test/common/config/filesystem_subscription_test_harness.h b/test/common/config/filesystem_subscription_test_harness.h index c2ed1bfc3f7f..9a14fc73dd5c 100644 --- a/test/common/config/filesystem_subscription_test_harness.h +++ b/test/common/config/filesystem_subscription_test_harness.h @@ -30,7 +30,7 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { FilesystemSubscriptionTestHarness() : path_(TestEnvironment::temporaryPath("eds.json")), api_(Api::createApiForTest(stats_store_)), dispatcher_(api_->allocateDispatcher()), - subscription_(*dispatcher_, path_, stats_, validation_visitor_, *api_) {} + subscription_(*dispatcher_, path_, callbacks_, stats_, validation_visitor_, *api_) {} ~FilesystemSubscriptionTestHarness() { if (::access(path_.c_str(), F_OK) != -1) { @@ -41,7 +41,7 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { void startSubscription(const std::set& cluster_names) override { std::ifstream config_file(path_); file_at_start_ = config_file.good(); - subscription_.start(cluster_names, callbacks_); + subscription_.start(cluster_names); } void updateResources(const std::set& cluster_names) override { diff --git a/test/common/config/grpc_subscription_impl_test.cc b/test/common/config/grpc_subscription_impl_test.cc index 490a74c228e3..7b5302ff4661 100644 --- a/test/common/config/grpc_subscription_impl_test.cc +++ b/test/common/config/grpc_subscription_impl_test.cc @@ -18,7 +18,7 @@ TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); EXPECT_CALL(random_, random()); EXPECT_CALL(*timer_, enableTimer(_)); - subscription_->start({"cluster0", "cluster1"}, callbacks_); + subscription_->start({"cluster0", "cluster1"}); verifyStats(2, 0, 0, 1, 0); // Ensure this doesn't cause an issue by sending a request, since we don't // have a gRPC stream. diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index 5c23300dab28..097329b70d00 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -44,8 +44,8 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { })); subscription_ = std::make_unique( local_info_, std::unique_ptr(async_client_), dispatcher_, random_, - *method_descriptor_, Config::TypeUrl::get().ClusterLoadAssignment, stats_, stats_store_, - rate_limit_settings_, init_fetch_timeout); + *method_descriptor_, Config::TypeUrl::get().ClusterLoadAssignment, callbacks_, stats_, + stats_store_, rate_limit_settings_, init_fetch_timeout); } ~GrpcSubscriptionTestHarness() override { EXPECT_CALL(async_stream_, sendMessage(_, false)); } @@ -79,7 +79,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); last_cluster_names_ = cluster_names; expectSendMessage(last_cluster_names_, ""); - subscription_->start(cluster_names, callbacks_); + subscription_->start(cluster_names); } void deliverConfigUpdate(const std::vector& cluster_names, diff --git a/test/common/config/http_subscription_test_harness.h b/test/common/config/http_subscription_test_harness.h index 7cf202ec4635..c6d0a0572cbf 100644 --- a/test/common/config/http_subscription_test_harness.h +++ b/test/common/config/http_subscription_test_harness.h @@ -47,8 +47,8 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { })); subscription_ = std::make_unique( local_info_, cm_, "eds_cluster", dispatcher_, random_gen_, std::chrono::milliseconds(1), - std::chrono::milliseconds(1000), *method_descriptor_, stats_, init_fetch_timeout, - validation_visitor_); + std::chrono::milliseconds(1000), *method_descriptor_, callbacks_, stats_, + init_fetch_timeout, validation_visitor_); } ~HttpSubscriptionTestHarness() { @@ -102,7 +102,7 @@ class HttpSubscriptionTestHarness : public SubscriptionTestHarness { version_ = ""; cluster_names_ = cluster_names; expectSendMessage(cluster_names, ""); - subscription_->start(cluster_names, callbacks_); + subscription_->start(cluster_names); } void updateResources(const std::set& cluster_names) override { diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_test.cc index fbfce63c43f5..0c6f0fffa795 100644 --- a/test/common/config/subscription_factory_test.cc +++ b/test/common/config/subscription_factory_test.cc @@ -39,7 +39,7 @@ class SubscriptionFactoryTest : public testing::Test { config, local_info_, dispatcher_, cm_, random_, stats_store_, "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints", "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints", - Config::TypeUrl::get().ClusterLoadAssignment, validation_visitor_, *api_); + Config::TypeUrl::get().ClusterLoadAssignment, validation_visitor_, *api_, callbacks_); } Upstream::MockClusterManager cm_; @@ -188,14 +188,13 @@ TEST_F(SubscriptionFactoryTest, FilesystemSubscription) { EXPECT_CALL(dispatcher_, createFilesystemWatcher_()).WillOnce(Return(watcher)); EXPECT_CALL(*watcher, addWatch(test_path, _, _)); EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); - subscriptionFromConfigSource(config)->start({"foo"}, callbacks_); + subscriptionFromConfigSource(config)->start({"foo"}); } TEST_F(SubscriptionFactoryTest, FilesystemSubscriptionNonExistentFile) { envoy::api::v2::core::ConfigSource config; config.set_path("/blahblah"); - EXPECT_THROW_WITH_MESSAGE(subscriptionFromConfigSource(config)->start({"foo"}, callbacks_), - EnvoyException, + EXPECT_THROW_WITH_MESSAGE(subscriptionFromConfigSource(config)->start({"foo"}), EnvoyException, "envoy::api::v2::Path must refer to an existing path in the system: " "'/blahblah' does not exist") } @@ -211,9 +210,8 @@ TEST_F(SubscriptionFactoryTest, LegacySubscription) { EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_THROW_WITH_REGEX( - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_), EnvoyException, - "REST_LEGACY no longer a supported ApiConfigSource.*"); + EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config)->start({"static_cluster"}), + EnvoyException, "REST_LEGACY no longer a supported ApiConfigSource.*"); } TEST_F(SubscriptionFactoryTest, HttpSubscriptionCustomRequestTimeout) { @@ -234,7 +232,7 @@ TEST_F(SubscriptionFactoryTest, HttpSubscriptionCustomRequestTimeout) { EXPECT_CALL( cm_.async_client_, send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(5000)))); - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_); + subscriptionFromConfigSource(config)->start({"static_cluster"}); } TEST_F(SubscriptionFactoryTest, HttpSubscription) { @@ -262,7 +260,7 @@ TEST_F(SubscriptionFactoryTest, HttpSubscription) { return &http_request_; })); EXPECT_CALL(http_request_, cancel()); - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_); + subscriptionFromConfigSource(config)->start({"static_cluster"}); } // Confirm error when no refresh delay is set (not checked by schema). @@ -277,9 +275,9 @@ TEST_F(SubscriptionFactoryTest, HttpSubscriptionNoRefreshDelay) { EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_CALL(cluster, info()).Times(2); EXPECT_CALL(*cluster.info_, addedViaApi()); - EXPECT_THROW_WITH_MESSAGE( - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_), EnvoyException, - "refresh_delay is required for REST API configuration sources"); + EXPECT_THROW_WITH_MESSAGE(subscriptionFromConfigSource(config)->start({"static_cluster"}), + EnvoyException, + "refresh_delay is required for REST API configuration sources"); } TEST_F(SubscriptionFactoryTest, GrpcSubscription) { @@ -306,7 +304,7 @@ TEST_F(SubscriptionFactoryTest, GrpcSubscription) { EXPECT_CALL(random_, random()); EXPECT_CALL(dispatcher_, createTimer_(_)); EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_); + subscriptionFromConfigSource(config)->start({"static_cluster"}); } INSTANTIATE_TEST_SUITE_P(SubscriptionFactoryTestApiConfigSource, @@ -327,7 +325,7 @@ TEST_P(SubscriptionFactoryTestApiConfigSource, NonExistentCluster) { Upstream::ClusterManager::ClusterInfoMap cluster_map; EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map)); EXPECT_THROW_WITH_MESSAGE( - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_), EnvoyException, + subscriptionFromConfigSource(config)->start({"static_cluster"}), EnvoyException, "envoy::api::v2::core::ConfigSource must have a statically defined " "non-EDS cluster: 'static_cluster' does not exist, was added via api, or is an EDS cluster"); } @@ -349,7 +347,7 @@ TEST_P(SubscriptionFactoryTestApiConfigSource, DynamicCluster) { EXPECT_CALL(cluster, info()); EXPECT_CALL(*cluster.info_, addedViaApi()).WillOnce(Return(true)); EXPECT_THROW_WITH_MESSAGE( - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_), EnvoyException, + subscriptionFromConfigSource(config)->start({"static_cluster"}), EnvoyException, "envoy::api::v2::core::ConfigSource must have a statically defined " "non-EDS cluster: 'static_cluster' does not exist, was added via api, or is an EDS cluster"); } @@ -372,7 +370,7 @@ TEST_P(SubscriptionFactoryTestApiConfigSource, EDSClusterBackingEDSCluster) { EXPECT_CALL(*cluster.info_, addedViaApi()); EXPECT_CALL(*cluster.info_, type()).WillOnce(Return(envoy::api::v2::Cluster::EDS)); EXPECT_THROW_WITH_MESSAGE( - subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_), EnvoyException, + subscriptionFromConfigSource(config)->start({"static_cluster"}), EnvoyException, "envoy::api::v2::core::ConfigSource must have a statically defined " "non-EDS cluster: 'static_cluster' does not exist, was added via api, or is an EDS cluster"); } diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index ca456e9710d7..8c094c6f5b3b 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -41,14 +41,14 @@ namespace { class VhdsTest : public testing::Test { public: void SetUp() override { - factory_function_ = {[](const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, - Event::Dispatcher&, Upstream::ClusterManager&, - Envoy::Runtime::RandomGenerator&, Stats::Scope&, const std::string&, - const std::string&, absl::string_view, - ProtobufMessage::ValidationVisitor&, - Api::Api&) -> std::unique_ptr { - return std::unique_ptr(); - }}; + factory_function_ = { + [](const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, + Event::Dispatcher&, Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, + Stats::Scope&, const std::string&, const std::string&, absl::string_view, + ProtobufMessage::ValidationVisitor&, Api::Api&, + Envoy::Config::SubscriptionCallbacks&) -> std::unique_ptr { + return std::unique_ptr(); + }}; default_vhds_config_ = R"EOF( name: my_route diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index af565971ffaa..596b73837430 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -44,9 +44,8 @@ template class MockSubscriptionCallbacks : public Subscript class MockSubscription : public Subscription { public: - MOCK_METHOD2_T(start, - void(const std::set& resources, SubscriptionCallbacks& callbacks)); - MOCK_METHOD1_T(updateResources, void(const std::set& update_to_these_names)); + MOCK_METHOD1(start, void(const std::set& resources)); + MOCK_METHOD1(updateResources, void(const std::set& update_to_these_names)); }; class MockGrpcMuxWatch : public GrpcMuxWatch { From bf1d29d501251752bc1c989ca579f5fc858167cf Mon Sep 17 00:00:00 2001 From: Fred Douglas <43351173+fredlas@users.noreply.github.com> Date: Mon, 3 Jun 2019 16:01:01 -0700 Subject: [PATCH 03/13] config: fix EDS for empty update (#7143) Risk Level: low Testing: added to unit test Signed-off-by: Fred Douglas --- source/common/upstream/eds.cc | 4 +++- test/common/upstream/eds_test.cc | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 37f133551be7..f86c261802f5 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -133,7 +133,9 @@ void EdsClusterImpl::onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const Protobuf::RepeatedPtrField&, const std::string&) { - validateUpdateSize(resources.size()); + if (!validateUpdateSize(resources.size())) { + return; + } Protobuf::RepeatedPtrField unwrapped_resource; *unwrapped_resource.Add() = resources[0].resource(); onConfigUpdate(unwrapped_resource, resources[0].version()); diff --git a/test/common/upstream/eds_test.cc b/test/common/upstream/eds_test.cc index 9eb6e447dbe8..4da7882a21f8 100644 --- a/test/common/upstream/eds_test.cc +++ b/test/common/upstream/eds_test.cc @@ -224,7 +224,10 @@ TEST_F(EdsTest, OnConfigUpdateEmpty) { bool initialized = false; cluster_->initialize([&initialized] { initialized = true; }); cluster_->onConfigUpdate({}, ""); - EXPECT_EQ(1UL, stats_.counter("cluster.name.update_empty").value()); + Protobuf::RepeatedPtrField resources; + Protobuf::RepeatedPtrField removed_resources; + cluster_->onConfigUpdate(resources, removed_resources, ""); + EXPECT_EQ(2UL, stats_.counter("cluster.name.update_empty").value()); EXPECT_TRUE(initialized); } From 699e5102af0c206d68f4cd2666926f24f39c33b1 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 4 Jun 2019 08:24:22 -0700 Subject: [PATCH 04/13] [docs] minor fixes to clang-format docs (#7150) Signed-off-by: Derek Argueta --- bazel/README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/bazel/README.md b/bazel/README.md index a949d5cd891c..faf3da2f0b92 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -529,19 +529,20 @@ to run clang-format scripts on your workstation directly: * It's possible there is a speed advantage * Docker itself can sometimes go awry and you then have to deal with that * Type-ahead doesn't always work when waiting running a command through docker + To run the tools directly, you must install the correct version of clang. This -may change over time but as of January 2019, -[clang+llvm-7.0.0](https://releases.llvm.org/download.html) works well. You must +may change over time but as of June 2019, +[clang+llvm-8.0.0](https://releases.llvm.org/download.html) works well. You must also have 'buildifier' installed from the bazel distribution. Edit the paths shown here to reflect the installation locations on your system: ```shell -export CLANG_FORMAT="$HOME/ext/clang+llvm-7.0.0-x86_64-linux-gnu-ubuntu-16.04/bin/clang-format" +export CLANG_FORMAT="$HOME/ext/clang+llvm-8.0.0-x86_64-linux-gnu-ubuntu-16.04/bin/clang-format" export BUILDIFIER_BIN="/usr/bin/buildifier" ``` -Once this is set up, you can run clang-tidy without docker: +Once this is set up, you can run clang-format without docker: ```shell ./tools/check_format.py check From 34dbd85ab20219ba72f0b1585381948483b82b0e Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Tue, 4 Jun 2019 17:24:50 +0200 Subject: [PATCH 05/13] upstream: Use valid enum value in subset lb docs (#7153) Signed-off-by: Kateryna Nezdolii --- docs/root/intro/arch_overview/load_balancing/subsets.rst | 2 +- source/docs/subset_load_balancer.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/root/intro/arch_overview/load_balancing/subsets.rst b/docs/root/intro/arch_overview/load_balancing/subsets.rst index ac2f6add6a3e..637d8d3bc23a 100644 --- a/docs/root/intro/arch_overview/load_balancing/subsets.rst +++ b/docs/root/intro/arch_overview/load_balancing/subsets.rst @@ -15,7 +15,7 @@ condition described above. If subsets are :ref:`configured ` and a route specifies no metadata or no subset matching the metadata exists, the subset load balancer initiates -its fallback policy. The default policy is ``NO_ENDPOINT``, in which case the request fails as if +its fallback policy. The default policy is ``NO_FALLBACK``, in which case the request fails as if the cluster had no hosts. Conversely, the ``ANY_ENDPOINT`` fallback policy load balances across all hosts in the cluster, without regard to host metadata. Finally, the ``DEFAULT_SUBSET`` causes fallback to load balance among hosts that match a specific set of metadata. diff --git a/source/docs/subset_load_balancer.md b/source/docs/subset_load_balancer.md index b7995e77ebb8..c34e032f6d35 100644 --- a/source/docs/subset_load_balancer.md +++ b/source/docs/subset_load_balancer.md @@ -15,7 +15,7 @@ balancer types except the Original DST load balancer may be used for subset load The SLB can be configured with one of three fallback policies. If no subset matching the `LoadBalancerContext` is found: -1. `NO_ENDPOINT` specifies that `chooseHost` returns `nullptr` and load balancing fails. +1. `NO_FALLBACK` specifies that `chooseHost` returns `nullptr` and load balancing fails. 2. `ANY_ENDPOINT` specifies that load balancing occurs over the entire set of upstream hosts. 3. `DEFAULT_SUBSET` specifies that load balancing occurs over a specific subset of upstream hosts. If the default subset is empty, `chooseHost` returns `nullptr` and load balancing fails. From 1fdaf8e74f7023cd40fa56919f44163241bd9b95 Mon Sep 17 00:00:00 2001 From: Andres Guedez <34292400+AndresGuedez@users.noreply.github.com> Date: Tue, 4 Jun 2019 11:56:09 -0400 Subject: [PATCH 06/13] router: scoped rds (2d): integrate SRDS API into HttpConnectionManager (#7068) Integrate the Scoped Route Discovery Service (SRDS) API into the HttpConnectionManager. NOTES: - This is the last PR in the chain originating from parent #5839. - The scoped routing logic which can be configured through this API has not yet been implemented; it will be done in follow up PRs. Risk Level: Low (API is not yet fully implemented and should not be enabled) Testing: Integration tests added. Signed-off-by: Andres Guedez --- source/common/http/BUILD | 2 + source/common/http/conn_manager_config.h | 15 +- source/common/http/conn_manager_impl.cc | 2 +- source/common/http/conn_manager_impl.h | 2 + source/common/protobuf/utility.h | 19 ++ source/common/router/scoped_rds.cc | 38 +++ source/common/router/scoped_rds.h | 13 ++ .../network/http_connection_manager/BUILD | 2 + .../network/http_connection_manager/config.cc | 41 +++- .../network/http_connection_manager/config.h | 13 +- source/server/http/BUILD | 1 + source/server/http/admin.cc | 1 + source/server/http/admin.h | 29 ++- test/common/http/BUILD | 13 ++ test/common/http/conn_manager_impl_common.h | 54 +++++ .../http/conn_manager_impl_fuzz_test.cc | 24 +- test/common/http/conn_manager_impl_test.cc | 27 +-- test/common/http/conn_manager_utility_test.cc | 3 +- test/common/router/BUILD | 3 + test/common/router/scoped_rds_test.cc | 105 +++++---- .../http_connection_manager/config_test.cc | 65 ++++-- test/integration/BUILD | 17 ++ test/integration/integration_admin_test.cc | 6 +- .../scoped_rds_integration_test.cc | 219 ++++++++++++++++++ test/mocks/router/mocks.h | 2 +- 25 files changed, 593 insertions(+), 123 deletions(-) create mode 100644 test/common/http/conn_manager_impl_common.h create mode 100644 test/integration/scoped_rds_integration_test.cc diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 503a1d4c4e0f..63ee261385d7 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -120,6 +120,7 @@ envoy_cc_library( hdrs = ["conn_manager_config.h"], deps = [ ":date_provider_lib", + "//include/envoy/config:config_provider_interface", "//include/envoy/http:filter_interface", "//include/envoy/router:rds_interface", "//source/common/network:utility_lib", @@ -159,6 +160,7 @@ envoy_cc_library( "//include/envoy/network:drain_decision_interface", "//include/envoy/network:filter_interface", "//include/envoy/router:rds_interface", + "//include/envoy/router:scopes_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/server:overload_manager_interface", "//include/envoy/ssl:connection_interface", diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 38fd4b05b725..66530c2754f3 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/config/config_provider.h" #include "envoy/http/filter.h" #include "envoy/router/rds.h" #include "envoy/stats/scope.h" @@ -241,10 +242,18 @@ class ConnectionManagerConfig { virtual std::chrono::milliseconds delayedCloseTimeout() const PURE; /** - * @return Router::RouteConfigProvider& the configuration provider used to acquire a route - * config for each request flow. + * @return Router::RouteConfigProvider* the configuration provider used to acquire a route + * config for each request flow. Pointer ownership is _not_ transferred to the caller of + * this function. This will return nullptr when scoped routing is enabled. */ - virtual Router::RouteConfigProvider& routeConfigProvider() PURE; + virtual Router::RouteConfigProvider* routeConfigProvider() PURE; + + /** + * @return Config::ConfigProvider* the configuration provider used to acquire scoped routing + * configuration for each request flow. Pointer ownership is _not_ transferred to the caller of + * this function. This will return nullptr when scoped routing is not enabled. + */ + virtual Config::ConfigProvider* scopedRouteConfigProvider() PURE; /** * @return const std::string& the server name to write into responses. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index e2053474fd7d..6c7d5459c23a 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -399,7 +399,7 @@ void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_re ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connection_manager) : connection_manager_(connection_manager), - snapped_route_config_(connection_manager.config_.routeConfigProvider().config()), + snapped_route_config_(connection_manager.config_.routeConfigProvider()->config()), stream_id_(connection_manager.random_generator_.random()), request_response_timespan_(new Stats::Timespan( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index fbf53d7e7ac8..02a718e81d8c 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -18,6 +18,7 @@ #include "envoy/network/drain_decision.h" #include "envoy/network/filter.h" #include "envoy/router/rds.h" +#include "envoy/router/scopes.h" #include "envoy/runtime/runtime.h" #include "envoy/server/overload_manager.h" #include "envoy/ssl/connection.h" @@ -491,6 +492,7 @@ class ConnectionManagerImpl : Logger::Loggable, ConnectionManagerImpl& connection_manager_; Router::ConfigConstSharedPtr snapped_route_config_; + Router::ScopedConfigConstSharedPtr snapped_scoped_route_config_; Tracing::SpanPtr active_span_; const uint64_t stream_id_; StreamEncoder* response_encoder_{}; diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index cb659325dfd3..3df86caefa2d 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -140,6 +140,25 @@ class RepeatedPtrUtil { } return HashUtil::xxHash64(text); } + + /** + * Converts a proto repeated field into a generic vector of const Protobuf::Message unique_ptr's. + * + * @param repeated_field the proto repeated field to convert. + * @return ProtobufType::ConstMessagePtrVector the vector of const Message pointers. + */ + template + static ProtobufTypes::ConstMessagePtrVector + convertToConstMessagePtrVector(const Protobuf::RepeatedPtrField& repeated_field) { + ProtobufTypes::ConstMessagePtrVector ret_vector; + std::transform(repeated_field.begin(), repeated_field.end(), std::back_inserter(ret_vector), + [](const ProtoType& proto_message) -> std::unique_ptr { + Protobuf::Message* clone = proto_message.New(); + clone->MergeFrom(proto_message); + return std::unique_ptr(clone); + }); + return ret_vector; + } }; class ProtoValidationException : public EnvoyException { diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index 9a1d5889c368..4462995081f7 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -19,6 +19,44 @@ using Envoy::Config::ConfigProviderPtr; namespace Envoy { namespace Router { +namespace ScopedRoutesConfigProviderUtil { + +ConfigProviderPtr +create(const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& + config, + Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, + ConfigProviderManager& scoped_routes_config_provider_manager) { + ASSERT(config.route_specifier_case() == envoy::config::filter::network::http_connection_manager:: + v2::HttpConnectionManager::kScopedRoutes); + + switch (config.scoped_routes().config_specifier_case()) { + case envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes:: + kScopedRouteConfigurationsList: { + const envoy::config::filter::network::http_connection_manager::v2:: + ScopedRouteConfigurationsList& scoped_route_list = + config.scoped_routes().scoped_route_configurations_list(); + return scoped_routes_config_provider_manager.createStaticConfigProvider( + RepeatedPtrUtil::convertToConstMessagePtrVector( + scoped_route_list.scoped_route_configurations()), + factory_context, + ScopedRoutesConfigProviderManagerOptArg(config.scoped_routes().name(), + config.scoped_routes().rds_config_source(), + config.scoped_routes().scope_key_builder())); + } + case envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes::kScopedRds: + return scoped_routes_config_provider_manager.createXdsConfigProvider( + config.scoped_routes().scoped_rds(), factory_context, stat_prefix, + ScopedRoutesConfigProviderManagerOptArg(config.scoped_routes().name(), + config.scoped_routes().rds_config_source(), + config.scoped_routes().scope_key_builder())); + default: + // Proto validation enforces that is not reached. + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +} // namespace ScopedRoutesConfigProviderUtil + InlineScopedRoutesConfigProvider::InlineScopedRoutesConfigProvider( ProtobufTypes::ConstMessagePtrVector&& config_protos, std::string name, Server::Configuration::FactoryContext& factory_context, diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h index 020cdd54fa3f..19dab8f4482b 100644 --- a/source/common/router/scoped_rds.h +++ b/source/common/router/scoped_rds.h @@ -13,6 +13,19 @@ namespace Envoy { namespace Router { +// Scoped routing configuration utilities. +namespace ScopedRoutesConfigProviderUtil { + +// If enabled in the HttpConnectionManager config, returns a ConfigProvider for scoped routing +// configuration. +Envoy::Config::ConfigProviderPtr +create(const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& + config, + Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, + Envoy::Config::ConfigProviderManager& scoped_routes_config_provider_manager); + +} // namespace ScopedRoutesConfigProviderUtil + class ScopedRoutesConfigProviderManager; // A ConfigProvider for inline scoped routing configuration. diff --git a/source/extensions/filters/network/http_connection_manager/BUILD b/source/extensions/filters/network/http_connection_manager/BUILD index 29f6420e6084..1f5474a651e2 100644 --- a/source/extensions/filters/network/http_connection_manager/BUILD +++ b/source/extensions/filters/network/http_connection_manager/BUILD @@ -17,6 +17,7 @@ envoy_cc_library( srcs = ["config.cc"], hdrs = ["config.h"], deps = [ + "//include/envoy/config:config_provider_manager_interface", "//include/envoy/filesystem:filesystem_interface", "//include/envoy/http:filter_interface", "//include/envoy/registry", @@ -36,6 +37,7 @@ envoy_cc_library( "//source/common/json:json_loader_lib", "//source/common/protobuf:utility_lib", "//source/common/router:rds_lib", + "//source/common/router:scoped_rds_lib", "//source/extensions/filters/network:well_known_names", "//source/extensions/filters/network/common:factory_base_lib", ], diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index fbf589a962fd..6a8a18b5325a 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -23,6 +23,7 @@ #include "common/json/config_schemas.h" #include "common/protobuf/utility.h" #include "common/router/rds_impl.h" +#include "common/router/scoped_rds.h" namespace Envoy { namespace Extensions { @@ -69,6 +70,7 @@ std::unique_ptr createInternalAddressConfig( // Singleton registration via macro defined in envoy/singleton/manager.h SINGLETON_MANAGER_REGISTRATION(date_provider); SINGLETON_MANAGER_REGISTRATION(route_config_provider_manager); +SINGLETON_MANAGER_REGISTRATION(scoped_routes_config_provider_manager); Network::FilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( @@ -88,14 +90,21 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProtoTyped( return std::make_shared(context.admin()); }); + std::shared_ptr scoped_routes_config_provider_manager = + context.singletonManager().getTyped( + SINGLETON_MANAGER_REGISTERED_NAME(scoped_routes_config_provider_manager), [&context] { + return std::make_shared(context.admin()); + }); + std::shared_ptr filter_config(new HttpConnectionManagerConfig( - proto_config, context, *date_provider, *route_config_provider_manager)); + 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. Moreover, keep in mind the capture list determines // destruction order. - return [route_config_provider_manager, filter_config, &context, - date_provider](Network::FilterManager& filter_manager) -> void { + return [route_config_provider_manager, scoped_routes_config_provider_manager, filter_config, + &context, date_provider](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(), @@ -125,7 +134,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& config, Server::Configuration::FactoryContext& context, Http::DateProvider& date_provider, - Router::RouteConfigProviderManager& route_config_provider_manager) + Router::RouteConfigProviderManager& route_config_provider_manager, + Config::ConfigProviderManager& scoped_routes_config_provider_manager) : context_(context), stats_prefix_(fmt::format("http.{}.", config.stat_prefix())), stats_(Http::ConnectionManagerImpl::generateStats(stats_prefix_, context_.scope())), tracing_stats_( @@ -135,6 +145,7 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( xff_num_trusted_hops_(config.xff_num_trusted_hops()), skip_xff_append_(config.skip_xff_append()), via_(config.via()), route_config_provider_manager_(route_config_provider_manager), + scoped_routes_config_provider_manager_(scoped_routes_config_provider_manager), http2_settings_(Http::Utility::parseHttp2Settings(config.http2_protocol_options())), http1_settings_(Http::Utility::parseHttp1Settings(config.http_protocol_options())), max_request_headers_kb_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( @@ -162,9 +173,25 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( 0 #endif ))) { - - route_config_provider_ = Router::RouteConfigProviderUtil::create(config, context_, stats_prefix_, - route_config_provider_manager_); + // If scoped RDS is enabled, avoid creating a route config provider. Route config providers will + // be managed by the scoped routing logic instead. + switch (config.route_specifier_case()) { + case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::kRds: + case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager:: + kRouteConfig: + route_config_provider_ = Router::RouteConfigProviderUtil::create( + config, context_, stats_prefix_, route_config_provider_manager_); + break; + case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager:: + kScopedRoutes: + ENVOY_LOG(warn, "Scoped routing has been enabled but it is not yet fully implemented! HTTP " + "request routing DOES NOT work (yet) with this configuration."); + scoped_routes_config_provider_ = Router::ScopedRoutesConfigProviderUtil::create( + config, context_, stats_prefix_, scoped_routes_config_provider_manager_); + break; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } switch (config.forward_client_cert_details()) { case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::SANITIZE: diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index c1e7332b653c..0fca2f39d9b7 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -7,6 +7,7 @@ #include #include +#include "envoy/config/config_provider_manager.h" #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.validate.h" #include "envoy/http/filter.h" #include "envoy/router/route_config_provider_manager.h" @@ -78,7 +79,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& config, Server::Configuration::FactoryContext& context, Http::DateProvider& date_provider, - Router::RouteConfigProviderManager& route_config_provider_manager); + Router::RouteConfigProviderManager& route_config_provider_manager, + Config::ConfigProviderManager& scoped_routes_config_provider_manager); // Http::FilterChainFactory void createFilterChain(Http::FilterChainFactoryCallbacks& callbacks) override; @@ -104,7 +106,12 @@ class HttpConnectionManagerConfig : Logger::Loggable, absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } - Router::RouteConfigProvider& routeConfigProvider() override { return *route_config_provider_; } + Router::RouteConfigProvider* routeConfigProvider() override { + return route_config_provider_.get(); + } + Config::ConfigProvider* scopedRouteConfigProvider() override { + return scoped_routes_config_provider_.get(); + } const std::string& serverName() override { return server_name_; } Http::ConnectionManagerStats& stats() override { return stats_; } Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } @@ -151,6 +158,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ForwardClientCertType forward_client_cert_; std::vector set_current_client_cert_details_; Router::RouteConfigProviderManager& route_config_provider_manager_; + Config::ConfigProviderManager& scoped_routes_config_provider_manager_; CodecType codec_type_; const Http::Http2Settings http2_settings_; const Http::Http1Settings http1_settings_; @@ -162,6 +170,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, std::chrono::milliseconds stream_idle_timeout_; std::chrono::milliseconds request_timeout_; Router::RouteConfigProviderPtr route_config_provider_; + Config::ConfigProviderPtr scoped_routes_config_provider_; std::chrono::milliseconds drain_timeout_; bool generate_request_id_; Http::DateProvider& date_provider_; diff --git a/source/server/http/BUILD b/source/server/http/BUILD index b6761f286585..65b8bc56688c 100644 --- a/source/server/http/BUILD +++ b/source/server/http/BUILD @@ -53,6 +53,7 @@ envoy_cc_library( "//source/common/network:utility_lib", "//source/common/profiler:profiler_lib", "//source/common/router:config_lib", + "//source/common/router:scoped_config_lib", "//source/common/stats:histogram_lib", "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index dbffdbf81bcd..cb3a22238d01 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -1163,6 +1163,7 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) tracing_stats_( Http::ConnectionManagerImpl::generateTracingStats("http.admin.", no_op_store_)), route_config_provider_(server.timeSource()), + scoped_route_config_provider_(server.timeSource()), // TODO(jsedgwick) add /runtime_reset endpoint that removes all admin-set values handlers_{ {"/", "Admin home page", MAKE_ADMIN_HANDLER(handlerAdminHome), false, false}, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index aa95d912ed71..a0c2c8fccb2d 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -27,6 +27,7 @@ #include "common/http/default_server_string.h" #include "common/http/utility.h" #include "common/network/raw_buffer_socket.h" +#include "common/router/scoped_config_impl.h" #include "common/stats/isolated_store_impl.h" #include "server/http/config_tracker_impl.h" @@ -107,7 +108,10 @@ class AdminImpl : public Admin, std::chrono::milliseconds streamIdleTimeout() const override { return {}; } std::chrono::milliseconds requestTimeout() const override { return {}; } std::chrono::milliseconds delayedCloseTimeout() const override { return {}; } - Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } + Router::RouteConfigProvider* routeConfigProvider() override { return &route_config_provider_; } + Config::ConfigProvider* scopedRouteConfigProvider() override { + return &scoped_route_config_provider_; + } const std::string& serverName() override { return Http::DefaultServerString::get(); } Http::ConnectionManagerStats& stats() override { return stats_; } Http::ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } @@ -164,6 +168,28 @@ class AdminImpl : public Admin, TimeSource& time_source_; }; + /** + * Implementation of ScopedRouteConfigProvider that returns a null scoped route config. + */ + struct NullScopedRouteConfigProvider : public Config::ConfigProvider { + NullScopedRouteConfigProvider(TimeSource& time_source) + : config_(std::make_shared()), + time_source_(time_source) {} + + ~NullScopedRouteConfigProvider() override = default; + + // Config::ConfigProvider + SystemTime lastUpdated() const override { return time_source_.systemTime(); } + const Protobuf::Message* getConfigProto() const override { return nullptr; } + std::string getConfigVersion() const override { return ""; } + ConfigConstSharedPtr getConfig() const override { return config_; } + ApiType apiType() const override { return ApiType::Full; } + ConfigProtoVector getConfigProtos() const override { return {}; } + + Router::ScopedConfigConstSharedPtr config_; + TimeSource& time_source_; + }; + friend class AdminStatsTest; /** @@ -314,6 +340,7 @@ class AdminImpl : public Admin, Stats::IsolatedStoreImpl no_op_store_; Http::ConnectionManagerTracingStats tracing_stats_; NullRouteConfigProvider route_config_provider_; + NullScopedRouteConfigProvider scoped_route_config_provider_; std::list handlers_; const uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; absl::optional idle_timeout_; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index be6fe9f4c2fb..72043800423b 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -129,6 +129,17 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "conn_manager_impl_common_lib", + hdrs = ["conn_manager_impl_common.h"], + deps = [ + "//include/envoy/common:time_interface", + "//include/envoy/config:config_provider_interface", + "//include/envoy/router:rds_interface", + "//test/mocks/router:router_mocks", + ], +) + envoy_proto_library( name = "conn_manager_impl_fuzz_proto", srcs = ["conn_manager_impl_fuzz.proto"], @@ -142,6 +153,7 @@ envoy_cc_fuzz_test( srcs = ["conn_manager_impl_fuzz_test.cc"], corpus = "conn_manager_impl_corpus", deps = [ + ":conn_manager_impl_common_lib", ":conn_manager_impl_fuzz_proto_cc", "//source/common/common:empty_string", "//source/common/http:conn_manager_lib", @@ -167,6 +179,7 @@ envoy_cc_test( name = "conn_manager_impl_test", srcs = ["conn_manager_impl_test.cc"], deps = [ + ":conn_manager_impl_common_lib", "//include/envoy/access_log:access_log_interface", "//include/envoy/buffer:buffer_interface", "//include/envoy/event:dispatcher_interface", diff --git a/test/common/http/conn_manager_impl_common.h b/test/common/http/conn_manager_impl_common.h new file mode 100644 index 000000000000..1f68cc59412d --- /dev/null +++ b/test/common/http/conn_manager_impl_common.h @@ -0,0 +1,54 @@ +#pragma once + +#include + +#include "envoy/common/time.h" +#include "envoy/config/config_provider.h" +#include "envoy/router/rds.h" + +#include "test/mocks/router/mocks.h" + +#include "gmock/gmock.h" + +using testing::NiceMock; + +namespace Envoy { +namespace Http { +namespace ConnectionManagerImplHelper { + +// Test RouteConfigProvider that returns a mocked config. +struct RouteConfigProvider : public Router::RouteConfigProvider { + RouteConfigProvider(TimeSource& time_source) : time_source_(time_source) {} + + // Router::RouteConfigProvider + Router::ConfigConstSharedPtr config() override { return route_config_; } + absl::optional configInfo() const override { return {}; } + SystemTime lastUpdated() const override { return time_source_.systemTime(); } + void onConfigUpdate() override {} + + TimeSource& time_source_; + std::shared_ptr route_config_{new NiceMock()}; +}; + +// Test ScopedRouteConfigProvider that returns a mocked config. +struct ScopedRouteConfigProvider : public Config::ConfigProvider { + ScopedRouteConfigProvider(TimeSource& time_source) + : config_(std::make_shared()), time_source_(time_source) {} + + ~ScopedRouteConfigProvider() override = default; + + // Config::ConfigProvider + SystemTime lastUpdated() const override { return time_source_.systemTime(); } + const Protobuf::Message* getConfigProto() const override { return nullptr; } + Envoy::Config::ConfigProvider::ConfigProtoVector getConfigProtos() const override { return {}; } + std::string getConfigVersion() const override { return ""; } + ConfigConstSharedPtr getConfig() const override { return config_; } + ApiType apiType() const override { return ApiType::Delta; } + + std::shared_ptr config_; + TimeSource& time_source_; +}; + +} // namespace ConnectionManagerImplHelper +} // namespace Http +} // namespace Envoy diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 056b1096b467..d8355bfd3dc1 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -20,6 +20,7 @@ #include "common/network/address_impl.h" #include "common/network/utility.h" +#include "test/common/http/conn_manager_impl_common.h" #include "test/common/http/conn_manager_impl_fuzz.pb.h" #include "test/fuzz/fuzz_runner.h" #include "test/fuzz/utility.h" @@ -44,21 +45,8 @@ namespace Http { class FuzzConfig : public ConnectionManagerConfig { public: - struct RouteConfigProvider : public Router::RouteConfigProvider { - RouteConfigProvider(TimeSource& time_source) : time_source_(time_source) {} - - // Router::RouteConfigProvider - Router::ConfigConstSharedPtr config() override { return route_config_; } - absl::optional configInfo() const override { return {}; } - SystemTime lastUpdated() const override { return time_source_.systemTime(); } - void onConfigUpdate() override {} - - TimeSource& time_source_; - std::shared_ptr route_config_{new NiceMock()}; - }; - FuzzConfig() - : route_config_provider_(time_system_), + : route_config_provider_(time_system_), scoped_route_config_provider_(time_system_), stats_{{ALL_HTTP_CONN_MAN_STATS(POOL_COUNTER(fake_stats_), POOL_GAUGE(fake_stats_), POOL_HISTOGRAM(fake_stats_))}, "", @@ -96,7 +84,10 @@ class FuzzConfig : public ConnectionManagerConfig { std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } - Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } + Router::RouteConfigProvider* routeConfigProvider() override { return &route_config_provider_; } + Config::ConfigProvider* scopedRouteConfigProvider() override { + return &scoped_route_config_provider_; + } const std::string& serverName() override { return server_name_; } ConnectionManagerStats& stats() override { return stats_; } ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } @@ -127,7 +118,8 @@ class FuzzConfig : public ConnectionManagerConfig { NiceMock filter_factory_; Event::SimulatedTimeSystem time_system_; SlowDateProviderImpl date_provider_{time_system_}; - RouteConfigProvider route_config_provider_; + ConnectionManagerImplHelper::RouteConfigProvider route_config_provider_; + ConnectionManagerImplHelper::ScopedRouteConfigProvider scoped_route_config_provider_; std::string server_name_; Stats::IsolatedStoreImpl fake_stats_; ConnectionManagerStats stats_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 2a9b41aa002e..81a555eb2c90 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -26,6 +26,7 @@ #include "extensions/access_loggers/file/file_access_log_impl.h" +#include "test/common/http/conn_manager_impl_common.h" #include "test/mocks/access_log/mocks.h" #include "test/mocks/buffer/mocks.h" #include "test/mocks/common.h" @@ -64,22 +65,10 @@ namespace Http { class HttpConnectionManagerImplTest : public testing::Test, public ConnectionManagerConfig { public: - struct RouteConfigProvider : public Router::RouteConfigProvider { - RouteConfigProvider(TimeSource& time_source) : time_source_(time_source) {} - - // Router::RouteConfigProvider - Router::ConfigConstSharedPtr config() override { return route_config_; } - absl::optional configInfo() const override { return {}; } - SystemTime lastUpdated() const override { return time_source_.systemTime(); } - void onConfigUpdate() override {} - - TimeSource& time_source_; - std::shared_ptr route_config_{new NiceMock()}; - }; - HttpConnectionManagerImplTest() - : route_config_provider_(test_time_.timeSystem()), http_context_(fake_stats_.symbolTable()), - access_log_path_("dummy_path"), + : route_config_provider_(test_time_.timeSystem()), + scoped_route_config_provider_(test_time_.timeSystem()), + http_context_(fake_stats_.symbolTable()), access_log_path_("dummy_path"), access_logs_{ AccessLog::InstanceSharedPtr{new Extensions::AccessLoggers::File::FileAccessLog( access_log_path_, {}, AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter(), @@ -263,7 +252,10 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } std::chrono::milliseconds requestTimeout() const override { return request_timeout_; } std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } - Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } + Router::RouteConfigProvider* routeConfigProvider() override { return &route_config_provider_; } + Config::ConfigProvider* scopedRouteConfigProvider() override { + return &scoped_route_config_provider_; + } const std::string& serverName() override { return server_name_; } ConnectionManagerStats& stats() override { return stats_; } ConnectionManagerTracingStats& tracingStats() override { return tracing_stats_; } @@ -287,7 +279,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan bool shouldNormalizePath() const override { return normalize_path_; } DangerousDeprecatedTestTime test_time_; - RouteConfigProvider route_config_provider_; + ConnectionManagerImplHelper::RouteConfigProvider route_config_provider_; + ConnectionManagerImplHelper::ScopedRouteConfigProvider scoped_route_config_provider_; NiceMock tracer_; Stats::IsolatedStoreImpl fake_stats_; Http::ContextImpl http_context_; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 89b519f952dc..f78bce8b01ee 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -56,7 +56,8 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds()); MOCK_CONST_METHOD0(requestTimeout, std::chrono::milliseconds()); MOCK_CONST_METHOD0(delayedCloseTimeout, std::chrono::milliseconds()); - MOCK_METHOD0(routeConfigProvider, Router::RouteConfigProvider&()); + MOCK_METHOD0(routeConfigProvider, Router::RouteConfigProvider*()); + MOCK_METHOD0(scopedRouteConfigProvider, Config::ConfigProvider*()); MOCK_METHOD0(serverName, const std::string&()); MOCK_METHOD0(stats, ConnectionManagerStats&()); MOCK_METHOD0(tracingStats, ConnectionManagerTracingStats&()); diff --git a/test/common/router/BUILD b/test/common/router/BUILD index b5b4ce77cf67..9e5a061f7370 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -81,6 +81,9 @@ envoy_cc_test( envoy_cc_test( name = "scoped_rds_test", srcs = ["scoped_rds_test.cc"], + external_deps = [ + "abseil_strings", + ], deps = [ "//source/common/config:utility_lib", "//source/common/http:message_lib", diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 52b939f93a36..15b282bd77b2 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -10,6 +10,7 @@ #include "test/test_common/simulated_time_system.h" #include "absl/strings/string_view.h" +#include "absl/strings/substitute.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -32,15 +33,12 @@ void parseScopedRouteConfigurationFromYaml(ProtobufWkt::Any& scoped_route_config scoped_route_config.PackFrom(parseScopedRouteConfigurationFromYaml(yaml)); } -std::vector> -protosToMessageVec(std::vector&& protos) { - std::vector> messages; - for (const auto& proto : protos) { - Protobuf::Message* message = proto.New(); - message->CopyFrom(proto); - messages.push_back(std::unique_ptr(message)); - } - return messages; +envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager +parseHttpConnectionManagerFromYaml(const std::string& config_yaml) { + envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager + http_connection_manager; + TestUtility::loadFromYaml(config_yaml, http_connection_manager); + return http_connection_manager; } class ScopedRoutesTestBase : public testing::Test { @@ -280,37 +278,44 @@ TEST_F(ScopedRoutesConfigProviderManagerTest, ConfigDump) { expected_config_dump); EXPECT_EQ(expected_config_dump.DebugString(), scoped_routes_config_dump.DebugString()); - const std::string config_yaml = R"EOF( -name: foo -route_configuration_name: foo-route-config -key: - fragments: { string_key: "172.10.10.10" } -)EOF"; - const std::string config_yaml2 = R"EOF( -name: foo2 -route_configuration_name: foo-route-config2 -key: - fragments: { string_key: "172.10.10.20" } -)EOF"; - std::vector> config_protos = - protosToMessageVec({parseScopedRouteConfigurationFromYaml(config_yaml), - parseScopedRouteConfigurationFromYaml(config_yaml2)}); - timeSystem().setSystemTime(std::chrono::milliseconds(1234567891234)); - envoy::config::filter::network::http_connection_manager::v2 ::ScopedRoutes::ScopeKeyBuilder - scope_key_builder; - TestUtility::loadFromYaml(R"EOF( -fragments: - - header_value_extractor: { name: X-Google-VIP } -)EOF", - scope_key_builder); + const std::string hcm_base_config_yaml = R"EOF( +codec_type: auto +stat_prefix: foo +http_filters: + - name: http_dynamo_filter + config: +scoped_routes: + name: $0 + scope_key_builder: + fragments: + - header_value_extractor: { name: X-Google-VIP } + rds_config_source: + api_config_source: + api_type: REST + cluster_names: + - foo_rds_cluster + refresh_delay: { seconds: 1, nanos: 0 } +$1 +)EOF"; + const std::string inline_scoped_route_configs_yaml = R"EOF( + scoped_route_configurations_list: + scoped_route_configurations: + - name: foo + route_configuration_name: foo-route-config + key: + fragments: { string_key: "172.10.10.10" } + - name: foo2 + route_configuration_name: foo-route-config2 + key: + fragments: { string_key: "172.10.10.20" } +)EOF"; // Only load the inline scopes. - Envoy::Config::ConfigProviderPtr inline_config = - config_provider_manager_->createStaticConfigProvider( - std::move(config_protos), factory_context_, - ScopedRoutesConfigProviderManagerOptArg("foo-scoped-routes", rds_config_source_, - scope_key_builder)); + Envoy::Config::ConfigProviderPtr inline_config = ScopedRoutesConfigProviderUtil::create( + parseHttpConnectionManagerFromYaml(absl::Substitute(hcm_base_config_yaml, "foo-scoped-routes", + inline_scoped_route_configs_yaml)), + factory_context_, "foo.", *config_provider_manager_); message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["route_scopes"](); const auto& scoped_routes_config_dump2 = MessageUtil::downcastAndValidate( @@ -336,21 +341,19 @@ route_configuration_name: foo-route-config2 EXPECT_EQ(expected_config_dump.DebugString(), scoped_routes_config_dump2.DebugString()); setupMockClusterMap(); - envoy::config::filter::network::http_connection_manager::v2::ScopedRds scoped_rds_config; - const std::string config_source_yaml = R"EOF( -scoped_rds_config_source: - api_config_source: - api_type: REST - cluster_names: - - foo_cluster - refresh_delay: { seconds: 1, nanos: 0 } + const std::string scoped_rds_config_yaml = R"EOF( + scoped_rds: + scoped_rds_config_source: + api_config_source: + api_type: REST + cluster_names: + - foo_cluster + refresh_delay: { seconds: 1, nanos: 0 } )EOF"; - TestUtility::loadFromYaml(config_source_yaml, scoped_rds_config); - Envoy::Config::ConfigProviderPtr dynamic_provider = - config_provider_manager_->createXdsConfigProvider( - scoped_rds_config, factory_context_, "foo.", - ScopedRoutesConfigProviderManagerOptArg("foo-dynamic-scoped-routes", rds_config_source_, - scope_key_builder)); + Envoy::Config::ConfigProviderPtr dynamic_provider = ScopedRoutesConfigProviderUtil::create( + parseHttpConnectionManagerFromYaml(absl::Substitute( + hcm_base_config_yaml, "foo-dynamic-scoped-routes", scoped_rds_config_yaml)), + factory_context_, "foo.", *config_provider_manager_); Protobuf::RepeatedPtrField resources; resources.Add()->PackFrom(parseScopedRouteConfigurationFromYaml(R"EOF( diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 22a8d8d37c4b..c232ad8fd276 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -5,6 +5,7 @@ #include "extensions/filters/network/http_connection_manager/config.h" +#include "test/mocks/config/mocks.h" #include "test/mocks/http/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/server/mocks.h" @@ -37,6 +38,7 @@ class HttpConnectionManagerConfigTest : public testing::Test { NiceMock context_; Http::SlowDateProviderImpl date_provider_{context_.dispatcher().timeSource()}; NiceMock route_config_provider_manager_; + NiceMock scoped_routes_config_provider_manager_; }; TEST_F(HttpConnectionManagerConfigTest, ValidateFail) { @@ -68,7 +70,8 @@ stat_prefix: router EXPECT_THROW_WITH_MESSAGE( HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_), + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_), EnvoyException, "Didn't find a registered implementation for name: 'foo'"); } @@ -97,7 +100,8 @@ stat_prefix: router )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_THAT(std::vector({Http::LowerCaseString("foo")}), ContainerEq(config.tracingConfig()->request_headers_for_tags_)); @@ -120,7 +124,8 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(100, config.tracingConfig()->client_sampling_.numerator()); EXPECT_EQ(envoy::type::FractionalPercent::HUNDRED, @@ -153,7 +158,8 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingConfigured) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(1, config.tracingConfig()->client_sampling_.numerator()); EXPECT_EQ(envoy::type::FractionalPercent::HUNDRED, @@ -178,7 +184,8 @@ TEST_F(HttpConnectionManagerConfigTest, UnixSocketInternalAddress) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); Network::Address::PipeInstance unixAddress{"/foo"}; Network::Address::Ipv4Instance internalIpAddress{"127.0.0.1", 0}; Network::Address::Ipv4Instance externalIpAddress{"12.0.0.1", 0}; @@ -197,7 +204,8 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbDefault) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(60, config.maxRequestHeadersKb()); } @@ -212,7 +220,8 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbConfigured) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(16, config.maxRequestHeadersKb()); } @@ -227,7 +236,8 @@ TEST_F(HttpConnectionManagerConfigTest, MaxRequestHeadersKbMaxConfigurable) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(96, config.maxRequestHeadersKb()); } @@ -243,7 +253,8 @@ TEST_F(HttpConnectionManagerConfigTest, DisabledStreamIdleTimeout) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(0, config.streamIdleTimeout().count()); } @@ -262,7 +273,8 @@ TEST_F(HttpConnectionManagerConfigTest, NormalizePathDefault) { .WillOnce(Invoke(&context_.runtime_loader_.snapshot_, &Runtime::MockSnapshot::featureEnabledDefault)); HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); #ifdef ENVOY_NORMALIZE_PATH_BY_DEFAULT EXPECT_TRUE(config.shouldNormalizePath()); #else @@ -284,7 +296,8 @@ TEST_F(HttpConnectionManagerConfigTest, NormalizePathRuntime) { featureEnabled("http_connection_manager.normalize_path", An())) .WillOnce(Return(true)); HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_TRUE(config.shouldNormalizePath()); } @@ -303,7 +316,8 @@ TEST_F(HttpConnectionManagerConfigTest, NormalizePathTrue) { featureEnabled("http_connection_manager.normalize_path", An())) .Times(0); HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_TRUE(config.shouldNormalizePath()); } @@ -322,7 +336,8 @@ TEST_F(HttpConnectionManagerConfigTest, NormalizePathFalse) { featureEnabled("http_connection_manager.normalize_path", An())) .Times(0); HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_FALSE(config.shouldNormalizePath()); } @@ -337,7 +352,8 @@ TEST_F(HttpConnectionManagerConfigTest, ConfiguredRequestTimeout) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(53 * 1000, config.requestTimeout().count()); } @@ -352,7 +368,8 @@ TEST_F(HttpConnectionManagerConfigTest, DisabledRequestTimeout) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(0, config.requestTimeout().count()); } @@ -366,7 +383,8 @@ TEST_F(HttpConnectionManagerConfigTest, UnconfiguredRequestTimeout) { )EOF"; HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); EXPECT_EQ(0, config.requestTimeout().count()); } @@ -543,7 +561,8 @@ stat_prefix: router TEST_F(FilterChainTest, createFilterChain) { HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(basic_config_), context_, - date_provider_, route_config_provider_manager_); + date_provider_, route_config_provider_manager_, + scoped_routes_config_provider_manager_); Http::MockFilterChainFactoryCallbacks callbacks; EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo @@ -557,7 +576,8 @@ TEST_F(FilterChainTest, createUpgradeFilterChain) { hcm_config.add_upgrade_configs()->set_upgrade_type("websocket"); HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, - route_config_provider_manager_); + route_config_provider_manager_, + scoped_routes_config_provider_manager_); NiceMock callbacks; // Check the case where WebSockets are configured in the HCM, and no router @@ -603,7 +623,8 @@ TEST_F(FilterChainTest, createUpgradeFilterChainHCMDisabled) { hcm_config.mutable_upgrade_configs(0)->mutable_enabled()->set_value(false); HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, - route_config_provider_manager_); + route_config_provider_manager_, + scoped_routes_config_provider_manager_); NiceMock callbacks; // Check the case where WebSockets are off in the HCM, and no router config is present. @@ -652,7 +673,8 @@ TEST_F(FilterChainTest, createCustomUpgradeFilterChain) { "envoy.http_dynamo_filter"); HttpConnectionManagerConfig config(hcm_config, context_, date_provider_, - route_config_provider_manager_); + route_config_provider_manager_, + scoped_routes_config_provider_manager_); { Http::MockFilterChainFactoryCallbacks callbacks; @@ -681,7 +703,8 @@ TEST_F(FilterChainTest, invalidConfig) { hcm_config.add_upgrade_configs()->set_upgrade_type("websocket"); EXPECT_THROW_WITH_MESSAGE(HttpConnectionManagerConfig(hcm_config, context_, date_provider_, - route_config_provider_manager_), + route_config_provider_manager_, + scoped_routes_config_provider_manager_), EnvoyException, "Error: multiple upgrade configs with the same name: 'websocket'"); } diff --git a/test/integration/BUILD b/test/integration/BUILD index c55e3956ea87..ce5cb1e55458 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -778,3 +778,20 @@ envoy_cc_fuzz_test( corpus = "h1_corpus", deps = [":h1_fuzz_lib"], ) + +envoy_cc_test( + name = "scoped_rds_integration_test", + srcs = [ + "scoped_rds_integration_test.cc", + ], + deps = [ + ":http_integration_lib", + "//source/common/config:resources_lib", + "//source/common/event:dispatcher_includes", + "//source/common/event:dispatcher_lib", + "//source/common/network:connection_lib", + "//source/common/network:utility_lib", + "//test/common/grpc:grpc_client_integration_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 1d45bc55cf47..09adf7ca6382 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -366,7 +366,9 @@ TEST_P(IntegrationAdminTest, Admin) { "type.googleapis.com/envoy.admin.v2alpha.BootstrapConfigDump", "type.googleapis.com/envoy.admin.v2alpha.ClustersConfigDump", "type.googleapis.com/envoy.admin.v2alpha.ListenersConfigDump", + "type.googleapis.com/envoy.admin.v2alpha.ScopedRoutesConfigDump", "type.googleapis.com/envoy.admin.v2alpha.RoutesConfigDump"}; + for (Json::ObjectSharedPtr obj_ptr : json->getObjectArray("configs")) { EXPECT_TRUE(expected_types[index].compare(obj_ptr->getString("@type")) == 0); index++; @@ -375,11 +377,11 @@ TEST_P(IntegrationAdminTest, Admin) { // Validate we can parse as proto. envoy::admin::v2alpha::ConfigDump config_dump; TestUtility::loadFromJson(response->body(), config_dump); - EXPECT_EQ(4, config_dump.configs_size()); + EXPECT_EQ(5, config_dump.configs_size()); // .. and that we can unpack one of the entries. envoy::admin::v2alpha::RoutesConfigDump route_config_dump; - config_dump.configs(3).UnpackTo(&route_config_dump); + config_dump.configs(4).UnpackTo(&route_config_dump); EXPECT_EQ("route_config_0", route_config_dump.static_route_configs(0).route_config().name()); } diff --git a/test/integration/scoped_rds_integration_test.cc b/test/integration/scoped_rds_integration_test.cc new file mode 100644 index 000000000000..7dc43494c232 --- /dev/null +++ b/test/integration/scoped_rds_integration_test.cc @@ -0,0 +1,219 @@ +#include "envoy/api/v2/srds.pb.h" + +#include "common/config/resources.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/integration/http_integration.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +class ScopedRdsIntegrationTest : public HttpIntegrationTest, + public Grpc::GrpcClientIntegrationParamTest { +protected: + struct FakeUpstreamInfo { + FakeHttpConnectionPtr connection_; + FakeUpstream* upstream_{}; + FakeStreamPtr stream_; + }; + + ScopedRdsIntegrationTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, ipVersion(), realTime()) {} + + ~ScopedRdsIntegrationTest() override { + resetConnections(); + cleanupUpstreamAndDownstream(); + } + + void initialize() override { + config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + // Add the static cluster to serve SRDS. + auto* scoped_rds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + scoped_rds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + scoped_rds_cluster->set_name("srds_cluster"); + scoped_rds_cluster->mutable_http2_protocol_options(); + + // Add the static cluster to serve RDS. + auto* rds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + rds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + rds_cluster->set_name("rds_cluster"); + rds_cluster->mutable_http2_protocol_options(); + }); + + config_helper_.addConfigModifier( + [this](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& + http_connection_manager) { + const std::string& scope_key_builder_config_yaml = R"EOF( +fragments: + - header_value_extractor: { name: X-Google-VIP } +)EOF"; + envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes::ScopeKeyBuilder + scope_key_builder; + TestUtility::loadFromYaml(scope_key_builder_config_yaml, scope_key_builder); + auto* scoped_routes = http_connection_manager.mutable_scoped_routes(); + scoped_routes->set_name("foo-scoped-routes"); + *scoped_routes->mutable_scope_key_builder() = scope_key_builder; + + envoy::api::v2::core::ApiConfigSource* rds_api_config_source = + scoped_routes->mutable_rds_config_source()->mutable_api_config_source(); + rds_api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + envoy::api::v2::core::GrpcService* grpc_service = + rds_api_config_source->add_grpc_services(); + setGrpcService(*grpc_service, "rds_cluster", getRdsFakeUpstream().localAddress()); + + envoy::api::v2::core::ApiConfigSource* srds_api_config_source = + scoped_routes->mutable_scoped_rds() + ->mutable_scoped_rds_config_source() + ->mutable_api_config_source(); + srds_api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + grpc_service = srds_api_config_source->add_grpc_services(); + setGrpcService(*grpc_service, "srds_cluster", getScopedRdsFakeUpstream().localAddress()); + }); + + HttpIntegrationTest::initialize(); + } + + void createUpstreams() override { + HttpIntegrationTest::createUpstreams(); + // Create the SRDS upstream. + fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, + timeSystem(), enable_half_close_)); + // Create the RDS upstream. + fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, + timeSystem(), enable_half_close_)); + } + + void resetFakeUpstreamInfo(FakeUpstreamInfo* upstream_info) { + ASSERT(upstream_info->upstream_ != nullptr); + + upstream_info->upstream_->set_allow_unexpected_disconnects(true); + AssertionResult result = upstream_info->connection_->close(); + RELEASE_ASSERT(result, result.message()); + result = upstream_info->connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + upstream_info->connection_.reset(); + } + + void resetConnections() { + if (rds_upstream_info_.upstream_ != nullptr) { + resetFakeUpstreamInfo(&rds_upstream_info_); + } + resetFakeUpstreamInfo(&scoped_rds_upstream_info_); + } + + FakeUpstream& getRdsFakeUpstream() const { return *fake_upstreams_[2]; } + + FakeUpstream& getScopedRdsFakeUpstream() const { return *fake_upstreams_[1]; } + + void createStream(FakeUpstreamInfo* upstream_info, FakeUpstream& upstream) { + upstream_info->upstream_ = &upstream; + AssertionResult result = + upstream_info->upstream_->waitForHttpConnection(*dispatcher_, upstream_info->connection_); + RELEASE_ASSERT(result, result.message()); + result = upstream_info->connection_->waitForNewStream(*dispatcher_, upstream_info->stream_); + RELEASE_ASSERT(result, result.message()); + upstream_info->stream_->startGrpcStream(); + } + + void createRdsStream() { createStream(&rds_upstream_info_, getRdsFakeUpstream()); } + + void createScopedRdsStream() { + createStream(&scoped_rds_upstream_info_, getScopedRdsFakeUpstream()); + } + + void sendScopedRdsResponse(const std::vector& resource_protos, + const std::string& version) { + ASSERT(scoped_rds_upstream_info_.stream_ != nullptr); + + envoy::api::v2::DiscoveryResponse response; + response.set_version_info(version); + response.set_type_url(Config::TypeUrl::get().ScopedRouteConfiguration); + + for (const auto& resource_proto : resource_protos) { + envoy::api::v2::ScopedRouteConfiguration scoped_route_proto; + TestUtility::loadFromYaml(resource_proto, scoped_route_proto); + response.add_resources()->PackFrom(scoped_route_proto); + } + + scoped_rds_upstream_info_.stream_->sendGrpcMessage(response); + } + + FakeUpstreamInfo scoped_rds_upstream_info_; + FakeUpstreamInfo rds_upstream_info_; +}; + +INSTANTIATE_TEST_CASE_P(IpVersionsAndGrpcTypes, ScopedRdsIntegrationTest, + GRPC_CLIENT_INTEGRATION_PARAMS); + +// Test that a SRDS DiscoveryResponse is successfully processed. +TEST_P(ScopedRdsIntegrationTest, BasicSuccess) { + const std::string scope_route1 = R"EOF( +name: foo_scope1 +route_configuration_name: foo_route1 +key: + fragments: + - string_key: x-foo-key +)EOF"; + const std::string scope_route2 = R"EOF( +name: foo_scope2 +route_configuration_name: foo_route2 +key: + fragments: + - string_key: x-foo-key +)EOF"; + + on_server_init_function_ = [this, &scope_route1, &scope_route2]() { + createScopedRdsStream(); + sendScopedRdsResponse({scope_route1, scope_route2}, "1"); + }; + initialize(); + + test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_attempt", 1); + test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_success", 1); + // The version gauge should be set to xxHash64("1"). + test_server_->waitForGaugeEq("http.config_test.scoped_rds.foo-scoped-routes.version", + 13237225503670494420UL); + + const std::string scope_route3 = R"EOF( +name: foo_scope3 +route_configuration_name: foo_route3 +key: + fragments: + - string_key: x-baz-key +)EOF"; + sendScopedRdsResponse({scope_route3}, "2"); + + test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_attempt", 2); + test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_success", 2); + test_server_->waitForGaugeEq("http.config_test.scoped_rds.foo-scoped-routes.version", + 6927017134761466251UL); + + // TODO(AndresGuedez): test actual scoped routing logic; only the config handling is implemented + // at this point. +} + +// Test that a bad config update updates the corresponding stats. +TEST_P(ScopedRdsIntegrationTest, ConfigUpdateFailure) { + // 'name' will fail to validate due to empty string. + const std::string scope_route1 = R"EOF( +name: +route_configuration_name: foo_route1 +key: + fragments: + - string_key: x-foo-key +)EOF"; + on_server_init_function_ = [this, &scope_route1]() { + createScopedRdsStream(); + sendScopedRdsResponse({scope_route1}, "1"); + }; + initialize(); + + test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_rejected", + 1); +} + +} // namespace +} // namespace Envoy diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 4b89f70ee1a3..f84aacda8ab8 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -399,7 +399,7 @@ class MockScopedConfig : public ScopedConfig { MockScopedConfig(); ~MockScopedConfig(); - MOCK_CONST_METHOD1(getRouterConfig, ConfigConstSharedPtr(const Http::HeaderMap& headers)); + MOCK_CONST_METHOD1(getRouteConfig, ConfigConstSharedPtr(const Http::HeaderMap& headers)); }; } // namespace Router From 9bdf99060fce442b025be7478357fdbb974ce993 Mon Sep 17 00:00:00 2001 From: asraa Date: Tue, 4 Jun 2019 13:07:30 -0400 Subject: [PATCH 07/13] fuzz: fix unitialized ptr to downstreamSslConnectionInfo in TestStreamInfo and add mock (#7155) Initializes downstreamSslConnectionInfo pointer in TestStreamInfo and adds a mock object for it in fuzzers. Fixes crash in: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15066 Risk Level: Low Testing: Add a corpus entry with downstream info. Signed-off-by: Asra Ali --- test/common/access_log/access_log_formatter_fuzz_test.cc | 8 +++++--- ...fuzz-testcase-header_parser_fuzz_test-5702537941876736 | 6 ++++++ test/common/router/header_parser_fuzz_test.cc | 4 +++- test/common/stream_info/test_util.h | 2 +- test/fuzz/BUILD | 1 + test/fuzz/utility.h | 8 +++++++- 6 files changed, 23 insertions(+), 6 deletions(-) create mode 100644 test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5702537941876736 diff --git a/test/common/access_log/access_log_formatter_fuzz_test.cc b/test/common/access_log/access_log_formatter_fuzz_test.cc index 1df65dd1736e..09d6b4322510 100644 --- a/test/common/access_log/access_log_formatter_fuzz_test.cc +++ b/test/common/access_log/access_log_formatter_fuzz_test.cc @@ -10,12 +10,14 @@ namespace { DEFINE_PROTO_FUZZER(const test::common::access_log::TestCase& input) { try { + NiceMock connection_info; std::vector formatters = AccessLog::AccessLogFormatParser::parse(input.format()); for (const auto& it : formatters) { - it->format( - Fuzz::fromHeaders(input.request_headers()), Fuzz::fromHeaders(input.response_headers()), - Fuzz::fromHeaders(input.response_trailers()), Fuzz::fromStreamInfo(input.stream_info())); + it->format(Fuzz::fromHeaders(input.request_headers()), + Fuzz::fromHeaders(input.response_headers()), + Fuzz::fromHeaders(input.response_trailers()), + Fuzz::fromStreamInfo(input.stream_info(), &connection_info)); } ENVOY_LOG_MISC(trace, "Success"); } catch (const EnvoyException& e) { diff --git a/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5702537941876736 b/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5702537941876736 new file mode 100644 index 000000000000..d9a0b9b95ed1 --- /dev/null +++ b/test/common/router/header_parser_corpus/clusterfuzz-testcase-header_parser_fuzz_test-5702537941876736 @@ -0,0 +1,6 @@ +headers_to_add { + header { + key: "m" + value: "%DOWNSTREAM_PEER_SUBJECT%" + } +} diff --git a/test/common/router/header_parser_fuzz_test.cc b/test/common/router/header_parser_fuzz_test.cc index 24750341c4a2..451aae702c4e 100644 --- a/test/common/router/header_parser_fuzz_test.cc +++ b/test/common/router/header_parser_fuzz_test.cc @@ -20,7 +20,9 @@ DEFINE_PROTO_FUZZER(const test::common::router::TestCase& input) { Router::HeaderParserPtr parser = Router::HeaderParser::configure(headers_to_add, headers_to_remove); Http::HeaderMapImpl header_map; - parser->evaluateHeaders(header_map, fromStreamInfo(input.stream_info())); + NiceMock connection_info; + TestStreamInfo test_stream_info = fromStreamInfo(input.stream_info(), &connection_info); + parser->evaluateHeaders(header_map, test_stream_info); ENVOY_LOG_MISC(trace, "Success"); } catch (const EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException: {}", e.what()); diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 931c22feea20..a532ba1a8dc8 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -207,7 +207,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo { Network::Address::InstanceConstSharedPtr downstream_local_address_; Network::Address::InstanceConstSharedPtr downstream_direct_remote_address_; Network::Address::InstanceConstSharedPtr downstream_remote_address_; - const Ssl::ConnectionInfo* downstream_connection_info_; + const Ssl::ConnectionInfo* downstream_connection_info_{}; const Router::RouteEntry* route_entry_{}; envoy::api::v2::core::Metadata metadata_{}; Envoy::StreamInfo::FilterStateImpl filter_state_{}; diff --git a/test/fuzz/BUILD b/test/fuzz/BUILD index 9d6efb14a862..5cb9b78c9bf0 100644 --- a/test/fuzz/BUILD +++ b/test/fuzz/BUILD @@ -51,6 +51,7 @@ envoy_cc_test_library( ":common_proto_cc", "//source/common/network:utility_lib", "//test/common/stream_info:test_util", + "//test/mocks/ssl:ssl_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", ], diff --git a/test/fuzz/utility.h b/test/fuzz/utility.h index f33e76b76ad7..73782414b256 100644 --- a/test/fuzz/utility.h +++ b/test/fuzz/utility.h @@ -4,6 +4,7 @@ #include "test/common/stream_info/test_util.h" #include "test/fuzz/common.pb.h" +#include "test/mocks/ssl/mocks.h" #include "test/mocks/upstream/host.h" #include "test/test_common/utility.h" @@ -50,7 +51,8 @@ inline test::fuzz::Headers toHeaders(const Http::HeaderMap& headers) { return fuzz_headers; } -inline TestStreamInfo fromStreamInfo(const test::fuzz::StreamInfo& stream_info) { +inline TestStreamInfo fromStreamInfo(const test::fuzz::StreamInfo& stream_info, + const Ssl::MockConnectionInfo* connection_info) { TestStreamInfo test_stream_info; test_stream_info.metadata_ = stream_info.dynamic_metadata(); // libc++ clocks don't track at nanosecond on macOS. @@ -73,6 +75,10 @@ inline TestStreamInfo fromStreamInfo(const test::fuzz::StreamInfo& stream_info) test_stream_info.downstream_local_address_ = address; test_stream_info.downstream_direct_remote_address_ = address; test_stream_info.downstream_remote_address_ = address; + test_stream_info.setDownstreamSslConnection(connection_info); + ON_CALL(*connection_info, subjectPeerCertificate()) + .WillByDefault(testing::Return( + "CN=Test Server,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US")); return test_stream_info; } From 8286635ffeb36fd7e09c04a7e63f0d0f17349105 Mon Sep 17 00:00:00 2001 From: Fred Douglas <43351173+fredlas@users.noreply.github.com> Date: Tue, 4 Jun 2019 14:32:20 -0700 Subject: [PATCH 08/13] config: centralize gRPC and REST method strings in SubscriptionFactory (#7092) Previously, constant strings like "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints" were all over eds.cc, rds_impl.cc, etc, being fed into SubscriptionFactory. Now, they are all centralized in SubscriptionFactory, the only place that actually uses them. #4991 Risk Level: low Signed-off-by: Fred Douglas --- source/common/config/BUILD | 16 ++++ source/common/config/subscription_factory.cc | 23 +++--- source/common/config/subscription_factory.h | 6 +- source/common/config/type_to_endpoint.cc | 79 +++++++++++++++++++ source/common/config/type_to_endpoint.h | 19 +++++ source/common/router/rds_impl.cc | 2 - source/common/router/scoped_rds.cc | 3 +- source/common/router/vhds.cc | 2 +- source/common/router/vhds.h | 7 +- source/common/secret/sds_api.cc | 2 - source/common/upstream/cds_api_impl.cc | 5 -- source/common/upstream/eds.cc | 2 - source/server/lds_api.cc | 2 - .../config/config_provider_impl_test.cc | 1 - .../config/subscription_factory_test.cc | 2 - test/common/router/vhds_test.cc | 3 +- test/mocks/config/mocks.h | 1 - 17 files changed, 134 insertions(+), 41 deletions(-) create mode 100644 source/common/config/type_to_endpoint.cc create mode 100644 source/common/config/type_to_endpoint.h diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 4d2020e5bab7..ffb1ed7f18b9 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -318,6 +318,7 @@ envoy_cc_library( ":grpc_mux_subscription_lib", ":grpc_subscription_lib", ":http_subscription_lib", + ":type_to_endpoint_lib", ":utility_lib", "//include/envoy/config:subscription_interface", "//include/envoy/upstream:cluster_manager_interface", @@ -338,6 +339,21 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "type_to_endpoint_lib", + srcs = ["type_to_endpoint.cc"], + hdrs = ["type_to_endpoint.h"], + deps = [ + "//source/common/grpc:common_lib", + "//source/common/protobuf", + "@envoy_api//envoy/api/v2:cds_cc", + "@envoy_api//envoy/api/v2:eds_cc", + "@envoy_api//envoy/api/v2:lds_cc", + "@envoy_api//envoy/api/v2:rds_cc", + "@envoy_api//envoy/api/v2:srds_cc", + ], +) + envoy_cc_library( name = "utility_lib", srcs = ["utility.cc"], diff --git a/source/common/config/subscription_factory.cc b/source/common/config/subscription_factory.cc index 84eda4fd253f..677634e822c3 100644 --- a/source/common/config/subscription_factory.cc +++ b/source/common/config/subscription_factory.cc @@ -5,6 +5,7 @@ #include "common/config/grpc_mux_subscription_impl.h" #include "common/config/grpc_subscription_impl.h" #include "common/config/http_subscription_impl.h" +#include "common/config/type_to_endpoint.h" #include "common/config/utility.h" #include "common/protobuf/protobuf.h" @@ -14,9 +15,9 @@ namespace Config { std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( const envoy::api::v2::core::ConfigSource& config, const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, - Stats::Scope& scope, const std::string& rest_method, const std::string& grpc_method, - absl::string_view type_url, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api, SubscriptionCallbacks& callbacks) { + Stats::Scope& scope, absl::string_view type_url, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, + SubscriptionCallbacks& callbacks) { std::unique_ptr result; SubscriptionStats stats = Utility::generateStats(scope); switch (config.config_source_specifier_case()) { @@ -39,9 +40,8 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( result = std::make_unique( local_info, cm, api_config_source.cluster_names()[0], dispatcher, random, Utility::apiConfigSourceRefreshDelay(api_config_source), - Utility::apiConfigSourceRequestTimeout(api_config_source), - *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(rest_method), callbacks, - stats, Utility::configSourceInitialFetchTimeout(config), validation_visitor); + Utility::apiConfigSourceRequestTimeout(api_config_source), restMethod(type_url), + callbacks, stats, Utility::configSourceInitialFetchTimeout(config), validation_visitor); break; case envoy::api::v2::core::ApiConfigSource::GRPC: result = std::make_unique( @@ -49,9 +49,8 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(), api_config_source, scope) ->create(), - dispatcher, random, - *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method), type_url, - callbacks, stats, scope, Utility::parseRateLimitSettings(api_config_source), + dispatcher, random, sotwGrpcMethod(type_url), type_url, callbacks, stats, scope, + Utility::parseRateLimitSettings(api_config_source), Utility::configSourceInitialFetchTimeout(config)); break; case envoy::api::v2::core::ApiConfigSource::DELTA_GRPC: { @@ -61,9 +60,9 @@ std::unique_ptr SubscriptionFactory::subscriptionFromConfigSource( Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(), api_config_source, scope) ->create(), - dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method), - type_url, random, scope, Utility::parseRateLimitSettings(api_config_source), callbacks, - stats, Utility::configSourceInitialFetchTimeout(config)); + dispatcher, deltaGrpcMethod(type_url), type_url, random, scope, + Utility::parseRateLimitSettings(api_config_source), callbacks, stats, + Utility::configSourceInitialFetchTimeout(config)); break; } default: diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h index 1ce63fa85901..8b557e46fc2f 100644 --- a/source/common/config/subscription_factory.h +++ b/source/common/config/subscription_factory.h @@ -33,9 +33,9 @@ class SubscriptionFactory { static std::unique_ptr subscriptionFromConfigSource( const envoy::api::v2::core::ConfigSource& config, const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, Runtime::RandomGenerator& random, - Stats::Scope& scope, const std::string& rest_method, const std::string& grpc_method, - absl::string_view type_url, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api& api, SubscriptionCallbacks& callbacks); + Stats::Scope& scope, absl::string_view type_url, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api, + SubscriptionCallbacks& callbacks); }; } // namespace Config diff --git a/source/common/config/type_to_endpoint.cc b/source/common/config/type_to_endpoint.cc new file mode 100644 index 000000000000..71905d570b0b --- /dev/null +++ b/source/common/config/type_to_endpoint.cc @@ -0,0 +1,79 @@ +#include "common/config/type_to_endpoint.h" + +#include "envoy/api/v2/cds.pb.h" +#include "envoy/api/v2/eds.pb.h" +#include "envoy/api/v2/lds.pb.h" +#include "envoy/api/v2/rds.pb.h" +#include "envoy/api/v2/srds.pb.h" + +#include "common/grpc/common.h" + +namespace Envoy { +namespace Config { + +const char UnknownMethod[] = "could_not_lookup_method_due_to_unknown_type_url"; + +#define TYPE_URL_IS(x) \ + (type_url == Grpc::Common::typeUrl(envoy::api::v2::x().GetDescriptor()->full_name())) +const Protobuf::MethodDescriptor& deltaGrpcMethod(absl::string_view type_url) { + std::string method_name = UnknownMethod; + if (TYPE_URL_IS(RouteConfiguration)) { + method_name = "envoy.api.v2.RouteDiscoveryService.DeltaRoutes"; + } else if (TYPE_URL_IS(ScopedRouteConfiguration)) { + method_name = "envoy.api.v2.ScopedRoutesDiscoveryService.DeltaScopedRoutes"; + } else if (TYPE_URL_IS(route::VirtualHost)) { + method_name = "envoy.api.v2.VirtualHostDiscoveryService.DeltaVirtualHosts"; + } else if (TYPE_URL_IS(auth::Secret)) { + method_name = "envoy.service.discovery.v2.SecretDiscoveryService.DeltaSecrets"; + } else if (TYPE_URL_IS(Cluster)) { + method_name = "envoy.api.v2.ClusterDiscoveryService.DeltaClusters"; + } else if (TYPE_URL_IS(ClusterLoadAssignment)) { + method_name = "envoy.api.v2.EndpointDiscoveryService.DeltaEndpoints"; + } else if (TYPE_URL_IS(Listener)) { + method_name = "envoy.api.v2.ListenerDiscoveryService.DeltaListeners"; + } + ASSERT(method_name != UnknownMethod); + return *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(method_name); +} + +const Protobuf::MethodDescriptor& sotwGrpcMethod(absl::string_view type_url) { + std::string method_name = UnknownMethod; + if (TYPE_URL_IS(RouteConfiguration)) { + method_name = "envoy.api.v2.RouteDiscoveryService.StreamRoutes"; + } else if (TYPE_URL_IS(ScopedRouteConfiguration)) { + method_name = "envoy.api.v2.ScopedRoutesDiscoveryService.StreamScopedRoutes"; + } else if (TYPE_URL_IS(auth::Secret)) { + method_name = "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets"; + } else if (TYPE_URL_IS(Cluster)) { + method_name = "envoy.api.v2.ClusterDiscoveryService.StreamClusters"; + } else if (TYPE_URL_IS(ClusterLoadAssignment)) { + method_name = "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints"; + } else if (TYPE_URL_IS(Listener)) { + method_name = "envoy.api.v2.ListenerDiscoveryService.StreamListeners"; + } + ASSERT(method_name != UnknownMethod); + return *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(method_name); +} + +const Protobuf::MethodDescriptor& restMethod(absl::string_view type_url) { + std::string method_name = UnknownMethod; + if (TYPE_URL_IS(RouteConfiguration)) { + method_name = "envoy.api.v2.RouteDiscoveryService.FetchRoutes"; + } else if (TYPE_URL_IS(ScopedRouteConfiguration)) { + method_name = "envoy.api.v2.ScopedRoutesDiscoveryService.FetchScopedRoutes"; + } else if (TYPE_URL_IS(auth::Secret)) { + method_name = "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets"; + } else if (TYPE_URL_IS(Cluster)) { + method_name = "envoy.api.v2.ClusterDiscoveryService.FetchClusters"; + } else if (TYPE_URL_IS(ClusterLoadAssignment)) { + method_name = "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints"; + } else if (TYPE_URL_IS(Listener)) { + method_name = "envoy.api.v2.ListenerDiscoveryService.FetchListeners"; + } + ASSERT(method_name != UnknownMethod); + return *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(method_name); +} +#undef TYPE_URL_IS + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/type_to_endpoint.h b/source/common/config/type_to_endpoint.h new file mode 100644 index 000000000000..c5ef2d7e7e01 --- /dev/null +++ b/source/common/config/type_to_endpoint.h @@ -0,0 +1,19 @@ +#pragma once + +#include "common/protobuf/protobuf.h" + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Config { + +// Translates an xDS resource type_url to the name of the delta gRPC service that carries it. +const Protobuf::MethodDescriptor& deltaGrpcMethod(absl::string_view type_url); +// Translates an xDS resource type_url to the name of the state-of-the-world gRPC service that +// carries it. +const Protobuf::MethodDescriptor& sotwGrpcMethod(absl::string_view type_url); +// Translates an xDS resource type_url to the name of the REST service that carries it. +const Protobuf::MethodDescriptor& restMethod(absl::string_view type_url); + +} // namespace Config +} // namespace Envoy diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 37dc4491db90..5ea463ec37ce 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -70,8 +70,6 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( rds.config_source(), factory_context.localInfo(), factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), *scope_, - "envoy.api.v2.RouteDiscoveryService.FetchRoutes", - "envoy.api.v2.RouteDiscoveryService.StreamRoutes", Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), factory_context.messageValidationVisitor(), factory_context.api(), *this); diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index 4462995081f7..b2de09f8a8e9 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -88,8 +88,7 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription( subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( scoped_rds.scoped_rds_config_source(), factory_context.localInfo(), factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), - *scope_, "envoy.api.v2.ScopedRoutesDiscoveryService.FetchScopedRoutes", - "envoy.api.v2.ScopedRoutesDiscoveryService.StreamScopedRoutes", + *scope_, Grpc::Common::typeUrl( envoy::api::v2::ScopedRouteConfiguration().GetDescriptor()->full_name()), validation_visitor_, factory_context.api(), *this); diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index eb39ec65b238..3bd07c05fee9 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -44,7 +44,7 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, subscription_ = factory_function( config_update_info_->routeConfiguration().vhds().config_source(), factory_context.localInfo(), factory_context.dispatcher(), factory_context.clusterManager(), factory_context.random(), - *scope_, "none", "envoy.api.v2.VirtualHostDiscoveryService.DeltaVirtualHosts", + *scope_, Grpc::Common::typeUrl(envoy::api::v2::route::VirtualHost().GetDescriptor()->full_name()), factory_context.messageValidationVisitor(), factory_context.api(), *this); } diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 1767b98db720..2463b177c6bb 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -40,9 +40,9 @@ struct VhdsStats { typedef std::unique_ptr (*SubscriptionFactoryFunction)( const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, Event::Dispatcher&, - Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, Stats::Scope&, const std::string&, - const std::string&, absl::string_view, ProtobufMessage::ValidationVisitor& validation_visitor, - Api::Api&, Envoy::Config::SubscriptionCallbacks&); + Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, Stats::Scope&, absl::string_view, + ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api&, + Envoy::Config::SubscriptionCallbacks&); class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, Logger::Loggable { @@ -56,7 +56,6 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, ~VhdsSubscription() override { init_target_.ready(); } // Config::SubscriptionCallbacks - // TODO(fredlas) deduplicate void onConfigUpdate(const Protobuf::RepeatedPtrField&, const std::string&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 4354866552d5..657aa9d3d54e 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -80,8 +80,6 @@ void SdsApi::validateUpdateSize(int num_resources) { void SdsApi::initialize() { subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, - "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", - "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets", Grpc::Common::typeUrl(envoy::api::v2::auth::Secret().GetDescriptor()->full_name()), validation_visitor_, api_, *this); subscription_->start({sds_config_name_}); diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index 580bd4f2912f..a2840fdefe59 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -34,13 +34,8 @@ CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, Clu validation_visitor_(validation_visitor) { Config::Utility::checkLocalInfo("cds", local_info); - const bool is_delta = (cds_config.api_config_source().api_type() == - envoy::api::v2::core::ApiConfigSource::DELTA_GRPC); - const std::string grpc_method = is_delta ? "envoy.api.v2.ClusterDiscoveryService.DeltaClusters" - : "envoy.api.v2.ClusterDiscoveryService.StreamClusters"; subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource( cds_config, local_info, dispatcher, cm, random, *scope_, - "envoy.api.v2.ClusterDiscoveryService.FetchClusters", grpc_method, Grpc::Common::typeUrl(envoy::api::v2::Cluster().GetDescriptor()->full_name()), validation_visitor, api, *this); } diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index f86c261802f5..47419e1bb1c5 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -27,8 +27,6 @@ EdsClusterImpl::EdsClusterImpl( const auto& eds_config = cluster.eds_cluster_config().eds_config(); subscription_ = Config::SubscriptionFactory::subscriptionFromConfigSource( eds_config, local_info_, dispatcher, cm, random, info_->statsScope(), - "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints", - "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints", Grpc::Common::typeUrl(envoy::api::v2::ClusterLoadAssignment().GetDescriptor()->full_name()), factory_context.messageValidationVisitor(), factory_context.api(), *this); } diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index b0f297f49948..df2bc694cade 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -26,8 +26,6 @@ LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config, validation_visitor_(validation_visitor) { subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( lds_config, local_info, dispatcher, cm, random, *scope_, - "envoy.api.v2.ListenerDiscoveryService.FetchListeners", - "envoy.api.v2.ListenerDiscoveryService.StreamListeners", Grpc::Common::typeUrl(envoy::api::v2::Listener().GetDescriptor()->full_name()), validation_visitor_, api, *this); Config::Utility::checkLocalInfo("lds", local_info); diff --git a/test/common/config/config_provider_impl_test.cc b/test/common/config/config_provider_impl_test.cc index 23e9219f8a28..2b3e320a7a62 100644 --- a/test/common/config/config_provider_impl_test.cc +++ b/test/common/config/config_provider_impl_test.cc @@ -55,7 +55,6 @@ class DummyConfigSubscription : public ConfigSubscriptionInstance, void start() override {} // Envoy::Config::SubscriptionCallbacks - // TODO(fredlas) deduplicate void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, const std::string& version_info) override { auto config = TestUtility::anyConvert(resources[0]); diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_test.cc index 0c6f0fffa795..8069561f687d 100644 --- a/test/common/config/subscription_factory_test.cc +++ b/test/common/config/subscription_factory_test.cc @@ -37,8 +37,6 @@ class SubscriptionFactoryTest : public testing::Test { subscriptionFromConfigSource(const envoy::api::v2::core::ConfigSource& config) { return SubscriptionFactory::subscriptionFromConfigSource( config, local_info_, dispatcher_, cm_, random_, stats_store_, - "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints", - "envoy.api.v2.EndpointDiscoveryService.StreamEndpoints", Config::TypeUrl::get().ClusterLoadAssignment, validation_visitor_, *api_, callbacks_); } diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index 8c094c6f5b3b..cb35c34cb3ee 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -44,8 +44,7 @@ class VhdsTest : public testing::Test { factory_function_ = { [](const envoy::api::v2::core::ConfigSource&, const LocalInfo::LocalInfo&, Event::Dispatcher&, Upstream::ClusterManager&, Envoy::Runtime::RandomGenerator&, - Stats::Scope&, const std::string&, const std::string&, absl::string_view, - ProtobufMessage::ValidationVisitor&, Api::Api&, + Stats::Scope&, absl::string_view, ProtobufMessage::ValidationVisitor&, Api::Api&, Envoy::Config::SubscriptionCallbacks&) -> std::unique_ptr { return std::unique_ptr(); }}; diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h index 596b73837430..7c37573a764f 100644 --- a/test/mocks/config/mocks.h +++ b/test/mocks/config/mocks.h @@ -31,7 +31,6 @@ template class MockSubscriptionCallbacks : public Subscript } template static std::string resourceName_(const T& resource) { return resource.name(); } - // TODO(fredlas) deduplicate MOCK_METHOD2_T(onConfigUpdate, void(const Protobuf::RepeatedPtrField& resources, const std::string& version_info)); MOCK_METHOD3_T(onConfigUpdate, From be9d2f8a8f669edc320e7e6c1d2fc1896a81d28b Mon Sep 17 00:00:00 2001 From: Dan Rosen Date: Tue, 4 Jun 2019 18:49:30 -0400 Subject: [PATCH 09/13] router: implement append_cluster, append_upstream_host, and do_not_forward (#6893) Implement three new router behaviors, append_cluster, append_upstream_host and do_not_forward. These are enabled via the new StreamDebugInfo class, although no API is provided to set them. Risk Level: low/medium (changes to router) Testing: added unit tests for behaviors separately and combined. Docs Changes: n/a Release Notes: n/a (I think...) Fixes: #6453 Signed-off-by: Dan Rosen --- include/envoy/http/conn_pool.h | 5 + source/common/http/headers.h | 4 + source/common/http/http1/conn_pool.h | 1 + source/common/http/http2/conn_pool.h | 1 + source/common/router/BUILD | 12 ++ source/common/router/debug_config.cc | 24 +++ source/common/router/debug_config.h | 68 +++++++++ source/common/router/router.cc | 66 +++++++- source/common/router/router.h | 1 + test/common/http/http1/conn_pool_test.cc | 5 + test/common/http/http2/conn_pool_test.cc | 5 + test/common/router/router_test.cc | 187 +++++++++++++++++++++++ test/mocks/http/conn_pool.cc | 4 +- test/mocks/http/conn_pool.h | 1 + 14 files changed, 378 insertions(+), 6 deletions(-) create mode 100644 source/common/router/debug_config.cc create mode 100644 source/common/router/debug_config.h diff --git a/include/envoy/http/conn_pool.h b/include/envoy/http/conn_pool.h index 41017293ad71..5bd5a2390bb8 100644 --- a/include/envoy/http/conn_pool.h +++ b/include/envoy/http/conn_pool.h @@ -116,6 +116,11 @@ class Instance : public Event::DeferredDeletable { * should be done by resetting the stream. */ virtual Cancellable* newStream(Http::StreamDecoder& response_decoder, Callbacks& callbacks) PURE; + + /** + * @return Upstream::HostDescriptionConstSharedPtr the host for which connections are pooled. + */ + virtual Upstream::HostDescriptionConstSharedPtr host() const PURE; }; typedef std::unique_ptr InstancePtr; diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 796671ea4f3b..3a74c1eb277e 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -37,6 +37,7 @@ class HeaderValues { const LowerCaseString Date{"date"}; const LowerCaseString EnvoyAttemptCount{"x-envoy-attempt-count"}; const LowerCaseString EnvoyAuthPartialBody{"x-envoy-auth-partial-body"}; + const LowerCaseString EnvoyCluster{"x-envoy-cluster"}; const LowerCaseString EnvoyDegraded{"x-envoy-degraded"}; const LowerCaseString EnvoyDownstreamServiceCluster{"x-envoy-downstream-service-cluster"}; const LowerCaseString EnvoyDownstreamServiceNode{"x-envoy-downstream-service-node"}; @@ -48,6 +49,7 @@ class HeaderValues { const LowerCaseString EnvoyInternalRequest{"x-envoy-internal"}; const LowerCaseString EnvoyIpTags{"x-envoy-ip-tags"}; const LowerCaseString EnvoyMaxRetries{"x-envoy-max-retries"}; + const LowerCaseString EnvoyNotForwarded{"x-envoy-not-forwarded"}; const LowerCaseString EnvoyOriginalDstHost{"x-envoy-original-dst-host"}; const LowerCaseString EnvoyOriginalPath{"x-envoy-original-path"}; const LowerCaseString EnvoyOverloaded{"x-envoy-overloaded"}; @@ -57,6 +59,8 @@ class HeaderValues { const LowerCaseString EnvoyRetriableStatusCodes{"x-envoy-retriable-status-codes"}; const LowerCaseString EnvoyUpstreamAltStatName{"x-envoy-upstream-alt-stat-name"}; const LowerCaseString EnvoyUpstreamCanary{"x-envoy-upstream-canary"}; + const LowerCaseString EnvoyUpstreamHostAddress{"x-envoy-upstream-host-address"}; + const LowerCaseString EnvoyUpstreamHostname{"x-envoy-upstream-hostname"}; const LowerCaseString EnvoyUpstreamRequestTimeoutAltResponse{ "x-envoy-upstream-rq-timeout-alt-response"}; const LowerCaseString EnvoyUpstreamRequestTimeoutMs{"x-envoy-upstream-rq-timeout-ms"}; diff --git a/source/common/http/http1/conn_pool.h b/source/common/http/http1/conn_pool.h index 9f28b0ac8c58..832de5925042 100644 --- a/source/common/http/http1/conn_pool.h +++ b/source/common/http/http1/conn_pool.h @@ -43,6 +43,7 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase { bool hasActiveConnections() const override; ConnectionPool::Cancellable* newStream(StreamDecoder& response_decoder, ConnectionPool::Callbacks& callbacks) override; + Upstream::HostDescriptionConstSharedPtr host() const override { return host_; }; // ConnPoolImplBase void checkForDrained() override; diff --git a/source/common/http/http2/conn_pool.h b/source/common/http/http2/conn_pool.h index ae24573fbd0a..134f38750a6e 100644 --- a/source/common/http/http2/conn_pool.h +++ b/source/common/http/http2/conn_pool.h @@ -36,6 +36,7 @@ class ConnPoolImpl : public ConnectionPool::Instance, public ConnPoolImplBase { bool hasActiveConnections() const override; ConnectionPool::Cancellable* newStream(Http::StreamDecoder& response_decoder, ConnectionPool::Callbacks& callbacks) override; + Upstream::HostDescriptionConstSharedPtr host() const override { return host_; }; protected: struct ActiveClient : public Network::ConnectionCallbacks, diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 3cf5d8568b8e..4bc5d19057f7 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -66,6 +66,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "debug_config_lib", + srcs = ["debug_config.cc"], + hdrs = ["debug_config.h"], + deps = [ + "//include/envoy/http:header_map_interface", + "//include/envoy/stream_info:filter_state_interface", + "//source/common/common:macros", + ], +) + envoy_cc_library( name = "route_config_update_impl_lib", srcs = ["route_config_update_receiver_impl.cc"], @@ -204,6 +215,7 @@ envoy_cc_library( hdrs = ["router.h"], deps = [ ":config_lib", + ":debug_config_lib", ":header_parser_lib", ":retry_state_lib", "//include/envoy/event:dispatcher_interface", diff --git a/source/common/router/debug_config.cc b/source/common/router/debug_config.cc new file mode 100644 index 000000000000..8ec9cb59687e --- /dev/null +++ b/source/common/router/debug_config.cc @@ -0,0 +1,24 @@ +#include "common/router/debug_config.h" + +#include "common/common/macros.h" + +namespace Envoy { +namespace Router { + +DebugConfig::DebugConfig(bool append_cluster, absl::optional cluster_header, + bool append_upstream_host, + absl::optional hostname_header, + absl::optional host_address_header, + bool do_not_forward, + absl::optional not_forwarded_header) + : append_cluster_(append_cluster), cluster_header_(std::move(cluster_header)), + append_upstream_host_(append_upstream_host), hostname_header_(std::move(hostname_header)), + host_address_header_(std::move(host_address_header)), do_not_forward_(do_not_forward), + not_forwarded_header_(std::move(not_forwarded_header)) {} + +const std::string& DebugConfig::key() { + CONSTRUCT_ON_FIRST_USE(std::string, "envoy.router.debug_config"); +} + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/debug_config.h b/source/common/router/debug_config.h new file mode 100644 index 000000000000..96f99080ab7c --- /dev/null +++ b/source/common/router/debug_config.h @@ -0,0 +1,68 @@ +#pragma once + +#include + +#include "envoy/http/header_map.h" +#include "envoy/stream_info/filter_state.h" + +#include "absl/types/optional.h" + +namespace Envoy { +namespace Router { + +/** + * Configuration for debugging router behavior. The router tries to look up an instance of this + * class by its `key`, in FilterState (via StreamInfo). If it's not present, debugging features are + * not enabled. + * + * There is currently no public API for populating this configuration -- neither globally nor + * per-request -- users desiring to use the router debugging features should create and install + * their own custom StreamDecoderFilter to set DebugConfig as desired before the router consumes + * it. + * + * This is intended to be temporary, and should be replaced by some proper configuration (e.g. in + * router.proto) when we get unified matchers. See https://github.com/envoyproxy/envoy/issues/5569. + * + * TODO(mergeconflict): Keep this promise. + */ +struct DebugConfig : public StreamInfo::FilterState::Object { + + DebugConfig(bool append_cluster, absl::optional cluster_header, + bool append_upstream_host, absl::optional hostname_header, + absl::optional host_address_header, bool do_not_forward, + absl::optional not_forwarded_header); + + /** + * @return the string key for finding DebugConfig, if present, in FilterState. + */ + static const std::string& key(); + + /** + * Append cluster information as a response header if `append_cluster_` is true. The router will + * use `cluster_header_` as the header name, if specified, or "x-envoy-cluster" by default. + */ + bool append_cluster_{}; + absl::optional cluster_header_; + + /** + * Append upstream host name and address as response headers, if `append_upstream_host_` is true. + * The router will use `hostname_header_` and `host_address_header_` as the header names, if + * specified, or "x-envoy-upstream-hostname" and "x-envoy-upstream-host-address" by default. + */ + bool append_upstream_host_{}; + absl::optional hostname_header_; + absl::optional host_address_header_; + + /** + * Do not forward the associated request to the upstream cluster, if `do_not_forward_` is true. + * If the router would have forwarded it (assuming all other preconditions are met), it will + * instead respond with a 204 "no content." Append `not_forwarded_header_`, if specified, or + * "x-envoy-not-forwarded" by default. Any debug headers specified above (or others introduced by + * other filters) will be appended to this empty response. + */ + bool do_not_forward_{}; + absl::optional not_forwarded_header_; +}; + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 1b92c21b322b..0edc7133807e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -24,9 +24,12 @@ #include "common/http/message_impl.h" #include "common/http/utility.h" #include "common/router/config_impl.h" +#include "common/router/debug_config.h" #include "common/router/retry_state_impl.h" #include "common/tracing/http_tracer_impl.h" +#include "extensions/filters/http/well_known_names.h" + namespace Envoy { namespace Router { namespace { @@ -296,6 +299,15 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e downstream_headers_ = &headers; + // Extract debug configuration from filter state. This is used further along to determine whether + // we should append cluster and host headers to the response, and whether to forward the request + // upstream. + const StreamInfo::FilterState& filter_state = callbacks_->streamInfo().filterState(); + const DebugConfig* debug_config = + filter_state.hasData(DebugConfig::key()) + ? &(filter_state.getDataReadOnly(DebugConfig::key())) + : nullptr; + // TODO: Maybe add a filter API for this. grpc_request_ = Grpc::Common::hasGrpcContentType(headers); @@ -303,6 +315,11 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // that get handled by earlier filters. config_.stats_.rq_total_.inc(); + // Initialize the `modify_headers` function as a no-op (so we don't have to remember to check it + // against nullptr before calling it), and feed it behavior later if/when we have cluster info + // headers to append. + std::function modify_headers = [](Http::HeaderMap&) {}; + // Determine if there is a route entry or a direct response for the request. route_ = callbacks_->route(); if (!route_) { @@ -311,7 +328,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e headers.Path()->value().getStringView()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); - callbacks_->sendLocalReply(Http::Code::NotFound, "", nullptr, absl::nullopt, + callbacks_->sendLocalReply(Http::Code::NotFound, "", modify_headers, absl::nullopt, StreamInfo::ResponseCodeDetails::get().RouteNotFound); return Http::FilterHeadersStatus::StopIteration; } @@ -339,14 +356,20 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // A route entry matches for the request. route_entry_ = route_->routeEntry(); callbacks_->streamInfo().setRouteName(route_entry_->routeName()); - + if (debug_config && debug_config->append_cluster_) { + // The cluster name will be appended to any local or upstream responses from this point. + modify_headers = [this, debug_config](Http::HeaderMap& headers) { + headers.addCopy(debug_config->cluster_header_.value_or(Http::Headers::get().EnvoyCluster), + route_entry_->clusterName()); + }; + } Upstream::ThreadLocalCluster* cluster = config_.cm_.get(route_entry_->clusterName()); if (!cluster) { config_.stats_.no_cluster_.inc(); ENVOY_STREAM_LOG(debug, "unknown cluster '{}'", *callbacks_, route_entry_->clusterName()); callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound); - callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", nullptr, + callbacks_->sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "", modify_headers, absl::nullopt, StreamInfo::ResponseCodeDetails::get().ClusterNotFound); return Http::FilterHeadersStatus::StopIteration; @@ -375,10 +398,12 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, true); callbacks_->sendLocalReply( Http::Code::ServiceUnavailable, "maintenance mode", - [this](Http::HeaderMap& headers) { + [modify_headers, this](Http::HeaderMap& headers) { if (!config_.suppress_envoy_headers_) { headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); } + // Note: append_cluster_info does not respect suppress_envoy_headers. + modify_headers(headers); }, absl::nullopt, StreamInfo::ResponseCodeDetails::get().MaintenanceMode); cluster_->stats().upstream_rq_maintenance_mode_.inc(); @@ -391,6 +416,31 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e sendNoHealthyUpstreamResponse(); return Http::FilterHeadersStatus::StopIteration; } + if (debug_config && debug_config->append_upstream_host_) { + // The hostname and address will be appended to any local or upstream responses from this point, + // possibly in addition to the cluster name. + modify_headers = [modify_headers, debug_config, conn_pool](Http::HeaderMap& headers) { + modify_headers(headers); + headers.addCopy( + debug_config->hostname_header_.value_or(Http::Headers::get().EnvoyUpstreamHostname), + conn_pool->host()->hostname()); + headers.addCopy(debug_config->host_address_header_.value_or( + Http::Headers::get().EnvoyUpstreamHostAddress), + conn_pool->host()->address()->asString()); + }; + } + + // If we've been instructed not to forward the request upstream, send an empty local response. + if (debug_config && debug_config->do_not_forward_) { + modify_headers = [modify_headers, debug_config](Http::HeaderMap& headers) { + modify_headers(headers); + headers.addCopy( + debug_config->not_forwarded_header_.value_or(Http::Headers::get().EnvoyNotForwarded), + "true"); + }; + callbacks_->sendLocalReply(Http::Code::NoContent, "", modify_headers, absl::nullopt, ""); + return Http::FilterHeadersStatus::StopIteration; + } hedging_params_ = FilterUtility::finalHedgingParams(*route_entry_, headers); @@ -426,6 +476,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e ENVOY_STREAM_LOG(debug, "router decoding headers:\n{}", *callbacks_, headers); + // Hang onto the modify_headers function for later use in handling upstream responses. + modify_headers_ = modify_headers; + UpstreamRequestPtr upstream_request = std::make_unique(*this, *conn_pool); upstream_request->moveIntoList(std::move(upstream_request), upstream_requests_); upstream_requests_.front()->encodeHeaders(end_stream); @@ -455,7 +508,7 @@ Http::ConnectionPool::Instance* Filter::getConnPool() { void Filter::sendNoHealthyUpstreamResponse() { callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoHealthyUpstream); chargeUpstreamCode(Http::Code::ServiceUnavailable, nullptr, false); - callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", nullptr, + callbacks_->sendLocalReply(Http::Code::ServiceUnavailable, "no healthy upstream", modify_headers_, absl::nullopt, StreamInfo::ResponseCodeDetails::get().NoHealthyUpstream); } @@ -734,6 +787,7 @@ void Filter::onUpstreamAbort(Http::Code code, StreamInfo::ResponseFlag response_ if (dropped && !config_.suppress_envoy_headers_) { headers.insertEnvoyOverloaded().value(Http::Headers::get().EnvoyOverloadedValues.True); } + modify_headers_(headers); }, absl::nullopt, details); } @@ -892,6 +946,8 @@ void Filter::onUpstreamHeaders(uint64_t response_code, Http::HeaderMapPtr&& head UpstreamRequest& upstream_request, bool end_stream) { ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={}", *callbacks_, end_stream); + modify_headers_(*headers); + upstream_request.upstream_host_->outlierDetector().putHttpResponseCode(response_code); if (headers->EnvoyImmediateHealthCheckFail() != nullptr) { diff --git a/source/common/router/router.h b/source/common/router/router.h index cf3e181428ef..a4b93e13ef44 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -497,6 +497,7 @@ class Filter : Logger::Loggable, MonotonicTime downstream_request_complete_time_; uint32_t buffer_limit_{0}; MetadataMatchCriteriaConstPtr metadata_match_; + std::function modify_headers_; // list of cookies to add to upstream headers std::vector downstream_set_cookies_; diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 51ccfbabc07c..95c114232acf 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -203,6 +203,11 @@ struct ActiveTestRequest { ConnPoolCallbacks callbacks_; }; +/** + * Verify that the pool's host is a member of the cluster the pool was constructed with. + */ +TEST_F(Http1ConnPoolImplTest, Host) { EXPECT_EQ(cluster_.get(), &conn_pool_.host()->cluster()); } + /** * Verify that connections are drained when requested. */ diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index c7b5577154a3..48a5929c8d0e 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -198,6 +198,11 @@ void Http2ConnPoolImplTest::completeRequestCloseUpstream(size_t index, ActiveTes closeClient(index); } +/** + * Verify that the pool retains and returns the host it was constructed with. + */ +TEST_F(Http2ConnPoolImplTest, Host) { EXPECT_EQ(host_, pool_.host()); } + /** * Verify that connections are drained when requested. */ diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index d38ae33d2c71..22458eb26b52 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -10,6 +10,7 @@ #include "common/network/socket_option_factory.h" #include "common/network/utility.h" #include "common/router/config_impl.h" +#include "common/router/debug_config.h" #include "common/router/router.h" #include "common/tracing/http_tracer_impl.h" #include "common/upstream/upstream_impl.h" @@ -220,6 +221,11 @@ class RouterTestBase : public testing::Test { envoy::type::FractionalPercent::HUNDRED); } + void testAppendCluster(absl::optional cluster_header_name); + void testAppendUpstreamHost(absl::optional hostname_header_name, + absl::optional host_address_header_name); + void testDoNotForward(absl::optional not_forwarded_header_name); + Event::SimulatedTimeSystem test_time_; std::string upstream_zone_{"to_az"}; envoy::api::v2::core::Locality upstream_locality_; @@ -807,6 +813,187 @@ TEST_F(RouterTest, EnvoyUpstreamServiceTime) { EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); } +// Validate that the cluster is appended to the response when configured. +void RouterTestBase::testAppendCluster(absl::optional cluster_header_name) { + auto debug_config = std::make_unique( + /* append_cluster */ true, + /* cluster_header */ cluster_header_name, + /* append_upstream_host */ false, + /* hostname_header */ absl::nullopt, + /* host_address_header */ absl::nullopt, + /* do_not_forward */ false, + /* not_forwarded_header */ absl::nullopt); + callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly); + + NiceMock encoder; + Http::StreamDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks) + -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder, cm_.conn_pool_.host_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}}); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + EXPECT_CALL(callbacks_, encodeHeaders_(_, true)) + .WillOnce(Invoke([&cluster_header_name](Http::HeaderMap& headers, bool) { + const Http::HeaderEntry* cluster_header = + headers.get(cluster_header_name.value_or(Http::Headers::get().EnvoyCluster)); + EXPECT_NE(nullptr, cluster_header); + EXPECT_EQ("fake_cluster", cluster_header->value().getStringView()); + })); + response_decoder->decodeHeaders(std::move(response_headers), true); + EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); +} + +// Append cluster with default header name. +TEST_F(RouterTest, AppendCluster0) { testAppendCluster(absl::nullopt); } + +// Append cluster with custom header name. +TEST_F(RouterTest, AppendCluster1) { + testAppendCluster(absl::make_optional(Http::LowerCaseString("x-custom-cluster"))); +} + +// Validate that the upstream hostname and address are appended to the response when configured. +void RouterTestBase::testAppendUpstreamHost( + absl::optional hostname_header_name, + absl::optional host_address_header_name) { + auto debug_config = std::make_unique( + /* append_cluster */ false, + /* cluster_header */ absl::nullopt, + /* append_upstream_host */ true, + /* hostname_header */ hostname_header_name, + /* host_address_header */ host_address_header_name, + /* do_not_forward */ false, + /* not_forwarded_header */ absl::nullopt); + callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly); + cm_.conn_pool_.host_->hostname_ = "scooby.doo"; + + NiceMock encoder; + Http::StreamDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks) + -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder, cm_.conn_pool_.host_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}}); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + EXPECT_CALL(callbacks_, encodeHeaders_(_, true)) + .WillOnce(Invoke([&hostname_header_name, &host_address_header_name](Http::HeaderMap& headers, + bool) { + const Http::HeaderEntry* hostname_header = + headers.get(hostname_header_name.value_or(Http::Headers::get().EnvoyUpstreamHostname)); + EXPECT_NE(nullptr, hostname_header); + EXPECT_EQ("scooby.doo", hostname_header->value().getStringView()); + + const Http::HeaderEntry* host_address_header = headers.get( + host_address_header_name.value_or(Http::Headers::get().EnvoyUpstreamHostAddress)); + EXPECT_NE(nullptr, host_address_header); + EXPECT_EQ("10.0.0.5:9211", host_address_header->value().getStringView()); + })); + response_decoder->decodeHeaders(std::move(response_headers), true); + EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); +} + +// Append hostname and address with default header names. +TEST_F(RouterTest, AppendUpstreamHost00) { testAppendUpstreamHost(absl::nullopt, absl::nullopt); } + +// Append hostname and address with custom host address header name. +TEST_F(RouterTest, AppendUpstreamHost01) { + testAppendUpstreamHost( + absl::nullopt, absl::make_optional(Http::LowerCaseString("x-custom-upstream-host-address"))); +} + +// Append hostname and address with custom hostname header name. +TEST_F(RouterTest, AppendUpstreamHost10) { + testAppendUpstreamHost(absl::make_optional(Http::LowerCaseString("x-custom-upstream-hostname")), + absl::nullopt); +} + +// Append hostname and address with custom header names. +TEST_F(RouterTest, AppendUpstreamHost11) { + testAppendUpstreamHost( + absl::make_optional(Http::LowerCaseString("x-custom-upstream-hostname")), + absl::make_optional(Http::LowerCaseString("x-custom-upstream-host-address"))); +} + +// Validate that the request is not forwarded upstream when configured. +void RouterTestBase::testDoNotForward( + absl::optional not_forwarded_header_name) { + auto debug_config = std::make_unique( + /* append_cluster */ false, + /* cluster_header */ absl::nullopt, + /* append_upstream_host */ false, + /* hostname_header */ absl::nullopt, + /* host_address_header */ absl::nullopt, + /* do_not_forward */ true, + /* not_forwarded_header */ not_forwarded_header_name); + callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly); + + Http::TestHeaderMapImpl response_headers{ + {":status", "204"}, + {not_forwarded_header_name.value_or(Http::Headers::get().EnvoyNotForwarded).get(), "true"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); + + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); +} + +// Do not forward, with default not-forwarded header name +TEST_F(RouterTest, DoNotForward0) { testDoNotForward(absl::nullopt); } + +// Do not forward, with custom not-forwarded header name +TEST_F(RouterTest, DoNotForward1) { + testDoNotForward(absl::make_optional(Http::LowerCaseString("x-custom-not-forwarded"))); +} + +// Validate that all DebugConfig options play nicely with each other. +TEST_F(RouterTest, AllDebugConfig) { + auto debug_config = std::make_unique( + /* append_cluster */ true, + /* cluster_header */ absl::nullopt, + /* append_upstream_host */ true, + /* hostname_header */ absl::nullopt, + /* host_address_header */ absl::nullopt, + /* do_not_forward */ true, + /* not_forwarded_header */ absl::nullopt); + callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly); + cm_.conn_pool_.host_->hostname_ = "scooby.doo"; + + Http::TestHeaderMapImpl response_headers{{":status", "204"}, + {"x-envoy-cluster", "fake_cluster"}, + {"x-envoy-upstream-hostname", "scooby.doo"}, + {"x-envoy-upstream-host-address", "10.0.0.5:9211"}, + {"x-envoy-not-forwarded", "true"}}; + EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), true)); + + Http::TestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); +} + // Validate that x-envoy-upstream-service-time is not added when Envoy header // suppression is enabled. // TODO(htuch): Probably should be TEST_P with diff --git a/test/mocks/http/conn_pool.cc b/test/mocks/http/conn_pool.cc index 0140260e8bc1..77d21bdf85a3 100644 --- a/test/mocks/http/conn_pool.cc +++ b/test/mocks/http/conn_pool.cc @@ -8,7 +8,9 @@ MockCancellable::MockCancellable() = default; MockCancellable::~MockCancellable() = default; MockInstance::MockInstance() - : host_{std::make_shared>()} {} + : host_{std::make_shared>()} { + ON_CALL(*this, host()).WillByDefault(Return(host_)); +} MockInstance::~MockInstance() = default; } // namespace ConnectionPool diff --git a/test/mocks/http/conn_pool.h b/test/mocks/http/conn_pool.h index e6fbcd8baf39..18ea24063c56 100644 --- a/test/mocks/http/conn_pool.h +++ b/test/mocks/http/conn_pool.h @@ -32,6 +32,7 @@ class MockInstance : public Instance { MOCK_CONST_METHOD0(hasActiveConnections, bool()); MOCK_METHOD2(newStream, Cancellable*(Http::StreamDecoder& response_decoder, Http::ConnectionPool::Callbacks& callbacks)); + MOCK_CONST_METHOD0(host, Upstream::HostDescriptionConstSharedPtr()); std::shared_ptr> host_; }; From 30c44cc15aca484abd2355b049436dfc99137b75 Mon Sep 17 00:00:00 2001 From: Derek Argueta Date: Tue, 4 Jun 2019 16:39:07 -0700 Subject: [PATCH 10/13] [test] remove translateTcpRateLimitFilter (#7137) Signed-off-by: Derek Argueta --- source/common/config/BUILD | 1 - source/common/config/filter_json.cc | 20 ---------------- source/common/config/filter_json.h | 11 --------- test/common/network/BUILD | 2 +- .../network/filter_manager_impl_test.cc | 20 +++++++--------- .../filters/network/ratelimit/BUILD | 2 +- .../filters/network/ratelimit/config_test.cc | 24 +++++++++++++++---- .../network/ratelimit/ratelimit_test.cc | 19 --------------- 8 files changed, 31 insertions(+), 68 deletions(-) diff --git a/source/common/config/BUILD b/source/common/config/BUILD index ffb1ed7f18b9..36338af90b20 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -163,7 +163,6 @@ envoy_cc_library( "@envoy_api//envoy/config/filter/network/client_ssl_auth/v2:client_ssl_auth_cc", "@envoy_api//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager_cc", "@envoy_api//envoy/config/filter/network/mongo_proxy/v2:mongo_proxy_cc", - "@envoy_api//envoy/config/filter/network/rate_limit/v2:rate_limit_cc", "@envoy_api//envoy/config/filter/network/redis_proxy/v2:redis_proxy_cc", "@envoy_api//envoy/config/filter/network/tcp_proxy/v2:tcp_proxy_cc", ], diff --git a/source/common/config/filter_json.cc b/source/common/config/filter_json.cc index f46cf38275cb..87b19780c9a1 100644 --- a/source/common/config/filter_json.cc +++ b/source/common/config/filter_json.cc @@ -397,26 +397,6 @@ void FilterJson::translateTcpProxy( } } -void FilterJson::translateTcpRateLimitFilter( - const Json::Object& json_config, - envoy::config::filter::network::rate_limit::v2::RateLimit& proto_config) { - json_config.validateSchema(Json::Schema::RATELIMIT_NETWORK_FILTER_SCHEMA); - - JSON_UTIL_SET_STRING(json_config, proto_config, stat_prefix); - JSON_UTIL_SET_STRING(json_config, proto_config, domain); - JSON_UTIL_SET_DURATION(json_config, proto_config, timeout); - - auto* descriptors = proto_config.mutable_descriptors(); - for (const auto& json_descriptor : json_config.getObjectArray("descriptors", false)) { - auto* entries = descriptors->Add()->mutable_entries(); - for (const auto& json_entry : json_descriptor->asObjectArray()) { - auto* entry = entries->Add(); - JSON_UTIL_SET_STRING(*json_entry, *entry, key); - JSON_UTIL_SET_STRING(*json_entry, *entry, value); - } - } -} - void FilterJson::translateHttpRateLimitFilter( const Json::Object& json_config, envoy::config::filter::http::rate_limit::v2::RateLimit& proto_config) { diff --git a/source/common/config/filter_json.h b/source/common/config/filter_json.h index 73ca0f7085dc..aaebf49b4826 100644 --- a/source/common/config/filter_json.h +++ b/source/common/config/filter_json.h @@ -11,7 +11,6 @@ #include "envoy/config/filter/network/client_ssl_auth/v2/client_ssl_auth.pb.h" #include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" #include "envoy/config/filter/network/mongo_proxy/v2/mongo_proxy.pb.h" -#include "envoy/config/filter/network/rate_limit/v2/rate_limit.pb.h" #include "envoy/config/filter/network/redis_proxy/v2/redis_proxy.pb.h" #include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.h" #include "envoy/json/json_object.h" @@ -141,16 +140,6 @@ class FilterJson { translateTcpProxy(const Json::Object& json_config, envoy::config::filter::network::tcp_proxy::v2::TcpProxy& proto_config); - /** - * Translate a v1 JSON TCP Rate Limit filter object to v2 - * envoy::config::filter::network::rate_limit::v2::RateLimit. - * @param json_config source v1 JSON Tcp Rate Limit Filter object. - * @param proto_config destination v2 envoy::config::filter::network::rate_limit::v2::RateLimit. - */ - static void translateTcpRateLimitFilter( - const Json::Object& json_config, - envoy::config::filter::network::rate_limit::v2::RateLimit& proto_config); - /** * Translate a v1 JSON HTTP Rate Limit filter object to v2 * envoy::config::filter::http::rate_limit::v2::RateLimit. diff --git a/test/common/network/BUILD b/test/common/network/BUILD index e70e4aa230c9..e28b3f6a5919 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -112,7 +112,6 @@ envoy_cc_test( srcs = ["filter_manager_impl_test.cc"], deps = [ "//source/common/buffer:buffer_lib", - "//source/common/config:filter_json_lib", "//source/common/event:dispatcher_lib", "//source/common/network:filter_manager_lib", "//source/common/stats:stats_lib", @@ -130,6 +129,7 @@ envoy_cc_test( "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:host_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:utility_lib", ], ) diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index 4ff4e17ffc02..0e8d2cd438f6 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -2,7 +2,6 @@ #include #include "common/buffer/buffer_impl.h" -#include "common/config/filter_json.h" #include "common/network/filter_manager_impl.h" #include "common/tcp_proxy/tcp_proxy.h" #include "common/upstream/upstream_impl.h" @@ -20,6 +19,7 @@ #include "test/mocks/upstream/host.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/printers.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -164,14 +164,13 @@ TEST_F(NetworkFilterManagerTest, RateLimitAndTcpProxy) { NiceMock conn_pool; FilterManagerImpl manager(connection_); - std::string rl_json = R"EOF( - { - "domain": "foo", - "descriptors": [ - [{"key": "hello", "value": "world"}] - ], - "stat_prefix": "name" - } + std::string rl_yaml = R"EOF( +domain: foo +descriptors: +- entries: + - key: hello + value: world +stat_prefix: name )EOF"; ON_CALL(factory_context.runtime_loader_.snapshot_, @@ -181,9 +180,8 @@ TEST_F(NetworkFilterManagerTest, RateLimitAndTcpProxy) { featureEnabled("ratelimit.tcp_filter_enforcing", 100)) .WillByDefault(Return(true)); - Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(rl_json); envoy::config::filter::network::rate_limit::v2::RateLimit proto_config{}; - Config::FilterJson::translateTcpRateLimitFilter(*json_config, proto_config); + TestUtility::loadFromYaml(rl_yaml, proto_config); Extensions::NetworkFilters::RateLimitFilter::ConfigSharedPtr rl_config( new Extensions::NetworkFilters::RateLimitFilter::Config(proto_config, factory_context.scope_, diff --git a/test/extensions/filters/network/ratelimit/BUILD b/test/extensions/filters/network/ratelimit/BUILD index 226e8e847423..a79472354228 100644 --- a/test/extensions/filters/network/ratelimit/BUILD +++ b/test/extensions/filters/network/ratelimit/BUILD @@ -17,7 +17,6 @@ envoy_extension_cc_test( extension_name = "envoy.filters.network.ratelimit", deps = [ "//source/common/buffer:buffer_lib", - "//source/common/config:filter_json_lib", "//source/common/event:dispatcher_lib", "//source/common/stats:stats_lib", "//source/extensions/filters/network/ratelimit:ratelimit_lib", @@ -36,5 +35,6 @@ envoy_extension_cc_test( deps = [ "//source/extensions/filters/network/ratelimit:config", "//test/mocks/server:server_mocks", + "//test/test_common:utility_lib", ], ) diff --git a/test/extensions/filters/network/ratelimit/config_test.cc b/test/extensions/filters/network/ratelimit/config_test.cc index 5fdf847dfb7b..387a03b216d4 100644 --- a/test/extensions/filters/network/ratelimit/config_test.cc +++ b/test/extensions/filters/network/ratelimit/config_test.cc @@ -1,10 +1,10 @@ +#include "envoy/config/filter/network/rate_limit/v2/rate_limit.pb.h" #include "envoy/config/filter/network/rate_limit/v2/rate_limit.pb.validate.h" -#include "common/config/filter_json.h" - #include "extensions/filters/network/ratelimit/config.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -24,7 +24,7 @@ TEST(RateLimitFilterConfigTest, ValidateFail) { ProtoValidationException); } -TEST(RateLimitFilterConfigTest, RatelimitCorrectProto) { +TEST(RateLimitFilterConfigTest, CorrectProto) { const std::string yaml = R"EOF( stat_prefix: my_stat_prefix domain: fake_domain @@ -56,7 +56,7 @@ TEST(RateLimitFilterConfigTest, RatelimitCorrectProto) { cb(connection); } -TEST(RateLimitFilterConfigTest, RateLimitFilterEmptyProto) { +TEST(RateLimitFilterConfigTest, EmptyProto) { NiceMock context; NiceMock instance; RateLimitConfigFactory factory; @@ -67,6 +67,22 @@ TEST(RateLimitFilterConfigTest, RateLimitFilterEmptyProto) { EXPECT_THROW(factory.createFilterFactoryFromProto(empty_proto_config, context), EnvoyException); } +TEST(RateLimitFilterConfigTest, IncorrectProto) { + std::string yaml_string = R"EOF( +stat_prefix: my_stat_prefix +domain: fake_domain +descriptors: +- entries: + - key: my_key + value: my_value +ip_white_list: '12' + )EOF"; + + envoy::config::filter::network::rate_limit::v2::RateLimit proto_config; + EXPECT_THROW_WITH_REGEX(TestUtility::loadFromYaml(yaml_string, proto_config), EnvoyException, + "ip_white_list: Cannot find field"); +} + } // namespace RateLimitFilter } // namespace NetworkFilters } // namespace Extensions diff --git a/test/extensions/filters/network/ratelimit/ratelimit_test.cc b/test/extensions/filters/network/ratelimit/ratelimit_test.cc index 323fadb89d2e..1ff47f4dbe6c 100644 --- a/test/extensions/filters/network/ratelimit/ratelimit_test.cc +++ b/test/extensions/filters/network/ratelimit/ratelimit_test.cc @@ -5,8 +5,6 @@ #include "envoy/stats/stats.h" #include "common/buffer/buffer_impl.h" -#include "common/config/filter_json.h" -#include "common/json/json_loader.h" #include "extensions/filters/network/ratelimit/ratelimit.h" @@ -96,23 +94,6 @@ failure_mode_deny: true Filters::Common::RateLimit::RequestCallbacks* request_callbacks_{}; }; -TEST_F(RateLimitFilterTest, BadRatelimitConfig) { - std::string json_string = R"EOF( - { - "stat_prefix": "my_stat_prefix", - "domain" : "fake_domain", - "descriptors": [[{ "key" : "my_key", "value" : "my_value" }]], - "ip_white_list": "12" - } - )EOF"; - - Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); - envoy::config::filter::network::rate_limit::v2::RateLimit proto_config{}; - - EXPECT_THROW(Envoy::Config::FilterJson::translateTcpRateLimitFilter(*json_config, proto_config), - Json::Exception); -} - TEST_F(RateLimitFilterTest, OK) { InSequence s; SetUpTest(filter_config_); From e0c92845b71e7bb52a6be56727683932c013fa48 Mon Sep 17 00:00:00 2001 From: Laurent Le Brun Date: Wed, 5 Jun 2019 01:40:14 +0200 Subject: [PATCH 11/13] Update dependency on rules_foreign_cc (#7168) With this update, Envoy is compatible with the upcoming Bazel 0.27. Signed-off-by: Laurent Le Brun --- bazel/repository_locations.bzl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index aa55df22d733..c772767cfd72 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -221,10 +221,10 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/bazelbuild/rules_go/releases/download/0.18.5/rules_go-0.18.5.tar.gz"], ), rules_foreign_cc = dict( - sha256 = "ccffb49fb24aee3f0a005fa59810dc596228fe86eacacbe0c004d59ed1881bd8", - strip_prefix = "rules_foreign_cc-bf99a0bf0080bcd50431aa7124ef23e5afd58325", - # 2019-05-17 - urls = ["https://github.com/bazelbuild/rules_foreign_cc/archive/bf99a0bf0080bcd50431aa7124ef23e5afd58325.tar.gz"], + sha256 = "980c1b74f5c18ea099889b0fb0479ee34b8a02845d3d302ecb16b15d73d624c8", + strip_prefix = "rules_foreign_cc-a0dc109915cea85909bef586e2b2a9bbdc6c8ff5", + # 2019-06-04 + urls = ["https://github.com/bazelbuild/rules_foreign_cc/archive/a0dc109915cea85909bef586e2b2a9bbdc6c8ff5.tar.gz"], ), six_archive = dict( sha256 = "105f8d68616f8248e24bf0e9372ef04d3cc10104f1980f54d57b2ce73a5ad56a", From c167c65b8b93a3bcd92ba24c50c6f175b053f06e Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 4 Jun 2019 18:54:55 -0700 Subject: [PATCH 12/13] tls: update BoringSSL to e534d74f (3770). (#7169) Signed-off-by: Piotr Sikora --- bazel/repository_locations.bzl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index c772767cfd72..7d8a49c20a53 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -5,10 +5,10 @@ REPOSITORY_LOCATIONS = dict( ), boringssl = dict( # Use commits from branch "chromium-stable-with-bazel" - sha256 = "4825306f702fa5cb76fd86c987a88c9bbb241e75f4d86dbb3714530ca73c1fb1", - strip_prefix = "boringssl-8cb07520451f0dc454654f2da5cdecf0b806f823", - # chromium-74.0.3729.131 - urls = ["https://github.com/google/boringssl/archive/8cb07520451f0dc454654f2da5cdecf0b806f823.tar.gz"], + sha256 = "448773376d063b1e9a19e4fd41002d1a31a968a13be20b3b66ecd4aab9cf14a8", + strip_prefix = "boringssl-e534d74f5732e1aeebd514f05271d089c530c2f9", + # chromium-75.0.3770.80 + urls = ["https://github.com/google/boringssl/archive/e534d74f5732e1aeebd514f05271d089c530c2f9.tar.gz"], ), boringssl_fips = dict( sha256 = "b12ad676ee533824f698741bd127f6fbc82c46344398a6d78d25e62c6c418c73", From 094496fdfde03f784ccaff776fda0344b051d75c Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Wed, 5 Jun 2019 21:03:33 +0530 Subject: [PATCH 13/13] downgrade dns log level (#7176) Signed-off-by: Rama Chavali --- source/common/network/dns_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 810dd19429d5..3702210c50ce 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -148,7 +148,7 @@ void DnsResolverImpl::updateAresTimer() { if (timeout_result != nullptr) { const auto ms = std::chrono::milliseconds(timeout_result->tv_sec * 1000 + timeout_result->tv_usec / 1000); - ENVOY_LOG(debug, "Setting DNS resolution timer for {} milliseconds", ms.count()); + ENVOY_LOG(trace, "Setting DNS resolution timer for {} milliseconds", ms.count()); timer_->enableTimer(ms); } else { timer_->disableTimer();