connmgr: Limit max connections per host.#3698
Conversation
davecgh
force-pushed
the
connmgr_maxsamehost
branch
3 times, most recently
from
1d1f2bf to
8524bd9
Compare
davecgh
force-pushed
the
connmgr_maxsamehost
branch
2 times, most recently
from
92268d2 to
a295a18
Compare
| Listeners: []net.Listener{listener}, | ||
| MaxNormalConns: 30, // High enough to not interfere with per-host tests. | ||
| MaxConnsPerHost: maxConnsPerHost, | ||
| TargetOutbound: maxConnsPerHost, |
There was a problem hiding this comment.
Shouldn't TargetOutbound be a number greater than maxConnsPerHost, similar to MaxNormalConns, to ensure the number of conns is definitely being limited by MaxConnsPerHost and not by TargetOutbound?
There was a problem hiding this comment.
I don't believe so. It is intentionally limiting the target number of outbounds it tries to automatically make to that limit so the test can ensure they are all allowed. Then the tests attempt to add more to the same host (both whitelisted and not, manual, pending, and persistent) in order to ensure it's actually being limited for those cases.
If it were to try to make more automatic outbound connections, they'd end up repeatedly being rejected (because the address func is only returning addresses for the same host) which would mess up the rest of the tests due to the pending connections and inability to to effectively pause the connections on demand via the trick that forces the addrs returned to be errors forcing the auto outbounds into the retry backoff.
I'm open to suggestions there, but, I was unable to find a better way to effectively pause it without needing to put extra instrumentation in the production code that only exists for the sole purpose of allowing tests to control it.
davecgh
force-pushed
the
connmgr_maxsamehost
branch
from
a295a18 to
a09f8f5
Compare
davecgh
force-pushed
the
connmgr_maxsamehost
branch
from
a09f8f5 to
9e2eaf8
Compare
davecgh
force-pushed
the
connmgr_maxsamehost
branch
from
9e2eaf8 to
5055f01
Compare
| } | ||
|
|
||
| // Limit the max number of connections per host. | ||
| err = cm.rejectMaxConnsPerHost(rAddr, rAddrHostKey, isWhitelisted, false) |
There was a problem hiding this comment.
Sorry, found one more thing to ask you about.
Inbound per-host limiting isn't atomic. The check here holds connMtx, but the matching addActiveConn increment runs in the goroutine spawned just below, so it lands under a separate lock hold. Since the accept loop is serial, a burst from one host can pass N checks before any goroutine increments, so the extra conns get seated and persist until close. I reproduced this with MaxConnsPerHost=1, which seats a same-host burst well past the limit. The total-conn semaphore still caps the absolute count, but per-host monopolization is exactly what this is meant to stop. Outbound doesn't have the gap since dial reserves its slot under one hold, and the MaxSameIP check this PR removes was atomic too. Should inbound reserve the slot before spawning (only OnAccept needs the goroutine), or is it intended to be best-effort?
There was a problem hiding this comment.
Yes, it was intended to be best effort because, in practice, it's fast enough that you can only realistically spray connections fast enough from localhost which is already exempt anyway.
That said, I think it's probably worth it to just take the time to switch the pattern over to a reservation style which is generally more robust. That would not only make it fully atomic, it should also simplify it a bit in that it would only need to increment in one place (instead of multiple) and be more consistent with the semapohores style.
There was a problem hiding this comment.
Also, I saw you had already approved, but I haven't addressed the config param = 0 yet either.
There was a problem hiding this comment.
Oh, by "ill update it later" I thought you meant you'd do it in another PR
There was a problem hiding this comment.
Here is what I'm thinking for an updated design:
- Move rejection out of
dialand instead require a per-host permit prior to dialing (but instead of a per-host semaphore, it would still use the map to keep memory bound during floods) - Make the inbound conns try to acquire a permit and reject if it can't (same semantics as the total conns semaphore)
- Obviously the accounting would no longer happen in the add/remove of pending/active conns and instead be done exactly as the semaphores are done
- Modify
AddPersistentto manually count the number of persistent conns (established or not) to the same host and reject if adding it would be >MaxConnsPerHost. I think it needs to manually count for this particular check instead of using theperHostCountsmap because there might not be any slots available at the exact moment of adding the persistent connection, but they could become available later
I believe that will have better behavior across the board.
Sound reasonable?
There was a problem hiding this comment.
I like it. For 4th point, hopefully the rejection error makes it clear it's specifically too many persistent conns for that host, so folks know to bump maxsameip or drop one of the existing ones. Accepting the add when the host is temporarily saturated by other conns and letting retries grab a slot later sounds good.
There was a problem hiding this comment.
I've reworked this accordingly.
I pulled in a few of commits from PR #3701 related to using the concrete address more often and waiting for all goroutines to finish because it's now needed earlier and the tests need the latter for stability.
For now, I just added a couple of commits that make the changes so they're easier to review in isolation. I'll squash them into the associated commits before merging though.
davecgh
force-pushed
the
connmgr_maxsamehost
branch
from
5055f01 to
3ec06c3
Compare
Currently the whitelisting logic happens in the server which makes it inaccessible to the connection manager. In order to pave the way for supporting various connection-related logic that currently happens in the server, but ideally should be happening in the connection manager, this adds basic support for whitelisting CIDR prefixes to the connection manager. The connection manager config struct now accepts a slice of prefixes and a new method named IsWhitelisted is added. Note that this only adds support . It does not update anything to use the new functionality yet.
This adds tests to ensure the new whitelist detection method works as expected.
This modifies the server to pass in the parsed whitelist entries to the connection manager config and the relevant code to make use of the new method it exposes. Finally, it removes the no longer used local isWhitelisted method.
This adds a new TryAcquire method to the context-aware semaphore. As the name implies, the method supports conditionally acquiring the semaphore only when resources are immediately available. In other words, it will not block when there are no resources immediately available.
This adds tests for the new TryAcquire method on the context-aware semaphore to ensure the semantics work as expected.
The current overall total connection limits are enforced by the server rather than the connection manager. This is not ideal for many reasons, but one of the most important consequences is that it makes DoS attacks easier. Another example of some less than ideal behavior that it allows is that some rare combinations of events can lead to temporary extra connection churn. It is much more robust and natural to perform the limiting in the connection manager itself via semaphores. That approach not only significantly hardens the server against DoS attacks and solves various edge cases present in the current code, it also paves the way for even more advanced features such as traffic shaping in the future. To that end, this adds semaphore-based limiting for the total overall number of normal connections to the connection manager and removes the relevant current limiting for it from the server. Normal connections are the automatic outbound, manual outbound, and inbound connections. Persistent connections, on the other hand, are not subject to the limit since they have their own limiting. This is consistent with them not being subject to the automatic target outbound limit either.
This adds tests to ensure that the new max normal connection limiting properly enforces the limit including automatic outbound, manual outbound, and inbound connections. It also ensures that it not applied to persistent connections.
This modifies the persistent connection and target outbound handler shutdown logic to wait for any goroutines they have launched to finish before returning. This ensures there are no dangling goroutines from them once Run returns.
This updates the primary parsing method in the connection manager tests to return a concrete addrmgr address instead of a stdlib net.TCPAddr. The goal is to eventually use the concrete address type almost everywhere to avoid a lot of the less than ideal address reparsing and repeated host/port splitting and joining.
This modifies the address handling to parse the stdlib net.Addr to a concrete addrmgr address earlier in the connect and persistent paths rather than doing it in the dial method and switch the callback to get new addresses to return the concrete type. The server is updated to simply the return the chosen address which no longer needs to be converted. Note that this is theoretically a semantics change because the code in server previously potentially resolved the address via DNS and that no longer is the case. In practice, nothing is really changing in terms of resolution though because the address manager only ever works with resolved addresses since it needs to gossip addresses via the wire protocol which only deals with resolved addresses.
Similar to the recent total normal connection limiting, the current per-host connection limits are enforced by the server. For similar reasons, it is much more robust and natural to perform the limiting early in the connection manager. With that in mind, this implements the per-host connection limiting in the connection manager and removes the relevant current limiting for it from the server. The limiting is applied to inbound, outbound, and persistent connections. The new limiting is handled early in both the inbound and outbound paths now which allows it to take advantage of fast connection shedding for inbound connections and to preemptively prevent all outbound attempts that would exceed the limit regardless of source. It also provides the flexibility to apply independent special permissions in the future. This also slightly changes the semantics to exempt whitelisted addresses for both inbound and outbound connections as opposed to only inbound connections.
This adds tests to ensure that the new max connections per host limiting properly enforces the limit including automatic outbound, manual outbound, inbound, and persistent connections. It also tests whitelisted addresses are exempt.
davecgh
force-pushed
the
connmgr_maxsamehost
branch
from
3ec06c3 to
8d1b5bf
Compare
davecgh
force-pushed
the
connmgr_maxsamehost
branch
from
8d1b5bf to
ba92435
Compare
3 participants
This requires #3697.
Similar to the recent total normal connection limiting, the current per-host connection limits are enforced by the server. For similar reasons, it is much more robust and natural to perform the limiting early in the connection manager.
With that in mind, this implements the per-host connection limiting in the connection manager and removes the relevant current limiting for it from the server. The limiting is applied to inbound, outbound, and persistent connections.
The new limiting is handled early in both the inbound and outbound paths now which allows it to take advantage of fast connection shedding for inbound connections and to preemptively prevent all outbound attempts that would exceed the limit regardless of source. It also provides the flexibility to apply independent special permissions in the future.
This also slightly changes the semantics to exempt whitelisted addresses for both inbound and outbound connections as opposed to only inbound connections.
It also adds tests to ensure that the new max connections per host limiting properly enforces the limit including automatic outbound, manual outbound, inbound, and persistent connections and also that whitelisted addresses are exempt.