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: Support specific network prefix in conn limiter #2807

Merged
merged 5 commits into from
May 23, 2024

Conversation

MarcoPolo
Copy link
Collaborator

@MarcoPolo MarcoPolo commented May 21, 2024

Despite the field being called "ConnLimitPerCIDR" you weren't able to define limits for a specific CIDR address block. I've renamed this to ConnLimitPerSubnet to make it clearer that this connection limit applies to each subnet with the same N bit prefix.

I've also added an option to allow defining limits on specific address prefix blocks. If an IP address matches one of these blocks then that limit is used instead of the more generic ConnLimitPerSubnet. By default you are allowed unlimited connections from localhost. This should prevent test breakages we've seen.

This does break the existing API. I think that's okay because:

  • We just released this API, so hopefully no one is using it yet. Or, if they are, it's fresh on their mind.
  • Most users probably just stuck with the defaults. And the defaults here are better.
  • The old names were confusing at best.

@MarcoPolo MarcoPolo requested a review from sukunrt May 21, 2024 21:48
@MarcoPolo MarcoPolo changed the title rcmgr: Support cidr in conn limiter rcmgr: Support specific network prefix in conn limiter May 22, 2024
p2p/host/resource-manager/conn_limiter.go Outdated Show resolved Hide resolved
p2p/host/resource-manager/conn_limiter.go Outdated Show resolved Hide resolved
p2p/host/resource-manager/conn_limiter.go Outdated Show resolved Hide resolved
p2p/host/resource-manager/conn_limiter.go Outdated Show resolved Hide resolved
p2p/host/resource-manager/conn_limiter.go Show resolved Hide resolved
p2p/host/resource-manager/conn_limiter.go Show resolved Hide resolved
@MarcoPolo
Copy link
Collaborator Author

On second thought, we shouldn't make a breaking change in a patch release. I'm going to add back the old name of ConnLimitPerCIDR and mark it as deprecated. That will avoid breaking users who update to a patch release. We can remove the poorly named one in the next major version. I'll make an issue for it.

@MarcoPolo MarcoPolo mentioned this pull request May 23, 2024
},
}

func WithLimitPeersPerCIDR(ipv4 []ConnLimitPerCIDR, ipv6 []ConnLimitPerCIDR) Option {
Copy link
Member

Choose a reason for hiding this comment

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

We can't change this name for the patch release. I'm fine with just making a new release. v0.35.

Copy link
Member

Choose a reason for hiding this comment

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

On the same topic can we introduce a new Option in a patch release(WithNetworkPrefixLimit)? It's backwards compatible but definitely not a bug fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, let's make a new release to v0.35.

On the same topic can we introduce a new Option in a patch release(WithNetworkPrefixLimit)? It's backwards compatible but definitely not a bug fix.

Technically you're right. It's not a patch release per semver. We don't have a great way of distinguishing between major and minor releases right now though and I'd argue a minor release is closer to a patch release in spirit than a minor release is to a major relase.

@MarcoPolo
Copy link
Collaborator Author

Plan is to merge this and release it in v0.35 with just this change.

@MarcoPolo MarcoPolo merged commit 9b03220 into master May 23, 2024
11 checks passed
This was referenced May 24, 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.

2 participants