Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Max Requests Per Connection in Backend Traffic Policy #2508

Closed
guydc opened this issue Jan 26, 2024 · 3 comments · Fixed by #2513 or #2539
Closed

Support Max Requests Per Connection in Backend Traffic Policy #2508

guydc opened this issue Jan 26, 2024 · 3 comments · Fixed by #2513 or #2539
Labels

Comments

@guydc
Copy link
Contributor

guydc commented Jan 26, 2024

Description:

Envoy's Common HTTP Protocol Options can be used to set a limit on the total number of requests that can be handled by a connection. Once the limit is reached, the connection is closed. Several envoy-based projects expose this setting for upstream connections.

This parameter can be used for two purposes:

  • effectively disabling upstream connection reuse
  • coordinating persistent connection lifespan with the upstream backend. For example, nginx resets persistent connection after 100/1k requests by default. If the proxy does not preemptively close the connection, a TCP race condition may occur, leading to request failures.

Relevant Links:
Contour: https://projectcontour.io/docs/main/config/api/#projectcontour.io/v1alpha1.ClusterParameters
Istio: https://istio.io/latest/docs/reference/config/networking/destination-rule/#ConnectionPoolSettings-HTTPSettings
Gloo Edge: https://docs.solo.io/gloo-edge/1.7.23/reference/api/github.com/solo-io/gloo/projects/gloo/api/v1/connection.proto.sk/#connectionconfig

@guydc guydc added the triage label Jan 26, 2024
@arkodg
Copy link
Contributor

arkodg commented Jan 26, 2024

should we split up circuitBreaker into http and tcp and move this there ?

@guydc
Copy link
Contributor Author

guydc commented Jan 26, 2024

should we split up circuitBreaker into http and tcp and move this there ?

I think that envoy circuit breakers currently apply cluster-level limits on concurrent resource use (concurrent connections, requests in flight, requests in queue). This setting will apply a connection-level limit on total resources (count of requests). So, this will slightly generalize the meaning of circuit breaker in EG.

I think that the istio ConnectionPoolSettings is an interesting interface covering both types of limits (and other attributes).

For the time being, I think that we can add this setting to circuit breaker as you suggest. Or, we can consider a new top-level container that will also include circuit breaker, timeout, keep alive and possibly other BTP attributes. WDYT?

@arkodg
Copy link
Contributor

arkodg commented Jan 26, 2024

yeah from a user perspective, this field is about limiting a request (returning early) when the circuit trips, so Id vote to put it in circuitBreaker

connectionPool as another top level field doesnt feel very useful to me - I like retry , circuitBreaker at the top level, but others might find that too overwhelming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants