[management] Affected peers for user updates - #7099
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughJWT group and user updates now calculate affected peers from group membership, allowed-user, policy, router, resource, and peer snapshots. Updates target impacted peers asynchronously. IPv6 reconciliation still triggers account-wide propagation. ChangesAffected-peer update flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UserUpdate
participant AffectedPeerResolver
participant PeerUpdateDispatcher
participant Peer
UserUpdate->>AffectedPeerResolver: Resolve changed groups and allowed users
AffectedPeerResolver-->>UserUpdate: Return affected peers
UserUpdate->>PeerUpdateDispatcher: Dispatch targeted update
PeerUpdateDispatcher->>Peer: Send network map update
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/account.go (1)
2445-2456: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTwo settings reads can disagree about IPv6 reconciliation.
SyncUserJWTGroupscallsipv6ReconcileNeededwith thesettingsloaded before the transaction (Line 1701).reconcileIPv6ForGroupChangesre-reads settings inside the transaction and evaluates the same predicate again (Lines 2446-2453). IfIPv6EnabledGroupschanges between the two reads, the reconciliation runs whilerequiresAccountUpdatestaysfalse, so the account-wide update is skipped and peers keep stale IPv6 addresses. Return the decision fromreconcileIPv6ForGroupChanges, or pass the transaction-scoped settings to both call sites.🔧 Proposed direction
-func (am *DefaultAccountManager) reconcileIPv6ForGroupChanges(ctx context.Context, transaction store.Store, accountID string, groupIDs []string) error { +// reconcileIPv6ForGroupChanges reports whether it reassigned addresses, so callers that +// compute an affected-peers set can fall back to an account-wide update. +func (am *DefaultAccountManager) reconcileIPv6ForGroupChanges(ctx context.Context, transaction store.Store, accountID string, groupIDs []string) (bool, error) { settings, err := transaction.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) if err != nil { - return fmt.Errorf("get account settings: %w", err) + return false, fmt.Errorf("get account settings: %w", err) } if !ipv6ReconcileNeeded(settings, groupIDs) { - return nil + return false, nil } - return am.updatePeerIPv6Addresses(ctx, transaction, accountID, settings) + return true, am.updatePeerIPv6Addresses(ctx, transaction, accountID, settings) }Then set
requiresAccountUpdatefrom that return value inSyncUserJWTGroupsand inprocessUserUpdate, and drop the separateipv6ReconcileNeededcall at both sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/account.go` around lines 2445 - 2456, Make the transaction-scoped IPv6 reconciliation decision authoritative: update reconcileIPv6ForGroupChanges to return whether ipv6ReconcileNeeded was true, then use that result to set requiresAccountUpdate in SyncUserJWTGroups and processUserUpdate. Remove the separate ipv6ReconcileNeeded calls at both callers while preserving the existing account update and peer-address update behavior.
🧹 Nitpick comments (2)
management/server/user.go (2)
887-896: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
allGroupIDsto state that it returns the All group.The plural name reads as "the IDs of all groups". The function returns only the account's All group ID. Rename it to
allGroupIDorgroupAllIDsto prevent a future caller from treating the result as every group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/user.go` around lines 887 - 896, Rename the allGroupIDs function to singularly indicate that it returns the account’s All group ID, such as allGroupID, and update every call site and related references consistently while preserving its slice return type and behavior.
646-649: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnwrapped
affectedpeers.Loaderrors in both transaction callbacks. Both new snapshot loads return the store error without context, while every other error in the same callback is wrapped. A failure therefore reaches the caller log without identifying the failing step.
management/server/user.go#L646-L649: wrap the error, for examplefmt.Errorf("load affected peers for user %s: %w", update.Id, err).management/server/account.go#L1712-L1715: wrap the error, for examplefmt.Errorf("load affected peers: %w", err).As per coding guidelines: "add concise error context without prefixes such as
failed toorerror".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/user.go` around lines 646 - 649, Wrap the errors returned by affectedpeers.Load in both transaction callbacks with concise context using %w, without prefixes such as “failed to” or “error”. Update management/server/user.go:646-649 in the user update callback to identify the affected user, and management/server/account.go:1712-1715 in the account callback to identify the affected-peers loading step.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@management/server/affectedpeers/resolver.go`:
- Around line 878-883: The ruleShipsAllowedUsers logic must classify NetBird SSH
rules with AuthorizedUser as allowed-user rules so destination peers refresh
when AllowedUsersChanged is true. Update the NetBird SSH branch to match
AuthorizedUser rules appropriately, while preserving the existing empty
AuthorizedGroups and empty AuthorizedUser condition for rules without a named
user.
In `@management/server/user.go`:
- Line 849: Update SaveOrAddUsers to handle a nil settings result from
GetAccountSettings before evaluating settings.GroupsPropagationEnabled. Preserve
existing store-error handling, and either guard the dereference or apply the
established validation defaults for missing account settings.
---
Outside diff comments:
In `@management/server/account.go`:
- Around line 2445-2456: Make the transaction-scoped IPv6 reconciliation
decision authoritative: update reconcileIPv6ForGroupChanges to return whether
ipv6ReconcileNeeded was true, then use that result to set requiresAccountUpdate
in SyncUserJWTGroups and processUserUpdate. Remove the separate
ipv6ReconcileNeeded calls at both callers while preserving the existing account
update and peer-address update behavior.
---
Nitpick comments:
In `@management/server/user.go`:
- Around line 887-896: Rename the allGroupIDs function to singularly indicate
that it returns the account’s All group ID, such as allGroupID, and update every
call site and related references consistently while preserving its slice return
type and behavior.
- Around line 646-649: Wrap the errors returned by affectedpeers.Load in both
transaction callbacks with concise context using %w, without prefixes such as
“failed to” or “error”. Update management/server/user.go:646-649 in the user
update callback to identify the affected user, and
management/server/account.go:1712-1715 in the account callback to identify the
affected-peers loading step.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 554f5863-67d3-401c-aab3-1f63ebf961cc
📒 Files selected for processing (5)
management/server/account.gomanagement/server/affected_peers_jwt_test.gomanagement/server/affectedpeers/resolver.gomanagement/server/affectedpeers/resolver_test.gomanagement/server/user.go
| func ruleShipsAllowedUsers(rule *types.PolicyRule) bool { | ||
| if rule.Protocol == types.PolicyRuleProtocolNetbirdSSH { | ||
| return len(rule.AuthorizedGroups) == 0 && rule.AuthorizedUser == "" | ||
| } | ||
| return types.PolicyRuleImpliesLegacySSH(rule) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how AuthorizedUser is applied in the network map and whether blocked users are filtered.
set -euo pipefail
rg -n -C 8 'AuthorizedUser\b' --type=go -g '!**/*_test.go'
rg -n -C 6 'IsBlocked\(\)' --type=go -g '**/networkmap/**' -g '**/network_map/**' -g '!**/*_test.go'Repository: netbirdio/netbird
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files around resolver:"
git ls-files | rg 'management/server/affectedpeers/resolver\.go|policy|routers|networkmap|network_map' | head -200
echo
echo "Resolver lines 800-920:"
if [ -f management/server/affectedpeers/resolver.go ]; then
sed -n '800,920p' management/server/affectedpeers/resolver.go | nl -ba -v800
fi
echo
echo "Search AuthorizedUser (no non-test Go files removed):"
rg -n -C 8 'AuthorizedUser\b' --glob '*.go' | head -300 || true
echo
echo "Search IsBlocked in Go files:"
rg -n -C 6 'IsBlocked\(\)' --glob '*.go' | head -400 || true
echo
echo "Search PolicyRuleImpliesLegacySSH/netbird ssh:"
rg -n -C 6 'PolicyRuleImpliesLegacySSH|PolicyRuleProtocolNetbirdSSH|NetbirdSSH|netbirdssh' --glob '*.go' | head -500 || true
echo
echo "Locate types files:"
git ls-files | rg '(^|/)types\.go$|policy\.go$|peers\.go$|rules' | head -200Repository: netbirdio/netbird
Length of output: 3129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Resolver lines 800-920:"
sed -n '800,920p' management/server/affectedpeers/resolver.go | cat -n -v
echo
echo "Search AuthorizedUser in tracked Go files:"
rg -n -C 8 'AuthorizedUser\b' "${PWD}" -g '*.go' | head -400 || true
echo
echo "Search IsBlocked in tracked Go files:"
rg -n -C 6 'IsBlocked\(\)' "${PWD}" -g '*.go' | head -400 || true
echo
echo "Search PolicyRule protocols and helpers in tracked Go files:"
rg -n -C 6 'PolicyRuleImpliesLegacySSH|PolicyRuleProtocolNetbirdSSH|NetbirdSSH|netbirdssh|CollectFromSSHAuthorizedGroups|ruleShipsAllowedUsers|collectFromAllowedUsers' "${PWD}" -g '*.go' || trueRepository: netbirdio/netbird
Length of output: 50374
Include AuthorizedUser rules in ruleShipsAllowedUsers.
For NetBird SSH, a rule with AuthorizedUser still has its destination peers carry the account's allowed-user set (allowedUsers[auth.Wildcard][rule.AuthorizedUser]). The current fallback treats it as legacy SSH, so destination peers are not refreshed when AllowedUsersChanged is true. Match AuthorizedUser here instead: len(rule.AuthorizedGroups) == 0 && rule.AuthorizedUser == "" stays false for named-user rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@management/server/affectedpeers/resolver.go` around lines 878 - 883, The
ruleShipsAllowedUsers logic must classify NetBird SSH rules with AuthorizedUser
as allowed-user rules so destination peers refresh when AllowedUsersChanged is
true. Update the NetBird SSH branch to match AuthorizedUser rules appropriately,
while preserving the existing empty AuthorizedGroups and empty AuthorizedUser
condition for rules without a named user.
| } | ||
|
|
||
| var removedGroups, addedGroups []string | ||
| if update.AutoGroups != nil && settings.GroupsPropagationEnabled { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether GetAccountSettings can return (nil, nil) and how other callers guard it.
set -euo pipefail
ast-grep run --pattern 'func (s *SqlStore) GetAccountSettings($$$) ($$$) { $$$ }' --lang go
rg -n -C 3 'GetAccountSettings\(' --type=go -g '!**/*_test.go' -g '!**/*_mock.go'Repository: netbirdio/netbird
Length of output: 1454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | grep -E '(^|/)user\.go$|(^|/)account\.go$|(^|/)sql_store\.go$' || true
echo "== processUserUpdate and SaveOrAddUsers call =="
rg -n -C 8 'func processUserUpdate|processUserUpdate\(|func SaveOrAddUsers|SaveOrAddUsers\(' management/server/user.go management/server/account.go || true
echo "== GetAccountSettings callers excluding mocks/tests =="
rg -n -C 4 'GetAccountSettings\(' --type=go -g '!**/*_test.go' -g '!**/*_mock.go' || true
echo "== user.go lines around processUserUpdate =="
wc -l management/server/user.go
sed -n '800,870p' management/server/user.go
echo "== sql_store GetAccountSettings implementation =="
sed -n '2938,2970p' management/server/store/sql_store.goRepository: netbirdio/netbird
Length of output: 5744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SaveOrAddUsers full relevant lines =="
sed -n '579,650p' management/server/user.go
echo "== processUserUpdate dereferences settings =="
sed -n '782,870p' management/server/user.go
echo [=[
== AccountSettings nil return and validation helpers/defaults if present
]=]
rg -n -C 3 'Validate.*Settings|Settings.*Validation|Settings.*Default|default_settings|GroupsPropagationEnabled|GetAccountSettings' management/server --type=go -g '!**_*test.go' -g '!**_mock.go' || true
echo "== exact GetAccountSettings implementation =="
sed -n '2946,2966p' management/server/store/sql_store.go
echo "== call sites by searching broader references =="
rg -n 'GetAccountSettings|settings GroupsPropagationEnabled|settings.*GroupsPropagation' --type=go -g '!**/*_test.go' -g '!**/*_mock.go' || trueRepository: netbirdio/netbird
Length of output: 6374
Handle missing account settings before dereferencing settings.
SaveOrAddUsers returns on store errors from GetAccountSettings, but a missing settings row can still return (nil, nil) and hit settings.GroupsPropagationEnabled. Add nil handling before this dereference or route missing settings through validation defaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@management/server/user.go` at line 849, Update SaveOrAddUsers to handle a nil
settings result from GetAccountSettings before evaluating
settings.GroupsPropagationEnabled. Preserve existing store-error handling, and
either guard the dereference or apply the established validation defaults for
missing account settings.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
management/server/affected_peers_user_test.go (2)
155-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected result for
updTarget.Line 155 drains
updTarget, but no assertion follows for that channel. The unblocked user ownstargetPeer, so its expected behavior is the most relevant outcome of this subtest. Add eitherpeerShouldReceiveUpdate(t, updTarget)orpeerShouldNotReceiveUpdate(t, updTarget)to record the intended contract.💚 Proposed addition
+ peerShouldReceiveUpdate(t, updTarget) peerShouldReceiveUpdate(t, upd2) peerShouldNotReceiveUpdate(t, upd3)Select the assertion that matches the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/affected_peers_user_test.go` around lines 155 - 168, Add an explicit assertion for updTarget after SaveUser in the affected subtest, using peerShouldReceiveUpdate or peerShouldNotReceiveUpdate according to the intended unblock-refresh contract; keep the existing upd2 and upd3 assertions unchanged.
110-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the required subtest order.
These subtests share one account, one peer set, and one channel set. Each subtest depends on the state that the previous subtest leaves behind. Subtest 4 depends on
IPv6EnabledGroupsand the auto-groups set here. A futuret.Parallel()call or a reordering breaks the test in a way that is hard to diagnose. Add a short comment at the top of the test that states the subtests run in order and must not run in parallel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/affected_peers_user_test.go` around lines 110 - 132, Add a short comment at the beginning of the parent test containing these subtests, before the first t.Run call, stating that subtests share state, must execute in order, and must not run in parallel. Use the surrounding test function as the insertion point and leave the subtest logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@management/server/affected_peers_user_test.go`:
- Around line 155-168: Add an explicit assertion for updTarget after SaveUser in
the affected subtest, using peerShouldReceiveUpdate or
peerShouldNotReceiveUpdate according to the intended unblock-refresh contract;
keep the existing upd2 and upd3 assertions unchanged.
- Around line 110-132: Add a short comment at the beginning of the parent test
containing these subtests, before the first t.Run call, stating that subtests
share state, must execute in order, and must not run in parallel. Use the
surrounding test function as the insertion point and leave the subtest logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8621f19-6b53-454c-b594-e644d1593022
📒 Files selected for processing (1)
management/server/affected_peers_user_test.go
|
2 participants
Describe your changes
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:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit