Allow configuration of upstream connection limits in Envoy

[crhino]

Fixes #6359

This PR adds a limits field to the upstream configuration of a connect envoy proxy. This limits field is intended to contain configuration regarding connections/requests of the given service to the specified upstream, such as max_connections, max_pending_requests, and max_requests.

This is currently only allowed on the proxy.upstreams[*].config part of the service definition, a future improvement would be to allow these upstream configurations to be set in a service's service-defaults central config.

Example:

services {
  name = "s1"
  port = 8080
  connect {
    sidecar_service {
      proxy {
        upstreams = [
          {
            destination_name = "s2"
            local_bind_port = 5000
            config {
              limits {
                max_connections = 3
                max_pending_requests = 4
                max_requests = 5
              }
            }
          }
        ]
      }
    }
  }
}

UX

Some relevant UX decisions I made, looking for feedback on this.

Limits Block

I chose to scope this section under a limits block in order to more concretely define them as related, as well as simplify the implementation. This should be a natural place to add more related configuration if necessary as well.

I considered naming this something to the effect of connection_limits, but felt that was a little too wordy when typing.

max_* Naming

After looking into the big three proxy configuration (Envoy, Nginx, HAProxy), I eventually settled on keeping the naming that Envoy uses for these values.

For max_connections and max_pending_requests, all three had similar configuration values, although they varied in whether those values were set on a per-server or per-{cluster,frontend} basis.

For max_requests, this represents the max allowed in-flight requests to an upstream cluster by Envoy. This is mostly useful in a HTTP/2 context, since HTTP/1.1 requests are mostly bounded by the max_connections parameter. I was not able to find an equivalent configuration option in HAProxy or Nginx.

I also considered changing max_requests to max_inflight_requests or max_concurrent_requests to be more specific in naming, but decided it was better to keep 100% consistent with Envoy than to change a single name.

TODO

• Update all related documentation with this new configuration value.
• Decide whether to support service-defaults in this PR or future one.

[crhino]

Another high-level question I have regarding this configuration/PR is whether we want to use this opportunity to add support for configuring upstream values like these in service-defaults now, or have that be a future improvement?

From my understanding, centralizing configuration in the service-defaults is the way we want to trend towards, and thus only supporting this in the service definition feels like a slight step backwards.

[banks]

Great job @crhino!

I've not yet dug through code but will soon but want to share initial feedback on the PR description and questions.

Overall I think this looks great and is a sensible place to start. I think the next discussion after this PR is where the right place to add this in service-defaults is (or possible service-resolver but that's a longer conversation...) but let's do that separately.

On the naming choosen, I think all of it is great and appreciate the research and rationale there.

My only suggestion would be that max_requests alone is not super intuitive - it might seem like a hard limit on the total number of requests (which makes no sense) but then could be a maximum number per conection before cycling or per stream or... What do you think about calling it max_requests_inflight or max_concurrent_requests?

whether we want to use this opportunity to add support for configuring upstream values like these in service-defaults now

You're exactly right this is the plan but it's actually way more subtle and difficult that it seems - I have an RFC in progress for it but the sticky part is resolving how upstream ports are assigned and discovered once the definition isn't tied to a specific instance.

So yes that's coming but we shouldn't mix it up here just yet!

[crhino]

I agree max_concurrent_requests is more descriptive than max_requests. My only hesitance is around whether to adhere strictly to Envoy naming conventions, or change to be more descriptive.

I don't think I have a strong opinion on this naming, other than I like max_concurrent_requests more than max_requests_inflight. Will update names according and see how it feels.

[banks]

@crhino this looks great!

I had one minor question inline and I'll wait for docs update to approve as it seems worth doing in one PR.

I like this name better personally and I don't think deviation form Envoy naming is necessarily bad - we are trying to find a middle ground that's agnostic enough for any proxy after all so being precise/meaningful is more important to me than copying Envoy. It's a bit better UX but it also hopefully means other proxy implementations end up implementing something closer since the semantics are more precise.

[crhino]

Link to the preview docs website

Edit: Just realized you can click the Netlify details and it will take you to the preview. 💥

[banks]

Approving because I think this is good as is.

Do you think we should add detail to the docs to point out that max connections doesn't really do much for http2 traffic while max requests doesn't really do what you want for http1?

I don't think that's too envoy specific as the way those protocols use connections is likely to be the same in any proxy?

[ghost]

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.