add peer groups to status JSON/YAML output#5775
Open
Paulpatou wants to merge 3 commits intonetbirdio:mainfrom
Open
add peer groups to status JSON/YAML output#5775Paulpatou wants to merge 3 commits intonetbirdio:mainfrom
Paulpatou wants to merge 3 commits intonetbirdio:mainfrom
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughAdds peer group membership tracking end-to-end: introduces Groups on peer state, persists and exposes group names in status and sync payloads, populates per-peer group-name maps server-side, extends protobufs and CLI output, and adds tests and anonymization support for labels. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Engine
participant StatusRecorder
participant GRPC as "gRPC/Conversion"
participant Controller
participant NetworkMap
participant AccountStore as "Account/DB"
Client->>Engine: connect / request sync
Engine->>StatusRecorder: update peer FQDN / allowed IP
StatusRecorder->>StatusRecorder: UpdatePeerGroups(pubKey, groups)
Controller->>NetworkMap: build/merge network map
Controller->>AccountStore: GetAccountGroups(account)
AccountStore-->>Controller: groups with member lists
Controller->>NetworkMap: PopulatePeerGroupsNames(account)
Controller->>GRPC: ToSyncResponse(networkMap)
GRPC->>Client: SyncResponse includes RemotePeerConfig.groupsNames
Client->>Engine: receive RemotePeerConfig
Engine->>StatusRecorder: UpdatePeerGroups(peerPubKey, groupsFromSync)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
🧹 Nitpick comments (3)
management/server/types/account.go (1)
2081-2092: Sort group names before returning for stable status output.This method works, but sorting the result avoids non-deterministic ordering in JSON/YAML output.
♻️ Proposed change
func (a *Account) GetPeerGroupNames(peerID string) []string { var names []string for _, group := range a.Groups { for _, id := range group.Peers { if id == peerID { names = append(names, group.Name) break } } } + slices.Sort(names) return names }🤖 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 2081 - 2092, The GetPeerGroupNames method returns group names in iteration order which can be non-deterministic; update the function (Account.GetPeerGroupNames) to sort the names slice before returning so output is stable for JSON/YAML serialization—after collecting matching group.Name values push them into names, call sort.Strings(names) and then return names (ensure to import "sort" if not already).client/status/status_test.go (1)
153-175: Consider adding one non-emptygroupsfixture/assertion.Current updates verify empty-slice rendering well; adding one peer with actual group values would better guard end-to-end serialization of populated data.
Also applies to: 310-310, 334-334, 443-443, 463-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/status/status_test.go` around lines 153 - 175, Add at least one non-empty Groups value to one of the peer fixtures and assert it is serialized/rendered correctly: locate the peer struct literal (the entry with IP "192.168.178.102") in client/status/status_test.go and change Groups from []string{} to a populated slice (e.g. []string{"team-a"}), then update or add the corresponding test assertion that checks the output contains that group string; repeat the same change for the other peer fixtures referenced in the comment locations so the tests cover both empty and populated Groups cases.management/internals/controllers/network_map/controller/controller.go (1)
404-411: Consider extracting duplicate group name population logic.This code block is nearly identical to lines 264-271 in
sendUpdateAccountPeers. Additionally, the variable namepeerGroupsNames2is inconsistent withpeerGroupsNamesused in the other function.♻️ Suggestion: Extract helper function
func buildPeerGroupsNames(account *types.Account, peers []*nbpeer.Peer, offlinePeers []*nbpeer.Peer) map[string][]string { peerGroupsNames := make(map[string][]string) for _, rp := range peers { peerGroupsNames[rp.ID] = account.GetPeerGroupNames(rp.ID) } for _, rp := range offlinePeers { peerGroupsNames[rp.ID] = account.GetPeerGroupNames(rp.ID) } return peerGroupsNames }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/controllers/network_map/controller/controller.go` around lines 404 - 411, The duplicate population of peer group names (peerGroupsNames2 in controller.go) should be extracted into a shared helper to avoid repetition with sendUpdateAccountPeers; add a function (e.g., buildPeerGroupsNames(account *types.Account, peers []*nbpeer.Peer, offlinePeers []*nbpeer.Peer) map[string][]string) that calls account.GetPeerGroupNames for each rp.ID and returns the map, then replace the inline loops in both remotePeerNetworkMap population (where peerGroupsNames2 is set) and sendUpdateAccountPeers (where peerGroupsNames is used) to call this helper so the variable names and logic are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/status/status.go`:
- Line 76: The exported Groups field in client/status/status.go (Groups []string
`json:"groups" yaml:"groups"`) is leaked in anonymized output; update the
peer-anonymization code path (where anonymization is performed — e.g., the
function handling "--anonymize" / anonymizePeerDetails / AnonymizePeer) to clear
or replace the Groups field (set to nil, empty slice, or a fixed placeholder
like "REDACTED") before marshaling, and add a unit test that validates Groups is
scrubbed in anonymized output; ensure both JSON and YAML output flows use the
same scrub logic.
---
Nitpick comments:
In `@client/status/status_test.go`:
- Around line 153-175: Add at least one non-empty Groups value to one of the
peer fixtures and assert it is serialized/rendered correctly: locate the peer
struct literal (the entry with IP "192.168.178.102") in
client/status/status_test.go and change Groups from []string{} to a populated
slice (e.g. []string{"team-a"}), then update or add the corresponding test
assertion that checks the output contains that group string; repeat the same
change for the other peer fixtures referenced in the comment locations so the
tests cover both empty and populated Groups cases.
In `@management/internals/controllers/network_map/controller/controller.go`:
- Around line 404-411: The duplicate population of peer group names
(peerGroupsNames2 in controller.go) should be extracted into a shared helper to
avoid repetition with sendUpdateAccountPeers; add a function (e.g.,
buildPeerGroupsNames(account *types.Account, peers []*nbpeer.Peer, offlinePeers
[]*nbpeer.Peer) map[string][]string) that calls account.GetPeerGroupNames for
each rp.ID and returns the map, then replace the inline loops in both
remotePeerNetworkMap population (where peerGroupsNames2 is set) and
sendUpdateAccountPeers (where peerGroupsNames is used) to call this helper so
the variable names and logic are consistent.
In `@management/server/types/account.go`:
- Around line 2081-2092: The GetPeerGroupNames method returns group names in
iteration order which can be non-deterministic; update the function
(Account.GetPeerGroupNames) to sort the names slice before returning so output
is stable for JSON/YAML serialization—after collecting matching group.Name
values push them into names, call sort.Strings(names) and then return names
(ensure to import "sort" if not already).
🪄 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: 4d5d5067-ddc6-4b93-9d1f-29ba5c79d777
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goshared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
client/internal/engine.goclient/internal/peer/status.goclient/internal/peer/status_test.goclient/proto/daemon.protoclient/status/status.goclient/status/status_test.gomanagement/internals/controllers/network_map/controller/controller.gomanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/types/account.gomanagement/server/types/account_test.gomanagement/server/types/network.goshared/management/proto/management.proto
Add group membership (group names) for each peer in `netbird status --json` and `netbird status --yaml` output, resolving issue netbirdio#5714. Changes: - shared/management/proto: add groupsNames field to RemotePeerConfig - management/server/types: add PeerGroupsNames map to NetworkMap and GetPeerGroupNames helper on Account - management/internals/grpc: populate group names in RemotePeerConfig when building sync response (both controller and server paths) - client/proto: add groups field to PeerState daemon proto - client/internal/peer: add Groups field to State, UpdatePeerGroups method, and include groups in ToProto() - client/internal/engine: store peer groups on add and modify - client/status: expose groups in PeerStateDetailOutput (json:"groups")
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use protoc-gen-go v1.36.6 + protoc v33.1 for daemon.pb.go and protoc-gen-go v1.26.0 + protoc v34.1 for management.pb.go, matching the versions used by the upstream project. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Paulpatou
force-pushed
the
feature/add-groups-to-status-json-output
branch
from
ef897e8 to
27cae9c
Compare
|
Author
|
Rebased on latest main and addressed the review feedback. Regenerated |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/anonymize/anonymize.go`:
- Around line 146-159: AnonymizeLabel currently may return a generated token
equal to the original label (e.g. original "anon-label-1"); change
AnonymizeLabel on type Anonymizer to ensure the generated anonymized value is
never equal to the source: when creating a new anonymized value (using
a.labelCounter and fmt.Sprintf("anon-label-%d", ...)), loop/increment the
counter until the candidate != label and is not already present in
a.labelAnonymizer, then assign it to a.labelAnonymizer[label]; also add a
regression test that calls AnonymizeLabel with inputs already shaped like
"anon-label-N" to assert the returned anonymized value differs from the original
and remains consistent across repeated calls.
In `@management/internals/shared/grpc/server.go`:
- Around line 891-902: The initial-sync path builds peerGroupsNames without
sorting, causing order drift vs the update path that uses
PopulatePeerGroupsNames/account.GetPeerGroupNames which returns sorted names;
update the block that constructs peerGroupsNames (the loop feeding
networkMap.PeerGroupsNames after calling
s.accountManager.GetStore().GetAccountGroups) to sort each []string before
assignment so peerGroupsNames[peerID] contains a deterministically ordered list
matching account.GetPeerGroupNames.
🪄 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: a443182c-863a-4f82-97f8-9633609043b1
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goshared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
client/anonymize/anonymize.goclient/anonymize/anonymize_test.goclient/internal/engine.goclient/internal/peer/status.goclient/internal/peer/status_test.goclient/proto/daemon.protoclient/status/status.goclient/status/status_test.gomanagement/internals/controllers/network_map/controller/controller.gomanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/types/account.gomanagement/server/types/account_test.gomanagement/server/types/network.goshared/management/proto/management.proto
✅ Files skipped from review due to trivial changes (1)
- client/status/status_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- client/internal/peer/status_test.go
- client/proto/daemon.proto
- management/internals/shared/grpc/conversion.go
- client/internal/engine.go
- management/internals/controllers/network_map/controller/controller.go
- management/server/types/account_test.go
- client/internal/peer/status.go
Comment on lines
+146
to
+159
| // AnonymizeLabel anonymizes an arbitrary text label (e.g. group names) using a consistent mapping. | ||
| func (a *Anonymizer) AnonymizeLabel(label string) string { | ||
| if label == "" { | ||
| return label | ||
| } | ||
|
|
||
| anonymized, ok := a.labelAnonymizer[label] | ||
| if !ok { | ||
| a.labelCounter++ | ||
| anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter) | ||
| a.labelAnonymizer[label] = anonymized | ||
| } | ||
| return anonymized | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid generating an anonymized label that equals the original.
A real group name like anon-label-1 will pass through unchanged the first time it is seen, because the first generated token is also anon-label-1. That means --anonymize can still leak a user label. Please skip candidates that match the source label and add a regression test for labels already shaped like anon-label-N.
🛠️ Minimal fix
func (a *Anonymizer) AnonymizeLabel(label string) string {
if label == "" {
return label
}
anonymized, ok := a.labelAnonymizer[label]
if !ok {
- a.labelCounter++
- anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
+ for {
+ a.labelCounter++
+ anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter)
+ if anonymized != label {
+ break
+ }
+ }
a.labelAnonymizer[label] = anonymized
}
return anonymized
}📝 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
| // AnonymizeLabel anonymizes an arbitrary text label (e.g. group names) using a consistent mapping. | |
| func (a *Anonymizer) AnonymizeLabel(label string) string { | |
| if label == "" { | |
| return label | |
| } | |
| anonymized, ok := a.labelAnonymizer[label] | |
| if !ok { | |
| a.labelCounter++ | |
| anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter) | |
| a.labelAnonymizer[label] = anonymized | |
| } | |
| return anonymized | |
| } | |
| // AnonymizeLabel anonymizes an arbitrary text label (e.g. group names) using a consistent mapping. | |
| func (a *Anonymizer) AnonymizeLabel(label string) string { | |
| if label == "" { | |
| return label | |
| } | |
| anonymized, ok := a.labelAnonymizer[label] | |
| if !ok { | |
| for { | |
| a.labelCounter++ | |
| anonymized = fmt.Sprintf("anon-label-%d", a.labelCounter) | |
| if anonymized != label { | |
| break | |
| } | |
| } | |
| a.labelAnonymizer[label] = anonymized | |
| } | |
| return anonymized | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/anonymize/anonymize.go` around lines 146 - 159, AnonymizeLabel
currently may return a generated token equal to the original label (e.g.
original "anon-label-1"); change AnonymizeLabel on type Anonymizer to ensure the
generated anonymized value is never equal to the source: when creating a new
anonymized value (using a.labelCounter and fmt.Sprintf("anon-label-%d", ...)),
loop/increment the counter until the candidate != label and is not already
present in a.labelAnonymizer, then assign it to a.labelAnonymizer[label]; also
add a regression test that calls AnonymizeLabel with inputs already shaped like
"anon-label-N" to assert the returned anonymized value differs from the original
and remains consistent across repeated calls.
Comment on lines
+891
to
+902
| allGroups, err := s.accountManager.GetStore().GetAccountGroups(ctx, store.LockingStrengthNone, peer.AccountID) | ||
| if err != nil { | ||
| log.WithContext(ctx).Warnf("failed to get account groups for peer groups names: %v", err) | ||
| } else { | ||
| peerGroupsNames := make(map[string][]string) | ||
| for _, g := range allGroups { | ||
| for _, peerID := range g.Peers { | ||
| peerGroupsNames[peerID] = append(peerGroupsNames[peerID], g.Name) | ||
| } | ||
| } | ||
| networkMap.PeerGroupsNames = peerGroupsNames | ||
| } |
Contributor
There was a problem hiding this comment.
Sorting inconsistency between initial sync and update sync paths.
This code builds peerGroupsNames by iterating groups and appending names without sorting. However, PopulatePeerGroupsNames (used in the controller's update path) calls account.GetPeerGroupNames, which sorts the names before returning.
This means initial sync may return groups in a different order than subsequent update syncs, potentially causing unnecessary state churn on clients if they compare group lists.
🔧 Proposed fix: Sort names for consistency
+ "slices"
...
allGroups, err := s.accountManager.GetStore().GetAccountGroups(ctx, store.LockingStrengthNone, peer.AccountID)
if err != nil {
log.WithContext(ctx).Warnf("failed to get account groups for peer groups names: %v", err)
} else {
peerGroupsNames := make(map[string][]string)
for _, g := range allGroups {
for _, peerID := range g.Peers {
peerGroupsNames[peerID] = append(peerGroupsNames[peerID], g.Name)
}
}
+ for peerID := range peerGroupsNames {
+ slices.Sort(peerGroupsNames[peerID])
+ }
networkMap.PeerGroupsNames = peerGroupsNames
}📝 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
| allGroups, err := s.accountManager.GetStore().GetAccountGroups(ctx, store.LockingStrengthNone, peer.AccountID) | |
| if err != nil { | |
| log.WithContext(ctx).Warnf("failed to get account groups for peer groups names: %v", err) | |
| } else { | |
| peerGroupsNames := make(map[string][]string) | |
| for _, g := range allGroups { | |
| for _, peerID := range g.Peers { | |
| peerGroupsNames[peerID] = append(peerGroupsNames[peerID], g.Name) | |
| } | |
| } | |
| networkMap.PeerGroupsNames = peerGroupsNames | |
| } | |
| allGroups, err := s.accountManager.GetStore().GetAccountGroups(ctx, store.LockingStrengthNone, peer.AccountID) | |
| if err != nil { | |
| log.WithContext(ctx).Warnf("failed to get account groups for peer groups names: %v", err) | |
| } else { | |
| peerGroupsNames := make(map[string][]string) | |
| for _, g := range allGroups { | |
| for _, peerID := range g.Peers { | |
| peerGroupsNames[peerID] = append(peerGroupsNames[peerID], g.Name) | |
| } | |
| } | |
| for peerID := range peerGroupsNames { | |
| slices.Sort(peerGroupsNames[peerID]) | |
| } | |
| networkMap.PeerGroupsNames = peerGroupsNames | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/internals/shared/grpc/server.go` around lines 891 - 902, The
initial-sync path builds peerGroupsNames without sorting, causing order drift vs
the update path that uses PopulatePeerGroupsNames/account.GetPeerGroupNames
which returns sorted names; update the block that constructs peerGroupsNames
(the loop feeding networkMap.PeerGroupsNames after calling
s.accountManager.GetStore().GetAccountGroups) to sort each []string before
assignment so peerGroupsNames[peerID] contains a deterministically ordered list
matching account.GetPeerGroupNames.
Collaborator
|
Hello @Paulpatou thanks for submitting the PR. In general, we won't be sending the groups to the peers as part of the network map in this way. We have a few plans to optimize the network map and offload some of it to the peer that can allow similar information to be displayed, but current feedback from many users is that sharing internal infrastructure configuration details is not the desired mode. With that, I think the PR is a bit early, and we should revisit the idea in a few weeks. |
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
3 participants
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
This PR adds group membership information (group names) to each peer in the
netbird status --jsonandnetbird status --yamloutput.Problem
When using
netbird status --jsonornetbird status --yamlto inventory peers programmatically, group membership was missing from the output. This made the CLI output impractical for automation use cases where peers need to be filtered or categorized by group.The API already exposes this information, but not everyone can use the API in their tooling architecture - they rely on the CLI status output.
Solution
Added a
groupsfield to each peer entry in the status JSON/YAML output, containing the list of group names the peer belongs to.Before:
{ "peers": { "details": [ { "fqdn": "peer.example.netbird.cloud", "netbirdIp": "100.64.0.1", "status": "Connected" } ] } }After:
{ "peers": { "details": [ { "fqdn": "peer.example.netbird.cloud", "netbirdIp": "100.64.0.1", "status": "Connected", "groups": ["All", "Production"] } ] } }Changes
The feature is implemented across the full data pipeline:
Management server → client sync
shared/management/proto/management.proto— addrepeated string groupsNames = 6toRemotePeerConfigmanagement/server/types/network.go— addPeerGroupsNames map[string][]stringtoNetworkMapmanagement/server/types/account.go— addGetPeerGroupNames(peerID string) []stringhelpermanagement/internals/shared/grpc/conversion.go— populateGroupsNamesinappendRemotePeerConfigmanagement/internals/controllers/network_map/controller/controller.go— fillPeerGroupsNamesbeforeToSyncResponsemanagement/internals/shared/grpc/server.go— fillPeerGroupsNamesviaGetAccountGroupsinsendInitialSyncClient daemon → CLI
client/proto/daemon.proto— addrepeated string groups = 20toPeerStateclient/internal/peer/status.go— addGroups []stringtoState, addUpdatePeerGroups()method, include groups inToProto()client/internal/engine.go— callUpdatePeerGroupsinaddNewPeerandmodifyPeersCLI output
client/status/status.go— addGroups []string \json:"groups" yaml:"groups"`toPeerStateDetailOutput, populate inmapPeers()`Issue ticket number and link
#5714
Stack
Checklist
Documentation
This is a feature addition that exposes existing group membership data in the CLI status output. The API already provides this information; we're just making it visible in the JSON/YAML status output for automation use cases.
Docs PR URL (required if "docs added" is checked)
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests