internal/xds: convert v3 protobufs automatically #3074

jpeach · 2020-10-28T04:59:08Z

Signed-off-by: James Peach jpeach@vmware.com

jpeach · 2020-10-28T04:59:23Z

codecov · 2020-10-28T05:03:20Z

Codecov Report

Merging #3074 into main will decrease coverage by 0.38%.
The diff coverage is 44.82%.

@@            Coverage Diff             @@
##             main    #3074      +/-   ##
==========================================
- Coverage   73.99%   73.61%   -0.39%     
==========================================
  Files         101      101              
  Lines        6280     6360      +80     
==========================================
+ Hits         4647     4682      +35     
- Misses       1530     1576      +46     
+ Partials      103      102       -1

Impacted Files	Coverage Δ
cmd/contour/serve.go	`0.00% <0.00%> (ø)`
internal/xds/util.go	`45.45% <40.84%> (-54.55%)`	⬇️
internal/xds/v3/contour.go	`67.79% <77.77%> (-1.30%)`	⬇️
internal/protobuf/helpers.go	`93.10% <100.00%> (+0.51%)`	⬆️
internal/xds/v2/contour.go	`89.09% <100.00%> (ø)`
internal/dag/cache.go	`96.13% <0.00%> (+0.77%)`	⬆️

stevesloka · 2020-10-28T19:47:05Z

Thanks for this @jpeach! This is a neat approach to moving form one version to the next. Taking some time to think through this a bit more and how it might integrate I'm leaning towards our original plan of duplicating code into a v3 package.

One goal was to allow users to decide if they want to run a v2 or v3 cluster, so doing this approach won't let us serve v2 once we start migrating over to v3 unless, like you mentioned, we'd need to convert from v3 back to v2.
There is a deprecated field with that I'm aware of already (DrainConnectionsOnHostRemoval has been removed from v3), so we'd have a special case there to convert. I'm not sure if there are more or not.

I'm still open to discussing this moving forward since this is a pretty big decision. What is everyone else's thoughts?

internal/xds/util.go

jpeach · 2020-10-30T00:00:16Z

I verified that migrating the protobufs through JSON serialization drops the migrated fields:

2020/10/30 10:47:01 original *envoy_api_v2.Cluster:
{"name":"default/httpbin/80/f7b4732cd5","altStatName":"default_httpbin_80","type":"EDS","edsClusterConfig":{"edsConfig":{"apiConfigSource":{"apiType":"GRPC","grpcServices":[{"envoyGrpc":{"clusterName":"contour"}}]}},"serviceName":"default/httpbin/http"},"connectTimeout":"0.250s","healthChecks":[{"timeout":"2s","interval":"10s","unhealthyThreshold":3,"healthyThreshold":2,"httpHealthCheck":{"host":"contour-envoy-healthcheck","path":"/status/200"}}],
"commonLbConfig":{"healthyPanicThreshold":{}},"drainConnectionsOnHostRemoval":true}

2020/10/30 10:47:01 converted *envoy_config_cluster_v3.Cluster:
{"name":"default/httpbin/80/f7b4732cd5","altStatName":"default_httpbin_80","type":"EDS","edsClusterConfig":{"edsConfig":{"apiConfigSource":{"apiType":"GRPC","transportApiVersion":"V3","grpcServices":[{"envoyGrpc":{"clusterName":"contour"}}]},"resourceApiVersion":"V3"},"serviceName":"default/httpbin/http"},"connectTimeout":"0.250s","healthChecks":[{"timeout":"2s","interval":"10s","unhealthyThreshold":3,"healthyThreshold":2,"httpHealthCheck":{"host":"contour-envoy-healthcheck","path":"/status/200"}}],
"commonLbConfig":{"healthyPanicThreshold":{}}}

Envoy has internal code that migrates field in the version converter. Possibly if we just rewrite the typeURL the internal envoy code will do the right thing. Experimenting ...

jpeach · 2020-10-30T00:08:52Z

Envoy has internal code that migrates field in the version converter. Possibly if we just rewrite the typeURL the internal envoy code will do the right thing. Experimenting ...

Confirmed that this works:

"cluster": {
      "@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
      "name": "default/httpbin/80/f7b4732cd5",
      "type": "EDS",
      "eds_cluster_config": {
       "eds_config": {
        "api_config_source": {
         "api_type": "GRPC",
         "grpc_services": [
          {
           "envoy_grpc": {
            "cluster_name": "contour"
           }
          }
         ]
        }
       },
       "service_name": "default/httpbin/http"
      },
      "connect_timeout": "0.250s",
      "health_checks": [
       {
        "timeout": "2s",
        "interval": "10s",
        "unhealthy_threshold": 3,
        "healthy_threshold": 2,
        "http_health_check": {
         "host": "contour-envoy-healthcheck",
         "path": "/status/200"
        }
       }
      ],
      "common_lb_config": {
       "healthy_panic_threshold": {}
      },
      "alt_stat_name": "default_httpbin_80",
      "ignore_health_on_host_removal": true
     },

However, doing this once at the top level ends up with Envoy requesting a mixture of v2 and v3 resources:

INFO[0007] handling v3 xDS resource request              connection=3 context=xds node_id=envoy-t84qj node_version=v1.16.0 resource_names="[]" response_nonce=1 type_url=type.googleapis.com/envoy.config.listener.v3.Listener version_info=1
INFO[0007] handling v3 xDS resource request              connection=3 context=xds node_id=envoy-t84qj node_version=v1.16.0 resource_names="[]" response_nonce=1 type_url=type.googleapis.com/envoy.config.listener.v3.Listener version_info=1
INFO[0009] handling v2 xDS resource request              connection=2 context=xds node_id=envoy-t84qj node_version=v1.16.0 resource_names="[ingress_http]" response_nonce=1 type_url=type.googleapis.com/envoy.api.v2.RouteConfiguration version_info=1
INFO[0009] handling v2 xDS resource request              connection=2 context=xds node_id=envoy-t84qj node_version=v1.16.0 resource_names="[ingress_http]" response_nonce=1 type_url=type.googleapis.com/envoy.api.v2.RouteConfiguration version_info=1

So we probably need to rewrite all the interior typeURLs too.

youngnick

LGTM with one small comment; I think @stevesloka needs to approve too though, as I'm not familiar enough with the protos to be 100%.

internal/xds/util.go

stevesloka

Testing through this on my local cluster I keep getting log messages like this:

2020/10/30 10:42:12 missing conversion for *envoy_config_accesslog_v2.FileAccessLog, good luck
2020/10/30 10:42:12 missing conversion for *envoy_config_filter_http_lua_v2.Lua, good luck
2020/10/30 10:42:12 missing conversion for *envoy_config_filter_http_ext_authz_v2.ExtAuthz, good luck
2020/10/30 10:42:12 missing conversion for *envoy_config_accesslog_v2.FileAccessLog, good luck
2020/10/30 10:42:12 missing conversion for *envoy_config_filter_http_ext_authz_v2.ExtAuthzPerRoute, good luck
2020/10/30 10:42:12 missing conversion for *envoy_extensions_filters_network_http_connection_manager_v3.HttpConnectionManager, good luck

internal/xds/util.go

internal/xds/v3/contour.go

Force support for xDS v3 by mapping the v3 endpoints to the internal v2 resource caches. Since the xDS upgrade is wire-compatible, we can force an initial upgrade my rewriting the type URLs of embedded messages and forcing v3 in the dynamic resource configuration. This updates projectcontour#1898. Signed-off-by: James Peach <jpeach@vmware.com>

skriss

No other comments; LGTM assuming testing's OK (has seemed fine in my limited smoke-tests).

jpeach added this to In progress in Contour Project Board via automation Oct 28, 2020

jpeach force-pushed the xds-convert-resources branch 3 times, most recently from 5a7c045 to 68a6754 Compare October 29, 2020 08:38

skriss reviewed Oct 29, 2020

View reviewed changes

internal/xds/util.go Show resolved Hide resolved

stevesloka mentioned this pull request Oct 29, 2020

V3 feature tests authorization #3080

Closed

jpeach force-pushed the xds-convert-resources branch from 0f0ba79 to bc79a4c Compare October 30, 2020 05:17

jpeach marked this pull request as ready for review October 30, 2020 05:19

youngnick approved these changes Oct 30, 2020

View reviewed changes

internal/xds/util.go Show resolved Hide resolved

jpeach force-pushed the xds-convert-resources branch 3 times, most recently from 0bdf204 to 095a894 Compare October 30, 2020 06:18

stevesloka suggested changes Oct 30, 2020

View reviewed changes

internal/xds/util.go Outdated Show resolved Hide resolved

skriss reviewed Oct 30, 2020

View reviewed changes

internal/xds/v3/contour.go Outdated Show resolved Hide resolved

jpeach force-pushed the xds-convert-resources branch from 095a894 to d59b1c9 Compare October 30, 2020 21:28

skriss approved these changes Oct 30, 2020

View reviewed changes

jpeach merged commit 59c9e43 into projectcontour:main Nov 2, 2020

Contour Project Board automation moved this from In progress to 1.10 Release Nov 2, 2020

jpeach deleted the xds-convert-resources branch November 2, 2020 00:34

danehans mentioned this pull request Nov 2, 2020

Adds xDS Resource Version to Envoy DaemonSet projectcontour/contour-operator#96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/xds: convert v3 protobufs automatically #3074

internal/xds: convert v3 protobufs automatically #3074

jpeach commented Oct 28, 2020

jpeach commented Oct 28, 2020

codecov bot commented Oct 28, 2020 •

edited

stevesloka commented Oct 28, 2020

jpeach commented Oct 30, 2020

jpeach commented Oct 30, 2020 •

edited

youngnick left a comment

stevesloka left a comment

skriss left a comment

internal/xds: convert v3 protobufs automatically #3074

internal/xds: convert v3 protobufs automatically #3074

Conversation

jpeach commented Oct 28, 2020

jpeach commented Oct 28, 2020

codecov bot commented Oct 28, 2020 • edited

Codecov Report

stevesloka commented Oct 28, 2020

jpeach commented Oct 30, 2020

jpeach commented Oct 30, 2020 • edited

youngnick left a comment

Choose a reason for hiding this comment

stevesloka left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 28, 2020 •

edited

jpeach commented Oct 30, 2020 •

edited