xDS Server Implementation #4731
Conversation
banks
force-pushed
the
xds-server-config
branch
from
eaba4bb to
f9d5008
Compare
agent/xds/clusters.go
Outdated
| if cfgSnap == nil { | ||
| return nil, errors.New("nil config given") | ||
| } | ||
| // Inlude the "app" cluster for the public listener |
agent/xds/listeners.go
Outdated
| // as JSON and decoding again!! | ||
| cfgStruct, err := util.MessageToStruct(cfg) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
So practically speaking I'm guessing this is never going to happen?
agent/xds/server.go
Outdated
| case stateInit: | ||
| if req == nil { | ||
| // This can't happen (tm) since stateCh is nil until after the first req | ||
| // is recieved but lets not panic about it. |
agent/xds/server.go
Outdated
| // range the map which has no determined order. It's important because: | ||
| // | ||
| // 1. Envoy needs to see a consistent snapshot to avoid potentially | ||
| // dropping or misrouting traffic due to inconsitencies. This is the |
There was a problem hiding this comment.
typo: s/inconsitencies/inconsistencies/
banks
force-pushed
the
xds-server-config
branch
from
f9d5008 to
a8455b2
Compare
agent/proxycfg/manager.go
Outdated
| continue | ||
| } | ||
| // TODO(banks): need to work out when to default some stuff. For example | ||
| // Proxy.LocalServicePort is practially necessary for any sidecar and can |
agent/proxycfg/manager.go
Outdated
| } | ||
| } | ||
|
|
||
| // ensureProxyServiceLocked adss or changes the proxy to our state. |
agent/proxycfg/manager.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // We are updating the proxy, close it's old state |
agent/proxycfg/manager.go
Outdated
| } | ||
|
|
||
| // We are updating the proxy, close it's old state | ||
| state.Close() |
There was a problem hiding this comment.
What about the error this returns?
agent/proxycfg/manager.go
Outdated
| ch := state.Watch() | ||
| for { | ||
| select { | ||
| case snap, ok := <-ch: |
There was a problem hiding this comment.
If you only have the one case, why bother with the select at all? You could just do a bare snap, ok := <-ch
agent/proxycfg/manager.go
Outdated
|
|
||
| // Closing state will let the goroutine we started in Ensure finish since | ||
| // watch chan is closed. | ||
| state.Close() |
agent/proxycfg/manager.go
Outdated
| // TODO(banks): should we close their chan here to force them to eventually | ||
| // notice they are too slow? Not sure if it really helps. | ||
| select { | ||
| case ch <- snap: |
There was a problem hiding this comment.
Is this the thing I already commented on in the prior PR with the head-swapping of the topmost item in a chan-of-size-1?
agent/proxycfg/manager.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // removeProxyService is called when a service deregisteres and frees all |
… for custom listeners
|
Last commits addressed most of the points here which were awesome. I also realised that the "let the user give us custom config" was a bit too liberal to be useful at all - they would also not get Connect certs or Authz that way! The new behaviour implemented (and tested) in last few commits is to override the TLS config and insert the ext_authz filter onto the start of every configured filter chain even ones coming from the user. This at least ensures their main public listener as advertised in Consul will have the right TLS configs and with do Auth correctly before anything else. I also realised we probably need to allow customisation of the Cluster config too for some features but we can iterate on that after this PR/initial release. |
* Vendor updates for gRPC and xDS server * xDS server implementation for serving Envoy as a Connect proxy * Address initial review comments * consistent envoy package aliases; typos fixed; override TLS and authz for custom listeners * Moar Typos * Moar typos
* Vendor updates for gRPC and xDS server * xDS server implementation for serving Envoy as a Connect proxy * Address initial review comments * consistent envoy package aliases; typos fixed; override TLS and authz for custom listeners * Moar Typos * Moar typos
* Vendor updates for gRPC and xDS server * xDS server implementation for serving Envoy as a Connect proxy * Address initial review comments * consistent envoy package aliases; typos fixed; override TLS and authz for custom listeners * Moar Typos * Moar typos
* Vendor updates for gRPC and xDS server * xDS server implementation for serving Envoy as a Connect proxy * Address initial review comments * consistent envoy package aliases; typos fixed; override TLS and authz for custom listeners * Moar Typos * Moar typos
| } | ||
|
|
||
| func makeFilter(name string, cfg proto.Message) (envoylistener.Filter, error) { | ||
| // Ridiculous dance to make that pbstruct into types.Struct by... encoding it |
There was a problem hiding this comment.
@banks I love this comment, hits close to home 😂 seems like an unavoidable dance sometimes
Note: this PR targets merging into f-envoy and depends directly on #4730. This is part of a series of PRs that were developed together but split for easier review.
This is the actual xDS API server implementation. It authenticates gRPC streams, fetches config and watches for changes from the
proxycfg.Managerfrom #4729, and then delivers updates to Envoy for TLS, upstream listeners and service discovery results.Overview
Notably Missing
connect envoyCLI command and some other plumbing will be in a separate PR next. End to end tests aren't really possible at this point so I'll wait.