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

AndresGuedez · 2019-04-22T18:03:34Z

This PR introduces the configuration protos required for scoped routing support (#4704).

This is the second chain of PRs for SRDS support. This chain will be split up into multiple PRs in the following cadence:

Configuration protos
Configuration parsing
SRDS config subscription handling

Risk Level: Low (the protos are not yet referenced by any code)
Testing: None (will be introduced in the next PR)

Protos to statically and dynamically (via the Scoped Route Discovery Service) configure scoped routing logic in Envoy. Signed-off-by: Andres Guedez <aguedez@google.com>

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

AndresGuedez · 2019-04-22T18:04:08Z

For context, refer to PR #5839 which includes the full set of changes which will be introduced by this PR chain.

AndresGuedez · 2019-04-22T18:55:08Z

@htuch PTAL

htuch

Great, these are awesome proto comments. I would encourage anyone in the Envoy community who cares about the design-level aspect of multi-tenancy to give this a read, it is a significant part of the story (@envoyproxy/maintainers).

LGTM modulo a few comments.
/wait

api/envoy/api/v2/srds.proto

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

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

htuch · 2019-04-23T18:58:08Z

@lizan @mattklein123 @snowp would one of you like to volunteer as a non-Googler reviewers of this API? I'm pretty happy with the current state modulo unresolved comments.

mattklein123 · 2019-04-23T19:02:49Z

Yes I would like to review this, but it will take a few days. Sorry for the delay.

AndresGuedez · 2019-04-24T18:59:56Z

/retest

repokitteh-read-only · 2019-04-24T19:00:00Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

mattklein123

Thanks this generally looks great. A few comments that I think can help around readability. This is a great feature and I want to make sure people can read the docs and understand it.

/wait

api/envoy/api/v2/srds.proto

mattklein123 · 2019-04-24T19:18:30Z

api/envoy/api/v2/srds.proto

+//
+// [#comment:next free field: 4]
+// [#proto-status: experimental]
+message ScopedRouteConfiguration {


I wonder whether this message should be in the same file as the related HCM protos? Is there any compelling reason to split them like this? It seems like they are very closely related? Don't have a strong opinion on this but putting all of the required definitions together in an srds.proto might make this complicated feature easier to grok? WDYT?

My intent is to maintain consistency by having the xDS API "top-level" proto specification under api/envoy/api/v2.

I found it more readable to have the ScopedRoutes proto defined in the http_connection_manager.proto along its consumer rather than pairing it with the ScopedRouteConfiguration proto here. However, this may just be my bias being the implementor so I can certainly consolidate the protos into srds.proto if that is more approachable for those with less context.

@htuch thoughts here? My preference is to keep them together for readability, but I don't feel strongly about it if others do.

I think the split is reasonable to me, given how the service and the builder are used. I think it might even warrant moving to a separate peer proto alongside HCM config (but same package namespace) as it's self-contained, but we can do that optionally in a followup.

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

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

mattklein123 · 2019-04-25T22:45:12Z

Thanks, LGTM modulo the question of whether to merge the protos or not. Will let @htuch weigh in on that.

/wait-any

htuch

Great, thanks!

mattklein123

Very nice.

* master: (35 commits) Revert "api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692)" (envoyproxy#6761) Add test for the SocketOptionFactory::buildLiteralOptions() method. (envoyproxy#6724) Add test of parsing weighted_cluster route configuration to improve test coverage. (envoyproxy#6711) test: reducing H2 test permutations, increasing coverage time (envoyproxy#6753) Support gRPC-JSON translate without the google.api.http option. (envoyproxy#6731) quiche: implement QuicEpollClock (envoyproxy#6745) http: rc details for main Envoy workflow (envoyproxy#6560) quiche: implement QuicSystemEventLoopImpl (envoyproxy#6723) http: tracking 100s from upstream in stats (envoyproxy#6746) coverage: run without deprecated option (envoyproxy#6752) quiche: Implement spdy_test_helpers_impl. (envoyproxy#6741) [test] convert listener test stubs to v2 API (envoyproxy#6735) api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) quiche: Implement http2_reconstruct_object_impl.h. (envoyproxy#6717) build: patch protobuf for UBSAN issue. (envoyproxy#6721) router: scoped rds (2a): scoped routing configuration protos (envoyproxy#6675) tap: use move semantics for submitTrace (envoyproxy#6709) quiche: add epoll_server for testing (envoyproxy#6650) Increase timeout of the coverage test run to 3000 seconds as it is now bumping in the current 2000s limit causing coverage run to abort sometimes. (envoyproxy#6722) quiche: Update tarball to commit 43a1c0f10f2855c3cd142f500e8d19ac6d6f5a8c (envoyproxy#6718) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

…oxy#6675) Protos to statically and dynamically (via the Scoped Route Discovery Service) configure scoped routing logic in Envoy. Signed-off-by: Andres Guedez <aguedez@google.com> Signed-off-by: Jeff Piazza <jeffpiazza@google.com>

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>

AndresGuedez added 2 commits April 22, 2019 13:50

Add Scoped RDS protos.

fc2da90

Protos to statically and dynamically (via the Scoped Route Discovery Service) configure scoped routing logic in Envoy. Signed-off-by: Andres Guedez <aguedez@google.com>

Pedantic spelling additions.

90ec8eb

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

htuch self-assigned this Apr 22, 2019

htuch suggested changes Apr 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 22, 2019

Review feedback based cleanup.

bd149b0

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

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

mattklein123 self-assigned this Apr 23, 2019

mattklein123 requested changes Apr 24, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 24, 2019

Cleanup and comments.

ab12667

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

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

repokitteh-read-only bot added waiting:any and removed waiting:any labels Apr 25, 2019

htuch approved these changes Apr 26, 2019

View reviewed changes

mattklein123 approved these changes Apr 26, 2019

View reviewed changes

mattklein123 merged commit 2778c3c into envoyproxy:master Apr 26, 2019

AndresGuedez mentioned this pull request May 14, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AndresGuedez commented Apr 22, 2019

AndresGuedez commented Apr 22, 2019

AndresGuedez commented Apr 22, 2019

htuch left a comment

htuch commented Apr 23, 2019

mattklein123 commented Apr 23, 2019

AndresGuedez commented Apr 24, 2019

repokitteh-read-only bot commented Apr 24, 2019

mattklein123 left a comment

mattklein123 Apr 24, 2019

AndresGuedez Apr 25, 2019

mattklein123 Apr 25, 2019

htuch Apr 26, 2019

mattklein123 commented Apr 25, 2019

htuch left a comment

mattklein123 left a comment

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

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

Conversation

AndresGuedez commented Apr 22, 2019

AndresGuedez commented Apr 22, 2019

AndresGuedez commented Apr 22, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch commented Apr 23, 2019

mattklein123 commented Apr 23, 2019

AndresGuedez commented Apr 24, 2019

repokitteh-read-only bot commented Apr 24, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Apr 24, 2019

Choose a reason for hiding this comment

AndresGuedez Apr 25, 2019

Choose a reason for hiding this comment

mattklein123 Apr 25, 2019

Choose a reason for hiding this comment

htuch Apr 26, 2019

Choose a reason for hiding this comment

mattklein123 commented Apr 25, 2019

htuch left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment