internal/xds: convert xDS resources for the envoy xDS server #3087

jpeach · 2020-11-02T06:09:14Z

Add a snapshotter interfact to paper over the go-control-plane API
bifurcation. Use this to populate v3 and v2 resources in the snapshot
cache.

This updates #1898.

Signed-off-by: James Peach [email protected]

jpeach · 2020-11-02T06:10:22Z

This doesn't work. AFAICT the listeners get populated and send correctly, but perhaps Envoy doesn't like them for some reason. At any rate, the HTTP listener never shows up in the Envoy admin endpoint.

codecov · 2020-11-02T06:13:47Z

Codecov Report

Merging #3087 into main will decrease coverage by 0.44%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3087      +/-   ##
==========================================
- Coverage   73.58%   73.14%   -0.45%     
==========================================
  Files         101      103       +2     
  Lines        6360     6401      +41     
==========================================
+ Hits         4680     4682       +2     
- Misses       1577     1617      +40     
+ Partials      103      102       -1

Impacted Files	Coverage Δ
cmd/contour/serve.go	`0.00% <0.00%> (ø)`
internal/protobuf/helpers.go	`79.41% <0.00%> (-13.70%)`	⬇️
internal/xds/hash.go	`0.00% <0.00%> (ø)`
internal/xds/v2/snapshotter.go	`0.00% <0.00%> (ø)`
internal/xds/v3/snapshotter.go	`0.00% <0.00%> (ø)`
internal/xdscache/snapshot.go	`9.52% <0.00%> (-0.48%)`	⬇️
internal/dag/cache.go	`96.13% <0.00%> (+0.77%)`	⬆️

jpeach · 2020-11-02T08:34:29Z

This doesn't work. AFAICT the listeners get populated and send correctly, but perhaps Envoy doesn't like them for some reason. At any rate, the HTTP listener never shows up in the Envoy admin endpoint.

OK I figured out what is going on. Here's the debug log that gives us the clue:

2020/11/02 19:29:17 returning 2 of 2 rewritten *envoy_api_v2.Listener resources
2020/11/02 19:29:17 getting 1 "type.googleapis.com/envoy.config.listener.v3.Listener" resources

We report adding 2 listeners to the snapshot, but the snapshot only emits 1. I'm calling v3.NewSnapshot() which calls v3.NewResources. NewResources indexes the resources by name, but does it using a type switch on the xDS v3 types. Since we are using the xDS v2 types (on purpose!), they all end up being named "", which is clearly idiotic.

skriss · 2020-11-02T18:45:09Z

internal/xds/hash.go

 // any instance of Envoy to connect to Contour regardless of the
 // service-node flag configured on Envoy.
-type ConstantHash string
+type ConstantHashV2 struct{}


is it worth moving this into internal/xds/v2, and the v3 bits into internal/xds/v3?

I think that I'd rather leave it for now. If I move the 2 implementations I have to either duplicate the "contour" value, or leave the CONSTANT_HASH_VALUE const behind. I don't mind moving them, but I didn't like either of those options very much.

internal/xdscache/snapshot.go

skriss

couple more nits but otherwise LGTM

skriss · 2020-11-02T20:56:54Z

cmd/contour/serve.go

 		case config.ContourServerType:
 			contour_xds_v3.RegisterServer(contour_xds_v3.NewContourServer(log, xdscache.ResourcesOf(resources)...), grpcServer)

 			// Check an internal feature flag to disable xDS v2 endpoints. This is strictly for testing.
