Skip to content

[management] Refactor network map controller #4789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pascal-fischer merged 54 commits into from
Dec 2, 2025
Merged

[management] Refactor network map controller #4789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
03b599a
move network map logic into new design
pascal-fischer Nov 10, 2025
1d25cf8
fix tests
pascal-fischer Nov 11, 2025
092fc24
go mod tidy
pascal-fischer Nov 11, 2025
da1ffe4
fix client tests
pascal-fischer Nov 11, 2025
febde70
fix linter
pascal-fischer Nov 11, 2025
ee45532
fix user delete test
pascal-fischer Nov 11, 2025
51ad8d7
drain channel before delete peer test
pascal-fischer Nov 11, 2025
e6aa119
remove leftover timer
pascal-fischer Nov 11, 2025
d35f904
fix peers handler
pascal-fischer Nov 11, 2025
71684e4
fix some of the rabbit remarks
pascal-fischer Nov 11, 2025
addb4df
delete testdata result
pascal-fischer Nov 11, 2025
bfc84c0
remove proxy code
pascal-fischer Nov 11, 2025
abb8782
fix imports
pascal-fischer Nov 11, 2025
04209c5
remove proxy container
pascal-fischer Nov 11, 2025
41744aa
Revert "remove proxy container"
pascal-fischer Nov 11, 2025
1908cfa
Revert "fix imports"
pascal-fischer Nov 11, 2025
705fd84
Revert "remove proxy code"
pascal-fischer Nov 11, 2025
f2bb181
bring back proxy map merge
pascal-fischer Nov 11, 2025
83b8216
fix test constructor
pascal-fischer Nov 11, 2025
0294c04
fix client side
pascal-fischer Nov 11, 2025
a31e293
remove channel drain for non experimental
pascal-fischer Nov 11, 2025
a0b7a8f
fix flaky test
pascal-fischer Nov 11, 2025
513ab7b
update network map controller
pascal-fischer Nov 11, 2025
3fb4a94
remove channel check on peers api
pascal-fischer Nov 11, 2025
67460a2
make network map controller group multiple changes at once
pascal-fischer Nov 12, 2025
46f0e6c
do not double recalculate on partial update
pascal-fischer Nov 12, 2025
596820b
send update
pascal-fischer Nov 12, 2025
40808a0
expect network map on all peer updates
pascal-fischer Nov 12, 2025
df87122
Merge branch 'refactor/network-map-controller-new-design' into refact…
pascal-fischer Nov 12, 2025
9eaeb09
fix merge conflicts
pascal-fischer Nov 12, 2025
78d7de6
Merge branch 'main' into refactor/network-map-controller
pascal-fischer Nov 13, 2025
9530738
handle peer expiration in networkmap controller
pascal-fischer Nov 13, 2025
88d8835
send events
pascal-fischer Nov 14, 2025
c443de7
extract ephemeral lifetime
pascal-fischer Nov 14, 2025
ab8b0d9
fix tests
pascal-fischer Nov 14, 2025
6d2f609
fix tests
pascal-fischer Nov 14, 2025
7829659
update management integrations
pascal-fischer Nov 14, 2025
bcd9ca1
Merge branch 'main' into refactor/network-map-controller
pascal-fischer Nov 14, 2025
4fc1b4d
fix tests
pascal-fischer Nov 14, 2025
822e0c7
fix tests
pascal-fischer Nov 14, 2025
7b80e00
return interfaces on bootstrapping
pascal-fischer Nov 17, 2025
2b89747
Merge branch 'main' into refactor/network-map-controller
pascal-fischer Nov 19, 2025
e25f01d
fix merge
pascal-fischer Nov 19, 2025
ad87fbc
fix account test
pascal-fischer Nov 19, 2025
d2916da
remove checkPeerStatus check
pascal-fischer Nov 20, 2025
0d7aa44
remove test expectation
pascal-fischer Nov 20, 2025
b5d3a1e
improve error handling
pascal-fischer Nov 21, 2025
fbf92bb
extract wg key handling to the secrets manager
pascal-fischer Nov 25, 2025
e6c11d1
Merge branch 'main' into refactor/network-map-controller
pascal-fischer Nov 26, 2025
8afe24a
fix test and update mock
pascal-fischer Nov 26, 2025
fd0cc43
fix peers handler test
pascal-fischer Nov 26, 2025
3fc8c8b
reconfigure redis cache
pascal-fischer Nov 27, 2025
7b3b588
fix key send
pascal-fischer Nov 27, 2025
f2a5e25
Merge branch 'main' into refactor/network-map-controller
pascal-fischer Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions client/cmd/testutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import (

"github.com/netbirdio/netbird/management/internals/controllers/network_map/controller"
"github.com/netbirdio/netbird/management/internals/controllers/network_map/update_channel"
"github.com/netbirdio/netbird/management/internals/modules/peers"
"github.com/netbirdio/netbird/management/internals/modules/peers/ephemeral/manager"
nbgrpc "github.com/netbirdio/netbird/management/internals/shared/grpc"

clientProto "github.com/netbirdio/netbird/client/proto"
Expand All @@ -24,8 +26,6 @@ import (
"github.com/netbirdio/netbird/management/server/activity"
"github.com/netbirdio/netbird/management/server/groups"
"github.com/netbirdio/netbird/management/server/integrations/port_forwarding"
"github.com/netbirdio/netbird/management/server/peers"
"github.com/netbirdio/netbird/management/server/peers/ephemeral/manager"
"github.com/netbirdio/netbird/management/server/permissions"
"github.com/netbirdio/netbird/management/server/settings"
"github.com/netbirdio/netbird/management/server/store"
Expand Down Expand Up @@ -116,15 +116,18 @@ func startManagement(t *testing.T, config *config.Config, testFile string) (*grp
ctx := context.Background()
updateManager := update_channel.NewPeersUpdateManager(metrics)
requestBuffer := mgmt.NewAccountRequestBuffer(ctx, store)
networkMapController := controller.NewController(ctx, store, metrics, updateManager, requestBuffer, mgmt.MockIntegratedValidator{}, settingsMockManager, "netbird.cloud", port_forwarding.NewControllerMock(), config)
networkMapController := controller.NewController(ctx, store, metrics, updateManager, requestBuffer, mgmt.MockIntegratedValidator{}, settingsMockManager, "netbird.cloud", port_forwarding.NewControllerMock(), manager.NewEphemeralManager(store, peersmanager), config)

accountManager, err := mgmt.BuildManager(context.Background(), config, store, networkMapController, nil, "", eventStore, nil, false, iv, metrics, port_forwarding.NewControllerMock(), settingsMockManager, permissionsManagerMock, false)
if err != nil {
t.Fatal(err)
}

secretsManager := nbgrpc.NewTimeBasedAuthSecretsManager(updateManager, config.TURNConfig, config.Relay, settingsMockManager, groupsManager)
mgmtServer, err := nbgrpc.NewServer(config, accountManager, settingsMockManager, updateManager, secretsManager, nil, &manager.EphemeralManager{}, nil, &mgmt.MockIntegratedValidator{}, networkMapController)
secretsManager, err := nbgrpc.NewTimeBasedAuthSecretsManager(updateManager, config.TURNConfig, config.Relay, settingsMockManager, groupsManager)
if err != nil {
t.Fatal(err)
}
mgmtServer, err := nbgrpc.NewServer(config, accountManager, settingsMockManager, secretsManager, nil, nil, &mgmt.MockIntegratedValidator{}, networkMapController)
if err != nil {
t.Fatal(err)
}
Expand Down
13 changes: 8 additions & 5 deletions client/internal/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ import (

"github.com/netbirdio/netbird/management/internals/controllers/network_map/controller"
"github.com/netbirdio/netbird/management/internals/controllers/network_map/update_channel"
"github.com/netbirdio/netbird/management/internals/modules/peers"
"github.com/netbirdio/netbird/management/internals/modules/peers/ephemeral/manager"
nbgrpc "github.com/netbirdio/netbird/management/internals/shared/grpc"

"github.com/netbirdio/netbird/management/internals/server/config"
"github.com/netbirdio/netbird/management/server/groups"
"github.com/netbirdio/netbird/management/server/peers/ephemeral/manager"

"github.com/netbirdio/netbird/client/iface"
"github.com/netbirdio/netbird/client/iface/configurer"
Expand All @@ -54,7 +55,6 @@ import (
"github.com/netbirdio/netbird/management/server"
"github.com/netbirdio/netbird/management/server/activity"
"github.com/netbirdio/netbird/management/server/integrations/port_forwarding"
"github.com/netbirdio/netbird/management/server/peers"
"github.com/netbirdio/netbird/management/server/permissions"
"github.com/netbirdio/netbird/management/server/settings"
"github.com/netbirdio/netbird/management/server/store"
Expand Down Expand Up @@ -1628,14 +1628,17 @@ func startManagement(t *testing.T, dataDir, testFile string) (*grpc.Server, stri

updateManager := update_channel.NewPeersUpdateManager(metrics)
requestBuffer := server.NewAccountRequestBuffer(context.Background(), store)
networkMapController := controller.NewController(context.Background(), store, metrics, updateManager, requestBuffer, server.MockIntegratedValidator{}, settingsMockManager, "netbird.selfhosted", port_forwarding.NewControllerMock(), config)
networkMapController := controller.NewController(context.Background(), store, metrics, updateManager, requestBuffer, server.MockIntegratedValidator{}, settingsMockManager, "netbird.selfhosted", port_forwarding.NewControllerMock(), manager.NewEphemeralManager(store, peersManager), config)
accountManager, err := server.BuildManager(context.Background(), config, store, networkMapController, nil, "", eventStore, nil, false, ia, metrics, port_forwarding.NewControllerMock(), settingsMockManager, permissionsManager, false)
if err != nil {
return nil, "", err
}

secretsManager := nbgrpc.NewTimeBasedAuthSecretsManager(updateManager, config.TURNConfig, config.Relay, settingsMockManager, groupsManager)
mgmtServer, err := nbgrpc.NewServer(config, accountManager, settingsMockManager, updateManager, secretsManager, nil, &manager.EphemeralManager{}, nil, &server.MockIntegratedValidator{}, networkMapController)
secretsManager, err := nbgrpc.NewTimeBasedAuthSecretsManager(updateManager, config.TURNConfig, config.Relay, settingsMockManager, groupsManager)
if err != nil {
return nil, "", err
}
mgmtServer, err := nbgrpc.NewServer(config, accountManager, settingsMockManager, secretsManager, nil, nil, &server.MockIntegratedValidator{}, networkMapController)
if err != nil {
return nil, "", err
}
Expand Down
13 changes: 8 additions & 5 deletions client/server/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ import (

"github.com/netbirdio/netbird/management/internals/controllers/network_map/controller"
"github.com/netbirdio/netbird/management/internals/controllers/network_map/update_channel"
"github.com/netbirdio/netbird/management/internals/modules/peers"
"github.com/netbirdio/netbird/management/internals/modules/peers/ephemeral/manager"
nbgrpc "github.com/netbirdio/netbird/management/internals/shared/grpc"

"github.com/netbirdio/netbird/management/internals/server/config"
"github.com/netbirdio/netbird/management/server/groups"
"github.com/netbirdio/netbird/management/server/peers/ephemeral/manager"

log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
Expand All @@ -35,7 +36,6 @@ import (
"github.com/netbirdio/netbird/management/server"
"github.com/netbirdio/netbird/management/server/activity"
"github.com/netbirdio/netbird/management/server/integrations/port_forwarding"
"github.com/netbirdio/netbird/management/server/peers"
"github.com/netbirdio/netbird/management/server/permissions"
"github.com/netbirdio/netbird/management/server/settings"
"github.com/netbirdio/netbird/management/server/store"
Expand Down Expand Up @@ -316,14 +316,17 @@ func startManagement(t *testing.T, signalAddr string, counter *int) (*grpc.Serve

requestBuffer := server.NewAccountRequestBuffer(context.Background(), store)
peersUpdateManager := update_channel.NewPeersUpdateManager(metrics)
networkMapController := controller.NewController(context.Background(), store, metrics, peersUpdateManager, requestBuffer, server.MockIntegratedValidator{}, settingsMockManager, "netbird.selfhosted", port_forwarding.NewControllerMock(), config)
networkMapController := controller.NewController(context.Background(), store, metrics, peersUpdateManager, requestBuffer, server.MockIntegratedValidator{}, settingsMockManager, "netbird.selfhosted", port_forwarding.NewControllerMock(), manager.NewEphemeralManager(store, peersManager), config)
accountManager, err := server.BuildManager(context.Background(), config, store, networkMapController, nil, "", eventStore, nil, false, ia, metrics, port_forwarding.NewControllerMock(), settingsMockManager, permissionsManagerMock, false)
if err != nil {
return nil, "", err
}

secretsManager := nbgrpc.NewTimeBasedAuthSecretsManager(peersUpdateManager, config.TURNConfig, config.Relay, settingsMockManager, groupsManager)
mgmtServer, err := nbgrpc.NewServer(config, accountManager, settingsMockManager, peersUpdateManager, secretsManager, nil, &manager.EphemeralManager{}, nil, &server.MockIntegratedValidator{}, networkMapController)
secretsManager, err := nbgrpc.NewTimeBasedAuthSecretsManager(peersUpdateManager, config.TURNConfig, config.Relay, settingsMockManager, groupsManager)
if err != nil {
return nil, "", err
}
mgmtServer, err := nbgrpc.NewServer(config, accountManager, settingsMockManager, secretsManager, nil, nil, &server.MockIntegratedValidator{}, networkMapController)
if err != nil {
return nil, "", err
}
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ require (
github.com/mdlayher/socket v0.5.1
github.com/miekg/dns v1.1.59
github.com/mitchellh/hashstructure/v2 v2.0.2
github.com/netbirdio/management-integrations/integrations v0.0.0-20251027212525-d751b79f5d48
github.com/netbirdio/management-integrations/integrations v0.0.0-20251114143509-4eff2374da63
github.com/netbirdio/signal-dispatcher/dispatcher v0.0.0-20250805121659-6b4ac470ca45
github.com/okta/okta-sdk-golang/v2 v2.18.0
github.com/oschwald/maxminddb-golang v1.12.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,8 @@ github.com/netbirdio/go-netroute v0.0.0-20240611143515-f59b0e1d3944 h1:TDtJKmM6S
github.com/netbirdio/go-netroute v0.0.0-20240611143515-f59b0e1d3944/go.mod h1:sHA6TRxjQ6RLbnI+3R4DZo2Eseg/iKiPRfNmcuNySVQ=
github.com/netbirdio/ice/v4 v4.0.0-20250908184934-6202be846b51 h1:Ov4qdafATOgGMB1wbSuh+0aAHcwz9hdvB6VZjh1mVMI=
github.com/netbirdio/ice/v4 v4.0.0-20250908184934-6202be846b51/go.mod h1:ZSIbPdBn5hePO8CpF1PekH2SfpTxg1PDhEwtbqZS7R8=
github.com/netbirdio/management-integrations/integrations v0.0.0-20251027212525-d751b79f5d48 h1:moJbL1uuaWR35yUgHZ6suijjqqW8/qGCuPPBXu5MeWQ=
github.com/netbirdio/management-integrations/integrations v0.0.0-20251027212525-d751b79f5d48/go.mod h1:ifKa2jGPsOzZhJFo72v2AE5nMP3GYvlhoZ9JV6lHlJ8=
github.com/netbirdio/management-integrations/integrations v0.0.0-20251114143509-4eff2374da63 h1:ecs4GMANgObopiy29zMmz2dIdOTJMwezUbrFy+zfSwE=
github.com/netbirdio/management-integrations/integrations v0.0.0-20251114143509-4eff2374da63/go.mod h1:JIWpjbCgDvZIt45C9vYpikU2gRXeDWrN7SiyGYd3Qrc=
github.com/netbirdio/service v0.0.0-20240911161631-f62744f42502 h1:3tHlFmhTdX9axERMVN63dqyFqnvuD+EMJHzM7mNGON8=
github.com/netbirdio/service v0.0.0-20240911161631-f62744f42502/go.mod h1:CIMRFEJVL+0DS1a3Nx06NaMn4Dz63Ng6O7dl0qH0zVM=
github.com/netbirdio/signal-dispatcher/dispatcher v0.0.0-20250805121659-6b4ac470ca45 h1:ujgviVYmx243Ksy7NdSwrdGPSRNE3pb8kEDSpH0QuAQ=
Expand Down
160 changes: 100 additions & 60 deletions management/internals/controllers/network_map/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
nbdns "github.com/netbirdio/netbird/dns"
"github.com/netbirdio/netbird/management/internals/controllers/network_map"
"github.com/netbirdio/netbird/management/internals/controllers/network_map/controller/cache"
"github.com/netbirdio/netbird/management/internals/modules/peers/ephemeral"
"github.com/netbirdio/netbird/management/internals/server/config"
"github.com/netbirdio/netbird/management/internals/shared/grpc"
"github.com/netbirdio/netbird/management/server/account"
Expand All @@ -42,6 +43,7 @@ type Controller struct {
accountManagerMetrics *telemetry.AccountManagerMetrics
peersUpdateManager network_map.PeersUpdateManager
settingsManager settings.Manager
EphemeralPeersManager ephemeral.Manager

accountUpdateLocks sync.Map
sendAccountUpdateLocks sync.Map
Expand Down Expand Up @@ -70,7 +72,7 @@ type bufferUpdate struct {

var _ network_map.Controller = (*Controller)(nil)

func NewController(ctx context.Context, store store.Store, metrics telemetry.AppMetrics, peersUpdateManager network_map.PeersUpdateManager, requestBuffer account.RequestBuffer, integratedPeerValidator integrated_validator.IntegratedValidator, settingsManager settings.Manager, dnsDomain string, proxyController port_forwarding.Controller, config *config.Config) *Controller {
func NewController(ctx context.Context, store store.Store, metrics telemetry.AppMetrics, peersUpdateManager network_map.PeersUpdateManager, requestBuffer account.RequestBuffer, integratedPeerValidator integrated_validator.IntegratedValidator, settingsManager settings.Manager, dnsDomain string, proxyController port_forwarding.Controller, ephemeralPeersManager ephemeral.Manager, config *config.Config) *Controller {
nMetrics, err := newMetrics(metrics.UpdateChannelMetrics())
if err != nil {
log.Fatal(fmt.Errorf("error creating metrics: %w", err))
Expand Down Expand Up @@ -99,14 +101,40 @@ func NewController(ctx context.Context, store store.Store, metrics telemetry.App
dnsDomain: dnsDomain,
config: config,

proxyController: proxyController,
proxyController: proxyController,
EphemeralPeersManager: ephemeralPeersManager,

holder: types.NewHolder(),
expNewNetworkMap: newNetworkMapBuilder,
expNewNetworkMapAIDs: expIDs,
}
}

func (c *Controller) OnPeerConnected(ctx context.Context, accountID string, peerID string) (chan *network_map.UpdateMessage, error) {
peer, err := c.repo.GetPeerByID(ctx, accountID, peerID)
if err != nil {
return nil, fmt.Errorf("failed to get peer %s: %v", peerID, err)
}

c.EphemeralPeersManager.OnPeerConnected(ctx, peer)

return c.peersUpdateManager.CreateChannel(ctx, peerID), nil
}

func (c *Controller) OnPeerDisconnected(ctx context.Context, accountID string, peerID string) {
c.peersUpdateManager.CloseChannel(ctx, peerID)
peer, err := c.repo.GetPeerByID(ctx, accountID, peerID)
if err != nil {
log.WithContext(ctx).Errorf("failed to get peer %s: %v", peerID, err)
return
}
c.EphemeralPeersManager.OnPeerDisconnected(ctx, peer)
}

func (c *Controller) CountStreams() int {
return c.peersUpdateManager.CountStreams()
}
Comment on lines +113 to +136
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Guard ephemeral manager usage in OnPeerConnected/OnPeerDisconnected

The overall flow of the new lifecycle hooks looks good:

  • Lookup peer by (accountID, peerID).
  • Notify EphemeralPeersManager of connect/disconnect.
  • Manage per‑peer update channels via PeersUpdateManager.
  • Surface a chan *UpdateMessage from OnPeerConnected.

Two points to tighten:

  1. Nil guard for EphemeralPeersManager

Assuming the field can be nil in any configuration or tests, both methods will panic. A lightweight guard would make this safer:

 func (c *Controller) OnPeerConnected(ctx context.Context, accountID string, peerID string) (chan *network_map.UpdateMessage, error) {
 	peer, err := c.repo.GetPeerByID(ctx, accountID, peerID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get peer %s: %v", peerID, err)
 	}

-	c.EphemeralPeersManager.OnPeerConnected(ctx, peer)
+	if c.EphemeralPeersManager != nil {
+		c.EphemeralPeersManager.OnPeerConnected(ctx, peer)
+	}

 	return c.peersUpdateManager.CreateChannel(ctx, peerID), nil
 }
 
 func (c *Controller) OnPeerDisconnected(ctx context.Context, accountID string, peerID string) {
 	c.peersUpdateManager.CloseChannel(ctx, peerID)
 	peer, err := c.repo.GetPeerByID(ctx, accountID, peerID)
 	if err != nil {
 		log.WithContext(ctx).Errorf("failed to get peer %s: %v", peerID, err)
 		return
 	}
-	c.EphemeralPeersManager.OnPeerDisconnected(ctx, peer)
+	if c.EphemeralPeersManager != nil {
+		c.EphemeralPeersManager.OnPeerDisconnected(ctx, peer)
+	}
 }
  1. Consistency of error semantics

OnPeerConnected fails fast on repo lookup errors, which is good. OnPeerDisconnected logs and returns on lookup failure, which is also reasonable because the channel is already closed and the peer may legitimately be gone. The behavior is consistent; just be aware that the ephemeral manager may not see a disconnect in that corner case.

I'll verify this review comment by inspecting the codebase to determine if EphemeralPeersManager can actually be nil and whether nil guards are necessary.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find the Controller struct definition and EphemeralPeersManager field
fd -e go --exec grep -l "type Controller struct" {} \; | head -5

Length of output: 71

🏁 Script executed:

#!/bin/bash
# Search for EphemeralPeersManager field declaration
rg -n "EphemeralPeersManager\s*\w+" --type go -A 2 -B 2 | head -50

Length of output: 543

🏁 Script executed:

#!/bin/bash
# Find Controller initialization functions
rg -n "func.*New.*Controller|func.*newController" --type go -A 10 | head -80

Length of output: 7165

🏁 Script executed:

#!/bin/bash
# Search for all usages of EphemeralPeersManager in the codebase
rg -n "EphemeralPeersManager" --type go -C 2 | head -100

Length of output: 1985


---

🏁 Script executed:

```shell
#!/bin/bash
# Find ephemeral.Manager type definition
rg -n "type Manager" --type go -A 3 management/internals/ | grep -A 3 "ephemeral"

Length of output: 372

🏁 Script executed:

#!/bin/bash
# Search for all NewController callers
rg -n "NewController\(" --type go -B 2 -A 2

Length of output: 26310

🏁 Script executed:

#!/bin/bash
# Check test files for EphemeralPeersManager usage or nil patterns
find . -name "*test.go" -type f -exec rg -l "EphemeralPeersManager\|ephemeralPeersManager" {} \;

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Look for where ephemeral.Manager is defined
fd -e go -path "*/ephemeral*" --exec grep -l "type Manager" {} \;

Length of output: 168


---

🏁 Script executed:

```shell
#!/bin/bash
# Find BaseServer.EphemeralManager() method implementation
rg -n "func.*EphemeralManager" --type go -A 5 | head -40

Length of output: 3585

🏁 Script executed:

#!/bin/bash
# Check if there are any nil checks for EphemeralManager elsewhere in the codebase
rg -n "EphemeralManager.*nil\|nil.*EphemeralManager" --type go

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check the full controller struct definition and field visibility
rg -n "type Controller struct" --type go -A 20 management/internals/controllers/network_map/controller/controller.go

Length of output: 798

Add nil guards for EphemeralPeersManager in lifecycle methods

The review comment is valid. While all current code paths initialize EphemeralPeersManager as non-nil, the field is exported and can theoretically become nil if:

  • External code bypasses the constructor and sets it directly
  • An undocumented edge case in BaseServer.EphemeralManager() returns nil

Without guards, both OnPeerConnected (line 119) and OnPeerDisconnected (line 131) will panic. Adding nil checks is sound defensive programming for exported interface fields.

The suggested diffs in the review are appropriate:

  • OnPeerConnected should guard before calling c.EphemeralPeersManager.OnPeerConnected()
  • OnPeerDisconnected should guard before calling c.EphemeralPeersManager.OnPeerDisconnected()
🤖 Prompt for AI Agents 
In management/internals/controllers/network_map/controller/controller.go around
lines 113 to 136, add nil guards for the exported EphemeralPeersManager before
invoking its methods to avoid panics: in OnPeerConnected check if
c.EphemeralPeersManager != nil before calling OnPeerConnected (still return the
peersUpdateManager channel as before if the manager is nil), and in
OnPeerDisconnected check if c.EphemeralPeersManager != nil before calling
OnPeerDisconnected (optionally log a debug/warn when it is nil); leave all other
behavior unchanged.


func (c *Controller) sendUpdateAccountPeers(ctx context.Context, accountID string) error {
log.WithContext(ctx).Tracef("updating peers for account %s from %s", accountID, util.GetCallerName())
var (
Expand Down Expand Up @@ -366,38 +394,6 @@ func (c *Controller) BufferUpdateAccountPeers(ctx context.Context, accountID str
return nil
}

func (c *Controller) DeletePeer(ctx context.Context, accountId string, peerId string) error {
network, err := c.repo.GetAccountNetwork(ctx, accountId)
if err != nil {
return err
}

peers, err := c.repo.GetAccountPeers(ctx, accountId)
if err != nil {
return err
}

dnsFwdPort := computeForwarderPort(peers, network_map.DnsForwarderPortMinVersion)
c.peersUpdateManager.SendUpdate(ctx, peerId, &network_map.UpdateMessage{
Update: &proto.SyncResponse{
RemotePeers: []*proto.RemotePeerConfig{},
RemotePeersIsEmpty: true,
NetworkMap: &proto.NetworkMap{
Serial: network.CurrentSerial(),
RemotePeers: []*proto.RemotePeerConfig{},
RemotePeersIsEmpty: true,
FirewallRules: []*proto.FirewallRule{},
FirewallRulesIsEmpty: true,
DNSConfig: &proto.DNSConfig{
ForwarderPort: dnsFwdPort,
},
},
},
})
c.peersUpdateManager.CloseChannel(ctx, peerId)
return nil
}

func (c *Controller) GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID string, peer *nbpeer.Peer) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) {
if isRequiresApproval {
network, err := c.repo.GetAccountNetwork(ctx, accountID)
Expand Down Expand Up @@ -698,35 +694,83 @@ func isPeerInPolicySourceGroups(account *types.Account, peerID string, policy *t
return false, nil
}

func (c *Controller) OnPeerUpdated(accountId string, peer *nbpeer.Peer) {
c.UpdatePeerInNetworkMapCache(accountId, peer)
_ = c.bufferSendUpdateAccountPeers(context.Background(), accountId)
func (c *Controller) OnPeersUpdated(ctx context.Context, accountID string, peerIDs []string) error {
peers, err := c.repo.GetPeersByIDs(ctx, accountID, peerIDs)
if err != nil {
return fmt.Errorf("failed to get peers by ids: %w", err)
}

for _, peer := range peers {
c.UpdatePeerInNetworkMapCache(accountID, peer)
}

err = c.bufferSendUpdateAccountPeers(ctx, accountID)
if err != nil {
log.WithContext(ctx).Errorf("failed to buffer update account peers for peer update in account %s: %v", accountID, err)
}

return nil
}
Comment on lines +697 to 713
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

OnPeersUpdated swallows buffer errors, diverging from other handlers

OnPeersUpdated correctly reloads peers and updates the cache, but it explicitly ignores errors from bufferSendUpdateAccountPeers:

err = c.bufferSendUpdateAccountPeers(ctx, accountID)
if err != nil {
	log.WithContext(ctx).Errorf("failed to buffer update account peers for peer update in account %s: %v", accountID, err)
}

return nil

Both OnPeersAdded and OnPeersDeleted propagate bufferSendUpdateAccountPeers’s error to the caller, while OnPeersUpdated hides it. This can lead to silent failures perceived as success by the caller.

Consider aligning behavior with the other methods by returning the error (you can keep logging if you want):

-	err = c.bufferSendUpdateAccountPeers(ctx, accountID)
-	if err != nil {
-		log.WithContext(ctx).Errorf("failed to buffer update account peers for peer update in account %s: %v", accountID, err)
-	}
-
-	return nil
+	if err := c.bufferSendUpdateAccountPeers(ctx, accountID); err != nil {
+		log.WithContext(ctx).Errorf("failed to buffer update account peers for peer update in account %s: %v", accountID, err)
+		return fmt.Errorf("buffer update account peers for account %s: %w", accountID, err)
+	}
+	return nil
🤖 Prompt for AI Agents 
In management/internals/controllers/network_map/controller/controller.go around
lines 697 to 713, OnPeersUpdated currently calls bufferSendUpdateAccountPeers
and only logs errors instead of returning them, diverging from
OnPeersAdded/OnPeersDeleted; change the function so that after calling
bufferSendUpdateAccountPeers it returns the error (optionally still logging it)
instead of always returning nil, ensuring the error is propagated to the caller
like the other handlers.


func (c *Controller) OnPeerAdded(ctx context.Context, accountID string, peerID string) error {
if c.experimentalNetworkMap(accountID) {
account, err := c.requestBuffer.GetAccountWithBackpressure(ctx, accountID)
if err != nil {
return err
}
func (c *Controller) OnPeersAdded(ctx context.Context, accountID string, peerIDs []string) error {
for _, peerID := range peerIDs {
if c.experimentalNetworkMap(accountID) {
account, err := c.requestBuffer.GetAccountWithBackpressure(ctx, accountID)
if err != nil {
return err
}

err = c.onPeerAddedUpdNetworkMapCache(account, peerID)
if err != nil {
return err
err = c.onPeerAddedUpdNetworkMapCache(account, peerID)
if err != nil {
return err
}
}
}
return c.bufferSendUpdateAccountPeers(ctx, accountID)
}

func (c *Controller) OnPeerDeleted(ctx context.Context, accountID string, peerID string) error {
if c.experimentalNetworkMap(accountID) {
account, err := c.requestBuffer.GetAccountWithBackpressure(ctx, accountID)
if err != nil {
return err
}
err = c.onPeerDeletedUpdNetworkMapCache(account, peerID)
if err != nil {
return err
func (c *Controller) OnPeersDeleted(ctx context.Context, accountID string, peerIDs []string) error {
network, err := c.repo.GetAccountNetwork(ctx, accountID)
if err != nil {
return err
}

peers, err := c.repo.GetAccountPeers(ctx, accountID)
if err != nil {
return err
}

dnsFwdPort := computeForwarderPort(peers, network_map.DnsForwarderPortMinVersion)
for _, peerID := range peerIDs {
c.peersUpdateManager.SendUpdate(ctx, peerID, &network_map.UpdateMessage{
Update: &proto.SyncResponse{
RemotePeers: []*proto.RemotePeerConfig{},
RemotePeersIsEmpty: true,
NetworkMap: &proto.NetworkMap{
Serial: network.CurrentSerial(),
RemotePeers: []*proto.RemotePeerConfig{},
RemotePeersIsEmpty: true,
FirewallRules: []*proto.FirewallRule{},
FirewallRulesIsEmpty: true,
DNSConfig: &proto.DNSConfig{
ForwarderPort: dnsFwdPort,
},
},
},
})
c.peersUpdateManager.CloseChannel(ctx, peerID)

if c.experimentalNetworkMap(accountID) {
account, err := c.requestBuffer.GetAccountWithBackpressure(ctx, accountID)
if err != nil {
log.WithContext(ctx).Errorf("failed to get account %s: %v", accountID, err)
continue
}
err = c.onPeerDeletedUpdNetworkMapCache(account, peerID)
if err != nil {
log.WithContext(ctx).Errorf("failed to update network map cache for deleted peer %s in account %s: %v", peerID, accountID, err)
continue
}
}
}

Expand Down Expand Up @@ -778,10 +822,6 @@ func (c *Controller) GetNetworkMap(ctx context.Context, peerID string) (*types.N
return networkMap, nil
}

func (c *Controller) DisconnectPeers(ctx context.Context, peerIDs []string) {
func (c *Controller) DisconnectPeers(ctx context.Context, accountId string, peerIDs []string) {
c.peersUpdateManager.CloseChannels(ctx, peerIDs)
}

func (c *Controller) IsConnected(peerID string) bool {
return c.peersUpdateManager.HasChannel(peerID)
}
Loading
Loading