Add a quota handler callback

[rg0now]

Note: the PR is on top of https://github.com/pion/turn/pull/419, see the changes on top of that PR here.

RFC 8656 includes the following text:

To mitigate either intentional or unintentional denial-of-service attacks against the server by clients with valid usernames and passwords, it is RECOMMENDED that the server impose limits on both the number of allocations active at one time for a given username and on the amount of bandwidth those allocations can use. The server should reject new allocations that would exceed the limit on the allowed number of allocations active at one time with a 486 (Allocation Quota Exceeded [sic!]) [error].

(Note the mistake in the text: the name of the error is in fact "Allocation Quota Reached", not "Allocation Quota Exceeded".)

This PR adds a quota handler callback function which, if specified, is called by the server just before making an allocation for a user. The handler should return a single bool: if true then the allocation request can proceed, otherwise the request is rejected with the 486 (Allocation Quota Reached) error. Then, the lifecycle API can be used to track the number of active allocations per user and this callback can be leveraged to reject allocation requests that would exceed the user's quota.

Note that the other DoS mitigation recommendation given in the RFC (limiting the amount of bandwidth a single user can use) is not targeted by this PR.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 91.37931% with 10 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 68.59%. Comparing base (a4a8111) to head (189f430). :warning: Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/server/util.go	73.91%	4 Missing and 2 partials :warning:
internal/allocation/five_tuple.go	50.00%	4 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #423      +/-   ##
==========================================
+ Coverage   67.43%   68.59%   +1.15%     
==========================================
  Files          43       43              
  Lines        3083     3187     +104     
==========================================
+ Hits         2079     2186     +107     
+ Misses        843      839       -4     
- Partials      161      162       +1     

Flag	Coverage Δ
go	68.59% <91.37%> (+1.15%)	:arrow_up:
wasm	25.57% <6.89%> (-0.84%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JoeTurki]

@rg0now Sorry for causing conflicts in your PRs due to the linter updates [1]. Would you like me to fix them for all of your branches?

---

On a side note, I’ve been reviewing past contributions, to understand more about Pion, and I’m a big fan of your reviews. I'm going to get more involved with Pion and would love to submit PRs to implement some of the missing TURN specs (after I finish my todolist for other repos). I’m really looking forward to getting your reviews, can't wait :)

Thank you for all the work you do on Pion/Turn

1. https://github.com/pion/.goassets/pull/220

[rg0now]

@joeturki Just fixed the conflicts, no worries. I super-appreciate your work on pion, what you're doing is so immensely useful! If we are at it, can you please also take a look at https://github.com/pion/turn/pull/419 ?

And thanks for the nice words too. Whenever I have time I come here and do core-reviews. My only worry is that sometimes we may get too picky and this can scare people off (I guess this is how we lost @AndyLc , see https://github.com/pion/turn/pull/315). I guess sometimes we should just let unfinished code in and fix stuff later in order to encourage people to contribute...

[JoeTurki]

@rg0now

Thanks :)

can you please also take a look at https://github.com/pion/turn/pull/419 ?

I did actually take a look, I do like it.

My main perspective on this is from a developer experience (DX) standpoint as a library user. And going through all the Pion repos, I believe we need an abstraction for defining events, systematically:

1. Blocking and behavior-changing events (like the control mechanisms in your PR), That allow us to change a setting or cancel an action etc.
2. Non-blocking telemetry events that allow users to inspect ongoing activity.
3. Events can send custom state.

Something like an event emitter library could help establish consistent behavior for library users, similar to the web EventEmitter, but more tailored to Pion's, I think we need this.

However, I'm not sure, I can starting working on drafting this right now and we can consider postponing it until v5, but I believe we can introduce this without making breaking changes.

My only worry is that sometimes we may get too picky and this can scare people off (I guess this is how we lost @AndyLc , see https://github.com/pion/turn/pull/315).

Feel free to go hard on me as you can, I just want to do things right :)

cc @jech maybe this is something we should add to the v5 wiki?

[jech]

As a general rule, I'm suspicious of callbacks, I prefer direct style whenever possible, especially in Go. In this commit, you're adding a whole bunch of callbacks, so naturally I'm suspicious a whole bunch ;-)

Are all of these callbacks necessary? Perhaps some of these can be omitted, and others can be converted to direct code?

[JoeTurki]

As a general rule, I'm suspicious of callbacks, I prefer direct style whenever possible, especially in Go. In this commit, you're adding a whole bunch of callbacks, so naturally I'm suspicious a whole bunch ;-)

I do agree, But what design pattern do you suggest for making overridable / controllable behavior? Should we expose more of the library and make users define their own Allocation here for example?

[jech]

First of all, we shold consider which of these callbacks are actually needed. My suspicion is that there's only one or two that are actually useful, and that the others are just here for completeness. We should only implement the ones that are provably needed, since we can always add more later, but once we've added them, we won't be able to remove them.

Second, we should consider whether the full generality of a callback is required. Perhaps we can just add a boolean or integer option somewhere, and avoid a callback?

[rg0now]

As a general rule, I'm suspicious of callbacks, I prefer direct style whenever possible, especially in Go. In this commit, you're adding a whole bunch of callbacks, so naturally I'm suspicious a whole bunch ;-)

I do agree, But what design pattern do you suggest for making overridable / controllable behavior?

I think your distinction between callbacks that implement "overridable/controllable behavior" and ones used for telemetry is crucial. For the former (like the auth-handler or the quota-handler) synchronous callbacks seem the best fit, while for the latter (like the event-handler API in https://github.com/pion/turn/pull/419) something asynchronous (like a channel for emitting events) feels way more "right" than the synchronous callbacks we propose above.

Oh, and I agree: pion needs a generic event emitter lib for generalizing this. Do you have any good reference design from a major Go lib we could copy? We could try to prototype the API here in a branch and then, once we're happy with the design we can move it into a separate module.

[rg0now]

First of all, we shold consider which of these callbacks are actually needed. My suspicion is that there's only one or two that are actually useful, and that the others are just here for completeness. We should only implement the ones that are provably needed, since we can always add more later, but once we've added them, we won't be able to remove them. Second, we should consider whether the full generality of a callback is required. Perhaps we can just add a boolean or integer option somewhere, and avoid a callback?

Essentially all the server events defined in https://github.com/pion/turn/pull/419 have been asked or proposed in some way or another earlier: authentication events in https://github.com/pion/turn/pull/402, allocation lifecycle events in https://github.com/pion/turn/issues/324, channel lifecycle events in https://github.com/pion/turn/pull/360, etc., and our OpenTelemetry reference design uses each.

In general I see no reason not to be liberal in defining our events: the hooks come with zero runtime overhead and the more data we surface with events the more future proof pion becomes.

[jech]

Essentially all the server events defined in #419 have been asked or proposed in some way or another earlier:

Oh, good.

In general I see no reason not to be liberal in defining our events: the hooks come with zero runtime overhead and the more data we surface with events the more future proof pion becomes.

If we define a callback now, and then it turns out we need more data in the callback, we cannot easily add a new parameter without breaking compatibility.

If we refrain from defining a callback until there's a demonstrated need for it, there's an increased chance that we'll get it right.

[rg0now]

@JoeTurki I've rebased on top of the latest lifecycle-API PR and fixed all failing tests. pls merge if you think this is OK.