From 67c2041fb5534c19d7b2320617cfc3aaab8e7d3d Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:40:17 +0200 Subject: [PATCH] [management] update proxy mappings on peer IP change (#8139) --- .../reverseproxy/service/manager/manager.go | 10 ++- .../service/manager/manager_test.go | 40 +++++++++ management/server/account.go | 15 ++++ management/server/account_test.go | 81 +++++++++++++++++++ 4 files changed, 142 insertions(+), 4 deletions(-) diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index 900b7759f..d1ed8d078 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -2,6 +2,7 @@ package manager import ( "context" + "errors" "fmt" "math/rand/v2" "net" @@ -1079,15 +1080,16 @@ func (m *Manager) ReloadAllServicesForAccount(ctx context.Context, accountID str oidcCfg := m.proxyController.GetOIDCValidationConfig() + var errs []error for _, s := range services { - err = m.replaceHostByLookup(ctx, accountID, s) - if err != nil { - return fmt.Errorf("failed to replace host by lookup for service %s: %w", s.ID, err) + if err := m.replaceHostByLookup(ctx, accountID, s); err != nil { + errs = append(errs, fmt.Errorf("replace host by lookup for service %s: %w", s.ID, err)) + continue } m.proxyController.SendServiceUpdateToCluster(ctx, accountID, s.ToProtoMapping(service.Update, "", oidcCfg), s.ProxyCluster) } - return nil + return errors.Join(errs...) } func (m *Manager) GetGlobalServices(ctx context.Context) ([]*service.Service, error) { diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index 2ec1af1a2..f9273d594 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -27,6 +27,7 @@ import ( "github.com/netbirdio/netbird/management/server/permissions/operations" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/shared/management/status" ) @@ -1481,3 +1482,42 @@ func TestReplaceHostByLookup_SkipsClusterTarget(t *testing.T) { require.NoError(t, mgr.replaceHostByLookup(ctx, accountID, svc), "cluster target must not trigger peer/resource lookup") assert.Equal(t, "127.0.0.1", svc.Targets[0].Host, "operator-supplied host must be preserved for cluster target") } + +func TestReloadAllServicesForAccount_ContinuesAfterFailedService(t *testing.T) { + ctx := context.Background() + ctrl := gomock.NewController(t) + mockStore := store.NewMockStore(ctrl) + proxyController := proxy.NewMockController(ctrl) + accountID := "test-account" + + broken := &rpservice.Service{ + ID: "svc-broken", + AccountID: accountID, + ProxyCluster: "eu.proxy.netbird.io", + Targets: []*rpservice.Target{{TargetId: "x", TargetType: "unknown"}}, + } + healthy := &rpservice.Service{ + ID: "svc-healthy", + AccountID: accountID, + ProxyCluster: "eu.proxy.netbird.io", + Targets: []*rpservice.Target{{TargetId: "peer-1", TargetType: rpservice.TargetTypePeer, Host: "100.64.0.1", Protocol: "http", Port: 8080, Enabled: true}}, + } + + mockStore.EXPECT().GetAccountServices(ctx, store.LockingStrengthNone, accountID).Return([]*rpservice.Service{broken, healthy}, nil) + mockStore.EXPECT().GetPeerByID(ctx, store.LockingStrengthNone, accountID, "peer-1"). + Return(&nbpeer.Peer{ID: "peer-1", IP: netip.MustParseAddr("100.70.0.9")}, nil) + proxyController.EXPECT().GetOIDCValidationConfig().Return(proxy.OIDCValidationConfig{}) + proxyController.EXPECT().SendServiceUpdateToCluster(ctx, accountID, gomock.Any(), "eu.proxy.netbird.io"). + Do(func(_ context.Context, _ string, mapping *proto.ProxyMapping, _ string) { + assert.Equal(t, "svc-healthy", mapping.GetId()) + require.Len(t, mapping.GetPath(), 1) + assert.Contains(t, mapping.GetPath()[0].GetTarget(), "100.70.0.9", "mapping must carry the peer's current IP") + }) + + mgr := &Manager{store: mockStore, proxyController: proxyController} + + err := mgr.ReloadAllServicesForAccount(ctx, accountID) + require.Error(t, err) + assert.Contains(t, err.Error(), "svc-broken") + assert.NotContains(t, err.Error(), "svc-healthy") +} diff --git a/management/server/account.go b/management/server/account.go index 4003e7e7b..6de1642e5 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -2706,6 +2706,8 @@ func (am *DefaultAccountManager) UpdatePeerIP(ctx context.Context, accountID, us } if updateNetworkMap { + am.reloadServicesTargetingPeer(ctx, accountID, peerID) + peer, err := am.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peerID) if err != nil { return err @@ -2720,6 +2722,19 @@ func (am *DefaultAccountManager) UpdatePeerIP(ctx context.Context, accountID, us return nil } +// reloadServicesTargetingPeer resends the account's reverse proxy services unless none of them targets the peer. +func (am *DefaultAccountManager) reloadServicesTargetingPeer(ctx context.Context, accountID, peerID string) { + serviceID, err := am.serviceManager.GetServiceIDByTargetID(ctx, accountID, peerID) + if err != nil { + log.WithContext(ctx).Warnf("failed to look up services targeting peer %s, reloading all: %v", peerID, err) + } else if serviceID == "" { + return + } + if err := am.serviceManager.ReloadAllServicesForAccount(ctx, accountID); err != nil { + log.WithContext(ctx).Warnf("failed to reload services for account %s: %v", accountID, err) + } +} + func (am *DefaultAccountManager) updatePeerIPInTransaction(ctx context.Context, accountID, userID, peerID string, newIP netip.Addr) (bool, error) { var updateNetworkMap bool err := am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { diff --git a/management/server/account_test.go b/management/server/account_test.go index 7dc02b428..d3ad2dfc9 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -3,6 +3,7 @@ package server import ( "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -4460,6 +4461,86 @@ func TestDefaultAccountManager_UpdatePeerIP(t *testing.T) { }) } +func TestDefaultAccountManager_UpdatePeerIP_ReloadsTargetingServices(t *testing.T) { + manager, _, err := createManager(t) + require.NoError(t, err, "unable to create account manager") + + ctx := context.Background() + accountID, err := manager.GetAccountIDByUserID(ctx, auth.UserAuth{UserId: userID}) + require.NoError(t, err, "unable to create an account") + + key, err := wgtypes.GenerateKey() + require.NoError(t, err, "unable to generate WireGuard key") + peer, _, _, _, err := manager.AddPeer(ctx, "", "", userID, &nbpeer.Peer{ + Key: key.PublicKey().String(), + Meta: nbpeer.PeerSystemMeta{Hostname: "proxied-peer"}, + }, false) + require.NoError(t, err, "unable to add peer") + + account, err := manager.Store.GetAccount(ctx, accountID) + require.NoError(t, err, "unable to get account") + network := netip.MustParsePrefix(account.Network.Net.String()) + + t.Run("targeted peer reloads services", func(t *testing.T) { + serviceManager := service.NewMockManager(gomock.NewController(t)) + manager.SetServiceManager(serviceManager) + serviceManager.EXPECT().GetServiceIDByTargetID(gomock.Any(), accountID, peer.ID).Return("svc-1", nil) + serviceManager.EXPECT().ReloadAllServicesForAccount(gomock.Any(), accountID).Return(nil) + + current, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peer.ID) + require.NoError(t, err) + newIP, err := types.AllocatePeerIP(network, []netip.Addr{current.IP}) + require.NoError(t, err) + require.NoError(t, manager.UpdatePeerIP(ctx, accountID, userID, peer.ID, newIP)) + }) + + t.Run("untargeted peer skips reload", func(t *testing.T) { + serviceManager := service.NewMockManager(gomock.NewController(t)) + manager.SetServiceManager(serviceManager) + serviceManager.EXPECT().GetServiceIDByTargetID(gomock.Any(), accountID, peer.ID).Return("", nil) + + current, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peer.ID) + require.NoError(t, err) + newIP, err := types.AllocatePeerIP(network, []netip.Addr{current.IP}) + require.NoError(t, err) + require.NoError(t, manager.UpdatePeerIP(ctx, accountID, userID, peer.ID, newIP)) + }) + + t.Run("lookup failure reloads services", func(t *testing.T) { + serviceManager := service.NewMockManager(gomock.NewController(t)) + manager.SetServiceManager(serviceManager) + serviceManager.EXPECT().GetServiceIDByTargetID(gomock.Any(), accountID, peer.ID).Return("", errors.New("store unavailable")) + serviceManager.EXPECT().ReloadAllServicesForAccount(gomock.Any(), accountID).Return(nil) + + current, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peer.ID) + require.NoError(t, err) + newIP, err := types.AllocatePeerIP(network, []netip.Addr{current.IP}) + require.NoError(t, err) + require.NoError(t, manager.UpdatePeerIP(ctx, accountID, userID, peer.ID, newIP)) + }) + + t.Run("network map notification failure still reloads services", func(t *testing.T) { + ctrl := gomock.NewController(t) + serviceManager := service.NewMockManager(ctrl) + manager.SetServiceManager(serviceManager) + serviceManager.EXPECT().GetServiceIDByTargetID(gomock.Any(), accountID, peer.ID).Return("svc-1", nil) + serviceManager.EXPECT().ReloadAllServicesForAccount(gomock.Any(), accountID).Return(nil) + + originalController := manager.networkMapController + t.Cleanup(func() { manager.networkMapController = originalController }) + networkMapController := network_map.NewMockController(ctrl) + networkMapController.EXPECT().GetDNSDomain(gomock.Any()).Return("netbird.cloud").AnyTimes() + networkMapController.EXPECT().OnPeersUpdated(gomock.Any(), accountID, []string{peer.ID}, gomock.Any()).Return(errors.New("notify failed")) + manager.networkMapController = networkMapController + + current, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, accountID, peer.ID) + require.NoError(t, err) + newIP, err := types.AllocatePeerIP(network, []netip.Addr{current.IP}) + require.NoError(t, err) + require.Error(t, manager.UpdatePeerIP(ctx, accountID, userID, peer.ID, newIP)) + }) +} + func TestAddNewUserToDomainAccountWithApproval(t *testing.T) { manager, _, err := createManager(t) if err != nil {