Update filter chain creation for sidecar/ingress listeners #11245
Conversation
|
|
| cfgSnap, | ||
| nil, | ||
| ) | ||
| if upstreamCfg != nil && upstreamCfg.HasLocalPortOrSocket() { |
There was a problem hiding this comment.
The outboundListener being nil doesn't really matter here. That just indicates that it's not a transparent proxy.
However, creating explicit listeners applies to all sidecar proxies: transparent or not. The only thing that matters is having a local port or socket.
| if !useRDS { | ||
| // When not using RDS we must generate a cluster name to attach to the filter chain. | ||
| // With RDS, cluster names get attached to the dynamic routes instead. | ||
| target, err := simpleChainTarget(chain) |
There was a problem hiding this comment.
I had to stare at this a lot.
So ultimately down in the listener construction clusterName!="" is required when !useRDS and vice versa.
So up here it only makes sense to compute a clusterName if not doing RDS ✔️
I was concerned that the chain.IsDefault() code was omitted here, but after thinking about it more, if you have a default chain (and not a prepared query), then you resolved that USING the upstream naming attributes (dest/dc/ns/ap) and there was no redirect so using the SNI target name for the chain you already have is effectively doing the same thing so the conditional branch for IsDefault() isn't necessary here ✔️ ❓
There was a problem hiding this comment.
In this block either the protocol is TCP or the chain is default. If the protocol is TCP and the chain is not default then there could be a redirect, but that should be OK since there should only be one node in the chain.
Say you have a service web with a redirect to web in dc2. There would be a single chain node with target web.default.default.dc2, which is what we're looking for. The cluster name needs to be for the redirect target because the xds cluster will also have that name:
Lines 610 to 612 in 7d95d90
If there is a resolver with failovers there would also only be a single discovery chain node, despite having multiple chain targets.
agent/xds/listeners.go
Outdated
| dc := u.Datacenter | ||
| if dc == "" { | ||
| dc = cfgSnap.Datacenter | ||
| } |
There was a problem hiding this comment.
You don't need this here since u is block-local and created just above without a Datacenter field populated. You can just pass cfgSnap.Datacenter into Sprintf
| filterName := fmt.Sprintf("%s.%s.%s.%s", chain.ServiceName, chain.Namespace, chain.Partition, chain.Datacenter) | ||
|
|
||
| l := makePortListenerWithDefault(id, address, u.LocalBindPort, envoy_core_v3.TrafficDirection_OUTBOUND) | ||
| filterChain, err := s.makeUpstreamFilterChain(filterChainOpts{ |
There was a problem hiding this comment.
By switching to the new function here you've lost your EnvoyListenerJSON escape hatch.
There was a problem hiding this comment.
You could probably fix that by adding this blurb right after the cfg := ...
if cfg.EnvoyListenerJSON != "" {
listener, err := makeListenerFromUserConfig(cfg.EnvoyListenerJSON)
if err != nil {
return nil, err
}
resources = append(resources, listener)
continue
}
There was a problem hiding this comment.
Ingress gateways never actually supported escape hatches, their upstream definitions are generated from this data:
https://www.consul.io/docs/connect/config-entries/ingress-gateway#services
| @@ -45,19 +44,42 @@ func (s *ResourceGenerator) makeIngressGatewayListeners(address string, cfgSnap | |||
| id := u.Identifier() | |||
|
|
|||
| chain := cfgSnap.IngressGateway.DiscoveryChain[id] | |||
There was a problem hiding this comment.
I feel like the answer is yes, but is chain actually guaranteed to be non-nil here?
There was a problem hiding this comment.
Actually not so sure. If the IG config entry comes back first then you'll loop over that before all of the chains come in, so maybe the right thing in this loop is :
if chain == nil {
continue // wait until we get the chain
}
There was a problem hiding this comment.
Adding this if-nil-continue check doesn't appear to break anything so that should resolve the incidental nil-ness due to eventual consistency.
agent/xds/testdata/listeners/ingress-with-sds-listener-listener-level.envoy-1-19-x.golden
Outdated
Show resolved
Hide resolved
freddygv
force-pushed
the
xds/chain-refactor
branch
from
02d5139 to
0c97455
Compare
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/498544. |
The duo of
makeUpstreamFilterChainForDiscoveryChainandmakeListenerForDiscoveryChainwere really hard to reason about, and led to concealing a bug in their branching logic. There were several issues here:Rather than handling all of those tasks inside of these functions, this PR pulls out the RDS/clusterName/filterName logic.
This refactor also fixed a bug with the handling of UpstreamDefaults. These defaults get stored as UpstreamConfig in the proxy snapshot with a DestinationName of "*", since they apply to all upstreams. However, this wildcard destination name must not be used when creating the name of the associated upstream cluster. The coalescing logic in the original functions here was in some situations creating clusters with a
*.prefix, which is not a valid destination.Each commit shows the progressive untangling.