diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce..72d058df76f8 100644 --- a/changelogs/current.yaml +++ b/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 `, + 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 babfb11c2a0a..32857af69b0f 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -72,6 +72,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_original_dst_rely_on_idle_timeout); RUNTIME_GUARD(envoy_reloadable_features_prefer_ipv6_dns_on_macos); RUNTIME_GUARD(envoy_reloadable_features_proxy_104); 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 cd5d25bc3fbb..c7e2205b1281 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