-			if config.GetenvOr("CONTOUR_INTERNAL_DISABLE_XDSV2", "N") != "N" {
+			if config.GetenvOr("CONTOUR_INTERNAL_DISABLE_XDSV2", "N") == "N" {


oops, guess we all missed this on the last PR :/

internal/xdscache/snapshot.go

skriss · 2020-11-02T21:06:50Z

internal/protobuf/helpers.go

@@ -101,3 +103,15 @@ func AnyMessageTypeOf(msg proto.Message) string {
 	a := MustMarshalAny(msg)
 	return a.TypeUrl
 }
+
+/// MustMarshalJSON marshals msg to indented JSON.
+func MustMarshalJSON(msg proto.Message) string {


I don't see this being used anywhere, can remove if that's the case.

I ended up having to rewrite it in various ad-hoc debugging, so even though it's not used I'd like to keep it for future debug logging :)

stevesloka · 2020-11-02T21:40:09Z

I see lots of log messages for this:

[2020-11-02 21:38:17.769][1][warning][misc] [source/common/protobuf/message_validator_impl.cc:21] Deprecated field: type envoy.type.matcher.v3.RegexMatcher.GoogleRE2 Using deprecated option 'envoy.type.matcher.v3.RegexMatcher.GoogleRE2.max_program_size' from file regex.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.

Let me look into what needs to happen to remove that, it's not breaking, just very log spammy.

jpeach · 2020-11-02T21:53:05Z

I see lots of log messages for this:

[2020-11-02 21:38:17.769][1][warning][misc] [source/common/protobuf/message_validator_impl.cc:21] Deprecated field: type envoy.type.matcher.v3.RegexMatcher.GoogleRE2 Using deprecated option 'envoy.type.matcher.v3.RegexMatcher.GoogleRE2.max_program_size' from file regex.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.

Let me look into what needs to happen to remove that, it's not breaking, just very log spammy.

That's interesting ... they must be checking the deprecation attributes after the protobuf migration.

stevesloka

/lgtm

Add a snapshotter interface to paper over the go-control-plane API bifurcation. Use this to populate v3 and v2 resources in the snapshot cache. This updates projectcontour#1898. Signed-off-by: James Peach <[email protected]>

jpeach requested review from stevesloka, skriss and youngnick November 2, 2020 06:09

jpeach force-pushed the xds-conversion-envoy branch from 135d523 to c7d1553 Compare November 2, 2020 09:15

jpeach marked this pull request as ready for review November 2, 2020 09:15

skriss requested changes Nov 2, 2020

View reviewed changes

jpeach force-pushed the xds-conversion-envoy branch from c7d1553 to 000987e Compare November 2, 2020 19:48

jpeach mentioned this pull request Nov 2, 2020

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

Merged

skriss approved these changes Nov 2, 2020

View reviewed changes

jpeach force-pushed the xds-conversion-envoy branch from 000987e to a6ba9f7 Compare November 2, 2020 21:35

jpeach force-pushed the xds-conversion-envoy branch from a6ba9f7 to 1d2e79b Compare November 3, 2020 00:05

stevesloka approved these changes Nov 3, 2020

View reviewed changes

internal/xds: convert xDS resources for the envoy xDS server

ac979b0

Add a snapshotter interface to paper over the go-control-plane API bifurcation. Use this to populate v3 and v2 resources in the snapshot cache. This updates projectcontour#1898. Signed-off-by: James Peach <[email protected]>

jpeach force-pushed the xds-conversion-envoy branch from 37d2286 to ac979b0 Compare November 3, 2020 20:10

jpeach merged commit abb1462 into projectcontour:main Nov 3, 2020

jpeach deleted the xds-conversion-envoy branch November 3, 2020 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/xds: convert xDS resources for the envoy xDS server #3087

internal/xds: convert xDS resources for the envoy xDS server #3087

jpeach commented Nov 2, 2020

jpeach commented Nov 2, 2020

codecov bot commented Nov 2, 2020 •

edited

Loading

jpeach commented Nov 2, 2020

skriss Nov 2, 2020

jpeach Nov 2, 2020

skriss left a comment

skriss Nov 2, 2020

jpeach Nov 2, 2020

skriss Nov 2, 2020

jpeach Nov 2, 2020

stevesloka commented Nov 2, 2020

jpeach commented Nov 2, 2020

stevesloka left a comment

internal/xds: convert xDS resources for the envoy xDS server #3087

internal/xds: convert xDS resources for the envoy xDS server #3087

Conversation

jpeach commented Nov 2, 2020

jpeach commented Nov 2, 2020

codecov bot commented Nov 2, 2020 • edited Loading

Codecov Report

jpeach commented Nov 2, 2020

skriss Nov 2, 2020

Choose a reason for hiding this comment

jpeach Nov 2, 2020

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

skriss Nov 2, 2020

Choose a reason for hiding this comment

jpeach Nov 2, 2020

Choose a reason for hiding this comment

skriss Nov 2, 2020

Choose a reason for hiding this comment

jpeach Nov 2, 2020

Choose a reason for hiding this comment

stevesloka commented Nov 2, 2020

jpeach commented Nov 2, 2020

stevesloka left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2020 •

edited

Loading