API: Add extension information the Node message. #9301

yanavlasov · 2019-12-10T18:24:03Z

Report extension information in the Node message.

Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: N/A
Fixes: #8332

Signed-off-by: Yan Avlasov yavlasov@google.com

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only · 2019-12-10T18:24:11Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9301 was opened by yanavlasov.

see: more, trace.

yanavlasov · 2019-12-10T18:31:35Z

This PR is to discuss how to best add versioning and categorization to the extension registry so it can be added to the Node message. Specifically:

do we need to have "canonical" category for the extensions, expressed as enum in the Node proto. This would be broad buckets, like network and http filter, stats sink, cred provider, etc. The existing category is a free form string.
Would it be better to have common base interface for all factories where the "getter" for the version and potentially canonical category can be added. Without the base class it is difficult to retrieve factory specific attributes in a generic manner.

htuch

@yanavlasov thanks, this is pretty much aligned with what we had previously discussed, so looks great. A few nits and I've tagged folks who might be interested in the review.
/wait

api/envoy/api/v2/core/base.proto

include/envoy/registry/registry.h

api/envoy/api/v2/core/base.proto

include/envoy/registry/registry.h

markdroth

Thanks for looping me in, @htuch!

api/envoy/api/v2/core/base.proto

Signed-off-by: Yan Avlasov <yavlasov@google.com>

api/envoy/api/v2/core/base.proto

docs/root/api/client_features.rst

include/envoy/registry/registry.h

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Run proto_format Fix tests with deprecated build_version Signed-off-by: Yan Avlasov <yavlasov@google.com>

Signed-off-by: Yan Avlasov <yavlasov@google.com>

…umented client features Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch

Generally looks great, thanks. @jpeach any further comments?
/wait

htuch · 2019-12-17T03:37:34Z

api/envoy/api/v2/core/base.proto

+  // configuration, e.g. envoy.router, com.acme.widget.
+  string name = 1;
+
+  // Category of the extension


Can you add docs on where to find these names and some examples?

Good point. Right now the category comes from the factory's category() method. There is no single place where all categories are listed. Also not categories right now do not follow the DNS naming. I.e. tracers use the "tracing" category.
Some things to consider:

Should we consolidate all category names into a common header?

Should we add a notion of a deprecated category name and rename all non-conforming categories to use DNS notation?

I think we could move towards (2), probably file an issue. For now, I think if we could do (1) with decent comments that would be awesome, we could just provide the GitHub URL to that header.

docs/root/api/client_features.rst

htuch · 2019-12-17T03:45:29Z

include/envoy/registry/registry.h

