[feature request] connect: service-defaults/service-resolver support for upstream connection limits

[chrisboulton]

Feature Description

#6829 added support for configuring Envoy's circuit breaker settings for max connections/pending requests/concurrent requests for upstreams as part of the upstream configuration on a service (proxy.upstreams[*].config), however deferred the addition of these to central configs.

This means as of today there's no configurability for upstream services provided through the discovery chain, which includes ingress gateways, etc. Because (TMK) for upstreams via the discovery chain you also can't use an escape hatch, we've hit a critical blocker with our Consul Connect/Envoy rollout, as we need to adjust these limits (primarily we need max connections > the default of 1,024 for a heavily trafficked service)

I'm raising this feature request but actually have changes locally that I'd love to contribute upstream that allow configuration of these via a service-defaults entry:

Kind      = "service-defaults"                                          
Name      = "web"       
Protocol  = "http"

Limits = {
  MaxConnections        = 200
  MaxPendingRequests    = 201
  MaxConcurrentRequests = 202
}

There was some discussion by @banks and @crhino in the mentioned PR with respect to the right place to implement configurability for these, either in service-defaults or via a service-resolver. Before I finalise these changes and raise a PR, I wanted to kick off a discussion to make sure I'm aligned to y'alls vision for this:

• Is service-defaults where you see this being configurable, or should it fall under service-resolver?
• Would you expect this to be supported for proxy-defaults for a "set once for all services" ability?
• Does Limits make sense? For me, it's confusing as I'd expect it to be the limits for each of my web service instances but actuality, it's the limits for the upstreams to my service. Does UpstreamLimits make more sense?
• Should the configurability of these at a proxy config/upstreams layer (where these are supported today) be deprecated in favour of configuration of these via config entries?
• There's a subtle difference in the behaviour of this: today at the proxy upstream layer, service-A basically gets to decide what its upstream limits for talking to service-B are. With what I've implemented, all upstreams of service-B would be configured equally - is that okay?
• If I have a service-defaults for Limits.MaxConnections and Limits.MaxPendingRequests, but then a proxy upstream config for just Limits.MaxConnections, would you expect the resulting configuration to merge?

[chrisboulton]

@rboyer - if y'all are open to it, I'd love to find the time to have a chat about an approach here.

I ended up landing on an implementation close to what I describe above, in https://github.com/bigcommerce/consul/compare/0af93a5dfc865dadfb975b58aab2cba775ba64ad..fc270ba01e7457e0721d2951049e8d7790c72b66. I'd love to find a way to get this incorporated into Consul, but I want to make sure it's directionally aligned with the direction HashiCorp wants to go with this kind of thing. I'm happy to just land a pull request and refine from there, if you think that works too.

(We've been in production with this on one of our most heavily trafficked services since the beginning of February, and I consider the changes stable)

[chrisboulton]

Actually, I have a pretty good feeling y'all have a handle on this -- I just ran across #9872

[freddygv]

@chrisboulton Just catching up here. A first pass at it is being released in an experimental alpha likely this week.

Will collect feedback on it over the coming weeks and will have a more finalized design/implementation for the 1.10 beta and GA releases.

[chrisboulton]

Sweet - that's great, and I'll keep my eye out for it because we'd love to help kick the tires, given a bunch of these settings are critical for our Consul Connect implementation.

[freddygv]

Hey there, going to close this given that it was released in the 1.10.0-beta1 last week.

Here are the docs:
https://www.consul.io/docs/connect/config-entries/service-defaults#upstream-configuration-beta