backport to 1.31: rds: normalize rds provider's config before calcula…

…ting hash (#37180) (#37844)

Backport #37180 to release 1.31
Additional Description:
This is not a security fix, but affects certain users and requires
expensive work-around.
Identical to backport to 1.32: #37749 
Risk Level: Low
Testing: Added unit test
Docs Changes: No
Release Notes: Yes
Platform Specific Features: No
Runtime guard: Yes

Signed-off-by: Christoph Pakulski <[email protected]>

changelogs/current.yaml

@@ -5,6 +5,14 @@ behavior_changes:
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
+- area: rds
+  change: |
+    When a new RDS provider config is pushed via xDS and the only difference is change to
+    :ref:`initial_fetch_timeout <envoy_v3_api_field_config.core.v3.ConfigSource.initial_fetch_timeout>`,
+    the already existing provider will be reused. Envoy will not ask RDS server for routes
+    config because existing provider already has up to date routes config.
+    This behavioral change can be enabled by setting runtime guard
+    ``envoy.reloadable_features.normalize_rds_provider_config`` to true.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

source/common/rds/route_config_provider_manager.cc

@@ -75,25 +75,6 @@ RouteConfigProviderPtr RouteConfigProviderManager::addStaticProvider(
   return provider;
 }
 
-RouteConfigProviderSharedPtr RouteConfigProviderManager::addDynamicProvider(
-    const Protobuf::Message& rds, const std::string& route_config_name, Init::Manager& init_manager,
-    std::function<
-        std::pair<RouteConfigProviderSharedPtr, const Init::Target*>(uint64_t manager_identifier)>
-        create_dynamic_provider) {
-  // RdsRouteConfigSubscriptions are unique based on their serialized RDS config.
-  const uint64_t manager_identifier = MessageUtil::hash(rds);
-  auto existing_provider =
-      reuseDynamicProvider(manager_identifier, init_manager, route_config_name);
-
-  if (existing_provider) {
-    return existing_provider;
-  }
-  auto new_provider = create_dynamic_provider(manager_identifier);
-  init_manager.add(*new_provider.second);
-  dynamic_route_config_providers_.insert({manager_identifier, new_provider});
-  return new_provider.first;
-}
-
 RouteConfigProviderSharedPtr
 RouteConfigProviderManager::reuseDynamicProvider(uint64_t manager_identifier,
                                                  Init::Manager& init_manager,

source/common/rds/route_config_provider_manager.h

@@ -12,6 +12,7 @@
 
 #include "source/common/common/matchers.h"
 #include "source/common/protobuf/utility.h"
+#include "source/common/runtime/runtime_features.h"
 
 #include "absl/container/node_hash_map.h"
 #include "absl/container/node_hash_set.h"
@@ -34,12 +35,43 @@ class RouteConfigProviderManager {
 
   RouteConfigProviderPtr
   addStaticProvider(std::function<RouteConfigProviderPtr()> create_static_provider);
+
+  template <class RdsConfig>
   RouteConfigProviderSharedPtr
-  addDynamicProvider(const Protobuf::Message& rds, const std::string& route_config_name,
+  addDynamicProvider(const RdsConfig& rds, const std::string& route_config_name,
                      Init::Manager& init_manager,
                      std::function<std::pair<RouteConfigProviderSharedPtr, const Init::Target*>(
                          uint64_t manager_identifier)>
-                         create_dynamic_provider);
+                         create_dynamic_provider) {
+
+    uint64_t manager_identifier;
+
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.normalize_rds_provider_config")) {
+      // Normalize the config_source part of the passed config. Some parts of the config_source
+      // do not affect selection of the RDS provider. They will be cleared (zeroed) and restored
+      // after calculating hash.
+      // Since rds is passed as const, the constness must be casted away before modifying rds.
+      auto* orig_initial_timeout =
+          const_cast<RdsConfig&>(rds).mutable_config_source()->release_initial_fetch_timeout();
+      manager_identifier = MessageUtil::hash(rds);
+      const_cast<RdsConfig&>(rds).mutable_config_source()->set_allocated_initial_fetch_timeout(
+          orig_initial_timeout);
+
+    } else {
+      manager_identifier = MessageUtil::hash(rds);
+    }
+
+    auto existing_provider =
+        reuseDynamicProvider(manager_identifier, init_manager, route_config_name);
+
+    if (existing_provider) {
+      return existing_provider;
+    }
+    auto new_provider = create_dynamic_provider(manager_identifier);
+    init_manager.add(*new_provider.second);
+    dynamic_route_config_providers_.insert({manager_identifier, new_provider});
+    return new_provider.first;
+  }
 
 private:
   // TODO(jsedgwick) These two members are prime candidates for the owned-entry list/map

source/common/runtime/runtime_features.cc

@@ -167,6 +167,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_explicit_internal_address_config);
 // A flag to set the maximum TLS version for google_grpc client to TLS1.2, when needed for
 // compliance restrictions.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_google_grpc_disable_tls_13);
+// A flag to enable normalization of RDS provider config. (see PR 37180).
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_normalize_rds_provider_config);
 
 // Block of non-boolean flags. Use of int flags is deprecated. Do not add more.
 ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT

test/common/router/rds_impl_test.cc

@@ -1153,6 +1153,55 @@ version_info: '1'
   EXPECT_EQ(expected_route_config_dump.DebugString(), route_config_dump3.DebugString());
 }
 
+TEST_F(RouteConfigProviderManagerImplTest, NormalizeDynamicProviderConfig) {
+  setup();
+
+  const auto route_config = parseRouteConfigurationFromV3Yaml(R"EOF(
+name: foo_route_config
+virtual_hosts:
+  - name: bar
+    domains: ["*"]
+    routes:
+      - match: { prefix: "/" }
+        route: { cluster: baz }
+)EOF");
+  const auto decoded_resources = TestUtility::decodeResources({route_config});
+
+  EXPECT_TRUE(server_factory_context_.cluster_manager_.subscription_factory_.callbacks_
+                  ->onConfigUpdate(decoded_resources.refvec_, "1")
+                  .ok());
+
+  UniversalStringMatcher universal_name_matcher;
+  EXPECT_EQ(1UL, route_config_provider_manager_->dumpRouteConfigs(universal_name_matcher)
+                     ->dynamic_route_configs()
+                     .size());
+
+  for (bool normalize_config : std::vector<bool>({true, false})) {
+    Runtime::maybeSetRuntimeGuard("envoy.reloadable_features.normalize_rds_provider_config",
+                                  normalize_config);
+    envoy::extensions::filters::network::http_connection_manager::v3::Rds rds2;
+    rds2 = rds_;
+    // The following is valid only when normalize_config is true:
+    // Modify parameters which should not affect the provider. In other words, the same provider
+    // should be picked, regardless of the fact that initial_fetch_timeout is different for both
+    // configs.
+    rds2.mutable_config_source()->mutable_initial_fetch_timeout()->set_seconds(
+        rds_.config_source().initial_fetch_timeout().seconds() + 1);
+
+    RouteConfigProviderSharedPtr provider2 =
+        route_config_provider_manager_->createRdsRouteConfigProvider(
+            rds2, server_factory_context_, "foo_prefix", outer_init_manager_);
+
+    EXPECT_TRUE(server_factory_context_.cluster_manager_.subscription_factory_.callbacks_
+                    ->onConfigUpdate(decoded_resources.refvec_, "provider2")
+                    .ok());
+    EXPECT_EQ(normalize_config ? 1UL : 2UL,
+              route_config_provider_manager_->dumpRouteConfigs(universal_name_matcher)
+                  ->dynamic_route_configs()
+                  .size());
+  }
+}
+
 } // namespace
 } // namespace Router
 } // namespace Envoy

tools/spelling/spelling_dictionary.txt

@@ -38,6 +38,7 @@ BPF
 Bdecoded
 Bencoded
 CIO
+constness
 deadcode
 DFP
 Dynatrace