router: scoped RDS (2/4): support scoped routing configuration #5839

AndresGuedez · 2019-02-05T16:00:58Z

This PR introduces:

Support for scoped routing configuration parsing.
Inline and dynamic config providers using the ConfigProvider framework (introduced in config: Scoped RDS (PR 1/4): Introduce {static,dynamic} config provider framework #5243).
Unit and integration testing scaffolding.

This is the second PR in the chain to implement Scoped RDS (see #4704 for design).

NOTE: this PR does not actually implement the scoped routing logic; it will be introduced in the next PR in this chain.

Risk Level: Low; this API is not yet fully implemented and is config guarded (defaulting to disabled)
Testing: Unit and integration tests added
Docs Changes: SRDS (ScopedRouteConfigurationsSet) proto documentation

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>

snowp

Looks pretty good from what I can tell, left you come comments to get you started.

snowp · 2019-02-07T18:54:27Z

api/envoy/api/v2/srds.proto

+//   The Scoped RDS API is not yet fully implemented and *should not* be enabled in
+//   :ref:`envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager`.
+
+// The resource_names field in DiscoveryRequest specifies a set of route configuration scopes. Each


Add ref link to resource_names/DiscoveryRequest?

snowp · 2019-02-07T20:32:13Z

api/envoy/api/v2/srds.proto

+  }
+}
+
+// This configuration represents a set of "scopes", each containing independent routing


maybe a set of routing "scopes"?

snowp · 2019-02-07T20:40:16Z

api/envoy/api/v2/srds.proto

+      //
+      // .. code::
+      //
+      //    X-Header: a,b;c,d


I think using a=b;c=d in the example makes it more obvious that we're parsing key value pairs in this example

snowp · 2019-02-07T20:41:07Z

api/envoy/api/v2/srds.proto

+    // Specifies the mechanism for constructing fragments which are composed into keys.
+    message FragmentBuilder {
+      // Specifies how the value of a header should be extracted.
+      // The following example maps the structure of a header to the fields in this message.


Maybe include an example of a HeaderValueExtractor that can be used to extract a value from the header

Added an example in the ScopedRouteConfigurationsSet documentation.

snowp · 2019-02-07T21:12:27Z

api/envoy/api/v2/srds.proto

+      }
+    }
+
+    // The constructed key consists of the union of these fragments.


Does order matter?

Yes, updated comment.

snowp · 2019-02-07T22:33:53Z

source/extensions/filters/network/http_connection_manager/config.cc

@@ -153,9 +200,19 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
                                                                         context_.listenerScope())),
      proxy_100_continue_(config.proxy_100_continue()),
      delayed_close_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, delayed_close_timeout, 1000)) {
+  // Throws an exception on failure.


would it be more accurate to say that it throws an exception on invalid config?

snowp · 2019-02-07T22:50:57Z

test/common/http/conn_manager_impl_test.cc

@@ -74,8 +74,25 @@ class HttpConnectionManagerImplTest : public TestBase, public ConnectionManagerC
    std::shared_ptr<Router::MockConfig> route_config_{new NiceMock<Router::MockConfig>()};
  };

+  struct ScopedRouteConfigProvider : public Config::ConfigProvider {


Is this the same as the one defined in conn_manager_impl_fuzz_test.cc? Maybe define it in some shared file (alongside the mocks?) or maybe reuse the null provider?

Yes. I've now pulled this out into a common implementation included by both this test and the fuzz_test.

snowp · 2019-02-07T23:04:55Z

test/common/router/scoped_rds_test.cc

+  parseScopedRoutesConfigFromYaml(*resources.Add(), config_yaml);
+  EXPECT_THROW(subscription.onConfigUpdate(resources, "1"), ProtoValidationException);
+
+  // 'scope_key_builder.fragments' validation


Mind including comments about what is invalid about the configuration? Here and for other invalid config tests. Might also be valuable to verify the exception message to ensure that we're tickling the correct validation

snowp · 2019-02-07T23:06:45Z

test/common/router/scoped_rds_test.cc

+  ~ScopedRoutesConfigProviderManagerTest() override = default;
+};
+
+// Tests that the /configdump handler returns the corresponding scoped routing config.


nit: /config_dump

snowp · 2019-02-07T23:10:41Z

test/extensions/filters/network/http_connection_manager/config_test.cc

