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

feat: add max-connections-per-listener config option #6058

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

flawedmatrix
Copy link
Contributor

Adds a setting for max-connections-per-listener to limit the number of active connections to a listener.

fixes #5654

@flawedmatrix flawedmatrix requested a review from a team as a code owner January 6, 2024 01:02
@flawedmatrix flawedmatrix requested review from skriss and sunjayBhatia and removed request for a team January 6, 2024 01:02
@flawedmatrix
Copy link
Contributor Author

cc @christianang

@sunjayBhatia sunjayBhatia requested review from a team and clayton-gonsalves and removed request for a team January 6, 2024 01:05
Copy link

github-actions bot commented Jan 6, 2024

Hi @flawedmatrix! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

@sunjayBhatia sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Jan 8, 2024
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c1dd2da) 78.81% compared to head (c593eac) 78.82%.
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6058      +/-   ##
==========================================
+ Coverage   78.81%   78.82%   +0.01%     
==========================================
  Files         138      138              
  Lines       19765    19784      +19     
==========================================
+ Hits        15577    15595      +18     
- Misses       3885     3886       +1     
  Partials      303      303              
Files Coverage Δ
cmd/contour/servecontext.go 83.92% <100.00%> (+0.03%) ⬆️
internal/xdscache/v3/runtime.go 100.00% <100.00%> (ø)
pkg/config/parameters.go 86.51% <100.00%> (+0.11%) ⬆️
cmd/contour/serve.go 20.75% <0.00%> (-0.03%) ⬇️

... and 2 files with indirect coverage changes

@flawedmatrix flawedmatrix force-pushed the listener-conn-limit branch 2 times, most recently from d91c36f to 5886377 Compare January 8, 2024 19:59
@sunjayBhatia
Copy link
Member

breadcrumb to think about when I get a chance to do a full review: think about admin/stats/etc. Listeners

@sunjayBhatia
Copy link
Member

breadcrumb to think about when I get a chance to do a full review: think about admin/stats/etc. Listeners

so far my thought is to exclude those Listeners from this change since the shouldn't be at as much risk from a DOS attack (should not be exposed outside of the cluster, or even node), but we could do a follow on to set a reasonable default number of max connections for these listeners (or even make the limit configurable)

If we do want to ever set the global connection limit, that applies to all Listeners including the admin one, stats, etc., so we would have to make sure those don't get starved, and maybe set the "ignore global limit" flag for them

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

One little changelog nit, otherwise LGTM. I agree with @sunjayBhatia that it should be fine to not apply this setting to the admin/stats listeners.

changelogs/unreleased/6058-flawedmatrix-minor.md Outdated Show resolved Hide resolved
Setting Max Connections Per Listener sets the limit on the number of
active connections to a listener.

Co-authored-by: Christian Ang <[email protected]>
Signed-off-by: Edwin Xie <[email protected]>
@flawedmatrix
Copy link
Contributor Author

Yeah, that makes sense. If we ever need to set a listener connection limit on the admin and stats endpoint that can be a follow-on change.

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, will leave for @sunjayBhatia to review as well. Thanks @flawedmatrix and @christianang!

@sunjayBhatia sunjayBhatia enabled auto-merge (squash) January 10, 2024 16:00
@sunjayBhatia sunjayBhatia merged commit 9e8e129 into projectcontour:main Jan 10, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make downstream Listener connection limit configurable
3 participants