[client] Add IPv6 support to ACL manager, USP filter, and forwarder#5688
Open
lixmal wants to merge 9 commits intoclient-ipv6-ssh-netflowfrom
Open
[client] Add IPv6 support to ACL manager, USP filter, and forwarder#5688lixmal wants to merge 9 commits intoclient-ipv6-ssh-netflowfrom
lixmal wants to merge 9 commits intoclient-ipv6-ssh-netflowfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded dual‑stack IPv4/IPv6 support across firewall, NAT, forwarder, conntrack and tests: version-aware decoding, per-family MSS/clamping and checksums, ICMPv6 tracking/handling and reply synthesis, IPv6-aware DNAT/rewrites, lock-free local IP snapshot, and netip-based address APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Filter
participant Decoder
participant Parser4
participant Parser6
Client->>Filter: deliver raw packet
Filter->>Decoder: decodePacket(raw)
Decoder->>Decoder: inspect first nibble -> decide IP version
alt IPv4
Decoder->>Parser4: DecodeLayers(raw)
Parser4-->>Decoder: IPv4 + transport
else IPv6
Decoder->>Parser6: DecodeLayers(raw)
Parser6-->>Decoder: IPv6 + transport
end
Decoder-->>Filter: decoded layers + version
Filter->>Filter: family-specific logic (MSS, checksums, ICMP tracking, NAT)
Filter-->>Client: forward or drop
sequenceDiagram
participant ICMPHandler
participant ConnTracker
participant EventSink
participant Pinger
ICMPHandler->>ConnTracker: inbound echo reply (v4 or v6)
ConnTracker->>ConnTracker: validate by type & parse original packet (v4/v6)
ConnTracker->>EventSink: emit start/end events (Protocol = ICMP or ICMPv6)
alt Echo request handling
ICMPHandler->>Pinger: run ping (rate-limited)
Pinger-->>ICMPHandler: result
ICMPHandler->>ICMPHandler: synthesize reply & inject
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/internal/engine.go (1)
1486-1507:⚠️ Potential issue | 🟠 MajorDon’t hard-require
/32for the peer’s IPv4 overlay address.The new v4 branch now drops
/24-style overlayAllowedIpsthat the repo still uses for peers (for example100.64.0.21/24and100.64.0.10/24inclient/internal/engine_test.go). It also still can’t distinguish a routed IPv4/32from the peer overlay address because, unlike the v6 path, there is no overlay-prefix containment check. That means the status recorder and SSH metadata can lose or misidentify the peer’s IPv4 address.💡 Possible fix
-func overlayAddrsFromAllowedIPs(allowedIPs []string, ourV6Net netip.Prefix) (v4, v6 netip.Addr) { +func overlayAddrsFromAllowedIPs(allowedIPs []string, ourV4Net, ourV6Net netip.Prefix) (v4, v6 netip.Addr) { for _, cidr := range allowedIPs { prefix, err := netip.ParsePrefix(cidr) if err != nil { log.Warnf("failed to parse AllowedIP %q: %v", cidr, err) continue } addr := prefix.Addr().Unmap() switch { - case addr.Is4() && prefix.Bits() == 32 && !v4.IsValid(): + case addr.Is4() && ourV4Net.Contains(addr) && !v4.IsValid(): v4 = addr case addr.Is6() && prefix.Bits() == 128 && ourV6Net.Contains(addr) && !v6.IsValid(): v6 = addr }peerV4, peerV6 := overlayAddrsFromAllowedIPs( peerConfig.GetAllowedIps(), e.wgInterface.Address().Network, e.wgInterface.Address().IPv6Net, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine.go` around lines 1486 - 1507, The function overlayAddrsFromAllowedIPs currently only accepts IPv4 host routes (/32) which drops valid peer IPv4 entries like 100.64.0.21/24; change the signature to accept ourV4Net (netip.Prefix) in addition to ourV6Net and update the IPv4 branch to accept an address when either prefix.Bits() == 32 OR the parsed addr falls inside ourV4Net (e.g., change the condition to addr.Is4() && (prefix.Bits() == 32 || ourV4Net.Contains(addr)) && !v4.IsValid()); update all callers of overlayAddrsFromAllowedIPs to pass e.wgInterface.Address().Network (or equivalent) for the new ourV4Net parameter.client/internal/acl/manager.go (1)
108-135:⚠️ Potential issue | 🔴 CriticalDon't replace the active peer ACL set after a partial apply failure.
If any
AddPeerFilteringcall fails, this loop still installs the rest, logs once, and then removes every old rule missing fromnewRulePairs. A single failed DROP therefore leaves a partially applied, fail-open policy afterFlush(). Please keep the current peer ACL set intact unless the full replacement succeeds, or abort immediately on any deny-rule failure.Based on learnings: In the netbird client firewall iptables implementation (
client/firewall/iptables/), IPv6 is not currently handled or supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/acl/manager.go` around lines 108 - 135, The loop in manager.go currently accumulates errors into merr while modifying d.peerRulesPairs and newRulePairs, which can leave a partially applied (fail-open) ACL when any AddPeerFiltering/deny rule fails; change the flow so that protoRuleToFirewallRule (and any deny-rule handling) is treated as fatal: on error return immediately and do not mutate d.peerRulesPairs or replace the active set; build newRulePairs and ipsetByRuleSelectors/ipsetCounter in a separate staging area (use local maps/vars), validate the entire set succeeds (no errors) and only then swap into d.peerRulesPairs or call Flush(); ensure any deny-rule failure aborts the operation immediately to keep the existing peer ACL intact (refer to getRuleGroupingSelector, protoRuleToFirewallRule, newRulePairs, d.peerRulesPairs, ipsetByRuleSelectors, d.ipsetCounter, and AddPeerFiltering).client/firewall/uspfilter/forwarder/forwarder.go (1)
213-220:⚠️ Potential issue | 🔴 CriticalFix IPv6 host:port formatting in TCP and UDP dialers.
The TCP and UDP forwarders at
client/firewall/uspfilter/forwarder/tcp.go:35andudp.go:160usefmt.Sprintf("%s:%d", ...)to format the result ofdetermineDialAddr, which now returns::1for IPv6 loopback. This produces invalid addresses like::1:portinstead of[::1]:port, breaking IPv6 netstack forwarding to local services.Replace both call sites with
net.JoinHostPortor usenetip.AddrPortfor proper IPv6 bracket handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/forwarder.go` around lines 213 - 220, The formatting of IPv6 addresses is broken because determineDialAddr returns IPv6 loopback (::1) which when combined with fmt.Sprintf("%s:%d", ...) yields invalid addresses like "::1:port"; update the TCP and UDP dialer call sites that currently use fmt.Sprintf to build host:port (the usages in the TCP forwarder and UDP forwarder that call determineDialAddr) to use net.JoinHostPort(host, strconv.Itoa(port)) or construct a netip.AddrPort so IPv6 addresses get bracketed correctly; ensure you import net and strconv or use netip.Addr/AddrPort accordingly and replace the fmt.Sprintf calls with the chosen safe joiner.
🧹 Nitpick comments (1)
client/firewall/uspfilter/filter_test.go (1)
1109-1153: Add one real IPv6 MSS clamping case.These assertions prove the configured IPv6 clamp value, but every packet in this test still goes through the IPv4 helpers. That leaves the new IPv6 clamping path and its pseudo-header checksum update unexercised. Please add at least one IPv6 SYN/SYN-ACK case and assert it clamps to
manager.mssClampValueIPv6.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/filter_test.go` around lines 1109 - 1153, Test coverage misses the IPv6 code path for MSS clamping; add at least one test case that creates an IPv6 SYN or SYN-ACK using the existing helpers (e.g. call generateSYNPacketWithMSS or generateSYNACKPacketWithMSS with an IPv6 src/dst like "fd00::2" and "2001:4860:4860::8888"), pass the packet through manager.filterOutbound, parse it with parsePacket, and assert the MSS option value equals manager.mssClampValueIPv6 to exercise the IPv6 clamp logic and pseudo-header checksum update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/conntrack/icmp.go`:
- Around line 24-27: The change raised MaxICMPPayloadLength to 48 but the
parsing gate in ICMPInfo.String() and parseOriginalPacket() still uses that
constant, causing IPv4 quotes (28 bytes) to be skipped; introduce separate
family-specific parse thresholds (e.g., MinICMPParseLengthIPv4 = 28 and
MinICMPParseLengthIPv6 = 48) while keeping MaxICMPPayloadLength = 48 as the
buffer cap, then update parseOriginalPacket() to check the packet family and use
the corresponding MinICMPParseLength* before attempting to parse and have
ICMPInfo.String() rely on whether parsing succeeded (or the family-aware
threshold) rather than MaxICMPPayloadLength.
In `@client/firewall/uspfilter/filter.go`:
- Around line 1266-1277: The IPv6 fragment check in the l==1 branch currently
returns (true, true) for fragments (using d.ip6.NextHeader ==
layers.IPProtocolIPv6Fragment), which routes them into filterInbound's fragment
fast path and bypasses ACL/routing; change this so IPv6 fragments do NOT take
the fragment fast-path — remove the (true, true) return and instead return
values that force normal processing (so fragments go through peerACLsBlock,
routeACLsPass and forwarder injection) when d.ip6.NextHeader ==
layers.IPProtocolIPv6Fragment; update the branch around d.decoded,
d.ip6.NextHeader, and the l==1 handling to ensure fragments are handled by the
regular path instead of being auto-allowed.
- Around line 1330-1339: The isSpecialICMP helper currently exempts ICMPv4
Destination Unreachable and Time Exceeded and ICMPv6 Destination Unreachable and
Time Exceeded; update the ICMPv6 branch in Manager.isSpecialICMP (using the
decoder d and fields d.icmp6.TypeCode.Type()) to also return true for ICMPv6
Packet Too Big (ICMPv6 type 2) by adding layers.ICMPv6TypePacketTooBig to the
boolean condition so PMTUD-related ICMPv6 messages are allowed through.
- Around line 301-309: The invalid-routed drop rule is only created for
iface.Address().Network (IPv4); when iface.Address().IPv6Net is valid you must
also create a second drop rule for the IPv6 prefix. Update the code that calls
m.addRouteFiltering (and any place that builds the rule for
iface.Address().Network) to create a separate rule for iface.Address().IPv6Net
when v6.IsValid(), and ensure EnableRouting and DisableRouting are updated to
install and remove both rules (track both rule identifiers or entries so cleanup
removes the IPv6 block as well as the IPv4 one).
In `@client/firewall/uspfilter/forwarder/endpoint.go`:
- Around line 55-61: The code reads raw := pkt.NetworkHeader().View().AsSlice()
then unconditionally parses it as IPv4 in the else branch, which can produce an
invalid address for empty headers; add an explicit guard for len(raw) == 0
immediately after raw is obtained (in the same function that contains pkt/ raw/
address) and either return early with an error/zero value or set address to the
zero TCP/IP address before continuing, otherwise proceed with the existing
IPv6/IPv4 parsing using header.IPv6(raw).DestinationAddress() and
header.IPv4(raw).DestinationAddress() as currently written.
In `@client/firewall/uspfilter/forwarder/icmp.go`:
- Around line 268-287: The ICMPv6 checksum is being computed incorrectly by
passing the pseudo-header checksum (from header.PseudoHeaderChecksum) into
ICMPv6Checksum as PayloadCsum in synthesizeICMPv6EchoReply; change this so
PayloadCsum is 0 (or compute the checksum of only the payload bytes after the
ICMP header) and let header.ICMPv6Checksum compute the pseudo-header internally
— update the call that builds header.ICMPv6Checksum (the ICMPv6ChecksumParams
passed to replyHdr.SetChecksum) to use PayloadCsum: 0 (or the checksum of
replyICMP[header.ICMPv6MinimumSize:]) and ensure PayloadLen is the payload
length only.
In `@client/firewall/uspfilter/nat.go`:
- Around line 294-305: The extractPacketIPs function currently indexes
packetData at fixed offsets (ipv4SrcOffset, ipv4DstOffset, ipv6SrcOffset,
ipv6DstOffset) without verifying packetData length; add explicit bounds checks
using len(packetData) before creating the 4- or 16-byte arrays or slices, and
return zero-value netip.Addr (or a safe fallback) when the packet is too short
instead of indexing out of range; update the IPv4 branch (where netip.AddrFrom4
is called) to ensure the 4 bytes exist and the IPv6 branch (netip.AddrFrom16) to
ensure the 16-byte slice exists before conversion, keeping the function
signature and return behavior consistent.
In `@client/firewall/uspfilter/tracer.go`:
- Around line 135-143: In buildIPLayer (method PacketBuilder.buildIPLayer)
reject mixed-family endpoints by validating SrcIP and DstIP belong to the same
IP family before selecting IPv4 vs IPv6: check both endpoints with their
Is6()/Is4() helpers (or equivalent) and if one is IPv6 and the other IPv4 return
a clear error rather than constructing an IPv6 header; otherwise proceed with
the existing IPv6 branch (both Is6()) or the IPv4 branch (both Is4()), ensuring
you use the same SrcIP.AsSlice()/DstIP.AsSlice() logic when families match.
In `@client/internal/ebpf/ebpf/dns_fwd_linux.go`:
- Around line 48-54: The ip2int function can panic because net.ParseIP may
return nil; change ip2int to validate the ParseIP result and propagate errors
instead of silently returning 0: update ip2int signature to return (uint32,
error), check if ip == nil before calling To4, return a clear error for
invalid/malformed input, and adjust callers (e.g., LoadDNSFwd which calls
s.wgInterface.Address().IP.String()) to handle the error and fail fast rather
than treating 0 as a valid IP; keep using binary.BigEndian.Uint32(ip4) when ip4
!= nil.
In `@client/internal/engine.go`:
- Around line 1472-1475: The code unconditionally calls String() on netip.Addr
values returned by overlayAddrsFromAllowedIPs, producing "invalid IP" for
missing families; add a helper function (e.g., addrStringOrEmpty(addr
netip.Addr) string) that returns "" when addr.IsValid() is false and
addr.String() otherwise, then replace direct .String() calls in the assignment
to replacement[i] = peer.State{ IP: ..., IPv6: ... } to use
addrStringOrEmpty(v4) and addrStringOrEmpty(v6) (and any other call sites of
overlayAddrsFromAllowedIPs) so single‑stack peers serialize empty strings
instead of "invalid IP".
---
Outside diff comments:
In `@client/firewall/uspfilter/forwarder/forwarder.go`:
- Around line 213-220: The formatting of IPv6 addresses is broken because
determineDialAddr returns IPv6 loopback (::1) which when combined with
fmt.Sprintf("%s:%d", ...) yields invalid addresses like "::1:port"; update the
TCP and UDP dialer call sites that currently use fmt.Sprintf to build host:port
(the usages in the TCP forwarder and UDP forwarder that call determineDialAddr)
to use net.JoinHostPort(host, strconv.Itoa(port)) or construct a netip.AddrPort
so IPv6 addresses get bracketed correctly; ensure you import net and strconv or
use netip.Addr/AddrPort accordingly and replace the fmt.Sprintf calls with the
chosen safe joiner.
In `@client/internal/acl/manager.go`:
- Around line 108-135: The loop in manager.go currently accumulates errors into
merr while modifying d.peerRulesPairs and newRulePairs, which can leave a
partially applied (fail-open) ACL when any AddPeerFiltering/deny rule fails;
change the flow so that protoRuleToFirewallRule (and any deny-rule handling) is
treated as fatal: on error return immediately and do not mutate d.peerRulesPairs
or replace the active set; build newRulePairs and
ipsetByRuleSelectors/ipsetCounter in a separate staging area (use local
maps/vars), validate the entire set succeeds (no errors) and only then swap into
d.peerRulesPairs or call Flush(); ensure any deny-rule failure aborts the
operation immediately to keep the existing peer ACL intact (refer to
getRuleGroupingSelector, protoRuleToFirewallRule, newRulePairs,
d.peerRulesPairs, ipsetByRuleSelectors, d.ipsetCounter, and AddPeerFiltering).
In `@client/internal/engine.go`:
- Around line 1486-1507: The function overlayAddrsFromAllowedIPs currently only
accepts IPv4 host routes (/32) which drops valid peer IPv4 entries like
100.64.0.21/24; change the signature to accept ourV4Net (netip.Prefix) in
addition to ourV6Net and update the IPv4 branch to accept an address when either
prefix.Bits() == 32 OR the parsed addr falls inside ourV4Net (e.g., change the
condition to addr.Is4() && (prefix.Bits() == 32 || ourV4Net.Contains(addr)) &&
!v4.IsValid()); update all callers of overlayAddrsFromAllowedIPs to pass
e.wgInterface.Address().Network (or equivalent) for the new ourV4Net parameter.
---
Nitpick comments:
In `@client/firewall/uspfilter/filter_test.go`:
- Around line 1109-1153: Test coverage misses the IPv6 code path for MSS
clamping; add at least one test case that creates an IPv6 SYN or SYN-ACK using
the existing helpers (e.g. call generateSYNPacketWithMSS or
generateSYNACKPacketWithMSS with an IPv6 src/dst like "fd00::2" and
"2001:4860:4860::8888"), pass the packet through manager.filterOutbound, parse
it with parsePacket, and assert the MSS option value equals
manager.mssClampValueIPv6 to exercise the IPv6 clamp logic and pseudo-header
checksum update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7845f6eb-e829-4335-832b-5dc39db70aec
📒 Files selected for processing (21)
client/firewall/uspfilter/conntrack/icmp.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/filter_bench_test.goclient/firewall/uspfilter/filter_filter_test.goclient/firewall/uspfilter/filter_test.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/firewall/uspfilter/forwarder/tcp.goclient/firewall/uspfilter/forwarder/udp.goclient/firewall/uspfilter/localip.goclient/firewall/uspfilter/localip_test.goclient/firewall/uspfilter/nat.goclient/firewall/uspfilter/nat_bench_test.goclient/firewall/uspfilter/nat_test.goclient/firewall/uspfilter/tracer.goclient/internal/acl/manager.goclient/internal/ebpf/ebpf/dns_fwd_linux.goclient/internal/engine.goclient/internal/engine_ssh.goclient/internal/engine_test.go
lixmal
force-pushed
the
client-ipv6-ssh-netflow
branch
from
e5e177e to
1a7e835
Compare
lixmal
force-pushed
the
client-ipv6-acl-usp
branch
from
b924706 to
780fd66
Compare
7 tasks
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/firewall/uspfilter/tracer.go (1)
204-219:⚠️ Potential issue | 🟡 MinorICMPv6 layer missing network layer for checksum calculation.
ICMPv6 checksums include a pseudo-header with source/destination addresses (per RFC 4443). The
layers.ICMPv6layer needsSetNetworkLayerForChecksumto compute a valid checksum. Without it, serialized trace packets may have incorrect checksums.Suggested fix
-func (p *PacketBuilder) buildICMPLayer() ([]gopacket.SerializableLayer, error) { +func (p *PacketBuilder) buildICMPLayer(ipLayer gopacket.SerializableLayer) ([]gopacket.SerializableLayer, error) { if p.SrcIP.Is6() || p.DstIP.Is6() { icmp := &layers.ICMPv6{ TypeCode: layers.CreateICMPv6TypeCode(p.ICMPType, p.ICMPCode), } + if nl, ok := ipLayer.(gopacket.NetworkLayer); ok { + if err := icmp.SetNetworkLayerForChecksum(nl); err != nil { + return nil, fmt.Errorf("set network layer for ICMPv6 checksum: %w", err) + } + } return []gopacket.SerializableLayer{icmp}, nil }Note: You'll also need to update the call site in
buildTransportLayerto passipLayertobuildICMPLayer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/tracer.go` around lines 204 - 219, The ICMPv6 layer created in PacketBuilder.buildICMPLayer lacks a network-layer reference for checksum computation; modify buildICMPLayer to accept the IP layer (passed from buildTransportLayer) and call SetNetworkLayerForChecksum(ipLayer) on the layers.ICMPv6 instance before returning it so the ICMPv6 checksum is calculated against the pseudo-header; keep existing ICMPv4 behavior (Id/Seq) unchanged and ensure buildTransportLayer is updated to supply the ipLayer when invoking buildICMPLayer.
♻️ Duplicate comments (1)
client/firewall/uspfilter/nat.go (1)
294-305:⚠️ Potential issue | 🟡 MinorAdd bounds checking before accessing packet bytes.
This was flagged in a previous review.
extractPacketIPsreads at fixed offsets without verifying packet length. While the decoder likely validates this, a malformed packet could cause a panic.Proposed fix
func extractPacketIPs(packetData []byte, d *decoder) (src, dst netip.Addr) { switch d.decoded[0] { case layers.LayerTypeIPv4: + if len(packetData) < ipv4DstOffset+4 { + return netip.Addr{}, netip.Addr{} + } src = netip.AddrFrom4([4]byte{packetData[ipv4SrcOffset], packetData[ipv4SrcOffset+1], packetData[ipv4SrcOffset+2], packetData[ipv4SrcOffset+3]}) dst = netip.AddrFrom4([4]byte{packetData[ipv4DstOffset], packetData[ipv4DstOffset+1], packetData[ipv4DstOffset+2], packetData[ipv4DstOffset+3]}) case layers.LayerTypeIPv6: + if len(packetData) < ipv6DstOffset+16 { + return netip.Addr{}, netip.Addr{} + } src = netip.AddrFrom16([16]byte(packetData[ipv6SrcOffset : ipv6SrcOffset+16])) dst = netip.AddrFrom16([16]byte(packetData[ipv6DstOffset : ipv6DstOffset+16])) } return src, dst }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/nat.go` around lines 294 - 305, extractPacketIPs currently indexes packetData at fixed offsets (ipv4SrcOffset, ipv4DstOffset, ipv6SrcOffset, ipv6DstOffset) without validating packet length, which can panic on malformed packets; add explicit bounds checks at the top of extractPacketIPs: when d.decoded[0] == layers.LayerTypeIPv4 ensure len(packetData) >= ipv4DstOffset+4 (or ipv4SrcOffset+4) before reading 4 bytes, and when LayerTypeIPv6 ensure len(packetData) >= ipv6DstOffset+16 before slicing 16 bytes; if the check fails, return zero/unspecified netip.Addr values (or a sentinel) and avoid indexing into packetData so no panic occurs.
🧹 Nitpick comments (3)
client/firewall/uspfilter/filter_filter_test.go (1)
700-783: LGTM with minor cleanup opportunity.The
createTestPacket()refactor correctly handles both IPv4 and IPv6 packet generation. The address family detection viasrc.To4() == nilis idiomatic, and the ICMPv6 handling properly sets up the checksum layer.The assignments
_ = setChecksum(line 750) and_ = networkLayer(line 779) are unused and could be removed in a future cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/filter_filter_test.go` around lines 700 - 783, The function createTestPacket declares unused variables networkLayer and setChecksum and then assigns them to blank identifier (_ = setChecksum, _ = networkLayer); remove these dead declarations and the two dummy assignments: delete the setChecksum variable and its assignment/usage and remove the networkLayer variable (or its assignment) in createTestPacket so the code compiles without unused-symbol noise while keeping the existing gopacket.SerializeLayers logic for IPv4/IPv6 and protocol handling.client/firewall/uspfilter/forwarder/forwarder.go (1)
265-271: Consider handling zero-length addresses inaddrToNetipAddr.The function assumes addresses are either 4 or 16 bytes. If a zero-length
tcpip.Addressis passed,Len()returns 0, causing the function to fall through to the IPv6 branch and callAs16()on an invalid address.Suggested guard
func addrToNetipAddr(addr tcpip.Address) netip.Addr { + if addr.Len() == 0 { + return netip.Addr{} + } if addr.Len() == 4 { return netip.AddrFrom4(addr.As4()) } return netip.AddrFrom16(addr.As16()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/forwarder.go` around lines 265 - 271, The addrToNetipAddr function assumes tcpip.Address lengths of 4 or 16 and can call As16() on an invalid zero-length address; update addrToNetipAddr to explicitly guard for addr.Len() == 0 (and any non-4/non-16 lengths) and return the zero-value netip.Addr in those cases, otherwise call As4()/As16() as currently done; reference the addrToNetipAddr function and methods Len(), As4(), As16(), and constructors netip.AddrFrom4/AddrFrom16 when making this change.client/firewall/uspfilter/localip_test.go (1)
75-104: Use documentation IPv6 addresses here to keep the test host-independent.
UpdateLocalIPs()also pulls in real interface addresses, so thesefd00::/64examples can collide with an existing ULA on the runner and make the assertions pass or fail for the wrong reason. Using2001:db8::/32for the synthetic IPv6 cases would keep this new coverage deterministic.Suggested test-data tweak
{ name: "IPv6 address matches", setupAddr: wgaddr.Address{ IP: netip.MustParseAddr("100.64.0.1"), Network: netip.MustParsePrefix("100.64.0.0/16"), - IPv6: netip.MustParseAddr("fd00::1"), - IPv6Net: netip.MustParsePrefix("fd00::/64"), + IPv6: netip.MustParseAddr("2001:db8::1"), + IPv6Net: netip.MustParsePrefix("2001:db8::/64"), }, - testIP: netip.MustParseAddr("fd00::1"), + testIP: netip.MustParseAddr("2001:db8::1"), expected: true, }, { name: "IPv6 address does not match", setupAddr: wgaddr.Address{ IP: netip.MustParseAddr("100.64.0.1"), Network: netip.MustParsePrefix("100.64.0.0/16"), - IPv6: netip.MustParseAddr("fd00::1"), - IPv6Net: netip.MustParsePrefix("fd00::/64"), + IPv6: netip.MustParseAddr("2001:db8::1"), + IPv6Net: netip.MustParsePrefix("2001:db8::/64"), }, - testIP: netip.MustParseAddr("fd00::99"), + testIP: netip.MustParseAddr("2001:db8::99"), expected: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/localip_test.go` around lines 75 - 104, Replace the synthetic ULA IPv6 addresses in the test cases so they use documentation-only addresses to avoid colliding with real interface addresses: in the table entries that set wgaddr.Address.IPv6 and IPv6Net and the corresponding testIP values (the cases named "IPv6 address matches" and "IPv6 address does not match"), change fd00::1 to 2001:db8::1, fd00::/64 to 2001:db8::/32, and update the testIP fd00::99 to 2001:db8::99; leave the "IPv6 loopback" case (testIP ::1) unchanged; this targets the setup in wgaddr.Address and the test logic around UpdateLocalIPs() to make the tests deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/filter_bench_test.go`:
- Around line 1091-1094: The assignment to manager.mssClampValueIPv6 is
misindented and visually appears outside the if block; fix by aligning its
indentation with manager.mssClampValueIPv4 inside the if sc.enabled block so
both assignments are clearly nested under the condition (adjust indentation for
the line containing manager.mssClampValueIPv6 = 1220 to match the preceding
line).
In `@client/firewall/uspfilter/localip.go`:
- Around line 22-24: The ipv4LowBitmap currently packs IPv4 octets into an
overlapping 24-bit key causing aliasing (e.g. 192.168.1.1 vs 192.168.0.17);
change ipv4LowBitmap to use an exact representation instead of the packed low
value: replace the bitmap array with a precise keying (recommended:
map[netip.Addr]struct{} in the snapshot or a true 16/16 two-level structure) and
update all related methods that compute/lookup the packed value (the
ipv4LowBitmap type methods and any Add/Has/Remove helpers that build the low
key) so they use the netip.Addr (or non-overlapping 16/16 indices). Update
IsLocalIP call sites to use the new lookup API, and add a regression test that
inserts 192.168.1.1 and 192.168.0.17 and asserts they are distinct (one present,
the other absent as appropriate) to prevent future collisions.
---
Outside diff comments:
In `@client/firewall/uspfilter/tracer.go`:
- Around line 204-219: The ICMPv6 layer created in PacketBuilder.buildICMPLayer
lacks a network-layer reference for checksum computation; modify buildICMPLayer
to accept the IP layer (passed from buildTransportLayer) and call
SetNetworkLayerForChecksum(ipLayer) on the layers.ICMPv6 instance before
returning it so the ICMPv6 checksum is calculated against the pseudo-header;
keep existing ICMPv4 behavior (Id/Seq) unchanged and ensure buildTransportLayer
is updated to supply the ipLayer when invoking buildICMPLayer.
---
Duplicate comments:
In `@client/firewall/uspfilter/nat.go`:
- Around line 294-305: extractPacketIPs currently indexes packetData at fixed
offsets (ipv4SrcOffset, ipv4DstOffset, ipv6SrcOffset, ipv6DstOffset) without
validating packet length, which can panic on malformed packets; add explicit
bounds checks at the top of extractPacketIPs: when d.decoded[0] ==
layers.LayerTypeIPv4 ensure len(packetData) >= ipv4DstOffset+4 (or
ipv4SrcOffset+4) before reading 4 bytes, and when LayerTypeIPv6 ensure
len(packetData) >= ipv6DstOffset+16 before slicing 16 bytes; if the check fails,
return zero/unspecified netip.Addr values (or a sentinel) and avoid indexing
into packetData so no panic occurs.
---
Nitpick comments:
In `@client/firewall/uspfilter/filter_filter_test.go`:
- Around line 700-783: The function createTestPacket declares unused variables
networkLayer and setChecksum and then assigns them to blank identifier (_ =
setChecksum, _ = networkLayer); remove these dead declarations and the two dummy
assignments: delete the setChecksum variable and its assignment/usage and remove
the networkLayer variable (or its assignment) in createTestPacket so the code
compiles without unused-symbol noise while keeping the existing
gopacket.SerializeLayers logic for IPv4/IPv6 and protocol handling.
In `@client/firewall/uspfilter/forwarder/forwarder.go`:
- Around line 265-271: The addrToNetipAddr function assumes tcpip.Address
lengths of 4 or 16 and can call As16() on an invalid zero-length address; update
addrToNetipAddr to explicitly guard for addr.Len() == 0 (and any non-4/non-16
lengths) and return the zero-value netip.Addr in those cases, otherwise call
As4()/As16() as currently done; reference the addrToNetipAddr function and
methods Len(), As4(), As16(), and constructors netip.AddrFrom4/AddrFrom16 when
making this change.
In `@client/firewall/uspfilter/localip_test.go`:
- Around line 75-104: Replace the synthetic ULA IPv6 addresses in the test cases
so they use documentation-only addresses to avoid colliding with real interface
addresses: in the table entries that set wgaddr.Address.IPv6 and IPv6Net and the
corresponding testIP values (the cases named "IPv6 address matches" and "IPv6
address does not match"), change fd00::1 to 2001:db8::1, fd00::/64 to
2001:db8::/32, and update the testIP fd00::99 to 2001:db8::99; leave the "IPv6
loopback" case (testIP ::1) unchanged; this targets the setup in wgaddr.Address
and the test logic around UpdateLocalIPs() to make the tests deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbee32bc-3f02-4a38-8cb9-e417b44480da
📒 Files selected for processing (23)
client/firewall/uspfilter/conntrack/icmp.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/filter_bench_test.goclient/firewall/uspfilter/filter_filter_test.goclient/firewall/uspfilter/filter_test.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/firewall/uspfilter/forwarder/tcp.goclient/firewall/uspfilter/forwarder/udp.goclient/firewall/uspfilter/localip.goclient/firewall/uspfilter/localip_test.goclient/firewall/uspfilter/nat.goclient/firewall/uspfilter/nat_bench_test.goclient/firewall/uspfilter/nat_test.goclient/firewall/uspfilter/tracer.goclient/internal/acl/manager.goclient/internal/dns/service_listener.goclient/internal/ebpf/ebpf/dns_fwd_linux.goclient/internal/ebpf/manager/manager.goclient/internal/engine.goclient/internal/engine_ssh.goclient/internal/engine_test.go
✅ Files skipped from review due to trivial changes (2)
- client/internal/engine_test.go
- client/firewall/uspfilter/forwarder/icmp.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/firewall/uspfilter/forwarder/tcp.go
- client/firewall/uspfilter/forwarder/endpoint.go
- client/internal/ebpf/ebpf/dns_fwd_linux.go
lixmal
commented
Mar 25, 2026
Collaborator
Author
lixmal
left a comment
lixmal
left a comment
There was a problem hiding this comment.
The ICMPv6 checksum is already correct in the current code. CodeRabbit confirmed our explanation.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/firewall/uspfilter/localip.go (1)
34-60: Normalize all stored and queried addresses the same way.
processInterfacecanonicalizes throughUnmap(), but theiface.Address()path andIsLocalIPlookups keep rawnetip.Addrvalues. Normalizing both sides and skipping invalid zero-values makes membership checks source-independent.Suggested refactor
if iface != nil { - ip := iface.Address().IP - ips[ip] = struct{}{} - addresses = append(addresses, ip) - if v6 := iface.Address().IPv6; v6.IsValid() { + addr := iface.Address() + if ip := addr.IP.Unmap(); ip.IsValid() { + ips[ip] = struct{}{} + addresses = append(addresses, ip) + } + if v6 := addr.IPv6.Unmap(); v6.IsValid() { ips[v6] = struct{}{} addresses = append(addresses, v6) } } @@ func (m *localIPManager) IsLocalIP(ip netip.Addr) bool { + if !ip.IsValid() { + return false + } + ip = ip.Unmap() s := m.snapshot.Load()Also applies to: 79-86, 107-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/localip.go` around lines 34 - 60, The code stores Unmap()-normalized addresses in processInterface but compares against raw netip.Addr elsewhere; update all address writes and lookups to use parsed.Unmap() and skip invalid/zero netip.Addr values so membership checks are consistent: normalize addresses inside processInterface (already done) and likewise normalize return values from iface.Address() and any callers like IsLocalIP (or the function performing membership checks) by calling Unmap() after parsing with AddrFromSlice/AddrFromString and ignoring Addr.IsZero() or AddrFromSlice/ok==false; ensure the map keys and the addresses slice are always the Unmap()ed form to make lookups source-independent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/localip.go`:
- Around line 26-31: newLocalIPManager() currently creates an empty
localIPSnapshot so IsLocalIP may misclassify loopback addresses before the first
UpdateLocalIPs(); update newLocalIPManager to initialize its snapshot with
loopback semantics (include IPv4 127/8 and IPv6 ::1 as local) so the initial
snapshot used by IsLocalIP matches snapshots produced after UpdateLocalIPs();
modify the snapshot creation in newLocalIPManager (and if needed the
localIPSnapshot representation used by IsLocalIP and UpdateLocalIPs) to include
those loopback entries/prefixes.
---
Nitpick comments:
In `@client/firewall/uspfilter/localip.go`:
- Around line 34-60: The code stores Unmap()-normalized addresses in
processInterface but compares against raw netip.Addr elsewhere; update all
address writes and lookups to use parsed.Unmap() and skip invalid/zero
netip.Addr values so membership checks are consistent: normalize addresses
inside processInterface (already done) and likewise normalize return values from
iface.Address() and any callers like IsLocalIP (or the function performing
membership checks) by calling Unmap() after parsing with
AddrFromSlice/AddrFromString and ignoring Addr.IsZero() or
AddrFromSlice/ok==false; ensure the map keys and the addresses slice are always
the Unmap()ed form to make lookups source-independent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f968cc2-7469-4330-b1ba-00f135df04d8
📒 Files selected for processing (3)
client/firewall/uspfilter/filter_bench_test.goclient/firewall/uspfilter/localip.goclient/firewall/uspfilter/localip_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/firewall/uspfilter/filter_bench_test.go
lixmal
force-pushed
the
client-ipv6-acl-usp
branch
from
72f9a86 to
aebf3ce
Compare
This was referenced Mar 26, 2026
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
client/firewall/uspfilter/forwarder/icmp.go (1)
296-304:⚠️ Potential issue | 🔴 CriticalStill double-counting the ICMPv6 pseudo-header.
replyHdralready contains the full ICMPv6 message. Passingheader.PseudoHeaderChecksum(...)asPayloadCsummakesheader.ICMPv6Checksum(...)count the pseudo-header twice, so synthesized echo replies go out with an invalid checksum.🐛 Proposed fix
- psum := header.PseudoHeaderChecksum(header.ICMPv6ProtocolNumber, id.LocalAddress, id.RemoteAddress, uint16(len(replyICMP))) replyHdr.SetChecksum(header.ICMPv6Checksum(header.ICMPv6ChecksumParams{ - Header: replyHdr, - Src: id.LocalAddress, - Dst: id.RemoteAddress, - PayloadCsum: psum, - PayloadLen: len(replyICMP) - header.ICMPv6MinimumSize, + Header: replyHdr, + Src: id.LocalAddress, + Dst: id.RemoteAddress, }))Verify against upstream gVisor; the expected result is that
ICMPv6Checksumderives the pseudo-header fromSrc/Dst, whilePayloadCsumis only for payload bytes not already present inHeader.#!/bin/bash python - <<'PY' import urllib.request urls = [ "https://raw.githubusercontent.com/google/gvisor/master/pkg/tcpip/header/icmpv6.go", "https://raw.githubusercontent.com/google/gvisor/main/pkg/tcpip/header/icmpv6.go", ] src = None for url in urls: try: with urllib.request.urlopen(url, timeout=10) as resp: src = resp.read().decode("utf-8", "ignore") print(f"Fetched {url}") break except Exception as exc: print(f"Failed {url}: {exc}") if src is None: raise SystemExit("Could not fetch gVisor icmpv6.go") for needle in ("type ICMPv6ChecksumParams struct", "func ICMPv6Checksum("): idx = src.find(needle) if idx == -1: raise SystemExit(f"{needle} not found") start = max(0, src.rfind("\n", 0, idx - 300)) end = src.find("\n}\n", idx) print(src[start:end + 3]) print("----") PYBased on learnings,
header.ICMPv6Checksumalready computes the pseudo-header whenHeadercontains the full ICMPv6 message;PayloadCsum/PayloadLenare only for scattered buffers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/icmp.go` around lines 296 - 304, The code is double-counting the ICMPv6 pseudo-header by passing header.PseudoHeaderChecksum(...) as PayloadCsum into header.ICMPv6Checksum; remove the pseudo-header usage and let ICMPv6Checksum derive the pseudo-header from Src/Dst. Concretely, delete the psum calculation and call replyHdr.SetChecksum(header.ICMPv6Checksum(header.ICMPv6ChecksumParams{Header: replyHdr, Src: id.LocalAddress, Dst: id.RemoteAddress, PayloadCsum: 0, PayloadLen: 0})), or omit PayloadCsum/PayloadLen entirely, since replyHdr already contains the full ICMPv6 message (symbols: replyHdr, header.PseudoHeaderChecksum, header.ICMPv6Checksum, header.ICMPv6ChecksumParams, replyICMP, header.ICMPv6MinimumSize, id.LocalAddress, id.RemoteAddress).
🧹 Nitpick comments (1)
client/firewall/uspfilter/forwarder/icmp.go (1)
338-370: Consider table-driving the ping binary/timeout selection.This switch now carries multiple per-OS quirks for both the binary name and timeout flag. A tiny helper plus table tests would make the next platform-specific adjustment much safer.
Based on learnings, ping timeout handling is OS-specific and should be encapsulated behind a helper with tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/icmp.go` around lines 338 - 370, The buildPingCommand function contains OS-specific logic for choosing ping binary and timeout flag; refactor by extracting a helper (e.g., determinePingArgs or pingPlatformSpec) that returns the binary name and []string flags given runtime.GOOS, isV6, and timeoutSec, then have buildPingCommand call exec.CommandContext(ctx, bin, args...) using pingBin/ping6Bin only inside that helper; add table-driven unit tests for determinePingArgs covering linux, darwin/ios, freebsd, openbsd/netbsd, windows and default cases to verify binary selection and timeout flag formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/forwarder/forwarder.go`:
- Around line 224-268: The code currently takes ownership of payload and
intercepts every IPv4/IPv6 ICMP packet without reapplying fragment and
transport-length gating; move and add checks before creating the
PacketBuffer/handling so fragments and short payloads are rejected (return
false). Specifically: for IPv4 (header.IPv4(payload)) ensure this is not a
fragment (fragment offset == 0 and "More Fragments" flag clear) and for both
IPv4/IPv6 verify the remaining transport bytes (len(payload) - ipHdrLen) are at
least the minimal ICMP header length required for the ICMP type you will handle
(use your ICMP parsing helpers or explicit size checks) before calling
stack.NewPacketBuffer, pkt.TransportHeader().Consume, f.handleICMP or
f.handleICMPv6; keep the existing branch on payload[0]>>4 and only take
ownership after these validations.
In `@client/firewall/uspfilter/forwarder/icmp.go`:
- Around line 231-241: The ICMPv6 path incorrectly reuses the IPv4-only flag
hasRawICMPAccess, causing failed raw-forward attempts and dropped packets;
change the logic to use a family-specific capability (e.g., add/consult
hasRawICMPv6Access or a method that checks v6 raw-socket capability) instead of
hasRawICMPAccess when handling ICMPv6, and if v6 raw access is not available
call forwardICMPPacket with the non-raw path (or return false so netstack
handles it) rather than attempting a raw forward and returning true; update any
probe/initialization that sets hasRawICMPAccess to also set the new v6 flag.
In `@client/firewall/uspfilter/localip_bench_test.go`:
- Around line 10-24: setupManager currently ignores the error returned by
m.UpdateLocalIPs(mock) allowing a failed setup to produce a misleading "passing"
benchmark; change setupManager (which returns *localIPManager) to check the
error from UpdateLocalIPs and call b.Fatalf (or b.Fatal) with a descriptive
message including the error so the benchmark fails on setup errors — update the
call site in setupManager that uses IFaceMock and m.UpdateLocalIPs to handle the
returned error.
In `@client/firewall/uspfilter/localip.go`:
- Around line 20-21: The atomic pointer protects lock-free reads but not
concurrent writes: add a writer lock to localIPManager (e.g., a sync.Mutex or
sync.RWMutex field like rebuildMu) and take that lock around any code that
constructs and stores a new snapshot (the code path used by
Manager.UpdateLocalIPs that writes to localIPManager.snapshot). Keep reads using
the atomic.Pointer load lock-free, but wrap the snapshot rebuild/Store calls
(the routine that creates a localIPSnapshot and does snapshot.Store(...)) with
rebuildMu.Lock()/Unlock() so two overlapping refreshes cannot race and an older
result cannot overwrite a newer one; apply the same change to the other rebuild
code paths noted (lines ~63-97).
In `@client/internal/engine.go`:
- Around line 1506-1524: The function overlayAddrsFromAllowedIPs currently
accepts any /32 as the IPv4 overlay address; change its signature to accept a
local IPv4 overlay prefix (e.g., add ourV4Net netip.Prefix parameter) and mirror
the IPv6 check: only set v4 when addr.Is4() && prefix.Bits() == 32 &&
ourV4Net.Contains(addr) && !v4.IsValid(). Update all call sites to pass the
local v4 overlay prefix and add a unit test for overlayAddrsFromAllowedIPs that
feeds an exported routed /32 before the true overlay /32 and asserts the routed
/32 is ignored and the correct overlay address is returned.
---
Duplicate comments:
In `@client/firewall/uspfilter/forwarder/icmp.go`:
- Around line 296-304: The code is double-counting the ICMPv6 pseudo-header by
passing header.PseudoHeaderChecksum(...) as PayloadCsum into
header.ICMPv6Checksum; remove the pseudo-header usage and let ICMPv6Checksum
derive the pseudo-header from Src/Dst. Concretely, delete the psum calculation
and call
replyHdr.SetChecksum(header.ICMPv6Checksum(header.ICMPv6ChecksumParams{Header:
replyHdr, Src: id.LocalAddress, Dst: id.RemoteAddress, PayloadCsum: 0,
PayloadLen: 0})), or omit PayloadCsum/PayloadLen entirely, since replyHdr
already contains the full ICMPv6 message (symbols: replyHdr,
header.PseudoHeaderChecksum, header.ICMPv6Checksum, header.ICMPv6ChecksumParams,
replyICMP, header.ICMPv6MinimumSize, id.LocalAddress, id.RemoteAddress).
---
Nitpick comments:
In `@client/firewall/uspfilter/forwarder/icmp.go`:
- Around line 338-370: The buildPingCommand function contains OS-specific logic
for choosing ping binary and timeout flag; refactor by extracting a helper
(e.g., determinePingArgs or pingPlatformSpec) that returns the binary name and
[]string flags given runtime.GOOS, isV6, and timeoutSec, then have
buildPingCommand call exec.CommandContext(ctx, bin, args...) using
pingBin/ping6Bin only inside that helper; add table-driven unit tests for
determinePingArgs covering linux, darwin/ios, freebsd, openbsd/netbsd, windows
and default cases to verify binary selection and timeout flag formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eefc2700-3a38-482c-9074-777eba83a099
📒 Files selected for processing (5)
client/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/firewall/uspfilter/localip.goclient/firewall/uspfilter/localip_bench_test.goclient/internal/engine.go
lixmal
force-pushed
the
client-ipv6-acl-usp
branch
2 times, most recently
from
ac8cb93 to
71266a5
Compare
lixmal
force-pushed
the
client-ipv6-acl-usp
branch
from
71266a5 to
ed5cfa6
Compare
|
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
SourcePrefixesfrom firewall rules (compact bytes), fall back to deprecatedPeerIPfor old management. Accumulate errors instead of rolling back on first failure.rewriteIPv4/rewriteIPv6split (v6 has no header checksum),updateICMPv6Checksumwith pseudo-header, zero-allocextractPacketIPs[v6]:portformattingsplitAllowedIPs/extractPeerIPsintooverlayAddrsFromAllowedIPsreturning typednetip.AddrIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Behavior Changes
Performance
Tests