Add a callback to handle allocation requests #325
Conversation
rg0now
force-pushed
the
feature_allocation_handler
branch
from
2eb9164
to
70c67df
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #325 +/- ##
==========================================
+ Coverage 67.91% 68.19% +0.27%
==========================================
Files 39 39
Lines 2475 2509 +34
==========================================
+ Hits 1681 1711 +30
- Misses 662 666 +4
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Thanks @rg0now for taken a first shot at this :-) There is not much I can add. I fully agree with your observations 👍 Regarding the redirect handler: couldnt we have two return values, for alternate-server and alternate-domain? |
|
Sure, it's perfectly doable. Any thoughts on the client side API? How should we return the alt-server and alt-domain from I'll update the redirect handler to return the alt-domain too ASAP. |
|
Hi @rg0now,
I think, I would merge the server side handlers first and care about the client side changes via another PR.
I think this would break the API which I would like to avoid. type TryAlternateError struct {
AlternateServer net.Addr
AlternateDomain string
}func main() {
...
conn, err := c.Allocate()
if err != nil {
var altErr *turn.TryAlternateError
if errors.As(err, &altErr) {
connectAlternate(altErr.AlternateServer, altErr.AlternateDomain)
}
}
} |
|
On a second thought: I think we should probably also simplify the signature of the allocation handler to: type AllocationHandler func(clientAddr net.Addr) errorThen we can use the same |
|
I would also prefer to keep handlers closely related to the associated TURN methods. Following my line, we might also need to add an |
|
In my projects, I usually try to use interfaces for the implementation of handlers: type AllocationHandler interface {
Allocate(clientAddr net.Addr, username string) error
AllocationExpired(clientAddr net.Addr, username string) error
}
type PermissionHandler interface {
CreatePermission(clientAddr net.Addr, peerIP net.IP) error
PermissionExpired(clientAddr net.Addr, peerIP net.IP) error
}
type AuthenticationHandler interface {
Authenticate(username, realm string, srcAddr net.Addr) (key []byte, err error)
}
type ServerConfig struct {
Handler any // this handler can implement any of the previous interfaces
}The advantage of tying the handlers to a struct would be, that the user can easily associate state to the handlers (like a quota database). Or implement handlers which implement subset of the aforementioned interfaces. |
|
This makes a lot of sense. In Kubernetes we use API versions to handle breaking API changes: we release a new version (say, v1alpha2 on top of v1alpha1), an automatic converter that upgrades an API call from the old version to the new one, and then we deprecate the old API and stop supporting it after 2-3 releases. I really miss API versioning from Go (or any other programming language for that matter): it would make such API upgrades much simpler. Anyway, here's the current plan, let me know if you're fine with it:
As per the last point, I'm happy to implement the |
|
Hi @rg0now, I am missing just one thing: the I am thinking about attributes like:
What do you think about simply passing the |
|
I'm a bit reluctant to make users having to deal with super-low-level APIs like As per the quota handler, we still have the problem of allowing the caller to track the lifetime of allocations from outside the server (the caller needs to know when allocations are going away). We could call a This would an API like the following (I'm not insisting on the names at all): This lets only one option to the caller to implement user quotas: track all allocations per user by the 5-tuple. Would this be enough? |
Yes, this is the same idea which I had in mind with the The problem I see here is that we do not know the five-tuple at the time when |
I had the same idea. I think this would work as well as we can extend the structure in the future 👍 |
Hmm, I really want to get this right so let's see what is that I don't understand. Let's pass a struct like this to the type AllocationDescriptor struct {
Username stun.Username
FiveTuple allocation.FiveTuple
RequestedTransportProtocol allocation.Protocol
RequestedAddressFamily proto.RequestedAddressFamily
AdditionalAddressFamily proto.RequestedAddressFamily
}The allocation handler interface would look like this: type AllocationHandler interface {
// CreateAllocation is called before the server making a new allocation. Return an error to prevent the allocation from being made.
CreateAllocation(desc AllocationDescriptor) error
// AllocationDeleted is called by the server after deleting an allocation. This is useful to track the lifetime of allocations.
AllocationDeleted(fiveTuple *allocation.FiveTuple)
}We would call the func handleAllocateRequest(r Request, m *stun.Message) error {
// integrity check
// create client-side 5-tuple
fiveTuple := &allocation.FiveTuple{
SrcAddr: r.SrcAddr,
DstAddr: r.Conn.LocalAddr(),
Protocol: allocation.UDP,
}
// check if allocation already exists
if alloc := r.AllocationManager.GetAllocation(fiveTuple); alloc != nil { ... }
// handle RequestedTrasnport, AttrDontFragment, ReservationToken and EvenPort
// call the allocation handler's CreateAllocation callback
desc := alloc.NewAllocationDescriptor()
err := r.AllocationManager.allocationHandler.CreateAllocation(desc)
if err != nil {
// return err to client
}
// create the allocation...
}Another option here would be to call the Finally, func (m *Manager) DeleteAllocation(fiveTuple *FiveTuple) {
m.allocationHandler.AllocationDeleted(fiveTuple)
// delete and close allocation
}Wdyt? |
|
Hey @rg0now, thanks for the detailed description :-) I think I got this wrong. I was under the impression that the five-tuple contains:
However, you seem to be correctly using the clients connection local address (from server perspective) instead of the relayed address. I still need to understand why this works. This way, it seems like the client can not create multiple allocations using the same connection to the server as these would all use the same five-tuple? |
Yes, this strikes me as odd too, but it seems that each The useful consequence of this assumption is that we can use the func (m *Manager) GetAllocation(fiveTuple *FiveTuple) *Allocation {
m.lock.RLock()
defer m.lock.RUnlock()
return m.allocations[fiveTuple.Fingerprint()]
}Maybe the reason of why you're finding this confusing now is that you're deep into the implementation of RFC6062 TCP allocations and the client model is so much different for TCP. |
|
Okay, I've read the TURN RFC (again) 😁 It clearly says that the five-tuple is identifying the client-server connection and that a client may only have a single or dual allocation active (dual as in dual-stack when requesting an allocation with relayed addresses for multiple address familiies e.g. IPv4 + IPv6). See: https://datatracker.ietf.org/doc/html/rfc8656#name-allocations-2 This now also resolves some of my confusion in regard to the TCP allocation PRs which we are currently working on 😃 So now, I can fully support your proposed implementation 👍 Just with one remark: I would try to keep the handlers out of the I still try to understand why somebody thought it was a good idea to have both a public |
First take at #324.
The idea is to make it possible to customize the behavior of the TURN server in order to admit/deny allocation requests (e.g., to implement user quotas) or redirect clients to an alternate server.
Currently the allocation handler is per-listener (i.e., per
PacketConnConfigand perListenerConfig) and it is called just before the server would create a new allocation.The PR contains a minimalistic implementation plus a test to get a feeling of the new API.
Some observations:
client.Allocate(), should the server respond with a "Try Alternate" error (maybe the returned error could have aStatusCodeand aParameterof typeanyto return meaningful errors?) -- an alternative would be to automatically follow redirects in the client until we get to a usable server, but I guess this behavior should be made configurable,ALTERNATE-DOMAINattribute: the current API allows to return an alternate server address, but no alternate domain (see here).My takeaway is that most probably we'll need two distinct APIs to handle user quotas and redirects:
RedirectHandler, plus we may also need to handle the alternate domain case)QuotaHandler:new=truewould mean it's a new allocation (otherwise we're deleting one), and returning (false,nil) would mean to deny access for the new allocation.