Proxy envoy ADS requests to Consul server #4

jshahriddhi · 2022-08-29T15:35:06Z

consul-dataplane will allow configuring an xDS server for dynamic envoy configuration. By default, this xDS server will be hosted locally by the consul-dataplane process itself.
This PR adds functionality to host a gRPC server to serve xDS requests within consul-dataplane. The gRPC server mainly acts like a proxy to forward the envoy xDS requests to a Consul server the consul-dataplane process is connected to.

Related JIRA: https://hashicorp.atlassian.net/browse/NET-99

jshahriddhi · 2022-08-29T15:41:51Z

pkg/consuldp/grpc.go

+		// TODO (NET-148): Ensure the server connection here is the one acquired via the server discovery library
+		return outCtx, cdp.consulServer.grpcClientConn, nil
+	}
+	gRPCServer := grpc.NewServer(grpc.UnknownServiceHandler(proxy.TransparentHandler(director)))


The library has most of the scaffolding required to proxy grpc requests to a desired target.
The main proxy logic is here.

boxofrad · 2022-08-30T10:57:32Z

Really like this approach! ⭐

I was a little concerned that we'd end up "manually" proxying the request (i.e. unmarshaling and re-marshaling requests and responses) in a similar way to how we forward RPCs in Consul. This is much better because it means we don't need to consume and upgrade the Envoy protobufs.

I guess one concern is that the library is marked as "proof of concept" but as we control our gRPC version here I don't think it's a particularly big issue.

jshahriddhi · 2022-08-30T13:46:00Z

Elaborating a bit on the library background (will update the PR with comments):
The core library being used is actually this - https://github.com/mwitkow/grpc-proxy. However, we needed this fix and hence I ended up using the forked library.

I guess one concern is that the library is marked as "proof of concept" but as we control our gRPC version here I don't think it's a particularly big issue.

Agreed. But, I did get some confidence in it seeing this - https://github.com/siderolabs/grpc-proxy, that uses mwitkow/grpc-proxy as the core foundation to build on more features.

This is a fork of awesome mwitkow/grpc-proxy package with added support for one to many proxying.

boxofrad · 2022-09-01T15:29:08Z

I realized while reviewing hashicorp/consul-server-connection-manager#3, that when the server terminates an xDS stream to rebalance load, we may need to also close Envoy's connection to get it to reset state (nonces etc.)

I'm not quite sure where that logic would go if using this library. Will do some digging!

Edit: I think it should "just work" and we'll pass the error along to Envoy as-is, which should cause it to retry.

boxofrad

Looks good to me! Left a couple of small points, but nothing blocking 👏🏻

pkg/consuldp/grpc.go

pkg/consuldp/consul_dataplane.go

pkg/consuldp/grpc.go

boxofrad

Lookin' good! I've left a handful of (mostly optional) comments 😅

cmd/consul-dataplane/main.go

pkg/consuldp/config.go

boxofrad · 2022-09-06T20:45:14Z

pkg/consuldp/consul_dataplane.go

+func (cdp *ConsulDataplane) checkAndEnableLocalXDSServer() {
+	if checkLocalXDSServer(cdp.cfg.XDSServer.BindAddress) {
+		cdp.localXDSServer.enabled = true
+	}
+}


It seems odd that somebody would want to use consul-dataplane without the xDS proxy, other than the example in your comment above - which I think is unlikely because they could run consul connect envoy against the server instead.

What do you think about just erroring out when a non-local bind address is given?

I was split about this too. Will check with Matt about the use case of allowing non local xds-bind-address as mentioned in the RFC.

@mkeeler clarified via slack that the address was not intended to be a non-local address (since there is no use case for it).
I will fix up the PR accordingly.

pkg/consuldp/xds.go

boxofrad · 2022-09-08T09:32:42Z

pkg/consuldp/consul_dataplane.go

 		return errors.New("envoy xDS bind address not specified")
-	case cfg.XDSServer.BindPort == 0 && !checkLocalXDSServer(cfg.XDSServer.BindAddress):
-		return errors.New("envoy xDS bind port not specified")
+	case !strings.HasPrefix(cfg.XDSServer.BindAddress, "unix://") && cfg.XDSServer.BindAddress != "127.0.0.1" && cfg.XDSServer.BindAddress != "localhost":


Optional: We might want to parse the address and use IsLoopback in case the user provides an IPv6 address.

mkeeler

LGTM

mkeeler · 2022-09-08T14:58:44Z

pkg/consuldp/bootstrap.go

-			// For now we just give the server address directly.
-			AgentAddress: cdp.consulServer.address.String(),
-			AgentPort:    strconv.Itoa(cdp.cfg.Consul.GRPCPort),
+			AgentAddress: cdp.cfg.XDSServer.BindAddress,


It would be nice to eventually refactor this to not call things Agent*. Its very non-urgent though.

Yea for now we have copy-pasted a lot of this from the existing consul connect command in the consul repo. We can diverge to a more suitable naming at a later point.

jshahriddhi commented Aug 29, 2022

View reviewed changes

jshahriddhi requested review from boxofrad and mkeeler August 29, 2022 21:24

jshahriddhi added 2 commits September 1, 2022 09:48

Proxy envoy ADS requests to Consul server

8fc7c83

Start,stop,exit the grpc server gracefully

dc8c349

jshahriddhi force-pushed the envoy-ads branch from 8c5cefc to dc8c349 Compare September 2, 2022 19:18

boxofrad approved these changes Sep 5, 2022

View reviewed changes

pkg/consuldp/grpc.go Outdated Show resolved Hide resolved

pkg/consuldp/consul_dataplane.go Outdated Show resolved Hide resolved

pkg/consuldp/grpc.go Outdated Show resolved Hide resolved

Pass flags, add tests, cleanup and polish

4dd7652

jshahriddhi marked this pull request as ready for review September 6, 2022 14:18

boxofrad reviewed Sep 6, 2022

View reviewed changes

jshahriddhi added 2 commits September 6, 2022 14:08

Semantic fixes

e2c2ec9

Only allow local xds bind address

943f7a7

boxofrad approved these changes Sep 8, 2022

View reviewed changes

mkeeler approved these changes Sep 8, 2022

View reviewed changes

Check loopback

cd799a4

jshahriddhi merged commit 1dc4bee into main Sep 8, 2022

jshahriddhi deleted the envoy-ads branch September 8, 2022 16:19

jshahriddhi mentioned this pull request Sep 9, 2022

Integrate consul-server-connection-manager library #7

Merged

Proxy envoy ADS requests to Consul server #4

Proxy envoy ADS requests to Consul server #4

Uh oh!

Conversation

jshahriddhi commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boxofrad commented Aug 30, 2022

Uh oh!

jshahriddhi commented Aug 30, 2022

Uh oh!

boxofrad commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boxofrad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

boxofrad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkeeler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jshahriddhi commented Aug 29, 2022 •

edited

Loading

boxofrad commented Sep 1, 2022 •

edited

Loading