+  - name: envoy.router
+)EOF";
+
+  HttpConnectionManagerConfig(parseHttpConnectionManagerFromV2Yaml(config_yaml), context_,


use EXPECT_NO_THROW to indicate that we don't expect this to throw

mattklein123

A few high level comments from just looking at the API/docs. Thank you for working on this. Cool stuff.

mattklein123 · 2019-02-13T23:28:24Z

api/envoy/api/v2/srds.proto

+
+        oneof extract_type {
+          // Specifies the index of the element to extract.
+          int32 index = 3;


I didn't find mention of signed vs unsigned in Envoy's style guide so I defaulted to Google's style of using signed integers and relying on assertions to check val >= 0.

mattklein123 · 2019-02-13T23:32:00Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+
+    // Must be set to true when scoped RDS is enabled.
+    // If this is enabled, RDS subscriptions will be triggered by scoped RDS config updates using
+    // this configuration as a template.


mattklein123 · 2019-02-13T23:33:57Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

@@ -63,6 +64,14 @@ message HttpConnectionManager {
    envoy.api.v2.RouteConfiguration route_config = 4;
  }

+  oneof scoped_routes_specifier {
+    // Configuration for the Scoped RDS API which dynamically loads routing configuration scopes.
+    ScopedRds scoped_rds = 30;


Would it be simpler if these options were just in the route_specifier oneof above? Why have them be orthogonal?

My initial intention keeping them separate was to avoid confusion that SRDS and RDS are tightly coupled and require the same config source, etc.

However, based on the feedback in this review and reassessing this from the user's standpoint, I agree this is simpler if ScopedRds becomes part of the route_specifier, and ScopedRds expands to include both the ConfigSource for RDS and SRDS. This would allow me to remove scoped_rds_template from Rds, which avoids the confusion related to that field (I've never been quite happy with that field since I started working on this).

mattklein123 · 2019-02-13T23:35:36Z

api/envoy/api/v2/srds.proto

+  }
+}
+
+// This configuration represents a set of "scopes", each containing independent routing


I'm going to be honest in saying that it's very hard for me to follow the messages/documentation in this file. I read the spec a while ago but I've forgotten it. Would it be possible to add some high level documentation here that walks the end user through some examples of how the API is setup, the fragment builder works, etc. If you plan on doing this in arch overview docs that's fine too, whatever is easiest. The reason I ask for this now is I think it would make it a lot easier to review.

