Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2709 +/- ##
=======================================
Coverage 64.57% 64.57%
=======================================
Files 122 122
Lines 21115 21115
=======================================
Hits 13636 13636
Misses 6632 6632
Partials 847 847 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
thoughts on calling this circuitBreaker ?
There was a problem hiding this comment.
Yea, Connection is a very vague name... I think that CircuitBreaker implies that some sort of failure or unexpected degradation in service triggers the blocking behavior, which isn't really the case here. WDYT about Resources or Limits?
There was a problem hiding this comment.
isn't this the same as https://gateway.envoyproxy.io/latest/user/circuit-breaker/
I'm not tied to a specific name, but hoping both downstream and upstream use the same name e.g. connectionPool
There was a problem hiding this comment.
Some examples of how this feature is named in other products:
- Nginx:
limit_conn- https://nginx.org/en/docs/http/ngx_http_limit_conn_module.html - Kemp:
client connection limiter(this is actually more of a connection rate limiter than a total connection limiter) - https://support.kemptechnologies.com/hc/en-us/articles/115004547103-How-To-Limit-The-Number-Of-Connections-Per-Second-From-A-Specific-Host - HAProxy:
maxconn- https://www.haproxy.com/documentation/haproxy-configuration-manual/latest/#maxconn - Traefik:
InFlightConn- https://doc.traefik.io/traefik/v3.0/middlewares/tcp/inflightconn/ - F5:
Connection Limits- https://techdocs.f5.com/kb/en-us/products/big-ip_ltm/manuals/product/ltm-implementations-13-1-0/25.html - Gloo Edge:
ConnectionLimits- https://docs.solo.io/gloo-edge/main/guides/security/limit-connection/
So, I feel that Connection[Limits|Settings|Options] is a pretty intuitive name for a container which can also include other settings from the timeout and circuit breaker domains. However, not everything is symmetric, due to missing envoy functionality.
connectionSettings:
maxParallelConnections: 10000 # *[CircuitBreaker;Symmetric]* Total active connections
maxNewConnectionsPerSecond: 100 # [NEW;Downstream] CPS rate limit, in theory such a rate limit can also be implemented for clusters, but is not supported today
maxConnectionBufferSize: 100kb # *[NEW;Symmetric]* per connection memory allocation
maxConnectionIdleTime: 600s # *[Timeout; Symmetric]*
maxConnectTime: 10s # *[Timeout; Symmetric]* time to establish a connection
http:
maxRequestsPerConnection: 5000 # *[CircuitBreaker; Symmetric]* close connection after this amount of request
maxConnectionDuration: 3600s # *[CircuitBreaker; Symmetric]* close connection after this duration
maxParallelRequests: 1000 # *[CircuitBreaker; Upstream]* No envoy downstream equivalent that I know of, but could maybe be implemented through a simplified concurrency controller for adaptive concurrency filter. https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/adaptive_concurrency_filter#concurrency-controllers).
maxPendingRequest: 500 # *[CircuitBreaker; Upstream]* only for upstream, don't think that it makes sense for envoy to queue downstream requests... There was a problem hiding this comment.
thanks for doing the research !
I'm on board with the top level connection field now :)
my only follow up since limit is not a struct but a value, how do we expose the delay knob, which is useful to thwart ddos
|
/retest |
1 similar comment
|
/retest |
|
/retest |
api/v1alpha1/connection_types.go
Outdated
There was a problem hiding this comment.
thoughts on value ? , else connections.limi.maxActiveConnections seems repetitive ...
edd130f to
e9b3b50
Compare
Signed-off-by: Guy Daich <guy.daich@sap.com>
e9b3b50 to
d90aff3
Compare
|
/retest |
1 similar comment
|
/retest |
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com> Signed-off-by: Guy Daich <guy.daich@sap.com>
|
/retest |
1 similar comment
|
/retest |
What this PR does / why we need it:
Adds a container for connection-level settings to CTP called
Connection. Currently, connection will only support theLimitcontainer for setting the max concurrent limits and close delay. Limits will be enforced using the Envoy Connection Limit Filter.When a CTP with a connection limit attaches to a
Gateway, eachListenerwill inherit the parent gateway's limit. This proposal does not deal with Envoy listener runtime connection limits.In the future, we can consider using this container for #2600, #2670, downstream TCP keepalive, connection local rate limits, ...
Which issue(s) this PR fixes:
Relates to #2685