From 5d42b9b3171c8fa27a0f5eba96d02be78a4bbb8c Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Wed, 21 Aug 2019 10:00:31 -0700 Subject: [PATCH] rds: validate config in depth before update config dump (#7956) Route config need deep validation for virtual host duplication check, regex check, per filter config validation etc, which PGV wasn't enough. 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 | 66 +++++++++++++++++++ 7 files changed, 88 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..d94910325cdb 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 necessary 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..1046b5827be0 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -493,6 +493,72 @@ 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_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_yaml); + + EXPECT_CALL(init_watcher_, ready()); + + 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 = + 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