My initial plan was to add in-depth documentation including examples to the arch overview docs (which I wasn't planning to do until the feature is fully implemented in PR 3), but I agree better documentation in the proto definition would be helpful for this review. I am working on this.

Thanks a ton. I think even just some YAML examples here in the proto docs would be super helpful. You could move them to the arch overview later or just link to them here, so hopefully zero wasted effort.

AndresGuedez · 2019-02-19T04:00:04Z

@snowp @mattklein123 Thanks for the feedback and patience. I wasn't able to work on this last week but I will be actively addressing the comments and updating the PR this week.

stale · 2019-02-26T19:15:18Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

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

AndresGuedez · 2019-02-27T19:56:43Z

Apologies for the delay responding to feedback.

I have addressed most of the review comments. I am now working on removing the scoped_rds_template per #5839 (comment). This should simplify the logic in HttpConnectionManager and also greatly clarify the interaction between SRDS and RDS ConfigSource configuration.

AndresGuedez · 2019-02-28T15:11:39Z

api/envoy/api/v2/srds.proto

+// which Envoy would fetch via RDS.
+//
+// [#comment:next free field: 4]
+message ScopedRouteConfigurationsSet {


Upon revisiting the SRDS proto in the context of Incremental xDS, I have decided to make a few changes after discussing with @htuch.

To easily enable incremental updates to the routing scopes (represented by the Scope message) by leveraging the existing Incremental xDS API, I am going to move the ScopeKeyBuilder specification out of the ScopedRouteConfigurationsSet into the HttpConnectionManager, and instead of publishing a repeated Scope scopes = in this message, each SRDS proto (which will most likely be renamed to ScopedRouteConfiguration) will contain a single Scope.

This change creates much better alignment with the API/protocol already defined for incremental updates, and also simplifies the SRDS API.

I am actively working on this now and will introduce the changes discussed as part of the next few commits to this PR.

@AndresGuedez cool I will defer looking at the API again until this is done. Thank you for iterating on this.

/wait

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

AndresGuedez · 2019-04-12T21:05:58Z

I have made the changes to modify the SRDS API to be delta friendly; what is the best way to move forward with this review?

Can this PR be reopened?

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

htuch · 2019-04-15T14:22:04Z

@AndresGuedez can you merge master to resolve conflicts?

…-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>

AndresGuedez · 2019-04-16T21:03:23Z

@AndresGuedez can you merge master to resolve conflicts?

Merged master and pushed commit with cleanup of the ConfigProvider framework.

Actively working on addressing test coverage.

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

htuch · 2019-04-19T21:46:41Z

source/common/config/utility.h

@@ -329,6 +331,41 @@ class Utility {
  static void translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
                                    const ProtobufWkt::Struct& config,
                                    Protobuf::Message& out_proto);
+
+#if 0


htuch · 2019-04-19T21:53:32Z

api/envoy/admin/v2alpha/config_dump.proto

@@ -178,3 +179,43 @@ message RoutesConfigDump {
  // The dynamically loaded route configs.
  repeated DynamicRouteConfig dynamic_route_configs = 3 [(gogoproto.nullable) = false];
 }
+
+// Envoy's scoped RDS implementation fills this message with all currently loaded route


Thanks @AndresGuedez for this, my main thought looking at this is that the PR is too big to reasonably review and without diluting review bandwidth. Can we split the PR up further for review?

I think we should have this master PR kept open to show how things tie together, but then separate PRs for:

Protos.

Scoped RDS configuration parser and handling logic. This can just be unit tested and doesn't need to be wired up yet.

Config subscription and wiring of scoped RDS into the rest of Envoy. The integration test can be added here.

Optionally, other PRs to reduce the amount of renaming noise, where reasonable.

I think we can get better review velocity if we do it this way.

Ack. Working on splitting this up.

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 #5839 for full context into these changes. PRs 2a (#6675) and 2b (#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 #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. Signed-off-by: Andres Guedez <aguedez@google.com>

#7068) Integrate the Scoped Route Discovery Service (SRDS) API into the HttpConnectionManager. NOTES: - This is the last PR in the chain originating from parent #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. Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123 · 2019-06-18T22:38:40Z

I think this is done and can be closed.

AndresGuedez added 5 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>

htuch self-assigned this Feb 5, 2019

Fix srds.proto docs.

76b7196

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

mattklein123 self-assigned this Feb 6, 2019

AndresGuedez added 2 commits February 6, 2019 09:40

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>

htuch assigned snowp Feb 7, 2019

htuch requested a review from snowp February 7, 2019 17:06

snowp suggested changes Feb 7, 2019

View reviewed changes

junr03 self-assigned this Feb 11, 2019

mattklein123 reviewed Feb 13, 2019

View reviewed changes

mattklein123 added the waiting label Feb 13, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 26, 2019

AndresGuedez added 2 commits February 27, 2019 11:37

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>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 27, 2019

repokitteh-read-only bot removed the waiting label Feb 27, 2019

AndresGuedez added 4 commits February 27, 2019 13:55

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>

AndresGuedez commented Feb 28, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 28, 2019

AndresGuedez added 3 commits April 12, 2019 12:07

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>

htuch reopened this Apr 15, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 15, 2019

repokitteh-read-only bot removed the waiting label Apr 15, 2019

AndresGuedez added 2 commits April 15, 2019 11:35

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

a7a621b

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

AndresGuedez added 3 commits April 17, 2019 09:02

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>

htuch suggested changes Apr 19, 2019

View reviewed changes

htuch added the waiting:any label Apr 19, 2019

snowp removed their assignment Apr 19, 2019

repokitteh-read-only bot removed the waiting:any label Apr 22, 2019

AndresGuedez mentioned this pull request Apr 22, 2019

router: scoped rds (2a): scoped routing configuration protos #6675

Merged

mattklein123 added no stalebot Disables stalebot from closing an issue waiting labels Apr 24, 2019

AndresGuedez mentioned this pull request May 2, 2019

config: scoped rds (2b): support delta APIs in ConfigProvider framework #6781

Merged

AndresGuedez mentioned this pull request May 14, 2019

router: scoped rds (2c): implement scoped rds API #6932

Merged

AndresGuedez mentioned this pull request May 24, 2019

router: scoped rds (2d): integrate SRDS API into HttpConnectionManager #7068

Merged

mattklein123 removed the waiting label Jun 18, 2019

mattklein123 closed this Jun 18, 2019

router: scoped RDS (2/4): support scoped routing configuration #5839

router: scoped RDS (2/4): support scoped routing configuration #5839

Conversation

AndresGuedez commented Feb 5, 2019

snowp 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

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

mattklein123 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

AndresGuedez commented Feb 19, 2019 • edited Loading

stale bot commented Feb 26, 2019

AndresGuedez commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndresGuedez commented Apr 12, 2019

htuch commented Apr 15, 2019

AndresGuedez commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Jun 18, 2019

AndresGuedez commented Feb 19, 2019 •

edited

Loading