Skip to content
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.

Sign up for GitHub

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

Jump to bottom

srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. #9366

Merged
merged 3 commits into from
Dec 18, 2019
Merged

srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. #9366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7af0f6c
remove scopes first then add scopes to avoid scope key conflict when …
stevenzzzz Dec 17, 2019
5f78c17
fix typo
stevenzzzz Dec 17, 2019
9e28188
add more test scenarios and feedback fixes
stevenzzzz Dec 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions source/common/router/scoped_rds.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,20 @@ bool ScopedRdsConfigSubscription::addOrUpdateScopes(
return any_applied;
}

bool ScopedRdsConfigSubscription::removeScopes(
std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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()) {
route_provider_by_scope_.erase(scope_name);
auto rds_config_provider_helper_iter = route_provider_by_scope_.find(scope_name);
if (rds_config_provider_helper_iter != route_provider_by_scope_.end()) {
to_be_removed_rds_providers.emplace_back(
std::move(rds_config_provider_helper_iter->second));
route_provider_by_scope_.erase(rds_config_provider_helper_iter);
}
scope_name_by_hash_.erase(iter->second->scopeKey().hash());
scoped_route_map_.erase(iter);
applyConfigUpdate([scope_name](ConfigProvider::ConfigConstSharedPtr config)
Expand All @@ -200,17 +207,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.
Expand Down Expand Up @@ -247,12 +256,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}));
Expand Down
10 changes: 6 additions & 4 deletions source/common/router/scoped_rds.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -132,9 +132,11 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio
Init::Manager& init_manager, const std::string& version_info,
std::vector<std::string>& exception_msgs);
// Removes given scopes from the managed set of scopes.
// Returns true if any scope updated, false otherwise.
bool removeScopes(const Protobuf::RepeatedPtrField<std::string>& scope_names,
const std::string& version_info);
// Returns a list of to be removed helpers which is temporally held in the onConfigUpdate method,
// to make sure new scopes sharing the same RDS source configs could reuse the subscriptions.
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({}); }
Expand Down
150 changes: 141 additions & 9 deletions test/common/router/scoped_rds_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ name: foo_scoped_routes

// Helper function which pushes an update to given RDS subscription, the start(_) of the
// subscription must have been called.
void pushRdsConfig(const std::string& route_config_name, const std::string& version) {
void pushRdsConfig(const std::vector<std::string>& route_config_names,
const std::string& version) {
const std::string route_config_tmpl = R"EOF(
name: {}
virtual_hosts:
Expand All @@ -187,10 +188,12 @@ name: foo_scoped_routes
- match: {{ prefix: "/" }}
route: {{ cluster: bluh }}
)EOF";
Protobuf::RepeatedPtrField<ProtobufWkt::Any> resources;
resources.Add()->PackFrom(TestUtility::parseYaml<envoy::api::v2::RouteConfiguration>(
fmt::format(route_config_tmpl, route_config_name)));
rds_subscription_by_name_[route_config_name]->onConfigUpdate(resources, version);
for (const std::string& name : route_config_names) {
Protobuf::RepeatedPtrField<ProtobufWkt::Any> resources;
resources.Add()->PackFrom(TestUtility::parseYaml<envoy::api::v2::RouteConfiguration>(
fmt::format(route_config_tmpl, name)));
rds_subscription_by_name_[name]->onConfigUpdate(resources, version);
}
}

ScopedRdsConfigProvider* getScopedRdsProvider() const {
Expand Down Expand Up @@ -304,7 +307,7 @@ route_configuration_name: foo_routes
->name(),
"");
// RDS updates foo_routes.
pushRdsConfig("foo_routes", "111");
pushRdsConfig({"foo_routes"}, "111");
EXPECT_EQ(getScopedRdsProvider()
->config<ScopedConfigImpl>()
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})
Expand Down Expand Up @@ -381,7 +384,7 @@ route_configuration_name: foo_routes
->name(),
"");
// RDS updates foo_routes.
pushRdsConfig("foo_routes", "111");
pushRdsConfig({"foo_routes"}, "111");
EXPECT_EQ(getScopedRdsProvider()
->config<ScopedConfigImpl>()
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})
Expand Down Expand Up @@ -414,7 +417,7 @@ route_configuration_name: foo_routes
"foo_routes");
}

// Tests that conflict resources are detected.
// Tests that conflict resources in the same push are detected.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 's/push/config update' to maintain consistency with Envoy terminology.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

