From 104d04eda3c446f3ca26c1091c637d1107bab6dc Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Tue, 4 Aug 2026 17:29:39 +0200 Subject: [PATCH] fix(agentnetwork): address live mapping updates to the serving proxy reconcile sent each mapping delta to a cluster recovered from the mapping's domain by stripping the first DNS label. That is only correct when a service's domain is exactly one label under its proxy's address. For any other shape it names a cluster no proxy declares, GetProxiesForCluster comes back empty, and the update is dropped with a debug line -- so provider and settings changes stopped reaching the proxy until it reconnected and re-snapshotted. The same derivation drove change detection, which made a change of serving proxy invisible: a placement-free endpoint keeps the same domain when its serving proxy changes, so the derived cluster was equal before and after and the move was classified as a plain update, addressed to whichever cluster the domain happened to imply. The synthesised service already carries the answer, so the reconcile cache now records it alongside the mapping and both the diff and the send read the recorded value. clusterFromMapping and currentClusterChanged are deleted rather than fixed -- there is nothing to derive once the value is carried. --- .../internals/modules/agentnetwork/manager.go | 10 +-- .../modules/agentnetwork/reconcile.go | 77 ++++++++-------- .../modules/agentnetwork/reconcile_test.go | 87 +++++++++++++++---- .../synthesizer_realstore_test.go | 2 +- 4 files changed, 111 insertions(+), 65 deletions(-) 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