Skip to content

api: connection limits#2709

Merged
arkodg merged 6 commits intoenvoyproxy:mainfrom
guydc:api-connection-limits
Mar 15, 2024
Merged

api: connection limits#2709
arkodg merged 6 commits intoenvoyproxy:mainfrom
guydc:api-connection-limits

Conversation

@guydc
Copy link
Copy Markdown
Contributor

@guydc guydc commented Feb 26, 2024

What this PR does / why we need it:
Adds a container for connection-level settings to CTP called Connection. Currently, connection will only support the Limit container 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, each Listener will 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

@guydc guydc requested a review from a team as a code owner February 26, 2024 23:30
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.57%. Comparing base (f52a6f6) to head (a70dba1).

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.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on calling this circuitBreaker ?

Copy link
Copy Markdown
Contributor Author

@guydc guydc Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@guydc guydc Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some examples of how this feature is named in other products:

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... 

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 5, 2024

/retest

1 similar comment
@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 5, 2024

/retest

@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 13, 2024

/retest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on value ? , else connections.limi.maxActiveConnections seems repetitive ...

@guydc guydc force-pushed the api-connection-limits branch from edd130f to e9b3b50 Compare March 14, 2024 16:44
Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc guydc force-pushed the api-connection-limits branch from e9b3b50 to d90aff3 Compare March 14, 2024 16:46
@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 14, 2024

/retest

1 similar comment
@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 14, 2024

/retest

guydc and others added 5 commits March 14, 2024 18:12
Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 15, 2024

/retest

1 similar comment
@guydc
Copy link
Copy Markdown
Contributor Author

guydc commented Mar 15, 2024

/retest

zhaohuabing

This comment was marked as resolved.

Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small thing, “Connection" looks a bit vague, probably ConnectionSetting ? I'm not good at naming anyway.

Not blocking :-)

Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks !

@arkodg arkodg merged commit d145921 into envoyproxy:main Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants