config: Honor transport_api_version for non-ADS xDS services #11788

dio · 2020-06-27T22:46:52Z

Commit Message:

This patch makes sure the chosen endpoint is controlled by the specified transport_api_version. For transport_api_version that equals envoy::config::core::v3::ApiVersion::AUTO, the endpoint will be defaulted to envoy::config::core::v3::ApiVersion::V2.

Additional Description:

Risk Level: Low
Testing: Unit.
Docs Changes: N/A
Release Notes: N/A (no behavior change)
Fix #10650

Signed-off-by: Dhi Aurrahman [email protected]

This patch makes sure the chosen endpoint is controlled by the specified `transport_api_version`. For `envoy::config::core::v3::ApiVersion::AUTO`, the endpoint will be inferred from the `type_url`. Signed-off-by: Dhi Aurrahman <[email protected]>

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-06-28T01:18:44Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-28T01:18:53Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-06-28T04:50:01Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-28T04:50:09Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-06-28T07:45:52Z

Interesting (unrelated) errors on Windows CI:

2020-06-28T05:49:18.1334404Z   C:/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.26.28801/bin/HostX64/x64/cl.exe @bazel-out/x64_windows-opt/bin/test/common/common/_objs/assert_test/assert_test.obj.params
2020-06-28T05:49:18.1335808Z Execution platform: @rbe_windows_msvc_cl//config:platform
2020-06-28T05:49:18.1336807Z test/common/common/assert_test.cc(60): error C2220: the following warning is treated as an error
2020-06-28T05:49:18.1342922Z test/common/common/assert_test.cc(60): warning C4003: not enough arguments for function-like macro invocation '_ENVOY_BUG_VERBOSE'
2020-06-28T05:49:18.1344513Z test/common/common/assert_test.cc(60): error C2059: syntax error: ')'
2020-06-28T05:49:18.1693784Z test/common/common/assert_test.cc(66): warning C4003: not enough arguments for function-like macro invocation '_ENVOY_BUG_VERBOSE'
2020-06-28T05:49:18.1695674Z test/common/common/assert_test.cc(66): error C2059: syntax error: ')'
2020-06-28T05:49:18.1696843Z test/common/common/assert_test.cc(68): warning C4003: not enough arguments for function-like macro invocation '_ENVOY_BUG_VERBOSE'
2020-06-28T05:49:18.1698599Z test/common/common/assert_test.cc(68): error C2059: syntax error: ')'

This is resolved.

htuch

Thanks for taking this on before the next release. Looking at the tests and call sites, I think this makes total sense. However, I'd like to see what we can do to make the implementation in type_to_endpoint.cc a bit more comprehensible to simple folks like me :)

htuch · 2020-06-29T18:36:08Z

source/common/config/type_to_endpoint.cc

