router: scoped rds (2d): integrate SRDS API into HttpConnectionManager by AndresGuedez · Pull Request #7068 · envoyproxy/envoy

AndresGuedez · 2019-05-24T13:20:21Z

Integrate the Scoped Route Discovery Service (SRDS) API into the HttpConnectionManager.

NOTES:

This is the last PR in the chain originating from parent router: scoped RDS (2/4): support scoped routing configuration #5839.
The scoped routing logic which can be configured through this API has not yet been implemented; it will be done in follow up PRs.

Risk Level: Low (API is not yet fully implemented and should not be enabled)
Testing: Integration 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-integration Signed-off-by: Andres Guedez <aguedez@google.com>

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

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

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

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

htuch · 2019-05-28T15:39:19Z

Please merge master to pick up #7090

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

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

AndresGuedez · 2019-05-28T20:29:47Z

/retest

repokitteh-read-only · 2019-05-28T20:29:51Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7068 (comment) was created by @AndresGuedez.

see: more, trace.

AndresGuedez · 2019-05-30T14:18:15Z

Is there any way to retest just the envoy-macos CI run? It appears to have flaked but /retest did not kick off a rerun.

htuch · 2019-05-30T22:21:29Z

/azp run
@AndresGuedez ^^ like so

azure-pipelines · 2019-05-30T22:21:34Z

Command 'run @AndresGuedez' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run. Example: "run" or "run pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

htuch · 2019-05-30T22:24:37Z

/azp run

azure-pipelines · 2019-05-30T22:24:53Z

Azure Pipelines successfully started running 2 pipeline(s).

htuch

Generally looks good, a few comments on some of the proto munging.

source/common/http/conn_manager_config.h

htuch · 2019-05-31T03:56:53Z

source/common/router/scoped_rds.cc

+                                                config.scoped_routes().scope_key_builder()));
+
+  case envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes::
+      CONFIG_SPECIFIER_NOT_SET:


Should we have a proto constraint to ensure this never happens?

We do have one, see http_connection_manager.proto.

I've removed handling of the condition to allow it to fall through to the default: to avoid ambiguity.

source/common/router/scoped_rds.cc

htuch · 2019-05-31T03:58:29Z

source/common/router/scoped_rds.cc

+                     Protobuf::Message* clone = scoped_route_config.New();
+                     clone->MergeFrom(scoped_route_config);
+                     return std::unique_ptr<const Protobuf::Message>(clone);
+                   });


Should there just be some util to convert a RepeatedPtrField to these const vectors in source/common/protobuf? Seems generic enough

I've added the utility function.

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

htuch

LGTM modulo comments.
/wait

htuch · 2019-06-03T22:48:50Z

source/common/router/scoped_rds.cc

+                                                config.scoped_routes().rds_config_source(),
+                                                config.scoped_routes().scope_key_builder()));
+  }
+


Nit: superfluous blank line.

htuch · 2019-06-03T22:51:48Z

test/integration/scoped_rds_integration_test.cc

+)EOF";
+          envoy::config::filter::network::http_connection_manager::v2::ScopedRoutes::ScopeKeyBuilder
+              scope_key_builder;
+          MessageUtil::loadFromYaml(scope_key_builder_config_yaml, scope_key_builder);


Can you merge master to pick up changes? This will need to change to TestUtil::loadFromYaml..

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

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

htuch

Thanks!

AndresGuedez · 2019-06-04T16:08:20Z

Thanks for the reviews throughout this whole PR chain!

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 …

e8d5bff

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

Fix build after merge and cleanup.

c2ec4d4

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

AndresGuedez added 5 commits May 23, 2019 21:50

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

fd3963e

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

Fix format.

d5ae805

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

Cleanup.

3ad4c0a

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

Add warning log when SRDS is enabled.

0e4d51c

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

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

62c4359

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

mattklein123 assigned htuch May 26, 2019

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

c4a947a

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

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

5f16708

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

htuch added the waiting label May 28, 2019

Fix build after master merge.

0155a91

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

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

htuch reviewed May 31, 2019

View reviewed changes

AndresGuedez added 2 commits May 31, 2019 15:18

Cleanup.

ecfe946

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

Move function to more appropriate class.

e876cae

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

htuch suggested changes Jun 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 3, 2019

AndresGuedez added 2 commits June 3, 2019 23:24

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

37ec3f8

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

Build fix + cleanup.

eaf694b

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

repokitteh-read-only bot removed the waiting label Jun 4, 2019

htuch approved these changes Jun 4, 2019

View reviewed changes

htuch merged commit 1fdaf8e into envoyproxy:master Jun 4, 2019

Conversation

AndresGuedez commented May 24, 2019

Uh oh!

htuch commented May 28, 2019

Uh oh!

AndresGuedez commented May 28, 2019

Uh oh!

repokitteh-read-only bot commented May 28, 2019

Uh oh!

AndresGuedez commented May 30, 2019

Uh oh!

htuch commented May 30, 2019

Uh oh!

azure-pipelines bot commented May 30, 2019

Uh oh!

htuch commented May 30, 2019

Uh oh!

azure-pipelines bot commented May 30, 2019

Uh oh!

htuch left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

AndresGuedez commented Jun 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants