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

stevenzzzz · 2019-12-16T21:12:57Z

Description: remove scopes first then add scopes to avoid scope key conflict when a new scope has the same key as an existing scope

Risk Level: MID
Testing: unit test
Docs Changes:
Release Notes:
Fixes: #9349

…a new scope has the same key as an existing scope. Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2019-12-17T03:36:52Z

/assign AndresGuedez

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

AndresGuedez

Thanks for working on this. A few minor comments.

AndresGuedez · 2019-12-17T15:24:46Z

source/common/router/scoped_rds.h

@@ -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.


Please update comment.

Please update comment.

oopps, done.

AndresGuedez · 2019-12-17T15:26:22Z

source/common/router/scoped_rds.cc

@@ -184,12 +184,15 @@ bool ScopedRdsConfigSubscription::addOrUpdateScopes(
  return any_applied;
 }

-bool ScopedRdsConfigSubscription::removeScopes(
+std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>


Consider using an alias for this type to improve readability.

it's only used within this module, I would leave it if you don't feel strongly.

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.

AndresGuedez · 2019-12-17T15:37:58Z

source/common/router/scoped_rds.cc

  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]));


Have you considered obtaining the iterator instead to avoid a second lookup in the erase()?

AndresGuedez · 2019-12-17T15:50:21Z

test/common/router/scoped_rds_test.cc

@@ -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.


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

AndresGuedez · 2019-12-17T16:09:11Z

test/common/router/scoped_rds_test.cc

@@ -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) {


nit:s/pushes/config updates

AndresGuedez · 2019-12-17T16:12:09Z

test/common/router/scoped_rds_test.cc

+                ->getRouteConfig(TestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}})
+                ->name(),
+            "foo_routes");
+}


I suggest also testing the edge case where the scope key is reused but the route_configuration_name is new.

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.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2019-12-17T21:35:20Z

/assign htuch

AndresGuedez · 2019-12-17T20:59:11Z

source/common/router/scoped_rds.cc

@@ -184,12 +184,15 @@ bool ScopedRdsConfigSubscription::addOrUpdateScopes(
  return any_applied;
 }

-bool ScopedRdsConfigSubscription::removeScopes(
+std::list<std::unique_ptr<ScopedRdsConfigSubscription::RdsRouteConfigProviderHelper>>


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.

htuch

Thanks!

* master: (167 commits) stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy (envoyproxy#8779) Make %UPSTREAM_LOCAL_ADDRESS% access-log format work for HTTP requests. (envoyproxy#9362) tools: API boosting support for using decls and enum constants. (envoyproxy#9418) Fix incorrect cluster InitializePhase type (envoyproxy#9379) build: fix merge race between envoyproxy#9241 and envoyproxy#9413. (envoyproxy#9427) fuzz: fix incorrect evaluator test (envoyproxy#9402) server: fix bogus startup log message (envoyproxy#9404) tools: Add protoxform tests (envoyproxy#9241) api: options after import (envoyproxy#9413) misc: use std::move instead of constructing a copy (envoyproxy#9415) tools: API boosting support for rewriting elaborated types. (envoyproxy#9375) docs: fix invalid transport_socket value (envoyproxy#9403) fix typo in docs (envoyproxy#9394) srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. (envoyproxy#9366) api: generate whole directory and sync (envoyproxy#9382) bazel: Add load statements for proto_library (envoyproxy#9367) Fix typo (envoyproxy#9388) Correct test of OptionsImpl argc type (Was: Correct type for std::array size() result) (envoyproxy#9290) http1 encode trailers in chunk encoding (envoyproxy#8667) Add mode to PipeInstance (envoyproxy#8423) ...

…void scope key conflict. (envoyproxy#9366) Remove scopes first then add scopes to avoid scope key conflict when a new scope has the same key as an existing scope Risk Level: MID Testing: unit test Docs Changes: Release Notes: Fixes: envoyproxy#9349 Signed-off-by: Xin Zhuang <stevenzzz@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

remove scopes first then add scopes to avoid scope key conflict when …

7af0f6c

…a new scope has the same key as an existing scope. Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz force-pushed the scoped-rds-conflict branch from e7748a5 to 7af0f6c Compare December 17, 2019 03:33

stevenzzzz changed the title ~~Scoped rds conflict~~ srds: remove to-de-removed scopes first and then apply additions to avoid scope conflict. Dec 17, 2019

stevenzzzz changed the title ~~srds: remove to-de-removed scopes first and then apply additions to avoid scope conflict.~~ srds: remove to-de-removed scopes first and then apply additions to avoid scope key conflict. Dec 17, 2019

repokitteh-read-only bot assigned AndresGuedez Dec 17, 2019

fix typo

5f78c17

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

AndresGuedez suggested changes Dec 17, 2019

View reviewed changes

add more test scenarios and feedback fixes

9e28188

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot assigned htuch Dec 17, 2019

AndresGuedez approved these changes Dec 18, 2019

View reviewed changes

htuch approved these changes Dec 18, 2019

View reviewed changes

htuch merged commit 58802ba into envoyproxy:master Dec 18, 2019

stevenzzzz deleted the scoped-rds-conflict branch June 25, 2020 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

stevenzzzz commented Dec 16, 2019 •

edited

Loading

stevenzzzz commented Dec 17, 2019

AndresGuedez left a comment

AndresGuedez Dec 17, 2019

stevenzzzz Dec 17, 2019

AndresGuedez Dec 17, 2019

stevenzzzz Dec 17, 2019

AndresGuedez Dec 17, 2019

AndresGuedez Dec 17, 2019

AndresGuedez Dec 17, 2019

stevenzzzz Dec 17, 2019

AndresGuedez Dec 17, 2019

stevenzzzz Dec 17, 2019

AndresGuedez Dec 17, 2019

stevenzzzz Dec 17, 2019

stevenzzzz commented Dec 17, 2019

AndresGuedez Dec 17, 2019

htuch left a comment

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

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

Conversation

stevenzzzz commented Dec 16, 2019 • edited Loading

stevenzzzz commented Dec 17, 2019

AndresGuedez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzzzz commented Dec 17, 2019

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

stevenzzzz commented Dec 16, 2019 •

edited

Loading