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 <lizan@tetrate.io>

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<RouteConfigProvider>;

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

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<ThreadLocalConfig>().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(); });

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,14 +160,15 @@ class RdsRouteConfigProviderImpl : public RouteConfigProvider,
   ~RdsRouteConfigProviderImpl() override;
 
   RdsRouteConfigSubscription& subscription() { return *subscription_; }
-  void onConfigUpdate() override;
 
   // Router::RouteConfigProvider
   Router::ConfigConstSharedPtr config() override;
   absl::optional<ConfigInfo> configInfo() const override {
     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 {

source/server/http/admin.h

@@ -172,6 +172,7 @@ class AdminImpl : public Admin,
     absl::optional<ConfigInfo> 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_;

test/common/http/conn_manager_impl_common.h

@@ -25,6 +25,7 @@ struct RouteConfigProvider : public Router::RouteConfigProvider {
   absl::optional<ConfigInfo> 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<Router::MockConfig> route_config_{new NiceMock<Router::MockConfig>()};

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<const envoy::admin::v2alpha::RoutesConfigDump&>(
+          *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<envoy::api::v2::DiscoveryResponse>(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<const envoy::admin::v2alpha::RoutesConfigDump&>(
+          *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