From 7a64f89463d5de73192927fa71a5bc1280131359 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Tue, 10 Dec 2024 08:58:16 -0500 Subject: [PATCH] rds: normalize rds provider's config before calculating hash (#37180) Signed-off-by: Christoph Pakulski --- changelogs/current.yaml | 8 +++ .../rds/route_config_provider_manager.cc | 19 ------- .../rds/route_config_provider_manager.h | 36 +++++++++++++- source/common/runtime/runtime_features.cc | 1 + test/common/router/rds_impl_test.cc | 49 +++++++++++++++++++ tools/spelling/spelling_dictionary.txt | 1 + 6 files changed, 93 insertions(+), 21 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f61294c58d29..ed4e6baac66f 100644 --- a/changelogs/current.yaml +++ b/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 `, + 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* diff --git a/source/common/rds/route_config_provider_manager.cc b/source/common/rds/route_config_provider_manager.cc index 9abda6d10e88..f4eae5ddf211 100644 --- a/source/common/rds/route_config_provider_manager.cc +++ b/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(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, diff --git a/source/common/rds/route_config_provider_manager.h b/source/common/rds/route_config_provider_manager.h index c527ae60f819..b4acbc170803 100644 --- a/source/common/rds/route_config_provider_manager.h +++ b/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 create_static_provider); + + template 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( 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(rds).mutable_config_source()->release_initial_fetch_timeout(); + manager_identifier = MessageUtil::hash(rds); + const_cast(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 diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 230631181721..5f02c869ca37 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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); diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index c3a2148f3ea0..5d9ce3db6426 100644 --- a/test/common/router/rds_impl_test.cc +++ b/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({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 diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 6fa48779d2b1..ec5bdab317bf 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -41,6 +41,7 @@ Bencoded CIO cbegin cend +constness deadcode DFP Dynatrace