+createServiceToVersionedMap(absl::string_view service, envoy::config::core::v3::ApiVersion version,
+                            const std::array<std::string, 2>& versioned_services) {
+  return {{static_cast<std::string>(service),
+           VersionedMap{version,


This is a bit confusing to the reader. Why is there a version, which then maps to more versions and services? Is one of these supported to be transport and one resource version? It's a bit unclear.

I this loop: https://github.com/envoyproxy/envoy/pull/11788/files#diff-539488a0c7c9a3a8c07a208efbafaf08R73.

This is the list of maps (as the input):

[ { "envoy.api.v2.RouteDiscoveryService": { "version_": "V2", "entries_": { "V2": "envoy.api.v2.RouteDiscoveryService", "V3": "envoy.service.route.v3.RouteDiscoveryService" } }, "envoy.service.route.v3.RouteDiscoveryService": { "version_": "V3", "entries_": { "V2": "envoy.api.v2.RouteDiscoveryService", "V3": "envoy.service.route.v3.RouteDiscoveryService" } } }, ... ]

The above list of maps will be transformed into:

{ "type.googleapis.com/envoy.api.v2.RouteConfiguration": { "sotw_grpc_": { "type_url_versions_": { "type.googleapis.com/envoy.api.v2.RouteConfiguration": "V2", "type.googleapis.com/envoy.config.route.v3.RouteConfiguration": "V3" }, "methods_": { "V2": "envoy.api.v2.RouteDiscoveryService.StreamRoutes", "V3": "envoy.service.route.v3.RouteDiscoveryService.StreamRoutes" }, ... } }, "type.googleapis.com/envoy.config.route.v3.RouteConfiguration": { "sotw_grpc_": { "type_url_versions_": { "type.googleapis.com/envoy.api.v2.RouteConfiguration": "V2", "type.googleapis.com/envoy.config.route.v3.RouteConfiguration": "V3" }, "methods_": { "V2": "envoy.api.v2.RouteDiscoveryService.StreamRoutes", "V3": "envoy.service.route.v3.RouteDiscoveryService.StreamRoutes" } }, ... } ... }

So it is convenient to get the effective transport_api_version when the value isAUTO: given a resource_type_url (e.g. type.googleapis.com/envoy.api.v2.RouteConfiguration) and type_url_versions_ map (we carry this along for each entry).

effective_version = type_url_versions_[resource_type_url]

It's probably fine as is, but I'd suggest adding in some type aliases for the keys that are std::string, so it's clear what is being keyed on, and also adding to the code comments some examples like above.

The other alternative is to compute all of this at runtime, instead of building all the maps, but it's unclear to me if that would be any simpler.

htuch · 2020-06-29T18:37:24Z

source/common/config/type_to_endpoint.cc

+};
+
+// Map from resource type URL to versioned service RPC methods.
+using ServiceToVersionedMap = std::unordered_map<std::string, VersionedMap>;


Is all this complexity because you want to support resource/transport independence still? I think it might be clearer if you provide some example maps and explain how this structure is used.

Haha. I think I made this too complicated in the price to support AUTO to default to the inferred "version" from a particular resource type URL. I will try to play around and simplify this.

source/common/config/type_to_endpoint.h

htuch · 2020-06-29T18:45:20Z

@dio re: the macros on Windows, I know that MSVC is much more picky about interleaving of macros and control flow. @wrowe can probably explain the fully story, but I know there were a number of macro fixups during the Windows port.

wrowe · 2020-06-29T19:02:36Z

@dio re: the macros on Windows, I know that MSVC is much more picky about interleaving of macros and control flow. @wrowe can probably explain the fully story, but I know there were a number of macro fixups during the Windows port.

Correct, this covered at line 66 of the proposed porting doc. https://github.com/envoyproxy/envoy/pull/11721/files#diff-d437a64a5901ee0232b3ba2fc7edff7aR66

In short we aren't talking c++ but K&R CPP, and although GCC and CLang make provisions to entangle line parsing and macro invocation, this still isn't part of the spec.

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch

Thanks, this is a lot clearer. Flushing a few comments.
/wait

htuch · 2020-07-06T14:23:38Z

source/common/config/type_to_endpoint.cc

+// }
+using ServiceVersionInfoMap = std::unordered_map<ServiceName, ServiceVersionInfo>;
+
+// This creates a ServiceToServiceVersionInfoMap, with service name (e.g.


This returns a ServiceVersionInfoMap, not ServiceToServiceVersionInfoMap

Thanks for catching this! Updated.

htuch · 2020-07-06T14:24:28Z

source/common/config/type_to_endpoint.cc

+using VersionedMethodMap = std::unordered_map<envoy::config::core::v3::ApiVersion, MethodName>;
+
+struct VersionedDiscoveryType {
+  // This holds a map of type url to its corresponding version. For example:


Corresponding transport or resource version

htuch · 2020-07-06T14:25:32Z

source/common/config/type_to_endpoint.cc

+  // }
+  TypeUrlVersionMap type_url_versions_;
+
+  // Versioned discovery service RPC method fully qualified names. e.g.


Again, would be good to clarify resource or transport version where possible (even if somewhat obvious here). Because, later we do tend to conflate them when making a default decision.

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch · 2020-07-06T15:35:42Z

@dio PR looks good now. I guess one question I have is whether we are technically breaking behavior here (taking a step back). Honoring transport API version makes sense, but the default for AUTO was set as v2, see https://github.com/envoyproxy/envoy/blob/master/api/envoy/config/core/v3/config_source.proto#L27. Do we need a semantically breaking API change as well?

CC @envoyproxy/api-shepherds

dio · 2020-07-06T16:02:07Z

@htuch that's interesting. I missed that part. Before this is in, the behavior was defaulting to v3 when you have v3 resource, so that's why I chose this path for AUTO.

htuch · 2020-07-06T16:31:21Z

@lizan @markdroth interested if you have any opinions on this as API shepherds. Basically, we've been saying that AUTO really means you retain v2 (for backwards compatibility). This PR moved the default transport version when AUTO to that of the resource version. Is this a breaking change? Is it preferable?

markdroth · 2020-07-06T16:53:36Z

Since gRPC hasn't yet implemented v3 support, there are no backward-compatibility issues for us; it's just a question of what behavior we actually want.

Given that both Envoy and gRPC are going to support receiving a different resource version than the one we requested, I am wondering if setting the transport version from the resource version is actually a bit backwards. It seems like what we actually want is the other way around: we should by default set the resource version from the transport version, so that what we send is always consistently within a particular version. What we receive is completely independent of that, so setting the resource version differently from the transport version should be extremely rare. (I don't think we would even bother supporting that ability in gRPC.)

If we do go with setting the transport version from the resource version, the description here will need to be more nuanced, since there are cases where there is no resource version -- e.g., when using SelfConfigSource (see #11754 and #11824).

dio · 2020-07-06T17:00:08Z

It seems like what we actually want is the other way around: we should by default set the resource version from the transport version

Interesting. Since the way of writing config for envoy is by specifying the resource type_url which includes version information and we can skip setting up the transport version (fallback to AUTO).

htuch · 2020-07-06T19:49:48Z

I think going forward, it would be ideal to have explicit versions specified, so we eliminate ambiguity during versions transitions. So, really, we want to discourage use of AUTO. That would lend itself to having this just be v2 as the default and should simplify the implementation. @dio sorry I didn't catch this earlier, I'm happy to be argued out of this position.

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-07-06T22:49:40Z

Sounds good. Updated to default to V2.

htuch

LGTM, thanks!

dio · 2020-07-07T00:55:21Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-07T00:55:29Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-07-07T03:32:16Z

macOS 😔.

dio · 2020-07-07T03:32:28Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-07T03:32:36Z

Azure Pipelines successfully started running 1 pipeline(s).

htuch · 2020-07-07T16:51:58Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-07T16:52:07Z

Azure Pipelines successfully started running 1 pipeline(s).

htuch · 2020-07-07T18:00:25Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-07T18:00:34Z

Azure Pipelines successfully started running 1 pipeline(s).

…oxy#11788) This patch makes sure the chosen endpoint is controlled by the specified transport_api_version. For transport_api_version that equals envoy::config::core::v3::ApiVersion::AUTO, the endpoint will be defaulted to envoy::config::core::v3::ApiVersion::V2. Risk Level: Low Testing: Unit. Docs Changes: N/A Release Notes: N/A (no behavior change) Fix envoyproxy#10650 Signed-off-by: Dhi Aurrahman <[email protected]> Signed-off-by: scheler <[email protected]>

dio added 4 commits June 28, 2020 05:35

Expect v2 from v3 type

fc1501f

Signed-off-by: Dhi Aurrahman <[email protected]>

absl::string_view

f9a6ab2

Signed-off-by: Dhi Aurrahman <[email protected]>

Remove unused comment

a6984a6

Signed-off-by: Dhi Aurrahman <[email protected]>

dio marked this pull request as ready for review June 28, 2020 01:14

Bring back the removed comment

d32d1c4

Signed-off-by: Dhi Aurrahman <[email protected]>

mattklein123 assigned htuch Jun 28, 2020

htuch reviewed Jun 29, 2020

View reviewed changes

dio added 6 commits June 30, 2020 02:34

Merge remote-tracking branch 'upstream/master'

fa14153

Use transport_api_version as name

cceb99b

Signed-off-by: Dhi Aurrahman <[email protected]>

Add comments

ada5650

Signed-off-by: Dhi Aurrahman <[email protected]>

Add ...

5394155

Signed-off-by: Dhi Aurrahman <[email protected]>

Merge remote-tracking branch 'upstream/master'

a187ad9

Signed-off-by: Dhi Aurrahman <[email protected]>

Merge remote-tracking branch 'upstream/master'

19c5fc6

htuch mentioned this pull request Jul 6, 2020

v3: ConfigSource.transport_api_version does not change service name #10650

Closed

htuch suggested changes Jul 6, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 6, 2020

dio added 2 commits July 6, 2020 21:42

Review comments

a83f7f4

Signed-off-by: Dhi Aurrahman <[email protected]>

Merge remote-tracking branch 'upstream/master'

391a4c0

repokitteh-read-only bot removed the waiting label Jul 6, 2020

Default to V2

bea5c32

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch approved these changes Jul 7, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

8273602

htuch merged commit 2309f9b into envoyproxy:master Jul 7, 2020

config: Honor transport_api_version for non-ADS xDS services #11788

config: Honor transport_api_version for non-ADS xDS services #11788

Conversation

dio commented Jun 27, 2020 • edited Loading

dio commented Jun 28, 2020

azure-pipelines bot commented Jun 28, 2020

dio commented Jun 28, 2020

azure-pipelines bot commented Jun 28, 2020

dio commented Jun 28, 2020 • edited Loading

htuch left a comment

Choose a reason for hiding this comment

htuch Jun 29, 2020

Choose a reason for hiding this comment

dio Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

htuch Jun 29, 2020

Choose a reason for hiding this comment

htuch Jun 29, 2020

Choose a reason for hiding this comment

dio Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

htuch commented Jun 29, 2020

wrowe commented Jun 29, 2020

htuch left a comment

Choose a reason for hiding this comment

htuch Jul 6, 2020

Choose a reason for hiding this comment

dio Jul 6, 2020

Choose a reason for hiding this comment

htuch Jul 6, 2020

Choose a reason for hiding this comment

htuch Jul 6, 2020

Choose a reason for hiding this comment

htuch commented Jul 6, 2020

dio commented Jul 6, 2020 • edited Loading

htuch commented Jul 6, 2020

markdroth commented Jul 6, 2020

dio commented Jul 6, 2020

htuch commented Jul 6, 2020

dio commented Jul 6, 2020

htuch left a comment

Choose a reason for hiding this comment

dio commented Jul 7, 2020

azure-pipelines bot commented Jul 7, 2020

dio commented Jul 7, 2020

dio commented Jul 7, 2020

azure-pipelines bot commented Jul 7, 2020

htuch commented Jul 7, 2020

azure-pipelines bot commented Jul 7, 2020

htuch commented Jul 7, 2020

azure-pipelines bot commented Jul 7, 2020

dio commented Jun 27, 2020 •

edited

Loading

dio commented Jun 28, 2020 •

edited

Loading

dio Jun 29, 2020 •

edited

Loading

dio Jun 29, 2020 •

edited

Loading

dio commented Jul 6, 2020 •

edited

Loading