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

rcmgr: Add conn_limiter to limit number of conns per ip cidr #2788

Merged
merged 3 commits into from
May 16, 2024

Conversation

MarcoPolo
Copy link
Collaborator

This change makes the resource manager aware of IPs. Adds some reasonable defaults too.

@MarcoPolo MarcoPolo requested a review from sukunrt May 13, 2024 17:15
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Just had a quick glance so far.

connLimitPerCIDRIP4 []ConnLimitPerCIDR
connLimitPerCIDRIP6 []ConnLimitPerCIDR
ip4connsPerLimit []map[string]int
ip6connsPerLimit []map[string]int
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a slice? How are these maps ever garbage collected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It’s a slice because the limits are a slice. Each map corresponds to each limit.

You’re right about the gc of course, I need to delete after removing all conns from a subnet.

Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

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

Should we add an exception for no limits on localhost so that tests don't break?

@MarcoPolo
Copy link
Collaborator Author

Should we add an exception for no limits on localhost so that tests don't break?

Are there any tests that currently break?

@sukunrt
Copy link
Member

sukunrt commented May 15, 2024

I think they're fine.

@MarcoPolo MarcoPolo merged commit 5d547cf into master May 16, 2024
10 of 11 checks passed
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