router: scoped rds (2c): implement scoped rds API by AndresGuedez · Pull Request #6932 · envoyproxy/envoy

AndresGuedez · 2019-05-14T14:07:48Z

Implement the scoped RDS (SRDS) API config subscription and provider based on the config protos introduced in #6675 and the ConfigProvider framework introduced in #5243 and #6781.

NOTES:

See parent PR router: scoped RDS (2/4): support scoped routing configuration #5839 for full context into these changes. PRs 2a (router: scoped rds (2a): scoped routing configuration protos #6675) and 2b (config: scoped rds (2b): support delta APIs in ConfigProvider framework #6781) have already been merged.
The API is not yet fully implemented. This PR introduces static and dynamic (xDS config subscription) handling of scoped routing configuration, but the new L7 multi tenant routing logic (see Support multi tenant route configuration selection based on HTTP attributes #4704) has not yet been introduced.
The API is not yet plumbed into the HttpConnectionManager, that will be done in the next PR.
This PR includes unit tests only; integration tests will follow in the next PR.

Risk Level: Low (this DS API is not yet integrated into the HCM and can not be enabled via config).
Testing: Unit tests added.
Docs Changes: N/A.

This commit introduces: - Support for scoped routing configuration parsing. - Inline and dynamic config providers using the ConfigProvider framework. - Unit and integration testing scaffolding. This is the second step in the PR chain for envoyproxy#4704. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

…-dynamic Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

- Top level SRDS resource is now called a 'ScopedRouteConfiguration'. - SRDS now operates like CDS where the full set of resources received via the DS builds the complete configuration state. - Modified the ConfigProvider framework to support delta APIs such as SRDS and CDS - Scoped RDS unit test mostly passing, integration tests have not yet been migrated to new API Signed-off-by: Andres Guedez <aguedez@google.com>

Also, cleanup. Signed-off-by: Andres Guedez <aguedez@google.com>

- Readability cleanup in various places. - Improve integration tests. - Add 'name' field to ScopedRoutes proto. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

…-dynamic Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

…-dynamic Signed-off-by: Andres Guedez <aguedez@google.com>

- Refactor the framework to create a cleaner, more cohesive set of abstractions based on the ConfigProvider::ApiType of the DS API. - Rename base classes for consistency and clarity. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

…scoped-rds-config-provider Signed-off-by: Andres Guedez <aguedez@google.com>

…-provider Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez · 2019-05-14T14:09:07Z

@stevenzzzz FYI

AndresGuedez · 2019-05-14T18:40:16Z

@htuch PTAL (or reassign as appropriate)

Signed-off-by: Andres Guedez <aguedez@google.com>

htuch · 2019-05-14T22:23:27Z

@AndresGuedez would be happy to take a pass, but curious also to get a review from @fredlas and @dmitri-d as well, since they have been doing a lot more work in the depths of RDS and xDS ingest lately.

fredlas

This only touches the code I'm familiar with, the speaking-xDS-with-server parts, a bit, but those parts look fine.

source/common/router/scoped_rds.h

htuch

Thanks, this looks good; I take it that outside of the config update/dump, it's largely plumbing and there isn't a ton to review. Can we get coverage to 100% here, or do we need to wait for followup PRs?

htuch · 2019-05-16T18:52:18Z

include/envoy/router/scopes.h

+   * @param headers the request headers to match the scoped routing configuration against.
+   * @return ConfigConstSharedPtr the router's Config matching the request headers.
+   */
+  virtual ConfigConstSharedPtr getRouterConfig(const Http::HeaderMap& headers) const PURE;


I think it's important to distinguish between three concepts; "route config", "router config" and "HTTP connection manager route configuration". I think you mean the first or last year, rather than the middle?

This returns an Envoy::Router::Config, hence the name. However, I agree it is fairly confusing since the documentation refers to it by yet another name (route table).

I am fine switching to getRouteTable or even getRouteConfiguration if you think either of those more directly convey the meaning.

getRouteConfiguration or even the shorter getRouteConfig would be clearer to me.

source/common/router/scoped_config_manager.cc

source/common/router/scoped_config_manager.h

source/common/router/scoped_rds.cc

htuch · 2019-05-16T20:08:16Z

source/common/router/scoped_rds.cc

+      Protobuf::Message* clone = it.New();
+      clone->CopyFrom(it);
+      config_protos.push_back(std::unique_ptr<const Protobuf::Message>(clone));
+    }


Do we need to copy, or could we pass the RepeatedFieldPtr intro createStaticConfigProvider somehow?

To keep the Envoy::Config::ConfigProviderManager::createStaticConfigProvider interface generic, the container passed into the function must contain Protobuf::Message pointers. Switching to RepeatedFieldPtr<Protobuf::Message> would still trigger a copy of each item in the container.

OK, maybe use std::copy or std::transform then.

I removed this code from the PR since it is not yet used. I will reintroduce it in the next PR with modifications based on the feedback.

htuch · 2019-05-16T20:12:22Z

