User-aware relay address generator

[enobufs]

Resolves #420

Description

Details of this feature are described in #420. This PR make following changes:

• Added username argument to AllocatePacketConn and AllocateConn callback methods in RelayAddressGenerator
• Added/updated corresponding unit tests.

NOTCE: This is a breaking change for TURN server applications.

Reference issue

Fixes #420

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 69.23077% with 16 lines in your changes missing coverage. Please review.

Project coverage is 67.43%. Comparing base (3ff9392) to head (2650e82). Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/server/turn.go	75.00%	2 Missing and 3 partials :warning:
internal/allocation/allocation_manager.go	57.14%	2 Missing and 1 partial :warning:
relay_address_generator_none.go	0.00%	2 Missing :warning:
relay_address_generator_range.go	0.00%	2 Missing :warning:
server.go	0.00%	2 Missing :warning:
internal/server/util.go	93.75%	1 Missing :warning:
relay_address_generator_static.go	50.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
+ Coverage   66.63%   67.43%   +0.80%     
==========================================
  Files          43       43              
  Lines        2919     2933      +14     
==========================================
+ Hits         1945     1978      +33     
+ Misses        807      793      -14     
+ Partials      167      162       -5     

Flag	Coverage Δ
go	67.43% <69.23%> (+0.80%)	:arrow_up:
wasm	28.98% <28.84%> (+1.84%)	:arrow_up:

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.

[rg0now]

Thanks a lot, this makes the code much leaner. I guess we'll have to wait for @Sean-Der to merge this though because API changes will require a new release (I guess we'll be at v5 already!).

[jech]

The basic idea is sound, but we're breaking backwards compatibility for a fairly ad hoc feature. If we're going to break compatibility, why don't we do it right, so that we don't need to break it again in the future?

I would suggest defining a new type:

type TurnContext struct {
    Username string
}

and passing that as the new argument. This way, if we ever feel the need to add extra parameters, we can just add them to TurnContext and not break compatibility again.

[enobufs]

Agreed. I will update the code.

[enobufs]

As I was making changes, i had a couple of thoughts:

Name the context as ~AllocationContext~ allocation.Context instead of TurnContext

~I feel it is more descriptive for what it does.~ @jech

Update: I thought about AllocationContext, then realized it is in the allocation package. To avoid stuttering, I ended up using allocation.Context.

Add Real field to the context as well

@rg0now I remember you suggested this earlier and I now feel it is good to have it as a piece of the context.

I will push my local changes shortly. Thanks.

[jech]

To avoid stuttering, I ended up using allocation.Context.

That will cause confusion with context.Context. What about Parameters?

(I'll review your code later.)

[enobufs]

"allocation context" feel right to me, but I agree that it could be confused by context.Context.

I had following candidate in my head:

• allocation.AllocationContext (stuttering but might not be too bad)
• allocation.UserInfo
• allocation.Metadata
• allocation.Session

(I will fix the lint error later - I have just noticed)

[enobufs]

Updated the code, and fixed the lint error also. @jech I chose "Metadata". It is concise and generic enough to accommodate other possible information we may have in the future.

[jech]

Metadata is fine. (Although frankly, why not just use data? What's so meta about it?)

Perhaps you could squash the commits, it would make review easier?

[enobufs]

Thanks @jech, I have squashed the commits.

I feel "metadata" is a suitable term because it represents contextual information related to the operation. It is not primary data, such as network or port number, nor is it a parameter required for the operation. Instead, it serves as supplemental information.

[JoTurk]

Sorry for causing conflicts in your PRs due to the linter updates. Would you like me to fix them for your branch? :)

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