feat(connection-limit): set bypass rules for connections #5720
Conversation
drHuangMHT
changed the title
|
I also found that you can get a mutable reference to the |
I don't know, but trusting an address also kind of makes sense? Though this behaviour is not primarily used for managing trusts between peers. |
Also you can use the rule to allow a range of peers from an address, for example behind a load balancer and such, or a domain(I guess it only works for dialing). Allowing a range of addresses will grant even greater flexibility but it will be a bit difficult to implement. |
There was a problem hiding this comment.
We can also allow all connections from specific listeners.
Not sure I understand what you mean. Won't using the listeners PeerId do exactly that?
I still have a slight preference to only allow bypassing of connection limits based on PeerIds. In allow-block-list we also only operated on PeerIds and not on multiaddresses.
But I don't feel strongly about it, so if from a user perspective it's useful and needed I am okay with also bypassing based on addresses.
misc/connection-limits/src/lib.rs
Outdated
| Kind::EstablishedTotal, | ||
| )?; | ||
|
|
||
| if !(self.bypass_rules.is_addr_bypassed(local_addr) |
There was a problem hiding this comment.
local_addr is our own address at which we accept the connection. I don't think any bypass rules should apply to it?
There was a problem hiding this comment.
That's what "allow connections from specific listeners" mean, at least that's what I believe it will do. For example you set a limit for connections from the Internet but not for intranet.
misc/connection-limits/src/lib.rs
Outdated
| is_bypassed = is_bypassed | ||
| || addresses | ||
| .iter() | ||
| .any(|addr| self.bypass_rules.is_addr_bypassed(addr)); |
There was a problem hiding this comment.
It can still happen that if there are multiple addresses but not all are in the BypassRules, the connection is established to an address that is not bypassed, and then denied in handle_pending_inbound_connection, right?
Should we then add a note on bypass_multiaddr that all known multiaddresses of the remote must be added in order to guarantee that the bypass will succeed?
There was a problem hiding this comment.
I'm not sure because the implementation will get complex very soon if we want to implement AND/OR logic. If we want to make it as simple as possible we should only check against PeerID and use a closure for more fine-grain control, what do you think?
|
Sorry for the late response. Looking at the comments and review, I think what we can do for now is exclude the address portion of the code and allow it based on |
Description
Add
BypassRulesforconnection_limit::Behaviourto allow bypasses.May close #5605
Notes & open questions
This implememtation does not distingush between local addresses and remote addresses, will that be a problem?
Change checklist