source/common/router/scoped_rds.cc

+  for (const auto& provider : immutableConfigProviders(ConfigProviderInstanceType::Inline)) {
+    const auto protos_info =
+        provider->configProtoInfoVector<envoy::api::v2::ScopedRouteConfiguration>();
+    if (protos_info == absl::nullopt) {


When would this be true? Maybe add a comment?

Converted to ASSERT().

htuch · 2019-05-16T20:13:25Z

source/common/router/scoped_rds.h

+#include "envoy/config/subscription.h"
+#include "envoy/stats/scope.h"
+
+#include "common/common/logger.h"


Suggest moving more of this file into the .cc and then we lose the .h dependency here on as many files.

source/common/router/scoped_config_manager.cc

dmitri-d · 2019-05-17T02:55:49Z

source/common/router/scoped_rds.cc

+  std::vector<std::string> exception_msgs;
+  // We need to keep track of which scoped routes we might need to remove.
+  ScopedConfigManager::ScopedRouteMap scoped_routes_to_remove =
+      scoped_config_manager_.scopedRouteMap();


Is the state kept in scoped_config_manager_ used anywhere else (or are there plans for that)? If not, I think you could clear the scoped route map on each update and then add routes that came in onConfigUpdate...

The state must persist across onConfigUpdate() calls such that the diff can be computed between the previous set of ScopedRouteConfiguration protos and the new set of protos.

AndresGuedez · 2019-05-17T18:47:46Z

Thanks, this looks good; I take it that outside of the config update/dump, it's largely plumbing and there isn't a ton to review. Can we get coverage to 100% here, or do we need to wait for followup PRs?

I am working on a commit to improve the coverage a bit, but the next PR with the integration tests will certainly add a significant amount of coverage as well, so I would prefer to avoid focusing on coverage too much until the next PR.

Signed-off-by: Andres Guedez <aguedez@google.com>

Also, fixes a bug removing scoped routes. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

htuch

LGTM modulo remaining comments.
/wait

htuch · 2019-05-21T02:18:04Z

include/envoy/router/scopes.h

+   * @param headers the request headers to match the scoped routing configuration against.
+   * @return ConfigConstSharedPtr the router's Config matching the request headers.
+   */
+  virtual ConfigConstSharedPtr getRouterConfig(const Http::HeaderMap& headers) const PURE;


getRouteConfiguration or even the shorter getRouteConfig would be clearer to me.

source/common/router/scoped_config_impl.h

source/common/router/scoped_rds.cc

htuch · 2019-05-21T02:26:44Z

source/common/router/scoped_rds.cc

+      Protobuf::Message* clone = it.New();
+      clone->CopyFrom(it);
+      config_protos.push_back(std::unique_ptr<const Protobuf::Message>(clone));
+    }


OK, maybe use std::copy or std::transform then.

source/common/router/scoped_rds.cc

htuch · 2019-05-21T02:30:02Z

test/common/router/scoped_rds_test.cc

+public:
+  ScopedRoutesConfigProviderManagerTest() = default;
+
+  ~ScopedRoutesConfigProviderManagerTest() override = default;


Nit: no need for these defaults.

test/mocks/router/mocks.h

Signed-off-by: Andres Guedez <aguedez@google.com>

htuch

Great, thanks!

* master: (65 commits) proto: Add PATCH method to RequestMethod enum (envoyproxy#6737) exe: drop unused deps on zlib compressor code (envoyproxy#7022) coverage: fix some misc coverage (envoyproxy#7033) Enable proto schema for router_check_tool (envoyproxy#6992) stats: rework stat sink flushing to centralize counter latching (envoyproxy#6996) [test] convert lds api test config stubs to v2 (envoyproxy#7021) router: scoped rds (2c): implement scoped rds API (envoyproxy#6932) build: Add option for size-optimized binary (envoyproxy#6960) test: adding an integration test framework for file-based LDS (envoyproxy#6933) doc: update obsolete ref to api/XDS_PROTOCOL.md (envoyproxy#7002) dispatcher: faster runOnAllThreads (envoyproxy#7011) example: add csrf sandbox (envoyproxy#6805) fix syntax of gcov exclusion zone. (envoyproxy#7023) /runtime_modify: add support for query params in body (envoyproxy#6977) stats: Create stats for http codes with the symbol table. (envoyproxy#6733) health check: fix more fallout from inline deletion change (envoyproxy#6988) Max heap fix (envoyproxy#7016) Add support to unregister from lifecycle notifications (envoyproxy#6984) build spdy_core_alt_svc_wire_format (envoyproxy#7010) ext_authz: Make sure initiateCall only called once (envoyproxy#6949) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

AndresGuedez added 30 commits February 5, 2019 10:32

Derive test class from TestBase.

db48450

Signed-off-by: Andres Guedez <aguedez@google.com>

fix_format fixes.

756afcd

Signed-off-by: Andres Guedez <aguedez@google.com>

Merge remote-tracking branch 'upstream/master' into scoped-rds-inline…

8e624ea

…-dynamic Signed-off-by: Andres Guedez <aguedez@google.com>

Minor cleanup.

98149e5

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix srds.proto docs.

76b7196

Signed-off-by: Andres Guedez <aguedez@google.com>

Pendatic spelling and docs cleanup.

f924eb7

Signed-off-by: Andres Guedez <aguedez@google.com>

fix_format.

51d8dff

Signed-off-by: Andres Guedez <aguedez@google.com>

More detailed documentation/examples for SRDS.

ccca775

Signed-off-by: Andres Guedez <aguedez@google.com>

fix_format.

e5d87db

Signed-off-by: Andres Guedez <aguedez@google.com>

Update pedantic spelling dictionary.

9c3d45b

Signed-off-by: Andres Guedez <aguedez@google.com>

Clarify Scope.Key documentation.

afcc2a5

Signed-off-by: Andres Guedez <aguedez@google.com>

Use EXPECT_NO_THROW() to validate successful allocation/construction.

c774f00

Signed-off-by: Andres Guedez <aguedez@google.com>

fix_format.

8cc1b13

Signed-off-by: Andres Guedez <aguedez@google.com>

Support delta SRDS proto in integration test.

5aed783

Also, cleanup. Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

15b1c7c

- Readability cleanup in various places. - Improve integration tests. - Add 'name' field to ScopedRoutes proto. Signed-off-by: Andres Guedez <aguedez@google.com>

Add comments.

b216034

Signed-off-by: Andres Guedez <aguedez@google.com>

Merge remote-tracking branch 'upstream/master' into scoped-rds-inline…

c8f531b

…-dynamic Signed-off-by: Andres Guedez <aguedez@google.com>

Fix build failures after master merge.

39309ac

Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup and comments.

8fe33ad

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix format.

8c38526

Signed-off-by: Andres Guedez <aguedez@google.com>

Comments.

dd2261c

Signed-off-by: Andres Guedez <aguedez@google.com>

Merge remote-tracking branch 'upstream/master' into scoped-rds-inline…

a7a621b

…-dynamic Signed-off-by: Andres Guedez <aguedez@google.com>

clang-tidy cleanup.

8b8a5e7

Signed-off-by: Andres Guedez <aguedez@google.com>

More clang-tidy cleanup.

43c95e8

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix build break.

6adaa22

Signed-off-by: Andres Guedez <aguedez@google.com>

Merge remote-tracking branch 'origin/scoped-rds-inline-dynamic' into …

3a4d4e6

…scoped-rds-config-provider Signed-off-by: Andres Guedez <aguedez@google.com>

Merge remote-tracking branch 'upstream/master' into scoped-rds-config…

2a4ee03

…-provider Signed-off-by: Andres Guedez <aguedez@google.com>

dio assigned stevenzzzz May 14, 2019

Fix GCC compiler warning related to hidden overloads.

d401e7c

Signed-off-by: Andres Guedez <aguedez@google.com>

htuch self-assigned this May 14, 2019

htuch requested review from dmitri-d and fredlas May 14, 2019 22:23

fredlas reviewed May 15, 2019

View reviewed changes

source/common/router/scoped_rds.h Show resolved Hide resolved

htuch reviewed May 16, 2019

View reviewed changes

dmitri-d reviewed May 17, 2019

View reviewed changes

AndresGuedez added 7 commits May 20, 2019 07:02

Improve test coverage.

52d32b5

Signed-off-by: Andres Guedez <aguedez@google.com>

Improve test coverage.

c611a46

Also, fixes a bug removing scoped routes. Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

e507e55

Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

22645d1

Signed-off-by: Andres Guedez <aguedez@google.com>

Use a namespace instead of a class with a static function.

f842132

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix clang-tidy issues.

68ed29b

Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

a5c87fc

Signed-off-by: Andres Guedez <aguedez@google.com>

htuch suggested changes May 21, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 21, 2019

Remove dead code and other minor cleanup.

28f39a6

Signed-off-by: Andres Guedez <aguedez@google.com>

repokitteh-read-only bot removed the waiting label May 21, 2019

AndresGuedez added 3 commits May 21, 2019 11:50

Remove unused code and other minor cleanup.

a8f97f6

Signed-off-by: Andres Guedez <aguedez@google.com>

Cleanup.

bed4340

Signed-off-by: Andres Guedez <aguedez@google.com>

Fix coverage test.

6716559

Signed-off-by: Andres Guedez <aguedez@google.com>

htuch approved these changes May 22, 2019

View reviewed changes

htuch merged commit 831d0cb into envoyproxy:master May 22, 2019

paul-r-gall mentioned this pull request May 14, 2021

Update ConfigDump documentation. #16491

Merged

Conversation

AndresGuedez commented May 14, 2019

Uh oh!

AndresGuedez commented May 14, 2019

Uh oh!

AndresGuedez commented May 14, 2019

Uh oh!

htuch commented May 14, 2019

Uh oh!

fredlas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndresGuedez commented May 17, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants