k8s api auth proxy - add embedded client WhoIs API for Kubernetes auth proxy integration #4975
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds peer identity propagation (Groups, UserId) from management to clients, a new kubeconfig CLI command, and a WhoIs client API; extends gRPC sync response and RemotePeerConfig proto to include identity details and wires a PeerGroupsLookup through ToSyncResponse. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Mgmt as Management Server
participant GRPC as gRPC Sync Handler
participant Client as NetBird Client
participant Status as Status Recorder
participant Caller as CLI / App
Mgmt->>GRPC: Build ToSyncResponse(..., PeerGroupsLookup)
note right of GRPC `#DDEBF7`: resolve groups/userId per remote peer
GRPC->>Client: Send SyncResponse (RemotePeerConfig includes groups,userId)
Client->>Status: UpdatePeerIdentity(peerPubKey, groups, userId)
Status-->>Status: Persist Groups & UserId (online/offline)
Caller->>Client: WhoIs(peerIP)
Client->>Status: GetPeerIdentityByIP(peerIP)
Status-->>Client: PeerIdentity{FQDN, UserId, Groups}
Client-->>Caller: Return PeerIdentity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
management/internals/controllers/network_map/controller/controller.go (1)
237-243: ReuseAccountPeerGroupsLookupacross peers instead of recreating per loop iterationBoth
sendUpdateAccountPeersandUpdateAccountPeernow passgrpc.NewAccountPeerGroupsLookup(account)intoToSyncResponse, which is correct and keeps identity data (groups/userId) in sync. To avoid allocating a new lookup for every peer, consider computing it once per account and reusing:remoteGroupsLookup := grpc.NewAccountPeerGroupsLookup(account) // ... update := grpc.ToSyncResponse(..., maps.Keys(peerGroups), dnsFwdPort, remoteGroupsLookup)This keeps behavior identical while shaving a tiny bit of per‑peer overhead.
Also applies to: 354-359
client/embed/embed.go (1)
335-367:WhoIsAPI and locking are sound; consider a typed “not found” errorThe
WhoIsimplementation correctly:
- Uses the client mutex to snapshot
c.connect.- Handles unstarted client and missing/unused status recorder.
- Delegates to
GetPeerIdentityByIPand maps the result intoPeerIdentity.If callers are expected to distinguish “peer not found” from other failures, consider introducing a exported sentinel (e.g.
var ErrPeerNotFound = errors.New("peer not found")) instead of a rawfmt.Errorf. Otherwise this looks good as‑is.management/internals/shared/grpc/server.go (1)
705-713: Consider using a more targeted query for peer groups.Loading the entire
Accountobject viaGetStore().GetAccount()just to resolve peer group names may be costly at scale (large accounts with many peers, groups, policies, etc.). If the store already exposes a method to fetch only theGroupsmap, or if this data can be cached/derived from the already-fetchedpeerGroups(line 700-702), that would reduce memory pressure during sync.However, this is a graceful degradation path (error logged, sync continues without group data), which is reasonable for a new feature.
client/internal/peer/status.go (1)
320-334: UseRLockfor read-only operation and consider defensive copy of Groups slice.
GetPeerIdentityByIPonly reads data, soRLock()/RUnlock()would allow concurrent reads.- The
Groupsslice is returned by reference, meaning external modifications could affect internal state.🔎 Proposed fix
func (d *Status) GetPeerIdentityByIP(ip string) (PeerIdentity, bool) { - d.mux.Lock() - defer d.mux.Unlock() + d.mux.RLock() + defer d.mux.RUnlock() for _, state := range d.peers { if state.IP == ip { return PeerIdentity{ FQDN: state.FQDN, UserId: state.UserId, - Groups: state.Groups, + Groups: slices.Clone(state.Groups), }, true } } return PeerIdentity{}, false }management/internals/shared/grpc/conversion.go (1)
171-185: Consider building a reverse lookup map for better performance.
GetPeerGroupNamesis O(G×P) per call (iterating all groups and their peer lists). Since this is called for every remote peer during sync (inappendRemotePeerConfig), the total complexity becomes O(N×G×P) where N is the number of remote peers.For large accounts, consider building a
peerID → []groupNamemap once during construction:🔎 Proposed optimization
type AccountPeerGroupsLookup struct { - groups map[string]*types.Group + peerGroups map[string][]string // peerID -> group names } func NewAccountPeerGroupsLookup(account *types.Account) *AccountPeerGroupsLookup { if account == nil { return nil } - return &AccountPeerGroupsLookup{groups: account.Groups} + peerGroups := make(map[string][]string) + for _, group := range account.Groups { + for _, peerID := range group.Peers { + peerGroups[peerID] = append(peerGroups[peerID], group.Name) + } + } + return &AccountPeerGroupsLookup{peerGroups: peerGroups} } func (a *AccountPeerGroupsLookup) GetPeerGroupNames(peerID string) []string { if a == nil || a.peerGroups == nil { return nil } - var groupNames []string - for _, group := range a.groups { - for _, id := range group.Peers { - if id == peerID { - groupNames = append(groupNames, group.Name) - break - } - } - } - return groupNames + return a.peerGroups[peerID] }client/grpc/dialer.go (1)
91-101: Consider migrating togrpc.NewClientin a future refactor.
grpc.DialContextandgrpc.WithBlockare deprecated in favor ofgrpc.NewClientwith lazy connection establishment. This is not urgent and can be addressed separately, as the current approach still works.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ 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 (20)
client/cmd/kubeconfig.go(1 hunks)client/cmd/root.go(1 hunks)client/embed/embed.go(1 hunks)client/grpc/dialer.go(2 hunks)client/grpc/dialer_generic.go(1 hunks)client/grpc/dialer_js.go(1 hunks)client/grpc/fallback.go(1 hunks)client/internal/connect.go(1 hunks)client/internal/engine.go(3 hunks)client/internal/peer/status.go(5 hunks)client/internal/peer/status_test.go(4 hunks)client/internal/routemanager/client/client_bench_test.go(1 hunks)client/proto/daemon.proto(1 hunks)client/server/server.go(1 hunks)flow/client/client.go(1 hunks)management/internals/controllers/network_map/controller/controller.go(2 hunks)management/internals/shared/grpc/conversion.go(3 hunks)management/internals/shared/grpc/server.go(1 hunks)management/server/peer_test.go(1 hunks)shared/management/proto/management.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T12:20:47.254Z
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.
Applied to files:
management/internals/controllers/network_map/controller/controller.go
🧬 Code graph analysis (8)
client/internal/connect.go (1)
client/internal/peer/status.go (1)
Status(191-226)
flow/client/client.go (4)
client/grpc/dialer_generic.go (1)
WithCustomDialer(22-32)client/grpc/dialer_js.go (2)
WithCustomDialer(12-14)IsWebSocketFallbackEnabled(24-24)util/wsproxy/constants.go (1)
FlowComponent(12-12)client/grpc/fallback.go (1)
IsWebSocketFallbackEnabled(41-45)
client/grpc/fallback.go (2)
client/grpc/dialer_js.go (3)
EnableWebSocketFallback(19-21)IsWebSocketFallbackEnabled(24-24)ShouldFallbackToWebSocket(27-27)shared/management/status/error.go (1)
FromError(78-87)
client/server/server.go (1)
management/server/permissions/modules/module.go (1)
Groups(8-8)
management/server/peer_test.go (1)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)
client/cmd/kubeconfig.go (3)
client/cmd/root.go (1)
DialClientGRPCServer(231-241)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(80-82)shared/management/status/error.go (1)
Errorf(70-75)
management/internals/controllers/network_map/controller/controller.go (1)
management/internals/shared/grpc/conversion.go (2)
ToSyncResponse(107-150)NewAccountPeerGroupsLookup(163-168)
client/grpc/dialer.go (4)
client/grpc/fallback.go (3)
IsWebSocketFallbackEnabled(41-45)ShouldFallbackToWebSocket(49-66)EnableWebSocketFallback(31-38)client/grpc/dialer_js.go (4)
IsWebSocketFallbackEnabled(24-24)ShouldFallbackToWebSocket(27-27)EnableWebSocketFallback(19-21)WithCustomDialer(12-14)shared/management/status/error.go (1)
Errorf(70-75)client/grpc/dialer_generic.go (1)
WithCustomDialer(22-32)
🔇 Additional comments (25)
client/cmd/root.go (1)
144-144: LGTM!The kubeconfig command registration follows the standard pattern used for other commands in this file.
shared/management/proto/management.proto (1)
342-350: LGTM!The new fields for peer identity are well-documented and use appropriate field numbers. The additions are backward-compatible.
client/grpc/dialer_js.go (1)
12-27: LGTM!The JS-specific implementation correctly handles WebSocket as the default transport. The no-op fallback functions and the
IsWebSocketFallbackEnabled() = truesemantics are appropriate for JS/WASM environments where WebSocket is always used.client/grpc/dialer_generic.go (3)
22-32: LGTM!The refactored dialer with conditional WebSocket/native transport selection is clean and well-structured.
34-51: Verify the context usage for WebSocket connections.Line 50 uses
context.Background()for the WebSocket NetConn wrapper. While the comment explains that the dialer context is cancelled after dial returns, this means the WebSocket connection will not be cancelled when the original context is cancelled.Please verify this is the intended behavior and that connection lifecycle is managed elsewhere (e.g., via gRPC's connection management). If the connection should respect the original context's cancellation, consider using a long-lived context passed from the caller rather than
context.Background().
53-76: LGTM with a note on privilege requirements.The native dialer correctly handles the Linux root privilege requirement. The fallback to standard dialer for non-root users is appropriate, though the comment correctly notes this may not route through the NetBird tunnel.
client/cmd/kubeconfig.go (2)
50-68: LGTM!The daemon connectivity check is appropriately non-fatal with clear warning messages. This allows kubeconfig generation to proceed even if the daemon is unavailable, which is a good user experience.
86-94: LGTM!Directory creation with 0700 and file writing with 0600 provide appropriate security for kubeconfig files.
client/internal/connect.go (1)
379-383: LGTM!Clean accessor method that exposes the status recorder for use by the embed client's WhoIs functionality.
client/proto/daemon.proto (1)
325-329: LGTM!The peer identity fields are consistent with the management proto changes and properly documented. The field numbers are appropriate and maintain backward compatibility.
client/internal/routemanager/client/client_bench_test.go (1)
49-49: LGTM!The benchmark test correctly updated to use the new
AddPeersignature withnilfor groups and empty string for userId, which is appropriate for benchmark data generation.flow/client/client.go (1)
57-64: Flow client WebSocket fallback wiring looks correctPassing
nbgrpc.IsWebSocketFallbackEnabled()intoWithCustomDialermatches the new API and the comment accurately describes the one‑time snapshot behavior for this non‑critical telemetry client. No further changes needed here.client/server/server.go (1)
1650-1674: Groups/userId propagation intoproto.PeerStateis correctly wiredMapping
peerState.GroupsandpeerState.UserIdinto the protobufPeerStatecompletes the identity propagation for status reporting. The change is additive and does not alter existing behavior.client/internal/peer/status_test.go (1)
17-24: Tests correctly updated to the extendedAddPeersignatureAll updated
AddPeerinvocations now supply the newgroupsanduserIdparameters (niland""), which is appropriate for tests that don’t care about identity details. No behavioral changes to the tests themselves.Also applies to: 32-33, 49-50, 88-89
management/server/peer_test.go (1)
1180-1183:TestToSyncResponseupdated correctly for new parametersThe
grpc.ToSyncResponsecall now passesaccountSettings,nilextra settings, an empty peer‑groups slice, andnilfor the remote peer‑groups lookup. This matches the extended signature and keeps the existing test focus intact.client/grpc/fallback.go (1)
1-66: WebSocket fallback helpers are thread‑safe and conservatively scopedThe new fallback utilities:
- Use an RWMutex to safely guard the global
webSocketFallbackEnabledflag.- Make
EnableWebSocketFallbackidempotent with a single log on first enable.- Classify only
context.DeadlineExceededandcodes.Unavailable/codes.DeadlineExceededfor fallback, which is a reasonable, conservative starting set.The design looks solid and can be extended later if additional error patterns are identified.
client/internal/engine.go (1)
662-669: Peer identity (groups/userId) propagation in the engine is consistent and correctly scopedThe three identity‑related changes work together coherently:
modifyPeers: updates FQDN and now also callsUpdatePeerIdentityfor peers that didn’t trigger a reconnect, ensuring group/user changes are reflected without tearing down connections.updateOfflinePeers: includesGroupsandUserIdin the replacementpeer.Stateslice, so offline peers retain identity data for lookups.addNewPeer: extendsAddPeerto seed the status recorder with the groups/userId fromRemotePeerConfigwhen a connection is first created (and also after reconnection for structurally modified peers).All of this runs under
syncMsgMuxviaupdateNetworkMap, so status recorder mutations remain serialized with other engine updates. No additional issues from these changes.Also applies to: 1265-1277, 1315-1318
management/internals/shared/grpc/server.go (1)
715-715: LGTM!The updated
ToSyncResponsecall correctly passes the newremotePeerGroupsLookupparameter. The nil-safety is handled within the lookup implementation.client/internal/peer/status.go (3)
72-77: LGTM - Good documentation for K8s Auth Proxy integration.The new
GroupsandUserIdfields are well-documented with clear explanations of their purpose for K8s RBAC mapping. These fields are appropriately placed in theStatestruct.
266-282: LGTM!The extended
AddPeersignature correctly initializes the new identity fields. The mutex is properly initialized for the new peer state.
609-625: LGTM!The
UpdatePeerIdentitymethod correctly uses mutex locking and follows the same pattern as other update methods likeUpdatePeerFQDN. Not triggering notifications is appropriate since identity changes don't affect connection state.management/internals/shared/grpc/conversion.go (2)
152-155: LGTM - Clean interface design.The
PeerGroupsLookupinterface provides a clean abstraction for peer group resolution, enabling testability and potential alternative implementations.
187-204: LGTM!The
appendRemotePeerConfigfunction correctly handles nilgroupsLookupand properly populates the newGroupsandUserIdfields in the proto message.client/grpc/dialer.go (2)
32-57: LGTM - Well-structured WebSocket fallback logic.The fallback mechanism is well-designed:
- Avoids redundant attempts by checking
IsWebSocketFallbackEnabled()first- Correctly excludes JS platform (which always uses WebSocket)
- Requires non-empty
componentfor fallback eligibility- Preserves both errors in the combined error message for debugging
60-75: The review comment is accurate and no changes are needed. The code correctly implements WebSocket TLS handling: whentlsEnabled=trueanduseWebSocket=true, thedialWebSocketfunction properly constructswss://URLs, and gRPC appropriately uses insecure credentials since TLS encryption is handled at the WebSocket transport layer rather than the gRPC layer.
| func generateKubeconfig(server, cluster, context, user, namespace string) string { | ||
| return fmt.Sprintf(`apiVersion: v1 | ||
| kind: Config | ||
| clusters: | ||
| - cluster: | ||
| insecure-skip-tls-verify: true | ||
| server: %s | ||
| name: %s | ||
| contexts: | ||
| - context: | ||
| cluster: %s | ||
| namespace: %s | ||
| user: %s | ||
| name: %s | ||
| current-context: %s | ||
| users: | ||
| - name: %s | ||
| user: | ||
| token: netbird-auth-proxy | ||
| `, server, cluster, cluster, namespace, user, context, context, user) | ||
| } |
There was a problem hiding this comment.
Security concern: TLS verification disabled by default.
Line 109 hardcodes insecure-skip-tls-verify: true in the generated kubeconfig. This disables TLS certificate verification, which could expose users to man-in-the-middle attacks.
Consider one of the following approaches:
- Add a flag to control this behavior (e.g.,
--insecure-skip-tls-verify) and default tofalse - If the NetBird-accessible cluster uses self-signed certificates that users trust, consider providing a way to specify a CA certificate bundle instead
- At minimum, add a warning message when generating the kubeconfig that TLS verification is disabled
The dummy token on line 122 is acceptable per the PR description (auth handled by NetBird IP).
🤖 Prompt for AI Agents
In client/cmd/kubeconfig.go around lines 104 to 124, the generated kubeconfig
unconditionally sets insecure-skip-tls-verify: true; change the function to
accept an insecureSkipTLS boolean (default false) and an optional caBundle
string/path, set insecure-skip-tls-verify to the boolean instead of hardcoding
true, and when caBundle is provided include it in the cluster block as
certificate-authority-data (base64) instead of enabling insecure skip;
additionally, if insecureSkipTLS is true emit a clear warning message to the
user so they know they are disabling certificate verification.
- Add groups/userId fields to proto and regenerate - Update management server to populate peer groups in sync response - Add GetPeerIdentityByIP to client status recorder for lookups
shyam0904a
force-pushed
the
feature/k8s-api-auth-proxy
branch
from
cf13e40 to
855326d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/cmd/kubeconfig.go (2)
78-84: Potential panic on string slice access.This issue was flagged in a previous review. Line 78 accesses
kubeconfigOutput[:2]without checking the length ofkubeconfigOutput. This will panic if the output path is empty or a single character. Usestrings.HasPrefixfor safe prefix checking.🔎 Proposed fix
+import "strings" + // Expand ~ in path - if kubeconfigOutput[:2] == "~/" { + if strings.HasPrefix(kubeconfigOutput, "~/") { home, err := os.UserHomeDir() if err != nil { return fmt.Errorf("failed to get home directory: %w", err) } kubeconfigOutput = filepath.Join(home, kubeconfigOutput[2:]) }
104-124: Security concern: TLS verification disabled by default.This issue was flagged in a previous review. Hardcoding
insecure-skip-tls-verify: truedisables TLS certificate verification, exposing users to man-in-the-middle attacks. Consider adding a--insecure-skip-tls-verifyflag (defaulting tofalse) or at minimum printing a warning when generating the kubeconfig.
🧹 Nitpick comments (7)
client/embed/embed.go (3)
335-340: Consider documenting the duplicatePeerIdentitytype.This
PeerIdentitytype mirrorspeer.PeerIdentityinclient/internal/peer/status.go. While this is likely intentional to provide a clean public API for the embed package without exposing internal types, a brief comment explaining this design choice would help future maintainers understand the relationship.
362-366: Shallow copy ofGroupsslice may cause unintended side effects.The
Groupsslice is assigned directly from the internal state, creating a shared reference to the underlying array. If the caller modifies the returned slice, it could affect the internal peer state. Consider cloning the slice for safety.🔎 Proposed fix
+ import "slices" + return PeerIdentity{ FQDN: identity.FQDN, UserId: identity.UserId, - Groups: identity.Groups, + Groups: slices.Clone(identity.Groups), }, nil
357-360: Include IP in error message for easier debugging.The error message "peer not found" doesn't include the IP that was looked up, which could make debugging more difficult.
🔎 Proposed fix
identity, found := statusRecorder.GetPeerIdentityByIP(ip) if !found { - return PeerIdentity{}, fmt.Errorf("peer not found") + return PeerIdentity{}, fmt.Errorf("peer not found for IP %s", ip) }client/internal/peer/status.go (3)
318-334: Consider usingRLockfor read-only operation and cloningGroups.
GetPeerIdentityByIPis a read-only operation, sod.mux.RLock()would allow concurrent reads.- The
Groupsslice should be cloned to prevent the caller from accidentally modifying internal state.🔎 Proposed fix
func (d *Status) GetPeerIdentityByIP(ip string) (PeerIdentity, bool) { - d.mux.Lock() - defer d.mux.Unlock() + d.mux.RLock() + defer d.mux.RUnlock() for _, state := range d.peers { if state.IP == ip { return PeerIdentity{ FQDN: state.FQDN, UserId: state.UserId, - Groups: state.Groups, + Groups: slices.Clone(state.Groups), }, true } } return PeerIdentity{}, false }
266-282: Clonegroupsslice to prevent aliasing.The
groupsslice is stored directly, which means the caller could inadvertently modify the internal state. Consider cloning it on storage.🔎 Proposed fix
d.peers[peerPubKey] = State{ PubKey: peerPubKey, IP: ip, ConnStatus: StatusIdle, FQDN: fqdn, Mux: new(sync.RWMutex), - Groups: groups, + Groups: slices.Clone(groups), UserId: userId, }
609-625: Clonegroupsslice inUpdatePeerIdentityfor consistency.Same aliasing concern as
AddPeer- thegroupsslice should be cloned to prevent the caller from modifying internal state.🔎 Proposed fix
peerState, ok := d.peers[peerPubKey] if !ok { return errors.New("peer doesn't exist") } - peerState.Groups = groups + peerState.Groups = slices.Clone(groups) peerState.UserId = userId d.peers[peerPubKey] = peerStatemanagement/internals/shared/grpc/conversion.go (1)
152-185: Consider optimizing group lookup performance for large accounts.The current implementation iterates through all groups and all peers within each group for every peer (O(N×M) complexity). For accounts with many groups and peers, this could become a performance bottleneck during sync operations.
💡 Optional optimization: pre-build a reverse index
Consider building a reverse index (peer ID → group names) once during AccountPeerGroupsLookup construction:
type AccountPeerGroupsLookup struct { - groups map[string]*types.Group + peerToGroups map[string][]string } func NewAccountPeerGroupsLookup(account *types.Account) *AccountPeerGroupsLookup { if account == nil { return nil } - return &AccountPeerGroupsLookup{groups: account.Groups} + peerToGroups := make(map[string][]string) + for _, group := range account.Groups { + for _, peerID := range group.Peers { + peerToGroups[peerID] = append(peerToGroups[peerID], group.Name) + } + } + return &AccountPeerGroupsLookup{peerToGroups: peerToGroups} } func (a *AccountPeerGroupsLookup) GetPeerGroupNames(peerID string) []string { - if a == nil || a.groups == nil { + if a == nil || a.peerToGroups == nil { return nil } - var groupNames []string - for _, group := range a.groups { - for _, id := range group.Peers { - if id == peerID { - groupNames = append(groupNames, group.Name) - break - } - } - } - return groupNames + return a.peerToGroups[peerID] }This changes the lookup from O(N×M) to O(1), which can significantly improve sync performance for large accounts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
client/cmd/kubeconfig.go(1 hunks)client/cmd/root.go(1 hunks)client/embed/embed.go(1 hunks)client/internal/connect.go(1 hunks)client/internal/engine.go(3 hunks)client/internal/peer/status.go(5 hunks)client/internal/peer/status_test.go(4 hunks)client/internal/routemanager/client/client_bench_test.go(1 hunks)management/internals/controllers/network_map/controller/controller.go(2 hunks)management/internals/shared/grpc/conversion.go(3 hunks)management/internals/shared/grpc/server.go(1 hunks)management/server/peer_test.go(1 hunks)shared/management/proto/management.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- client/internal/connect.go
- shared/management/proto/management.proto
- client/cmd/root.go
- client/internal/peer/status_test.go
- management/internals/shared/grpc/server.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T12:20:47.254Z
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.
Applied to files:
management/internals/controllers/network_map/controller/controller.go
🧬 Code graph analysis (3)
management/server/peer_test.go (1)
management/internals/shared/grpc/conversion.go (1)
ToSyncResponse(107-150)
client/cmd/kubeconfig.go (3)
client/cmd/root.go (1)
DialClientGRPCServer(231-241)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(80-82)shared/management/status/error.go (1)
Errorf(70-75)
management/internals/shared/grpc/conversion.go (1)
shared/management/proto/management.pb.go (12)
NetworkMap(1849-1879)NetworkMap(1894-1894)NetworkMap(1909-1911)SyncResponse(388-405)SyncResponse(420-420)SyncResponse(435-437)RemotePeerConfig(1999-2019)RemotePeerConfig(2034-2034)RemotePeerConfig(2049-2051)SSHConfig(2103-2114)SSHConfig(2129-2129)SSHConfig(2144-2146)
🔇 Additional comments (12)
client/cmd/kubeconfig.go (1)
50-68: LGTM - Non-blocking status check is appropriate.The approach of warning when NetBird daemon is unreachable but continuing with kubeconfig generation is good UX. It allows users to generate configs offline and ensures the command doesn't fail unexpectedly.
client/internal/peer/status.go (2)
72-77: Good documentation for new identity fields.Clear comments explaining the purpose of
GroupsandUserIdfields and their relationship to K8s Auth Proxy. This helps maintainers understand the cross-cutting concern.
311-316: LGTM - Well-documented type definition.The
PeerIdentitytype is clean and well-documented for its K8s Auth Proxy use case.client/internal/routemanager/client/client_bench_test.go (1)
49-52: LGTM - Benchmark correctly updated for new API signature.The addition of
nil, ""for the newgroupsanduserIdparameters aligns with theAddPeersignature change. Using nil/empty values is appropriate since this benchmark focuses on route calculation, not identity features.management/internals/controllers/network_map/controller/controller.go (2)
239-242: LGTM - Peer groups lookup correctly integrated.The
grpc.NewAccountPeerGroupsLookup(account)parameter enables propagation of peer group information in sync responses, which is essential for the K8s Auth Proxy to map NetBird groups to Kubernetes RBAC groups.
357-358: Consistent integration of peer groups lookup.The same pattern is correctly applied in
UpdateAccountPeer, ensuring both bulk and single-peer update paths include group information.client/internal/engine.go (3)
665-669: LGTM! Peer identity propagation correctly implemented.The UpdatePeerIdentity call ensures that peer group membership and user ownership information is kept up-to-date in the status recorder. The error handling is appropriate—logging a warning allows the system to continue operating even if the status update fails.
1275-1276: LGTM! Offline peer identity correctly preserved.The Groups and UserId fields are correctly populated from the RemotePeerConfig when creating offline peer states, ensuring identity information is available even for peers that are not currently connected.
1315-1315: LGTM! New peer identity correctly initialized.The AddPeer call correctly passes the groups and userId from the peer config to the status recorder, ensuring identity information is available from the moment a peer is added.
management/internals/shared/grpc/conversion.go (2)
187-204: LGTM! Remote peer configuration correctly extended with identity.The function correctly:
- Uses the groups lookup when available (lines 189-192)
- Populates the Groups field with resolved group names (line 199)
- Preserves the UserId from the peer record (line 200)
- Handles nil lookup gracefully
107-107: LGTM! Function signature correctly extended for identity propagation.The ToSyncResponse signature has been extended with the
remotePeerGroupsLookupparameter, and it's correctly passed through to bothappendRemotePeerConfigcalls for RemotePeers (line 125) and OfflinePeers (line 131), ensuring identity information is available for all peer types.Also applies to: 125-125, 131-131
management/server/peer_test.go (1)
1182-1182: Ensure test coverage for the new peer identity propagation feature.The test passes
nilfor the newremotePeerGroupsLookupparameter, which means the Groups field in RemotePeerConfig won't be populated in this test. While UserId is populated from the peer's UserID, the test doesn't verify either field. Confirm that additional tests exist elsewhere (in this file or another) that validate Groups and UserId are correctly propagated whenremotePeerGroupsLookupis provided with actual group data.
- Use strings.HasPrefix instead of slice access to prevent panic on short paths - Add warning about TLS verification being disabled (safe with WireGuard tunnel)
- Clone Groups slice to prevent caller from modifying internal state - Include IP address in 'peer not found' error for easier debugging - Add documentation explaining PeerIdentity type design
- Use RLock instead of Lock in GetPeerIdentityByIP for concurrent reads - Clone Groups slice in GetPeerIdentityByIP to prevent state modification - Clone Groups slice in UpdatePeerIdentity to prevent aliasing issues
- Pre-build peer ID to group names index in constructor for O(1) lookups - Improves sync performance for accounts with many groups and peers - Previously O(N*M) complexity, now O(1) per lookup
shyam0904a
changed the title
shyam0904a
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/internal/peer/status.go (1)
266-285: Clone thegroupsslice for consistency withUpdatePeerIdentity.
UpdatePeerIdentity(line 624) defensively clones the groups slice, butAddPeerstores it directly. This inconsistency could lead to subtle bugs if callers modify the slice after callingAddPeer.🔎 Proposed fix
func (d *Status) AddPeer(peerPubKey string, fqdn string, ip string, groups []string, userId string) error { d.mux.Lock() defer d.mux.Unlock() _, ok := d.peers[peerPubKey] if ok { return errors.New("peer already exist") } + // Clone the groups slice to prevent caller from modifying internal state + groupsCopy := make([]string, len(groups)) + copy(groupsCopy, groups) d.peers[peerPubKey] = State{ PubKey: peerPubKey, IP: ip, ConnStatus: StatusIdle, FQDN: fqdn, Mux: new(sync.RWMutex), - Groups: groups, + Groups: groupsCopy, UserId: userId, } d.peerListChangedForNotification = true return nil }
🧹 Nitpick comments (1)
management/internals/shared/grpc/conversion.go (1)
107-107: Consider consolidating parameters into an options struct.The
ToSyncResponsefunction now has 16 parameters, which impacts readability and maintainability. Consider grouping related parameters into a config/options struct.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/cmd/kubeconfig.go(1 hunks)client/embed/embed.go(1 hunks)client/internal/peer/status.go(5 hunks)management/internals/shared/grpc/conversion.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/cmd/kubeconfig.go
🔇 Additional comments (7)
management/internals/shared/grpc/conversion.go (2)
152-185: LGTM! Clean interface design with O(1) lookup optimization.The
PeerGroupsLookupinterface is well-designed with proper nil safety checks. The reverse index approach inAccountPeerGroupsLookupefficiently avoids O(N*M) iteration during sync responses.
187-204: LGTM! Groups and UserId correctly propagated to RemotePeerConfig.The implementation properly handles nil
groupsLookupand populates bothGroupsandUserIdfields from the peer data.client/embed/embed.go (2)
335-342: LGTM! Clean public API type.Mirroring the internal
peer.PeerIdentitytype here provides a stable public API without exposing internal implementation details.
344-375: LGTM! Well-implemented identity lookup with proper defensive copying.The method correctly:
- Uses lock/unlock pattern for thread-safe access to
connect- Returns appropriate errors for uninitialized states
- Clones the
Groupsslice to prevent caller modifications to internal stateclient/internal/peer/status.go (3)
72-77: LGTM! Well-documented identity fields.The comments clearly explain the purpose of these fields for K8s Auth Proxy integration.
311-337: LGTM! Thread-safe identity lookup with proper defensive copying.Good use of
RLockfor read-only access and cloning theGroupsslice before returning.One consideration: The O(n) linear scan through peers is acceptable for typical mesh sizes, but if this becomes a hot path with thousands of peers, an IP-indexed lookup could be added.
612-630: LGTM! Proper update method with defensive slice copying.The method correctly clones the input
groupsslice to prevent external modifications from affecting internal state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/internal/peer/status.go (3)
265-291: Excellent defensive copying to prevent shared-slice mutations.The groups slice is correctly cloned before storing to prevent external modifications. The implementation is sound and well-documented.
Optional: Use slices.Clone for consistency
The file already uses
slices.Cloneelsewhere (lines 978, 1015, 1235). Consider using it here for consistency:- // Clone the groups slice to prevent caller from modifying internal state - groupsCopy := make([]string, len(groups)) - copy(groupsCopy, groups) - d.peers[peerPubKey] = State{ PubKey: peerPubKey, IP: ip, ConnStatus: StatusIdle, FQDN: fqdn, Mux: new(sync.RWMutex), - Groups: groupsCopy, + Groups: slices.Clone(groups), UserId: userId, }
317-343: Well-implemented identity lookup with proper defensive copying.The
GetPeerIdentityByIPmethod correctly uses read locking and clones the Groups slice before returning to prevent external modifications. The documentation clearly explains its purpose for K8s Auth Proxy.Optional: Use slices.Clone for consistency
Similar to the suggestion for
AddPeer, consider usingslices.Clonefor consistency with the rest of the file:for _, state := range d.peers { if state.IP == ip { - // Clone the Groups slice to prevent caller from modifying internal state - groups := make([]string, len(state.Groups)) - copy(groups, state.Groups) return PeerIdentity{ FQDN: state.FQDN, UserId: state.UserId, - Groups: groups, + Groups: slices.Clone(state.Groups), }, true } }
618-636: Correct implementation with proper locking and defensive copying.The
UpdatePeerIdentitymethod appropriately uses write locking and clones the groups slice to maintain data integrity. The error handling and documentation are clear.Optional: Use slices.Clone for consistency
For consistency with the rest of the file, consider using
slices.Clone:peerState, ok := d.peers[peerPubKey] if !ok { return errors.New("peer doesn't exist") } - // Clone the groups slice to prevent caller from modifying internal state - peerState.Groups = make([]string, len(groups)) - copy(peerState.Groups, groups) + peerState.Groups = slices.Clone(groups) peerState.UserId = userId d.peers[peerPubKey] = peerState
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/cmd/kubeconfig.goclient/internal/peer/status.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/cmd/kubeconfig.go
🔇 Additional comments (1)
client/internal/peer/status.go (1)
72-77: LGTM - Clear documentation for identity fields.The new
GroupsandUserIdfields are well-documented with clear explanations of their purpose for K8s Auth Proxy impersonation.
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shyamganesh-tide. * #4975 (comment) The following files were modified: * `client/cmd/kubeconfig.go` * `client/cmd/root.go` * `management/internals/shared/grpc/conversion.go`
|
1 participant
Describe your changes
Add Kubernetes API Auth Proxy feature with embedded NetBird client for identity-aware access to the Kubernetes API.
Changes include:
client/embed/embed.go: Add WhoIs() method for peer identity lookup by IPclient/internal/connect.go: Expose StatusRecorder for embed clientclient/internal/peer/status.go: Add GetPeerIdentityByIP() and UpdatePeerIdentity() methodsclient/internal/engine.go: Update peer identity on network map changesclient/cmd/kubeconfig.go: Add CLI command to generate kubeconfig for auth proxyshared/management/proto/management.proto: Add groups/userId to RemotePeerConfigmanagement/internals/shared/grpc/conversion.go: Populate groups in sync responseHow it works:
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
527
Related PR for operator:
91
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.