config: scoped rds (2b): support delta APIs in ConfigProvider framework by AndresGuedez · Pull Request #6781 · envoyproxy/envoy

AndresGuedez · 2019-05-02T16:42:20Z

Add support for "delta" APIs to the ConfigProvider framework in preparation for scoped RDS (see #4704).

A delta API receives a subset of the configuration set through each resource distributed via the *DiscoveryResponse messages.

While this PR is not directly related to the ongoing incremental xDS effort, it creates the foundation in the ConfigProvider framework to support Delta xDS updates for APIs implemented on top of the framework.

Risk Level: Low (there is no DS API yet using this framework)
Testing: Unit tests added, all existing tests passing.
Docs Changes: N/A
Release Notes: N/A

An xDS delta API receives a subset of the config set as part of each resource contained in a DiscoveryResponse. An example of this type of API is CDS. Signed-off-by: Andres Guedez <aguedez@google.com>

…elta-api-support Signed-off-by: Andres Guedez <aguedez@google.com>

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

AndresGuedez · 2019-05-02T16:43:23Z

For context, refer to PR #5839 which includes the full set of changes which will be introduced by this PR chain (note that there are a few minor differences with this PR as a result of further testing and cleanup).

AndresGuedez · 2019-05-02T16:43:46Z

@htuch PTAL

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

AndresGuedez · 2019-05-02T21:38:28Z

/retest

repokitteh-read-only · 2019-05-02T21:38:31Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

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

source/common/config/config_provider_impl.cc

source/common/config/config_provider_impl.h

include/envoy/config/config_provider.h

htuch · 2019-05-06T19:40:04Z

Would also be good to get @fredlas's eyes on this one as well.

include/envoy/config/config_provider.h

source/common/config/config_provider_impl.cc

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

htuch

Looks good modulo comments.
/wait

include/envoy/config/config_provider.h

source/common/config/config_provider_impl.cc

source/common/config/config_provider_impl.h

source/common/config/config_provider_impl.cc

fredlas

In the class level comment for ConfigProvider,

Use config() to obtain a shared_ptr to the implementation of the config

I don't understand what "implementation of the config" means. Also, in the next sentence, what is the "underlying config proto"? Is that e.g. a ClusterLoadAssignment, RouteConfiguration, etc? And, is the parenthetical at the end saying configProtoInfo() is only applicable to dynamic config, or that just the part of it providing version is only applicable to dynamic config?

include/envoy/config/config_provider.h

source/common/config/config_provider_impl.h

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

source/common/config/config_provider_impl.h

htuch · 2019-05-09T12:04:54Z

test/common/config/config_provider_impl_test.cc

+  ConfigProviderPtr
+  createStaticConfigProvider(std::vector<std::unique_ptr<const Protobuf::Message>>&&,
+                             Server::Configuration::FactoryContext&, const OptionalArg&) override {
+    ASSERT(false || "this provider does not expect multiple config protos");


ASSERT_FALSE(true) << "some stuff"? Or something like that (I think gtest has some dedicated predicate for this somewhere).

This virtual function does not return an AssertionResult, so I can't use the gtest ASSERT_*s.

Can you do ASSERT(false, "some message") then for better readability?

source/common/config/config_provider_impl.h

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

htuch

Thanks, LGTM, I grok the subtractive updates at https://github.com/envoyproxy/envoy/pull/5839/files#diff-1578b366b91c6312bf7d120645d32b22R160.

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

AndresGuedez · 2019-05-09T15:32:42Z

@fredlas Missed your top level comment earlier:

In the class level comment for ConfigProvider,

Use config() to obtain a shared_ptr to the implementation of the config

I don't understand what "implementation of the config" means.

"Implementation of the config" means the Envoy logic that actually translates the config intent into Envoy actions. For example, RDS' config implementation is found at Envoy::Router::ConfigImpl, and CDS's logic is found in cluster_manager_impl.cc.

Also, in the next sentence, what is the "underlying config proto"? Is that e.g. a ClusterLoadAssignment, RouteConfiguration, etc?

Yes.

And, is the parenthetical at the end saying configProtoInfo() is only applicable to dynamic config, or that just the part of it providing version is only applicable to dynamic config?

It's meant to clarify that the version_ is only set by "dynamic" (i.e., DS API) providers that distribute a version as opposed to "static" providers that are configured via bootstrap.

fredlas · 2019-05-09T17:22:38Z

Thanks!

AndresGuedez · 2019-05-09T18:10:11Z

Thanks for the reviews!

* master: (88 commits) upstream: Null-deref on TCP health checker if setsockopt fails (envoyproxy#6793) ci: switch macOS CI to azure pipelines (envoyproxy#6889) os syscalls lib: break apart syscalls used for hot restart (envoyproxy#6880) Kafka codec: precompute request size before serialization, so we do n… (envoyproxy#6862) upstream: move static and strict_dns clusters to dedicated files (envoyproxy#6886) Rollforward of api: Add total_issued_requests to Upstream Locality and Endpoint Stats. (envoyproxy#6692) (envoyproxy#6784) fix explicit constructor in copy-initialization (envoyproxy#6884) stats: use tag iterator rather than constructing the tag-array and searching that. (envoyproxy#6853) common: use unscoped build target in generate_version_linkstamp (envoyproxy#6877) Addendum to envoyproxy#6778 (envoyproxy#6882) ci: add minimum Linux build for Azure Pipelines (envoyproxy#6881) grpc: utilities for inter-converting grpc::ByteBuffer and Buffer::Instance. (envoyproxy#6732) upstream: allow excluding hosts from lb calculations until initial health check (envoyproxy#6794) stats: prevent unused counters from leaking across hot restart (envoyproxy#6850) network filters: add `injectDataToFilterChain(data, end_stream)` method to network filter callbacks (envoyproxy#6750) delete things that snuck back in (envoyproxy#6873) config: scoped rds (2b): support delta APIs in ConfigProvider framework (envoyproxy#6781) string == string! (envoyproxy#6868) config: add mssing imports to delta_subscription_state (envoyproxy#6869) protobuf: add missing default case to enum (envoyproxy#6870) ... Signed-off-by: Michael Puncel <mpuncel@squareup.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 4 commits May 2, 2019 09:33

Add support for delta APIs to ConfigProvider framework.

114f530

An xDS delta API receives a subset of the config set as part of each resource contained in a DiscoveryResponse. An example of this type of API is CDS. Signed-off-by: Andres Guedez <aguedez@google.com>

Merge remote-tracking branch 'upstream/master' into config-provider-d…

5232f28

…elta-api-support Signed-off-by: Andres Guedez <aguedez@google.com>

Add unit test and minor fixes.

53c929b

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

Cleanup.

546cdc7

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

zuercher assigned htuch May 2, 2019

Cleanup.

1847855

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

Kick CI

1c1972b

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

htuch requested a review from fredlas May 6, 2019 16:41

htuch reviewed May 6, 2019

View reviewed changes

source/common/config/config_provider_impl.cc Show resolved Hide resolved

source/common/config/config_provider_impl.h Show resolved Hide resolved

include/envoy/config/config_provider.h Show resolved Hide resolved

htuch reviewed May 6, 2019

View reviewed changes

include/envoy/config/config_provider.h Outdated Show resolved Hide resolved

source/common/config/config_provider_impl.cc Outdated Show resolved Hide resolved

Cleanup.

914bbe0

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

htuch suggested changes May 6, 2019

View reviewed changes

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

fredlas reviewed May 6, 2019

View reviewed changes

include/envoy/config/config_provider.h Show resolved Hide resolved

include/envoy/config/config_provider.h Show resolved Hide resolved

include/envoy/config/config_provider.h Show resolved Hide resolved

source/common/config/config_provider_impl.h Show resolved Hide resolved

Cleanup and improved test coverage.

284b405

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

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

AndresGuedez added 2 commits May 8, 2019 13:16

Fix test breakage due to comparison of protos with repeated fields.

75b94de

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

Rename function to minimize ambiguity.

5b5c543

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

htuch reviewed May 9, 2019

View reviewed changes

Rename variable per Envoy style guide.

57e82d9

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

htuch previously approved these changes May 9, 2019

View reviewed changes

htuch added the waiting label May 9, 2019

Fix ASSERT() message.

e64cd8f

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

AndresGuedez dismissed htuch’s stale review via e64cd8f May 9, 2019 15:22

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

htuch approved these changes May 9, 2019

View reviewed changes

fredlas approved these changes May 9, 2019

View reviewed changes

htuch merged commit a65e228 into envoyproxy:master May 9, 2019

AndresGuedez mentioned this pull request May 14, 2019

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

Merged

Conversation

AndresGuedez commented May 2, 2019

Uh oh!

AndresGuedez commented May 2, 2019

Uh oh!

AndresGuedez commented May 2, 2019

Uh oh!

AndresGuedez commented May 2, 2019

Uh oh!

repokitteh-read-only bot commented May 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented May 6, 2019

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredlas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch May 9, 2019

Choose a reason for hiding this comment

Uh oh!

AndresGuedez May 9, 2019

Choose a reason for hiding this comment

Uh oh!

htuch May 9, 2019

Choose a reason for hiding this comment

Uh oh!

AndresGuedez May 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

AndresGuedez commented May 9, 2019

Uh oh!

fredlas commented May 9, 2019

Uh oh!

AndresGuedez commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fredlas left a comment •

edited

Loading

AndresGuedez commented May 9, 2019 •

edited

Loading