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.
This commit is contained in:
Brad Ison
2026-08-10 16:57:15 +02:00
parent 0bbbc74e02
commit 9feaa2e87f
4 changed files with 111 additions and 65 deletions
@@ -22,7 +22,6 @@ import (
"github.com/netbirdio/netbird/management/server/permissions/modules" "github.com/netbirdio/netbird/management/server/permissions/modules"
"github.com/netbirdio/netbird/management/server/permissions/operations" "github.com/netbirdio/netbird/management/server/permissions/operations"
"github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/store"
"github.com/netbirdio/netbird/shared/management/proto"
"github.com/netbirdio/netbird/shared/management/status" "github.com/netbirdio/netbird/shared/management/status"
) )
@@ -123,11 +122,10 @@ type managerImpl struct {
proxyController proxy.Controller proxyController proxy.Controller
// reconcileCache holds the last set of synthesised proxy mappings // reconcileCache holds the last set of synthesised proxy mappings
// per account so reconcile can emit precise Create/Update/Delete // per account, each paired with the proxy that served it, so a change
// updates instead of a full re-push on every mutation. Keyed by // of serving proxy can be diffed without re-deriving it.
// accountID, then by synthesised service ID.
reconcileMu sync.Mutex reconcileMu sync.Mutex
reconcileCache map[string]map[string]*proto.ProxyMapping reconcileCache map[string]map[string]syntheticMapping
// labelRngMu guards labelRng. PickUnique consumes math/rand.Source // labelRngMu guards labelRng. PickUnique consumes math/rand.Source
// state; concurrent provider creates would otherwise race. // state; concurrent provider creates would otherwise race.
@@ -151,7 +149,7 @@ func NewManager(
accountManager: accountManager, accountManager: accountManager,
permissionsManager: permissionsManager, permissionsManager: permissionsManager,
proxyController: proxyController, 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())), labelRng: rand.New(rand.NewSource(time.Now().UnixNano())),
} }
} }
@@ -10,6 +10,17 @@ import (
"github.com/netbirdio/netbird/shared/management/proto" "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 // reconcile recomputes the synthesised reverse-proxy services for an
// account, diffs them against the previously-synthesised set in the // account, diffs them against the previously-synthesised set in the
// in-memory cache, and emits Create / Update / Delete proxy mappings // 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() oidcCfg := m.proxyController.GetOIDCValidationConfig()
current := make(map[string]*proto.ProxyMapping, len(services)) current := make(map[string]syntheticMapping, len(services))
for _, svc := range services { for _, svc := range services {
if svc == nil || svc.ID == "" { if svc == nil || svc.ID == "" {
continue 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() m.reconcileMu.Lock()
previous := m.reconcileCache[accountID] previous := m.reconcileCache[accountID]
if previous == nil { if previous == nil {
previous = make(map[string]*proto.ProxyMapping) previous = make(map[string]syntheticMapping)
} }
creates, updates, deletes := diffMappings(previous, current) creates, updates, deletes := diffMappings(previous, current)
@@ -67,34 +81,36 @@ func (m *managerImpl) reconcile(ctx context.Context, accountID string) {
} }
m.reconcileMu.Unlock() m.reconcileMu.Unlock()
for _, mapping := range creates { for _, entry := range creates {
mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_CREATED entry.mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_CREATED
m.proxyController.SendServiceUpdateToCluster(ctx, accountID, mapping, clusterFromMapping(mapping)) m.proxyController.SendServiceUpdateToCluster(ctx, accountID, entry.mapping, entry.cluster)
} }
for _, mapping := range updates { for _, entry := range updates {
mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_MODIFIED entry.mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_MODIFIED
m.proxyController.SendServiceUpdateToCluster(ctx, accountID, mapping, clusterFromMapping(mapping)) m.proxyController.SendServiceUpdateToCluster(ctx, accountID, entry.mapping, entry.cluster)
} }
for _, mapping := range deletes { for _, entry := range deletes {
mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED entry.mapping.Type = proto.ProxyMappingUpdateType_UPDATE_TYPE_REMOVED
m.proxyController.SendServiceUpdateToCluster(ctx, accountID, mapping, clusterFromMapping(mapping)) m.proxyController.SendServiceUpdateToCluster(ctx, accountID, entry.mapping, entry.cluster)
} }
} }
// diffMappings classifies the previous→current transition for a // diffMappings classifies the previous→current transition for a single
// single account into Create / Update / Delete sets. // account into Create / Update / Delete sets.
// //
// Cluster moves (current.cluster != previous.cluster) are surfaced as // A change of serving proxy for the same service ID is surfaced as a Delete
// a Delete on the old cluster + Create on the new — handled by // addressed to the old proxy plus a Create addressed to the new one, so the
// emitting both a delete (on previous mapping) and a create (on the // mapping actually moves. Comparing the recorded cluster is what makes that
// current mapping) for that service ID. // detectable: with a placement-free endpoint the mapping's domain is identical
func diffMappings(previous, current map[string]*proto.ProxyMapping) (creates, updates, deletes []*proto.ProxyMapping) { // 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 { for id, cur := range current {
prev, existed := previous[id] prev, existed := previous[id]
switch { switch {
case !existed: case !existed:
creates = append(creates, cur) 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) deletes = append(deletes, prev)
creates = append(creates, cur) creates = append(creates, cur)
default: default:
@@ -108,24 +124,3 @@ func diffMappings(previous, current map[string]*proto.ProxyMapping) (creates, up
} }
return creates, updates, deletes 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 (`<slug>.<cluster>`) 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 ""
}
@@ -21,7 +21,7 @@ func newReconcileMgr(t *testing.T, ctrl *gomock.Controller) (*managerImpl, *stor
return &managerImpl{ return &managerImpl{
store: mockStore, store: mockStore,
proxyController: mockProxy, proxyController: mockProxy,
reconcileCache: make(map[string]map[string]*proto.ProxyMapping), reconcileCache: make(map[string]map[string]syntheticMapping),
}, mockStore, mockProxy }, mockStore, mockProxy
} }
@@ -196,7 +196,7 @@ func TestReconcile_PolicyRemoved_EmitsDelete(t *testing.T) {
func TestReconcile_NilProxyController_NoOp(t *testing.T) { func TestReconcile_NilProxyController_NoOp(t *testing.T) {
ctx := context.Background() ctx := context.Background()
mgr := &managerImpl{ 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. // Must not panic; must not query the store.
mgr.reconcile(ctx, "acct-1") mgr.reconcile(ctx, "acct-1")
@@ -212,21 +212,74 @@ func TestReconcile_EmptyAccountID_NoOp(t *testing.T) {
mgr.reconcile(ctx, "") mgr.reconcile(ctx, "")
} }
func TestClusterFromMapping(t *testing.T) { // TestDiffMappings_ServingProxyChange — when the proxy serving an account
tests := []struct { // changes, the same service ID must be deleted on the old proxy and created on
name string // the new one. The cluster cannot be recovered from the mapping's domain: with a
domain string // placement-free endpoint the domain does not change at all when the serving
want string // proxy does, so a domain-derived cluster sees no change and emits a plain
}{ // update, addressed to a proxy that does not exist.
{"simple", "openai.eu.proxy.netbird.io", "eu.proxy.netbird.io"}, func TestDiffMappings_ServingProxyChange(t *testing.T) {
{"deeply nested", "a.b.c.d", "b.c.d"}, previous := map[string]syntheticMapping{
{"no dot", "openai", ""}, "svc-1": {
{"empty", "", ""}, mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "brave-otter.gateway.example.com"},
cluster: "proxy.example.com",
},
} }
for _, tt := range tests { current := map[string]syntheticMapping{
t.Run(tt.name, func(t *testing.T) { "svc-1": {
got := clusterFromMapping(&proto.ProxyMapping{Domain: tt.domain}) mapping: &proto.ProxyMapping{Id: "svc-1", AccountId: "acct-1", Domain: "brave-otter.gateway.example.com"},
assert.Equal(t, tt.want, got) 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)
} }
@@ -147,7 +147,7 @@ func TestReconcile_RealStore_PushesPrivateAfterStatusToggle(t *testing.T) {
store: s, store: s,
accountManager: noopAccountManager{}, accountManager: noopAccountManager{},
proxyController: ctrl, proxyController: ctrl,
reconcileCache: make(map[string]map[string]*proto.ProxyMapping), reconcileCache: make(map[string]map[string]syntheticMapping),
} }
m.reconcile(ctx, testAccountID) // initial, provider enabled m.reconcile(ctx, testAccountID) // initial, provider enabled