From f66b55a837a8662026d8c571509351d868a65d35 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 22 Jan 2019 11:15:07 -0800 Subject: [PATCH 01/27] Base implementation of VHDS Signed-off-by: Dmitri Dolguikh --- include/envoy/router/router.h | 2 + source/common/config/resources.h | 1 + source/common/http/async_client_impl.h | 1 + source/common/router/config_impl.cc | 2 +- source/common/router/config_impl.h | 5 +- source/common/router/rds_impl.cc | 145 +++++++++-- source/common/router/rds_impl.h | 101 +++++++- test/common/router/rds_impl_test.cc | 53 ++++ test/integration/BUILD | 21 ++ test/integration/http_integration.cc | 10 +- test/integration/http_integration.h | 6 +- test/integration/integration.cc | 4 +- test/integration/integration.h | 19 +- test/integration/vhds_integration_test.cc | 298 ++++++++++++++++++++++ test/mocks/router/mocks.cc | 1 + test/mocks/router/mocks.h | 1 + 16 files changed, 632 insertions(+), 38 deletions(-) create mode 100644 test/integration/vhds_integration_test.cc diff --git a/include/envoy/router/router.h b/include/envoy/router/router.h index e090c9caadc6..c257179a74e6 100644 --- a/include/envoy/router/router.h +++ b/include/envoy/router/router.h @@ -786,6 +786,8 @@ class Config { * @return const std::string the RouteConfiguration name. */ virtual const std::string& name() const PURE; + + virtual bool usesVhds() const PURE; }; typedef std::shared_ptr ConfigConstSharedPtr; diff --git a/source/common/config/resources.h b/source/common/config/resources.h index 69ed2d91a46d..9f35373b511e 100644 --- a/source/common/config/resources.h +++ b/source/common/config/resources.h @@ -17,6 +17,7 @@ class TypeUrlValues { const std::string ClusterLoadAssignment{"type.googleapis.com/envoy.api.v2.ClusterLoadAssignment"}; const std::string Secret{"type.googleapis.com/envoy.api.v2.auth.Secret"}; const std::string RouteConfiguration{"type.googleapis.com/envoy.api.v2.RouteConfiguration"}; + const std::string VirtualHost{"type.googleapis.com/envoy.api.v2.route.VirtualHost"}; }; typedef ConstSingleton TypeUrl; diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 35fffefc2c47..53433106d9b4 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -172,6 +172,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, } const std::string& name() const override { return EMPTY_STRING; } + bool usesVhds() const override { return false; } static const std::list internal_only_headers_; }; diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 3cfdbc63f596..83747a3b8aed 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1086,7 +1086,7 @@ VirtualHostImpl::virtualClusterFromEntries(const Http::HeaderMap& headers) const ConfigImpl::ConfigImpl(const envoy::api::v2::RouteConfiguration& config, Server::Configuration::FactoryContext& factory_context, bool validate_clusters_default) - : name_(config.name()) { + : name_(config.name()), uses_vhds_(config.has_vhds()) { route_matcher_ = std::make_unique( config, *this, factory_context, PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, validate_clusters, validate_clusters_default)); diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index c9fdeb40403e..d98b193774e2 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -768,12 +768,15 @@ class ConfigImpl : public Config { const std::string& name() const override { return name_; } + bool usesVhds() const override { return uses_vhds_; } + private: std::unique_ptr route_matcher_; std::list internal_only_headers_; HeaderParserPtr request_headers_parser_; HeaderParserPtr response_headers_parser_; const std::string name_; + const bool uses_vhds_; }; /** @@ -789,7 +792,7 @@ class NullConfigImpl : public Config { } const std::string& name() const override { return name_; } - + bool usesVhds() const override { return false; } private: std::list internal_only_headers_; const std::string name_; diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 2aa821ad838e..fb16720585a5 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -12,7 +12,6 @@ #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/config/rds_json.h" -#include "common/config/subscription_factory.h" #include "common/config/utility.h" #include "common/protobuf/utility.h" #include "common/router/config_impl.h" @@ -53,20 +52,19 @@ StaticRouteConfigProviderImpl::~StaticRouteConfigProviderImpl() { } // TODO(htuch): If support for multiple clusters is added per #1170 cluster_name_ -// initialization needs to be fixed. RdsRouteConfigSubscription::RdsRouteConfigSubscription( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, Envoy::Router::RouteConfigProviderManagerImpl& route_config_provider_manager) - : route_config_name_(rds.route_config_name()), + : factory_context_(factory_context), route_config_name_(rds.route_config_name()), init_target_(fmt::format("RdsRouteConfigSubscription {}", route_config_name_), [this]() { subscription_->start({route_config_name_}, *this); }), scope_(factory_context.scope().createScope(stat_prefix + "rds." + route_config_name_ + ".")), - stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}), + stat_prefix_(stat_prefix), stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}), route_config_provider_manager_(route_config_provider_manager), manager_identifier_(manager_identifier), time_source_(factory_context.timeSource()), - last_updated_(factory_context.timeSource().systemTime()) { + last_updated_(factory_context.timeSource().systemTime()), uses_vhds_(false) { Envoy::Config::Utility::checkLocalInfo("rds", factory_context.localInfo()); subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( @@ -116,10 +114,19 @@ void RdsRouteConfigSubscription::onConfigUpdate( config_info_ = {new_hash, version_info}; route_config_proto_ = route_config; stats_.config_reload_.inc(); - ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, - new_hash); - for (auto* provider : route_config_providers_) { - provider->onConfigUpdate(); + + uses_vhds_ = route_config_proto_.has_vhds(); + if (!uses_vhds_) { + ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, + new_hash); + for (auto* provider : route_config_providers_) { + provider->onConfigUpdate(); + } + vhds_subscription_.release(); + } else { + auto s = new VhdsSubscription(route_config_proto_, factory_context_, stat_prefix_, this); + s->registerInitTargetWithInitManager(factory_context_.initManager()); + vhds_subscription_.reset(s); } } @@ -132,15 +139,27 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { init_target_.ready(); } +absl::optional RdsRouteConfigSubscription::configInfo() const { + return (uses_vhds_ ? vhds_subscription_->configInfo() : config_info_); +} + +envoy::api::v2::RouteConfiguration& RdsRouteConfigSubscription::routeConfiguration() { + return (uses_vhds_ ? vhds_subscription_->routeConfiguration() : route_config_proto_); +} + +SystemTime RdsRouteConfigSubscription::lastUpdated() const { + return (uses_vhds_ ? vhds_subscription_->lastUpdated() : last_updated_); +} + RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( RdsRouteConfigSubscriptionSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context) : subscription_(std::move(subscription)), factory_context_(factory_context), tls_(factory_context.threadLocal().allocateSlot()) { ConfigConstSharedPtr initial_config; - if (subscription_->config_info_.has_value()) { + if (subscription_->configInfo().has_value()) { initial_config = - std::make_shared(subscription_->route_config_proto_, factory_context_, false); + std::make_shared(subscription_->routeConfiguration(), factory_context_, false); } else { initial_config = std::make_shared(); } @@ -159,21 +178,117 @@ Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() { } absl::optional RdsRouteConfigProviderImpl::configInfo() const { - if (!subscription_->config_info_) { + if (!subscription_->configInfo()) { return {}; } else { - return ConfigInfo{subscription_->route_config_proto_, - subscription_->config_info_.value().last_config_version_}; + return ConfigInfo{subscription_->routeConfiguration(), + subscription_->configInfo().value().last_config_version_}; } } void RdsRouteConfigProviderImpl::onConfigUpdate() { ConfigConstSharedPtr new_config( - new ConfigImpl(subscription_->route_config_proto_, factory_context_, false)); + new ConfigImpl(subscription_->routeConfiguration(), factory_context_, false)); tls_->runOnAllThreads( [this, new_config]() -> void { tls_->getTyped().config_ = new_config; }); } +VhdsSubscription::VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, + RdsRouteConfigSubscription* rds_subscription, + SubscriptionFactoryFunction factory_function) + : route_config_proto_(route_configuration), route_config_name_(route_configuration.name()), + init_target_(fmt::format("VhdsConfigSubscription {}", route_config_name_), + [this]() { subscription_->start({}, *this); }), + scope_(factory_context.scope().createScope(stat_prefix + "vhds." + route_config_name_ + ".")), + stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), time_source_(factory_context.timeSource()), + last_updated_(factory_context.timeSource().systemTime()), + rds_subscription_(rds_subscription) { + Envoy::Config::Utility::checkLocalInfo("vhds", factory_context.localInfo()); + const auto& config_source = + route_configuration.vhds().config_source().api_config_source().api_type(); + if (config_source != envoy::api::v2::core::ApiConfigSource::DELTA_GRPC) { + throw EnvoyException("vhds: only 'DELTA_GRPC' is supported as an api_type."); + } + + initializeVhosts(route_config_proto_); + subscription_ = factory_function( + route_configuration.vhds().config_source(), factory_context.localInfo(), + 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.api()); +} + +void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { + // We need to allow server startup to continue, even if we have a bad + // config. + init_target_.ready(); +} + +void VhdsSubscription::onConfigUpdate( + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& version_info) { + last_updated_ = time_source_.systemTime(); + removeVhosts(virtual_hosts_, removed_resources); + updateVhosts(virtual_hosts_, added_resources); + rebuildRouteConfig(virtual_hosts_, route_config_proto_); + + const uint64_t new_hash = MessageUtil::hash(route_config_proto_); + if (!config_info_ || new_hash != config_info_.value().last_config_hash_) { + config_info_ = {new_hash, version_info}; + stats_.config_reload_.inc(); + ENVOY_LOG(debug, "vhds: loading new configuration: config_name={} hash={}", route_config_name_, + new_hash); + for (auto* provider : rds_subscription_->route_config_providers()) { + provider->onConfigUpdate(); + } + } + + init_target_.ready(); +} + +void VhdsSubscription::initializeVhosts( + const envoy::api::v2::RouteConfiguration& route_configuration) { + if (route_configuration.virtual_hosts_size() <= 0) { + return; + } + for (const auto& vhost : route_configuration.virtual_hosts()) { + virtual_hosts_.emplace(vhost.name(), vhost); + } +} + +void VhdsSubscription::removeVhosts( + std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& removed_vhost_names) { + for (const auto vhost_name : removed_vhost_names) { + vhosts.erase(vhost_name); + } +} + +void VhdsSubscription::updateVhosts( + std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& added_resources) { + // TODO validate aaded_resources + for (const auto& resource : added_resources) { + envoy::api::v2::route::VirtualHost vhost = + MessageUtil::anyConvert(resource.resource()); + vhosts.emplace(vhost.name(), vhost); + } +} + +void VhdsSubscription::rebuildRouteConfig( + const std::unordered_map& vhosts, + envoy::api::v2::RouteConfiguration& route_config) { + + route_config.clear_virtual_hosts(); + for (const auto& vhost : vhosts) { + route_config.mutable_virtual_hosts()->Add()->CopyFrom(vhost.second); + } +} + RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& admin) { config_tracker_entry_ = admin.getConfigTracker().add("routes", [this] { return dumpRouteConfigs(); }); diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index a435e564bc5c..7145199f147a 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -22,6 +22,7 @@ #include "envoy/thread_local/thread_local.h" #include "common/common/logger.h" +#include "common/config/subscription_factory.h" #include "common/init/target_impl.h" #include "common/protobuf/utility.h" @@ -87,7 +88,25 @@ struct RdsStats { ALL_RDS_STATS(GENERATE_COUNTER_STRUCT) }; +// clang-format off +#define ALL_VHDS_STATS(COUNTER) \ + COUNTER(config_reload) \ + COUNTER(update_empty) + +// clang-format on + +struct VhdsStats { + ALL_VHDS_STATS(GENERATE_COUNTER_STRUCT) +}; + +struct LastConfigInfo { + uint64_t last_config_hash_; + std::string last_config_version_; +}; + class RdsRouteConfigProviderImpl; +class VhdsSubscription; +typedef std::unique_ptr VhdsSubscriptionPtr; /** * A class that fetches the route configuration dynamically using the RDS API and updates them to @@ -110,13 +129,14 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource).name(); } + std::unordered_set& route_config_providers() { + return route_config_providers_; + } + absl::optional configInfo() const; + envoy::api::v2::RouteConfiguration& routeConfiguration(); + SystemTime lastUpdated() const; private: - struct LastConfigInfo { - uint64_t last_config_hash_; - std::string last_config_version_; - }; - RdsRouteConfigSubscription( const envoy::config::filter::network::http_connection_manager::v2::Rds& rds, const uint64_t manager_identifier, Server::Configuration::FactoryContext& factory_context, @@ -124,9 +144,11 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, RouteConfigProviderManagerImpl& route_config_provider_manager); std::unique_ptr subscription_; + Server::Configuration::FactoryContext& factory_context_; const std::string route_config_name_; Init::TargetImpl init_target_; Stats::ScopePtr scope_; + std::string stat_prefix_; RdsStats stats_; RouteConfigProviderManagerImpl& route_config_provider_manager_; const uint64_t manager_identifier_; @@ -135,12 +157,72 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, absl::optional config_info_; envoy::api::v2::RouteConfiguration route_config_proto_; std::unordered_set route_config_providers_; + VhdsSubscriptionPtr vhds_subscription_; + bool uses_vhds_; friend class RouteConfigProviderManagerImpl; friend class RdsRouteConfigProviderImpl; }; typedef std::shared_ptr RdsRouteConfigSubscriptionSharedPtr; +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, Api::Api&); + +class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, + Logger::Loggable { +public: + VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, RdsRouteConfigSubscription* rds_subscription, + SubscriptionFactoryFunction factory_function = + Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); + ~VhdsSubscription() { init_target_.ready(); } + + // Config::SubscriptionCallbacks + // TODO(fredlas) deduplicate + void onConfigUpdate(const Protobuf::RepeatedPtrField&, + const std::string&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void onConfigUpdate(const Protobuf::RepeatedPtrField&, + const Protobuf::RepeatedPtrField&, const std::string&) override; + void onConfigUpdateFailed(const EnvoyException* e) override; + std::string resourceName(const ProtobufWkt::Any& resource) override { + return MessageUtil::anyConvert(resource).name(); + } + + void registerInitTargetWithInitManager(Init::Manager& m) { m.add(init_target_); } + void initializeVhosts(const envoy::api::v2::RouteConfiguration& route_configuration); + void removeVhosts(std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& removed_vhost_names); + void updateVhosts(std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& added_resources); + void rebuildRouteConfig( + const std::unordered_map& vhosts, + envoy::api::v2::RouteConfiguration& route_config); + absl::optional configInfo() const { return config_info_; } + envoy::api::v2::RouteConfiguration& routeConfiguration() { return route_config_proto_; } + SystemTime lastUpdated() const { return last_updated_; } + + std::unique_ptr subscription_; + envoy::api::v2::RouteConfiguration route_config_proto_; + const std::string route_config_name_; + Init::TargetImpl init_target_; + Stats::ScopePtr scope_; + VhdsStats stats_; + TimeSource& time_source_; + SystemTime last_updated_; + RdsRouteConfigSubscription* rds_subscription_; + std::unordered_map virtual_hosts_; + absl::optional config_info_; +}; + +struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { + ThreadLocalConfig(ConfigConstSharedPtr initial_config) : config_(initial_config) {} + ConfigConstSharedPtr config_; +}; /** * Implementation of RouteConfigProvider that fetches the route configuration dynamically using @@ -157,20 +239,15 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; absl::optional configInfo() const override; - SystemTime lastUpdated() const override { return subscription_->last_updated_; } + SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } private: - struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { - ThreadLocalConfig(ConfigConstSharedPtr initial_config) : config_(initial_config) {} - - ConfigConstSharedPtr config_; - }; - RdsRouteConfigProviderImpl(RdsRouteConfigSubscriptionSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context); RdsRouteConfigSubscriptionSharedPtr subscription_; Server::Configuration::FactoryContext& factory_context_; + SystemTime last_updated_; ThreadLocal::SlotPtr tls_; friend class RouteConfigProviderManagerImpl; diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index c73ea59e0190..362419448766 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -375,6 +375,59 @@ TEST_F(RdsImplTest, Failure) { factory_context_.scope_.counter("foo.rds.foo_route_config.update_rejected").value()); } +TEST_F(RdsImplTest, VhdsInstantiationShouldSucceedWithDELTA_GRPC) { + void* subscription; + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + const std::string context("vhds_test"); + + Envoy::Router::SubscriptionFactoryFunction 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, + Api::Api&) -> std::unique_ptr { + return std::unique_ptr(); + }}; + EXPECT_NO_THROW(VhdsSubscription(route_config, factory_context_, context, + static_cast(subscription), + factory_function)); +} + +TEST_F(RdsImplTest, VhdsInstantiationShouldFailWithoutDELTA_GRPC) { + void* subscription; + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +vhds: + config_source: + api_config_source: + api_type: GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + const std::string context("vhds_test"); + + Envoy::Router::SubscriptionFactoryFunction 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, + Api::Api&) -> std::unique_ptr { + return std::unique_ptr(); + }}; + EXPECT_THROW(VhdsSubscription(route_config, factory_context_, context, + static_cast(subscription), + factory_function), + EnvoyException); +} + class RouteConfigProviderManagerImplTest : public RdsTestBase { public: void setup() { diff --git a/test/integration/BUILD b/test/integration/BUILD index f7103c90ee9b..fbd512635fef 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -109,6 +109,27 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "vhds_integration_test", + srcs = ["vhds_integration_test.cc"], + data = [ + "//test/config/integration/certs", + ], + deps = [ + ":http_integration_lib", + "//source/common/config:protobuf_link_hacks", + "//source/common/config:resources_lib", + "//source/common/protobuf:utility_lib", + "//test/common/grpc:grpc_client_integration_lib", + "//test/mocks/runtime:runtime_mocks", + "//test/mocks/server:server_mocks", + "//test/test_common:network_utility_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/api/v2:rds_cc", + "@envoy_api//envoy/api/v2:discovery_cc", + ], +) + exports_files(["test_utility.sh"]) envoy_sh_test( diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index d2d6e4d33fc7..eaef14da5de0 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -345,7 +345,8 @@ void HttpIntegrationTest::testRouterRequestAndResponseWithBody( IntegrationStreamDecoderPtr HttpIntegrationTest::makeHeaderOnlyRequest(ConnectionCreationFunction* create_connection, - int upstream_index, const std::string& path) { + int upstream_index, const std::string& path, + const std::string& authority) { // This is called multiple times per test in ads_integration_test. Only call // initialize() the first time. if (!initialized()) { @@ -356,15 +357,16 @@ HttpIntegrationTest::makeHeaderOnlyRequest(ConnectionCreationFunction* create_co Http::TestHeaderMapImpl request_headers{{":method", "GET"}, {":path", path}, {":scheme", "http"}, - {":authority", "host"}, + {":authority", authority}, {"x-lyft-user-id", "123"}}; return sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 0, upstream_index); } void HttpIntegrationTest::testRouterHeaderOnlyRequestAndResponse( - ConnectionCreationFunction* create_connection, int upstream_index, const std::string& path) { - auto response = makeHeaderOnlyRequest(create_connection, upstream_index, path); + ConnectionCreationFunction* create_connection, int upstream_index, const std::string& path, + const std::string& authority) { + auto response = makeHeaderOnlyRequest(create_connection, upstream_index, path, authority); checkSimpleRequestSuccess(0U, 0U, response.get()); } diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 3fb328ac2be5..f8051e9d67b5 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -140,7 +140,8 @@ class HttpIntegrationTest : public BaseIntegrationTest { // Sends a simple header-only HTTP request, and waits for a response. IntegrationStreamDecoderPtr makeHeaderOnlyRequest(ConnectionCreationFunction* create_connection, int upstream_index, - const std::string& path = "/test/long/url"); + const std::string& path = "/test/long/url", + const std::string& authority = "host"); void testRouterNotFound(); void testRouterNotFoundWithBody(); @@ -149,7 +150,8 @@ class HttpIntegrationTest : public BaseIntegrationTest { ConnectionCreationFunction* creator = nullptr); void testRouterHeaderOnlyRequestAndResponse(ConnectionCreationFunction* creator = nullptr, int upstream_index = 0, - const std::string& path = "/test/long/url"); + const std::string& path = "/test/long/url", + const std::string& authority = "host"); void testRequestAndResponseShutdownWithActiveConnection(); // Disconnect tests diff --git a/test/integration/integration.cc b/test/integration/integration.cc index e38b97c209ee..5fd8f8e4c0e5 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -529,10 +529,10 @@ AssertionResult BaseIntegrationTest::compareDiscoveryRequest( AssertionResult BaseIntegrationTest::compareDeltaDiscoveryRequest( const std::string& expected_type_url, const std::vector& expected_resource_subscriptions, - const std::vector& expected_resource_unsubscriptions, + const std::vector& expected_resource_unsubscriptions, FakeStreamPtr& xds_stream, const Protobuf::int32 expected_error_code, const std::string& expected_error_message) { envoy::api::v2::DeltaDiscoveryRequest request; - VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, request)); + VERIFY_ASSERTION(xds_stream->waitForGrpcMessage(*dispatcher_, request)); EXPECT_TRUE(request.has_node()); EXPECT_FALSE(request.node().id().empty()); diff --git a/test/integration/integration.h b/test/integration/integration.h index cb7e8c937b7f..b1e2d3183462 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -221,11 +221,28 @@ class BaseIntegrationTest : Logger::Loggable { const std::vector& expected_resource_subscriptions, const std::vector& expected_resource_unsubscriptions, const Protobuf::int32 expected_error_code = Grpc::Status::GrpcStatus::Ok, + const std::string& expected_error_message = "") { + return compareDeltaDiscoveryRequest(expected_type_url, expected_resource_subscriptions, + expected_resource_unsubscriptions, xds_stream_, + expected_error_code, expected_error_message); + } + + AssertionResult compareDeltaDiscoveryRequest( + const std::string& expected_type_url, + const std::vector& expected_resource_subscriptions, + const std::vector& expected_resource_unsubscriptions, FakeStreamPtr& stream, + const Protobuf::int32 expected_error_code = Grpc::Status::GrpcStatus::Ok, const std::string& expected_error_message = ""); template void sendDeltaDiscoveryResponse(const std::vector& added_or_updated, const std::vector& removed, const std::string& version) { + sendDeltaDiscoveryResponse(added_or_updated, removed, version, xds_stream_); + } + template + void sendDeltaDiscoveryResponse(const std::vector& added_or_updated, + const std::vector& removed, + const std::string& version, FakeStreamPtr& stream) { envoy::api::v2::DeltaDiscoveryResponse response; response.set_system_version_info("system_version_info_this_is_a_test"); for (const auto& message : added_or_updated) { @@ -236,7 +253,7 @@ class BaseIntegrationTest : Logger::Loggable { } *response.mutable_removed_resources() = {removed.begin(), removed.end()}; response.set_nonce("noncense"); - xds_stream_->sendGrpcMessage(response); + stream->sendGrpcMessage(response); } private: diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc new file mode 100644 index 000000000000..91e88f97caf2 --- /dev/null +++ b/test/integration/vhds_integration_test.cc @@ -0,0 +1,298 @@ +#include "envoy/api/v2/discovery.pb.h" +#include "envoy/api/v2/rds.pb.h" +#include "envoy/grpc/status.h" +#include "envoy/stats/scope.h" + +#include "common/config/protobuf_link_hacks.h" +#include "common/config/resources.h" +#include "common/protobuf/protobuf.h" +#include "common/protobuf/utility.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/integration/http_integration.h" +#include "test/integration/utility.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "absl/synchronization/notification.h" +#include "gtest/gtest.h" + +using testing::AssertionFailure; +using testing::AssertionResult; +using testing::AssertionSuccess; +using testing::IsSubstring; + +namespace Envoy { +namespace { + +const char Config[] = R"EOF( +admin: + access_log_path: /dev/null + address: + socket_address: + address: 127.0.0.1 + port_value: 0 +static_resources: + clusters: + - name: xds_cluster + type: STATIC + http2_protocol_options: {} + hosts: + socket_address: + address: 127.0.0.1 + port_value: 0 + - name: my_service + type: STATIC + http2_protocol_options: {} + hosts: + socket_address: + address: 127.0.0.1 + port_value: 0 + listeners: + - name: http + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + filter_chains: + - filters: + - name: envoy.http_connection_manager + config: + stat_prefix: config_test + http_filters: + - name: envoy.router + codec_type: HTTP2 + rds: + route_config_name: my_route + config_source: + api_config_source: + api_type: GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster +)EOF"; + +const char RdsConfig[] = R"EOF( +name: my_route +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster +)EOF"; + +const char RdsConfigWithVhosts[] = R"EOF( +name: my_route +virtual_hosts: +- name: vhost_rds1 + domains: ["vhost.rds.first"] + routes: + - match: { prefix: "/rdsone" } + route: { cluster: my_service } +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster +)EOF"; + +class VhdsIntegrationTest : public HttpIntegrationTest, + public Grpc::GrpcClientIntegrationParamTest { +public: + VhdsIntegrationTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, ipVersion(), realTime(), Config) {} + + void TearDown() override { + cleanUpXdsConnection(); + test_server_.reset(); + fake_upstreams_.clear(); + } + + std::string virtualHostYaml(std::string name, std::string domain) { + return fmt::format(R"EOF( + name: {} + domains: [{}] + routes: + - match: {{ prefix: "/" }} + route: {{ cluster: "my_service" }} + )EOF", + name, domain); + } + + envoy::api::v2::route::VirtualHost buildVirtualHost() { + return TestUtility::parseYaml( + virtualHostYaml("vhost_0", "host")); + } + + std::vector buildVirtualHost1() { + return {TestUtility::parseYaml( + virtualHostYaml("vhost_1", "vhost.first")), + TestUtility::parseYaml( + virtualHostYaml("vhost_2", "vhost.second"))}; + } + + envoy::api::v2::route::VirtualHost buildVirtualHost2() { + return TestUtility::parseYaml( + virtualHostYaml("vhost_1", "vhost.first")); + } + + // Overridden to insert this stuff into the initialize() at the very beginning of + // HttpIntegrationTest::testRouterRequestAndResponseWithBody(). + void initialize() override { + // Controls how many fake_upstreams_.emplace_back(new FakeUpstream) will happen in + // BaseIntegrationTest::createUpstreams() (which is part of initialize()). + // Make sure this number matches the size of the 'clusters' repeated field in the bootstrap + // config that you use! + setUpstreamCount(2); // the CDS cluster + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); // CDS uses gRPC uses HTTP2. + + // BaseIntegrationTest::initialize() does many things: + // 1) It appends to fake_upstreams_ as many as you asked for via setUpstreamCount(). + // 2) It updates your bootstrap config with the ports your fake upstreams are actually listening + // on (since you're supposed to leave them as 0). + // 3) It creates and starts an IntegrationTestServer - the thing that wraps the almost-actual + // Envoy used in the tests. + // 4) Bringing up the server usually entails waiting to ensure that any listeners specified in + // the bootstrap config have come up, and registering them in a port map (see lookupPort()). + // However, this test needs to defer all of that to later. + defer_listener_finalization_ = true; + HttpIntegrationTest::initialize(); + + // Now that the upstream has been created, process Envoy's request to discover it. + // (First, we have to let Envoy establish its connection to the CDS server.) + AssertionResult result = // xds_connection_ is filled with the new FakeHttpConnection. + fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, xds_connection_); + RELEASE_ASSERT(result, result.message()); + result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_); + RELEASE_ASSERT(result, result.message()); + xds_stream_->startGrpcStream(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + EXPECT_TRUE( + compareDiscoveryRequest(Config::TypeUrl::get().RouteConfiguration, "", {"my_route"})); + sendDiscoveryResponse( + Config::TypeUrl::get().RouteConfiguration, {rdsConfig()}, "1"); + + result = xds_connection_->waitForNewStream(*dispatcher_, vhds_stream_, true); + RELEASE_ASSERT(result, result.message()); + vhds_stream_->startGrpcStream(); + + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + sendDeltaDiscoveryResponse({buildVirtualHost()}, {}, "1", + vhds_stream_); + + // Wait for our statically specified listener to become ready, and register its port in the + // test framework's downstream listener port map. + test_server_->waitUntilListenersReady(); + registerTestServerPorts({"http"}); + } + + void useRdsWithVhosts() { use_rds_with_vhosts = true; } + const envoy::api::v2::RouteConfiguration rdsConfig() const { + return TestUtility::parseYaml( + use_rds_with_vhosts ? RdsConfigWithVhosts : RdsConfig); + } + + FakeStreamPtr vhds_stream_; + bool use_rds_with_vhosts{false}; +}; + +INSTANTIATE_TEST_CASE_P(IpVersionsClientType, VhdsIntegrationTest, GRPC_CLIENT_INTEGRATION_PARAMS); + +TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { + // Calls our initialize(), which includes establishing a listener, route, and cluster. + testRouterHeaderOnlyRequestAndResponse(nullptr, 1); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + + sendDeltaDiscoveryResponse(buildVirtualHost1(), {}, "2", + vhds_stream_); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/one", "vhost.first"); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/two", "vhost.second"); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + + sendDeltaDiscoveryResponse({}, {"vhost_1", "vhost_2"}, "3", + vhds_stream_); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/one"}, + {":scheme", "http"}, + {":authority", "vhost.first"}, + {"x-lyft-user-id", "123"}}; + IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); + response->waitForHeaders(); + EXPECT_STREQ("404", response->headers().Status()->value().c_str()); + + cleanupUpstreamAndDownstream(); +} + +TEST_P(VhdsIntegrationTest, RdsWithVirtualHostsVhdsVirtualHostAddUpdateRemove) { + useRdsWithVhosts(); + + testRouterHeaderOnlyRequestAndResponse(nullptr, 1); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + + sendDeltaDiscoveryResponse(buildVirtualHost1(), {}, "2", + vhds_stream_); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/rdsone", "vhost.rds.first"); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/one", "vhost.first"); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/two", "vhost.second"); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + + sendDeltaDiscoveryResponse({}, {"vhost_1", "vhost_2"}, "3", + vhds_stream_); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + + testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/rdsone", "vhost.rds.first"); + cleanupUpstreamAndDownstream(); + codec_client_->waitForDisconnect(); + + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + Http::TestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/one"}, + {":scheme", "http"}, + {":authority", "vhost.first"}, + {"x-lyft-user-id", "123"}}; + IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); + response->waitForHeaders(); + EXPECT_STREQ("404", response->headers().Status()->value().c_str()); + + cleanupUpstreamAndDownstream(); +} + +} // namespace +} // namespace Envoy diff --git a/test/mocks/router/mocks.cc b/test/mocks/router/mocks.cc index da2b3f22aa20..099d1095c7e7 100644 --- a/test/mocks/router/mocks.cc +++ b/test/mocks/router/mocks.cc @@ -90,6 +90,7 @@ MockConfig::MockConfig() : route_(new NiceMock()) { ON_CALL(*this, route(_, _)).WillByDefault(Return(route_)); ON_CALL(*this, internalOnlyHeaders()).WillByDefault(ReturnRef(internal_only_headers_)); ON_CALL(*this, name()).WillByDefault(ReturnRef(name_)); + ON_CALL(*this, usesVhds()).WillByDefault(Return(false)); } MockConfig::~MockConfig() {} diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index ba40815aaab4..3fff19454949 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -335,6 +335,7 @@ class MockConfig : public Config { MOCK_CONST_METHOD2(route, RouteConstSharedPtr(const Http::HeaderMap&, uint64_t random_value)); MOCK_CONST_METHOD0(internalOnlyHeaders, const std::list&()); MOCK_CONST_METHOD0(name, const std::string&()); + MOCK_CONST_METHOD0(usesVhds, bool()); std::shared_ptr route_; std::list internal_only_headers_; From 0e6c97ed779a7682899d44ae28463fb96b6d6c8e Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 3 Apr 2019 12:24:30 -0700 Subject: [PATCH 02/27] additional formatting fixes --- source/common/router/config_impl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h index d98b193774e2..32e03733ed0e 100644 --- a/source/common/router/config_impl.h +++ b/source/common/router/config_impl.h @@ -793,6 +793,7 @@ class NullConfigImpl : public Config { const std::string& name() const override { return name_; } bool usesVhds() const override { return false; } + private: std::list internal_only_headers_; const std::string name_; From b447f33d9b18f3457d4f3a4df0579a7f6cea1597 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 3 Apr 2019 12:33:04 -0700 Subject: [PATCH 03/27] More formatting fixes --- test/integration/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/BUILD b/test/integration/BUILD index fbd512635fef..f119ed65275d 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -125,8 +125,8 @@ envoy_cc_test( "//test/mocks/server:server_mocks", "//test/test_common:network_utility_lib", "//test/test_common:utility_lib", - "@envoy_api//envoy/api/v2:rds_cc", "@envoy_api//envoy/api/v2:discovery_cc", + "@envoy_api//envoy/api/v2:rds_cc", ], ) From 2da973fb923e4d99fb4f6769ba242f0107c3f688 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 3 Apr 2019 12:44:02 -0700 Subject: [PATCH 04/27] more formatting fixes --- source/common/router/rds_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index fb16720585a5..e5a29238f8d8 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -271,7 +271,7 @@ void VhdsSubscription::removeVhosts( void VhdsSubscription::updateVhosts( std::unordered_map& vhosts, const Protobuf::RepeatedPtrField& added_resources) { - // TODO validate aaded_resources + // TODO (dmitri-d) validate added_resources? for (const auto& resource : added_resources) { envoy::api::v2::route::VirtualHost vhost = MessageUtil::anyConvert(resource.resource()); From 21bb7afaede67ad84b8a7c48fb2d5a6f5b95a7e3 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 3 Apr 2019 13:11:35 -0700 Subject: [PATCH 05/27] Fixing test failures --- test/common/router/rds_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 362419448766..08ba226122d2 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -376,7 +376,7 @@ TEST_F(RdsImplTest, Failure) { } TEST_F(RdsImplTest, VhdsInstantiationShouldSucceedWithDELTA_GRPC) { - void* subscription; + void* subscription = nullptr; const auto route_config = TestUtility::parseYaml(R"EOF( name: my_route vhds: @@ -402,7 +402,7 @@ name: my_route } TEST_F(RdsImplTest, VhdsInstantiationShouldFailWithoutDELTA_GRPC) { - void* subscription; + void* subscription = nullptr; const auto route_config = TestUtility::parseYaml(R"EOF( name: my_route vhds: From 2a6c77f31bb26b523b89039a443be34f316131d9 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 3 Apr 2019 15:43:04 -0700 Subject: [PATCH 06/27] wrapped update details into a dedicated class --- source/common/router/rds_impl.cc | 24 ++++++--------- source/common/router/rds_impl.h | 52 +++++++++++++++++++++++++++++--- 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index e5a29238f8d8..8f65d6acf228 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -64,7 +64,7 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( stat_prefix_(stat_prefix), stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}), route_config_provider_manager_(route_config_provider_manager), manager_identifier_(manager_identifier), time_source_(factory_context.timeSource()), - last_updated_(factory_context.timeSource().systemTime()), uses_vhds_(false) { + last_updated_(factory_context.timeSource().systemTime()) { Envoy::Config::Utility::checkLocalInfo("rds", factory_context.localInfo()); subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource( @@ -74,6 +74,9 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( "envoy.api.v2.RouteDiscoveryService.StreamRoutes", Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), factory_context.api()); + + last_update_details_ = + std::make_unique(route_config_proto_, last_updated_, config_info_); } RdsRouteConfigSubscription::~RdsRouteConfigSubscription() { @@ -115,10 +118,12 @@ void RdsRouteConfigSubscription::onConfigUpdate( route_config_proto_ = route_config; stats_.config_reload_.inc(); - uses_vhds_ = route_config_proto_.has_vhds(); - if (!uses_vhds_) { + if (!route_config_proto_.has_vhds()) { ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, new_hash); + + last_update_details_.reset( + new RdsConfigUpdateDetails(route_config_proto_, last_updated_, config_info_)); for (auto* provider : route_config_providers_) { provider->onConfigUpdate(); } @@ -127,6 +132,7 @@ void RdsRouteConfigSubscription::onConfigUpdate( auto s = new VhdsSubscription(route_config_proto_, factory_context_, stat_prefix_, this); s->registerInitTargetWithInitManager(factory_context_.initManager()); vhds_subscription_.reset(s); + last_update_details_.reset(new VhdsConfigUpdateDetails(*vhds_subscription_)); } } @@ -139,18 +145,6 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { init_target_.ready(); } -absl::optional RdsRouteConfigSubscription::configInfo() const { - return (uses_vhds_ ? vhds_subscription_->configInfo() : config_info_); -} - -envoy::api::v2::RouteConfiguration& RdsRouteConfigSubscription::routeConfiguration() { - return (uses_vhds_ ? vhds_subscription_->routeConfiguration() : route_config_proto_); -} - -SystemTime RdsRouteConfigSubscription::lastUpdated() const { - return (uses_vhds_ ? vhds_subscription_->lastUpdated() : last_updated_); -} - RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( RdsRouteConfigSubscriptionSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context) diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 7145199f147a..ef8cdcfb4cb0 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -108,6 +108,45 @@ class RdsRouteConfigProviderImpl; class VhdsSubscription; typedef std::unique_ptr VhdsSubscriptionPtr; +class ConfigUpdateDetails { +public: + virtual ~ConfigUpdateDetails() = default; + + virtual absl::optional configInfo() const PURE; + virtual envoy::api::v2::RouteConfiguration& routeConfiguration() PURE; + virtual SystemTime lastUpdated() const PURE; +}; + +class RdsConfigUpdateDetails : public ConfigUpdateDetails { +public: + RdsConfigUpdateDetails(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, + absl::optional config_info) + : route_config_proto_(rc), last_updated_(last_updated), config_info_(config_info) {} + + absl::optional configInfo() const override { return config_info_; } + envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } + SystemTime lastUpdated() const override { return last_updated_; } + +private: + envoy::api::v2::RouteConfiguration& route_config_proto_; + SystemTime last_updated_; + absl::optional config_info_; +}; + +class VhdsConfigUpdateDetails : public ConfigUpdateDetails { +public: + VhdsConfigUpdateDetails(ConfigUpdateDetails& subscription) : subscription_(subscription) {} + + absl::optional configInfo() const override { return subscription_.configInfo(); } + envoy::api::v2::RouteConfiguration& routeConfiguration() override { + return subscription_.routeConfiguration(); + } + SystemTime lastUpdated() const override { return subscription_.lastUpdated(); } + +private: + ConfigUpdateDetails& subscription_; +}; + /** * A class that fetches the route configuration dynamically using the RDS API and updates them to * RDS config providers. @@ -132,9 +171,11 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, std::unordered_set& route_config_providers() { return route_config_providers_; } - absl::optional configInfo() const; - envoy::api::v2::RouteConfiguration& routeConfiguration(); - SystemTime lastUpdated() const; + absl::optional configInfo() const { return last_update_details_->configInfo(); } + envoy::api::v2::RouteConfiguration& routeConfiguration() { + return last_update_details_->routeConfiguration(); + } + SystemTime lastUpdated() const { return last_update_details_->lastUpdated(); } private: RdsRouteConfigSubscription( @@ -158,7 +199,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, envoy::api::v2::RouteConfiguration route_config_proto_; std::unordered_set route_config_providers_; VhdsSubscriptionPtr vhds_subscription_; - bool uses_vhds_; + std::unique_ptr last_update_details_; friend class RouteConfigProviderManagerImpl; friend class RdsRouteConfigProviderImpl; @@ -171,7 +212,8 @@ typedef std::unique_ptr (*SubscriptionFactoryFuncti const std::string&, absl::string_view, Api::Api&); class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, - Logger::Loggable { + Logger::Loggable, + public ConfigUpdateDetails { public: VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, Server::Configuration::FactoryContext& factory_context, From dbafd584f71ecf87f8e453392aa919d888fc55ee Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 3 Apr 2019 15:50:39 -0700 Subject: [PATCH 07/27] fixing build failures --- source/common/router/rds_impl.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index ef8cdcfb4cb0..0a241bebaa46 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -244,9 +244,9 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, void rebuildRouteConfig( const std::unordered_map& vhosts, envoy::api::v2::RouteConfiguration& route_config); - absl::optional configInfo() const { return config_info_; } - envoy::api::v2::RouteConfiguration& routeConfiguration() { return route_config_proto_; } - SystemTime lastUpdated() const { return last_updated_; } + absl::optional configInfo() const override { return config_info_; } + envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } + SystemTime lastUpdated() const override { return last_updated_; } std::unique_ptr subscription_; envoy::api::v2::RouteConfiguration route_config_proto_; From 200aa3dd639384ed069c27eadba58e0eb0531c60 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 4 Apr 2019 11:13:37 -0700 Subject: [PATCH 08/27] Fixing build failures --- source/common/router/rds_impl.cc | 7 +++---- source/common/router/rds_impl.h | 10 +++++----- test/integration/vhds_integration_test.cc | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 8f65d6acf228..ed925d739e5e 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -122,8 +122,7 @@ void RdsRouteConfigSubscription::onConfigUpdate( ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, new_hash); - last_update_details_.reset( - new RdsConfigUpdateDetails(route_config_proto_, last_updated_, config_info_)); + last_update_details_ = std::make_unique(route_config_proto_, last_updated_, config_info_); for (auto* provider : route_config_providers_) { provider->onConfigUpdate(); } @@ -132,7 +131,7 @@ void RdsRouteConfigSubscription::onConfigUpdate( auto s = new VhdsSubscription(route_config_proto_, factory_context_, stat_prefix_, this); s->registerInitTargetWithInitManager(factory_context_.initManager()); vhds_subscription_.reset(s); - last_update_details_.reset(new VhdsConfigUpdateDetails(*vhds_subscription_)); + last_update_details_ = std::make_unique(*vhds_subscription_); } } @@ -257,7 +256,7 @@ void VhdsSubscription::initializeVhosts( void VhdsSubscription::removeVhosts( std::unordered_map& vhosts, const Protobuf::RepeatedPtrField& removed_vhost_names) { - for (const auto vhost_name : removed_vhost_names) { + for (const auto& vhost_name : removed_vhost_names) { vhosts.erase(vhost_name); } } diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 0a241bebaa46..c005a2ffb596 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -106,7 +106,7 @@ struct LastConfigInfo { class RdsRouteConfigProviderImpl; class VhdsSubscription; -typedef std::unique_ptr VhdsSubscriptionPtr; +using VhdsSubscriptionPtr = std::unique_ptr; class ConfigUpdateDetails { public: @@ -121,7 +121,7 @@ class RdsConfigUpdateDetails : public ConfigUpdateDetails { public: RdsConfigUpdateDetails(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, absl::optional config_info) - : route_config_proto_(rc), last_updated_(last_updated), config_info_(config_info) {} + : route_config_proto_(rc), last_updated_(last_updated), config_info_(std::move(config_info)) {} absl::optional configInfo() const override { return config_info_; } envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } @@ -205,7 +205,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, friend class RdsRouteConfigProviderImpl; }; -typedef std::shared_ptr RdsRouteConfigSubscriptionSharedPtr; +using RdsRouteConfigSubscriptionSharedPtr = std::shared_ptr; 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&, @@ -220,7 +220,7 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, const std::string& stat_prefix, RdsRouteConfigSubscription* rds_subscription, SubscriptionFactoryFunction factory_function = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); - ~VhdsSubscription() { init_target_.ready(); } + ~VhdsSubscription() override { init_target_.ready(); } // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate @@ -262,7 +262,7 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, }; struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { - ThreadLocalConfig(ConfigConstSharedPtr initial_config) : config_(initial_config) {} + ThreadLocalConfig(ConfigConstSharedPtr initial_config) : config_(std::move(initial_config)) {} ConfigConstSharedPtr config_; }; diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index 91e88f97caf2..a31a8740a955 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -114,7 +114,7 @@ class VhdsIntegrationTest : public HttpIntegrationTest, fake_upstreams_.clear(); } - std::string virtualHostYaml(std::string name, std::string domain) { + std::string virtualHostYaml(const std::string& name, const std::string& domain) { return fmt::format(R"EOF( name: {} domains: [{}] From 68d7c52bb83f6cddbb301ed4a52ffc28f8a68c28 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 4 Apr 2019 14:47:08 -0700 Subject: [PATCH 09/27] Added more vhds tests --- source/common/router/rds_impl.cc | 24 ++- source/common/router/rds_impl.h | 11 +- test/common/router/BUILD | 19 ++ test/common/router/rds_impl_test.cc | 53 ------ test/common/router/vhds_test.cc | 258 ++++++++++++++++++++++++++++ 5 files changed, 297 insertions(+), 68 deletions(-) create mode 100644 test/common/router/vhds_test.cc diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index ed925d739e5e..c6b33dfa63bc 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -122,13 +122,15 @@ void RdsRouteConfigSubscription::onConfigUpdate( ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, new_hash); - last_update_details_ = std::make_unique(route_config_proto_, last_updated_, config_info_); + last_update_details_ = std::make_unique(route_config_proto_, + last_updated_, config_info_); for (auto* provider : route_config_providers_) { provider->onConfigUpdate(); } vhds_subscription_.release(); } else { - auto s = new VhdsSubscription(route_config_proto_, factory_context_, stat_prefix_, this); + auto s = new VhdsSubscription(route_config_proto_, factory_context_, stat_prefix_, + route_config_providers_); s->registerInitTargetWithInitManager(factory_context_.initManager()); vhds_subscription_.reset(s); last_update_details_ = std::make_unique(*vhds_subscription_); @@ -186,18 +188,18 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() { [this, new_config]() -> void { tls_->getTyped().config_ = new_config; }); } -VhdsSubscription::VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, - Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, - RdsRouteConfigSubscription* rds_subscription, - SubscriptionFactoryFunction factory_function) +VhdsSubscription::VhdsSubscription( + const envoy::api::v2::RouteConfiguration& route_configuration, + Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, + std::unordered_set& route_config_providers, + SubscriptionFactoryFunction factory_function) : route_config_proto_(route_configuration), route_config_name_(route_configuration.name()), init_target_(fmt::format("VhdsConfigSubscription {}", route_config_name_), [this]() { subscription_->start({}, *this); }), scope_(factory_context.scope().createScope(stat_prefix + "vhds." + route_config_name_ + ".")), stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), time_source_(factory_context.timeSource()), last_updated_(factory_context.timeSource().systemTime()), - rds_subscription_(rds_subscription) { + route_config_providers_(route_config_providers) { Envoy::Config::Utility::checkLocalInfo("vhds", factory_context.localInfo()); const auto& config_source = route_configuration.vhds().config_source().api_config_source().api_type(); @@ -235,7 +237,7 @@ void VhdsSubscription::onConfigUpdate( stats_.config_reload_.inc(); ENVOY_LOG(debug, "vhds: loading new configuration: config_name={} hash={}", route_config_name_, new_hash); - for (auto* provider : rds_subscription_->route_config_providers()) { + for (auto* provider : route_config_providers_) { provider->onConfigUpdate(); } } @@ -268,6 +270,10 @@ void VhdsSubscription::updateVhosts( for (const auto& resource : added_resources) { envoy::api::v2::route::VirtualHost vhost = MessageUtil::anyConvert(resource.resource()); + auto found = vhosts.find(vhost.name()); + if (found != vhosts.end()) { + vhosts.erase(found); + } vhosts.emplace(vhost.name(), vhost); } } diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index c005a2ffb596..95cce9a02b93 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -121,7 +121,8 @@ class RdsConfigUpdateDetails : public ConfigUpdateDetails { public: RdsConfigUpdateDetails(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, absl::optional config_info) - : route_config_proto_(rc), last_updated_(last_updated), config_info_(std::move(config_info)) {} + : route_config_proto_(rc), last_updated_(last_updated), config_info_(std::move(config_info)) { + } absl::optional configInfo() const override { return config_info_; } envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } @@ -168,9 +169,6 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource).name(); } - std::unordered_set& route_config_providers() { - return route_config_providers_; - } absl::optional configInfo() const { return last_update_details_->configInfo(); } envoy::api::v2::RouteConfiguration& routeConfiguration() { return last_update_details_->routeConfiguration(); @@ -217,7 +215,8 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, public: VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, RdsRouteConfigSubscription* rds_subscription, + const std::string& stat_prefix, + std::unordered_set& route_config_providers, SubscriptionFactoryFunction factory_function = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); ~VhdsSubscription() override { init_target_.ready(); } @@ -256,7 +255,7 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, VhdsStats stats_; TimeSource& time_source_; SystemTime last_updated_; - RdsRouteConfigSubscription* rds_subscription_; + std::unordered_set& route_config_providers_; std::unordered_map virtual_hosts_; absl::optional config_info_; }; diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 08811d0e0665..19a362c94246 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -71,6 +71,25 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "vhds_test", + srcs = ["vhds_test.cc"], + deps = [ + "//source/common/config:filter_json_lib", + "//source/common/config:utility_lib", + "//source/common/http:message_lib", + "//source/common/json:json_loader_lib", + "//source/common/router:rds_lib", + "//source/server/http:admin_lib", + "//test/mocks/local_info:local_info_mocks", + "//test/mocks/server:server_mocks", + "//test/mocks/thread_local:thread_local_mocks", + "//test/mocks/upstream:upstream_mocks", + "//test/test_common:simulated_time_system_lib", + "//test/test_common:utility_lib", + ], +) + envoy_cc_test( name = "retry_state_impl_test", srcs = ["retry_state_impl_test.cc"], diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 08ba226122d2..c73ea59e0190 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -375,59 +375,6 @@ TEST_F(RdsImplTest, Failure) { factory_context_.scope_.counter("foo.rds.foo_route_config.update_rejected").value()); } -TEST_F(RdsImplTest, VhdsInstantiationShouldSucceedWithDELTA_GRPC) { - void* subscription = nullptr; - const auto route_config = TestUtility::parseYaml(R"EOF( -name: my_route -vhds: - config_source: - api_config_source: - api_type: DELTA_GRPC - grpc_services: - envoy_grpc: - cluster_name: xds_cluster - )EOF"); - const std::string context("vhds_test"); - - Envoy::Router::SubscriptionFactoryFunction 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, - Api::Api&) -> std::unique_ptr { - return std::unique_ptr(); - }}; - EXPECT_NO_THROW(VhdsSubscription(route_config, factory_context_, context, - static_cast(subscription), - factory_function)); -} - -TEST_F(RdsImplTest, VhdsInstantiationShouldFailWithoutDELTA_GRPC) { - void* subscription = nullptr; - const auto route_config = TestUtility::parseYaml(R"EOF( -name: my_route -vhds: - config_source: - api_config_source: - api_type: GRPC - grpc_services: - envoy_grpc: - cluster_name: xds_cluster - )EOF"); - const std::string context("vhds_test"); - - Envoy::Router::SubscriptionFactoryFunction 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, - Api::Api&) -> std::unique_ptr { - return std::unique_ptr(); - }}; - EXPECT_THROW(VhdsSubscription(route_config, factory_context_, context, - static_cast(subscription), - factory_function), - EnvoyException); -} - class RouteConfigProviderManagerImplTest : public RdsTestBase { public: void setup() { diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc new file mode 100644 index 000000000000..4abc8f67a387 --- /dev/null +++ b/test/common/router/vhds_test.cc @@ -0,0 +1,258 @@ +#include +#include +#include + +#include "envoy/admin/v2alpha/config_dump.pb.h" +#include "envoy/admin/v2alpha/config_dump.pb.validate.h" +#include "envoy/stats/scope.h" + +#include "common/config/filter_json.h" +#include "common/config/utility.h" +#include "common/http/message_impl.h" +#include "common/json/json_loader.h" +#include "common/router/rds_impl.h" + +#include "server/http/admin.h" + +#include "test/mocks/init/mocks.h" +#include "test/mocks/local_info/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/mocks/thread_local/mocks.h" +#include "test/mocks/upstream/mocks.h" +#include "test/test_common/printers.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using testing::_; +using testing::InSequence; +using testing::Invoke; +using testing::Return; +using testing::ReturnRef; +using testing::SaveArg; + +namespace Envoy { +namespace Router { +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, + Api::Api&) -> std::unique_ptr { + return std::unique_ptr(); + }}; + } + + envoy::api::v2::route::VirtualHost buildVirtualHost(const std::string& name, + const std::string& domain) { + return TestUtility::parseYaml(fmt::format(R"EOF( + name: {} + domains: [{}] + routes: + - match: {{ prefix: "/" }} + route: {{ cluster: "my_service" }} + )EOF", + name, domain)); + } + + Protobuf::RepeatedPtrField + buildAddedResources(const std::vector& added_or_updated) { + Protobuf::RepeatedPtrField to_ret; + + for (const auto& vhost : added_or_updated) { + auto* resource = to_ret.Add(); + resource->set_name(vhost.name()); + resource->set_version("1"); + resource->mutable_resource()->PackFrom(vhost); + } + + return to_ret; + } + + Protobuf::RepeatedPtrField + buildRemovedResources(const std::vector& removed) { + return Protobuf::RepeatedPtrField{removed.begin(), removed.end()}; + } + + NiceMock factory_context_; + Init::ExpectableWatcherImpl init_watcher_; + Init::TargetHandlePtr init_target_handle_; + Envoy::Router::SubscriptionFactoryFunction factory_function_; + const std::string context_ = "vhds_test"; + std::unordered_set providers_; + google::protobuf::util::MessageDifferencer messageDifferencer_; +}; + +// verify that api_type: DELTA_GRPC passes validation +TEST_F(VhdsTest, VhdsInstantiationShouldSucceedWithDELTA_GRPC) { + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + EXPECT_NO_THROW( + VhdsSubscription(route_config, factory_context_, context_, providers_, factory_function_)); +} + +// verify that api_type: GRPC fails validation +TEST_F(VhdsTest, VhdsInstantiationShouldFailWithoutDELTA_GRPC) { + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +vhds: + config_source: + api_config_source: + api_type: GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + EXPECT_THROW( + VhdsSubscription(route_config, factory_context_, context_, providers_, factory_function_), + EnvoyException); +} + +// verify addtion/updating of virtual hosts +TEST_F(VhdsTest, VhdsAddsVirtualHosts) { + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + VhdsSubscription subscription(route_config, factory_context_, context_, providers_, + factory_function_); + EXPECT_EQ(0UL, subscription.routeConfiguration().virtual_hosts_size()); + + auto vhost = buildVirtualHost("vhost1", "vhost.first"); + const auto& added_resources = buildAddedResources({vhost}); + const Protobuf::RepeatedPtrField removed_resources; + subscription.onConfigUpdate(added_resources, removed_resources, "1"); + + EXPECT_EQ(1UL, subscription.routeConfiguration().virtual_hosts_size()); + EXPECT_TRUE( + messageDifferencer_.Equals(vhost, subscription.routeConfiguration().virtual_hosts(0))); +} + +// verify addtion/updating of virtual hosts to already existing ones +TEST_F(VhdsTest, VhdsAddsToExistingVirtualHosts) { + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +virtual_hosts: +- name: vhost_rds1 + domains: ["vhost.rds.first"] + routes: + - match: { prefix: "/rdsone" } + route: { cluster: my_service } +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + VhdsSubscription subscription(route_config, factory_context_, context_, providers_, + factory_function_); + EXPECT_EQ(1UL, subscription.routeConfiguration().virtual_hosts_size()); + EXPECT_EQ("vhost_rds1", subscription.routeConfiguration().virtual_hosts(0).name()); + + auto vhost = buildVirtualHost("vhost1", "vhost.first"); + const auto& added_resources = buildAddedResources({vhost}); + const Protobuf::RepeatedPtrField removed_resources; + subscription.onConfigUpdate(added_resources, removed_resources, "1"); + + EXPECT_EQ(2UL, subscription.routeConfiguration().virtual_hosts_size()); + auto actual_vhost_0 = subscription.routeConfiguration().virtual_hosts(0); + auto actual_vhost_1 = subscription.routeConfiguration().virtual_hosts(1); + EXPECT_TRUE("vhost_rds1" == actual_vhost_0.name() || "vhost_rds1" == actual_vhost_1.name()); + EXPECT_TRUE(messageDifferencer_.Equals(vhost, actual_vhost_0) || + messageDifferencer_.Equals(vhost, actual_vhost_1)); +} + +// verify removal of virtual hosts +TEST_F(VhdsTest, VhdsRemovesAnExistingVirtualHost) { + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +virtual_hosts: +- name: vhost_rds1 + domains: ["vhost.rds.first"] + routes: + - match: { prefix: "/rdsone" } + route: { cluster: my_service } +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + VhdsSubscription subscription(route_config, factory_context_, context_, providers_, + factory_function_); + EXPECT_EQ(1UL, subscription.routeConfiguration().virtual_hosts_size()); + EXPECT_EQ("vhost_rds1", subscription.routeConfiguration().virtual_hosts(0).name()); + + const Protobuf::RepeatedPtrField added_resources; + const auto removed_resources = buildRemovedResources({"vhost_rds1"}); + subscription.onConfigUpdate(added_resources, removed_resources, "1"); + + EXPECT_EQ(0UL, subscription.routeConfiguration().virtual_hosts_size()); +} + +// verify vhds overwrites existing virtual hosts +TEST_F(VhdsTest, VhdsOverwritesAnExistingVirtualHost) { + const auto route_config = TestUtility::parseYaml(R"EOF( +name: my_route +virtual_hosts: +- name: vhost_rds1 + domains: ["vhost.rds.first"] + routes: + - match: { prefix: "/rdsone" } + route: { cluster: my_service } +vhds: + config_source: + api_config_source: + api_type: DELTA_GRPC + grpc_services: + envoy_grpc: + cluster_name: xds_cluster + )EOF"); + + VhdsSubscription subscription(route_config, factory_context_, context_, providers_, + factory_function_); + EXPECT_EQ(1UL, subscription.routeConfiguration().virtual_hosts_size()); + EXPECT_EQ("vhost_rds1", subscription.routeConfiguration().virtual_hosts(0).name()); + + auto vhost = buildVirtualHost("vhost_rds1", "vhost.rds.first.mk2"); + const auto& added_resources = buildAddedResources({vhost}); + const Protobuf::RepeatedPtrField removed_resources; + subscription.onConfigUpdate(added_resources, removed_resources, "1"); + + EXPECT_EQ(1UL, subscription.routeConfiguration().virtual_hosts_size()); + EXPECT_TRUE( + messageDifferencer_.Equals(vhost, subscription.routeConfiguration().virtual_hosts(0))); +} + +} // namespace +} // namespace Router +} // namespace Envoy From e692f0e305cd1f51a490a3296b21ed4dde8397bb Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 4 Apr 2019 15:45:31 -0700 Subject: [PATCH 10/27] Fixed formatting errors --- test/common/router/BUILD | 1 + test/common/router/vhds_test.cc | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/router/BUILD b/test/common/router/BUILD index 19a362c94246..eb3f9a4e4eb3 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -79,6 +79,7 @@ envoy_cc_test( "//source/common/config:utility_lib", "//source/common/http:message_lib", "//source/common/json:json_loader_lib", + "//source/common/protobuf", "//source/common/router:rds_lib", "//source/server/http:admin_lib", "//test/mocks/local_info:local_info_mocks", diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index 4abc8f67a387..084bdfa400d4 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -10,6 +10,7 @@ #include "common/config/utility.h" #include "common/http/message_impl.h" #include "common/json/json_loader.h" +#include "common/protobuf/protobuf.h" #include "common/router/rds_impl.h" #include "server/http/admin.h" @@ -86,7 +87,7 @@ class VhdsTest : public testing::Test { Envoy::Router::SubscriptionFactoryFunction factory_function_; const std::string context_ = "vhds_test"; std::unordered_set providers_; - google::protobuf::util::MessageDifferencer messageDifferencer_; + Protobuf::util::MessageDifferencer messageDifferencer_; }; // verify that api_type: DELTA_GRPC passes validation From 38da6ab32fb2ba87df85f4982c6a54e64e7c560d Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 10 Apr 2019 12:37:55 -0700 Subject: [PATCH 11/27] fixes based on the PR feedback --- source/common/router/rds_impl.cc | 24 +++++++++------------ source/common/router/rds_impl.h | 36 ++++++++++++++++---------------- 2 files changed, 28 insertions(+), 32 deletions(-) diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index c6b33dfa63bc..e9d0abc8b552 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -75,8 +75,8 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription( Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()), factory_context.api()); - last_update_details_ = - std::make_unique(route_config_proto_, last_updated_, config_info_); + config_update_info_ = + std::make_unique(route_config_proto_, last_updated_, config_info_); } RdsRouteConfigSubscription::~RdsRouteConfigSubscription() { @@ -118,22 +118,21 @@ void RdsRouteConfigSubscription::onConfigUpdate( route_config_proto_ = route_config; stats_.config_reload_.inc(); - if (!route_config_proto_.has_vhds()) { + if (route_config_proto_.has_vhds()) { + vhds_subscription_ = std::make_unique( + route_config_proto_, factory_context_, stat_prefix_, route_config_providers_); + vhds_subscription_->registerInitTargetWithInitManager(factory_context_.initManager()); + config_update_info_ = std::make_unique(*vhds_subscription_); + } else { ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_, new_hash); - last_update_details_ = std::make_unique(route_config_proto_, - last_updated_, config_info_); + config_update_info_ = + std::make_unique(route_config_proto_, last_updated_, config_info_); for (auto* provider : route_config_providers_) { provider->onConfigUpdate(); } vhds_subscription_.release(); - } else { - auto s = new VhdsSubscription(route_config_proto_, factory_context_, stat_prefix_, - route_config_providers_); - s->registerInitTargetWithInitManager(factory_context_.initManager()); - vhds_subscription_.reset(s); - last_update_details_ = std::make_unique(*vhds_subscription_); } } @@ -247,9 +246,6 @@ void VhdsSubscription::onConfigUpdate( void VhdsSubscription::initializeVhosts( const envoy::api::v2::RouteConfiguration& route_configuration) { - if (route_configuration.virtual_hosts_size() <= 0) { - return; - } for (const auto& vhost : route_configuration.virtual_hosts()) { virtual_hosts_.emplace(vhost.name(), vhost); } diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 95cce9a02b93..457247ead38a 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -108,19 +108,19 @@ class RdsRouteConfigProviderImpl; class VhdsSubscription; using VhdsSubscriptionPtr = std::unique_ptr; -class ConfigUpdateDetails { +class ConfigUpdateInfo { public: - virtual ~ConfigUpdateDetails() = default; + virtual ~ConfigUpdateInfo() = default; virtual absl::optional configInfo() const PURE; virtual envoy::api::v2::RouteConfiguration& routeConfiguration() PURE; virtual SystemTime lastUpdated() const PURE; }; -class RdsConfigUpdateDetails : public ConfigUpdateDetails { +class RdsConfigUpdateInfo : public ConfigUpdateInfo { public: - RdsConfigUpdateDetails(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, - absl::optional config_info) + RdsConfigUpdateInfo(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, + absl::optional config_info) : route_config_proto_(rc), last_updated_(last_updated), config_info_(std::move(config_info)) { } @@ -134,9 +134,9 @@ class RdsConfigUpdateDetails : public ConfigUpdateDetails { absl::optional config_info_; }; -class VhdsConfigUpdateDetails : public ConfigUpdateDetails { +class VhdsConfigUpdateInfo : public ConfigUpdateInfo { public: - VhdsConfigUpdateDetails(ConfigUpdateDetails& subscription) : subscription_(subscription) {} + VhdsConfigUpdateInfo(ConfigUpdateInfo& subscription) : subscription_(subscription) {} absl::optional configInfo() const override { return subscription_.configInfo(); } envoy::api::v2::RouteConfiguration& routeConfiguration() override { @@ -145,7 +145,7 @@ class VhdsConfigUpdateDetails : public ConfigUpdateDetails { SystemTime lastUpdated() const override { return subscription_.lastUpdated(); } private: - ConfigUpdateDetails& subscription_; + ConfigUpdateInfo& subscription_; }; /** @@ -169,11 +169,11 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource).name(); } - absl::optional configInfo() const { return last_update_details_->configInfo(); } + absl::optional configInfo() const { return config_update_info_->configInfo(); } envoy::api::v2::RouteConfiguration& routeConfiguration() { - return last_update_details_->routeConfiguration(); + return config_update_info_->routeConfiguration(); } - SystemTime lastUpdated() const { return last_update_details_->lastUpdated(); } + SystemTime lastUpdated() const { return config_update_info_->lastUpdated(); } private: RdsRouteConfigSubscription( @@ -197,7 +197,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, envoy::api::v2::RouteConfiguration route_config_proto_; std::unordered_set route_config_providers_; VhdsSubscriptionPtr vhds_subscription_; - std::unique_ptr last_update_details_; + std::unique_ptr config_update_info_; friend class RouteConfigProviderManagerImpl; friend class RdsRouteConfigProviderImpl; @@ -211,7 +211,7 @@ typedef std::unique_ptr (*SubscriptionFactoryFuncti class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, Logger::Loggable, - public ConfigUpdateDetails { + public ConfigUpdateInfo { public: VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, Server::Configuration::FactoryContext& factory_context, @@ -260,11 +260,6 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, absl::optional config_info_; }; -struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { - ThreadLocalConfig(ConfigConstSharedPtr initial_config) : config_(std::move(initial_config)) {} - ConfigConstSharedPtr config_; -}; - /** * Implementation of RouteConfigProvider that fetches the route configuration dynamically using * the subscription. @@ -283,6 +278,11 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } private: + struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { + ThreadLocalConfig(ConfigConstSharedPtr initial_config) : config_(std::move(initial_config)) {} + ConfigConstSharedPtr config_; + }; + RdsRouteConfigProviderImpl(RdsRouteConfigSubscriptionSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context); From 9a146620236e2e244f856a4f2274bacb0401372a Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 10 Apr 2019 12:58:34 -0700 Subject: [PATCH 12/27] fixed a spelling mistake --- test/common/router/vhds_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index 084bdfa400d4..f572b142c909 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -125,7 +125,7 @@ name: my_route EnvoyException); } -// verify addtion/updating of virtual hosts +// verify addition/updating of virtual hosts TEST_F(VhdsTest, VhdsAddsVirtualHosts) { const auto route_config = TestUtility::parseYaml(R"EOF( name: my_route @@ -152,7 +152,7 @@ name: my_route messageDifferencer_.Equals(vhost, subscription.routeConfiguration().virtual_hosts(0))); } -// verify addtion/updating of virtual hosts to already existing ones +// verify addition/updating of virtual hosts to already existing ones TEST_F(VhdsTest, VhdsAddsToExistingVirtualHosts) { const auto route_config = TestUtility::parseYaml(R"EOF( name: my_route From 33f795b816cdcafa9225c0bcbcda33451fb12db2 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 10 Apr 2019 14:57:59 -0700 Subject: [PATCH 13/27] moved out VhdsSubscription class into its own file --- include/envoy/router/BUILD | 10 ++ include/envoy/router/rds.h | 2 + .../envoy/router/route_config_update_info.h | 26 ++++ source/common/router/BUILD | 27 ++++ source/common/router/rds_impl.cc | 97 -------------- source/common/router/rds_impl.h | 97 ++------------ source/common/router/vhds.cc | 120 ++++++++++++++++++ source/common/router/vhds.h | 100 +++++++++++++++ source/server/http/admin.h | 1 + test/common/router/BUILD | 1 + test/common/router/vhds_test.cc | 2 +- 11 files changed, 297 insertions(+), 186 deletions(-) create mode 100644 include/envoy/router/route_config_update_info.h create mode 100644 source/common/router/vhds.cc create mode 100644 source/common/router/vhds.h diff --git a/include/envoy/router/BUILD b/include/envoy/router/BUILD index 1952414e09d1..5f6d13d583eb 100644 --- a/include/envoy/router/BUILD +++ b/include/envoy/router/BUILD @@ -31,6 +31,16 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "route_config_update_info_interface", + hdrs = ["route_config_update_info.h"], + external_deps = ["abseil_optional"], + deps = [ + ":router_interface", + "//source/common/protobuf", + ], +) + envoy_cc_library( name = "router_interface", hdrs = ["router.h"], diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index 8ff43f213f4f..de417750b772 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -43,6 +43,8 @@ class RouteConfigProvider { * @return the last time this RouteConfigProvider was updated. Used for config dumps. */ virtual SystemTime lastUpdated() const PURE; + + virtual void onConfigUpdate() PURE; }; typedef std::unique_ptr RouteConfigProviderPtr; diff --git a/include/envoy/router/route_config_update_info.h b/include/envoy/router/route_config_update_info.h new file mode 100644 index 000000000000..936763e5eaf8 --- /dev/null +++ b/include/envoy/router/route_config_update_info.h @@ -0,0 +1,26 @@ +#pragma once + +#include + +#include "envoy/api/v2/rds.pb.h" + +namespace Envoy { +namespace Router { + +struct LastConfigInfo { + uint64_t last_config_hash_; + std::string last_config_version_; +}; + + +class RouteConfigUpdateInfo { +public: + virtual ~RouteConfigUpdateInfo() = default; + + virtual absl::optional configInfo() const PURE; + virtual envoy::api::v2::RouteConfiguration& routeConfiguration() PURE; + virtual SystemTime lastUpdated() const PURE; +}; + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 498babb17239..503e05157fa8 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -66,6 +66,31 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "vhds_lib", + srcs = ["vhds.cc"], + hdrs = ["vhds.h"], + deps = [ + ":config_lib", + "//include/envoy/config:subscription_interface", + "//include/envoy/http:codes_interface", + "//include/envoy/local_info:local_info_interface", + "//include/envoy/router:rds_interface", + "//include/envoy/router:route_config_provider_manager_interface", + "//include/envoy/router:route_config_update_info_interface", + "//include/envoy/singleton:instance_interface", + "//include/envoy/thread_local:thread_local_interface", + "//source/common/common:assert_lib", + "//source/common/common:minimal_logger_lib", + "//source/common/config:rds_json_lib", + "//source/common/config:subscription_factory_lib", + "//source/common/config:utility_lib", + "//source/common/init:target_lib", + "//source/common/protobuf:utility_lib", + "@envoy_api//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager_cc", + ], +) + envoy_cc_library( name = "rds_lib", srcs = ["rds_impl.cc"], @@ -77,6 +102,7 @@ envoy_cc_library( "//include/envoy/local_info:local_info_interface", "//include/envoy/router:rds_interface", "//include/envoy/router:route_config_provider_manager_interface", + "//include/envoy/router:route_config_update_info_interface", "//include/envoy/server:admin_interface", "//include/envoy/singleton:instance_interface", "//include/envoy/thread_local:thread_local_interface", @@ -86,6 +112,7 @@ envoy_cc_library( "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/init:target_lib", + "//source/common/router:vhds_lib", "//source/common/protobuf:utility_lib", "@envoy_api//envoy/admin/v2alpha:config_dump_cc", "@envoy_api//envoy/api/v2:rds_cc", diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index e9d0abc8b552..2a1da2a74d4a 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -187,103 +187,6 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() { [this, new_config]() -> void { tls_->getTyped().config_ = new_config; }); } -VhdsSubscription::VhdsSubscription( - const envoy::api::v2::RouteConfiguration& route_configuration, - Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - std::unordered_set& route_config_providers, - SubscriptionFactoryFunction factory_function) - : route_config_proto_(route_configuration), route_config_name_(route_configuration.name()), - init_target_(fmt::format("VhdsConfigSubscription {}", route_config_name_), - [this]() { subscription_->start({}, *this); }), - scope_(factory_context.scope().createScope(stat_prefix + "vhds." + route_config_name_ + ".")), - stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), time_source_(factory_context.timeSource()), - last_updated_(factory_context.timeSource().systemTime()), - route_config_providers_(route_config_providers) { - Envoy::Config::Utility::checkLocalInfo("vhds", factory_context.localInfo()); - const auto& config_source = - route_configuration.vhds().config_source().api_config_source().api_type(); - if (config_source != envoy::api::v2::core::ApiConfigSource::DELTA_GRPC) { - throw EnvoyException("vhds: only 'DELTA_GRPC' is supported as an api_type."); - } - - initializeVhosts(route_config_proto_); - subscription_ = factory_function( - route_configuration.vhds().config_source(), factory_context.localInfo(), - 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.api()); -} - -void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { - // We need to allow server startup to continue, even if we have a bad - // config. - init_target_.ready(); -} - -void VhdsSubscription::onConfigUpdate( - const Protobuf::RepeatedPtrField& added_resources, - const Protobuf::RepeatedPtrField& removed_resources, - const std::string& version_info) { - last_updated_ = time_source_.systemTime(); - removeVhosts(virtual_hosts_, removed_resources); - updateVhosts(virtual_hosts_, added_resources); - rebuildRouteConfig(virtual_hosts_, route_config_proto_); - - const uint64_t new_hash = MessageUtil::hash(route_config_proto_); - if (!config_info_ || new_hash != config_info_.value().last_config_hash_) { - config_info_ = {new_hash, version_info}; - stats_.config_reload_.inc(); - ENVOY_LOG(debug, "vhds: loading new configuration: config_name={} hash={}", route_config_name_, - new_hash); - for (auto* provider : route_config_providers_) { - provider->onConfigUpdate(); - } - } - - init_target_.ready(); -} - -void VhdsSubscription::initializeVhosts( - const envoy::api::v2::RouteConfiguration& route_configuration) { - for (const auto& vhost : route_configuration.virtual_hosts()) { - virtual_hosts_.emplace(vhost.name(), vhost); - } -} - -void VhdsSubscription::removeVhosts( - std::unordered_map& vhosts, - const Protobuf::RepeatedPtrField& removed_vhost_names) { - for (const auto& vhost_name : removed_vhost_names) { - vhosts.erase(vhost_name); - } -} - -void VhdsSubscription::updateVhosts( - std::unordered_map& vhosts, - const Protobuf::RepeatedPtrField& added_resources) { - // TODO (dmitri-d) validate added_resources? - for (const auto& resource : added_resources) { - envoy::api::v2::route::VirtualHost vhost = - MessageUtil::anyConvert(resource.resource()); - auto found = vhosts.find(vhost.name()); - if (found != vhosts.end()) { - vhosts.erase(found); - } - vhosts.emplace(vhost.name(), vhost); - } -} - -void VhdsSubscription::rebuildRouteConfig( - const std::unordered_map& vhosts, - envoy::api::v2::RouteConfiguration& route_config) { - - route_config.clear_virtual_hosts(); - for (const auto& vhost : vhosts) { - route_config.mutable_virtual_hosts()->Add()->CopyFrom(vhost.second); - } -} - RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& admin) { config_tracker_entry_ = admin.getConfigTracker().add("routes", [this] { return dumpRouteConfigs(); }); diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 457247ead38a..e753e5dc89a0 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -14,6 +14,7 @@ #include "envoy/http/codes.h" #include "envoy/local_info/local_info.h" #include "envoy/router/rds.h" +#include "envoy/router/route_config_update_info.h" #include "envoy/router/route_config_provider_manager.h" #include "envoy/server/admin.h" #include "envoy/server/filter_config.h" @@ -25,6 +26,7 @@ #include "common/config/subscription_factory.h" #include "common/init/target_impl.h" #include "common/protobuf/utility.h" +#include "common/router/vhds.h" namespace Envoy { namespace Router { @@ -63,6 +65,7 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider { return ConfigInfo{route_config_proto_, ""}; } SystemTime lastUpdated() const override { return last_updated_; } + void onConfigUpdate() override {} private: ConfigConstSharedPtr config_; @@ -88,36 +91,9 @@ struct RdsStats { ALL_RDS_STATS(GENERATE_COUNTER_STRUCT) }; -// clang-format off -#define ALL_VHDS_STATS(COUNTER) \ - COUNTER(config_reload) \ - COUNTER(update_empty) - -// clang-format on - -struct VhdsStats { - ALL_VHDS_STATS(GENERATE_COUNTER_STRUCT) -}; - -struct LastConfigInfo { - uint64_t last_config_hash_; - std::string last_config_version_; -}; - class RdsRouteConfigProviderImpl; -class VhdsSubscription; -using VhdsSubscriptionPtr = std::unique_ptr; -class ConfigUpdateInfo { -public: - virtual ~ConfigUpdateInfo() = default; - - virtual absl::optional configInfo() const PURE; - virtual envoy::api::v2::RouteConfiguration& routeConfiguration() PURE; - virtual SystemTime lastUpdated() const PURE; -}; - -class RdsConfigUpdateInfo : public ConfigUpdateInfo { +class RdsConfigUpdateInfo : public RouteConfigUpdateInfo { public: RdsConfigUpdateInfo(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, absl::optional config_info) @@ -134,9 +110,9 @@ class RdsConfigUpdateInfo : public ConfigUpdateInfo { absl::optional config_info_; }; -class VhdsConfigUpdateInfo : public ConfigUpdateInfo { +class VhdsConfigUpdateInfo : public RouteConfigUpdateInfo { public: - VhdsConfigUpdateInfo(ConfigUpdateInfo& subscription) : subscription_(subscription) {} + VhdsConfigUpdateInfo(RouteConfigUpdateInfo& subscription) : subscription_(subscription) {} absl::optional configInfo() const override { return subscription_.configInfo(); } envoy::api::v2::RouteConfiguration& routeConfiguration() override { @@ -145,7 +121,7 @@ class VhdsConfigUpdateInfo : public ConfigUpdateInfo { SystemTime lastUpdated() const override { return subscription_.lastUpdated(); } private: - ConfigUpdateInfo& subscription_; + RouteConfigUpdateInfo& subscription_; }; /** @@ -195,70 +171,15 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, SystemTime last_updated_; absl::optional config_info_; envoy::api::v2::RouteConfiguration route_config_proto_; - std::unordered_set route_config_providers_; + std::unordered_set route_config_providers_; VhdsSubscriptionPtr vhds_subscription_; - std::unique_ptr config_update_info_; + std::unique_ptr config_update_info_; friend class RouteConfigProviderManagerImpl; friend class RdsRouteConfigProviderImpl; }; using RdsRouteConfigSubscriptionSharedPtr = std::shared_ptr; -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, Api::Api&); - -class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, - Logger::Loggable, - public ConfigUpdateInfo { -public: - VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, - Server::Configuration::FactoryContext& factory_context, - const std::string& stat_prefix, - std::unordered_set& route_config_providers, - SubscriptionFactoryFunction factory_function = - Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); - ~VhdsSubscription() override { init_target_.ready(); } - - // Config::SubscriptionCallbacks - // TODO(fredlas) deduplicate - void onConfigUpdate(const Protobuf::RepeatedPtrField&, - const std::string&) override { - NOT_IMPLEMENTED_GCOVR_EXCL_LINE; - } - void onConfigUpdate(const Protobuf::RepeatedPtrField&, - const Protobuf::RepeatedPtrField&, const std::string&) override; - void onConfigUpdateFailed(const EnvoyException* e) override; - std::string resourceName(const ProtobufWkt::Any& resource) override { - return MessageUtil::anyConvert(resource).name(); - } - - void registerInitTargetWithInitManager(Init::Manager& m) { m.add(init_target_); } - void initializeVhosts(const envoy::api::v2::RouteConfiguration& route_configuration); - void removeVhosts(std::unordered_map& vhosts, - const Protobuf::RepeatedPtrField& removed_vhost_names); - void updateVhosts(std::unordered_map& vhosts, - const Protobuf::RepeatedPtrField& added_resources); - void rebuildRouteConfig( - const std::unordered_map& vhosts, - envoy::api::v2::RouteConfiguration& route_config); - absl::optional configInfo() const override { return config_info_; } - envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } - SystemTime lastUpdated() const override { return last_updated_; } - - std::unique_ptr subscription_; - envoy::api::v2::RouteConfiguration route_config_proto_; - const std::string route_config_name_; - Init::TargetImpl init_target_; - Stats::ScopePtr scope_; - VhdsStats stats_; - TimeSource& time_source_; - SystemTime last_updated_; - std::unordered_set& route_config_providers_; - std::unordered_map virtual_hosts_; - absl::optional config_info_; -}; /** * Implementation of RouteConfigProvider that fetches the route configuration dynamically using diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc new file mode 100644 index 000000000000..3eff05893c73 --- /dev/null +++ b/source/common/router/vhds.cc @@ -0,0 +1,120 @@ +#include "common/router/vhds.h" + +#include +#include +#include +#include + +#include "envoy/api/v2/rds.pb.validate.h" +#include "envoy/api/v2/route/route.pb.validate.h" + +#include "common/common/assert.h" +#include "common/common/fmt.h" +#include "common/config/rds_json.h" +#include "common/config/utility.h" +#include "common/protobuf/utility.h" +#include "common/router/config_impl.h" + +namespace Envoy { +namespace Router { + +VhdsSubscription::VhdsSubscription( + const envoy::api::v2::RouteConfiguration& route_configuration, + Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, + std::unordered_set& route_config_providers, + SubscriptionFactoryFunction factory_function) + : route_config_proto_(route_configuration), route_config_name_(route_configuration.name()), + init_target_(fmt::format("VhdsConfigSubscription {}", route_config_name_), + [this]() { subscription_->start({}, *this); }), + scope_(factory_context.scope().createScope(stat_prefix + "vhds." + route_config_name_ + ".")), + stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), time_source_(factory_context.timeSource()), + last_updated_(factory_context.timeSource().systemTime()), + route_config_providers_(route_config_providers) { + Envoy::Config::Utility::checkLocalInfo("vhds", factory_context.localInfo()); + const auto& config_source = + route_configuration.vhds().config_source().api_config_source().api_type(); + if (config_source != envoy::api::v2::core::ApiConfigSource::DELTA_GRPC) { + throw EnvoyException("vhds: only 'DELTA_GRPC' is supported as an api_type."); + } + + initializeVhosts(route_config_proto_); + subscription_ = factory_function( + route_configuration.vhds().config_source(), factory_context.localInfo(), + 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.api()); +} + +void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { + // We need to allow server startup to continue, even if we have a bad + // config. + init_target_.ready(); +} + +void VhdsSubscription::onConfigUpdate( + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& version_info) { + last_updated_ = time_source_.systemTime(); + removeVhosts(virtual_hosts_, removed_resources); + updateVhosts(virtual_hosts_, added_resources); + rebuildRouteConfig(virtual_hosts_, route_config_proto_); + + const uint64_t new_hash = MessageUtil::hash(route_config_proto_); + if (!config_info_ || new_hash != config_info_.value().last_config_hash_) { + config_info_ = {new_hash, version_info}; + stats_.config_reload_.inc(); + ENVOY_LOG(debug, "vhds: loading new configuration: config_name={} hash={}", route_config_name_, + new_hash); + for (auto* provider : route_config_providers_) { + provider->onConfigUpdate(); + } + } + + init_target_.ready(); +} + +void VhdsSubscription::initializeVhosts( + const envoy::api::v2::RouteConfiguration& route_configuration) { + for (const auto& vhost : route_configuration.virtual_hosts()) { + virtual_hosts_.emplace(vhost.name(), vhost); + } +} + +void VhdsSubscription::removeVhosts( + std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& removed_vhost_names) { + for (const auto& vhost_name : removed_vhost_names) { + vhosts.erase(vhost_name); + } +} + +void VhdsSubscription::updateVhosts( + std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& added_resources) { + // TODO (dmitri-d) validate added_resources? + for (const auto& resource : added_resources) { + envoy::api::v2::route::VirtualHost vhost = + MessageUtil::anyConvert(resource.resource()); + auto found = vhosts.find(vhost.name()); + if (found != vhosts.end()) { + vhosts.erase(found); + } + vhosts.emplace(vhost.name(), vhost); + } +} + +void VhdsSubscription::rebuildRouteConfig( + const std::unordered_map& vhosts, + envoy::api::v2::RouteConfiguration& route_config) { + + route_config.clear_virtual_hosts(); + for (const auto& vhost : vhosts) { + route_config.mutable_virtual_hosts()->Add()->CopyFrom(vhost.second); + } +} + + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h new file mode 100644 index 000000000000..2b2eb6da5bb7 --- /dev/null +++ b/source/common/router/vhds.h @@ -0,0 +1,100 @@ +#pragma once + +#include +#include +#include +#include +#include + +#include "envoy/api/v2/rds.pb.h" +#include "envoy/api/v2/route/route.pb.h" +#include "envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.pb.h" +#include "envoy/config/subscription.h" +#include "envoy/http/codes.h" +#include "envoy/local_info/local_info.h" +#include "envoy/router/rds.h" +#include "envoy/router/route_config_update_info.h" +#include "envoy/server/filter_config.h" +#include "envoy/singleton/instance.h" +#include "envoy/stats/scope.h" +#include "envoy/thread_local/thread_local.h" + +#include "common/common/logger.h" +#include "common/config/subscription_factory.h" +#include "common/init/target_impl.h" +#include "common/protobuf/utility.h" + +namespace Envoy { +namespace Router { + +// clang-format off +#define ALL_VHDS_STATS(COUNTER) \ + COUNTER(config_reload) \ + COUNTER(update_empty) + +// clang-format on + +struct VhdsStats { + ALL_VHDS_STATS(GENERATE_COUNTER_STRUCT) +}; + +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, Api::Api&); + +class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, + Logger::Loggable, + public RouteConfigUpdateInfo { +public: + VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, + std::unordered_set& route_config_providers, + SubscriptionFactoryFunction factory_function = + Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); + ~VhdsSubscription() override { init_target_.ready(); } + + // Config::SubscriptionCallbacks + // TODO(fredlas) deduplicate + void onConfigUpdate(const Protobuf::RepeatedPtrField&, + const std::string&) override { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + void onConfigUpdate(const Protobuf::RepeatedPtrField&, + const Protobuf::RepeatedPtrField&, const std::string&) override; + void onConfigUpdateFailed(const EnvoyException* e) override; + std::string resourceName(const ProtobufWkt::Any& resource) override { + return MessageUtil::anyConvert(resource).name(); + } + + void registerInitTargetWithInitManager(Init::Manager& m) { m.add(init_target_); } + void initializeVhosts(const envoy::api::v2::RouteConfiguration& route_configuration); + void removeVhosts(std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& removed_vhost_names); + void updateVhosts(std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& added_resources); + void rebuildRouteConfig( + const std::unordered_map& vhosts, + envoy::api::v2::RouteConfiguration& route_config); + absl::optional configInfo() const override { return config_info_; } + envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } + SystemTime lastUpdated() const override { return last_updated_; } + + std::unique_ptr subscription_; + envoy::api::v2::RouteConfiguration route_config_proto_; + const std::string route_config_name_; + Init::TargetImpl init_target_; + Stats::ScopePtr scope_; + VhdsStats stats_; + TimeSource& time_source_; + SystemTime last_updated_; + std::unordered_set& route_config_providers_; + std::unordered_map virtual_hosts_; + absl::optional config_info_; +}; + +using VhdsSubscriptionPtr = std::unique_ptr; + +} // namespace Router +} // namespace Envoy diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 31d8511a9c57..bbe0a734ec53 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -153,6 +153,7 @@ class AdminImpl : public Admin, Router::ConfigConstSharedPtr config() override { return config_; } absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } + void onConfigUpdate() override {} Router::ConfigConstSharedPtr config_; TimeSource& time_source_; diff --git a/test/common/router/BUILD b/test/common/router/BUILD index eb3f9a4e4eb3..d84c7cf1c518 100644 --- a/test/common/router/BUILD +++ b/test/common/router/BUILD @@ -81,6 +81,7 @@ envoy_cc_test( "//source/common/json:json_loader_lib", "//source/common/protobuf", "//source/common/router:rds_lib", + "//source/common/router:vhds_lib", "//source/server/http:admin_lib", "//test/mocks/local_info:local_info_mocks", "//test/mocks/server:server_mocks", diff --git a/test/common/router/vhds_test.cc b/test/common/router/vhds_test.cc index f572b142c909..ac84568ac035 100644 --- a/test/common/router/vhds_test.cc +++ b/test/common/router/vhds_test.cc @@ -86,7 +86,7 @@ class VhdsTest : public testing::Test { Init::TargetHandlePtr init_target_handle_; Envoy::Router::SubscriptionFactoryFunction factory_function_; const std::string context_ = "vhds_test"; - std::unordered_set providers_; + std::unordered_set providers_; Protobuf::util::MessageDifferencer messageDifferencer_; }; From 7a40f591d5486a780d2793653ea89532781067e3 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 22 Jan 2019 11:15:07 -0800 Subject: [PATCH 14/27] VHDS: Filter-based on-demand RDS updates --- include/envoy/config/subscription.h | 5 ++ include/envoy/http/filter.h | 2 + include/envoy/router/rds.h | 1 + .../envoy/router/route_config_update_info.h | 1 - .../common/config/delta_subscription_impl.h | 8 +++ source/common/http/async_client_impl.h | 4 ++ source/common/http/conn_manager_impl.cc | 15 ++++- source/common/http/conn_manager_impl.h | 4 ++ source/common/router/BUILD | 30 ++++++++- source/common/router/on_demand_update.cc | 58 +++++++++++++++++ source/common/router/on_demand_update.h | 54 ++++++++++++++++ source/common/router/rds_impl.cc | 36 ++++++++++- source/common/router/rds_impl.h | 15 ++++- source/common/router/router.cc | 14 +++++ source/common/router/vhds.cc | 63 ++++++++++--------- source/common/router/vhds.h | 13 ++-- source/extensions/filters/http/router/BUILD | 1 + .../extensions/filters/http/router/config.cc | 2 + source/server/http/admin.h | 1 + test/integration/vhds_integration_test.cc | 22 ++++++- 20 files changed, 303 insertions(+), 46 deletions(-) create mode 100644 source/common/router/on_demand_update.cc create mode 100644 source/common/router/on_demand_update.h diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index 2897e9798bef..7f3aa845e35a 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -8,6 +8,7 @@ #include "envoy/common/pure.h" #include "envoy/stats/stats_macros.h" +#include "common/common/assert.h" #include "common/protobuf/protobuf.h" namespace Envoy { @@ -81,6 +82,10 @@ class Subscription { * @param resources vector of resource names to fetch. */ virtual void updateResources(const std::vector& resources) PURE; + + virtual void updateResourcesViaAliases(const std::vector&) { + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } }; /** diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index c24a07e928b1..2a201a35f663 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -127,6 +127,8 @@ class StreamFilterCallbacks { */ virtual Router::RouteConstSharedPtr route() PURE; + virtual bool requestRouteConfigUpdate(std::function cb) PURE; + /** * Returns the clusterInfo for the cached route. * This method is to avoid multiple look ups in the filter chain, it also provides a consistent diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index de417750b772..83897f3f560e 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -45,6 +45,7 @@ class RouteConfigProvider { virtual SystemTime lastUpdated() const PURE; virtual void onConfigUpdate() PURE; + virtual bool requestConfigUpdate(const std::string for_domain, std::function cb) PURE; }; typedef std::unique_ptr RouteConfigProviderPtr; diff --git a/include/envoy/router/route_config_update_info.h b/include/envoy/router/route_config_update_info.h index 936763e5eaf8..8cc22ed2e224 100644 --- a/include/envoy/router/route_config_update_info.h +++ b/include/envoy/router/route_config_update_info.h @@ -12,7 +12,6 @@ struct LastConfigInfo { std::string last_config_version_; }; - class RouteConfigUpdateInfo { public: virtual ~RouteConfigUpdateInfo() = default; diff --git a/source/common/config/delta_subscription_impl.h b/source/common/config/delta_subscription_impl.h index 5cd583f357a3..225b179600e1 100644 --- a/source/common/config/delta_subscription_impl.h +++ b/source/common/config/delta_subscription_impl.h @@ -215,6 +215,14 @@ class DeltaSubscriptionImpl stats_.update_attempt_.inc(); } + void updateResourcesViaAliases(const std::vector& aliases) override { + ResourceNameDiff diff; + std::copy(aliases.begin(), aliases.end(), std::inserter(diff.added_, diff.added_.begin())); + queueDiscoveryRequest(diff); + // sendDiscoveryRequest(diff); + stats_.update_attempt_.inc(); + } + private: void disableInitFetchTimeoutTimer() { if (init_fetch_timeout_timer_) { diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 53433106d9b4..8a78cfae7705 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -78,6 +78,8 @@ class AsyncStreamImpl : public AsyncClient::Stream, AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCallbacks& callbacks, const AsyncClient::StreamOptions& options); + bool requestRouteConfigUpdate(std::function) override { return false; } + // Http::AsyncClient::Stream void sendHeaders(HeaderMap& headers, bool end_stream) override; void sendData(Buffer::Instance& data, bool end_stream) override; @@ -371,6 +373,8 @@ class AsyncRequestImpl final : public AsyncClient::Request, AsyncRequestImpl(MessagePtr&& request, AsyncClientImpl& parent, AsyncClient::Callbacks& callbacks, const AsyncClient::RequestOptions& options); + bool requestRouteConfigUpdate(std::function) override { return false; } + // AsyncClient::Request virtual void cancel() override; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 9c23eb39b6d1..4ae3a9d31cda 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -397,6 +397,7 @@ void ConnectionManagerImpl::chargeTracingStats(const Tracing::Reason& tracing_re ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connection_manager) : connection_manager_(connection_manager), + route_config_provider_(connection_manager.config_.routeConfigProvider()), snapped_route_config_(connection_manager.config_.routeConfigProvider().config()), stream_id_(connection_manager.random_generator_.random()), request_response_timespan_(new Stats::Timespan( @@ -1064,6 +1065,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { Router::RouteConstSharedPtr route; if (request_headers_ != nullptr) { route = snapped_route_config_->route(*request_headers_, stream_id_); + // route = route_config_provider_.config()->route(*request_headers_, stream_id_); } stream_info_.route_entry_ = route ? route->routeEntry() : nullptr; cached_route_ = std::move(route); @@ -1076,6 +1078,12 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } } +bool ConnectionManagerImpl::ActiveStream::requestRouteConfigUpdate(std::function cb) { + // TODO check for an empty header? + auto host_header = Http::LowerCaseString(request_headers_->Host()->value().c_str()).get(); + return route_config_provider_.requestConfigUpdate(host_header, cb); +} + void ConnectionManagerImpl::ActiveStream::sendLocalReply( bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, @@ -1710,13 +1718,18 @@ Upstream::ClusterInfoConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBas } Router::RouteConstSharedPtr ConnectionManagerImpl::ActiveStreamFilterBase::route() { - if (!parent_.cached_route_.has_value()) { + if (!parent_.cached_route_.has_value() || parent_.cached_route_.value() == nullptr) { parent_.refreshCachedRoute(); } return parent_.cached_route_.value(); } +bool ConnectionManagerImpl::ActiveStreamFilterBase::requestRouteConfigUpdate( + std::function cb) { + return parent_.requestRouteConfigUpdate(cb); +} + void ConnectionManagerImpl::ActiveStreamFilterBase::clearRouteCache() { parent_.cached_route_ = absl::optional(); parent_.cached_cluster_info_ = absl::optional(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 7b4f48614443..c18d8109c176 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -121,6 +121,7 @@ class ConnectionManagerImpl : Logger::Loggable, Event::Dispatcher& dispatcher() override; void resetStream() override; Router::RouteConstSharedPtr route() override; + bool requestRouteConfigUpdate(std::function cb) override; Upstream::ClusterInfoConstSharedPtr clusterInfo() override; void clearRouteCache() override; uint64_t streamId() override; @@ -271,6 +272,7 @@ class ConnectionManagerImpl : Logger::Loggable, void responseDataTooLarge(); void responseDataDrained(); + bool requestRouteConfigUpdate(std::function) { return false; } StreamEncoderFilterSharedPtr handle_; }; @@ -351,6 +353,7 @@ class ConnectionManagerImpl : Logger::Loggable, void traceRequest(); void refreshCachedRoute(); + bool requestRouteConfigUpdate(std::function cb); // Pass on watermark callbacks to watermark subscribers. This boils down to passing watermark // events for this stream and the downstream connection to the router filter. @@ -420,6 +423,7 @@ class ConnectionManagerImpl : Logger::Loggable, void onRequestTimeout(); ConnectionManagerImpl& connection_manager_; + Router::RouteConfigProvider& route_config_provider_; Router::ConfigConstSharedPtr snapped_route_config_; Tracing::SpanPtr active_span_; const uint64_t stream_id_; diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 503e05157fa8..6627da5b4987 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -112,8 +112,8 @@ envoy_cc_library( "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", "//source/common/init:target_lib", - "//source/common/router:vhds_lib", "//source/common/protobuf:utility_lib", + "//source/common/router:vhds_lib", "@envoy_api//envoy/admin/v2alpha:config_dump_cc", "@envoy_api//envoy/api/v2:rds_cc", "@envoy_api//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager_cc", @@ -187,6 +187,34 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "on_demand_update_lib", + srcs = ["on_demand_update.cc"], + hdrs = ["on_demand_update.h"], + deps = [ + "//include/envoy/event:dispatcher_interface", + "//include/envoy/event:timer_interface", + "//include/envoy/http:filter_interface", + "//include/envoy/server:filter_config_interface", + "//source/common/access_log:access_log_lib", + "//source/common/buffer:watermark_buffer_lib", + "//source/common/common:assert_lib", + "//source/common/common:empty_string", + "//source/common/common:enum_to_int", + "//source/common/common:hash_lib", + "//source/common/common:hex_lib", + "//source/common/common:minimal_logger_lib", + "//source/common/common:utility_lib", + "//source/common/grpc:common_lib", + "//source/common/http:codes_lib", + "//source/common/http:header_map_lib", + "//source/common/http:headers_lib", + "//source/common/http:message_lib", + "//source/common/http:utility_lib", + "@envoy_api//envoy/config/filter/http/router/v2:router_cc", + ], +) + envoy_cc_library( name = "router_ratelimit_lib", srcs = ["router_ratelimit.cc"], diff --git a/source/common/router/on_demand_update.cc b/source/common/router/on_demand_update.cc new file mode 100644 index 000000000000..450562ed13df --- /dev/null +++ b/source/common/router/on_demand_update.cc @@ -0,0 +1,58 @@ +#include "common/router/on_demand_update.h" + +#include "common/common/assert.h" +#include "common/common/enum_to_int.h" +#include "common/http/codes.h" + +#include "extensions/filters/http/well_known_names.h" + +namespace Envoy { +namespace Router { + +void OnDemandRouteUpdate::requestRouteConfigUpdate() { + if (callbacks_->route() != nullptr) { + filter_return_ = FilterReturn::ContinueDecoding; + } else { + auto configUpdateScheduled = + callbacks_->requestRouteConfigUpdate([this]() -> void { onComplete(); }); + filter_return_ = + configUpdateScheduled ? FilterReturn::StopDecoding : FilterReturn::ContinueDecoding; + } +} + +Http::FilterHeadersStatus OnDemandRouteUpdate::decodeHeaders(Http::HeaderMap&, bool) { + requestRouteConfigUpdate(); + return filter_return_ == FilterReturn::StopDecoding ? Http::FilterHeadersStatus::StopIteration + : Http::FilterHeadersStatus::Continue; +} + +Http::FilterDataStatus OnDemandRouteUpdate::decodeData(Buffer::Instance&, bool) { + return filter_return_ == FilterReturn::StopDecoding + ? Http::FilterDataStatus::StopIterationAndWatermark + : Http::FilterDataStatus::Continue; +} + +Http::FilterTrailersStatus OnDemandRouteUpdate::decodeTrailers(Http::HeaderMap&) { + return filter_return_ == FilterReturn::StopDecoding ? Http::FilterTrailersStatus::StopIteration + : Http::FilterTrailersStatus::Continue; +} + +void OnDemandRouteUpdate::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { + callbacks_ = &callbacks; +} + +void OnDemandRouteUpdate::onComplete() { + filter_return_ = FilterReturn::ContinueDecoding; + + if (!callbacks_->decodingBuffer() && // Redirects with body not yet supported. + callbacks_->recreateStream()) { + // cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); + return; + } + + // recreating stream failed, continue the filter-chain + callbacks_->continueDecoding(); +} + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/on_demand_update.h b/source/common/router/on_demand_update.h new file mode 100644 index 000000000000..59e221b015bc --- /dev/null +++ b/source/common/router/on_demand_update.h @@ -0,0 +1,54 @@ +#pragma once + +#include +#include +#include +#include + +#include "envoy/http/filter.h" +#include "envoy/local_info/local_info.h" +#include "envoy/runtime/runtime.h" +#include "envoy/stats/scope.h" +#include "envoy/upstream/cluster_manager.h" + +#include "common/common/assert.h" +#include "common/common/logger.h" +#include "common/common/matchers.h" +#include "common/http/header_map_impl.h" + +namespace Envoy { +namespace Router { + +class OnDemandRouteUpdate : public Logger::Loggable, + public Http::StreamDecoderFilter { +public: + OnDemandRouteUpdate() {} + + void requestRouteConfigUpdate(); + void onComplete(); + + // Http::StreamDecoderFilter + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; + Http::FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap& trailers) override; + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; + void onDestroy() override {} + +private: + // State of this filter's communication with the external authorization service. + // The filter has either not started calling the external service, in the middle of calling + // it or has completed. + enum class State { NotStarted, Calling, Complete }; + + // FilterReturn is used to capture what the return code should be to the filter chain. + // if this filter is either in the middle of calling the service or the result is denied then + // the filter chain should stop. Otherwise the filter chain can continue to the next filter. + enum class FilterReturn { ContinueDecoding, StopDecoding }; + + Http::StreamDecoderFilterCallbacks* callbacks_{}; + State state_{State::NotStarted}; + FilterReturn filter_return_{FilterReturn::ContinueDecoding}; +}; + +} // namespace Router +} // namespace Envoy diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 2a1da2a74d4a..851e5f69bfc2 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -145,11 +145,18 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { init_target_.ready(); } +void RdsRouteConfigSubscription::ondemandUpdate(const std::vector& aliases) { + if (vhds_subscription_.get() == nullptr) + return; + vhds_subscription_->ondemandUpdate(aliases); +} + RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( RdsRouteConfigSubscriptionSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context) : subscription_(std::move(subscription)), factory_context_(factory_context), - tls_(factory_context.threadLocal().allocateSlot()) { + tls_(factory_context.threadLocal().allocateSlot()), + config_update_callbacks_(factory_context.threadLocal().allocateSlot()) { ConfigConstSharedPtr initial_config; if (subscription_->configInfo().has_value()) { initial_config = @@ -160,6 +167,9 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( tls_->set([initial_config](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { return std::make_shared(initial_config); }); + config_update_callbacks_->set([](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { + return std::make_shared(); + }); subscription_->route_config_providers_.insert(this); } @@ -183,8 +193,28 @@ absl::optional RdsRouteConfigProviderImpl::conf void RdsRouteConfigProviderImpl::onConfigUpdate() { ConfigConstSharedPtr new_config( new ConfigImpl(subscription_->routeConfiguration(), factory_context_, false)); - tls_->runOnAllThreads( - [this, new_config]() -> void { tls_->getTyped().config_ = new_config; }); + tls_->runOnAllThreads([this, new_config]() -> void { + tls_->getTyped().config_ = new_config; + auto callbacks = config_update_callbacks_->getTyped().callbacks_; + if (!callbacks.empty()) { + auto cb = callbacks.front(); + callbacks.pop(); + cb(); + } + }); +} + +bool RdsRouteConfigProviderImpl::requestConfigUpdate(const std::string for_domain, + std::function cb) { + if (!config()->usesVhds()) { + return false; + } + // TODO check for an empty header? + factory_context_.dispatcher().post( + [this, for_domain]() -> void { subscription_->ondemandUpdate({for_domain}); }); + config_update_callbacks_->getTyped().callbacks_.push(cb); + + return true; } RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& admin) { diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index e753e5dc89a0..749a31c3fbc8 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -14,8 +15,8 @@ #include "envoy/http/codes.h" #include "envoy/local_info/local_info.h" #include "envoy/router/rds.h" -#include "envoy/router/route_config_update_info.h" #include "envoy/router/route_config_provider_manager.h" +#include "envoy/router/route_config_update_info.h" #include "envoy/server/admin.h" #include "envoy/server/filter_config.h" #include "envoy/singleton/instance.h" @@ -66,6 +67,7 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider { } SystemTime lastUpdated() const override { return last_updated_; } void onConfigUpdate() override {} + bool requestConfigUpdate(const std::string, std::function) override { return false; } private: ConfigConstSharedPtr config_; @@ -146,6 +148,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, return MessageUtil::anyConvert(resource).name(); } absl::optional configInfo() const { return config_update_info_->configInfo(); } + void ondemandUpdate(const std::vector& aliases); envoy::api::v2::RouteConfiguration& routeConfiguration() { return config_update_info_->routeConfiguration(); } @@ -181,6 +184,10 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, using RdsRouteConfigSubscriptionSharedPtr = std::shared_ptr; +struct ThreadLocalCallbacks : public ThreadLocal::ThreadLocalObject { + std::queue> callbacks_; +}; + /** * Implementation of RouteConfigProvider that fetches the route configuration dynamically using * the subscription. @@ -191,12 +198,13 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, ~RdsRouteConfigProviderImpl(); RdsRouteConfigSubscription& subscription() { return *subscription_; } - void onConfigUpdate(); + void onConfigUpdate() override; // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; absl::optional configInfo() const override; SystemTime lastUpdated() const override { return subscription_->lastUpdated(); } + bool requestConfigUpdate(const std::string for_domain, std::function cb) override; private: struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { @@ -207,10 +215,13 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigProviderImpl(RdsRouteConfigSubscriptionSharedPtr&& subscription, Server::Configuration::FactoryContext& factory_context); + void addConfigUpdateCallback(std::function cb); + RdsRouteConfigSubscriptionSharedPtr subscription_; Server::Configuration::FactoryContext& factory_context_; SystemTime last_updated_; ThreadLocal::SlotPtr tls_; + ThreadLocal::SlotPtr config_update_callbacks_; friend class RouteConfigProviderManagerImpl; }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 3d65db36144d..0a5e2ef9f5d0 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -272,6 +272,20 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // Determine if there is a route entry or a direct response for the request. route_ = callbacks_->route(); if (!route_) { + /* + attempting_internal_redirect_with_complete_stream_ = true; + // upstream_request_->upstream_timing_.last_upstream_rx_byte_received_ && + downstream_end_stream_; + + if (//downstream_end_stream_ && + !callbacks_->decodingBuffer() && // Redirects with body not yet supported. + callbacks_->recreateStream()) { + //cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); + return Http::FilterHeadersStatus::StopIteration; + } + + attempting_internal_redirect_with_complete_stream_ = false; + */ config_.stats_.no_route_.inc(); ENVOY_STREAM_LOG(debug, "no cluster match for URL '{}'", *callbacks_, headers.Path()->value().c_str()); diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 3eff05893c73..203e8d5d1264 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -18,32 +18,36 @@ namespace Envoy { namespace Router { -VhdsSubscription::VhdsSubscription( - const envoy::api::v2::RouteConfiguration& route_configuration, - Server::Configuration::FactoryContext& factory_context, const std::string& stat_prefix, - std::unordered_set& route_config_providers, - SubscriptionFactoryFunction factory_function) - : route_config_proto_(route_configuration), route_config_name_(route_configuration.name()), - init_target_(fmt::format("VhdsConfigSubscription {}", route_config_name_), - [this]() { subscription_->start({}, *this); }), - scope_(factory_context.scope().createScope(stat_prefix + "vhds." + route_config_name_ + ".")), - stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), time_source_(factory_context.timeSource()), - last_updated_(factory_context.timeSource().systemTime()), - route_config_providers_(route_config_providers) { +VhdsSubscription::VhdsSubscription(const envoy::api::v2::RouteConfiguration& route_configuration, + Server::Configuration::FactoryContext& factory_context, + const std::string& stat_prefix, + std::unordered_set& route_config_providers, + SubscriptionFactoryFunction factory_function) + : route_config_proto_(route_configuration), route_config_name_(route_configuration.name()), + init_target_(fmt::format("VhdsConfigSubscription {}", route_config_name_), + [this]() { subscription_->start({}, *this); }), + scope_(factory_context.scope().createScope(stat_prefix + "vhds." + route_config_name_ + ".")), + stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}), time_source_(factory_context.timeSource()), + last_updated_(factory_context.timeSource().systemTime()), + route_config_providers_(route_config_providers) { Envoy::Config::Utility::checkLocalInfo("vhds", factory_context.localInfo()); const auto& config_source = - route_configuration.vhds().config_source().api_config_source().api_type(); + route_configuration.vhds().config_source().api_config_source().api_type(); if (config_source != envoy::api::v2::core::ApiConfigSource::DELTA_GRPC) { throw EnvoyException("vhds: only 'DELTA_GRPC' is supported as an api_type."); } initializeVhosts(route_config_proto_); subscription_ = factory_function( - route_configuration.vhds().config_source(), factory_context.localInfo(), - 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.api()); + route_configuration.vhds().config_source(), factory_context.localInfo(), + 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.api()); +} + +void VhdsSubscription::ondemandUpdate(const std::vector& aliases) { + subscription_->updateResourcesViaAliases(aliases); } void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { @@ -53,9 +57,9 @@ void VhdsSubscription::onConfigUpdateFailed(const EnvoyException*) { } void VhdsSubscription::onConfigUpdate( - const Protobuf::RepeatedPtrField& added_resources, - const Protobuf::RepeatedPtrField& removed_resources, - const std::string& version_info) { + const Protobuf::RepeatedPtrField& added_resources, + const Protobuf::RepeatedPtrField& removed_resources, + const std::string& version_info) { last_updated_ = time_source_.systemTime(); removeVhosts(virtual_hosts_, removed_resources); updateVhosts(virtual_hosts_, added_resources); @@ -76,27 +80,27 @@ void VhdsSubscription::onConfigUpdate( } void VhdsSubscription::initializeVhosts( - const envoy::api::v2::RouteConfiguration& route_configuration) { + const envoy::api::v2::RouteConfiguration& route_configuration) { for (const auto& vhost : route_configuration.virtual_hosts()) { virtual_hosts_.emplace(vhost.name(), vhost); } } void VhdsSubscription::removeVhosts( - std::unordered_map& vhosts, - const Protobuf::RepeatedPtrField& removed_vhost_names) { + std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& removed_vhost_names) { for (const auto& vhost_name : removed_vhost_names) { vhosts.erase(vhost_name); } } void VhdsSubscription::updateVhosts( - std::unordered_map& vhosts, - const Protobuf::RepeatedPtrField& added_resources) { + std::unordered_map& vhosts, + const Protobuf::RepeatedPtrField& added_resources) { // TODO (dmitri-d) validate added_resources? for (const auto& resource : added_resources) { envoy::api::v2::route::VirtualHost vhost = - MessageUtil::anyConvert(resource.resource()); + MessageUtil::anyConvert(resource.resource()); auto found = vhosts.find(vhost.name()); if (found != vhosts.end()) { vhosts.erase(found); @@ -106,8 +110,8 @@ void VhdsSubscription::updateVhosts( } void VhdsSubscription::rebuildRouteConfig( - const std::unordered_map& vhosts, - envoy::api::v2::RouteConfiguration& route_config) { + const std::unordered_map& vhosts, + envoy::api::v2::RouteConfiguration& route_config) { route_config.clear_virtual_hosts(); for (const auto& vhost : vhosts) { @@ -115,6 +119,5 @@ void VhdsSubscription::rebuildRouteConfig( } } - } // namespace Router } // namespace Envoy diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 2b2eb6da5bb7..674614774d6f 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -39,9 +39,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, Api::Api&); + 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, Api::Api&); class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, Logger::Loggable, @@ -52,7 +52,7 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, const std::string& stat_prefix, std::unordered_set& route_config_providers, SubscriptionFactoryFunction factory_function = - Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); + Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource); ~VhdsSubscription() override { init_target_.ready(); } // Config::SubscriptionCallbacks @@ -75,8 +75,9 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, void updateVhosts(std::unordered_map& vhosts, const Protobuf::RepeatedPtrField& added_resources); void rebuildRouteConfig( - const std::unordered_map& vhosts, - envoy::api::v2::RouteConfiguration& route_config); + const std::unordered_map& vhosts, + envoy::api::v2::RouteConfiguration& route_config); + void ondemandUpdate(const std::vector& aliases); absl::optional configInfo() const override { return config_info_; } envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } SystemTime lastUpdated() const override { return last_updated_; } diff --git a/source/extensions/filters/http/router/BUILD b/source/extensions/filters/http/router/BUILD index ddffe3458ebd..82aeab231ac4 100644 --- a/source/extensions/filters/http/router/BUILD +++ b/source/extensions/filters/http/router/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "//include/envoy/registry", "//source/common/config:filter_json_lib", "//source/common/json:config_schemas_lib", + "//source/common/router:on_demand_update_lib", "//source/common/router:router_lib", "//source/common/router:shadow_writer_lib", "//source/extensions/filters/http:well_known_names", diff --git a/source/extensions/filters/http/router/config.cc b/source/extensions/filters/http/router/config.cc index 3c7b97a37d4d..a792c7fe6a07 100644 --- a/source/extensions/filters/http/router/config.cc +++ b/source/extensions/filters/http/router/config.cc @@ -5,6 +5,7 @@ #include "common/config/filter_json.h" #include "common/json/config_schemas.h" +#include "common/router/on_demand_update.h" #include "common/router/router.h" #include "common/router/shadow_writer_impl.h" @@ -21,6 +22,7 @@ Http::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoTyped( proto_config)); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamDecoderFilter(std::make_shared()); callbacks.addStreamDecoderFilter(std::make_shared(*filter_config)); }; } diff --git a/source/server/http/admin.h b/source/server/http/admin.h index bbe0a734ec53..e5653c1466d2 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -154,6 +154,7 @@ class AdminImpl : public Admin, absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} + bool requestConfigUpdate(const std::string, std::function) { return false; } Router::ConfigConstSharedPtr config_; TimeSource& time_source_; diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index a31a8740a955..d2349d073848 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -240,8 +240,17 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { {":authority", "vhost.first"}, {"x-lyft-user-id", "123"}}; IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); + EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"vhost.first"}, {}, + vhds_stream_)); + sendDeltaDiscoveryResponse({buildVirtualHost2()}, {}, "4", + vhds_stream_); + + waitForNextUpstreamRequest(1); + // Send response headers, and end_stream if there is no response body. + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForHeaders(); - EXPECT_STREQ("404", response->headers().Status()->value().c_str()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); cleanupUpstreamAndDownstream(); } @@ -288,8 +297,17 @@ TEST_P(VhdsIntegrationTest, RdsWithVirtualHostsVhdsVirtualHostAddUpdateRemove) { {":authority", "vhost.first"}, {"x-lyft-user-id", "123"}}; IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); + EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"vhost.first"}, {}, + vhds_stream_)); + sendDeltaDiscoveryResponse({buildVirtualHost2()}, {}, "4", + vhds_stream_); + + waitForNextUpstreamRequest(1); + // Send response headers, and end_stream if there is no response body. + upstream_request_->encodeHeaders(default_response_headers_, true); + response->waitForHeaders(); - EXPECT_STREQ("404", response->headers().Status()->value().c_str()); + EXPECT_STREQ("200", response->headers().Status()->value().c_str()); cleanupUpstreamAndDownstream(); } From 96a6c9008b2a09e98cdfe7ce99b40c9927ef240b Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 16 May 2019 14:17:13 -0700 Subject: [PATCH 15/27] Fixes after merging latest changes Signed-off-by: Dmitri Dolguikh --- include/envoy/config/subscription.h | 2 +- .../common/config/delta_subscription_impl.cc | 6 +++ .../common/config/delta_subscription_impl.h | 9 +--- .../common/config/delta_subscription_state.cc | 32 ++++++++++++--- .../common/config/delta_subscription_state.h | 4 ++ source/common/http/conn_manager_impl.cc | 2 +- source/common/router/rds_impl.cc | 14 +++++-- source/common/router/rds_impl.h | 41 +------------------ source/common/router/vhds.cc | 3 +- source/common/router/vhds.h | 2 +- .../http/conn_manager_impl_fuzz_test.cc | 3 ++ test/common/http/conn_manager_impl_test.cc | 3 ++ test/integration/vhds_integration_test.cc | 7 +++- test/mocks/http/mocks.cc | 2 + test/mocks/http/mocks.h | 2 + 15 files changed, 70 insertions(+), 62 deletions(-) diff --git a/include/envoy/config/subscription.h b/include/envoy/config/subscription.h index 0421cf77c5a3..6e2258755f72 100644 --- a/include/envoy/config/subscription.h +++ b/include/envoy/config/subscription.h @@ -83,7 +83,7 @@ class Subscription { */ virtual void updateResources(const std::set& update_to_these_names) PURE; - virtual void updateResourcesViaAliases(const std::vector&) { + virtual void updateResourcesViaAliases(const std::set&) { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } }; diff --git a/source/common/config/delta_subscription_impl.cc b/source/common/config/delta_subscription_impl.cc index e3d285ec8010..d4da9e7e78e5 100644 --- a/source/common/config/delta_subscription_impl.cc +++ b/source/common/config/delta_subscription_impl.cc @@ -42,6 +42,12 @@ void DeltaSubscriptionImpl::updateResources(const std::set& update_ trySendDiscoveryRequests(); } +void DeltaSubscriptionImpl::updateResourcesViaAliases(const std::set& updates_to_these_aliases) { + state_->updateResourceInterestViaAliases(updates_to_these_aliases); + // Tell the server about our new interests, if there are any. + trySendDiscoveryRequests(); +} + // Config::GrpcStreamCallbacks void DeltaSubscriptionImpl::onStreamEstablished() { state_->markStreamFresh(); diff --git a/source/common/config/delta_subscription_impl.h b/source/common/config/delta_subscription_impl.h index ba646eb0b80d..10faaa11c11e 100644 --- a/source/common/config/delta_subscription_impl.h +++ b/source/common/config/delta_subscription_impl.h @@ -38,6 +38,7 @@ class DeltaSubscriptionImpl : public Subscription, // Config::Subscription void start(const std::set& resources, SubscriptionCallbacks& callbacks) override; void updateResources(const std::set& update_to_these_names) override; + void updateResourcesViaAliases(const std::set& updates_to_these_aliases) override; // Config::GrpcStreamCallbacks void onStreamEstablished() override; @@ -47,14 +48,6 @@ class DeltaSubscriptionImpl : public Subscription, void onWriteable() override; - void updateResourcesViaAliases(const std::vector& aliases) override { - ResourceNameDiff diff; - std::copy(aliases.begin(), aliases.end(), std::inserter(diff.added_, diff.added_.begin())); - queueDiscoveryRequest(diff); - // sendDiscoveryRequest(diff); - stats_.update_attempt_.inc(); - } - private: void kickOffAck(UpdateAck ack); diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index c709f785d544..f947e84439fa 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -76,10 +76,14 @@ void DeltaSubscriptionState::updateResourceInterest( } } +void DeltaSubscriptionState::updateResourceInterestViaAliases(const std::set& updates_to_these_aliases) { + aliases_added_.insert(updates_to_these_aliases.begin(), updates_to_these_aliases.end()); +} + // Not having sent any requests yet counts as an "update pending" since you're supposed to resend // the entirety of your interest at the start of a stream, even if nothing has changed. bool DeltaSubscriptionState::subscriptionUpdatePending() const { - return !names_added_.empty() || !names_removed_.empty() || + return !aliases_added_.empty() || !names_added_.empty() || !names_removed_.empty() || !any_request_sent_yet_in_current_stream_; } @@ -143,12 +147,32 @@ void DeltaSubscriptionState::handleEstablishmentFailure() { envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { envoy::api::v2::DeltaDiscoveryRequest request; + if(!any_request_sent_yet_in_current_stream_) { + populateDiscoveryRequest(request); + } else if (!aliases_added_.empty()) { + populateDiscoveryRequestWithAliases(request); + } else { + populateDiscoveryRequest(request); + } + + request.set_type_url(type_url_); + request.mutable_node()->MergeFrom(local_info_.node()); + return request; +} + +void DeltaSubscriptionState::populateDiscoveryRequestWithAliases(envoy::api::v2::DeltaDiscoveryRequest &request) { + std::copy(aliases_added_.begin(), aliases_added_.end(), + Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_subscribe())); + aliases_added_.clear(); +} + +void DeltaSubscriptionState::populateDiscoveryRequest(envoy::api::v2::DeltaDiscoveryRequest &request) { if (!any_request_sent_yet_in_current_stream_) { any_request_sent_yet_in_current_stream_ = true; // initial_resource_versions "must be populated for first request in a stream". // Also, since this might be a new server, we must explicitly state *all* of our subscription // interest. - for (auto const& resource : resource_versions_) { + for (auto const &resource : resource_versions_) { // Populate initial_resource_versions with the resource versions we currently have. // Resources we are interested in, but are still waiting to get any version of from the // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) @@ -167,10 +191,6 @@ envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_unsubscribe())); names_added_.clear(); names_removed_.clear(); - - request.set_type_url(type_url_); - request.mutable_node()->MergeFrom(local_info_.node()); - return request; } void DeltaSubscriptionState::disableInitFetchTimeoutTimer() { diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 5fbb6f79f5a1..128c24eb6ab5 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -35,6 +35,7 @@ class DeltaSubscriptionState : public Logger::Loggable { // Update which resources we're interested in subscribing to. void updateResourceInterest(const std::set& update_to_these_names); + void updateResourceInterestViaAliases(const std::set& updates_to_these_aliases); // Whether there was a change in our subscription interest we have yet to inform the server of. bool subscriptionUpdatePending() const; @@ -75,6 +76,8 @@ class DeltaSubscriptionState : public Logger::Loggable { void setResourceVersion(const std::string& resource_name, const std::string& resource_version); void setResourceWaitingForServer(const std::string& resource_name); void setLostInterestInResource(const std::string& resource_name); + void populateDiscoveryRequest(envoy::api::v2::DeltaDiscoveryRequest &request); + void populateDiscoveryRequestWithAliases(envoy::api::v2::DeltaDiscoveryRequest &request); // A map from resource name to per-resource version. The keys of this map are exactly the resource // names we are currently interested in. Those in the waitingForServer state currently don't have @@ -99,6 +102,7 @@ class DeltaSubscriptionState : public Logger::Loggable { // Feel free to change to unordered if you can figure out how to make it work. std::set names_added_; std::set names_removed_; + std::set aliases_added_; SubscriptionStats& stats_; }; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 7929f779547f..d64be5ab0042 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1134,7 +1134,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { bool ConnectionManagerImpl::ActiveStream::requestRouteConfigUpdate(std::function cb) { // TODO check for an empty header? - auto host_header = Http::LowerCaseString(request_headers_->Host()->value().c_str()).get(); + auto host_header = Http::LowerCaseString(std::string(request_headers_->Host()->value().getStringView())).get(); return route_config_provider_.requestConfigUpdate(host_header, cb); } diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index e0893b3067e4..2e1fbdb7cf3f 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -138,7 +138,7 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { init_target_.ready(); } -void RdsRouteConfigSubscription::ondemandUpdate(const std::vector& aliases) { +void RdsRouteConfigSubscription::ondemandUpdate(const std::set& aliases) { if (vhds_subscription_.get() == nullptr) return; vhds_subscription_->ondemandUpdate(aliases); @@ -149,7 +149,7 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( Server::Configuration::FactoryContext& factory_context) : subscription_(std::move(subscription)), config_update_info_(subscription_->routeConfigUpdate()), factory_context_(factory_context), - tls_(factory_context.threadLocal().allocateSlot(), config_update_callbacks_(factory_context.threadLocal().allocateSlot()) { + tls_(factory_context.threadLocal().allocateSlot()), config_update_callbacks_(factory_context.threadLocal().allocateSlot()) { ConfigConstSharedPtr initial_config; if (config_update_info_->configInfo().has_value()) { initial_config = std::make_shared(config_update_info_->routeConfiguration(), @@ -191,7 +191,15 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() { ConfigConstSharedPtr new_config( new ConfigImpl(config_update_info_->routeConfiguration(), factory_context_, false)); tls_->runOnAllThreads( - [this, new_config]() -> void { tls_->getTyped().config_ = new_config; }); + [this, new_config]() -> void { + tls_->getTyped().config_ = new_config; + auto callbacks = config_update_callbacks_->getTyped().callbacks_; + if (!callbacks.empty()) { + auto cb = callbacks.front(); + callbacks.pop(); + cb(); + } + }); } RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& admin) { diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 779552391a85..b09a352aa280 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -96,37 +96,6 @@ struct RdsStats { class RdsRouteConfigProviderImpl; -class RdsConfigUpdateInfo : public RouteConfigUpdateInfo { -public: - RdsConfigUpdateInfo(envoy::api::v2::RouteConfiguration& rc, SystemTime last_updated, - absl::optional config_info) - : route_config_proto_(rc), last_updated_(last_updated), config_info_(std::move(config_info)) { - } - - absl::optional configInfo() const override { return config_info_; } - envoy::api::v2::RouteConfiguration& routeConfiguration() override { return route_config_proto_; } - SystemTime lastUpdated() const override { return last_updated_; } - -private: - envoy::api::v2::RouteConfiguration& route_config_proto_; - SystemTime last_updated_; - absl::optional config_info_; -}; - -class VhdsConfigUpdateInfo : public RouteConfigUpdateInfo { -public: - VhdsConfigUpdateInfo(RouteConfigUpdateInfo& subscription) : subscription_(subscription) {} - - absl::optional configInfo() const override { return subscription_.configInfo(); } - envoy::api::v2::RouteConfiguration& routeConfiguration() override { - return subscription_.routeConfiguration(); - } - SystemTime lastUpdated() const override { return subscription_.lastUpdated(); } - -private: - RouteConfigUpdateInfo& subscription_; -}; - /** * A class that fetches the route configuration dynamically using the RDS API and updates them to * RDS config providers. @@ -141,6 +110,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, } RouteConfigUpdatePtr& routeConfigUpdate() { return config_update_info_; } + void ondemandUpdate(const std::set& aliases); // Config::SubscriptionCallbacks // TODO(fredlas) deduplicate void onConfigUpdate(const Protobuf::RepeatedPtrField& resources, @@ -153,12 +123,6 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource).name(); } - absl::optional configInfo() const { return config_update_info_->configInfo(); } - void ondemandUpdate(const std::vector& aliases); - envoy::api::v2::RouteConfiguration& routeConfiguration() { - return config_update_info_->routeConfiguration(); - } - SystemTime lastUpdated() const { return config_update_info_->lastUpdated(); } private: RdsRouteConfigSubscription( @@ -168,7 +132,6 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks, RouteConfigProviderManagerImpl& route_config_provider_manager); std::unique_ptr subscription_; - Server::Configuration::FactoryContext& factory_context_; const std::string route_config_name_; Server::Configuration::FactoryContext& factory_context_; Init::TargetImpl init_target_; @@ -201,6 +164,7 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigSubscription& subscription() { return *subscription_; } void onConfigUpdate() override; + bool requestConfigUpdate(const std::string for_domain, std::function cb); // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; @@ -223,7 +187,6 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigSubscriptionSharedPtr subscription_; RouteConfigUpdatePtr& config_update_info_; Server::Configuration::FactoryContext& factory_context_; - SystemTime last_updated_; ThreadLocal::SlotPtr tls_; ThreadLocal::SlotPtr config_update_callbacks_; diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc index 482364ceedfb..d99897575fda 100644 --- a/source/common/router/vhds.cc +++ b/source/common/router/vhds.cc @@ -48,7 +48,8 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info, factory_context.api()); } -void VhdsSubscription::ondemandUpdate(const std::vector& aliases) { +void VhdsSubscription::ondemandUpdate(const std::set& aliases) { + std::cout << "11111111111111111111111111111111111\n"; subscription_->updateResourcesViaAliases(aliases); } diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h index 9996e1857b94..14e83887575e 100644 --- a/source/common/router/vhds.h +++ b/source/common/router/vhds.h @@ -66,7 +66,7 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks, std::string resourceName(const ProtobufWkt::Any& resource) override { return MessageUtil::anyConvert(resource).name(); } - void ondemandUpdate(const std::vector& aliases); + void ondemandUpdate(const std::set& aliases); void registerInitTargetWithInitManager(Init::Manager& m) { m.add(init_target_); } RouteConfigUpdatePtr& config_update_info_; diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 9711f6929d13..6e39e3fae5ba 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -52,6 +52,9 @@ class FuzzConfig : public ConnectionManagerConfig { absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} + bool requestConfigUpdate(const std::string, std::function) override { + return false; + } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index d8c8d3121aaa..7c5b33ed4808 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -71,6 +71,9 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} + bool requestConfigUpdate(const std::string, std::function) override { + return false; + } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index 90d5d80e86e9..7244878184e0 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -187,6 +187,8 @@ class VhdsIntegrationTest : public HttpIntegrationTest, compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); sendDeltaDiscoveryResponse({buildVirtualHost()}, {}, "1", vhds_stream_); + EXPECT_TRUE( + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); // Wait for our statically specified listener to become ready, and register its port in the // test framework's downstream listener port map. @@ -220,7 +222,7 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { sendDeltaDiscoveryResponse(buildVirtualHost1(), {}, "2", vhds_stream_); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/one", "vhost.first"); cleanupUpstreamAndDownstream(); @@ -233,7 +235,8 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { sendDeltaDiscoveryResponse({}, {"vhost_1", "vhost_2"}, "3", vhds_stream_); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + ENVOY_LOG_MISC(debug, "3333333333333333333333333333333333333333333333333333333333333333"); // an upstream request to an (now) unknown domain codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); diff --git a/test/mocks/http/mocks.cc b/test/mocks/http/mocks.cc index 2e071d5a11c0..38b697289b99 100644 --- a/test/mocks/http/mocks.cc +++ b/test/mocks/http/mocks.cc @@ -65,6 +65,7 @@ MockStreamDecoderFilterCallbacks::MockStreamDecoderFilterCallbacks() { ON_CALL(*this, activeSpan()).WillByDefault(ReturnRef(active_span_)); ON_CALL(*this, tracingConfig()).WillByDefault(ReturnRef(tracing_config_)); + ON_CALL(*this, requestRouteConfigUpdate(_)).WillByDefault(Return(false)); } MockStreamDecoderFilterCallbacks::~MockStreamDecoderFilterCallbacks() {} @@ -74,6 +75,7 @@ MockStreamEncoderFilterCallbacks::MockStreamEncoderFilterCallbacks() { ON_CALL(*this, encodingBuffer()).WillByDefault(Invoke(&buffer_, &Buffer::InstancePtr::get)); ON_CALL(*this, activeSpan()).WillByDefault(ReturnRef(active_span_)); ON_CALL(*this, tracingConfig()).WillByDefault(ReturnRef(tracing_config_)); + ON_CALL(*this, requestRouteConfigUpdate(_)).WillByDefault(Return(false)); } MockStreamEncoderFilterCallbacks::~MockStreamEncoderFilterCallbacks() {} diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 6dd46f28b347..9fbc75734903 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -136,6 +136,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(resetStream, void()); MOCK_METHOD0(clusterInfo, Upstream::ClusterInfoConstSharedPtr()); MOCK_METHOD0(route, Router::RouteConstSharedPtr()); + MOCK_METHOD1(requestRouteConfigUpdate, bool(std::function)); MOCK_METHOD0(clearRouteCache, void()); MOCK_METHOD0(streamId, uint64_t()); MOCK_METHOD0(streamInfo, StreamInfo::StreamInfo&()); @@ -213,6 +214,7 @@ class MockStreamEncoderFilterCallbacks : public StreamEncoderFilterCallbacks, MOCK_METHOD0(resetStream, void()); MOCK_METHOD0(clusterInfo, Upstream::ClusterInfoConstSharedPtr()); MOCK_METHOD0(route, Router::RouteConstSharedPtr()); + MOCK_METHOD1(requestRouteConfigUpdate, bool(std::function)); MOCK_METHOD0(clearRouteCache, void()); MOCK_METHOD0(streamId, uint64_t()); MOCK_METHOD0(streamInfo, StreamInfo::StreamInfo&()); From fe73bbdd26d7ed2d6ad73329e2bb0117fda0a06e Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 16 May 2019 14:32:33 -0700 Subject: [PATCH 16/27] Reponded to feedback Signed-off-by: Dmitri Dolguikh --- source/common/router/on_demand_update.cc | 7 +++---- test/integration/vhds_integration_test.cc | 1 - 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/source/common/router/on_demand_update.cc b/source/common/router/on_demand_update.cc index 450562ed13df..1ddb8fd24354 100644 --- a/source/common/router/on_demand_update.cc +++ b/source/common/router/on_demand_update.cc @@ -13,10 +13,10 @@ void OnDemandRouteUpdate::requestRouteConfigUpdate() { if (callbacks_->route() != nullptr) { filter_return_ = FilterReturn::ContinueDecoding; } else { - auto configUpdateScheduled = + auto config_update_scheduled = callbacks_->requestRouteConfigUpdate([this]() -> void { onComplete(); }); filter_return_ = - configUpdateScheduled ? FilterReturn::StopDecoding : FilterReturn::ContinueDecoding; + config_update_scheduled ? FilterReturn::StopDecoding : FilterReturn::ContinueDecoding; } } @@ -33,8 +33,7 @@ Http::FilterDataStatus OnDemandRouteUpdate::decodeData(Buffer::Instance&, bool) } Http::FilterTrailersStatus OnDemandRouteUpdate::decodeTrailers(Http::HeaderMap&) { - return filter_return_ == FilterReturn::StopDecoding ? Http::FilterTrailersStatus::StopIteration - : Http::FilterTrailersStatus::Continue; + return Http::FilterTrailersStatus::Continue; } void OnDemandRouteUpdate::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) { diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index 7244878184e0..8c6d3e75ac52 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -236,7 +236,6 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { vhds_stream_); EXPECT_TRUE( compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); - ENVOY_LOG_MISC(debug, "3333333333333333333333333333333333333333333333333333333333333333"); // an upstream request to an (now) unknown domain codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); From 374de2cc8a5aa029d555c29b0edab496f6c1aad3 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 16 May 2019 15:50:57 -0700 Subject: [PATCH 17/27] Moved on-demand filter to extensions/filters/http/on_demand dir Signed-off-by: Dmitri Dolguikh --- source/common/router/BUILD | 28 ----------- .../extensions/filters/http/on_demand/BUILD | 26 ++++++++++ .../http/on_demand}/on_demand_update.cc | 12 +++-- .../http/on_demand}/on_demand_update.h | 48 +++++++++---------- source/extensions/filters/http/router/BUILD | 2 +- .../extensions/filters/http/router/config.cc | 4 +- .../filters/http/well_known_names.h | 2 + 7 files changed, 61 insertions(+), 61 deletions(-) create mode 100644 source/extensions/filters/http/on_demand/BUILD rename source/{common/router => extensions/filters/http/on_demand}/on_demand_update.cc (89%) rename source/{common/router => extensions/filters/http/on_demand}/on_demand_update.h (51%) diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 7ce1aab1e205..6f66df20eac9 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -204,34 +204,6 @@ envoy_cc_library( ], ) -envoy_cc_library( - name = "on_demand_update_lib", - srcs = ["on_demand_update.cc"], - hdrs = ["on_demand_update.h"], - deps = [ - "//include/envoy/event:dispatcher_interface", - "//include/envoy/event:timer_interface", - "//include/envoy/http:filter_interface", - "//include/envoy/server:filter_config_interface", - "//source/common/access_log:access_log_lib", - "//source/common/buffer:watermark_buffer_lib", - "//source/common/common:assert_lib", - "//source/common/common:empty_string", - "//source/common/common:enum_to_int", - "//source/common/common:hash_lib", - "//source/common/common:hex_lib", - "//source/common/common:minimal_logger_lib", - "//source/common/common:utility_lib", - "//source/common/grpc:common_lib", - "//source/common/http:codes_lib", - "//source/common/http:header_map_lib", - "//source/common/http:headers_lib", - "//source/common/http:message_lib", - "//source/common/http:utility_lib", - "@envoy_api//envoy/config/filter/http/router/v2:router_cc", - ], -) - envoy_cc_library( name = "router_ratelimit_lib", srcs = ["router_ratelimit.cc"], diff --git a/source/extensions/filters/http/on_demand/BUILD b/source/extensions/filters/http/on_demand/BUILD new file mode 100644 index 000000000000..0ae69dbdad6b --- /dev/null +++ b/source/extensions/filters/http/on_demand/BUILD @@ -0,0 +1,26 @@ +licenses(["notice"]) # Apache 2 + +# On-demand RDS update HTTP filter + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "on_demand_update_lib", + srcs = ["on_demand_update.cc"], + hdrs = ["on_demand_update.h"], + deps = [ + "//include/envoy/event:dispatcher_interface", + "//include/envoy/http:filter_interface", + "//include/envoy/server:filter_config_interface", + "//source/common/common:enum_to_int", + "//source/common/http:header_map_lib", + "//source/common/common:assert_lib", + "//source/common/http:codes_lib", + ], +) diff --git a/source/common/router/on_demand_update.cc b/source/extensions/filters/http/on_demand/on_demand_update.cc similarity index 89% rename from source/common/router/on_demand_update.cc rename to source/extensions/filters/http/on_demand/on_demand_update.cc index 1ddb8fd24354..111dc00b9b6e 100644 --- a/source/common/router/on_demand_update.cc +++ b/source/extensions/filters/http/on_demand/on_demand_update.cc @@ -1,13 +1,13 @@ -#include "common/router/on_demand_update.h" +#include "extensions/filters/http/on_demand/on_demand_update.h" #include "common/common/assert.h" #include "common/common/enum_to_int.h" #include "common/http/codes.h" -#include "extensions/filters/http/well_known_names.h" - namespace Envoy { -namespace Router { +namespace Extensions { +namespace HttpFilters { +namespace OnDemand { void OnDemandRouteUpdate::requestRouteConfigUpdate() { if (callbacks_->route() != nullptr) { @@ -53,5 +53,7 @@ void OnDemandRouteUpdate::onComplete() { callbacks_->continueDecoding(); } -} // namespace Router +} // namespace OnDemand +} // namespace HttpFilters +} // namespace Extensions } // namespace Envoy diff --git a/source/common/router/on_demand_update.h b/source/extensions/filters/http/on_demand/on_demand_update.h similarity index 51% rename from source/common/router/on_demand_update.h rename to source/extensions/filters/http/on_demand/on_demand_update.h index 59e221b015bc..72cd8718f17c 100644 --- a/source/common/router/on_demand_update.h +++ b/source/extensions/filters/http/on_demand/on_demand_update.h @@ -1,54 +1,52 @@ #pragma once -#include -#include -#include -#include - #include "envoy/http/filter.h" -#include "envoy/local_info/local_info.h" -#include "envoy/runtime/runtime.h" -#include "envoy/stats/scope.h" -#include "envoy/upstream/cluster_manager.h" - -#include "common/common/assert.h" -#include "common/common/logger.h" -#include "common/common/matchers.h" -#include "common/http/header_map_impl.h" namespace Envoy { -namespace Router { +namespace Extensions { +namespace HttpFilters { +namespace OnDemand { -class OnDemandRouteUpdate : public Logger::Loggable, - public Http::StreamDecoderFilter { +class OnDemandRouteUpdate : public Http::StreamDecoderFilter { public: OnDemandRouteUpdate() {} void requestRouteConfigUpdate(); + void onComplete(); // Http::StreamDecoderFilter - Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; - Http::FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; - Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap& trailers) override; - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap &headers, bool end_stream) override; + + Http::FilterDataStatus decodeData(Buffer::Instance &data, bool end_stream) override; + + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap &trailers) override; + + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks &callbacks) override; + void onDestroy() override {} private: // State of this filter's communication with the external authorization service. // The filter has either not started calling the external service, in the middle of calling // it or has completed. - enum class State { NotStarted, Calling, Complete }; + enum class State { + NotStarted, Calling, Complete + }; // FilterReturn is used to capture what the return code should be to the filter chain. // if this filter is either in the middle of calling the service or the result is denied then // the filter chain should stop. Otherwise the filter chain can continue to the next filter. - enum class FilterReturn { ContinueDecoding, StopDecoding }; + enum class FilterReturn { + ContinueDecoding, StopDecoding + }; - Http::StreamDecoderFilterCallbacks* callbacks_{}; + Http::StreamDecoderFilterCallbacks *callbacks_{}; State state_{State::NotStarted}; FilterReturn filter_return_{FilterReturn::ContinueDecoding}; }; -} // namespace Router +} // namespace OnDemand +} // namespace HttpFilters +} // namespace Extensions } // namespace Envoy diff --git a/source/extensions/filters/http/router/BUILD b/source/extensions/filters/http/router/BUILD index 82aeab231ac4..2d80951c380d 100644 --- a/source/extensions/filters/http/router/BUILD +++ b/source/extensions/filters/http/router/BUILD @@ -19,7 +19,7 @@ envoy_cc_library( "//include/envoy/registry", "//source/common/config:filter_json_lib", "//source/common/json:config_schemas_lib", - "//source/common/router:on_demand_update_lib", + "//source/extensions/filters/http/on_demand:on_demand_update_lib", "//source/common/router:router_lib", "//source/common/router:shadow_writer_lib", "//source/extensions/filters/http:well_known_names", diff --git a/source/extensions/filters/http/router/config.cc b/source/extensions/filters/http/router/config.cc index a792c7fe6a07..bc4c095116d2 100644 --- a/source/extensions/filters/http/router/config.cc +++ b/source/extensions/filters/http/router/config.cc @@ -5,7 +5,7 @@ #include "common/config/filter_json.h" #include "common/json/config_schemas.h" -#include "common/router/on_demand_update.h" +#include "extensions/filters/http/on_demand/on_demand_update.h" #include "common/router/router.h" #include "common/router/shadow_writer_impl.h" @@ -22,7 +22,7 @@ Http::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoTyped( proto_config)); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); + callbacks.addStreamDecoderFilter(std::make_shared()); callbacks.addStreamDecoderFilter(std::make_shared(*filter_config)); }; } diff --git a/source/extensions/filters/http/well_known_names.h b/source/extensions/filters/http/well_known_names.h index 0352c5bc2c06..1c5b4c4e2628 100644 --- a/source/extensions/filters/http/well_known_names.h +++ b/source/extensions/filters/http/well_known_names.h @@ -42,6 +42,8 @@ class HttpFilterNameValues { const std::string HealthCheck = "envoy.health_check"; // Lua filter const std::string Lua = "envoy.lua"; + // On-demand RDS updates filter + const std::string OnDemand = "envoy.on_demand"; // Squash filter const std::string Squash = "envoy.squash"; // External Authorization filter From c5e314a67e5b622c16fc23bafd9909a80ed3c97c Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Fri, 17 May 2019 16:05:05 -0700 Subject: [PATCH 18/27] fix to build kafka extension under python3 Signed-off-by: Dmitri Dolguikh --- .../network/kafka/protocol_code_generator/kafka_generator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/kafka/protocol_code_generator/kafka_generator.py b/source/extensions/filters/network/kafka/protocol_code_generator/kafka_generator.py index 663c8a58fbf8..aed29b906a0d 100755 --- a/source/extensions/filters/network/kafka/protocol_code_generator/kafka_generator.py +++ b/source/extensions/filters/network/kafka/protocol_code_generator/kafka_generator.py @@ -235,7 +235,7 @@ def constructor_init_list(self): return ', '.join(init_list) def field_count(self): - return len(self.used_fields()) + return len(list(self.used_fields())) def example_value(self): return ', '.join(map(lambda x: x.example_value_for_test(self.version), self.used_fields())) From bd11db6be5df158f4ebb225c3b251d4c5991125c Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 12:38:12 -0700 Subject: [PATCH 19/27] Post-merge fixes Signed-off-by: Dmitri Dolguikh --- test/integration/vhds_integration_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index 6380540351e0..add0cd4c7a0a 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -249,8 +249,8 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"vhost.first"}, {}, vhds_stream_)); - sendDeltaDiscoveryResponse({buildVirtualHost2()}, {}, "4", - vhds_stream_); + sendDeltaDiscoveryResponse( + Config::TypeUrl::get().VirtualHost, {buildVirtualHost2()}, {}, "4", vhds_stream_); waitForNextUpstreamRequest(1); // Send response headers, and end_stream if there is no response body. @@ -312,8 +312,8 @@ TEST_P(VhdsIntegrationTest, RdsWithVirtualHostsVhdsVirtualHostAddUpdateRemove) { IntegrationStreamDecoderPtr response = codec_client_->makeHeaderOnlyRequest(request_headers); EXPECT_TRUE(compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {"vhost.first"}, {}, vhds_stream_)); - sendDeltaDiscoveryResponse({buildVirtualHost2()}, {}, "4", - vhds_stream_); + sendDeltaDiscoveryResponse( + Config::TypeUrl::get().VirtualHost, {buildVirtualHost2()}, {}, "4", vhds_stream_); waitForNextUpstreamRequest(1); // Send response headers, and end_stream if there is no response body. From bf63b6d671de61e39d7c083c2e210837d8eec848 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 13:42:53 -0700 Subject: [PATCH 20/27] Renamed requestRouteConfigUpdate to requestVirtualHostsUpdate, updated docs Signed-off-by: Dmitri Dolguikh --- include/envoy/router/rds.h | 8 +++++++- source/common/http/conn_manager_impl.cc | 4 ++-- source/common/router/rds_impl.cc | 4 ++-- source/common/router/rds_impl.h | 4 ++-- source/server/http/admin.h | 2 +- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index c90499903435..2f0bd8238549 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -49,7 +49,13 @@ class RouteConfigProvider { */ virtual void onConfigUpdate() PURE; - virtual bool requestConfigUpdate(const std::string for_domain, std::function cb) PURE; + /** + * Callback used to request an update to the route configuration. + * @param for_domain supplies the domain name that virtual hosts contained in the VHDS response must match on + * @param cb callback to be called when the configuration update has been propagated to worker threads + * @return whether a request for a configuration update has been successfully scheduled + */ + virtual bool requestVirtualHostsUpdate(const std::string &for_domain, std::function cb) PURE; }; typedef std::unique_ptr RouteConfigProviderPtr; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2cdd368130bd..b067b8e12b06 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1140,9 +1140,9 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } bool ConnectionManagerImpl::ActiveStream::requestRouteConfigUpdate(std::function cb) { - // TODO check for an empty header? + ASSERT(!request_headers_->Host()->value().empty()); auto host_header = Http::LowerCaseString(std::string(request_headers_->Host()->value().getStringView())).get(); - return route_config_provider_.requestConfigUpdate(host_header, cb); + return route_config_provider_.requestVirtualHostsUpdate(host_header, cb); } void ConnectionManagerImpl::ActiveStream::sendLocalReply( diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 16fdbecc0be3..c291eb81d857 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -205,8 +205,8 @@ Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() { return tls_->getTyped().config_; } -bool RdsRouteConfigProviderImpl::requestConfigUpdate(const std::string for_domain, - std::function cb) { +bool RdsRouteConfigProviderImpl::requestVirtualHostsUpdate(const std::string &for_domain, + std::function cb) { if (!config()->usesVhds()) { return false; } diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 2d38dd1e5a66..2024f5ce7065 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -68,7 +68,7 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider { } SystemTime lastUpdated() const override { return last_updated_; } void onConfigUpdate() override {} - bool requestConfigUpdate(const std::string, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string &, std::function) override { return false; } private: ConfigConstSharedPtr config_; @@ -164,7 +164,7 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigSubscription& subscription() { return *subscription_; } void onConfigUpdate() override; - bool requestConfigUpdate(const std::string for_domain, std::function cb); + bool requestVirtualHostsUpdate(const std::string &for_domain, std::function cb); // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 7c065b4c5992..32e130e2d8a0 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -155,7 +155,7 @@ class AdminImpl : public Admin, absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestConfigUpdate(const std::string, std::function) { return false; } + bool requestVirtualHostsUpdate(const std::string &, std::function) { return false; } Router::ConfigConstSharedPtr config_; TimeSource& time_source_; From bbc38b496fc811cad017660fa2bed5aa8144396d Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 15:51:46 -0700 Subject: [PATCH 21/27] Added comments re: stream restart after route config update Signed-off-by: Dmitri Dolguikh --- include/envoy/router/rds.h | 9 ++++-- source/common/http/conn_manager_impl.cc | 3 +- source/common/router/rds_impl.cc | 29 +++++++++---------- source/common/router/rds_impl.h | 6 ++-- .../extensions/filters/http/on_demand/BUILD | 4 +-- .../http/on_demand/on_demand_update.cc | 7 +++-- .../filters/http/on_demand/on_demand_update.h | 20 +++++-------- source/extensions/filters/http/router/BUILD | 2 +- .../extensions/filters/http/router/config.cc | 6 ++-- source/server/http/admin.h | 2 +- .../http/conn_manager_impl_fuzz_test.cc | 4 +-- test/common/http/conn_manager_impl_test.cc | 4 +-- test/integration/vhds_integration_test.cc | 4 +-- 13 files changed, 50 insertions(+), 50 deletions(-) diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index 2f0bd8238549..5e15d1b6a057 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -51,11 +51,14 @@ class RouteConfigProvider { /** * Callback used to request an update to the route configuration. - * @param for_domain supplies the domain name that virtual hosts contained in the VHDS response must match on - * @param cb callback to be called when the configuration update has been propagated to worker threads + * @param for_domain supplies the domain name that virtual hosts contained in the VHDS response + * must match on + * @param cb callback to be called when the configuration update has been propagated to worker + * threads * @return whether a request for a configuration update has been successfully scheduled */ - virtual bool requestVirtualHostsUpdate(const std::string &for_domain, std::function cb) PURE; + virtual bool requestVirtualHostsUpdate(const std::string& for_domain, + std::function cb) PURE; }; typedef std::unique_ptr RouteConfigProviderPtr; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index b067b8e12b06..151e21ea3700 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1141,7 +1141,8 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { bool ConnectionManagerImpl::ActiveStream::requestRouteConfigUpdate(std::function cb) { ASSERT(!request_headers_->Host()->value().empty()); - auto host_header = Http::LowerCaseString(std::string(request_headers_->Host()->value().getStringView())).get(); + auto host_header = + Http::LowerCaseString(std::string(request_headers_->Host()->value().getStringView())).get(); return route_config_provider_.requestVirtualHostsUpdate(host_header, cb); } diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index c291eb81d857..0e8be8548151 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -153,12 +153,10 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { init_target_.ready(); } - void RdsRouteConfigSubscription::ondemandUpdate(const std::set& aliases) { if (vhds_subscription_.get() == nullptr) return; vhds_subscription_->ondemandUpdate(aliases); - } bool RdsRouteConfigSubscription::validateUpdateSize(int num_resources) { @@ -180,7 +178,8 @@ RdsRouteConfigProviderImpl::RdsRouteConfigProviderImpl( Server::Configuration::FactoryContext& factory_context) : subscription_(std::move(subscription)), config_update_info_(subscription_->routeConfigUpdate()), factory_context_(factory_context), - tls_(factory_context.threadLocal().allocateSlot()), config_update_callbacks_(factory_context.threadLocal().allocateSlot()) { + tls_(factory_context.threadLocal().allocateSlot()), + config_update_callbacks_(factory_context.threadLocal().allocateSlot()) { ConfigConstSharedPtr initial_config; if (config_update_info_->configInfo().has_value()) { initial_config = std::make_shared(config_update_info_->routeConfiguration(), @@ -205,14 +204,13 @@ Router::ConfigConstSharedPtr RdsRouteConfigProviderImpl::config() { return tls_->getTyped().config_; } -bool RdsRouteConfigProviderImpl::requestVirtualHostsUpdate(const std::string &for_domain, +bool RdsRouteConfigProviderImpl::requestVirtualHostsUpdate(const std::string& for_domain, std::function cb) { if (!config()->usesVhds()) { return false; } - // TODO check for an empty header? factory_context_.dispatcher().post( - [this, for_domain]() -> void { subscription_->ondemandUpdate({for_domain}); }); + [this, for_domain]() -> void { subscription_->ondemandUpdate({for_domain}); }); config_update_callbacks_->getTyped().callbacks_.push(cb); return true; @@ -221,16 +219,15 @@ bool RdsRouteConfigProviderImpl::requestVirtualHostsUpdate(const std::string &fo void RdsRouteConfigProviderImpl::onConfigUpdate() { ConfigConstSharedPtr new_config( new ConfigImpl(config_update_info_->routeConfiguration(), factory_context_, false)); - tls_->runOnAllThreads( - [this, new_config]() -> void { - tls_->getTyped().config_ = new_config; - auto callbacks = config_update_callbacks_->getTyped().callbacks_; - if (!callbacks.empty()) { - auto cb = callbacks.front(); - callbacks.pop(); - cb(); - } - }); + tls_->runOnAllThreads([this, new_config]() -> void { + tls_->getTyped().config_ = new_config; + auto callbacks = config_update_callbacks_->getTyped().callbacks_; + if (!callbacks.empty()) { + auto cb = callbacks.front(); + callbacks.pop(); + cb(); + } + }); } RouteConfigProviderManagerImpl::RouteConfigProviderManagerImpl(Server::Admin& admin) { diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 2024f5ce7065..9f24746946ea 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -68,7 +68,9 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider { } SystemTime lastUpdated() const override { return last_updated_; } void onConfigUpdate() override {} - bool requestVirtualHostsUpdate(const std::string &, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { + return false; + } private: ConfigConstSharedPtr config_; @@ -164,7 +166,7 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigSubscription& subscription() { return *subscription_; } void onConfigUpdate() override; - bool requestVirtualHostsUpdate(const std::string &for_domain, std::function cb); + bool requestVirtualHostsUpdate(const std::string& for_domain, std::function cb); // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; diff --git a/source/extensions/filters/http/on_demand/BUILD b/source/extensions/filters/http/on_demand/BUILD index 0ae69dbdad6b..c5a21f6307fb 100644 --- a/source/extensions/filters/http/on_demand/BUILD +++ b/source/extensions/filters/http/on_demand/BUILD @@ -18,9 +18,9 @@ envoy_cc_library( "//include/envoy/event:dispatcher_interface", "//include/envoy/http:filter_interface", "//include/envoy/server:filter_config_interface", - "//source/common/common:enum_to_int", - "//source/common/http:header_map_lib", "//source/common/common:assert_lib", + "//source/common/common:enum_to_int", "//source/common/http:codes_lib", + "//source/common/http:header_map_lib", ], ) diff --git a/source/extensions/filters/http/on_demand/on_demand_update.cc b/source/extensions/filters/http/on_demand/on_demand_update.cc index 111dc00b9b6e..25a7c590435b 100644 --- a/source/extensions/filters/http/on_demand/on_demand_update.cc +++ b/source/extensions/filters/http/on_demand/on_demand_update.cc @@ -14,7 +14,7 @@ void OnDemandRouteUpdate::requestRouteConfigUpdate() { filter_return_ = FilterReturn::ContinueDecoding; } else { auto config_update_scheduled = - callbacks_->requestRouteConfigUpdate([this]() -> void { onComplete(); }); + callbacks_->requestRouteConfigUpdate([this]() -> void { onRouteConfigUpdateCompletion(); }); filter_return_ = config_update_scheduled ? FilterReturn::StopDecoding : FilterReturn::ContinueDecoding; } @@ -40,7 +40,10 @@ void OnDemandRouteUpdate::setDecoderFilterCallbacks(Http::StreamDecoderFilterCal callbacks_ = &callbacks; } -void OnDemandRouteUpdate::onComplete() { +// This is the callback which is called when an update requested in requestRouteConfigUpdate() +// has been propagated to workers, at which point the request processing is restarted from the +// beginning. +void OnDemandRouteUpdate::onRouteConfigUpdateCompletion() { filter_return_ = FilterReturn::ContinueDecoding; if (!callbacks_->decodingBuffer() && // Redirects with body not yet supported. diff --git a/source/extensions/filters/http/on_demand/on_demand_update.h b/source/extensions/filters/http/on_demand/on_demand_update.h index 72cd8718f17c..a4842f68db02 100644 --- a/source/extensions/filters/http/on_demand/on_demand_update.h +++ b/source/extensions/filters/http/on_demand/on_demand_update.h @@ -13,16 +13,16 @@ class OnDemandRouteUpdate : public Http::StreamDecoderFilter { void requestRouteConfigUpdate(); - void onComplete(); + void onRouteConfigUpdateCompletion(); // Http::StreamDecoderFilter - Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap &headers, bool end_stream) override; + Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool end_stream) override; - Http::FilterDataStatus decodeData(Buffer::Instance &data, bool end_stream) override; + Http::FilterDataStatus decodeData(Buffer::Instance& data, bool end_stream) override; - Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap &trailers) override; + Http::FilterTrailersStatus decodeTrailers(Http::HeaderMap& trailers) override; - void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks &callbacks) override; + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; void onDestroy() override {} @@ -30,18 +30,14 @@ class OnDemandRouteUpdate : public Http::StreamDecoderFilter { // State of this filter's communication with the external authorization service. // The filter has either not started calling the external service, in the middle of calling // it or has completed. - enum class State { - NotStarted, Calling, Complete - }; + enum class State { NotStarted, Calling, Complete }; // FilterReturn is used to capture what the return code should be to the filter chain. // if this filter is either in the middle of calling the service or the result is denied then // the filter chain should stop. Otherwise the filter chain can continue to the next filter. - enum class FilterReturn { - ContinueDecoding, StopDecoding - }; + enum class FilterReturn { ContinueDecoding, StopDecoding }; - Http::StreamDecoderFilterCallbacks *callbacks_{}; + Http::StreamDecoderFilterCallbacks* callbacks_{}; State state_{State::NotStarted}; FilterReturn filter_return_{FilterReturn::ContinueDecoding}; }; diff --git a/source/extensions/filters/http/router/BUILD b/source/extensions/filters/http/router/BUILD index 2d80951c380d..2da7ba87a6b0 100644 --- a/source/extensions/filters/http/router/BUILD +++ b/source/extensions/filters/http/router/BUILD @@ -19,10 +19,10 @@ envoy_cc_library( "//include/envoy/registry", "//source/common/config:filter_json_lib", "//source/common/json:config_schemas_lib", - "//source/extensions/filters/http/on_demand:on_demand_update_lib", "//source/common/router:router_lib", "//source/common/router:shadow_writer_lib", "//source/extensions/filters/http:well_known_names", "//source/extensions/filters/http/common:factory_base_lib", + "//source/extensions/filters/http/on_demand:on_demand_update_lib", ], ) diff --git a/source/extensions/filters/http/router/config.cc b/source/extensions/filters/http/router/config.cc index bc4c095116d2..ef014359e983 100644 --- a/source/extensions/filters/http/router/config.cc +++ b/source/extensions/filters/http/router/config.cc @@ -5,10 +5,11 @@ #include "common/config/filter_json.h" #include "common/json/config_schemas.h" -#include "extensions/filters/http/on_demand/on_demand_update.h" #include "common/router/router.h" #include "common/router/shadow_writer_impl.h" +#include "extensions/filters/http/on_demand/on_demand_update.h" + namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -22,7 +23,8 @@ Http::FilterFactoryCb RouterFilterConfig::createFilterFactoryFromProtoTyped( proto_config)); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter(std::make_shared()); + callbacks.addStreamDecoderFilter( + std::make_shared()); callbacks.addStreamDecoderFilter(std::make_shared(*filter_config)); }; } diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 32e130e2d8a0..10937adea95f 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -155,7 +155,7 @@ class AdminImpl : public Admin, absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestVirtualHostsUpdate(const std::string &, std::function) { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) { return false; } Router::ConfigConstSharedPtr config_; TimeSource& time_source_; diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 6bbf57e377cc..549ac98e9af6 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -52,9 +52,7 @@ class FuzzConfig : public ConnectionManagerConfig { absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestConfigUpdate(const std::string, std::function) override { - return false; - } + bool requestConfigUpdate(const std::string, std::function) override { return false; } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index d62caa79c7b7..106c32c37bf1 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -72,9 +72,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestConfigUpdate(const std::string, std::function) override { - return false; - } + bool requestConfigUpdate(const std::string, std::function) override { return false; } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/integration/vhds_integration_test.cc b/test/integration/vhds_integration_test.cc index add0cd4c7a0a..9c0635f2de20 100644 --- a/test/integration/vhds_integration_test.cc +++ b/test/integration/vhds_integration_test.cc @@ -224,7 +224,7 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { sendDeltaDiscoveryResponse( Config::TypeUrl::get().VirtualHost, buildVirtualHost1(), {}, "2", vhds_stream_); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); testRouterHeaderOnlyRequestAndResponse(nullptr, 1, "/one", "vhost.first"); cleanupUpstreamAndDownstream(); @@ -237,7 +237,7 @@ TEST_P(VhdsIntegrationTest, VhdsVirtualHostAddUpdateRemove) { sendDeltaDiscoveryResponse( Config::TypeUrl::get().VirtualHost, {}, {"vhost_1", "vhost_2"}, "3", vhds_stream_); EXPECT_TRUE( - compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); + compareDeltaDiscoveryRequest(Config::TypeUrl::get().VirtualHost, {}, {}, vhds_stream_)); // an upstream request to an (now) unknown domain codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); From 01eca26f4060f8155efcddd3c6b76a49318f3be4 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 15:55:13 -0700 Subject: [PATCH 22/27] Fixing build issues Signed-off-by: Dmitri Dolguikh --- source/common/http/conn_manager_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index bbad87dbd9fd..f0b42c38c4ce 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -319,7 +319,7 @@ class ConnectionManagerImpl : Logger::Loggable, void responseDataTooLarge(); void responseDataDrained(); - bool requestRouteConfigUpdate(std::function) { return false; } + bool requestRouteConfigUpdate(std::function) override { return false; } StreamEncoderFilterSharedPtr handle_; }; From 72896bbf130046bb1d9c16beb66f15699fe9e619 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 15:57:56 -0700 Subject: [PATCH 23/27] Fixing build issues Signed-off-by: Dmitri Dolguikh --- source/common/config/delta_subscription_impl.cc | 3 ++- source/common/config/delta_subscription_state.cc | 13 ++++++++----- source/common/config/delta_subscription_state.h | 4 ++-- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/source/common/config/delta_subscription_impl.cc b/source/common/config/delta_subscription_impl.cc index d4da9e7e78e5..909f23b2e4f0 100644 --- a/source/common/config/delta_subscription_impl.cc +++ b/source/common/config/delta_subscription_impl.cc @@ -42,7 +42,8 @@ void DeltaSubscriptionImpl::updateResources(const std::set& update_ trySendDiscoveryRequests(); } -void DeltaSubscriptionImpl::updateResourcesViaAliases(const std::set& updates_to_these_aliases) { +void DeltaSubscriptionImpl::updateResourcesViaAliases( + const std::set& updates_to_these_aliases) { state_->updateResourceInterestViaAliases(updates_to_these_aliases); // Tell the server about our new interests, if there are any. trySendDiscoveryRequests(); diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index 09969e220eec..01b30bfbe2a0 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -76,7 +76,8 @@ void DeltaSubscriptionState::updateResourceInterest( } } -void DeltaSubscriptionState::updateResourceInterestViaAliases(const std::set& updates_to_these_aliases) { +void DeltaSubscriptionState::updateResourceInterestViaAliases( + const std::set& updates_to_these_aliases) { aliases_added_.insert(updates_to_these_aliases.begin(), updates_to_these_aliases.end()); } @@ -161,7 +162,7 @@ void DeltaSubscriptionState::handleEstablishmentFailure() { envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { envoy::api::v2::DeltaDiscoveryRequest request; - if(!any_request_sent_yet_in_current_stream_) { + if (!any_request_sent_yet_in_current_stream_) { populateDiscoveryRequest(request); } else if (!aliases_added_.empty()) { populateDiscoveryRequestWithAliases(request); @@ -174,19 +175,21 @@ envoy::api::v2::DeltaDiscoveryRequest DeltaSubscriptionState::getNextRequest() { return request; } -void DeltaSubscriptionState::populateDiscoveryRequestWithAliases(envoy::api::v2::DeltaDiscoveryRequest &request) { +void DeltaSubscriptionState::populateDiscoveryRequestWithAliases( + envoy::api::v2::DeltaDiscoveryRequest& request) { std::copy(aliases_added_.begin(), aliases_added_.end(), Protobuf::RepeatedFieldBackInserter(request.mutable_resource_names_subscribe())); aliases_added_.clear(); } -void DeltaSubscriptionState::populateDiscoveryRequest(envoy::api::v2::DeltaDiscoveryRequest &request) { +void DeltaSubscriptionState::populateDiscoveryRequest( + envoy::api::v2::DeltaDiscoveryRequest& request) { if (!any_request_sent_yet_in_current_stream_) { any_request_sent_yet_in_current_stream_ = true; // initial_resource_versions "must be populated for first request in a stream". // Also, since this might be a new server, we must explicitly state *all* of our subscription // interest. - for (auto const &resource : resource_versions_) { + for (auto const& resource : resource_versions_) { // Populate initial_resource_versions with the resource versions we currently have. // Resources we are interested in, but are still waiting to get any version of from the // server, do not belong in initial_resource_versions. (But do belong in new subscriptions!) diff --git a/source/common/config/delta_subscription_state.h b/source/common/config/delta_subscription_state.h index 128c24eb6ab5..08c02e9872ef 100644 --- a/source/common/config/delta_subscription_state.h +++ b/source/common/config/delta_subscription_state.h @@ -76,8 +76,8 @@ class DeltaSubscriptionState : public Logger::Loggable { void setResourceVersion(const std::string& resource_name, const std::string& resource_version); void setResourceWaitingForServer(const std::string& resource_name); void setLostInterestInResource(const std::string& resource_name); - void populateDiscoveryRequest(envoy::api::v2::DeltaDiscoveryRequest &request); - void populateDiscoveryRequestWithAliases(envoy::api::v2::DeltaDiscoveryRequest &request); + void populateDiscoveryRequest(envoy::api::v2::DeltaDiscoveryRequest& request); + void populateDiscoveryRequestWithAliases(envoy::api::v2::DeltaDiscoveryRequest& request); // A map from resource name to per-resource version. The keys of this map are exactly the resource // names we are currently interested in. Those in the waitingForServer state currently don't have From 2592cceeab72609f356add6c741d310c7b865e16 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 20:27:02 -0700 Subject: [PATCH 24/27] Fixing build issues Signed-off-by: Dmitri Dolguikh --- source/common/router/rds_impl.h | 2 +- source/server/http/admin.h | 2 +- test/common/http/conn_manager_impl_fuzz_test.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 3 ++- test/extensions/filters/http/router/config_test.cc | 10 +++++++--- .../network/http_connection_manager/config_test.cc | 12 ++++++------ 6 files changed, 18 insertions(+), 13 deletions(-) diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h index 9f24746946ea..f20edc95ec9f 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -166,7 +166,7 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, RdsRouteConfigSubscription& subscription() { return *subscription_; } void onConfigUpdate() override; - bool requestVirtualHostsUpdate(const std::string& for_domain, std::function cb); + bool requestVirtualHostsUpdate(const std::string& for_domain, std::function cb) override; // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 10937adea95f..dfb763b0ba28 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -155,7 +155,7 @@ class AdminImpl : public Admin, absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestVirtualHostsUpdate(const std::string&, std::function) { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { return false; } Router::ConfigConstSharedPtr config_; TimeSource& time_source_; diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 549ac98e9af6..2d16e2ed10d4 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -52,7 +52,7 @@ class FuzzConfig : public ConnectionManagerConfig { absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestConfigUpdate(const std::string, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { return false; } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 106c32c37bf1..6393a377f424 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -72,7 +72,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestConfigUpdate(const std::string, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { return false; } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; @@ -2919,6 +2919,7 @@ TEST_F(HttpConnectionManagerImplTest, FilterClearRouteCache) { EXPECT_CALL(*route_config_provider_.route_config_, route(_, _)) .WillOnce(Return(route1)) .WillOnce(Return(route2)) + .WillOnce(Return(nullptr)) .WillOnce(Return(nullptr)); EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) diff --git a/test/extensions/filters/http/router/config_test.cc b/test/extensions/filters/http/router/config_test.cc index a1d8e5a7dd03..e321e886bc23 100644 --- a/test/extensions/filters/http/router/config_test.cc +++ b/test/extensions/filters/http/router/config_test.cc @@ -29,7 +29,7 @@ TEST(RouterFilterConfigTest, RouterFilterInJson) { RouterFilterConfig factory; Http::FilterFactoryCb cb = factory.createFilterFactory(*json_config, "stats", context); Http::MockFilterChainFactoryCallbacks filter_callback; - EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)).Times(2); cb(filter_callback); } @@ -55,7 +55,11 @@ TEST(RouterFilterConfigTest, RouterV2Filter) { RouterFilterConfig factory; Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(router_config, "stats", context); Http::MockFilterChainFactoryCallbacks filter_callback; - EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)).Times(2 + + + + ); cb(filter_callback); } @@ -65,7 +69,7 @@ TEST(RouterFilterConfigTest, RouterFilterWithEmptyProtoConfig) { Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats", context); Http::MockFilterChainFactoryCallbacks filter_callback; - EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)).Times(2); cb(filter_callback); } 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 9974516f569f..13ba46d110a6 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -547,7 +547,7 @@ TEST_F(FilterChainTest, createFilterChain) { Http::MockFilterChainFactoryCallbacks callbacks; EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)); // Router + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate config.createFilterChain(callbacks); } @@ -565,7 +565,7 @@ TEST_F(FilterChainTest, createUpgradeFilterChain) { // WebSockets. { EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)); // Router + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate EXPECT_TRUE(config.createUpgradeFilterChain("WEBSOCKET", nullptr, callbacks)); } @@ -589,7 +589,7 @@ TEST_F(FilterChainTest, createUpgradeFilterChain) { // anything. { EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)); // Router + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate std::map upgrade_map; upgrade_map.emplace(std::make_pair("WebSocket", true)); EXPECT_TRUE(config.createUpgradeFilterChain("WEBSOCKET", &upgrade_map, callbacks)); @@ -657,19 +657,19 @@ TEST_F(FilterChainTest, createCustomUpgradeFilterChain) { { Http::MockFilterChainFactoryCallbacks callbacks; EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)); // Router + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate config.createFilterChain(callbacks); } { Http::MockFilterChainFactoryCallbacks callbacks; - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)); // Router + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate EXPECT_TRUE(config.createUpgradeFilterChain("websocket", nullptr, callbacks)); } { Http::MockFilterChainFactoryCallbacks callbacks; - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)); // Router + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate EXPECT_CALL(callbacks, addStreamFilter(_)).Times(2); // Dynamo EXPECT_TRUE(config.createUpgradeFilterChain("Foo", nullptr, callbacks)); } From e5d9d9426218b395c6a7ce5799981eb637cece76 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 20:31:15 -0700 Subject: [PATCH 25/27] Fixed formatting issues Signed-off-by: Dmitri Dolguikh --- source/server/http/admin.h | 4 +++- test/common/http/conn_manager_impl_fuzz_test.cc | 4 +++- test/common/http/conn_manager_impl_test.cc | 4 +++- test/extensions/filters/http/router/config_test.cc | 7 +++---- .../network/http_connection_manager/config_test.cc | 12 ++++++------ 5 files changed, 18 insertions(+), 13 deletions(-) diff --git a/source/server/http/admin.h b/source/server/http/admin.h index dfb763b0ba28..a56402cd3181 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -155,7 +155,9 @@ class AdminImpl : public Admin, absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestVirtualHostsUpdate(const std::string&, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { + return false; + } Router::ConfigConstSharedPtr config_; TimeSource& time_source_; diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 2d16e2ed10d4..251a83a8f00e 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -52,7 +52,9 @@ class FuzzConfig : public ConnectionManagerConfig { absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestVirtualHostsUpdate(const std::string&, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { + return false; + } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 6393a377f424..2e7f65ab515e 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -72,7 +72,9 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} - bool requestVirtualHostsUpdate(const std::string&, std::function) override { return false; } + bool requestVirtualHostsUpdate(const std::string&, std::function) override { + return false; + } TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/extensions/filters/http/router/config_test.cc b/test/extensions/filters/http/router/config_test.cc index e321e886bc23..752833adb362 100644 --- a/test/extensions/filters/http/router/config_test.cc +++ b/test/extensions/filters/http/router/config_test.cc @@ -55,11 +55,10 @@ TEST(RouterFilterConfigTest, RouterV2Filter) { RouterFilterConfig factory; Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(router_config, "stats", context); Http::MockFilterChainFactoryCallbacks filter_callback; - EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)).Times(2 + EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)) + .Times(2 - - - ); + ); cb(filter_callback); } 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 13ba46d110a6..dd0c4dbfcc01 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -546,7 +546,7 @@ TEST_F(FilterChainTest, createFilterChain) { date_provider_, route_config_provider_manager_); Http::MockFilterChainFactoryCallbacks callbacks; - EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo + EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate config.createFilterChain(callbacks); } @@ -564,7 +564,7 @@ TEST_F(FilterChainTest, createUpgradeFilterChain) { // config is present. We should create an upgrade filter chain for // WebSockets. { - EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo + EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate EXPECT_TRUE(config.createUpgradeFilterChain("WEBSOCKET", nullptr, callbacks)); } @@ -588,7 +588,7 @@ TEST_F(FilterChainTest, createUpgradeFilterChain) { // For paranoia's sake make sure route-specific enabling doesn't break // anything. { - EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo + EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate std::map upgrade_map; upgrade_map.emplace(std::make_pair("WebSocket", true)); @@ -656,7 +656,7 @@ TEST_F(FilterChainTest, createCustomUpgradeFilterChain) { { Http::MockFilterChainFactoryCallbacks callbacks; - EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo + EXPECT_CALL(callbacks, addStreamFilter(_)); // Dynamo EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate config.createFilterChain(callbacks); } @@ -669,8 +669,8 @@ TEST_F(FilterChainTest, createCustomUpgradeFilterChain) { { Http::MockFilterChainFactoryCallbacks callbacks; - EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate - EXPECT_CALL(callbacks, addStreamFilter(_)).Times(2); // Dynamo + EXPECT_CALL(callbacks, addStreamDecoderFilter(_)).Times(2); // Router + OnDemandRouteUpdate + EXPECT_CALL(callbacks, addStreamFilter(_)).Times(2); // Dynamo EXPECT_TRUE(config.createUpgradeFilterChain("Foo", nullptr, callbacks)); } } From 7bab93636570ceae84f93227abea43d2289041c3 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 29 May 2019 21:11:50 -0700 Subject: [PATCH 26/27] Fixing build issues Signed-off-by: Dmitri Dolguikh --- source/extensions/filters/http/on_demand/on_demand_update.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/source/extensions/filters/http/on_demand/on_demand_update.h b/source/extensions/filters/http/on_demand/on_demand_update.h index a4842f68db02..b635077de591 100644 --- a/source/extensions/filters/http/on_demand/on_demand_update.h +++ b/source/extensions/filters/http/on_demand/on_demand_update.h @@ -27,18 +27,12 @@ class OnDemandRouteUpdate : public Http::StreamDecoderFilter { void onDestroy() override {} private: - // State of this filter's communication with the external authorization service. - // The filter has either not started calling the external service, in the middle of calling - // it or has completed. - enum class State { NotStarted, Calling, Complete }; - // FilterReturn is used to capture what the return code should be to the filter chain. // if this filter is either in the middle of calling the service or the result is denied then // the filter chain should stop. Otherwise the filter chain can continue to the next filter. enum class FilterReturn { ContinueDecoding, StopDecoding }; Http::StreamDecoderFilterCallbacks* callbacks_{}; - State state_{State::NotStarted}; FilterReturn filter_return_{FilterReturn::ContinueDecoding}; }; From 852c51baf547cc73aa2bc9d2ede2727bf036f0f0 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Thu, 30 May 2019 08:42:56 -0700 Subject: [PATCH 27/27] Fixing build issues Signed-off-by: Dmitri Dolguikh --- .../envoy/router/route_config_update_info.h | 25 ------------------- source/common/router/rds_impl.cc | 3 ++- .../filters/http/on_demand/on_demand_update.h | 2 +- 3 files changed, 3 insertions(+), 27 deletions(-) delete mode 100644 include/envoy/router/route_config_update_info.h diff --git a/include/envoy/router/route_config_update_info.h b/include/envoy/router/route_config_update_info.h deleted file mode 100644 index 8cc22ed2e224..000000000000 --- a/include/envoy/router/route_config_update_info.h +++ /dev/null @@ -1,25 +0,0 @@ -#pragma once - -#include - -#include "envoy/api/v2/rds.pb.h" - -namespace Envoy { -namespace Router { - -struct LastConfigInfo { - uint64_t last_config_hash_; - std::string last_config_version_; -}; - -class RouteConfigUpdateInfo { -public: - virtual ~RouteConfigUpdateInfo() = default; - - virtual absl::optional configInfo() const PURE; - virtual envoy::api::v2::RouteConfiguration& routeConfiguration() PURE; - virtual SystemTime lastUpdated() const PURE; -}; - -} // namespace Router -} // namespace Envoy diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index 0e8be8548151..56f43ba63d3f 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -154,8 +154,9 @@ void RdsRouteConfigSubscription::onConfigUpdateFailed(const EnvoyException*) { } void RdsRouteConfigSubscription::ondemandUpdate(const std::set& aliases) { - if (vhds_subscription_.get() == nullptr) + if (vhds_subscription_.get() == nullptr) { return; + } vhds_subscription_->ondemandUpdate(aliases); } diff --git a/source/extensions/filters/http/on_demand/on_demand_update.h b/source/extensions/filters/http/on_demand/on_demand_update.h index b635077de591..44331e389126 100644 --- a/source/extensions/filters/http/on_demand/on_demand_update.h +++ b/source/extensions/filters/http/on_demand/on_demand_update.h @@ -9,7 +9,7 @@ namespace OnDemand { class OnDemandRouteUpdate : public Http::StreamDecoderFilter { public: - OnDemandRouteUpdate() {} + OnDemandRouteUpdate() = default; void requestRouteConfigUpdate();