diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index ba2c06826..8c43d5748 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -22,7 +22,6 @@ import ( "github.com/netbirdio/netbird/management/server/permissions/modules" "github.com/netbirdio/netbird/management/server/permissions/operations" "github.com/netbirdio/netbird/management/server/store" - "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/shared/management/status" ) @@ -123,11 +122,10 @@ type managerImpl struct { proxyController proxy.Controller // reconcileCache holds the last set of synthesised proxy mappings - // per account so reconcile can emit precise Create/Update/Delete - // updates instead of a full re-push on every mutation. Keyed by - // accountID, then by synthesised service ID. + // per account, each paired with the proxy that served it, so a change + // of serving proxy can be diffed without re-deriving it. reconcileMu sync.Mutex - reconcileCache map[string]map[string]*proto.ProxyMapping + reconcileCache map[string]map[string]syntheticMapping // labelRngMu guards labelRng. PickUnique consumes math/rand.Source // state; concurrent provider creates would otherwise race. @@ -151,7 +149,7 @@ func NewManager( accountManager: accountManager, permissionsManager: permissionsManager, proxyController: proxyController, - reconcileCache: make(map[string]map[string]*proto.ProxyMapping), + reconcileCache: make(map[string]map[string]syntheticMapping), labelRng: rand.New(rand.NewSource(time.Now().UnixNano())), } } diff --git a/management/internals/modules/agentnetwork/reconcile.go b/management/internals/modules/agentnetwork/reconcile.go index 319553ebc..69e684014 100644 --- a/management/internals/modules/agentnetwork/reconcile.go +++ b/management/internals/modules/agentnetwork/reconcile.go @@ -10,6 +10,17 @@ import ( "github.com/netbirdio/netbird/shared/management/proto" ) +// syntheticMapping pairs a synthesised proxy mapping with the address of the +// proxy that serves it. The cluster is recorded rather than derived from the +// mapping's domain: ProxyMapping does not carry it, and the previous derivation +// -- everything after the first DNS label -- is wrong whenever the service's +// domain is not one label under its proxy's address, which silently addressed +// updates to a cluster no proxy declares. +type syntheticMapping struct { + mapping *proto.ProxyMapping + cluster string +} + // reconcile recomputes the synthesised reverse-proxy services for an // account, diffs them against the previously-synthesised set in the // in-memory cache, and emits Create / Update / Delete proxy mappings @@ -45,18 +56,21 @@ func (m *managerImpl) reconcile(ctx context.Context, accountID string) { } oidcCfg := m.proxyController.GetOIDCValidationConfig() - current := make(map[string]*proto.ProxyMapping, len(services)) + current := make(map[string]syntheticMapping, len(services)) for _, svc := range services { if svc == nil || svc.ID == "" { continue } - current[svc.ID] = svc.ToProtoMapping(rpservice.Update, "", oidcCfg) + current[svc.ID] = syntheticMapping{ + mapping: svc.ToProtoMapping(rpservice.Update, "", oidcCfg), + cluster: svc.ProxyCluster, + } } m.reconcileMu.Lock() previous := m.reconcileCache[accountID] if previous == nil { - previous = make(map[string]*proto.ProxyMapping) + previous = make(map[string]syntheticMapping) } creates, updates, deletes := diffMappings(previous, current) @@ -67,34 +81,36 @@ func (m *managerImpl) reconcile(ctx context.Context, accountID string) { } m.reconcileMu.Unlock() - for _, mapping := range creates { - mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_CREATED - m.proxyController.SendServiceUpdateToCluster(ctx, accountID, mapping, clusterFromMapping(mapping)) + for _, entry := range creates { + entry.mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_CREATED + m.proxyController.SendServiceUpdateToCluster(ctx, accountID, entry.mapping, entry.cluster) } - for _, mapping := range updates { - mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_MODIFIED - m.proxyController.SendServiceUpdateToCluster(ctx, accountID, mapping, clusterFromMapping(mapping)) + for _, entry := range updates { + entry.mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_MODIFIED + m.proxyController.SendServiceUpdateToCluster(ctx, accountID, entry.mapping, entry.cluster) } - for _, mapping := range deletes { - mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED - m.proxyController.SendServiceUpdateToCluster(ctx, accountID, mapping, clusterFromMapping(mapping)) + for _, entry := range deletes { + entry.mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED + m.proxyController.SendServiceUpdateToCluster(ctx, accountID, entry.mapping, entry.cluster) } } -// diffMappings classifies the previous→current transition for a -// single account into Create / Update / Delete sets. +// diffMappings classifies the previous→current transition for a single +// account into Create / Update / Delete sets. // -// Cluster moves (current.cluster != previous.cluster) are surfaced as -// a Delete on the old cluster + Create on the new — handled by -// emitting both a delete (on previous mapping) and a create (on the -// current mapping) for that service ID. -func diffMappings(previous, current map[string]*proto.ProxyMapping) (creates, updates, deletes []*proto.ProxyMapping) { +// A change of serving proxy for the same service ID is surfaced as a Delete +// addressed to the old proxy plus a Create addressed to the new one, so the +// mapping actually moves. Comparing the recorded cluster is what makes that +// detectable: with a placement-free endpoint the mapping's domain is identical +// before and after the move, so nothing about the mapping itself reveals it. +func diffMappings(previous, current map[string]syntheticMapping) (creates, updates, deletes []syntheticMapping) { for id, cur := range current { prev, existed := previous[id] switch { case !existed: creates = append(creates, cur) - case prev.GetDomain() == "" || cur.GetAccountId() == prev.GetAccountId() && currentClusterChanged(prev, cur): + case prev.mapping.GetDomain() == "" || + cur.mapping.GetAccountId() == prev.mapping.GetAccountId() && prev.cluster != cur.cluster: deletes = append(deletes, prev) creates = append(creates, cur) default: @@ -108,24 +124,3 @@ func diffMappings(previous, current map[string]*proto.ProxyMapping) (creates, up } return creates, updates, deletes } - -func currentClusterChanged(prev, cur *proto.ProxyMapping) bool { - return clusterFromMapping(prev) != clusterFromMapping(cur) -} - -// clusterFromMapping returns the cluster the mapping should be sent -// to. ProxyMapping doesn't carry the cluster directly, so we rely on -// the synthesised service's domain (`.`) and split on -// the first '.'. -func clusterFromMapping(m *proto.ProxyMapping) string { - if m == nil { - return "" - } - domain := m.GetDomain() - for i := 0; i < len(domain); i++ { - if domain[i] == '.' { - return domain[i+1:] - } - } - return "" -} diff --git a/management/internals/modules/agentnetwork/reconcile_test.go b/management/internals/modules/agentnetwork/reconcile_test.go index 0855f0dc1..d1fd53298 100644 --- a/management/internals/modules/agentnetwork/reconcile_test.go +++ b/management/internals/modules/agentnetwork/reconcile_test.go @@ -21,7 +21,7 @@ func newReconcileMgr(t *testing.T, ctrl *gomock.Controller) (*managerImpl, *stor return &managerImpl{ store: mockStore, proxyController: mockProxy, - reconcileCache: make(map[string]map[string]*proto.ProxyMapping), + reconcileCache: make(map[string]map[string]syntheticMapping), }, mockStore, mockProxy } @@ -196,7 +196,7 @@ func TestReconcile_PolicyRemoved_EmitsDelete(t *testing.T) { func TestReconcile_NilProxyController_NoOp(t *testing.T) { ctx := context.Background() mgr := &managerImpl{ - reconcileCache: make(map[string]map[string]*proto.ProxyMapping), + reconcileCache: make(map[string]map[string]syntheticMapping), } // Must not panic; must not query the store. mgr.reconcile(ctx, "acct-1") @@ -212,21 +212,74 @@ func TestReconcile_EmptyAccountID_NoOp(t *testing.T) { mgr.reconcile(ctx, "") } -func TestClusterFromMapping(t *testing.T) { - tests := []struct { - name string - domain string - want string - }{ - {"simple", "openai.eu.proxy.netbird.io", "eu.proxy.netbird.io"}, - {"deeply nested", "a.b.c.d", "b.c.d"}, - {"no dot", "openai", ""}, - {"empty", "", ""}, +// TestDiffMappings_ServingProxyChange — when the proxy serving an account +// changes, the same service ID must be deleted on the old proxy and created on +// the new one. The cluster cannot be recovered from the mapping's domain: with a +// placement-free endpoint the domain does not change at all when the serving +// proxy does, so a domain-derived cluster sees no change and emits a plain +// update, addressed to a proxy that does not exist. +func TestDiffMappings_ServingProxyChange(t *testing.T) { + previous := map[string]syntheticMapping{ + "svc-1": { + mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "brave-otter.gateway.example.com"}, + cluster: "proxy.example.com", + }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := clusterFromMapping(&proto.ProxyMapping{Domain: tt.domain}) - assert.Equal(t, tt.want, got) - }) + current := map[string]syntheticMapping{ + "svc-1": { + mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "brave-otter.gateway.example.com"}, + cluster: "brave-otter.gateway.example.com", + }, } + + creates, updates, deletes := diffMappings(previous, current) + + require.Len(t, deletes, 1, "the old proxy must be told to drop the mapping") + assert.Equal(t, "proxy.example.com", deletes[0].cluster) + require.Len(t, creates, 1, "the new proxy must be told to add it") + assert.Equal(t, "brave-otter.gateway.example.com", creates[0].cluster) + assert.Empty(t, updates, "a serving-proxy move is a delete plus a create, not an update") +} + +// TestDiffMappings_UnchangedClusterIsAnUpdate keeps the ordinary path: same +// service, same proxy, changed contents. +func TestDiffMappings_UnchangedClusterIsAnUpdate(t *testing.T) { + previous := map[string]syntheticMapping{ + "svc-1": { + mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "otter.proxy.example.com"}, + cluster: "proxy.example.com", + }, + } + current := map[string]syntheticMapping{ + "svc-1": { + mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "otter.proxy.example.com"}, + cluster: "proxy.example.com", + }, + } + + creates, updates, deletes := diffMappings(previous, current) + + assert.Empty(t, creates) + assert.Empty(t, deletes) + require.Len(t, updates, 1) + assert.Equal(t, "proxy.example.com", updates[0].cluster) +} + +// TestDiffMappings_RemovedServiceIsDeletedOnItsOwnCluster — a service that has +// gone away is deleted on the cluster it was last served by, which is recorded +// rather than re-derived. +func TestDiffMappings_RemovedServiceIsDeletedOnItsOwnCluster(t *testing.T) { + previous := map[string]syntheticMapping{ + "svc-1": { + mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "brave-otter.gateway.example.com"}, + cluster: "brave-otter.gateway.example.com", + }, + } + + creates, updates, deletes := diffMappings(previous, map[string]syntheticMapping{}) + + assert.Empty(t, creates) + assert.Empty(t, updates) + require.Len(t, deletes, 1) + assert.Equal(t, "brave-otter.gateway.example.com", deletes[0].cluster) } diff --git a/management/internals/modules/agentnetwork/synthesizer_realstore_test.go b/management/internals/modules/agentnetwork/synthesizer_realstore_test.go index 1e07c0e81..33351fd7f 100644 --- a/management/internals/modules/agentnetwork/synthesizer_realstore_test.go +++ b/management/internals/modules/agentnetwork/synthesizer_realstore_test.go @@ -147,7 +147,7 @@ func TestReconcile_RealStore_PushesPrivateAfterStatusToggle(t *testing.T) { store: s, accountManager: noopAccountManager{}, proxyController: ctrl, - reconcileCache: make(map[string]map[string]*proto.ProxyMapping), + reconcileCache: make(map[string]map[string]syntheticMapping), } m.reconcile(ctx, testAccountID) // initial, provider enabled