Skip to content

Commit

Permalink
Backport to 1.32:
Browse files Browse the repository at this point in the history
rds: normalize rds provider's config before calculating hash (#37180)

Signed-off-by: Christoph Pakulski <[email protected]>
  • Loading branch information
cpakulski committed Dec 18, 2024
1 parent b655872 commit c8a9439
Show file tree
Hide file tree
Showing 6 changed files with 93 additions and 21 deletions.
8 changes: 8 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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 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*
Expand Down
19 changes: 0 additions & 19 deletions source/common/rds/route_config_provider_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
36 changes: 34 additions & 2 deletions source/common/rds/route_config_provider_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
49 changes: 49 additions & 0 deletions test/common/router/rds_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions tools/spelling/spelling_dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Bencoded
CIO
cbegin
cend
constness
deadcode
DFP
Dynatrace
Expand Down

0 comments on commit c8a9439

Please sign in to comment.