-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. #9366
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,12 +184,15 @@ bool ScopedRdsConfigSubscription::addOrUpdateScopes( | |
return any_applied; | ||
} | ||
|
||
bool ScopedRdsConfigSubscription::removeScopes( | ||
std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>> | ||
ScopedRdsConfigSubscription::removeScopes( | ||
const Protobuf::RepeatedPtrField<std::string>& scope_names, const std::string& version_info) { | ||
bool any_applied = false; | ||
std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>> | ||
to_be_removed_rds_providers; | ||
for (const auto& scope_name : scope_names) { | ||
auto iter = scoped_route_map_.find(scope_name); | ||
if (iter != scoped_route_map_.end()) { | ||
to_be_removed_rds_providers.emplace_back(std::move(route_provider_by_scope_[scope_name])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered obtaining the iterator instead to avoid a second lookup in the |
||
route_provider_by_scope_.erase(scope_name); | ||
scope_name_by_hash_.erase(iter->second->scopeKey().hash()); | ||
scoped_route_map_.erase(iter); | ||
|
@@ -200,17 +203,19 @@ bool ScopedRdsConfigSubscription::removeScopes( | |
thread_local_scoped_config->removeRoutingScope(scope_name); | ||
return config; | ||
}); | ||
any_applied = true; | ||
ENVOY_LOG(debug, "srds: remove scoped route '{}', version: {}", scope_name, version_info); | ||
} | ||
} | ||
return any_applied; | ||
return to_be_removed_rds_providers; | ||
} | ||
|
||
void ScopedRdsConfigSubscription::onConfigUpdate( | ||
const Protobuf::RepeatedPtrField<envoy::api::v2::Resource>& added_resources, | ||
const Protobuf::RepeatedPtrField<std::string>& removed_resources, | ||
const std::string& version_info) { | ||
// NOTE: deletes are done before adds/updates. | ||
|
||
absl::flat_hash_map<std::string, ScopedRouteInfoConstSharedPtr> to_be_removed_scopes; | ||
// If new route config sources come after the factory_context_.initManager()'s initialize() been | ||
// called, that initManager can't accept new targets. Instead we use a local override which will | ||
// start new subscriptions but not wait on them to be ready. | ||
|
@@ -247,12 +252,18 @@ void ScopedRdsConfigSubscription::onConfigUpdate( | |
} | ||
}); | ||
} | ||
|
||
std::vector<std::string> exception_msgs; | ||
bool any_applied = addOrUpdateScopes( | ||
added_resources, | ||
(noop_init_manager == nullptr ? factory_context_.initManager() : *noop_init_manager), | ||
version_info, exception_msgs); | ||
any_applied = removeScopes(removed_resources, version_info) || any_applied; | ||
// Do not delete RDS config providers just yet, in case the to be deleted RDS subscriptions could | ||
// be reused by some to be added scopes. | ||
std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>> | ||
to_be_removed_rds_providers = removeScopes(removed_resources, version_info); | ||
bool any_applied = | ||
addOrUpdateScopes( | ||
added_resources, | ||
(noop_init_manager == nullptr ? factory_context_.initManager() : *noop_init_manager), | ||
version_info, exception_msgs) || | ||
!to_be_removed_rds_providers.empty(); | ||
ConfigSubscriptionCommonBase::onConfigUpdate(); | ||
if (any_applied) { | ||
setLastConfigInfo(absl::optional<LastConfigInfo>({absl::nullopt, version_info})); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio | |
const ScopedRouteMap& scopedRouteMap() const { return scoped_route_map_; } | ||
|
||
private: | ||
// A helper class that takes care the life cycle management of a RDS route provider and the | ||
// A helper class that takes care of the life cycle management of a RDS route provider and the | ||
// update callback handle. | ||
struct RdsRouteConfigProviderHelper { | ||
RdsRouteConfigProviderHelper( | ||
|
@@ -133,8 +133,9 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio | |
std::vector<std::string>& exception_msgs); | ||
// Removes given scopes from the managed set of scopes. | ||
// Returns true if any scope updated, false otherwise. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please update comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
oopps, done. |
||
bool removeScopes(const Protobuf::RepeatedPtrField<std::string>& scope_names, | ||
const std::string& version_info); | ||
std::list<std::unique_ptr<RdsRouteConfigProviderHelper>> | ||
removeScopes(const Protobuf::RepeatedPtrField<std::string>& scope_names, | ||
const std::string& version_info); | ||
|
||
// Envoy::Config::DeltaConfigSubscriptionInstance | ||
void start() override { subscription_->start({}); } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -414,7 +414,7 @@ route_configuration_name: foo_routes | |
"foo_routes"); | ||
} | ||
|
||
// Tests that conflict resources are detected. | ||
// Tests that conflict resources in the same push are detected. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: 's/push/config update' to maintain consistency with Envoy terminology. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
TEST_F(ScopedRdsTest, MultipleResourcesWithKeyConflict) { | ||
setup(); | ||
|
||
|
@@ -477,6 +477,70 @@ route_configuration_name: foo_routes | |
"foo_routes"); | ||
} | ||
|
||
// Tests that scope-key conflict resources in different pushes are handled correctly. | ||
TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushes) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:s/pushes/config updates There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
setup(); | ||
|
||
const std::string config_yaml = R"EOF( | ||
name: foo_scope | ||
route_configuration_name: foo_routes | ||
key: | ||
fragments: | ||
- string_key: x-foo-key | ||
)EOF"; | ||
Protobuf::RepeatedPtrField<ProtobufWkt::Any> resources; | ||
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml); | ||
EXPECT_CALL(context_init_manager_, state()).WillOnce(Return(Init::Manager::State::Uninitialized)); | ||
EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "1")); | ||
EXPECT_EQ( | ||
1UL, | ||
factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); | ||
// Scope key "x-foo-key" points to nowhere. | ||
EXPECT_NE(getScopedRdsProvider(), nullptr); | ||
EXPECT_NE(getScopedRdsProvider()->config<ScopedConfigImpl>(), nullptr); | ||
// No RDS "foo_routes" config push happened yet, Router::NullConfig is returned. | ||
EXPECT_THAT(getScopedRdsProvider() | ||
->config<ScopedConfigImpl>() | ||
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) | ||
->name(), | ||
""); | ||
context_init_manager_.initialize(init_watcher_); | ||
init_watcher_.expectReady().Times( | ||
2); // SRDS "foo_scope" and RDS "foo_routes"(though no real push). | ||
pushRdsConfig("foo_routes", "111"); | ||
EXPECT_EQ(server_factory_context_.scope_.counter("foo.rds.foo_routes.config_reload").value(), | ||
1UL); | ||
EXPECT_EQ(getScopedRdsProvider() | ||
->config<ScopedConfigImpl>() | ||
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) | ||
->name(), | ||
"foo_routes"); | ||
const std::string config_yaml2 = R"EOF( | ||
name: foo_scope2 | ||
route_configuration_name: foo_routes | ||
key: | ||
fragments: | ||
- string_key: x-foo-key | ||
)EOF"; | ||
resources.Clear(); | ||
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2); | ||
EXPECT_CALL(context_init_manager_, state()).WillOnce(Return(Init::Manager::State::Initialized)); | ||
EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "2")); | ||
EXPECT_EQ( | ||
2UL, | ||
factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value()); | ||
// foo_scope is deleted, and foo_scope2 is added. | ||
EXPECT_EQ(getScopedRouteMap().size(), 1UL); | ||
EXPECT_EQ(getScopedRouteMap().count("foo_scope"), 0); | ||
EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1); | ||
// The same scope-key now points to the same route table. | ||
EXPECT_EQ(getScopedRdsProvider() | ||
->config<ScopedConfigImpl>() | ||
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}) | ||
->name(), | ||
"foo_routes"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest also testing the edge case where the scope key is reused but the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a scenario where new scope has the same key but different route-table name that will (conflict/remove-old-add-new) with existing scope. |
||
|
||
// Tests that only one resource is provided during a config update. | ||
TEST_F(ScopedRdsTest, InvalidDuplicateResourceSotw) { | ||
setup(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using an alias for this type to improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's only used within this module, I would leave it if you don't feel strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth it, this is a long type and it's repeated in several places; however, I'll leave it up to the final reviewer.