[management] Add IPv6 overlay addressing and capability gating#5698
Open
lixmal wants to merge 10 commits intoproto-ipv6-overlayfrom
Open
[management] Add IPv6 overlay addressing and capability gating#5698lixmal wants to merge 10 commits intoproto-ipv6-overlayfrom
lixmal wants to merge 10 commits intoproto-ipv6-overlayfrom
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:
📝 WalkthroughWalkthroughAdds IPv6 overlay support across management: peer structs gain IPv6 addresses and capabilities; network and allocation APIs migrate to netip types; account settings and store persist IPv6 ranges/groups; network map, firewall rules, gRPC conversion, HTTP handlers, and tests are updated to be IPv6-aware. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Handler as Peers Handler
participant AcctMgr as Account Manager
participant Store as SQL Store
participant NetMap as Network Map Builder
participant gRPC as gRPC Server
User->>Handler: UpdatePeer with ipv6 field
Handler->>Handler: parseIPv6() validate
Handler->>AcctMgr: UpdatePeerIPv6(ctx, accountID, userID, peerID, ipv6Addr)
AcctMgr->>Store: Check IPv6 collisions / validate within NetworkRangeV6
alt IPv6 Valid & No Collision
AcctMgr->>Store: Persist peer IPv6
AcctMgr->>NetMap: Trigger network map rebuild
NetMap->>NetMap: Compute includeIPv6 per target peer
NetMap->>NetMap: Generate IPv6 firewall & route entries
NetMap->>gRPC: Push updated NetworkMap
gRPC-->>User: Success
else Invalid or Collision
AcctMgr-->>User: Error (InvalidArgument / conflict)
end
sequenceDiagram
participant Account as Account Manager
participant Types as Allocation Types
participant Store as SQL Store
Account->>Account: Settings update (NetworkRangeV6 / IPv6EnabledGroups)
Account->>Types: Determine peers needing IPv6 (peerWillHaveIPv6)
alt Need IPv6 allocation
Account->>Types: AllocateRandomPeerIPv6(prefix)
Types-->>Account: IPv6 address
Account->>Store: Persist updated peer IPv6 addresses
end
Account->>Account: Emit AccountIPv6Enabled/Disabled events
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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.
|
4 tasks
![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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
management/server/types/settings.go (1)
87-111:⚠️ Potential issue | 🟡 MinorDeep-copy
JWTAllowGroupsas well.
Copy()still aliases that slice into the clone, so mutating the returned settings can mutate the original even though the other group slices are now cloned.Suggested fix
- JWTAllowGroups: s.JWTAllowGroups, + JWTAllowGroups: slices.Clone(s.JWTAllowGroups),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/settings.go` around lines 87 - 111, The Settings.Copy() builder is currently aliasing the JWTAllowGroups slice which allows mutations on the copy to affect the original; update the construction of the new Settings (the Settings literal in Copy()) to deep-copy JWTAllowGroups just like PeerExposeGroups and IPv6EnabledGroups by using slices.Clone(s.JWTAllowGroups) so the returned Settings has its own independent slice.management/internals/modules/reverseproxy/service/manager/manager_test.go (1)
673-683:⚠️ Potential issue | 🔴 CriticalRemove the duplicated
IPv6field from these peer literals.Each literal assigns
IPv6twice, which is invalid in a keyed Go composite literal and prevents the test file from compiling.Suggested fix
- IP: netip.MustParseAddr("100.64.0.1"), - IPv6: netip.MustParseAddr("fd00::1"), - IPv6: netip.MustParseAddr("fd00::1"), + IP: netip.MustParseAddr("100.64.0.1"), + IPv6: netip.MustParseAddr("fd00::1"),- IP: netip.MustParseAddr("100.64.0.2"), - IPv6: netip.MustParseAddr("fd00::2"), - IPv6: netip.MustParseAddr("fd00::2"), + IP: netip.MustParseAddr("100.64.0.2"), + IPv6: netip.MustParseAddr("fd00::2"),Applies to: lines 673–683 and 746–756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/modules/reverseproxy/service/manager/manager_test.go` around lines 673 - 683, The test peer composite literals for testPeerID contain a duplicated IPv6 field which makes the Go file fail to compile; remove the extra IPv6 assignment in each peer literal (e.g., the map entry keyed by testPeerID used in the tests) so each composite literal only sets IPv6 once and preserve the correct value (same for the second occurrence later in the file referenced by the same test peer setup), leaving all other fields (ID, AccountID, Key, DNSLabel, Name, IP, Status, Meta) unchanged.management/server/types/firewall_rule.go (1)
71-110:⚠️ Potential issue | 🟠 MajorDon't emit a family-specific route rule with empty
SourceRanges.For a static IPv6 route,
sourceRangesbecomesv6Sources. If every group peer lacksIPv6, this still appends a rule withSourceRanges == [], while the v6 counterpart below is suppressed in the same situation. This branch should return no rule as well.💡 Suggested fix
sourceRanges := v4Sources if isV6Route { sourceRanges = v6Sources } + if len(sourceRanges) == 0 { + return rules + } baseRule := RouteFirewallRule{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/firewall_rule.go` around lines 71 - 110, The code currently emits a RouteFirewallRule even when the picked sourceRanges (set to v6Sources for IPv6 static routes) is empty; update the logic to skip generating/appending any rules when len(sourceRanges) == 0 by adding a guard before creating/using baseRule (and before appending/generating via generateRulesWithPortRanges or generateRulesWithPorts) so no family-specific rule is produced for empty SourceRanges; ensure the same guard prevents creating the v6 counterpart (use the existing isV6Route/includeIPv6 checks together with len(sourceRanges) > 0 or len(v6Sources) > 0 as appropriate).management/server/types/networkmapbuilder.go (1)
852-869:⚠️ Potential issue | 🟠 MajorKeep route-firewall delta maintenance in sync with the new IPv6 rules.
getPeerRoutesFirewallRulescan now emit IPv6 entries, but the incremental add/delete path still patches cached rules with onlypeer.IP(calculateRouteFirewallUpdates,findPeersAffectedByDeletedPeerACL,removeIPFromRouteFirewallRules). A newly added or deleted IPv6-capable peer will leave existing owners' IPv6SourceRangesstale until a full rebuild.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmapbuilder.go` around lines 852 - 869, getPeerRoutesFirewallRules now emits IPv6 entries but the incremental delta patching functions (calculateRouteFirewallUpdates, findPeersAffectedByDeletedPeerACL, removeIPFromRouteFirewallRules) only operate on peer.IP, so adding/removing an IPv6-capable peer leaves IPv6 SourceRanges stale; update those three functions to treat route firewall rules symmetrically for IPv4 and IPv6 by: when computing diffs in calculateRouteFirewallUpdates and when locating affected rules in findPeersAffectedByDeletedPeerACL, check both IPv4 and IPv6 source ranges/fields (e.g., peer.IP and peer.IPv6 or the rule.SourceRanges entries that are v4 vs v6), and modify removeIPFromRouteFirewallRules to remove the peer's IPv6 CIDR from IPv6 entries as well as IPv4 CIDR from IPv4 entries (use the same matching logic getPeerRoutesFirewallRules uses to emit CIDRs) so incremental add/delete keeps IPv6 SourceRanges in sync with full rebuilds.management/server/types/networkmap_components.go (1)
583-586:⚠️ Potential issue | 🟠 MajorThe default-permit route path still bypasses IPv6 gating.
When
len(r.AccessControlGroups) == 0, this branch appendsgetDefaultPermit(r)unconditionally.getDefaultPermitcan emit IPv6-source rules, so non-v6 peers with unrestricted routes will still receive IPv6 route firewall entries through this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 583 - 586, The default-permit branch unconditionally appends rules from getDefaultPermit(r) and thus can introduce IPv6-source rules even when IPv6 gating should block them; modify this branch so that before appending the returned rules to routesFirewallRules you apply the same IPv6 gating filter used elsewhere (e.g., the codepath that filters route rules by peer IPv6 capability) — for example, filter the slice returned by getDefaultPermit(r) to remove IPv6-source rules when the peer/route disallows IPv6 (use the same gating logic or helper function used elsewhere), then append the filtered results to routesFirewallRules instead of appending getDefaultPermit(r) directly, keeping references to r, getDefaultPermit, and routesFirewallRules to locate the change.management/server/http/handlers/accounts/accounts_handler.go (1)
386-404:⚠️ Potential issue | 🟠 MajorNormalize
ipv6_enabled_groupsbefore taking its address.
&settings.IPv6EnabledGroupsis non-nil even when the slice itself is nil, so this serializes"ipv6_enabled_groups": nullinstead of omitting the field or returning[]. That breaks the array contract for older or uninitialized accounts.🧩 Proposed fix
jwtAllowGroups := settings.JWTAllowGroups if jwtAllowGroups == nil { jwtAllowGroups = []string{} } + ipv6EnabledGroups := settings.IPv6EnabledGroups + if ipv6EnabledGroups == nil { + ipv6EnabledGroups = []string{} + } apiSettings := api.AccountSettings{ PeerLoginExpiration: int(settings.PeerLoginExpiration.Seconds()), @@ - Ipv6EnabledGroups: &settings.IPv6EnabledGroups, + Ipv6EnabledGroups: &ipv6EnabledGroups, EmbeddedIdpEnabled: &settings.EmbeddedIdpEnabled, LocalAuthDisabled: &settings.LocalAuthDisabled, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/accounts/accounts_handler.go` around lines 386 - 404, The code takes the address of settings.IPv6EnabledGroups directly which yields a non-nil pointer that serializes as null for nil slices; fix by normalizing into a local variable first: create a local variable (e.g. ipv6EnabledGroups := settings.IPv6EnabledGroups), if it is nil set it to an empty slice (ipv6EnabledGroups = []string{}), then pass &ipv6EnabledGroups into api.AccountSettings.Ipv6EnabledGroups so the JSON serializes as [] instead of null; update the construction of api.AccountSettings where Ipv6EnabledGroups is set.
🧹 Nitpick comments (8)
management/server/types/account_test.go (1)
1322-1455: Add a mixed A/AAAA case here.These fixtures now carry IPv6 addresses, but the assertions still only validate A records. That means the new AAAA filtering/gating behavior can regress without this test noticing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/account_test.go` around lines 1322 - 1455, Add a new table-driven test case "mixed A/AAAA" that includes IPv6 addresses in peersToConnect (use the existing IPv6 fields) and corresponding AAAA entries in customZone. Update the expectedRecords slice for that case to include both A records (Type int(dns.TypeA)) and AAAA records (Type int(dns.TypeAAAA)) with RData set to the peers' IPv6 strings; ensure the peers defined in peersToConnect have matching IPv6 values and that expiredPeers/peer are handled the same as other cases. Locate and modify the test table entries using the existing identifiers customZone, peersToConnect, expiredPeers, peer and expectedRecords so the assertions validate both A and AAAA records for the mixed case.management/server/store/sql_store_test.go (1)
2610-2615: Assert the IPv6 round-trip in this persistence test.This fixture now writes
Peer.IPv6, but the verification below still only checksIP. A broken serializer/column mapping for the new field would still pass here.Suggested assertion
assert.Equal(t, peer.ID, storedPeer.ID) assert.Equal(t, peer.AccountID, storedPeer.AccountID) assert.Equal(t, peer.Key, storedPeer.Key) assert.Equal(t, peer.IP.String(), storedPeer.IP.String()) +assert.Equal(t, peer.IPv6.String(), storedPeer.IPv6.String()) assert.Equal(t, peer.Meta, storedPeer.Meta) assert.Equal(t, peer.Name, storedPeer.Name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store_test.go` around lines 2610 - 2615, The test creates a Peer fixture that sets Peer.IPv6 but the verification only checks IP; update the persistence test in sql_store_test.go to assert the IPv6 round-trip by comparing the stored/retrieved peer's IPv6 to the original value (e.g. after you persist and load the peer, assert retrievedPeer.IPv6 == peer.IPv6). Ensure you reference the Peer variable name used in the test (peer) and the retrieved object (retrievedPeer or whatever the test uses) so a broken serializer/column mapping for Peer.IPv6 will fail the test.management/server/types/networkmap_golden_test.go (1)
736-750: Add a negative golden/assertion for a non-v6-capable recipient.These fixtures now enable IPv6 for
allGroupID, but every golden comparison still renderstestingPeerID, which is one of the0.40.0peers. That only exercises the positive path. A regression that sends IPv6 routes, firewall rules, or AAAA answers to older/non-capable clients would still pass this file.Also applies to: 886-886
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_golden_test.go` around lines 736 - 750, Add a negative golden/assertion that exercises a non-IPv6-capable recipient by adding a nbpeer.Peer with a legacy WtVersion (e.g. "0.25.0") and PeerSystemMeta lacking IPv6 capability, then run the same rendering/comparison using that peer as the recipient (instead of testingPeerID) to assert that no IPv6-specific output (IPv6 routes, firewall v6 rules, AAAA answers) is produced; locate the peer creation (nbpeer.Peer + nbpeer.PeerSystemMeta) in the loop and add a dedicated legacy peer and a complementary golden assertion where testingPeerID/allGroupID comparisons are performed (also add the same negative test around the other occurrence mentioned).management/server/route_test.go (1)
1895-1896: Exercise theincludeIPv6=falsebranch too.All of the new call sites pass
true, so this test never verifies the gate that suppresses IPv6 firewall rules. An ignored or inverted flag would still pass here. One negative assertion (for example, proving the::/0rule disappears forpeerEwhen the flag isfalse) would lock down the new behavior.Also applies to: 1949-1950, 1957-1958, 1991-1992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/route_test.go` around lines 1895 - 1896, The tests currently only call account.GetPeerRoutesFirewallRules(..., true) so they never exercise the includeIPv6=false branch; update the three test sites that call GetPeerRoutesFirewallRules (the ones passing the true flag) to also call the function with includeIPv6=false and add a negative assertion ensuring IPv6 rules are suppressed (for example, assert that the ::/0 route for peerE is absent when includeIPv6 is false). Locate calls to GetPeerRoutesFirewallRules in this file (and the other two similar spots noted) and add the new call/assertion alongside the existing true-case to verify the IPv6 gate works.management/server/account_test.go (1)
4117-4125: Assert that the failed update is side-effect free.This last case only checks the returned error. Since
UpdateAccountSettingsretroactively mutates peer IPv6 assignments, the test should also verify that the invalid group ID does not partially clear or reassign any peer IPv6 state before the error is returned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/account_test.go` around lines 4117 - 4125, Before calling manager.UpdateAccountSettings with the invalid IPv6 group, snapshot the current peer IPv6 assignment state (e.g. via the existing peer listing/getter used elsewhere in the test) for peers belonging to accountID/userID; after the call assert that an error was returned and that the snapshot equals the post-call peer state so no IPv6 addresses were cleared or reassigned. Specifically, around the failing UpdateAccountSettings invocation check the peers' IPv6 fields (the same fields UpdateAccountSettings would touch for IPv6EnabledGroups) before and after and assert equality to ensure the operation is side-effect free.management/server/peer_test.go (1)
1105-1132: Assert the IPv6 sync output, not just the fixtures.These peers now carry IPv6 addresses, but the test still only checks the legacy IPv4
AddressandAllowedIps[0]. A regression in the new IPv6 fields—or just a different allowed-IP ordering—would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/peer_test.go` around lines 1105 - 1132, The test builds nbpeer.Peer fixtures with IPv6 (IPv6 fields set) but only asserts legacy IPv4 outputs (Address and AllowedIps[0]); update the test (in the peer test where peer, networkMap and types.NetworkMap are constructed) to assert the generated sync/export contains the IPv6 address and corresponding allowed-ip entries for those peers (check fields derived from Peer.IPv6 in the code path that produces the sync), and make the assertions order-tolerant (e.g., verify both IPv4 and IPv6 are present in AllowedIps or compare as sets) so regressions in IPv6 handling or allowed-ips ordering will fail the test.shared/management/http/api/openapi.yml (1)
797-800: Addformat: ipv6to response IPv6 fields for schema consistency.
PeerRequest.ipv6usesformat: ipv6, but response models (Peer,AccessiblePeer) do not. Adding format improves client generation and validation hints.♻️ Suggested refactor
ipv6: description: Peer's IPv6 overlay address type: string + format: ipv6 example: "fd00:4e42:ab12::1" @@ ipv6: description: Peer's IPv6 overlay address type: string + format: ipv6 example: "fd00:4e42:ab12::1"Also applies to: 1019-1022
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 797 - 800, The OpenAPI schemas for response models are missing the IPv6 format: update the ipv6 property in the Peer and AccessiblePeer response schemas to include format: ipv6 (matching PeerRequest.ipv6) so client generators and validators get the correct hint; locate the ipv6 definitions under the Peer and AccessiblePeer schema objects in openapi.yml and add format: ipv6 to each ipv6 property (also apply the same change to the other occurrence noted in the comment).management/internals/shared/grpc/conversion.go (1)
319-370: Consider extracting wildcard rule expansion to reduce complexity.Static analysis flagged this function for cognitive complexity (28 > 20) and nesting depth (> 4 levels at line 346). The function handles multiple concerns: IPv6 filtering, source prefix encoding, and wildcard expansion.
Consider extracting the wildcard handling (lines 340-361) into a helper function to improve readability and reduce nesting:
♻️ Suggested refactor to reduce complexity
+// handleWildcardSourcePrefix handles the special case where PeerIP is "0.0.0.0", +// expanding it into separate v4 and v6 rules when appropriate. +func handleWildcardSourcePrefix( + fwRule *proto.FirewallRule, + rule *types.FirewallRule, + includeIPv6 bool, + result *[]*proto.FirewallRule, +) { + fwRule.SourcePrefixes = [][]byte{ + netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv4Unspecified(), 0)), + } + + if includeIPv6 { + v6Rule := cloneFirewallRule(fwRule) + v6Rule.PeerIP = "::" + v6Rule.SourcePrefixes = [][]byte{ + netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv6Unspecified(), 0)), + } + if shouldUsePortRange(v6Rule) { + v6Rule.PortInfo = rule.PortRange.ToProto() + } + *result = append(*result, v6Rule) + } +} func toProtocolFirewallRules(rules []*types.FirewallRule, includeIPv6, useSourcePrefixes bool) []*proto.FirewallRule { result := make([]*proto.FirewallRule, 0, len(rules)) for i := range rules { rule := rules[i] if !includeIPv6 && rule.IPv6 { continue } fwRule := &proto.FirewallRule{ PolicyID: []byte(rule.PolicyID), PeerIP: rule.PeerIP, Direction: getProtoDirection(rule.Direction), Action: getProtoAction(rule.Action), Protocol: getProtoProtocol(rule.Protocol), Port: rule.Port, } if useSourcePrefixes && rule.PeerIP != "" { addr, err := netip.ParseAddr(rule.PeerIP) if err == nil { if addr.IsUnspecified() { - // "0.0.0.0" means all peers: emit separate v4 and v6 wildcard rules - fwRule.SourcePrefixes = [][]byte{ - netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv4Unspecified(), 0)), - } - - if includeIPv6 { - v6Rule := cloneFirewallRule(fwRule) - v6Rule.PeerIP = "::" - v6Rule.SourcePrefixes = [][]byte{ - netiputil.EncodePrefix(netip.PrefixFrom(netip.IPv6Unspecified(), 0)), - } - if shouldUsePortRange(v6Rule) { - v6Rule.PortInfo = rule.PortRange.ToProto() - } - result = append(result, v6Rule) - } + handleWildcardSourcePrefix(fwRule, rule, includeIPv6, &result) } else { fwRule.SourcePrefixes = [][]byte{netiputil.EncodeAddr(addr.Unmap())} } } } // ... rest unchanged🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/shared/grpc/conversion.go` around lines 319 - 370, The toProtocolFirewallRules function is over-complex and deeply nested; extract the wildcard expansion block that handles addr.IsUnspecified() and related v6/v4 rule creation into a new helper (e.g., expandWildcardFirewallRules or handleWildcardSourcePrefixes) and call it from toProtocolFirewallRules; the helper should accept the current fwRule, the original rule (types.FirewallRule), includeIPv6 and return one or more *proto.FirewallRule (or append to a slice) after performing the v4/v6 SourcePrefixes population, setting PeerIP for v6, cloning via cloneFirewallRule, and applying shouldUsePortRange(rule) to set PortInfo using rule.PortRange.ToProto(); this reduces nesting in toProtocolFirewallRules while keeping behavior around netiputil.EncodePrefix, netiputil.EncodeAddr, netip.ParseAddr, and port range handling identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/account.go`:
- Around line 2286-2299: ensureIPv6Subnet allocates and persists network.NetV6
but doesn't update settings.NetworkRangeV6, leaving settings out of sync; after
generating and assigning network.NetV6 in ensureIPv6Subnet, set
settings.NetworkRangeV6 to the corresponding prefix (derived from network.NetV6
masked addr and prefix length) and persist that change (e.g., call
transaction.UpdateAccountSettings(ctx, accountID, settings) or the existing
UpdateAccountSettings equivalent) so the stored settings reflect the new IPv6
subnet; update only the branch where network.NetV6 is newly allocated (the code
paths in ensureIPv6Subnet that call transaction.UpdateAccountNetworkV6).
- Around line 2483-2545: UpdatePeerIPv6 currently sets peer.IPv6 without
checking the account's IPv6 allowlist, letting manual updates bypass
IPv6EnabledGroups; before assigning/saving peer.IPv6 in UpdatePeerIPv6 (after
loading peer and before setting peer.IPv6 and SavePeer) validate that the target
peer is permitted to have IPv6 using the same logic as buildIPv6AllowedPeers (or
consult account.IPv6EnabledGroups) and if the peer is not allowed return an
error (e.g., PreconditionFailed or InvalidArgument) or clear the IPv6 instead of
saving; ensure this check occurs inside the same transaction where you fetched
the peer so you use transaction-scoped data and prevent unauthorized
assignments.
- Around line 2493-2520: The transaction currently locks network → target peer →
all peers causing inconsistent lock order vs updatePeerIPv6Addresses (peers →
network); fix by fetching the account-wide peers before locking the target peer
so the order is peers → network → peer (i.e., call
transaction.GetAccountPeers(...) prior to transaction.GetPeerByID(...)),
preserving the same LockingStrength usage (use share for the account peers scan
and update for the specific peer) and keep the existing network read via
transaction.GetAccountNetwork(...) in the same transaction without changing its
lock strength so both paths use the same lock ordering and avoid deadlocks.
In `@management/server/http/handlers/accounts/accounts_handler.go`:
- Around line 179-190: The code currently ignores errors from
settingsManager.GetEffectiveNetworkRanges and continues returning a 200 with
missing resp.Settings.NetworkRange/NetworkRangeV6; change the block in
accounts_handler.go so that if GetEffectiveNetworkRanges(ctx, accountID) returns
a non-nil error you immediately propagate it (return early) instead of silently
continuing—call the handler's error path (the same pattern used elsewhere in the
file for manager/store errors) when GetEffectiveNetworkRanges fails; only
populate resp.Settings.NetworkRange and resp.Settings.NetworkRangeV6 when err ==
nil and the returned ranges are valid.
In `@management/server/mock_server/account_mock.go`:
- Around line 542-544: The UpdatePeerIPv6 mock currently always returns
Unimplemented; add a configurable hook to MockAccountManager named
UpdatePeerIPv6Func with signature func(ctx context.Context, accountID, userID,
peerID string, newIPv6 netip.Addr) error and change the UpdatePeerIPv6 method to
call am.UpdatePeerIPv6Func when non-nil (returning its result) and otherwise
return nil (simulate success), mirroring the pattern used by the other mock
methods.
In `@management/server/peer.go`:
- Around line 805-809: The call to am.Store.GetGroupByName when resolving the
"All" group is swallowing errors which can cause peerWillHaveIPv6 to
mis-evaluate when IPv6EnabledGroups contains "All"; update the block around
peer.ProxyMeta.Embedded to check the error returned by
am.Store.GetGroupByName(accountID, "All") and if err != nil and
IPv6EnabledGroups contains "All" (or otherwise influences peerWillHaveIPv6),
propagate that error immediately (return it) instead of assigning an empty
allGroupID and continuing; reference the symbols peer.ProxyMeta.Embedded,
am.Store.GetGroupByName, accountID, IPv6EnabledGroups, and peerWillHaveIPv6 when
locating where to add the early return.
In `@management/server/peer/peer.go`:
- Around line 229-232: SupportsIPv6 currently ignores the peer's opt-out flag;
update Peer.SupportsIPv6 to return true only if the peer has the IPv6 overlay
capability AND has not opted out (i.e., check the DisableIPv6 flag). Concretely,
change the logic in the SupportsIPv6 method to combine
p.HasCapability(PeerCapabilityIPv6Overlay) with a negation of p.DisableIPv6 so
callers won't treat opted-out peers as IPv6-enabled.
- Around line 195-196: The capabilities slice comparison uses slices.Equal which
is order-sensitive; change it to an order-insensitive set comparison. Replace
slices.Equal(p.Capabilities, other.Capabilities) with a call to a new helper
(e.g., capabilitiesEqualSet) or inline logic that first checks lengths then
builds a map[string]struct{} (or map[Capability]struct{}) from one side and
ensures every element of the other side exists in that map. Keep the surrounding
check using p.Flags.isEqual(other.Flags) the same and use the helper name
(capabilitiesEqualSet) or the inline map logic so capability order no longer
affects isEqual.
In `@management/server/route_test.go`:
- Around line 2860-2861: The test is asserting the wrong code path: in
TestAccount_GetPeerNetworkResourceFirewallRules it calls
account.GetPeerRoutesFirewallRules and asserts length 0, so it never verifies
GetPeerNetworkResourceFirewallRules; change the call to call
account.GetPeerNetworkResourceFirewallRules (with the same parameters:
context.Background(), "peerC", validatedPeers, true) and keep the assert.Len(t,
firewallRules, 0) to ensure the resource-firewall helper is actually tested;
leave other surrounding test setup unchanged.
In `@management/server/settings/manager.go`:
- Around line 131-136: The code is discarding errors from netip.ParsePrefix when
parsing network.Net and network.NetV6, producing zero-value prefixes (v4, v6)
instead of surfacing malformed data; update the parsing logic around the v4 and
v6 assignments so that you capture the error returned by netip.ParsePrefix for
network.Net.String() and network.NetV6.String() and immediately return or
propagate that error (rather than ignoring it), ensuring callers receive an
error when ParsePrefix fails; reference netip.ParsePrefix, the network.Net and
network.NetV6 inputs, and the v4/v6 variables to locate and fix the issue.
In `@management/server/store/sql_store_test.go`:
- Around line 3964-3966: The peer struct literals in sql_store_test.go contain
duplicate IPv6 fields which cause a compile error; locate the peer literal
instances that include repeated "IPv6: netip.MustParseAddr(...)" (the test peer
initializations around the failing assertions) and remove the duplicate entries
so each struct has only one IPv6 field (keep the intended IPv6 value and delete
the repeated line), ensuring the rest of the peer fields (e.g., IP) remain
unchanged.
In `@management/server/store/sql_store.go`:
- Around line 4618-4637: Summary: New UpdateAccountNetworkV6 writes IPv6 fields
but the read path still drops them; update the SELECT/Scan to load IPv6 columns.
Fix: modify getAccount()/getAccountPgx() to include the database columns
network_net_v6, settings_network_range_v6 and settings_ipv6_enabled_groups in
the SELECT and map them into the corresponding fields on types.Account
(Network.NetV6, Settings.NetworkRangeV6, Settings.IPv6EnabledGroups); ensure the
Scan call order and target variables match these new columns (or update the
row-to-account mapping helper used by getAccountPgx), and add any missing struct
tags or type conversions so subsequent GetAccount() calls return the persisted
IPv6 values instead of zero values.
In `@management/server/types/firewall_rule_test.go`:
- Around line 16-37: The IPv6 assertions in TestSplitPeerSourcesByFamily are
incorrect: the fixture includes an IPv6 for the second peer (fd00::2) so
splitPeerSourcesByFamily should produce fd00::1/128, fd00::2/128, fd00::3/128;
update the expected v6 slice in TestSplitPeerSourcesByFamily (and the similar
assertions in the later test around splitPeerSourcesByFamily) to include the
missing "fd00::2/128" entry so the test matches the provided peers and the
behavior of splitPeerSourcesByFamily.
- Around line 132-160: The test
TestGenerateRouteFirewallRules_DynamicRoute_NoV6Peers is accidentally exercising
the dual‑stack branch because both peer entries have IPv6 set; update the peers
in the test so their IPv6 fields are zeroed (set to the netip.Addr zero value)
so generateRouteFirewallRules(..., true) treats them as no‑v6 peers and the
expectation require.Len(t, rules, 1) and the v4 SourceRanges assertion hold.
In `@management/server/types/ipv6_groups_test.go`:
- Around line 107-117: The helper peerWithV6 currently always pre-populates IPv6
with fd00::1 so calls like peerWithV6("", ...) never produce a no-IPv6 peer;
update peerWithV6 to not set IPv6 by default (create nbpeer.Peer without IPv6)
and only assign p.IPv6 = netip.MustParseAddr(v6) when v6 != "" so callers
passing an empty string get the zero-value (no IPv6) case; locate the peerWithV6
function in the test and remove the default IPv6 initialization.
In `@management/server/types/network.go`:
- Around line 283-288: The current check computes hostStart := ones / 8 and
calls isZeroSuffix(ip[hostStart:]) / isAllOnesSuffix(ip[hostStart:]) which
incorrectly includes the partial network byte for non-byte-aligned prefixes
(e.g., /60); mask out the network bits in the partial byte before running the
suffix checks or else restrict/ document to byte-aligned prefixes: compute the
bitOffset := ones % 8, if bitOffset != 0 then clear the upper network bits in
ip[hostStart] (preserving only host bits) before calling
isZeroSuffix/isAllOnesSuffix (or skip masking and add a comment on limitations),
ensuring the logic around hostStart, isZeroSuffix, isAllOnesSuffix and the
fallback ip[15] = 1 remains consistent.
In `@management/server/types/networkmap_components.go`:
- Around line 119-123: The includeIPv6 flag is currently set solely by
p.SupportsIPv6(), which is too weak; change the predicate where includeIPv6 is
computed (around p := c.Peers[targetPeerID] and the call to
getPeerRoutesFirewallRules) to require both capability and a valid assigned
address (e.g., includeIPv6 = p != nil && p.SupportsIPv6() && p.IPv6.IsValid()),
and apply the same combined predicate in connResourcesGenerator so both
route/firewall generation and connection resource generation consistently gate
IPv6 state on capability + p.IPv6.IsValid(); update references to includeIPv6
and the getPeerRoutesFirewallRules/connResourcesGenerator usage accordingly.
In `@management/server/types/route_firewall_rule.go`:
- Around line 39-42: The Equal() method for the RouteFirewallRule type currently
omits the new IPv6 bool field so rules differing only by address family are
treated as equal; update the RouteFirewallRule.Equal(other RouteFirewallRule)
implementation to include a comparison of the IPv6 field (i.e., add a check like
r.IPv6 == other.IPv6) alongside the existing field comparisons so the equality
respects the address-family flag.
In `@shared/management/http/api/openapi.yml`:
- Around line 773-777: Update the PeerRequest.ipv6 field description to remove
the "Empty if IPv6 is not enabled" wording and instead state that the field will
be omitted (or not present) when IPv6 is not enabled; keep the type: string and
format: ipv6 as-is (or alternatively add nullable: true only if you intend to
allow an explicit empty/null value), so the OpenAPI schema for ipv6 matches its
literal IPv6 format and does not imply empty-string semantics.
- Around line 368-373: The OpenAPI field ipv6_enabled_groups has a conflicting
description and example; update one so they match: either change the description
for ipv6_enabled_groups to clarify that new accounts default to the "All" group
but the example shows an empty array for accounts that currently have no groups,
or change the example to reflect the stated default (e.g., example: ["All"]);
edit the ipv6_enabled_groups block in shared/management/http/api/openapi.yml to
make the description and example consistent and use the literal string "All" if
you choose the example-for-new-account approach.
---
Outside diff comments:
In `@management/internals/modules/reverseproxy/service/manager/manager_test.go`:
- Around line 673-683: The test peer composite literals for testPeerID contain a
duplicated IPv6 field which makes the Go file fail to compile; remove the extra
IPv6 assignment in each peer literal (e.g., the map entry keyed by testPeerID
used in the tests) so each composite literal only sets IPv6 once and preserve
the correct value (same for the second occurrence later in the file referenced
by the same test peer setup), leaving all other fields (ID, AccountID, Key,
DNSLabel, Name, IP, Status, Meta) unchanged.
In `@management/server/http/handlers/accounts/accounts_handler.go`:
- Around line 386-404: The code takes the address of settings.IPv6EnabledGroups
directly which yields a non-nil pointer that serializes as null for nil slices;
fix by normalizing into a local variable first: create a local variable (e.g.
ipv6EnabledGroups := settings.IPv6EnabledGroups), if it is nil set it to an
empty slice (ipv6EnabledGroups = []string{}), then pass &ipv6EnabledGroups into
api.AccountSettings.Ipv6EnabledGroups so the JSON serializes as [] instead of
null; update the construction of api.AccountSettings where Ipv6EnabledGroups is
set.
In `@management/server/types/firewall_rule.go`:
- Around line 71-110: The code currently emits a RouteFirewallRule even when the
picked sourceRanges (set to v6Sources for IPv6 static routes) is empty; update
the logic to skip generating/appending any rules when len(sourceRanges) == 0 by
adding a guard before creating/using baseRule (and before appending/generating
via generateRulesWithPortRanges or generateRulesWithPorts) so no family-specific
rule is produced for empty SourceRanges; ensure the same guard prevents creating
the v6 counterpart (use the existing isV6Route/includeIPv6 checks together with
len(sourceRanges) > 0 or len(v6Sources) > 0 as appropriate).
In `@management/server/types/networkmap_components.go`:
- Around line 583-586: The default-permit branch unconditionally appends rules
from getDefaultPermit(r) and thus can introduce IPv6-source rules even when IPv6
gating should block them; modify this branch so that before appending the
returned rules to routesFirewallRules you apply the same IPv6 gating filter used
elsewhere (e.g., the codepath that filters route rules by peer IPv6 capability)
— for example, filter the slice returned by getDefaultPermit(r) to remove
IPv6-source rules when the peer/route disallows IPv6 (use the same gating logic
or helper function used elsewhere), then append the filtered results to
routesFirewallRules instead of appending getDefaultPermit(r) directly, keeping
references to r, getDefaultPermit, and routesFirewallRules to locate the change.
In `@management/server/types/networkmapbuilder.go`:
- Around line 852-869: getPeerRoutesFirewallRules now emits IPv6 entries but the
incremental delta patching functions (calculateRouteFirewallUpdates,
findPeersAffectedByDeletedPeerACL, removeIPFromRouteFirewallRules) only operate
on peer.IP, so adding/removing an IPv6-capable peer leaves IPv6 SourceRanges
stale; update those three functions to treat route firewall rules symmetrically
for IPv4 and IPv6 by: when computing diffs in calculateRouteFirewallUpdates and
when locating affected rules in findPeersAffectedByDeletedPeerACL, check both
IPv4 and IPv6 source ranges/fields (e.g., peer.IP and peer.IPv6 or the
rule.SourceRanges entries that are v4 vs v6), and modify
removeIPFromRouteFirewallRules to remove the peer's IPv6 CIDR from IPv6 entries
as well as IPv4 CIDR from IPv4 entries (use the same matching logic
getPeerRoutesFirewallRules uses to emit CIDRs) so incremental add/delete keeps
IPv6 SourceRanges in sync with full rebuilds.
In `@management/server/types/settings.go`:
- Around line 87-111: The Settings.Copy() builder is currently aliasing the
JWTAllowGroups slice which allows mutations on the copy to affect the original;
update the construction of the new Settings (the Settings literal in Copy()) to
deep-copy JWTAllowGroups just like PeerExposeGroups and IPv6EnabledGroups by
using slices.Clone(s.JWTAllowGroups) so the returned Settings has its own
independent slice.
---
Nitpick comments:
In `@management/internals/shared/grpc/conversion.go`:
- Around line 319-370: The toProtocolFirewallRules function is over-complex and
deeply nested; extract the wildcard expansion block that handles
addr.IsUnspecified() and related v6/v4 rule creation into a new helper (e.g.,
expandWildcardFirewallRules or handleWildcardSourcePrefixes) and call it from
toProtocolFirewallRules; the helper should accept the current fwRule, the
original rule (types.FirewallRule), includeIPv6 and return one or more
*proto.FirewallRule (or append to a slice) after performing the v4/v6
SourcePrefixes population, setting PeerIP for v6, cloning via cloneFirewallRule,
and applying shouldUsePortRange(rule) to set PortInfo using
rule.PortRange.ToProto(); this reduces nesting in toProtocolFirewallRules while
keeping behavior around netiputil.EncodePrefix, netiputil.EncodeAddr,
netip.ParseAddr, and port range handling identical.
In `@management/server/account_test.go`:
- Around line 4117-4125: Before calling manager.UpdateAccountSettings with the
invalid IPv6 group, snapshot the current peer IPv6 assignment state (e.g. via
the existing peer listing/getter used elsewhere in the test) for peers belonging
to accountID/userID; after the call assert that an error was returned and that
the snapshot equals the post-call peer state so no IPv6 addresses were cleared
or reassigned. Specifically, around the failing UpdateAccountSettings invocation
check the peers' IPv6 fields (the same fields UpdateAccountSettings would touch
for IPv6EnabledGroups) before and after and assert equality to ensure the
operation is side-effect free.
In `@management/server/peer_test.go`:
- Around line 1105-1132: The test builds nbpeer.Peer fixtures with IPv6 (IPv6
fields set) but only asserts legacy IPv4 outputs (Address and AllowedIps[0]);
update the test (in the peer test where peer, networkMap and types.NetworkMap
are constructed) to assert the generated sync/export contains the IPv6 address
and corresponding allowed-ip entries for those peers (check fields derived from
Peer.IPv6 in the code path that produces the sync), and make the assertions
order-tolerant (e.g., verify both IPv4 and IPv6 are present in AllowedIps or
compare as sets) so regressions in IPv6 handling or allowed-ips ordering will
fail the test.
In `@management/server/route_test.go`:
- Around line 1895-1896: The tests currently only call
account.GetPeerRoutesFirewallRules(..., true) so they never exercise the
includeIPv6=false branch; update the three test sites that call
GetPeerRoutesFirewallRules (the ones passing the true flag) to also call the
function with includeIPv6=false and add a negative assertion ensuring IPv6 rules
are suppressed (for example, assert that the ::/0 route for peerE is absent when
includeIPv6 is false). Locate calls to GetPeerRoutesFirewallRules in this file
(and the other two similar spots noted) and add the new call/assertion alongside
the existing true-case to verify the IPv6 gate works.
In `@management/server/store/sql_store_test.go`:
- Around line 2610-2615: The test creates a Peer fixture that sets Peer.IPv6 but
the verification only checks IP; update the persistence test in
sql_store_test.go to assert the IPv6 round-trip by comparing the
stored/retrieved peer's IPv6 to the original value (e.g. after you persist and
load the peer, assert retrievedPeer.IPv6 == peer.IPv6). Ensure you reference the
Peer variable name used in the test (peer) and the retrieved object
(retrievedPeer or whatever the test uses) so a broken serializer/column mapping
for Peer.IPv6 will fail the test.
In `@management/server/types/account_test.go`:
- Around line 1322-1455: Add a new table-driven test case "mixed A/AAAA" that
includes IPv6 addresses in peersToConnect (use the existing IPv6 fields) and
corresponding AAAA entries in customZone. Update the expectedRecords slice for
that case to include both A records (Type int(dns.TypeA)) and AAAA records (Type
int(dns.TypeAAAA)) with RData set to the peers' IPv6 strings; ensure the peers
defined in peersToConnect have matching IPv6 values and that expiredPeers/peer
are handled the same as other cases. Locate and modify the test table entries
using the existing identifiers customZone, peersToConnect, expiredPeers, peer
and expectedRecords so the assertions validate both A and AAAA records for the
mixed case.
In `@management/server/types/networkmap_golden_test.go`:
- Around line 736-750: Add a negative golden/assertion that exercises a
non-IPv6-capable recipient by adding a nbpeer.Peer with a legacy WtVersion (e.g.
"0.25.0") and PeerSystemMeta lacking IPv6 capability, then run the same
rendering/comparison using that peer as the recipient (instead of testingPeerID)
to assert that no IPv6-specific output (IPv6 routes, firewall v6 rules, AAAA
answers) is produced; locate the peer creation (nbpeer.Peer +
nbpeer.PeerSystemMeta) in the loop and add a dedicated legacy peer and a
complementary golden assertion where testingPeerID/allGroupID comparisons are
performed (also add the same negative test around the other occurrence
mentioned).
In `@shared/management/http/api/openapi.yml`:
- Around line 797-800: The OpenAPI schemas for response models are missing the
IPv6 format: update the ipv6 property in the Peer and AccessiblePeer response
schemas to include format: ipv6 (matching PeerRequest.ipv6) so client generators
and validators get the correct hint; locate the ipv6 definitions under the Peer
and AccessiblePeer schema objects in openapi.yml and add format: ipv6 to each
ipv6 property (also apply the same change to the other occurrence noted in the
comment).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19e42dde-da36-41fe-8535-c2d56a97d534
📒 Files selected for processing (52)
.gitignoremanagement/cmd/pprof.gomanagement/internals/modules/reverseproxy/service/manager/l4_port_test.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/internals/modules/reverseproxy/service/manager/manager_test.gomanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/account.gomanagement/server/account/manager.gomanagement/server/account/manager_mock.gomanagement/server/account_test.gomanagement/server/activity/codes.gomanagement/server/group_test.gomanagement/server/http/handlers/accounts/accounts_handler.gomanagement/server/http/handlers/accounts/accounts_handler_test.gomanagement/server/http/handlers/groups/groups_handler_test.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/http/handlers/peers/peers_handler_test.gomanagement/server/http/testing/testing_tools/tools.gomanagement/server/mock_server/account_mock.gomanagement/server/peer.gomanagement/server/peer/peer.gomanagement/server/peer/peer_test.gomanagement/server/peer_test.gomanagement/server/policy_test.gomanagement/server/route_test.gomanagement/server/settings/manager.gomanagement/server/settings/manager_mock.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_get_account_test.gomanagement/server/store/sql_store_test.gomanagement/server/store/sqlstore_bench_test.gomanagement/server/store/store.gomanagement/server/store/store_mock.gomanagement/server/types/account.gomanagement/server/types/account_components.gomanagement/server/types/account_test.gomanagement/server/types/firewall_rule.gomanagement/server/types/firewall_rule_test.gomanagement/server/types/ipv6_groups_test.gomanagement/server/types/network.gomanagement/server/types/network_test.gomanagement/server/types/networkmap_comparison_test.gomanagement/server/types/networkmap_components.gomanagement/server/types/networkmap_golden_test.gomanagement/server/types/networkmapbuilder.gomanagement/server/types/route_firewall_rule.gomanagement/server/types/settings.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/netiputil/compact.goshared/netiputil/compact_test.go
…extraction, effective ranges
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…ship check on UpdatePeerIPv6
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/types/account.go (1)
1387-1391:⚠️ Potential issue | 🟠 MajorDefault-permit route rules still bypass the new IPv6 filtering.
This branch appends
getDefaultPermit(route)directly, but the v6/default entries from that helper are not marked withRouteFirewallRule.IPv6 = true.toProtocolRoutesFirewallRules()now filters only on that flag, so non-v6 peers will still receive these v6 permit rules.🛠️ One way to keep the default-permit path consistent
// If no access control groups are specified, accept all traffic. if len(route.AccessControlGroups) == 0 { defaultPermit := getDefaultPermit(route) + for _, rule := range defaultPermit { + if route.Network.Addr().Is6() || slices.Contains(rule.SourceRanges, "::/0") { + rule.IPv6 = true + } + } routesFirewallRules = append(routesFirewallRules, defaultPermit...) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/account.go` around lines 1387 - 1391, The default-permit branch appends getDefaultPermit(route) without marking v6/default rules as IPv6, so toProtocolRoutesFirewallRules will skip them for non-v6 peers; update the logic so that either getDefaultPermit(route) sets RouteFirewallRule.IPv6 = true for any v6/default entries it returns, or iterate the returned slice in this branch and set r.IPv6 = true for those v6/default entries before appending to routesFirewallRules (referencing getDefaultPermit, route.AccessControlGroups, RouteFirewallRule.IPv6, and toProtocolRoutesFirewallRules).
♻️ Duplicate comments (1)
management/server/peer/peer.go (1)
229-232:⚠️ Potential issue | 🟠 MajorHonor
DisableIPv6inSupportsIPv6().Any capable peer still looks IPv6-enabled after opting out, so callers keep sending v6 routes, rules, and AAAA data.
🛠️ Proposed fix
func (p *Peer) SupportsIPv6() bool { - return p.HasCapability(PeerCapabilityIPv6Overlay) + return !p.Meta.Flags.DisableIPv6 && p.HasCapability(PeerCapabilityIPv6Overlay) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/peer/peer.go` around lines 229 - 232, The SupportsIPv6 method currently only checks capability; update it to also respect the peer opt-out by returning true only when the peer has PeerCapabilityIPv6Overlay AND the peer has not opted out (i.e., !p.DisableIPv6). Locate the SupportsIPv6 function on the Peer type and change its return to combine p.HasCapability(PeerCapabilityIPv6Overlay) with a check of the DisableIPv6 flag (or equivalent field) so callers stop treating opted-out peers as IPv6-enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/internals/shared/grpc/conversion.go`:
- Around line 118-122: Create a single effective IPv6 predicate and use it for
both setting peerConfig.AddressV6 and determining includeIPv6 rather than
checking peer.IPv6.IsValid() and capability separately; compute a boolean like
hasEffectiveIPv6 := peer.IPv6.IsValid() || (capability && network.NetV6.IP !=
nil && network.NetV6.Mask != nil) (or similar logic matching existing capability
check and network.NetV6 presence), then replace the two distinct checks with
this predicate when populating AddressV6
(netip.PrefixFrom(...)/netiputil.EncodePrefix) and when computing
includeIPv6/response filtering so a peer only gets v6 addresses/routes/rules
when the same effective condition holds.
---
Outside diff comments:
In `@management/server/types/account.go`:
- Around line 1387-1391: The default-permit branch appends
getDefaultPermit(route) without marking v6/default rules as IPv6, so
toProtocolRoutesFirewallRules will skip them for non-v6 peers; update the logic
so that either getDefaultPermit(route) sets RouteFirewallRule.IPv6 = true for
any v6/default entries it returns, or iterate the returned slice in this branch
and set r.IPv6 = true for those v6/default entries before appending to
routesFirewallRules (referencing getDefaultPermit, route.AccessControlGroups,
RouteFirewallRule.IPv6, and toProtocolRoutesFirewallRules).
---
Duplicate comments:
In `@management/server/peer/peer.go`:
- Around line 229-232: The SupportsIPv6 method currently only checks capability;
update it to also respect the peer opt-out by returning true only when the peer
has PeerCapabilityIPv6Overlay AND the peer has not opted out (i.e.,
!p.DisableIPv6). Locate the SupportsIPv6 function on the Peer type and change
its return to combine p.HasCapability(PeerCapabilityIPv6Overlay) with a check of
the DisableIPv6 flag (or equivalent field) so callers stop treating opted-out
peers as IPv6-enabled.
🪄 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: 968eb437-0725-4a66-8368-741b019aeb7f
📒 Files selected for processing (9)
management/internals/shared/grpc/conversion.gomanagement/server/mock_server/account_mock.gomanagement/server/peer/peer.gomanagement/server/settings/manager.gomanagement/server/types/account.gomanagement/server/types/networkmap_components.gomanagement/server/types/networkmapbuilder.gomanagement/server/types/route_firewall_rule.goshared/management/http/api/openapi.yml
✅ Files skipped from review due to trivial changes (1)
- management/server/types/networkmapbuilder.go
🚧 Files skipped from review as they are similar to previous changes (4)
- management/server/settings/manager.go
- management/server/mock_server/account_mock.go
- management/server/types/networkmap_components.go
- shared/management/http/api/openapi.yml
…rift, test accuracy - Move IPv6 route filtering and ::/0 duplication from grpc to source (filterAndExpandRoutes in network map builders) - Gate AddressV6 in toPeerConfig on SupportsIPv6() - Remove FirewallRule.IPv6 and RouteFirewallRule.IPv6 fields: IPv6 rules gated at the producer (appendIPv6FirewallRule, generateRouteFirewallRules, getDefaultPermit) so grpc-layer filtering is removed - Pass includeIPv6 to getDefaultPermit to skip v6 default-permit rules - Add SupportsIPv6 check for DisableIPv6 opt-out flag - Add network_net_v6, settings_network_range_v6, settings_ipv6_enabled_groups to pgx account loader - Sync settings.NetworkRangeV6 after random subnet allocation - Fix duplicate IPv6 fields in reverse proxy manager test - Log GetGroupByName error in peerWillHaveIPv6 at debug level - Fix route_test to call correct method for network resource test - Fix peerWithV6 test helper to produce zero-value IPv6 when empty - Add includeIPv6=false test coverage for generateRouteFirewallRules - Reduce appendIPv6FirewallRule params with firewallRuleContext struct - OpenAPI: clarify ipv6_enabled_groups default, PeerRequest.ipv6 wording, add format:ipv6 on Peer response - Add AAAA staleness edge case comment for disable-ipv6 flag toggle - Remove accidentally committed management/cmd/pprof.go
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/types/networkmap_components.go (1)
610-636:⚠️ Potential issue | 🟠 MajorEmit a
::/0permit for cloned default routes.
filterAndExpandRoutes()now duplicates0.0.0.0/0as::/0, andgenerateRouteFirewallRules()already mirrors that for access-controlled routes. This helper still emits the second permit only for dynamic routes, so an ungated exit/default route will ship the IPv6 route without a matching permit rule.Suggested fix
- if includeIPv6 && r.IsDynamic() { + isDefaultV4 := r.Network.Addr().Is4() && r.Network.Bits() == 0 + if includeIPv6 && (r.IsDynamic() || isDefaultV4) { ruleV6 := rule ruleV6.SourceRanges = []string{"::/0"} + if isDefaultV4 { + ruleV6.Destination = "::/0" + ruleV6.RouteID = r.ID + "-v6-default" + } rules = append(rules, &ruleV6) }Please mirror the same fix in
management/server/types/account.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 610 - 636, getDefaultPermit currently only appends the IPv6 (::/0) permit for dynamic routes, but filterAndExpandRoutes duplicates default/exit routes to IPv6 so an ungated/non-dynamic cloned route needs a ::/0 permit too; update getDefaultPermit (and mirror the same change in the equivalent helper in management/server/types/account.go) to, when includeIPv6 is true, always create a separate IPv6 Rule (copy the RouteFirewallRule struct, set SourceRanges = []string{"::/0"}) and append it to rules regardless of r.IsDynamic(), ensuring you create a distinct copy (not a pointer alias) before modifying SourceRanges so you don’t mutate the original rule.
♻️ Duplicate comments (3)
management/server/http/handlers/accounts/accounts_handler.go (1)
194-205:⚠️ Potential issue | 🟡 MinorDon’t swallow
GetEffectiveNetworkRangesfailures.If Line 196 errors, this still returns
200with partially populated settings and makes the account look misconfigured instead of unhealthy.Based on learnings: methods returning
(T, error)should propagate the error immediately rather than continuing with partially-populated state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/accounts/accounts_handler.go` around lines 194 - 205, The current block calling h.settingsManager.GetEffectiveNetworkRanges(...) swallows errors and continues returning 200 with partial settings; change it to check the returned err immediately and propagate it (e.g., return an error response / non-200) instead of proceeding to populate resp.Settings.NetworkRange/NetworkRangeV6 when err != nil; specifically update the code around GetEffectiveNetworkRanges in accounts_handler.go so that when err is non-nil you abort the handler (use the existing handler error-response helper or return the error up) rather than silently ignoring the failure.shared/management/http/api/openapi.yml (1)
1020-1023:⚠️ Potential issue | 🟡 MinorAdd
format: ipv6toAccessiblePeer.ipv6for schema consistency.Line 1020-1023 defines an IPv6 address string but omits
format: ipv6, unlike other IPv6 fields in this file. This can lead to inconsistent validation/codegen behavior.💡 Suggested fix
ipv6: description: Peer's IPv6 overlay address type: string + format: ipv6 example: "fd00:4e42:ab12::1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 1020 - 1023, The AccessiblePeer schema's ipv6 property is missing the format specifier; update the AccessiblePeer definition to add "format: ipv6" to the ipv6 field so it matches other IPv6 fields and ensures consistent validation and codegen behavior (look for the AccessiblePeer.ipv6 property in the OpenAPI YAML and add the format: ipv6 entry beneath type: string).management/server/peer.go (1)
804-811:⚠️ Potential issue | 🟠 MajorPropagate
GetGroupByName("All")failures here.If this lookup fails while
IPv6EnabledGroupsincludes the All group,peerWillHaveIPv6()falls back to false, but the peer is still added to All a few lines later. Registration can then succeed without an IPv6 overlay address.Suggested fix
var allGroupID string if !peer.ProxyMeta.Embedded { - if allGroup, err := am.Store.GetGroupByName(ctx, store.LockingStrengthNone, accountID, "All"); err == nil { - allGroupID = allGroup.ID - } else { - log.WithContext(ctx).Debugf("failed to get All group for IPv6 check: %v", err) - } + allGroup, err := am.Store.GetGroupByName(ctx, store.LockingStrengthNone, accountID, "All") + if err != nil { + return nil, nil, nil, fmt.Errorf("get All group: %w", err) + } + allGroupID = allGroup.ID }Based on learnings, Store methods returning
(T, error)should propagate it and return early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/peer.go` around lines 804 - 811, The GetGroupByName("All") error is being swallowed which can cause inconsistent state; update the code in the peerWillHaveIPv6 (or the surrounding function) to propagate and return the error from am.Store.GetGroupByName(ctx, store.LockingStrengthNone, accountID, "All") instead of only logging it—i.e., if err != nil return "", err (or return the appropriate zero value and err) so callers don't proceed to add the peer to the All group when the lookup failed; keep the existing allGroupID assignment on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/http/handlers/accounts/accounts_handler.go`:
- Around line 93-105: The parseAndValidateNetworkRange function currently only
enforces IPv6 when requireV6 is true, allowing IPv6 CIDRs to be stored in the v4
slot; change it so when requireV6 is false it explicitly rejects IPv6 prefixes
(returning status.InvalidArgument with a clear message), i.e. after masking the
parsed prefix check prefix.Addr().Is6() and if requireV6==false return an error;
keep the existing call to h.validateNetworkRange and error flow intact—this
ensures the network_range (v4) path cannot accept IPv6 values while
network_range_v6 still uses requireV6==true.
In `@management/server/types/firewall_rule.go`:
- Around line 90-103: The duplicated IPv6 default-route ID is inconsistent:
filterAndExpandRoutes() generates a cloned default route as
"<routeID>-v6-default" but this block sets v6Rule.RouteID = route.ID + "-v6",
causing mismatch; add a shared helper (e.g., synthV6RouteID(routeID string)
string or a constant suffix) and use it wherever the synthetic IPv6 default
route ID is created and assigned (both in filterAndExpandRoutes() and here when
setting v6Rule.RouteID inside the includeIPv6 && isDefaultV4 branch), ensuring
generateRulesWithPortRanges/generateRulesWithPorts and RouteID are aligned with
the cloned route ID.
In `@management/server/types/networkmapbuilder.go`:
- Line 730: The incremental route-rule delta is IPv4-only: update
calculateRouteFirewallUpdates to record family-aware changes (include IP family
or CIDR string with /128 vs /32) instead of just newPeer.IP.String(), and modify
updateRouteFirewallRules to iterate and apply updates to every rule variant for
a RouteID (both IPv4 and IPv6 SourceRanges) rather than patching only the first
matching rule. Use the existing getPeerRoutesFirewallRules and RouteID
identifiers to locate affected rules and ensure the delta carries the IP family
(or full CIDR) so additions/removals are applied to the corresponding dual-stack
rules during incremental updates.
---
Outside diff comments:
In `@management/server/types/networkmap_components.go`:
- Around line 610-636: getDefaultPermit currently only appends the IPv6 (::/0)
permit for dynamic routes, but filterAndExpandRoutes duplicates default/exit
routes to IPv6 so an ungated/non-dynamic cloned route needs a ::/0 permit too;
update getDefaultPermit (and mirror the same change in the equivalent helper in
management/server/types/account.go) to, when includeIPv6 is true, always create
a separate IPv6 Rule (copy the RouteFirewallRule struct, set SourceRanges =
[]string{"::/0"}) and append it to rules regardless of r.IsDynamic(), ensuring
you create a distinct copy (not a pointer alias) before modifying SourceRanges
so you don’t mutate the original rule.
---
Duplicate comments:
In `@management/server/http/handlers/accounts/accounts_handler.go`:
- Around line 194-205: The current block calling
h.settingsManager.GetEffectiveNetworkRanges(...) swallows errors and continues
returning 200 with partial settings; change it to check the returned err
immediately and propagate it (e.g., return an error response / non-200) instead
of proceeding to populate resp.Settings.NetworkRange/NetworkRangeV6 when err !=
nil; specifically update the code around GetEffectiveNetworkRanges in
accounts_handler.go so that when err is non-nil you abort the handler (use the
existing handler error-response helper or return the error up) rather than
silently ignoring the failure.
In `@management/server/peer.go`:
- Around line 804-811: The GetGroupByName("All") error is being swallowed which
can cause inconsistent state; update the code in the peerWillHaveIPv6 (or the
surrounding function) to propagate and return the error from
am.Store.GetGroupByName(ctx, store.LockingStrengthNone, accountID, "All")
instead of only logging it—i.e., if err != nil return "", err (or return the
appropriate zero value and err) so callers don't proceed to add the peer to the
All group when the lookup failed; keep the existing allGroupID assignment on
success.
In `@shared/management/http/api/openapi.yml`:
- Around line 1020-1023: The AccessiblePeer schema's ipv6 property is missing
the format specifier; update the AccessiblePeer definition to add "format: ipv6"
to the ipv6 field so it matches other IPv6 fields and ensures consistent
validation and codegen behavior (look for the AccessiblePeer.ipv6 property in
the OpenAPI YAML and add the format: ipv6 entry beneath type: string).
🪄 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: e9dafdd1-2ebb-4b68-8163-abe3177074d9
📒 Files selected for processing (17)
management/internals/modules/reverseproxy/service/manager/manager_test.gomanagement/internals/shared/grpc/conversion.gomanagement/server/account.gomanagement/server/http/handlers/accounts/accounts_handler.gomanagement/server/peer.gomanagement/server/peer/peer.gomanagement/server/route_test.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store_test.gomanagement/server/types/account.gomanagement/server/types/firewall_rule.gomanagement/server/types/firewall_rule_test.gomanagement/server/types/ipv6_groups_test.gomanagement/server/types/networkmap_components.gomanagement/server/types/networkmapbuilder.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
✅ Files skipped from review due to trivial changes (1)
- management/server/types/ipv6_groups_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- management/server/types/firewall_rule_test.go
- management/server/route_test.go
- management/server/account.go
- management/internals/shared/grpc/conversion.go
- management/server/store/sql_store.go
| b.updateACGIndexForPeer(peerID, allRoutes) | ||
|
|
||
| routeFirewallRules := b.getPeerRoutesFirewallRules(account, peerID, b.validatedPeers) | ||
| routeFirewallRules := b.getPeerRoutesFirewallRules(account, peerID, b.validatedPeers, peer.SupportsIPv6() && peer.IPv6.IsValid()) |
Contributor
There was a problem hiding this comment.
The incremental route-rule cache is still IPv4-only.
These changes build IPv6 SourceRanges on full view generation, but calculateRouteFirewallUpdates still enqueues only newPeer.IP.String(), and updateRouteFirewallRules patches only the first rule matching a RouteID. Existing route owners will therefore miss /128 additions/removals for the new dual-stack rules until a full rebuild.
Extend the delta/update path to carry family-aware changes and apply them to every rule variant for the route.
Also applies to: 833-914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/types/networkmapbuilder.go` at line 730, The incremental
route-rule delta is IPv4-only: update calculateRouteFirewallUpdates to record
family-aware changes (include IP family or CIDR string with /128 vs /32) instead
of just newPeer.IP.String(), and modify updateRouteFirewallRules to iterate and
apply updates to every rule variant for a RouteID (both IPv4 and IPv6
SourceRanges) rather than patching only the first matching rule. Use the
existing getPeerRoutesFirewallRules and RouteID identifiers to locate affected
rules and ensure the delta carries the IP family (or full CIDR) so
additions/removals are applied to the corresponding dual-stack rules during
incremental updates.
Collaborator
Author
There was a problem hiding this comment.
Good catch. Both call sites in calculateRouteFirewallUpdates now also add the peer's IPv6 address when newPeer.IPv6.IsValid().
Contributor
There was a problem hiding this comment.
Rate Limit Exceeded
@lixmal have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 41 seconds before sending another message.
Collaborator
Author
There was a problem hiding this comment.
Actually, reverted this. The incremental path doesn't know whether the routing peer (info.PeerID) supports IPv6, so blindly adding the v6 source would diverge from the full rebuild. Left a TODO: needs to check the routing peer's capability before adding the v6 source range.
Contributor
There was a problem hiding this comment.
@lixmal, that's a sound call — the incremental path has no visibility into the routing peer's capabilities, so silently injecting a /128 source range that the routing peer can't enforce (and wouldn't appear in the full rebuild either) would create a correctness divergence that's harder to debug than the conservative omission.
The trade-off is clear: until the incremental path gains access to the routing peer's SupportsIPv6() state, the full rebuild already acts as the correctness backstop. The TODO is a good marker for a follow-up.
One thing worth keeping in mind for when the TODO is eventually resolved: the capability check will need to be done against b.cache.globalPeers[info.PeerID] (the routing peer), not the new peer — since the question is whether the route owner can handle IPv6 source ranges, not whether the joining peer has IPv6.
✏️ Learnings added
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5698
File: management/server/types/networkmapbuilder.go:730-730
Timestamp: 2026-03-26T09:11:58.696Z
Learning: In `management/server/types/networkmapbuilder.go` (netbirdio/netbird), `calculateRouteFirewallUpdates` intentionally does NOT add `newPeer.IPv6` as a source range in the incremental delta path. The reason is that the incremental path does not know whether the routing peer (`info.PeerID`) supports IPv6 (i.e., `SupportsIPv6() && IPv6.IsValid()`), so blindly adding a v6 source would diverge from the full rebuild behavior in `buildPeerRoutesView`/`getPeerRoutesFirewallRules`. A TODO is left in the code. When eventually fixed, the capability check must be against `b.cache.globalPeers[info.PeerID]` (the routing peer), not the new peer. Do not flag the absence of IPv6 incremental route-firewall-rule updates as a regression; it is a deliberate correctness-over-completeness trade-off.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5675
File: client/internal/peer/status.go:55-56
Timestamp: 2026-03-24T11:38:57.885Z
Learning: In netbirdio/netbird (client/internal/peer/status.go), `PeerByIP()` intentionally checks only `state.IP` (IPv4). Its only caller is `forwardingrules.go`, which resolves IPv4 translated addresses exclusively. Extending `PeerByIP()` to also match `state.IPv6` is deferred until forwarding rules gain IPv6 support. Do not flag the IPv4-only check in `PeerByIP()` as a bug in the context of this file.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5698
File: management/internals/shared/grpc/conversion.go:0-0
Timestamp: 2026-03-26T09:09:51.782Z
Learning: In netbirdio/netbird (management/internals/shared/grpc/conversion.go), the two IPv6 guards in `toPeerConfig` and `ToSyncResponse` serve different purposes and must NOT be unified. `toPeerConfig` uses `peer.IPv6.IsValid() && network.NetV6.IP != nil` because it constructs a prefix from the network mask and needs to guard against a nil mask. `ToSyncResponse` uses `peer.SupportsIPv6() && peer.IPv6.IsValid()` for `includeIPv6` and does not need the network object. Additionally, `peer.IPv6.IsValid() == true` is already proof that the peer is in an `IPv6EnabledGroups` group — IPv6 is only allocated to peers in those groups — so no additional group-membership gate is needed at the gRPC conversion layer. Do not flag these two distinct checks as a unification opportunity.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/filter.go:1281-1292
Timestamp: 2026-03-25T09:21:25.418Z
Learning: In netbirdio/netbird (`client/firewall/uspfilter/filter.go`), IPv4 and IPv6 fragments are intentionally allowed through (fast-path `return false` in `filterInbound`) without going through `peerACLsBlock`, `routeACLsPass`, or the forwarder. This is pre-existing behaviour tracked by the TODO comment "pass fragments of routed packets to forwarder". Do not flag the IPv6 fragment fast-path (`d.ip6.NextHeader == layers.IPProtocolIPv6Fragment` returning `(true, true)`) as a security regression; it mirrors the existing IPv4 fragment handling by design.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5698
File: management/server/settings/manager.go:131-144
Timestamp: 2026-03-26T09:09:54.227Z
Learning: In netbirdio/netbird (`management/server/settings/manager.go`), `GetEffectiveNetworkRanges` converts `net.IPNet` (from `network.Net` and `network.NetV6`) to `netip.Prefix` using `netip.AddrFromSlice(...) + if ok` guard rather than `netip.ParsePrefix`. Silent zero-value fallback on conversion failure is intentional: the `net.IPNet` values are always written by controlled internal functions (`AllocateIPv6Subnet`, `reallocateAccountPeerIPs`), so a non-nil IP implies a valid conversion. Do not flag the absence of error returns for this path.
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5687
File: client/internal/engine_ssh.go:44-50
Timestamp: 2026-03-25T06:24:52.840Z
Learning: In `client/internal/engine_ssh.go` (netbirdio/netbird), the IPv6 SSH setup steps — `AddListener` and `AddInboundDNAT` for the IPv6 overlay address — are intentionally best-effort and independent of each other, mirroring the v4 behavior. IPv6 soft-failure is by design so that IPv4 SSH continues to work when IPv6 cannot be set up. Do not flag the lack of atomicity between the IPv6 listener and IPv6 DNAT as a bug.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5675
File: client/iface/device/wg_link_freebsd.go:61-84
Timestamp: 2026-03-24T11:39:03.789Z
Learning: In `client/iface/device/wg_link_freebsd.go` (and related platform files), `UpdateAddr` is never called mid-lifetime by the engine. IPv6 overlay address changes are detected in `engine.go` via `hasIPv6Changed` and trigger `ErrResetConnection`, which causes a full client restart and interface recreation from scratch. Therefore, `assignAddr` always runs on a fresh interface, and there is no need to clean up pre-existing OS inet6 aliases in the soft-fail (ClearIPv6) path.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/nat.go:294-305
Timestamp: 2026-03-25T09:21:29.721Z
Learning: In netbirdio/netbird (client/firewall/uspfilter/nat.go and related files), `extractPacketIPs` and similar functions that switch on `d.decoded[0]` (a gopacket layer type set by the DecodingLayerParser) do not need explicit packet-length bounds checks before indexing at fixed IP header offsets. The gopacket decoder validates minimum header size before setting the layer type, so `LayerTypeIPv4` implies ≥20 bytes and `LayerTypeIPv6` implies ≥40 bytes. Do not flag the absence of redundant bounds checks in these switch-on-decoded-layer patterns as a bug.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5698
File: management/server/peer.go:805-811
Timestamp: 2026-03-26T09:09:36.634Z
Learning: In `management/server/peer.go` (netbirdio/netbird), the `GetGroupByName("All")` call inside `AddPeer`'s IPv6 allocation block (within `if len(settings.IPv6EnabledGroups) > 0 && network.NetV6.IP != nil`) intentionally swallows errors. The "All" group is built-in and always exists, so a failure indicates a serious DB issue. The design choice is to soft-fail: let the peer register without an IPv6 overlay address (it will receive one on the next sync) rather than blocking registration entirely over an optional feature. Do not flag this error-swallowing as a bug.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5668
File: client/firewall/create_linux.go:45-59
Timestamp: 2026-03-23T19:24:39.071Z
Learning: In netbirdio/netbird (client/firewall/create_linux.go), `AllowNetbird()` on the native firewall (nftables/iptables) adds blanket accept rules. These rules should ONLY be called when USPFilter wraps the native firewall as a backend (in-process filtering): USPFilter does per-peer ACL enforcement and needs the native firewall to pass all traffic through. When the native firewall is used directly (returned without USPFilter wrapping), `AllowNetbird()` must NOT be called, because the native firewall manages its own ACL rules and blanket accepts would bypass them. Do not flag the absence of `AllowNetbird()` on the native-only return path as a bug.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/forwarder/icmp.go:268-287
Timestamp: 2026-03-25T09:21:53.814Z
Learning: When using gVisor’s `header.ICMPv6Checksum(ICMPv6ChecksumParams)` (`gvisor.dev/gvisor/pkg/tcpip/header`), the pseudo-header checksum is computed internally from `Src`, `Dst`, and `Header`. Use `PayloadCsum`/`PayloadLen` only for scattered-buffer cases where the ICMPv6 payload bytes are not included in `Header`. If `Header` already contains the complete ICMPv6 message (ICMPv6 header + all data), call `ICMPv6Checksum` with `PayloadCsum: 0` and `PayloadLen: 0` (or omit those fields). Do not pass a separately computed pseudo-header checksum (e.g., `header.PseudoHeaderChecksum(...)`) as `PayloadCsum` in that case, because it will be double-counted and produce an incorrect checksum.
Learnt from: mlsmaycon
Repo: netbirdio/netbird PR: 5157
File: management/server/http/handlers/users/invites_handler.go:197-207
Timestamp: 2026-01-25T22:51:51.874Z
Learning: Ensure password validation in Go code enforces the policy used by AcceptUserInvite (minimum 8 characters, at least one digit, one uppercase letter, and one special character) via validatePassword or a centralized password policy utility. Apply this check to all handlers under management/server/http/handlers/users (and similar invite/auth flows) to maintain consistent strength requirements across invites and user creation.
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 5441
File: management/server/user.go:745-749
Timestamp: 2026-02-24T19:32:13.189Z
Learning: In Go codebases like netbirdio/netbird, methods returning (T, error) should follow the convention: if an error is returned, propagate it and return early. When the caller handles the error immediately, explicit nil checks on the returned value are unnecessary; rely on the error to guard control flow and only inspect the value when err is nil. This guidance applies broadly to Store methods such as GetUserByUserID in Go code; prefer early return on error and avoid redundant nil checks after a successful error path.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5530
File: proxy/internal/udp/relay.go:276-278
Timestamp: 2026-03-07T13:59:16.506Z
Learning: For Go 1.25+ projects, use sync.WaitGroup.Go() to run goroutines because it handles Add(1) and defer Done() automatically. Do not flag wg.Go(...) as a compilation error in codebases that require Go 1.25 or newer. If supporting older Go versions, gate such usage with build tags or avoid wg.Go() to maintain compatibility; verify the module targets Go 1.25+ before adopting this pattern.
lixmal
force-pushed
the
mgmt-ipv6-addressing
branch
from
5b51583 to
feb128b
Compare
lixmal
force-pushed
the
mgmt-ipv6-addressing
branch
from
1ff2e9b to
b116739
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)
management/server/types/networkmapbuilder.go (1)
730-735:⚠️ Potential issue | 🟠 MajorClone the route-firewall rules when cloning the default route.
filterAndExpandRoutes()adds a synthetic-v6-defaultroute only inassembleNetworkMap, butRouteFirewallRuleIDswere already cached from the unexpanded route list. BecausegetDefaultPermit()emits a rule only for the family of the original route, a v6-capable peer can receive::/0without any matchingRouteFirewallRule. Generate the expanded route set before caching route ACLs, or clone the matching rule alongside the cloned route.Also applies to: 833-850, 1110-1120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmapbuilder.go` around lines 730 - 735, The bug is that route ACLs are cached from the unexpanded route list so a synthesized IPv6 default route (added by filterAndExpandRoutes()/assembleNetworkMap) gets no matching RouteFirewallRule; update the logic in the block that calls getPeerRoutesFirewallRules / generateRouteFirewallRuleID / b.cache.globalRouteRules so that you first call filterAndExpandRoutes (or otherwise produce the expanded route set) and derive firewall rules from that expanded set, or when cloning a default route (the synthetic -v6-default) also clone its matching rule into b.cache.globalRouteRules and append its generated ID to view.RouteFirewallRuleIDs; apply the same change to the analogous blocks around getPeerRoutesFirewallRules usage at the other reported locations (the blocks using generateRouteFirewallRuleID and b.cache.globalRouteRules).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/account.go`:
- Around line 2557-2603: updatePeerIPv6InTransaction currently only checks range
membership and collisions but allows addresses the allocator deliberately
filters (e.g., subnet base/forbidden host patterns); before calling
transaction.SavePeer in updatePeerIPv6InTransaction, run the same host-part
validation used by AllocateRandomPeerIPv6() (or extract that logic into a shared
helper and call it from both AllocateRandomPeerIPv6 and
updatePeerIPv6InTransaction) and return an appropriate status error if the
newIPv6 fails that host-part check; keep the existing checks
(buildIPv6AllowedPeers, checkIPv6Collision) and place this validation right
after those and before peer.IPv6 assignment/SavePeer.
In `@management/server/types/network.go`:
- Around line 257-297: AllocateRandomPeerIPv6 can escape the prefix for
/121–/128 because setting ip[15]=1 overwrites preserved network bits; change the
fallback so it only flips a host bit inside the host portion (compute hostStart
= ones/8 and partialBits = ones%8, build a hostMask for the byte containing host
bits and set the lowest-order host bit within that mask instead of writing the
whole last byte), and treat /128 as invalid (return an error) since there are no
host bits; update the fallback logic in AllocateRandomPeerIPv6 to apply the
mask-aware modification so the result always stays inside the provided prefix.
---
Outside diff comments:
In `@management/server/types/networkmapbuilder.go`:
- Around line 730-735: The bug is that route ACLs are cached from the unexpanded
route list so a synthesized IPv6 default route (added by
filterAndExpandRoutes()/assembleNetworkMap) gets no matching RouteFirewallRule;
update the logic in the block that calls getPeerRoutesFirewallRules /
generateRouteFirewallRuleID / b.cache.globalRouteRules so that you first call
filterAndExpandRoutes (or otherwise produce the expanded route set) and derive
firewall rules from that expanded set, or when cloning a default route (the
synthetic -v6-default) also clone its matching rule into
b.cache.globalRouteRules and append its generated ID to
view.RouteFirewallRuleIDs; apply the same change to the analogous blocks around
getPeerRoutesFirewallRules usage at the other reported locations (the blocks
using generateRouteFirewallRuleID and b.cache.globalRouteRules).
🪄 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: 2f6386b9-435e-4cfb-a449-5e7f8c75f6d1
📒 Files selected for processing (10)
management/server/account.gomanagement/server/account_test.gomanagement/server/http/handlers/accounts/accounts_handler.gomanagement/server/peer.gomanagement/server/peer/peer.gomanagement/server/route_test.gomanagement/server/types/firewall_rule.gomanagement/server/types/network.gomanagement/server/types/network_test.gomanagement/server/types/networkmapbuilder.go
✅ Files skipped from review due to trivial changes (1)
- management/server/peer/peer.go
🚧 Files skipped from review as they are similar to previous changes (5)
- management/server/types/firewall_rule.go
- management/server/peer.go
- management/server/http/handlers/accounts/accounts_handler.go
- management/server/account_test.go
- management/server/route_test.go
Comment on lines
+2557
to
+2603
| func (am *DefaultAccountManager) updatePeerIPv6InTransaction(ctx context.Context, transaction store.Store, accountID, peerID string, newIPv6 netip.Addr) (bool, error) { | ||
| network, err := transaction.GetAccountNetwork(ctx, store.LockingStrengthShare, accountID) | ||
| if err != nil { | ||
| return false, fmt.Errorf("get network: %w", err) | ||
| } | ||
|
|
||
| if network.NetV6.IP == nil { | ||
| return false, status.Errorf(status.PreconditionFailed, "IPv6 is not configured for this account") | ||
| } | ||
|
|
||
| if !network.NetV6.Contains(newIPv6.AsSlice()) { | ||
| return false, status.Errorf(status.InvalidArgument, "IP %s is not within the account IPv6 range %s", newIPv6, network.NetV6.String()) | ||
| } | ||
|
|
||
| settings, err := transaction.GetAccountSettings(ctx, store.LockingStrengthShare, accountID) | ||
| if err != nil { | ||
| return false, fmt.Errorf("get settings: %w", err) | ||
| } | ||
|
|
||
| peer, err := transaction.GetPeerByID(ctx, store.LockingStrengthUpdate, accountID, peerID) | ||
| if err != nil { | ||
| return false, fmt.Errorf("get peer: %w", err) | ||
| } | ||
|
|
||
| allowedPeers, err := am.buildIPv6AllowedPeers(ctx, transaction, accountID, settings, []*nbpeer.Peer{peer}) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if _, ok := allowedPeers[peerID]; !ok { | ||
| return false, status.Errorf(status.PreconditionFailed, "peer is not in any IPv6-enabled group") | ||
| } | ||
|
|
||
| if peer.IPv6.IsValid() && peer.IPv6 == newIPv6 { | ||
| return false, nil | ||
| } | ||
|
|
||
| if err := am.checkIPv6Collision(ctx, transaction, accountID, peerID, newIPv6); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| peer.IPv6 = newIPv6 | ||
| if err := transaction.SavePeer(ctx, accountID, peer); err != nil { | ||
| return false, fmt.Errorf("save peer: %w", err) | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Manual IPv6 edits bypass the allocator’s reserved-address rules.
This path only checks range membership and collisions. It still accepts the subnet base address, and more generally any host pattern that AllocateRandomPeerIPv6() deliberately filters out, so the admin API can persist peer IPv6 values that the automatic allocator never emits. Reuse the same host-part validation here before SavePeer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/account.go` around lines 2557 - 2603,
updatePeerIPv6InTransaction currently only checks range membership and
collisions but allows addresses the allocator deliberately filters (e.g., subnet
base/forbidden host patterns); before calling transaction.SavePeer in
updatePeerIPv6InTransaction, run the same host-part validation used by
AllocateRandomPeerIPv6() (or extract that logic into a shared helper and call it
from both AllocateRandomPeerIPv6 and updatePeerIPv6InTransaction) and return an
appropriate status error if the newIPv6 fails that host-part check; keep the
existing checks (buildIPv6AllowedPeers, checkIPv6Collision) and place this
validation right after those and before peer.IPv6 assignment/SavePeer.
management/server/types/network.go
Outdated
Comment on lines
+257
to
+297
| func AllocateRandomPeerIPv6(prefix netip.Prefix) (netip.Addr, error) { | ||
| ones := prefix.Bits() | ||
| if ones == 0 || !prefix.Addr().Is6() { | ||
| return netip.Addr{}, fmt.Errorf("invalid IPv6 subnet: %s", prefix.String()) | ||
| } | ||
| return binary.BigEndian.Uint32(ip) | ||
|
|
||
| ip := prefix.Addr().As16() | ||
|
|
||
| rng := rand.New(rand.NewSource(time.Now().UnixNano())) | ||
|
|
||
| // Determine which byte the host bits start in | ||
| firstHostByte := ones / 8 | ||
| // If the prefix doesn't end on a byte boundary, handle the partial byte | ||
| partialBits := ones % 8 | ||
|
|
||
| if partialBits > 0 { | ||
| // Keep the network bits in the partial byte, randomize the rest | ||
| hostMask := byte(0xff >> partialBits) | ||
| ip[firstHostByte] = (ip[firstHostByte] & ^hostMask) | (byte(rng.Intn(256)) & hostMask) | ||
| firstHostByte++ | ||
| } | ||
|
|
||
| // Randomize remaining full host bytes | ||
| for i := firstHostByte; i < 16; i++ { | ||
| ip[i] = byte(rng.Intn(256)) | ||
| } | ||
|
|
||
| // Avoid all-zeros and all-ones host parts. | ||
| // Mask off network bits in the partial byte so only host bits are checked. | ||
| hostStart := ones / 8 | ||
| hostSlice := slices.Clone(ip[hostStart:]) | ||
| if partialBits > 0 { | ||
| hostSlice[0] &= 0xff >> partialBits | ||
| } | ||
| allZero := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0 }) | ||
| allOnes := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0xff }) | ||
| if allZero || allOnes { | ||
| ip[15] = 1 | ||
| } | ||
|
|
||
| return netip.AddrFrom16(ip).Unmap(), nil |
Contributor
There was a problem hiding this comment.
The IPv6 fallback can escape the prefix for /121-/128.
When allZero || allOnes, ip[15] = 1 rewrites the entire last byte. For /121-/127, that byte still contains preserved network bits, and for /128 there are no host bits at all, so this can return an address outside prefix.
💡 Suggested fix
func AllocateRandomPeerIPv6(prefix netip.Prefix) (netip.Addr, error) {
ones := prefix.Bits()
- if ones == 0 || !prefix.Addr().Is6() {
+ if ones == 0 || !prefix.Addr().Is6() || ones > 126 {
return netip.Addr{}, fmt.Errorf("invalid IPv6 subnet: %s", prefix.String())
}
@@
allZero := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0 })
allOnes := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0xff })
if allZero || allOnes {
- ip[15] = 1
+ ip = prefix.Masked().Addr().As16()
+ ip[15] |= 0x01
}
return netip.AddrFrom16(ip).Unmap(), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func AllocateRandomPeerIPv6(prefix netip.Prefix) (netip.Addr, error) { | |
| ones := prefix.Bits() | |
| if ones == 0 || !prefix.Addr().Is6() { | |
| return netip.Addr{}, fmt.Errorf("invalid IPv6 subnet: %s", prefix.String()) | |
| } | |
| return binary.BigEndian.Uint32(ip) | |
| ip := prefix.Addr().As16() | |
| rng := rand.New(rand.NewSource(time.Now().UnixNano())) | |
| // Determine which byte the host bits start in | |
| firstHostByte := ones / 8 | |
| // If the prefix doesn't end on a byte boundary, handle the partial byte | |
| partialBits := ones % 8 | |
| if partialBits > 0 { | |
| // Keep the network bits in the partial byte, randomize the rest | |
| hostMask := byte(0xff >> partialBits) | |
| ip[firstHostByte] = (ip[firstHostByte] & ^hostMask) | (byte(rng.Intn(256)) & hostMask) | |
| firstHostByte++ | |
| } | |
| // Randomize remaining full host bytes | |
| for i := firstHostByte; i < 16; i++ { | |
| ip[i] = byte(rng.Intn(256)) | |
| } | |
| // Avoid all-zeros and all-ones host parts. | |
| // Mask off network bits in the partial byte so only host bits are checked. | |
| hostStart := ones / 8 | |
| hostSlice := slices.Clone(ip[hostStart:]) | |
| if partialBits > 0 { | |
| hostSlice[0] &= 0xff >> partialBits | |
| } | |
| allZero := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0 }) | |
| allOnes := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0xff }) | |
| if allZero || allOnes { | |
| ip[15] = 1 | |
| } | |
| return netip.AddrFrom16(ip).Unmap(), nil | |
| func AllocateRandomPeerIPv6(prefix netip.Prefix) (netip.Addr, error) { | |
| ones := prefix.Bits() | |
| if ones == 0 || !prefix.Addr().Is6() || ones > 126 { | |
| return netip.Addr{}, fmt.Errorf("invalid IPv6 subnet: %s", prefix.String()) | |
| } | |
| ip := prefix.Addr().As16() | |
| rng := rand.New(rand.NewSource(time.Now().UnixNano())) | |
| // Determine which byte the host bits start in | |
| firstHostByte := ones / 8 | |
| // If the prefix doesn't end on a byte boundary, handle the partial byte | |
| partialBits := ones % 8 | |
| if partialBits > 0 { | |
| // Keep the network bits in the partial byte, randomize the rest | |
| hostMask := byte(0xff >> partialBits) | |
| ip[firstHostByte] = (ip[firstHostByte] & ^hostMask) | (byte(rng.Intn(256)) & hostMask) | |
| firstHostByte++ | |
| } | |
| // Randomize remaining full host bytes | |
| for i := firstHostByte; i < 16; i++ { | |
| ip[i] = byte(rng.Intn(256)) | |
| } | |
| // Avoid all-zeros and all-ones host parts. | |
| // Mask off network bits in the partial byte so only host bits are checked. | |
| hostStart := ones / 8 | |
| hostSlice := slices.Clone(ip[hostStart:]) | |
| if partialBits > 0 { | |
| hostSlice[0] &= 0xff >> partialBits | |
| } | |
| allZero := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0 }) | |
| allOnes := !slices.ContainsFunc(hostSlice, func(v byte) bool { return v != 0xff }) | |
| if allZero || allOnes { | |
| ip = prefix.Masked().Addr().As16() | |
| ip[15] |= 0x01 | |
| } | |
| return netip.AddrFrom16(ip).Unmap(), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/types/network.go` around lines 257 - 297,
AllocateRandomPeerIPv6 can escape the prefix for /121–/128 because setting
ip[15]=1 overwrites preserved network bits; change the fallback so it only flips
a host bit inside the host portion (compute hostStart = ones/8 and partialBits =
ones%8, build a hostMask for the byte containing host bits and set the
lowest-order host bit within that mask instead of writing the whole last byte),
and treat /128 as invalid (return an error) since there are no host bits; update
the fallback logic in AllocateRandomPeerIPv6 to apply the mask-aware
modification so the result always stays inside the provided prefix.
|
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
addrFamilyabstraction encapsulating IPv4/IPv6 header offsets, address lengths, set key types, and ICMP protocol numbersip6 netbirdtable with its own router and ACL manager when the interface has IPv6UpdateSetprefixes by family for dynamic DNS route setsTestNftablesCreateIpSet_IPv6) andcalculateLastIPtests covering both familiesStacked on #5706.
Issue 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:
netbirdio/docs#667
Summary by CodeRabbit
New Features
API / Docs
Tests
Chores