rds: normalize rds provider's config before calculating hash (#37180)

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

changelogs/current.yaml

@@ -91,6 +91,14 @@ minor_behavior_changes:
 - area: rate_limit
   change: |
     add ``WEEK`` to the unit of time for rate limit.
+- 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 temporarily reverted by setting runtime guard
+    ``envoy.reloadable_features.normalize_rds_provider_config`` to false.
 
 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

@@ -67,6 +67,7 @@ RUNTIME_GUARD(envoy_reloadable_features_lua_flow_control_while_http_call);
 RUNTIME_GUARD(envoy_reloadable_features_mmdb_files_reload_enabled);
 RUNTIME_GUARD(envoy_reloadable_features_no_extension_lookup_by_name);
 RUNTIME_GUARD(envoy_reloadable_features_no_timer_based_rate_limit_token_bucket);
+RUNTIME_GUARD(envoy_reloadable_features_normalize_rds_provider_config);
 RUNTIME_GUARD(envoy_reloadable_features_oauth2_use_refresh_token);
 RUNTIME_GUARD(envoy_reloadable_features_original_dst_rely_on_idle_timeout);
 RUNTIME_GUARD(envoy_reloadable_features_prefer_ipv6_dns_on_macos);

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

@@ -41,6 +41,7 @@ Bencoded
 CIO
 cbegin
 cend
+constness
 deadcode
 DFP
 Dynatrace