Added TCP support to vnet #375
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #375 +/- ##
==========================================
- Coverage 84.58% 82.93% -1.65%
==========================================
Files 38 41 +3
Lines 3074 3756 +682
==========================================
+ Hits 2600 3115 +515
- Misses 341 448 +107
- Partials 133 193 +60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func (c *TCPConn) SetWriteDeadline(t time.Time) error { | ||
| // Writes do not block in vnet. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
We have had many bugs due to deadlines with TCP and i think tcp vnet should be able to emulate real writes, as well as blocked writes. Non-blocking writes kinda defeat the purpose of testing tcp with vnet for many cases.
(I'm just going through this fast before i eventually review this in few days lol)
There was a problem hiding this comment.
Done, added deadlines, ACKs and small internal queue of 10 packets.
sirzooro
force-pushed
the
tcp_support
branch
11 times, most recently
from
25aba74 to
fb434b0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds TCP support to the vnet package, implementing virtual TCP connections, listeners, NAT translation, and comprehensive test coverage. The implementation includes TCP connection state machine, handshake logic, data transfer with ACK mechanism, and integration with the existing virtual network infrastructure.
Changes:
- Added TCP connection (
TCPConn) and listener (TCPListener) implementations with full handshake and state management - Extended NAT to support TCP translation for both NAPT and 1:1 NAT modes
- Added comprehensive test coverage for TCP functionality including connection tests, NAT behavior tests, and integration tests
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vnet/tcp_conn.go | Core TCP connection implementation with state machine, handshake, read/write operations |
| vnet/tcp_listener.go | TCP listener implementation for accepting incoming connections |
| vnet/tcp_map.go | Port mapping structures for TCP listeners and connections |
| vnet/tcp_test.go | Integration test for TCP dial/listen functionality |
| vnet/tcp_conn_test.go | Unit tests for TCP connection behavior |
| vnet/tcp_nat_test.go | Comprehensive NAT behavior tests for TCP |
| vnet/tcp_map_test.go | Unit tests for TCP mapping structures |
| vnet/net.go | Extended Net to support TCP operations, added constants and routing logic |
| vnet/nat.go | Extended NAT translator to support TCP in addition to UDP |
| vnet/chunk.go | Enhanced TCP chunk structure with sequence/ACK numbers |
| vnet/net_test.go | Added TCP loopback and dial tests |
| vnet/chunk_test.go | Updated test to use tcp constant |
| vnet/errors.go | Added nolint directive for single-use function |
| vnet/udpproxy_direct_test.go | Updated to use network constants instead of string literals |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // BadCase: Invalid address, error and ignore. | ||
| tcpAddr, err := net.ResolveTCPAddr("tcp4", "192.168.1.10:8000") | ||
| tcpAddr, err := net.ResolveTCPAddr(tcp4, "192.168.1.10:8000") |
There was a problem hiding this comment.
The string literal "tcp4" should be replaced with the constant tcp4 defined in net.go for consistency with the rest of the codebase.
|
|
||
| // BadCase: Invalid target address, ignore. | ||
| udpAddr, err := net.ResolveUDPAddr("udp4", "10.0.0.12:5788") | ||
| udpAddr, err := net.ResolveUDPAddr(udp4, "10.0.0.12:5788") |
There was a problem hiding this comment.
The string literal "udp4" should be replaced with the constant udp4 defined in net.go for consistency with the rest of the codebase.
vnet/tcp_nat_test.go
Outdated
| t, | ||
| oec1.(*chunkTCP).sourcePort, | ||
| oec3.(*chunkTCP).sourcePort, | ||
| "should match", |
There was a problem hiding this comment.
The assertion message is incorrect. Since the assertion is NotEqual, the message should be "should not match" instead of "should match".
| "should match", | |
| "should not match", |
vnet/tcp_conn_test.go
Outdated
| } | ||
| log.Debug("read data") | ||
| assert.Equal(t, len(data), n, "should match") | ||
| assert.Equal(t, string(data), string(data), "should match") |
There was a problem hiding this comment.
This assertion is comparing string(data) with itself instead of comparing the received data with the expected data. It should be comparing string(buf[:n]) with string(data).
| assert.Equal(t, string(data), string(data), "should match") | |
| assert.Equal(t, string(data), string(buf[:n]), "should match") |
vnet/tcp_conn_test.go
Outdated
| } | ||
| log.Debug("read data") | ||
| assert.Equal(t, len(data), n, "should match") | ||
| assert.Equal(t, string(data), string(data), "should match") |
There was a problem hiding this comment.
This assertion is comparing string(data) with itself instead of comparing the received data with the expected data. It should be comparing string(buf[:n]) with string(data).
| assert.Equal(t, string(data), string(data), "should match") | |
| assert.Equal(t, string(data), string(buf[:n]), "should match") |
vnet/nat.go
Outdated
| errNoAssociatedLocalAddress = errors.New("no associated local address") | ||
| errNoNATBindingFound = errors.New("no NAT binding found") | ||
| errHasNoPermission = errors.New("has no permission") | ||
| errNATRequriesMapping = errors.New("1:1 NAT requires more than one mapping") |
There was a problem hiding this comment.
The error variable name has a typo: "errNATRequriesMapping" should be "errNATRequiresMapping" (missing the 'i' in 'Requires').
| errNATRequriesMapping = errors.New("1:1 NAT requires more than one mapping") | |
| errNATRequiresMapping = errors.New("1:1 NAT requires more than one mapping") |
vnet/nat.go
Outdated
| errNoNATBindingFound = errors.New("no NAT binding found") | ||
| errHasNoPermission = errors.New("has no permission") | ||
| errNATRequriesMapping = errors.New("1:1 NAT requires more than one mapping") | ||
| errMismatchLengthIP = errors.New("length mismtach between mappedIPs and localIPs") |
There was a problem hiding this comment.
The error message has a typo: "mismtach" should be "mismatch".
| errMismatchLengthIP = errors.New("length mismtach between mappedIPs and localIPs") | |
| errMismatchLengthIP = errors.New("length mismatch between mappedIPs and localIPs") |
sirzooro
force-pushed
the
tcp_support
branch
4 times, most recently
from
d6022aa to
1078635
Compare
2 participants
Added TCP support to vnet. Github Copilot/GPT-5.2 did a good job :)
Ref: #27