Reduce allocations by using netip.AddrPort

[kcaffrey]

The normal methods to read and write from net.PacketConn return and accept a net.Addr, respectively. As an interface, this requires at least one allocation for each read. On the write path, this package already makes an effort to cache the addresses returned from reads to reuse, meaning the current usage of WriteTo() does not always allocate. The benchmark added in #941 shows 2 allocs/op on a read/write round trip. Profiling indicates both allocations are on the read path, with one allocation for the *net.UDPAddr struct and a second allocation for the backing IPv4 byte slice.

In Go 1.18 a pair of methods were added to *net.UDPConn to perform allocation-free reads and writes by using the netip.AddrPort struct instead of an interface: ReadFromUDPAddrPort and WriteToUDPAddrPort.

This change reduces the read/write roundtrip to 0 allocs/op for UDP connections (at least when using the built in UDP mux and a connection that supports allocation-free read/writes). A test is added to pin the allocations to zero.

Other than the two aforementioned allocations for every read, there were additional allocations related to net.Addr performed for periodic STUN binding requests (which in many cases happen every several seconds for an established connection). These allocations are reduced a fair amount but not entirely to zero.

This change refactors the internals of the agent to use netip.AddrPort instead of net.Addr where possible. A new exported interface, AddrPortReaderWriter, is added that lets users of the library opt into allocation-free read/writes by implementing the interface for PacketConns. The interface is automatically implemented for *net.UDPConn with an internal adapter. Custom packet connections that don't implement the new interface fall back to the prior behavior.

A few implementation notes:

• I rejected looking for ReadFromUDPAddrPort and WriteToUDPAddrPort directly with an unexported interface, because this could be a back-compat breakage for users who wrapped a UDP connection and only overrode ReadFrom and WriteTo.
• The fallback behavior requires us to keep parallel versions of the old net.Addr and new netip.AddrPort on candidates so that we can reuse the net.Addr in write calls to avoid adding an allocation.
• The NAT response symmetry check now looks at network type explicitly since netip.AddrPort doesn't carry a network type, but I believe this is behavior-preserving and matches the spec.
• Similarly, peer reflexive candidates now determine network type from the local candidate, which I also believe to be behavior-preserving.
• The UDPMux connection maps previously normalized to always preserve zone and map ipv4 to ipv4-in-6. They now use a standard/centralized function to canonicalize, which preserves zone only for ipv6 link-local and unmaps ipv4-in-6 to ipv4. For all practical purposes this should be the same behavior, as kernels usually strip zone from non-link local ipv6 addresses returned in a read call.

There are two known behavior changes:

The first is that previously mDNS candidates would fail to be matched and thus fall back to peer reflexive. This is because matching compared the candidate's SDP address string (Candidate.Address()), which for an mDNS candidate remains the unresolved somehostname.local name, against the stringified source IP of inbound traffic, so a resolved mDNS candidate could never match and traffic fell back to peer reflexive. This change incidentally changed the matching code to compare canonical IP/port to canonical IP/port, and the code will have already set the IP/port for mDNS candidates by the time it tries to match to a remote candidate. This change seems more in line with RFC 8445 (section 7.3.1.3 in particular, with the definition of transport address in section 4 being "address and port") so I kept the change and added a test.

The second is that consumers who call GetXORMappedAddr() or GetXORMappedAddrContext() directly on UniversalUDPMuxDefault with a custom net.Addr whose string representation isn't "ip:port" will now immediately get an error returned rather than the previous behavior of sending the request and getting a timeout error. Based on scanning open source projects that depend on pion/ice, I don't think this will come up in actual usage.

[codecov]

Codecov Report

❌ Patch coverage is 88.71473% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.35%. Comparing base (da70c86) to head (84fdb81).

Files with missing lines	Patch %	Lines
udp_mux.go	81.48%	7 Missing and 3 partials ⚠️
addr.go	87.23%	3 Missing and 3 partials ⚠️
shared_packet_conn.go	81.25%	3 Missing and 3 partials ⚠️
udp_muxed_conn.go	87.50%	4 Missing and 2 partials ⚠️
udp_mux_universal.go	92.15%	3 Missing and 1 partial ⚠️
candidate_base.go	92.50%	2 Missing and 1 partial ⚠️
agent.go	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #943      +/-   ##
==========================================
+ Coverage   88.26%   88.35%   +0.08%     
==========================================
  Files          46       46              
  Lines        6137     6277     +140     
==========================================
+ Hits         5417     5546     +129     
- Misses        494      498       +4     
- Partials      226      233       +7     

Flag	Coverage Δ
go	88.35% <88.71%> (+0.08%)	⬆️

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

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

🚀 New features to boost your workflow:

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

[JoTurk]

@kcaffrey Thank you so much, I reviewed this over the previous week and i don't see any issues. but I would request to break this into 4 PRs or at least 4 commits in this PR so they are easier to check the blame and for a cleaner history.
I would suggest to break the mdns fix into its own commit, and the UniversalUDPMuxDefault change, then the address refactor and later the optimization.

[kcaffrey]

@JoTurk thanks for the review, and for the good suggestion on how to split the commits. I have force pushed with this PR split into four commits roughly matching your suggestion, and agree it's better this way. The diff from my previous single commit to the sum of these four commits is empty, and each commit passes the tests and lints. This seems easier to merge as a single PR (with rebase instead of squash) but if you want me to open this as four separate PRs I can do so.

[JoTurk]

Thank you