api: xDS API version specifier #9468

Shikugawa · 2019-12-24T02:52:07Z

Signed-off-by: shikugawa [email protected]

Description: It enables to specify API versioning for xDS for #8421, allow to write like this config

eds_cluster_config:
    eds_config:
       version: V2 
       api_config_source:
          api_type: GRPC
	  grpc_services:
            envoy_grpc:
               cluster_name: eds_cluster

Risk Level: Low
Testing: unit test(maybe it is needed to add integration test)
Docs Changes: needed
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: shikugawa <[email protected]>

repokitteh-read-only · 2019-12-24T02:52:12Z

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

🐱

Caused by: #9468 was opened by Shikugawa.

see: more, trace.

lizan · 2019-12-24T09:22:50Z

api/envoy/api/v2/core/config_source.proto

 message ConfigSource {
+  enum XdsApiVersion {
+    // set api version automatically, default version is set as v2.


I'm wondering whether this should be match the version of ConfigSource, i.e. if the type is v3alpha.ConfigSource, use v3alpha. WDYT @htuch?

Making it explicit (as done in this PR) has the advantage that we can support mixed v2/v3 configurations, e.g. LDS loads from a v3 server but RDS comes from v2. We may get into a situation where a v3 Listener one day is expecting to have some v3-related route table delivered, but I think we can enforce these through consistency checking at config ingestion, the general case is that we support mixed.

To be clear, I meant this for AUTO, we should definitely keep the rest of options explicit.

Yeah, I think that would be reasonable semantics.

I think let's just say that it's V2 for now, it would be ideal to have folks specifically exactly what they are after going forward, and this will encourage that.

htuch

This is great @Shikugawa, definitely aligned with where I'm thinking. For this PR, I would suggest you get the build working and existing tests passing. You can skip adding new tests for v3 for now, I'll sync after some more PRs have landed and we can discuss the v3 tests. In my WiP banch, I have some support for testing with mixed v2/v3.

htuch · 2019-12-24T13:26:54Z

api/envoy/api/v2/core/config_source.proto

 message ConfigSource {
+  enum XdsApiVersion {
+    // set api version automatically, default version is set as v2.


Making it explicit (as done in this PR) has the advantage that we can support mixed v2/v3 configurations, e.g. LDS loads from a v3 server but RDS comes from v2. We may get into a situation where a v3 Listener one day is expecting to have some v3-related route table delivered, but I think we can enforce these through consistency checking at config ingestion, the general case is that we support mixed.

api/envoy/api/v3alpha/core/config_source.proto

Signed-off-by: shikugawa <[email protected]>

htuch · 2019-12-26T01:36:18Z

FYI, when #9480 lands, we'll need to use those annotations for the type URLs in this PR.

htuch · 2019-12-26T12:48:21Z

@Shikugawa any update here? Would be great to land this soon so we can work on v3 wire xDS.

Shikugawa · 2019-12-26T16:33:40Z

@htuch there is no update here, but build is failing on CI. This should be resolved, my local build process can work fine so It seems something wrong on CI build process.

htuch · 2019-12-26T17:10:06Z

@Shikugawa looking at the CI logs, I'm guessing this might be some interaction with the package movement that @lizan added for SRDS, see https://dev.azure.com/cncf/envoy/_build/results?buildId=24529&view=logs&j=4e3e56d1-4574-5496-ade8-f4f41cead1dc&t=481a44f5-6d65-59dc-f824-e3a9563c67cf.

Signed-off-by: shikugawa <[email protected]>

htuch

@Shikugawa this LGTM modulo nits and some tests. I think we just need to validate the correspondence between presented ConfigSource version and the type URL that gets used. Thanks.
/wait

source/common/router/vhds.cc

htuch · 2019-12-27T21:00:05Z

api/envoy/api/v2/core/config_source.proto

 message ConfigSource {
+  enum XdsApiVersion {
+    // set api version automatically, default version is set as v2.


I think let's just say that it's V2 for now, it would be ideal to have folks specifically exactly what they are after going forward, and this will encourage that.

Signed-off-by: shikugawa <[email protected]>

…fier Signed-off-by: shikugawa <[email protected]>

Signed-off-by: shikugawa <[email protected]>

htuch

Thanks @Shikugawa. I'm going to merge this as is to unblock other v3 works, please do add some unit tests (I'm going to be adding integration tests shortly as well).

This PR extends envoyproxy#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 envoyproxy#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 <[email protected]>

It enables to specify API versioning for xDS for envoyproxy#8421, allow to write like this config eds_cluster_config: eds_config: version: V2 api_config_source: api_type: GRPC grpc_services: envoy_grpc: cluster_name: eds_cluster Risk Level: Low Testing: unit test(maybe it is needed to add integration test) Signed-off-by: shikugawa <[email protected]> Signed-off-by: Prakhar <[email protected]>

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 <[email protected]>

enable version switching from config source

d845446

Signed-off-by: shikugawa <[email protected]>

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

lizan reviewed Dec 24, 2019

View reviewed changes

htuch reviewed Dec 24, 2019

View reviewed changes

Shikugawa added 2 commits December 25, 2019 15:00

Kick CI

d25a1a2

Signed-off-by: shikugawa <[email protected]>

refactor

f054a3d

Signed-off-by: shikugawa <[email protected]>

htuch added the api/v3 Major version release @ end of Q3 2019 label Dec 26, 2019

Shikugawa added 2 commits December 27, 2019 08:03

merge api_boost

aca8ccb

Signed-off-by: shikugawa <[email protected]>

merge

6ced70e

Signed-off-by: shikugawa <[email protected]>

Shikugawa marked this pull request as ready for review December 27, 2019 08:29

htuch suggested changes Dec 27, 2019

View reviewed changes

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

delete invalid const and fix description of auto flag

17f26e0

Signed-off-by: shikugawa <[email protected]>

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

delete invalid const

e38b282

Signed-off-by: shikugawa <[email protected]>

Shikugawa requested review from junr03, mattklein123 and snowp as code owners December 30, 2019 06:46

Shikugawa added 2 commits December 30, 2019 07:02

Merge remote-tracking branch 'upstream/master' into xds-version-speci…

5fab749

…fier Signed-off-by: shikugawa <[email protected]>

proto format

4ab383d

Signed-off-by: shikugawa <[email protected]>

Shikugawa force-pushed the xds-version-specifier branch from fe7d23b to 4ab383d Compare December 30, 2019 07:09

htuch approved these changes Dec 30, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Dec 30, 2019

htuch merged commit e3717b5 into envoyproxy:master Dec 30, 2019

htuch mentioned this pull request Dec 30, 2019

config: distinct resource/transport API versions. #9526

Merged

Shikugawa mentioned this pull request Jan 10, 2020

xDS: type url aggregation #9649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: xDS API version specifier #9468

api: xDS API version specifier #9468

Shikugawa commented Dec 24, 2019

repokitteh-read-only bot commented Dec 24, 2019

lizan Dec 24, 2019

htuch Dec 24, 2019

lizan Dec 24, 2019

htuch Dec 24, 2019

htuch Dec 27, 2019

htuch left a comment

htuch Dec 24, 2019

htuch commented Dec 26, 2019

htuch commented Dec 26, 2019

Shikugawa commented Dec 26, 2019

htuch commented Dec 26, 2019

htuch left a comment

htuch Dec 27, 2019

htuch left a comment

api: xDS API version specifier #9468

api: xDS API version specifier #9468

Conversation

Shikugawa commented Dec 24, 2019

repokitteh-read-only bot commented Dec 24, 2019

lizan Dec 24, 2019

Choose a reason for hiding this comment

htuch Dec 24, 2019

Choose a reason for hiding this comment

lizan Dec 24, 2019

Choose a reason for hiding this comment

htuch Dec 24, 2019

Choose a reason for hiding this comment

htuch Dec 27, 2019

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch Dec 24, 2019

Choose a reason for hiding this comment

htuch commented Dec 26, 2019

htuch commented Dec 26, 2019

Shikugawa commented Dec 26, 2019

htuch commented Dec 26, 2019

htuch left a comment

Choose a reason for hiding this comment

htuch Dec 27, 2019

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment