Refactor NAT to support TCP, fix typos#388
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #388 +/- ##
==========================================
- Coverage 84.72% 84.62% -0.11%
==========================================
Files 38 38
Lines 3122 3173 +51
==========================================
+ Hits 2645 2685 +40
- Misses 347 354 +7
- Partials 130 134 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
There is a lot of whitespace changes here. In diff options (gear icon next to "Submit review" button on the top) you can enable split layout and hide whitespaces to see actual changes. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the vnet NAT translation logic to share NAPT outbound behavior between protocols and extends NAT/NAPT translation to support TCP alongside UDP, while also cleaning up several error variable typos and expanding test coverage accordingly.
Changes:
- Renames/fixes NAT error variables and clarifies unsupported-protocol handling.
- Extracts shared outbound NAPT mapping logic and adds a separate TCP port counter.
- Adds TCP translation paths for outbound/inbound NAT/NAPT and updates NAT tests to run against both UDP and TCP.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| vnet/nat.go | Refactors outbound NAPT mapping into a shared closure, adds TCP translation support and a TCP port counter, and renames error variables. |
| vnet/nat_test.go | Refactors NAT tests to be protocol-parameterized (UDP/TCP) and adds TCP coverage across existing NAT behavior scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| errNoAssociatedLocalAddress = errors.New("no associated local address") | ||
| errNoNATBindingFound = errors.New("no NAT binding found") | ||
| errHasNoPermission = errors.New("has no permission") | ||
| errNATRequiresMapping = errors.New("1:1 NAT requires more than one mapping") |
JoTurk
left a comment
There was a problem hiding this comment.
i would do this in multiple commits, but since it's vnet ig that's not important.
|
thank you. |
Refactor and extend the NAT translation layer.
errNATRequriesMapping→errNATRequiresMapping,errMismatchLengthIP(spelling),errNonUDPTranslationNotSupported→errTranslationNotSupported.translateOutboundNAPTclosure (previously duplicated per-protocol).tcpPortCounterfield.case tcp:branches intranslateOutboundandtranslateInboundto support TCP NAT/NAPT and 1:1 NAT.nat_test.goto cover the new TCP NAT cases.Split out from #375