TEST_F(ScopedRdsTest, MultipleResourcesWithKeyConflict) {
setup();

Expand Down Expand Up @@ -467,7 +470,7 @@ route_configuration_name: foo_routes
EXPECT_EQ(getScopedRouteMap().size(), 1UL);
EXPECT_EQ(getScopedRouteMap().count("foo_scope"), 1);
// Scope key "x-foo-key" points to foo_routes due to partial rejection.
pushRdsConfig("foo_routes", "111"); // Push some real route configuration.
pushRdsConfig({"foo_routes"}, "111"); // Push some real route configuration.
EXPECT_EQ(1UL,
server_factory_context_.scope_.counter("foo.rds.foo_routes.config_reload").value());
EXPECT_EQ(getScopedRdsProvider()
Expand All @@ -477,6 +480,135 @@ route_configuration_name: foo_routes
"foo_routes");
}

// Tests that scope-key conflict resources in different config updates are handled correctly.
TEST_F(ScopedRdsTest, ScopeKeyReuseInDifferentPushes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:s/pushes/config updates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

setup();

const std::string config_yaml1 = R"EOF(
name: foo_scope1
route_configuration_name: foo_routes
key:
fragments:
- string_key: x-foo-key
)EOF";
const std::string config_yaml2 = R"EOF(
name: foo_scope2
route_configuration_name: bar_routes
key:
fragments:
- string_key: x-bar-key
)EOF";
Protobuf::RepeatedPtrField<ProtobufWkt::Any> resources;
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml1);
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2);
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(
3); // SRDS "foo_scope1" and RDS "foo/bar_routes"(though no real push).
pushRdsConfig({"foo_routes", "bar_routes"}, "111");
EXPECT_EQ(server_factory_context_.scope_.counter("foo.rds.foo_routes.config_reload").value(),
1UL);
EXPECT_EQ(server_factory_context_.scope_.counter("foo.rds.bar_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_yaml3 = R"EOF(
name: foo_scope3
route_configuration_name: foo_routes
key:
fragments:
- string_key: x-foo-key
)EOF";
resources.Clear();

// Remove foo_scope1 and add a new scope3 reuses the same scope_key.
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2);
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml3);
EXPECT_CALL(context_init_manager_, state())
.Times(2)
.WillRepeatedly(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(), 2UL);
EXPECT_EQ(getScopedRouteMap().count("foo_scope1"), 0);
EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1);
EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 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");

// Push a new scope foo_scope4 with the same key as foo_scope2 but a different route-table, this
// ends in an exception.
const std::string config_yaml4 = R"EOF(
name: foo_scope4
route_configuration_name: foo_routes
key:
fragments:
- string_key: x-bar-key
)EOF";
resources.Clear();
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml2);
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml3);
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml4);
EXPECT_THROW_WITH_REGEX(
srds_subscription_->onConfigUpdate(resources, "3"), EnvoyException,
"scope key conflict found, first scope is 'foo_scope2', second scope is 'foo_scope4'");
EXPECT_EQ(getScopedRouteMap().size(), 2UL);
EXPECT_EQ(getScopedRouteMap().count("foo_scope1"), 0);
EXPECT_EQ(getScopedRouteMap().count("foo_scope2"), 1);
EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1);
EXPECT_EQ(getScopedRdsProvider()
->config<ScopedConfigImpl>()
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})
->name(),
"bar_routes");

// Delete foo_scope2, and push a new foo_scope4 with the same scope key but different route-table.
resources.Clear();
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml3);
parseScopedRouteConfigurationFromYaml(*resources.Add(), config_yaml4);
EXPECT_NO_THROW(srds_subscription_->onConfigUpdate(resources, "4"));
EXPECT_EQ(
factory_context_.scope_.counter("foo.scoped_rds.foo_scoped_routes.config_reload").value(),
3UL);
EXPECT_EQ(getScopedRouteMap().size(), 2UL);
EXPECT_EQ(getScopedRouteMap().count("foo_scope3"), 1);
EXPECT_EQ(getScopedRouteMap().count("foo_scope4"), 1);
EXPECT_EQ(getScopedRdsProvider()
->config<ScopedConfigImpl>()
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}})
->name(),
"foo_routes");
EXPECT_EQ(getScopedRdsProvider()
->config<ScopedConfigImpl>()
->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})
->name(),
"foo_routes");
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 route_configuration_name is new.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
Expand Down