+   * version is used for registering vendor specific factories that are versioned
+   * independently of Envoy.
+   */
+  static void registerFactory(Base& factory, absl::string_view name,


Are we registering all the Envoy in-builts as well (using the semver of Envoy itself)? I think we should do this, as end users can optionally build Envoy with/without different filters, and it helps to know which ones are around.

As far as I know, yes. There is RegisterInternalFactory that does not register a category, which is used for some internal factories (i.e. SslContext). I'm unsure if these need to be reported as well. Perhaps we should audit these.

Can you pastebin what the full Node looks like somewhere? I'm curious to take a look at this.

https://pastebin.com/KyZZYUty

I notice that the build version information isn't populated; do we assume it's the same as the Envoy version? I think for semantic version this is fair. For labels, we actually have this metadata already in BUILD rules, see https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_library.bzl#L70.

I wonder if we want to populate labels with the security posture and status of extensions? Seems a pretty cool use of them and should not be too hard.

@markdroth will gRPC use SemVer for extension versioning? If not, what is your versioning scheme?
If we make an assumption that an empty extension version means that the version is the same as the "user agent", should the "user_agent_version" include SemVer as well? Will you version your xDS client using SemVer?

The gRPC client itself does use semantic versioning, and it definitely makes sense to use SemVer as the format for user_agent_version.

The question of versions for plugins is an interesting one. In principle, it seems reasonable for plugins to use their own semantic versioning, but in order to provide those versions to xDS, gRPC would probably need to provide a way for the plugin to record its version when it registers itself. @ejona86 and @dfawley, thoughts...?

It might be a little confusing to assume that the default extension version matches the user agent version, because there could be some very simple plugins that don't bother to set a version. It might make more sense to just treat an empty version as a unique value.

I suppose how the empty version is treated is up to the xDS server. Are you suggesting removing the note about empty version from the proto doc?

Yes. I think we should just say that an empty version means no version specified, not that it defaults to the user agent version.

alexburnos · 2019-12-17T06:11:55Z

api/envoy/api/v2/core/base.proto

+  string name = 1;
+
+  // Category of the extension
+  string category = 2;


Just to clarify, I would assume specific type/name of the WASM filter would go into the category as well? Because its name would be a generic "envoy.wasm" in all cases.

This field is populated from the factory's category() method. I've read through the #9358 and I'm still clear on what needs to happen for wasm filters. Perhaps WASM filter category should be supplied with the config somehow?

@PiotrSikora could you please chime in on how WASM could be reporting its specific kind through this API?

Also @jplevyak. I think that we can add some higher-order category support later for things like WASM. Ultimately I'd like to see how this plays out, but we shouldn't block.

Wasm doesn't use protobuf for configuration. Do we need an alternative way to categorize / type the Wasm filters?

Hmm, this would be rather unfortunate to add separate categorization for wasm extensions. What does wasm extensions use for configuration? Can they provide an identity config proto?

Wasm just uses an opaque blob. The reason being protobuf limits the choice of languages (and it's pretty slow in general).

I think we can support Wasm additively after this PR ships. While the Wasm extension providing the runtime is a true Envoy extension, its plugins will be their own thing and probably need a structured reporting mechanism. I don't think we break that by going with what is in this PR.

+1 I think wasm can fit into this scheme by having all its plugin to have the same category i.e. "wasm" and add thread safe implementation of the FactoryRegistryProxy that can provide names of available wasm plugins. The name can come from config metadata.

This should be fine for now, but I hope wasm will develop some first-class integration with xDS/Envoy version reporting mechanism, rather just leaving it for generic "wasm" and then providing opaque blob for the rest. That's a note for wasm in general, not specifically for this implementation.

api/envoy/api/v2/core/base.proto

Signed-off-by: Yan Avlasov <yavlasov@google.com>

This PR extends #9468 to support a distinct notion of transport and resource API version. The intuition here is that the opaque resources (and their type URLs) can be delivered via either v2 or v3 xDS, and the DiscoveryRequest etc. messages have their own versioning. Currently, the v2 and v3 transport protocols are indistinguishable modulo service endpoint. As v3 evolves, e.g. with #9301, differences will be introduced. At this point it will be necessary to have enhanced support in the gRPC mux and HTTP subscription modules to handle the protocol differences. This is technically a breaking v2 API change, but since the field it breaks was only added today, I think it's safe to assume it is not in use yet. Risk level: Low Testing: Integration tests added to validate service endpoint and type URL selection based on transport/resource version. Signed-off-by: Harvey Tuch <htuch@google.com>

Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch

LGTM, need to get CI to pass..

api/envoy/api/v2/core/base.proto

source/server/config_validation/server.cc

Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch · 2020-01-08T17:24:44Z

@yanavlasov LGTM, but format CI check is still failing.

Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch · 2020-01-08T20:04:00Z

I'm going to merge this as is, since the envoy-macos check is known to be broken.

This is a followup to envoyproxy#9301, where it was not possible to deprecate a field on DiscoveryRequest as we were previously assuming identical v2/v3 transport protocols. After this deprecation, build_version can't appear in v3 messages, and we need to convert back to a true v2 DiscoveryRequest prior to JSON serializing for REST. There's more work to be done in the future, when we add new v3-only fields, but this should work for 1.13.0. Future work tracked at envoyproxy#9619. Risk level: Low Testing: coverage of the behaviors for both gRPC and REST config sources is provided by api_version_integration_test (failing previously as soon as build_version was deprecated). Fixes envoyproxy#9604 Signed-off-by: Harvey Tuch <htuch@google.com>

This is a followup to #9301, where it was not possible to deprecate a field on DiscoveryRequest as we were previously assuming identical v2/v3 transport protocols. After this deprecation, build_version can't appear in v3 messages, and we need to convert back to a true v2 DiscoveryRequest prior to JSON serializing for REST. There's more work to be done in the future, when we add new v3-only fields, but this should work for 1.13.0. Future work tracked at #9619. Risk level: Low Testing: coverage of the behaviors for both gRPC and REST config sources is provided by api_version_integration_test (failing previously as soon as build_version was deprecated). Fixes #9604 Signed-off-by: Harvey Tuch <htuch@google.com>

Add extension information the Node message.

2a18f6a

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot added the api label Dec 10, 2019

yanavlasov changed the title ~~Add extension information the Node message.~~ WiP: Add extension information the Node message. Dec 10, 2019

yanavlasov marked this pull request as ready for review December 11, 2019 03:07

htuch self-assigned this Dec 11, 2019

htuch suggested changes Dec 11, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 11, 2019

htuch mentioned this pull request Dec 11, 2019

Fortio output: add missing fields & correct some of the output fields envoyproxy/nighthawk#226

Merged

9 tasks

jpeach reviewed Dec 12, 2019

View reviewed changes

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/core/base.proto Show resolved Hide resolved

include/envoy/registry/registry.h Show resolved Hide resolved

markdroth reviewed Dec 12, 2019

View reviewed changes

api/envoy/api/v2/core/base.proto Show resolved Hide resolved

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

Address comments

60dcdfc

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Dec 13, 2019

Address more comments

a22c3d6

Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch reviewed Dec 13, 2019

View reviewed changes

htuch added the waiting label Dec 13, 2019

Address comments

410c81b

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Dec 14, 2019

yanavlasov added 4 commits December 13, 2019 21:20

Merge branch 'master' into node

4bf1349

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Report disabled extensions.

f54a383

Run proto_format Fix tests with deprecated build_version Signed-off-by: Yan Avlasov <yavlasov@google.com>

Cleanup

adbca59

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Add envoy.config.require-any-fields-contain-struct to the list of doc…

6c8edf6

…umented client features Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch suggested changes Dec 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 17, 2019

alexburnos reviewed Dec 17, 2019

View reviewed changes

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

alexburnos reviewed Dec 17, 2019

View reviewed changes

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

alexburnos reviewed Dec 17, 2019

View reviewed changes

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

Merge branch 'master' into node

cc1dc2f

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot added the api label Jan 7, 2020

yanavlasov added 2 commits January 7, 2020 12:58

Merge branch 'master' into node

80743e2

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Fix build errors after mering master

7a9964f

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Address comments

04c1f3a

Signed-off-by: Yan Avlasov <yavlasov@google.com>

yanavlasov requested review from lizan and zuercher as code owners January 8, 2020 01:04

Merge branch 'master' into node

34ca337

Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch reviewed Jan 8, 2020

View reviewed changes

api/envoy/api/v2/core/base.proto Outdated Show resolved Hide resolved

source/server/config_validation/server.cc Outdated Show resolved Hide resolved

htuch added the waiting label Jan 8, 2020

htuch mentioned this pull request Jan 8, 2020

Mismatch of DiscoveryRequest message and transport API version #9604

Closed

Undeprecate Node.build_version due to issue envoyproxy#9604

1956612

Signed-off-by: Yan Avlasov <yavlasov@google.com>

repokitteh-read-only bot removed the waiting label Jan 8, 2020

yanavlasov added 2 commits January 8, 2020 12:01

Address comments

29c88fb

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Remove comment

514a73b

Signed-off-by: Yan Avlasov <yavlasov@google.com>

Fix proto format

7b22878

Signed-off-by: Yan Avlasov <yavlasov@google.com>

htuch approved these changes Jan 8, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 8, 2020

htuch merged commit f5efe4a into envoyproxy:master Jan 8, 2020

htuch mentioned this pull request Jan 9, 2020

api/config: deprecate build_version in DiscoveryRequest. #9620

Merged

yanavlasov deleted the node branch February 1, 2021 19:29

yanavlasov restored the node branch February 1, 2021 19:29

yanavlasov deleted the node branch December 16, 2022 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Add extension information the Node message. #9301

API: Add extension information the Node message. #9301

yanavlasov commented Dec 10, 2019 •

edited

Loading

repokitteh-read-only bot commented Dec 10, 2019

yanavlasov commented Dec 10, 2019

htuch left a comment

markdroth left a comment

htuch left a comment

htuch Dec 17, 2019

yanavlasov Dec 17, 2019

htuch Dec 18, 2019

yanavlasov Dec 19, 2019

htuch Dec 17, 2019

yanavlasov Dec 17, 2019

htuch Dec 18, 2019

yanavlasov Dec 18, 2019

htuch Dec 18, 2019

yanavlasov Dec 19, 2019

markdroth Jan 6, 2020

yanavlasov Jan 7, 2020

markdroth Jan 7, 2020

alexburnos Dec 17, 2019

yanavlasov Dec 17, 2019

alexburnos Dec 18, 2019

htuch Dec 23, 2019

kyessenov Dec 26, 2019

yanavlasov Dec 26, 2019

kyessenov Dec 26, 2019

htuch Dec 27, 2019

yanavlasov Dec 27, 2019

alexburnos Dec 27, 2019

htuch left a comment

htuch commented Jan 8, 2020

htuch commented Jan 8, 2020

API: Add extension information the Node message. #9301

API: Add extension information the Node message. #9301

Conversation

yanavlasov commented Dec 10, 2019 • edited Loading

repokitteh-read-only bot commented Dec 10, 2019

yanavlasov commented Dec 10, 2019

htuch left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

htuch 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch commented Jan 8, 2020

htuch commented Jan 8, 2020

yanavlasov commented Dec 10, 2019 •

edited

Loading