From 955f9123ef48e1bde3c661bcc049df36a222c842 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Fri, 16 Aug 2019 22:12:17 +0000 Subject: [PATCH 1/3] rds: validate config in depth before update config dump Description: Risk Level: Low Testing: regression test Docs Changes: N/A Release Notes: N/A Fixes #7939 Signed-off-by: Lizan Zhou --- include/envoy/router/rds.h | 5 ++ .../router/route_config_update_receiver.h | 1 + source/common/router/rds_impl.cc | 11 +++ source/common/router/rds_impl.h | 4 +- source/server/http/admin.h | 1 + test/common/http/conn_manager_impl_common.h | 1 + test/common/router/rds_impl_test.cc | 87 +++++++++++++++++++ 7 files changed, 109 insertions(+), 1 deletion(-) diff --git a/include/envoy/router/rds.h b/include/envoy/router/rds.h index 9dcd4c3f64e5..456e44922043 100644 --- a/include/envoy/router/rds.h +++ b/include/envoy/router/rds.h @@ -48,6 +48,11 @@ class RouteConfigProvider { * Callback used to notify RouteConfigProvider about configuration changes. */ virtual void onConfigUpdate() PURE; + + /** + * Validate if the route configuration can be applied to the context of the route config provider. + */ + virtual void validateConfig(const envoy::api::v2::RouteConfiguration& config) const PURE; }; using RouteConfigProviderPtr = std::unique_ptr; diff --git a/include/envoy/router/route_config_update_receiver.h b/include/envoy/router/route_config_update_receiver.h index 8ac284fae6d4..6e4d492b1676 100644 --- a/include/envoy/router/route_config_update_receiver.h +++ b/include/envoy/router/route_config_update_receiver.h @@ -28,6 +28,7 @@ class RouteConfigUpdateReceiver { */ virtual bool onRdsUpdate(const envoy::api::v2::RouteConfiguration& rc, const std::string& version_info) PURE; + /** * Called on updates via VHDS. * @param added_resources supplies Resources (each containing a VirtualHost) that have been diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index e316460e0f09..e0aa13c94d00 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -99,6 +99,11 @@ void RdsRouteConfigSubscription::onConfigUpdate( throw EnvoyException(fmt::format("Unexpected RDS configuration (expecting {}): {}", route_config_name_, route_config.name())); } + for (auto* provider : route_config_providers_) { + // This seems inefficient, though it is neccessary to validate config in each context, + // especially when it comes with per_filter_config, + provider->validateConfig(route_config); + } if (config_update_info_->onRdsUpdate(route_config, version_info)) { stats_.config_reload_.inc(); @@ -198,6 +203,12 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() { [this, new_config]() -> void { tls_->getTyped().config_ = new_config; }); } +void RdsRouteConfigProviderImpl::validateConfig( + const envoy::api::v2::RouteConfiguration& config) const { + // TODO(lizan): consider cache the config here until onConfigUpdate. + ConfigImpl validation_config(config, factory_context_, false); +} + 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 6ec2c3e16047..2e8bbc61a5b4 100644 --- a/source/common/router/rds_impl.h +++ b/source/common/router/rds_impl.h @@ -66,6 +66,7 @@ class StaticRouteConfigProviderImpl : public RouteConfigProvider { } SystemTime lastUpdated() const override { return last_updated_; } void onConfigUpdate() override {} + void validateConfig(const envoy::api::v2::RouteConfiguration&) const override {} private: ConfigConstSharedPtr config_; @@ -159,7 +160,6 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, ~RdsRouteConfigProviderImpl() override; RdsRouteConfigSubscription& subscription() { return *subscription_; } - void onConfigUpdate() override; // Router::RouteConfigProvider Router::ConfigConstSharedPtr config() override; @@ -167,6 +167,8 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider, return config_update_info_->configInfo(); } SystemTime lastUpdated() const override { return config_update_info_->lastUpdated(); } + void onConfigUpdate() override; + void validateConfig(const envoy::api::v2::RouteConfiguration& config) const override; private: struct ThreadLocalConfig : public ThreadLocal::ThreadLocalObject { diff --git a/source/server/http/admin.h b/source/server/http/admin.h index e3806f58e188..09bf1ecedf4c 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -172,6 +172,7 @@ class AdminImpl : public Admin, absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} + void validateConfig(const envoy::api::v2::RouteConfiguration&) const override {} Router::ConfigConstSharedPtr config_; TimeSource& time_source_; diff --git a/test/common/http/conn_manager_impl_common.h b/test/common/http/conn_manager_impl_common.h index 1f68cc59412d..f7b8134dbb06 100644 --- a/test/common/http/conn_manager_impl_common.h +++ b/test/common/http/conn_manager_impl_common.h @@ -25,6 +25,7 @@ struct RouteConfigProvider : public Router::RouteConfigProvider { absl::optional configInfo() const override { return {}; } SystemTime lastUpdated() const override { return time_source_.systemTime(); } void onConfigUpdate() override {} + void validateConfig(const envoy::api::v2::RouteConfiguration&) const override {} TimeSource& time_source_; std::shared_ptr route_config_{new NiceMock()}; diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index e84bb925b367..6c124e3dbc0e 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -493,6 +493,93 @@ TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateWrongSize) { EnvoyException, "Unexpected RDS resource length: 2"); } +// Regression test for https://github.com/envoyproxy/envoy/issues/7939 +TEST_F(RouteConfigProviderManagerImplTest, ConfigDumpAfterConfigRejected) { + auto message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); + const auto& route_config_dump = + MessageUtil::downcastAndValidate( + *message_ptr); + + // No routes at all, no last_updated timestamp + envoy::admin::v2alpha::RoutesConfigDump expected_route_config_dump; + TestUtility::loadFromYaml(R"EOF( +static_route_configs: +dynamic_route_configs: +)EOF", + expected_route_config_dump); + EXPECT_EQ(expected_route_config_dump.DebugString(), route_config_dump.DebugString()); + + timeSystem().setSystemTime(std::chrono::milliseconds(1234567891234)); + + // dynamic. + setup(); + EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); + factory_context_.init_manager_.initialize(init_watcher_); + + const std::string response1_json = R"EOF( +{ + "version_info": "1", + "resources": [ + { + "@type": "type.googleapis.com/envoy.api.v2.RouteConfiguration", + "name": "foo_route_config", + "virtual_hosts": [ + { + "name": "integration", + "domains": [ + "*" + ], + "routes": [ + { + "match": { + "prefix": "/foo" + }, + "route": { + "cluster_header": ":authority" + } + } + ] + }, + { + "name": "duplicate", + "domains": [ + "*" + ], + "routes": [ + { + "match": { + "prefix": "/foo" + }, + "route": { + "cluster_header": ":authority" + } + } + ] + } + ] + } + ] +} +)EOF"; + auto response1 = TestUtility::parseYaml(response1_json); + + EXPECT_CALL(init_watcher_, ready()); + + EXPECT_THROW(rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), + EnvoyException); + + message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); + const auto& route_config_dump3 = + MessageUtil::downcastAndValidate( + *message_ptr); + TestUtility::loadFromYaml(R"EOF( +static_route_configs: +dynamic_route_configs: +)EOF", + expected_route_config_dump); + EXPECT_EQ(expected_route_config_dump.DebugString(), route_config_dump3.DebugString()); +} + } // namespace } // namespace Router } // namespace Envoy From e3effbdda29d2c7b41c605ee6830ffe6329e2ba2 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Sat, 17 Aug 2019 02:15:01 +0000 Subject: [PATCH 2/3] spell Signed-off-by: Lizan Zhou --- 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 e0aa13c94d00..d94910325cdb 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -100,7 +100,7 @@ void RdsRouteConfigSubscription::onConfigUpdate( route_config_name_, route_config.name())); } for (auto* provider : route_config_providers_) { - // This seems inefficient, though it is neccessary to validate config in each context, + // This seems inefficient, though it is necessary to validate config in each context, // especially when it comes with per_filter_config, provider->validateConfig(route_config); } From 9ef7697069caf9fd09e703107cf973593338b45d Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Wed, 21 Aug 2019 07:28:56 +0000 Subject: [PATCH 3/3] address comments Signed-off-by: Lizan Zhou --- test/common/router/rds_impl_test.cc | 73 ++++++++++------------------- 1 file changed, 26 insertions(+), 47 deletions(-) diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 6c124e3dbc0e..1046b5827be0 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -516,57 +516,36 @@ TEST_F(RouteConfigProviderManagerImplTest, ConfigDumpAfterConfigRejected) { EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); factory_context_.init_manager_.initialize(init_watcher_); - const std::string response1_json = R"EOF( -{ - "version_info": "1", - "resources": [ - { - "@type": "type.googleapis.com/envoy.api.v2.RouteConfiguration", - "name": "foo_route_config", - "virtual_hosts": [ - { - "name": "integration", - "domains": [ - "*" - ], - "routes": [ - { - "match": { - "prefix": "/foo" - }, - "route": { - "cluster_header": ":authority" - } - } - ] - }, - { - "name": "duplicate", - "domains": [ - "*" - ], - "routes": [ - { - "match": { - "prefix": "/foo" - }, - "route": { - "cluster_header": ":authority" - } - } - ] - } - ] - } - ] -} + const std::string response1_yaml = R"EOF( +version_info: '1' +resources: +- "@type": type.googleapis.com/envoy.api.v2.RouteConfiguration + name: foo_route_config + virtual_hosts: + - name: integration + domains: + - "*" + routes: + - match: + prefix: "/foo" + route: + cluster_header: ":authority" + - name: duplicate + domains: + - "*" + routes: + - match: + prefix: "/foo" + route: + cluster_header: ":authority" )EOF"; - auto response1 = TestUtility::parseYaml(response1_json); + auto response1 = TestUtility::parseYaml(response1_yaml); EXPECT_CALL(init_watcher_, ready()); - EXPECT_THROW(rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), - EnvoyException); + EXPECT_THROW_WITH_MESSAGE( + rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), + EnvoyException, "Only a single wildcard domain is permitted"); message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); const auto& route_config_dump3 =