Fix: tcp-alloc example

[AfonsoVilalonga]

Description

Hello!

While trying out the tcp-alloc example in the turn-client examples folder, I came across an unexpected error:

panic: Failed to dial: invalid TURN server address

I documented this behavior in issue #391.

The error originated from line 151 of the example (conn, err := allocation.DialTCP("tcp", nil, peerAddr)). Inside the function DialTCP(), there is the following block of code:

if addr, ok := a.serverAddr.(*net.TCPAddr); ok {
	rAddrServer = &net.TCPAddr{
		IP:   addr.IP,
		Port: addr.Port,
	}
} else {
	return nil, errInvalidTURNAddress
}

The problem stems from the fact that a.serverAddr is of type *net.UDPAddr and not *net.TCPAddr, even though we are making a TCP allocation. Upon further examination of the code, we can verify that when creating a client with the function NewClient(config *ClientConfig), the TURN server address is always resolved as a UDP address, from what I understand.

(client.go lines 104-134, NewClient(config *ClientConfig))

if len(config.STUNServerAddr) > 0 {
	stunServ, err = config.Net.ResolveUDPAddr("udp4", config.STUNServerAddr)
	if err != nil {
			return nil, err
	}

	log.Debugf("Resolved STUN server %s to %s", config.STUNServerAddr, stunServ)
}

if len(config.TURNServerAddr) > 0 {
	turnServ, err = config.Net.ResolveUDPAddr("udp4", config.TURNServerAddr)
	if err != nil {
		return nil, err
	}

	log.Debugf("Resolved TURN server %s to %s", config.TURNServerAddr, turnServ)
}

c := &Client{
		conn:           config.Conn,
		stunServerAddr: stunServ,
		turnServerAddr: turnServ,
		username:       stun.NewUsername(config.Username),
		password:       config.Password,
		realm:          stun.NewRealm(config.Realm),
		software:       stun.NewSoftware(config.Software),
		trMap:          client.NewTransactionMap(),
		net:            config.Net,
		rto:            rto,
		log:            log,
}

Then, the turnServerAddr is used as the value for the ServerAddr of the TCP allocation (see below) when the function AllocateTCP() is called.

(file client.go, lines 353-390)

func (c *Client) AllocateTCP() (*client.TCPAllocation, error) {
	if err := c.allocTryLock.Lock(); err != nil {
		return nil, fmt.Errorf("%w: %s", errOneAllocateOnly, err.Error())
	}
	defer c.allocTryLock.Unlock()

	allocation := c.getTCPAllocation()
	if allocation != nil {
		return nil, fmt.Errorf("%w: %s", errAlreadyAllocated, allocation.Addr())
	}

	relayed, lifetime, nonce, err := c.sendAllocateRequest(proto.ProtoTCP)
	if err != nil {
		return nil, err
	}

	relayedAddr := &net.TCPAddr{
		IP:   relayed.IP,
		Port: relayed.Port,
	}

	allocation = client.NewTCPAllocation(&client.AllocationConfig{
		Client:      c,
		RelayedAddr: relayedAddr,
		ServerAddr:  c.turnServerAddr,
		Realm:       c.realm,
		Username:    c.username,
		Integrity:   c.integrity,
		Nonce:       nonce,
		Lifetime:    lifetime.Duration,
		Net:         c.net,
		Log:         c.log,
	})

	c.setTCPAllocation(allocation)

	return allocation, nil
}

This means the ServerAddr is of type (*net.UDPAddr) and not (*net.TCPAddr). For now, a simple solution is just to add the following code to the DialTCP function:

if addr, ok := a.serverAddr.(*net.TCPAddr); ok {
	rAddrServer = &net.TCPAddr{
		IP:   addr.IP,
		Port: addr.Port,
	}
} else if addr, ok := a.serverAddr.(*net.UDPAddr); ok {
	rAddrServer = &net.TCPAddr{
		IP:   addr.IP,
		Port: addr.Port,
	}
} else {
	return nil, errInvalidTURNAddress
}

I added this and included some documentation about the tcp-alloc in the README of the examples, as there was none.

Thank you for reading! As this is my first PR, any feedback or corrections are more than welcome.

Reference issue

#391

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 68.15%. Comparing base (55407d5) to head (4264a27).

Files	Patch %	Lines
internal/client/tcp_alloc.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage   68.27%   68.15%   -0.12%     
==========================================
  Files          43       43              
  Lines        2348     2352       +4     
==========================================
  Hits         1603     1603              
- Misses        578      582       +4     
  Partials      167      167              

Flag	Coverage Δ
go	68.15% <0.00%> (-0.12%)	⬇️
wasm	27.89% <0.00%> (-0.05%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rg0now]

Looks good to me. I think we can safely ignore the code coverage warning: we cannot test the tcp-alloc code without the server-side implementation.

[AfonsoVilalonga]

Looks good to me. I think we can safely ignore the code coverage warning: we cannot test the tcp-alloc code without the server-side implementation.

Nice, thank you! Is there anything else I need to do related to this pull request?

[rg0now]

We usually wait a couple of days before merging PRs so that others can chime in.

Anyway, I'd like to ask you to do a fresh rebase on the latest master, collapse your commits into a single commit (we usually try to avoid squash-merges so that authorship remains at you) and write a descriptive commit message. Look out, there are some fairly stringent rules on acceptable commit message formatting:

• separate subject from body with a blank line,
• limit the subject line to 50 characters,
• capitalize the subject line,
• do not end the subject line with a period, and
• wrap the body at 72 characters.

[AfonsoVilalonga]

Ok, that makes sense! Will do.

[rg0now]

Thanks a lot, applied