From 67b9c8c398dd60ee5e2be321fdf45a62f426a027 Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 20 Aug 2026 14:15:46 +0200 Subject: [PATCH 1/6] go mod tidy --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index ad692fee9..8a5e7e783 100644 --- a/go.mod +++ b/go.mod @@ -62,6 +62,7 @@ require ( github.com/goccy/go-yaml v1.18.0 github.com/godbus/dbus/v5 v5.2.2 github.com/golang-jwt/jwt/v5 v5.3.1 + github.com/golang/mock v1.6.0 github.com/google/go-cmp v0.7.0 github.com/google/gopacket v1.1.19 github.com/google/nftables v0.3.0 @@ -217,7 +218,6 @@ require ( github.com/gobwas/pool v0.2.1 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v4 v4.5.2 // indirect - github.com/golang/mock v1.6.0 // indirect github.com/google/btree v1.1.3 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/go-tpm v0.9.8 // indirect From e83886b77bf96534d61072cf0855c5977a3aa5a5 Mon Sep 17 00:00:00 2001 From: pascal Date: Fri, 21 Aug 2026 11:03:58 +0200 Subject: [PATCH 2/6] fix applied zones --- .../management/network_map_db/dns_test.go | 38 +++++++++++---- .../network_map_db/network_map_data.sql | 8 +++- .../network_map_data_golden.json | 8 ++-- .../network_map_db/network_map_data_test.go | 12 +++-- .../network_map/controller/controller.go | 21 +++++---- .../controller/ipv6_allowed_test.go | 47 +++++++++++++++++++ .../internals/network_map_db/pgsql/dns.go | 4 +- .../internals/network_map_db/shared_types.go | 8 +++- .../internals/network_map_db/sqlite/dns.go | 4 +- .../types/legacynmap/equivalence_test.go | 32 ++++++++----- 10 files changed, 134 insertions(+), 48 deletions(-) create mode 100644 management/internals/controllers/network_map/controller/ipv6_allowed_test.go diff --git a/integration_tests/management/network_map_db/dns_test.go b/integration_tests/management/network_map_db/dns_test.go index 63a609193..33023061d 100644 --- a/integration_tests/management/network_map_db/dns_test.go +++ b/integration_tests/management/network_map_db/dns_test.go @@ -16,11 +16,14 @@ func TestGetAppliedZoneCandidatesViaPgxConnection(t *testing.T) { ctx := context.TODO() execQuery(t, ctx, - `insert into zones (id, account_id, domain, enable_search_domain, distribution_groups) - VALUES('zone-1','account-1','test-1.com',true,'["group-one-resource-id"]')`) + `insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) + VALUES('zone-1','account-1','test-1.com',true,true,'["group-one-resource-id"]')`) execQuery(t, ctx, - `insert into zones (id, account_id, domain, enable_search_domain, distribution_groups) - VALUES('zone-2','account-1','test-2.com',false,'["group-two-resources-id"]')`) + `insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) + VALUES('zone-2','account-1','test-2.com',true,false,'["group-two-resources-id"]')`) + execQuery(t, ctx, + `insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) + VALUES('zone-3','account-1','test-3.com',false,true,'["group-one-resource-id"]')`) execQuery(t, ctx, `insert into records (id, account_id, zone_id, name, type, ttl, content) VALUES('record-1','account-1','zone-1','test.test-1.com','A',1800,'1.1.1.1')`) @@ -33,30 +36,45 @@ func TestGetAppliedZoneCandidatesViaPgxConnection(t *testing.T) { execQuery(t, ctx, `insert into records (id, account_id, zone_id, name, type, ttl, content) VALUES('record-4','account-1','zone-2','test2.test-2.com','CNAME',1800,'test3.test-2.com')`) + execQuery(t, ctx, + `insert into records (id, account_id, zone_id, name, type, ttl, content) + VALUES('record-5','account-1','zone-3','test.test-3.com','A',1800,'1.1.1.3')`) zoneCandidates, err := conn(t, ctx).GetAppliedZoneCandidates(ctx, "account-1") assert.NoError(t, err) + // Zone domains and record names are fully qualified, and the zone is served + // non-authoritatively — the account-side builder + // (types.buildAppliedZoneCandidates) states the same shape, and both feed the + // one client-facing map, so the two have to agree. assert.Contains(t, zoneCandidates, networkmap.AppliedZoneCandidate{ DistributionGroups: []string{"group-one-resource-id"}, Zone: nmdata.CustomZone{ - Domain: "test-1.com", + Domain: "test-1.com.", SearchDomainDisabled: false, + NonAuthoritative: true, Records: []nmdata.SimpleRecord{ - {Name: "test.test-1.com", Type: int(dns.TypeA), Class: "IN", TTL: 1800, RData: "1.1.1.1"}, - {Name: "test2.test-1.com", Type: int(dns.TypeA), Class: "IN", TTL: 1800, RData: "1.1.1.2"}, - {Name: "test3.test-1.com", Type: int(dns.TypeCNAME), Class: "IN", TTL: 1800, RData: "test4.test-1.com."}, + {Name: "test.test-1.com.", Type: int(dns.TypeA), Class: "IN", TTL: 1800, RData: "1.1.1.1"}, + {Name: "test2.test-1.com.", Type: int(dns.TypeA), Class: "IN", TTL: 1800, RData: "1.1.1.2"}, + {Name: "test3.test-1.com.", Type: int(dns.TypeCNAME), Class: "IN", TTL: 1800, RData: "test4.test-1.com."}, }, }, }) assert.Contains(t, zoneCandidates, networkmap.AppliedZoneCandidate{ DistributionGroups: []string{"group-two-resources-id"}, Zone: nmdata.CustomZone{ - Domain: "test-2.com", + Domain: "test-2.com.", SearchDomainDisabled: true, + NonAuthoritative: true, Records: []nmdata.SimpleRecord{ - {Name: "test2.test-2.com", Type: int(dns.TypeCNAME), Class: "IN", TTL: 1800, RData: "test3.test-2.com."}, + {Name: "test2.test-2.com.", Type: int(dns.TypeCNAME), Class: "IN", TTL: 1800, RData: "test3.test-2.com."}, }, }, }) + + // A zone an admin switched off reaches no peer. + for _, candidate := range zoneCandidates { + assert.NotEqual(t, "test-3.com.", candidate.Zone.Domain, "disabled zone must not be a candidate") + assert.NotEqual(t, "test-3.com", candidate.Zone.Domain, "disabled zone must not be a candidate") + } } diff --git a/integration_tests/management/network_map_db/network_map_data.sql b/integration_tests/management/network_map_db/network_map_data.sql index 60cfd291e..9f8e1bdb8 100644 --- a/integration_tests/management/network_map_db/network_map_data.sql +++ b/integration_tests/management/network_map_db/network_map_data.sql @@ -43,8 +43,12 @@ insert into peers (id,account_id,"key", ssh_key, dns_label, extra_dns_labels, us '[1,2]','{"RosenpassEnabled":false,"RosenpassPermissive":false,"ServerSSHAllowed":true,"DisableClientRoutes":false,"DisableServerRoutes":false,"DisableDNS":false,"DisableFirewall":false,"BlockLANAccess":false,"BlockInbound":false,"DisableIPv6":false,"LazyConnectionEnabled":false}',1, 'DE','Berlin','"46.201.150.187"'); -insert into zones (id, account_id, domain, enable_search_domain, distribution_groups) - VALUES('zone-331','account-33','test-331.com',true,'["33-group-one-resource-id"]'); +insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) + VALUES('zone-331','account-33','test-331.com',true,true,'["33-group-one-resource-id"]'); +insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) + VALUES('zone-332','account-33','disabled-331.com',false,true,'["33-group-one-resource-id"]'); +insert into records (id, account_id, zone_id, name, type, ttl, content) + VALUES('record-333','account-33','zone-332','test.disabled-331.com','A',1800,'1.1.1.9'); insert into records (id, account_id, zone_id, name, type, ttl, content) VALUES('record-331','account-33','zone-331','test.test-331.com','A',1800,'1.1.1.1'); insert into records (id, account_id, zone_id, name, type, ttl, content) diff --git a/integration_tests/management/network_map_db/network_map_data_golden.json b/integration_tests/management/network_map_db/network_map_data_golden.json index f34cd1f6c..cff044d89 100644 --- a/integration_tests/management/network_map_db/network_map_data_golden.json +++ b/integration_tests/management/network_map_db/network_map_data_golden.json @@ -493,17 +493,17 @@ "33-group-one-resource-id" ], "Zone": { - "Domain": "test-331.com", + "Domain": "test-331.com.", "Records": [ { - "Name": "test.test-331.com", + "Name": "test.test-331.com.", "Type": 1, "Class": "IN", "TTL": 1800, "RData": "1.1.1.1" }, { - "Name": "test2.test-331.com", + "Name": "test2.test-331.com.", "Type": 1, "Class": "IN", "TTL": 1800, @@ -511,7 +511,7 @@ } ], "SearchDomainDisabled": false, - "NonAuthoritative": false + "NonAuthoritative": true } } ], diff --git a/integration_tests/management/network_map_db/network_map_data_test.go b/integration_tests/management/network_map_db/network_map_data_test.go index 25c0c0e77..c07276fec 100644 --- a/integration_tests/management/network_map_db/network_map_data_test.go +++ b/integration_tests/management/network_map_db/network_map_data_test.go @@ -12,13 +12,14 @@ import ( "strings" "testing" - "github.com/golang/mock/gomock" + legacygomock "github.com/golang/mock/gomock" networkmapdb "github.com/netbirdio/netbird/management/internals/network_map_db" "github.com/netbirdio/netbird/management/server/integrations/integrated_validator" "github.com/netbirdio/netbird/management/server/settings" "github.com/netbirdio/netbird/management/server/types" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" ) //go:embed network_map_data.sql @@ -31,13 +32,14 @@ const EnvUpdateGoldenData = "NMAP_UPDATE_GOLDEN_DATA" func TestGetNetworkMapData(t *testing.T) { ctx := context.TODO() - ctrl := gomock.NewController(t) - extraSettingsManager := settings.NewMockManager(ctrl) + // The two mocks are generated by different mock frameworks, so each needs a + // controller of its own kind. + extraSettingsManager := settings.NewMockManager(gomock.NewController(t)) extraSettingsManager.EXPECT().GetExtraSettings(gomock.Any(), gomock.Any()).Return(&types.ExtraSettings{}, nil) - peerValidators := integrated_validator.NewMockIntegratedValidator(ctrl) - peerValidators.EXPECT().GetValidatedPeers(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return( + peerValidators := integrated_validator.NewMockIntegratedValidator(legacygomock.NewController(t)) + peerValidators.EXPECT().GetValidatedPeers(legacygomock.Any(), legacygomock.Any(), legacygomock.Any(), legacygomock.Any(), legacygomock.Any()).Return( map[string]struct{}{ "peer-id-1": {}, "peer-id-2": {}, diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index aa7e5e7b2..38fe608d9 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -500,15 +500,18 @@ func (c *Controller) getDNSDomainFromData(settings *nmdata.AccountSettingsInfo) func IPv6AllowedPeersFromData(nmData *networkmap.NetworkMapData) map[string]struct{} { result := make(map[string]struct{}) - if nmData.AccountSettings != nil { - for _, groupID := range nmData.AccountSettings.IPv6EnabledGroups { - group := nmData.Groups[groupID] - if group == nil { - continue - } - for _, peerID := range group.Peers { - result[peerID] = struct{}{} - } + // An account with no IPv6-enabled group runs no overlay at all, so the + // embedded-proxy carve-out below has nothing to reach and stays shut. + if nmData.AccountSettings == nil || len(nmData.AccountSettings.IPv6EnabledGroups) == 0 { + return result + } + for _, groupID := range nmData.AccountSettings.IPv6EnabledGroups { + group := nmData.Groups[groupID] + if group == nil { + continue + } + for _, peerID := range group.Peers { + result[peerID] = struct{}{} } } for id, p := range nmData.Peers { diff --git a/management/internals/controllers/network_map/controller/ipv6_allowed_test.go b/management/internals/controllers/network_map/controller/ipv6_allowed_test.go new file mode 100644 index 000000000..c80f3b734 --- /dev/null +++ b/management/internals/controllers/network_map/controller/ipv6_allowed_test.go @@ -0,0 +1,47 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/shared/management/networkmap" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" +) + +// The account-side builder (types.Account.peerIPv6AllowedSet) is the reference: +// an account with no IPv6-enabled group runs no IPv6 overlay at all, embedded +// proxy peers included — see TestPeerIPv6AllowedEmbeddedProxy. Both builders +// gate the same AAAA records, so the store-backed one has to agree. +func TestIPv6AllowedPeersFromData(t *testing.T) { + data := func(enabledGroups []string) *networkmap.NetworkMapData { + return &networkmap.NetworkMapData{ + AccountSettings: &nmdata.AccountSettingsInfo{IPv6EnabledGroups: enabledGroups}, + Peers: map[string]*nmdata.Peer{ + "peer1": {ID: "peer1"}, + "lonely": {ID: "lonely"}, + "proxy": {ID: "proxy", ProxyMeta: nmdata.ProxyMeta{Embedded: true, Cluster: "netbird.test"}}, + }, + Groups: map[string]*nmdata.Group{ + "group-devs": {ID: "group-devs", Peers: []string{"peer1"}}, + }, + } + } + + t.Run("embedded proxy allowed when any v6 group exists, without group membership", func(t *testing.T) { + allowed := IPv6AllowedPeersFromData(data([]string{"group-devs"})) + assert.Contains(t, allowed, "proxy", "embedded proxy participates in v6 overlay") + assert.Contains(t, allowed, "peer1", "regular peer in enabled group still allowed") + }) + + t.Run("embedded proxy denied when no v6 group enabled", func(t *testing.T) { + allowed := IPv6AllowedPeersFromData(data(nil)) + assert.NotContains(t, allowed, "proxy", "v6 disabled account-wide denies embedded proxies too") + assert.Empty(t, allowed, "no peer participates in the v6 overlay") + }) + + t.Run("non-embedded peer outside any enabled group is not pulled in", func(t *testing.T) { + allowed := IPv6AllowedPeersFromData(data([]string{"group-devs"})) + assert.NotContains(t, allowed, "lonely", "embedded-proxy bypass must not leak to regular peers") + }) +} diff --git a/management/internals/network_map_db/pgsql/dns.go b/management/internals/network_map_db/pgsql/dns.go index c55aa4f5d..b22b43903 100644 --- a/management/internals/network_map_db/pgsql/dns.go +++ b/management/internals/network_map_db/pgsql/dns.go @@ -13,8 +13,8 @@ const ( select zones.id as id, domain, not enable_search_domain as search_domain_disabled, distribution_groups, r.name as record_name, r.type as record_type, 'IN' record_class, r.ttl as record_ttl, r.content as record_rdata from zones - left join records as r on r.zone_id = zones.id - where zones.account_id=$1 + left join records as r on r.zone_id = zones.id + where zones.account_id=$1 and zones.enabled ` ) diff --git a/management/internals/network_map_db/shared_types.go b/management/internals/network_map_db/shared_types.go index 53d2aef59..bdd387877 100644 --- a/management/internals/network_map_db/shared_types.go +++ b/management/internals/network_map_db/shared_types.go @@ -248,6 +248,12 @@ func ZonesToAppliedZoneCandidates(zones []Zone) ([]networkmap.AppliedZoneCandida } if z.Id != currentZoneId { + // The account-side builder (types.buildAppliedZoneCandidates) states + // the shape of an applied zone: names fully qualified, served + // non-authoritatively. Both builders feed the same client-facing map, + // so this one has to produce the same value. + zone.Domain = dns.Fqdn(zone.Domain) + zone.NonAuthoritative = true zone.Records = []nmdata.SimpleRecord{} toret = append(toret, AppliedZoneCandidateFromZone(zone, distributionGroups)) currentZoneId = z.Id @@ -263,7 +269,7 @@ func ZonesToAppliedZoneCandidates(zones []Zone) ([]networkmap.AppliedZoneCandida lastZone := &toret[len(toret)-1] lastZone.Zone.Records = append(lastZone.Zone.Records, nmdata.SimpleRecord{ - Name: z.RecordName.String, + Name: dns.Fqdn(z.RecordName.String), Class: z.RecordClass.String, TTL: int(z.RecordTTL.Int64), RData: rdata, diff --git a/management/internals/network_map_db/sqlite/dns.go b/management/internals/network_map_db/sqlite/dns.go index 026467eae..dd2cb3758 100644 --- a/management/internals/network_map_db/sqlite/dns.go +++ b/management/internals/network_map_db/sqlite/dns.go @@ -12,8 +12,8 @@ const ( select zones.id as id, domain, not enable_search_domain as search_domain_disabled, distribution_groups, r.name as record_name, r.type as record_type, 'IN' record_class, r.ttl as record_ttl, r.content as record_rdata from zones - left join records as r on r.zone_id = zones.id - where zones.account_id=? + left join records as r on r.zone_id = zones.id + where zones.account_id=? and zones.enabled ` ) diff --git a/management/server/types/legacynmap/equivalence_test.go b/management/server/types/legacynmap/equivalence_test.go index 1be3c2539..d12e666b8 100644 --- a/management/server/types/legacynmap/equivalence_test.go +++ b/management/server/types/legacynmap/equivalence_test.go @@ -42,15 +42,15 @@ import ( "strings" "testing" - "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "google.golang.org/protobuf/encoding/prototext" goproto "google.golang.org/protobuf/proto" "gorm.io/driver/postgres" "gorm.io/gorm" gormlogger "gorm.io/gorm/logger" - nbdns "github.com/netbirdio/netbird/dns" + "github.com/netbirdio/netbird/management/internals/controllers/network_map/controller" "github.com/netbirdio/netbird/management/internals/controllers/network_map/controller/cache" networkmapdb "github.com/netbirdio/netbird/management/internals/network_map_db" networkmap_pgsql "github.com/netbirdio/netbird/management/internals/network_map_db/pgsql" @@ -60,7 +60,7 @@ import ( "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/management/server/types/legacynmap" - "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" + "github.com/netbirdio/netbird/shared/management/networkmap" "github.com/netbirdio/netbird/shared/management/proto" ) @@ -108,7 +108,7 @@ func TestNetworkMapProtoEquivalence(t *testing.T) { continue } - checkAccount(ctx, t, nmStore, account, maxPeers, stats) + checkAccount(ctx, t, testStore, nmStore, account, maxPeers, stats) account = nil debug.FreeOSMemory() @@ -126,7 +126,7 @@ func TestNetworkMapProtoEquivalence(t *testing.T) { // checkAccount compares both paths for every peer of one account. Nothing is // retained across peers, so memory stays flat within an account. -func checkAccount(ctx context.Context, t *testing.T, nmStore *networkmapdb.NetworkMapDBStoreImpl, account *types.Account, maxPeers int, stats *equivStats) { +func checkAccount(ctx context.Context, t *testing.T, accountStore store.Store, nmStore *networkmapdb.NetworkMapDBStoreImpl, account *types.Account, maxPeers int, stats *equivStats) { t.Helper() if len(account.Peers) == 0 { @@ -150,11 +150,14 @@ func checkAccount(ctx context.Context, t *testing.T, nmStore *networkmapdb.Netwo // Production fills ValidatedPeers via the integrated-validator wrapper; here // every peer counts as validated, matching the legacy side's map. nmData.ValidatedPeers = validated - // The legacy side receives no account zones (main sourced them from the - // external zones manager), so the DB-sourced applied-zone candidates must be - // dropped to keep the comparison surface identical. PrivateServiceCandidates - // stay: all paths derive them from account/DB data. - nmData.AppliedZoneCandidates = nil + + // Custom DNS zones are built twice from the same rows — the account side + // from the zones manager, the store side in SQL — so both are fed in and + // compared rather than dropped. The same goes for the peers zone below: + // each side computes it with its own helper, which is where an AAAA gate + // that disagrees between the two would show up. + accountZones, err := accountStore.GetAccountZones(ctx, store.LockingStrengthNone, account.Id) + require.NoError(t, err, "account %s: load account zones", account.Id) resourcePolicies := account.GetResourcePoliciesMap() routers := account.GetResourceRoutersMap() @@ -176,6 +179,9 @@ func checkAccount(ctx context.Context, t *testing.T, nmStore *networkmapdb.Netwo settings = &types.Settings{} } + accountPeersZone := account.GetPeersCustomZone(ctx, equivDNSName) + storePeersZone := networkmap.PeersCustomZone(ctx, account.Id, equivDNSName, nmData.Peers, controller.IPv6AllowedPeersFromData(nmData)) + for _, peerID := range peerIDs { peer := account.Peers[peerID] if peer == nil { @@ -188,7 +194,7 @@ func checkAccount(ctx context.Context, t *testing.T, nmStore *networkmapdb.Netwo // STORE PATH — nmdata store through the production computation, mirroring // the controller's networkMapFromData. - components := nmData.GetPeerNetworkMapComponents(peerID, nmdata.CustomZone{}) + components := nmData.GetPeerNetworkMapComponents(peerID, storePeersZone) storeNM := &types.NetworkMap{Network: components.Network} if !components.IsEmpty() { storeNM = types.CalculateNetworkMapFromComponents(ctx, components) @@ -202,7 +208,7 @@ func checkAccount(ctx context.Context, t *testing.T, nmStore *networkmapdb.Netwo // ACCOUNT PATH — Account → toNetworkMapData twins → components. acctNM := account.GetPeerNetworkMapFromComponents( - ctx, peerID, nbdns.CustomZone{}, nil, validated, resourcePolicies, routers, nil, groupUsers, + ctx, peerID, accountPeersZone, accountZones, validated, resourcePolicies, routers, nil, groupUsers, ) acctProto := mgmtgrpc.ToSyncResponse( ctx, nil, nil, nil, types.TwinPeer(peer), nil, nil, acctNM, equivDNSName, nil, @@ -211,7 +217,7 @@ func checkAccount(ctx context.Context, t *testing.T, nmStore *networkmapdb.Netwo // LEGACY PATH — main's frozen copy. legacyNM := legacynmap.GetPeerNetworkMapFromComponents( - &legacyAccount, ctx, peerID, nbdns.CustomZone{}, nil, validated, legacyResourcePolicies, routers, nil, groupUsers, + &legacyAccount, ctx, peerID, accountPeersZone, accountZones, validated, legacyResourcePolicies, routers, nil, groupUsers, ) if legacyNM == nil { t.Fatalf("after %d peers: account=%s peer=%s legacy NetworkMap nil, new non-nil", stats.peersChecked, account.Id, peerID) From f1c4b664f7255048197bb2ff2ba0ea5f7ea0008c Mon Sep 17 00:00:00 2001 From: pascal Date: Fri, 21 Aug 2026 11:48:17 +0200 Subject: [PATCH 3/6] add metrics --- .../network_map/controller/controller.go | 25 +++++++++++++++---- .../network_map/nmaptest/runner.go | 2 +- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index 38fe608d9..5567af0b2 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -452,7 +452,7 @@ func (c *Controller) sendUpdatesFromData(ctx context.Context, accountID string, return } - nmap := NetworkMapFromData(ctx, nmData, p.ID, peersCustomZone) + nmap := NetworkMapFromData(ctx, nmData, p.ID, peersCustomZone, c.accountManagerMetrics) c.metrics.CountCalcPeerNetworkMapDuration(time.Since(start)) @@ -522,12 +522,22 @@ func IPv6AllowedPeersFromData(nmData *networkmap.NetworkMapData) map[string]stru return result } -func NetworkMapFromData(ctx context.Context, nmData *networkmap.NetworkMapData, peerID string, peersCustomZone nmdata.CustomZone) *types.NetworkMap { +func NetworkMapFromData(ctx context.Context, nmData *networkmap.NetworkMapData, peerID string, peersCustomZone nmdata.CustomZone, metrics *telemetry.AccountManagerMetrics) *types.NetworkMap { + start := time.Now() + components := nmData.GetPeerNetworkMapComponents(peerID, peersCustomZone) if components.IsEmpty() { return &types.NetworkMap{Network: components.Network} } - return types.CalculateNetworkMapFromComponents(ctx, components) + nm := types.CalculateNetworkMapFromComponents(ctx, components) + + if metrics != nil { + objectCount := int64(len(nm.Peers) + len(nm.OfflinePeers) + len(nm.Routes) + len(nm.FirewallRules) + len(nm.RoutesFirewallRules)) + metrics.CountNetworkMapObjects(objectCount) + metrics.CountGetPeerNetworkMapDuration(time.Since(start)) + } + + return nm } // peerPostureChecksFromData mirrors getPeerPostureChecks on the twin store. The @@ -1174,7 +1184,7 @@ func (c *Controller) getValidatedPeerWithMapFromData(ctx context.Context, accoun dnsDomain := c.getDNSDomainFromData(nmData.AccountSettings) peersCustomZone := networkmap.PeersCustomZone(ctx, accountID, dnsDomain, nmData.Peers, IPv6AllowedPeersFromData(nmData)) - networkMap := NetworkMapFromData(ctx, nmData, peerID, peersCustomZone) + networkMap := NetworkMapFromData(ctx, nmData, peerID, peersCustomZone, c.accountManagerMetrics) dnsFwdPort := ComputeForwarderPortFromData(nmData.Peers, network_map.DnsForwarderPortMinVersion) return networkMap, postureChecks, dnsFwdPort, nil @@ -1405,7 +1415,12 @@ func (c *Controller) GetNetworkMap(ctx context.Context, peerID string) (*types.N groups[groupID] = group.Peers } - validatedPeers, err := c.integratedPeerValidator.GetValidatedPeers(ctx, account.Id, types.TwinGroups(maps.Values(account.Groups)), types.TwinPeers(maps.Values(account.Peers)), account.Settings.Extra) + extraSettings, err := c.settingsManager.GetExtraSettings(ctx, account.Id) + if err != nil { + return nil, err + } + + validatedPeers, err := c.integratedPeerValidator.GetValidatedPeers(ctx, account.Id, types.TwinGroups(maps.Values(account.Groups)), types.TwinPeers(maps.Values(account.Peers)), extraSettings) if err != nil { return nil, err } diff --git a/management/internals/controllers/network_map/nmaptest/runner.go b/management/internals/controllers/network_map/nmaptest/runner.go index 4ee4ed62e..c70bd7298 100644 --- a/management/internals/controllers/network_map/nmaptest/runner.go +++ b/management/internals/controllers/network_map/nmaptest/runner.go @@ -236,7 +236,7 @@ func computeMode(t *testing.T, ctx context.Context, mode Mode, nmData *networkma case ModeLegacy: return computeLegacy(t, ctx, legacy, peerID, zone, dnsDomain, dnsFwdPort) case ModeFull: - nmap := controller.NetworkMapFromData(ctx, nmData, peerID, zone) + nmap := controller.NetworkMapFromData(ctx, nmData, peerID, zone, nil) return mgmtgrpc.ToSyncResponse(ctx, nil, nil, nil, peer, nil, nil, nmap, dnsDomain, nil, &cache.DNSConfigCache{}, nmData.AccountSettings, nil, nil, dnsFwdPort).NetworkMap case ModeEnvelope: From 6e677cd2e6c62a0e9a19dbad45be1425ce14301d Mon Sep 17 00:00:00 2001 From: pascal Date: Fri, 21 Aug 2026 12:54:26 +0200 Subject: [PATCH 4/6] add request buffer for nmap data store --- .../network_map/controller/controller.go | 42 +++++-- .../internals/shared/requestbuffer/buffer.go | 102 ++++++++++++++++ .../shared/requestbuffer/buffer_test.go | 106 +++++++++++++++++ management/server/account_request_buffer.go | 111 +++--------------- 4 files changed, 258 insertions(+), 103 deletions(-) create mode 100644 management/internals/shared/requestbuffer/buffer.go create mode 100644 management/internals/shared/requestbuffer/buffer_test.go diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index 5567af0b2..778f73c01 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -21,6 +21,7 @@ import ( networkmapdb "github.com/netbirdio/netbird/management/internals/network_map_db" "github.com/netbirdio/netbird/management/internals/server/config" "github.com/netbirdio/netbird/management/internals/shared/grpc" + "github.com/netbirdio/netbird/management/internals/shared/requestbuffer" "github.com/netbirdio/netbird/management/server/account" "github.com/netbirdio/netbird/management/server/integrations/integrated_validator" "github.com/netbirdio/netbird/management/server/integrations/port_forwarding" @@ -39,6 +40,8 @@ import ( "github.com/netbirdio/netbird/version" ) +const defaultNetworkMapDataBufferInterval = 100 * time.Millisecond + type Controller struct { repo Repository metrics *metrics @@ -65,7 +68,8 @@ type Controller struct { perAccountServerSupportedSyncMessageVersions map[string]sharedgrpc.SyncMessageVersion - nmdataStore *networkmapdb.NetworkMapDBStoreImpl + nmdataStore *networkmapdb.NetworkMapDBStoreImpl + nmdataBuffer *requestbuffer.Buffer[*networkmap.NetworkMapData] } type bufferUpdate struct { @@ -89,7 +93,7 @@ func NewController(ctx context.Context, store store.Store, metrics telemetry.App log.Fatal(fmt.Errorf("error creating metrics: %w", err)) } - return &Controller{ + c := &Controller{ repo: newRepository(store), metrics: nMetrics, accountManagerMetrics: metrics.AccountManagerMetrics(), @@ -106,6 +110,14 @@ func NewController(ctx context.Context, store store.Store, metrics telemetry.App perAccountServerSupportedSyncMessageVersions: sharedgrpc.SyncMessageVersionsFromMap(config.PerAccountHighestSupportedSyncMessageVersion), nmdataStore: nmdataStore, } + + if nmdataStore != nil { + interval := requestbuffer.Interval(ctx, "NB_NETWORK_MAP_DATA_BUFFER_INTERVAL", defaultNetworkMapDataBufferInterval) + log.WithContext(ctx).Infof("set network map data request buffer interval to %s", interval) + c.nmdataBuffer = requestbuffer.New(ctx, "network map data request buffer", interval, c.fetchNetworkMapData) + } + + return c } func (c *Controller) OnPeerConnected(ctx context.Context, accountID string, peerID string) (chan *network_map.UpdateMessage, error) { @@ -392,8 +404,6 @@ func (c *Controller) sendUpdatesFromData(ctx context.Context, accountID string, return fmt.Errorf("failed to get flow enabled status: %v", err) } - nmData.PrecomputePostureValidation() - dnsCache := &cache.DNSConfigCache{} dnsDomain := c.getDNSDomainFromData(nmData.AccountSettings) peersCustomZone := networkmap.PeersCustomZone(ctx, accountID, dnsDomain, nmData.Peers, IPv6AllowedPeersFromData(nmData)) @@ -476,21 +486,37 @@ func (c *Controller) sendUpdatesFromData(ctx context.Context, accountID string, } func (c *Controller) getNetworkMapData(ctx context.Context, accountID string) *networkmap.NetworkMapData { - if c.nmdataStore == nil { + if c.nmdataBuffer == nil { return nil } - nmData, err := c.nmdataStore.GetNetworkMapData(ctx, accountID) + nmData, err := c.nmdataBuffer.Get(ctx, accountID) if err != nil { log.WithContext(ctx).Errorf("failed to get network map data for account %s, falling back to account-based computation: %v", accountID, err) return nil } - nmData.Services = c.proxyServicesFromRepo(ctx, accountID) - return nmData } +// fetchNetworkMapData reads the twin once per buffer window. Its result is +// shared by every waiter of that window, so the mutating steps run here, before +// it is handed out: the twin the callers see is read-only. Injected proxy +// policies carry no posture checks, so precomputing after the injection yields +// the same validation as precomputing before it. +func (c *Controller) fetchNetworkMapData(ctx context.Context, accountID string) (*networkmap.NetworkMapData, error) { + nmData, err := c.nmdataStore.GetNetworkMapData(ctx, accountID) + if err != nil { + return nil, err + } + + nmData.Services = c.proxyServicesFromRepo(ctx, accountID) + nmData.InjectProxyPolicies() + nmData.PrecomputePostureValidation() + + return nmData, nil +} + func (c *Controller) getDNSDomainFromData(settings *nmdata.AccountSettingsInfo) string { if settings == nil || settings.DNSDomain == "" { return c.dnsDomain diff --git a/management/internals/shared/requestbuffer/buffer.go b/management/internals/shared/requestbuffer/buffer.go new file mode 100644 index 000000000..c3823776c --- /dev/null +++ b/management/internals/shared/requestbuffer/buffer.go @@ -0,0 +1,102 @@ +// Package requestbuffer coalesces concurrent reads of the same expensive +// resource into a single fetch. +package requestbuffer + +import ( + "context" + "os" + "sync" + "time" + + log "github.com/sirupsen/logrus" +) + +// FetchFunc reads the resource identified by key. +type FetchFunc[T any] func(ctx context.Context, key string) (T, error) + +// Buffer batches requests per key: the first request opens a window, every +// request arriving within it joins the batch, and a single fetch serves them +// all. The fetch starts only after the window closed, so a caller never +// observes data read before its own request. +type Buffer[T any] struct { + ctx context.Context + name string + fetch FetchFunc[T] + interval time.Duration + + mu sync.Mutex + waiting map[string][]chan result[T] +} + +type result[T any] struct { + value T + err error +} + +// New returns a Buffer serving batched requests through fetch. ctx bounds the +// fetches, not the callers, and must outlive them. +func New[T any](ctx context.Context, name string, interval time.Duration, fetch FetchFunc[T]) *Buffer[T] { + return &Buffer[T]{ + ctx: ctx, + name: name, + fetch: fetch, + interval: interval, + waiting: make(map[string][]chan result[T]), + } +} + +// Get returns the value for key, sharing one fetch with the other callers of +// the current batch. The value is shared as is, so callers must treat it as +// read-only unless the fetch hands out copies. +func (b *Buffer[T]) Get(ctx context.Context, key string) (T, error) { + ch := make(chan result[T], 1) + + b.mu.Lock() + b.waiting[key] = append(b.waiting[key], ch) + first := len(b.waiting[key]) == 1 + b.mu.Unlock() + + if first { + time.AfterFunc(b.interval, func() { b.flush(key) }) + } + + select { + case res := <-ch: + return res.value, res.err + case <-ctx.Done(): + var zero T + return zero, ctx.Err() + } +} + +func (b *Buffer[T]) flush(key string) { + b.mu.Lock() + waiting := b.waiting[key] + delete(b.waiting, key) + b.mu.Unlock() + + if len(waiting) == 0 { + return + } + + start := time.Now() + value, err := b.fetch(b.ctx, key) + log.WithContext(b.ctx).Tracef("%s: fetched %s for %d waiters in %s", b.name, key, len(waiting), time.Since(start)) + + for _, ch := range waiting { + ch <- result[T]{value: value, err: err} + } +} + +// Interval reads a buffer interval from envVar, falling back to def. +func Interval(ctx context.Context, envVar string, def time.Duration) time.Duration { + value := os.Getenv(envVar) + interval, err := time.ParseDuration(value) + if err != nil { + if value != "" { + log.WithContext(ctx).Warnf("failed to parse %s: %s", envVar, err) + } + return def + } + return interval +} diff --git a/management/internals/shared/requestbuffer/buffer_test.go b/management/internals/shared/requestbuffer/buffer_test.go new file mode 100644 index 000000000..9e356145e --- /dev/null +++ b/management/internals/shared/requestbuffer/buffer_test.go @@ -0,0 +1,106 @@ +package requestbuffer + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBufferCoalescesConcurrentRequests(t *testing.T) { + var fetches atomic.Int32 + buffer := New(context.Background(), "test", 50*time.Millisecond, + func(ctx context.Context, key string) (string, error) { + fetches.Add(1) + return key, nil + }) + + var wg sync.WaitGroup + for range 10 { + wg.Add(1) + go func() { + defer wg.Done() + value, err := buffer.Get(context.Background(), "account") + assert.NoError(t, err) + assert.Equal(t, "account", value) + }() + } + wg.Wait() + + assert.Equal(t, int32(1), fetches.Load()) +} + +func TestBufferSeparatesKeys(t *testing.T) { + keys := make(chan string, 2) + buffer := New(context.Background(), "test", 10*time.Millisecond, + func(ctx context.Context, key string) (string, error) { + keys <- key + return key, nil + }) + + var wg sync.WaitGroup + for _, key := range []string{"a", "b"} { + wg.Add(1) + go func() { + defer wg.Done() + _, err := buffer.Get(context.Background(), key) + assert.NoError(t, err) + }() + } + wg.Wait() + close(keys) + + var fetched []string + for key := range keys { + fetched = append(fetched, key) + } + assert.ElementsMatch(t, []string{"a", "b"}, fetched) +} + +func TestBufferFetchesAfterRequest(t *testing.T) { + var version atomic.Int32 + buffer := New(context.Background(), "test", 10*time.Millisecond, + func(ctx context.Context, key string) (int32, error) { + return version.Load(), nil + }) + + first, err := buffer.Get(context.Background(), "account") + require.NoError(t, err) + assert.Equal(t, int32(0), first) + + version.Store(1) + + second, err := buffer.Get(context.Background(), "account") + require.NoError(t, err) + assert.Equal(t, int32(1), second) +} + +func TestBufferPropagatesError(t *testing.T) { + fetchErr := errors.New("fetch failed") + buffer := New(context.Background(), "test", 10*time.Millisecond, + func(ctx context.Context, key string) (*int, error) { + return nil, fetchErr + }) + + value, err := buffer.Get(context.Background(), "account") + assert.ErrorIs(t, err, fetchErr) + assert.Nil(t, value) +} + +func TestBufferHonorsCallerContext(t *testing.T) { + buffer := New(context.Background(), "test", time.Minute, + func(ctx context.Context, key string) (string, error) { + return key, nil + }) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + _, err := buffer.Get(ctx, "account") + assert.ErrorIs(t, err, context.DeadlineExceeded) +} diff --git a/management/server/account_request_buffer.go b/management/server/account_request_buffer.go index e1672c2d0..792099431 100644 --- a/management/server/account_request_buffer.go +++ b/management/server/account_request_buffer.go @@ -2,117 +2,38 @@ package server import ( "context" - "os" - "sync" "time" log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/management/internals/shared/requestbuffer" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" ) -// AccountRequest holds the result channel to return the requested account. -type AccountRequest struct { - AccountID string - ResultChan chan *AccountResult -} - -// AccountResult holds the account data or an error. -type AccountResult struct { - Account *types.Account - Err error -} +const defaultAccountBufferInterval = 100 * time.Millisecond type AccountRequestBuffer struct { - store store.Store - getAccountRequests map[string][]*AccountRequest - mu sync.Mutex - getAccountRequestCh chan *AccountRequest - bufferInterval time.Duration + buffer *requestbuffer.Buffer[*types.Account] } func NewAccountRequestBuffer(ctx context.Context, store store.Store) *AccountRequestBuffer { - bufferIntervalStr := os.Getenv("NB_GET_ACCOUNT_BUFFER_INTERVAL") - bufferInterval, err := time.ParseDuration(bufferIntervalStr) - if err != nil { - if bufferIntervalStr != "" { - log.WithContext(ctx).Warnf("failed to parse account request buffer interval: %s", err) - } - bufferInterval = 100 * time.Millisecond + interval := requestbuffer.Interval(ctx, "NB_GET_ACCOUNT_BUFFER_INTERVAL", defaultAccountBufferInterval) + log.WithContext(ctx).Infof("set account request buffer interval to %s", interval) + + return &AccountRequestBuffer{ + buffer: requestbuffer.New(ctx, "account request buffer", interval, store.GetAccount), } - - log.WithContext(ctx).Infof("set account request buffer interval to %s", bufferInterval) - - ac := AccountRequestBuffer{ - store: store, - getAccountRequests: make(map[string][]*AccountRequest), - getAccountRequestCh: make(chan *AccountRequest), - bufferInterval: bufferInterval, - } - - go ac.processGetAccountRequests(ctx) - - return &ac } + func (ac *AccountRequestBuffer) GetAccountWithBackpressure(ctx context.Context, accountID string) (*types.Account, error) { - req := &AccountRequest{ - AccountID: accountID, - ResultChan: make(chan *AccountResult, 1), + account, err := ac.buffer.Get(ctx, accountID) + if err != nil || account == nil { + return account, err } - log.WithContext(ctx).Tracef("requesting account %s with backpressure", accountID) - startTime := time.Now() - ac.getAccountRequestCh <- req - - result := <-req.ResultChan - log.WithContext(ctx).Tracef("got account with backpressure after %s", time.Since(startTime)) - return result.Account, result.Err -} - -func (ac *AccountRequestBuffer) processGetAccountBatch(ctx context.Context, accountID string) { - ac.mu.Lock() - requests := ac.getAccountRequests[accountID] - delete(ac.getAccountRequests, accountID) - ac.mu.Unlock() - - if len(requests) == 0 { - return - } - - startTime := time.Now() - account, err := ac.store.GetAccount(ctx, accountID) - log.WithContext(ctx).Tracef("getting account %s in batch took %s", accountID, time.Since(startTime)) - result := &AccountResult{Account: account, Err: err} - - for _, req := range requests { - if account != nil { - // Shallow copy the account so each goroutine gets its own struct value. - // This prevents data races when callers mutate fields like Policies. - accountCopy := *account - req.ResultChan <- &AccountResult{Account: &accountCopy, Err: err} - } else { - req.ResultChan <- result - } - close(req.ResultChan) - } -} - -func (ac *AccountRequestBuffer) processGetAccountRequests(ctx context.Context) { - for { - select { - case req := <-ac.getAccountRequestCh: - ac.mu.Lock() - ac.getAccountRequests[req.AccountID] = append(ac.getAccountRequests[req.AccountID], req) - if len(ac.getAccountRequests[req.AccountID]) == 1 { - go func(ctx context.Context, accountID string) { - time.Sleep(ac.bufferInterval) - ac.processGetAccountBatch(ctx, accountID) - }(ctx, req.AccountID) - } - ac.mu.Unlock() - case <-ctx.Done(): - return - } - } + // Shallow copy the account so each caller gets its own struct value. + // This prevents data races when callers mutate fields like Policies. + accountCopy := *account + return &accountCopy, nil } From 965deb4e682aa0620efcf2e7d74cb4863c9f7fff Mon Sep 17 00:00:00 2001 From: pascal Date: Fri, 21 Aug 2026 13:02:54 +0200 Subject: [PATCH 5/6] fix linter --- management/server/types/legacynmap/proto_legacy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/management/server/types/legacynmap/proto_legacy.go b/management/server/types/legacynmap/proto_legacy.go index 541f68e82..2bd8c8819 100644 --- a/management/server/types/legacynmap/proto_legacy.go +++ b/management/server/types/legacynmap/proto_legacy.go @@ -10,7 +10,7 @@ import ( "github.com/netbirdio/netbird/client/ssh/auth" nbconfig "github.com/netbirdio/netbird/management/internals/server/config" nbpeer "github.com/netbirdio/netbird/management/server/peer" - types "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/management/server/types" nbroute "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/networkmap" "github.com/netbirdio/netbird/shared/management/proto" @@ -89,7 +89,7 @@ func buildJWTConfig(config *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfi return &proto.JWTConfig{ Issuer: issuer, - Audience: audience, + Audience: audience, //nolint:staticcheck Audiences: audiences, KeysLocation: keysLocation, } From 1cef517a97f3154a78d81365f63fa8f5992c242a Mon Sep 17 00:00:00 2001 From: pascal Date: Fri, 21 Aug 2026 16:05:51 +0200 Subject: [PATCH 6/6] include NonAuthoritative zone in tests --- client/internal/dns_test.go | 87 +++++++++++++++++++ .../network_map_db/network_map_data.sql | 6 ++ .../network_map_data_golden.json | 26 ++++++ .../testdata/cases/dns-config/case.json | 2 +- .../cases/dns-config/golden/peer-a.json | 21 +++++ .../cases/dns-config/golden/peer-c.json | 13 +++ .../testdata/cases/dns-config/nmdata.json | 22 +++++ 7 files changed, 176 insertions(+), 1 deletion(-) diff --git a/client/internal/dns_test.go b/client/internal/dns_test.go index e15cc8fb7..031431efe 100644 --- a/client/internal/dns_test.go +++ b/client/internal/dns_test.go @@ -8,7 +8,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/client/iface/wgaddr" nbdns "github.com/netbirdio/netbird/dns" + mgmProto "github.com/netbirdio/netbird/shared/management/proto" ) func TestCreatePTRRecord_IPv4(t *testing.T) { @@ -136,3 +138,88 @@ func TestAddReverseZone_IPv6(t *testing.T) { assert.Len(t, reverseZone.Records, 1) assert.Equal(t, int(dns.TypePTR), reverseZone.Records[0].Type) } + +// TestToDNSConfig_ZoneFlagsPreserved pins the per-zone NonAuthoritative flag +// through the legacy DNSConfig path. A non-authoritative zone is match-only: +// the local resolver falls through to the upstream for an in-zone name it does +// not define. The built-in peer zone is the authoritative one and must stay +// that way, so the flag has to travel per zone rather than be derived. +func TestToDNSConfig_ZoneFlagsPreserved(t *testing.T) { + config := toDNSConfig(&mgmProto.DNSConfig{ + ServiceEnable: true, + CustomZones: []*mgmProto.CustomZone{ + { + Domain: "netbird.cloud.", + Records: []*mgmProto.SimpleRecord{ + {Name: "peer1.netbird.cloud.", Type: int64(dns.TypeA), Class: nbdns.DefaultClass, TTL: 300, RData: "100.64.0.1"}, + }, + }, + { + Domain: "corp.internal.", + NonAuthoritative: true, + SearchDomainDisabled: true, + Records: []*mgmProto.SimpleRecord{ + {Name: "db.corp.internal.", Type: int64(dns.TypeA), Class: nbdns.DefaultClass, TTL: 300, RData: "10.10.0.5"}, + }, + }, + }, + }, wgaddr.Address{ + IP: netip.MustParseAddr("100.64.0.1"), + Network: netip.MustParsePrefix("100.64.0.0/16"), + }) + + zones := make(map[string]nbdns.CustomZone, len(config.CustomZones)) + for _, zone := range config.CustomZones { + zones[zone.Domain] = zone + } + + peerZone, ok := zones["netbird.cloud."] + require.True(t, ok, "peer zone must survive") + assert.False(t, peerZone.NonAuthoritative, "the built-in peer zone owns the account domain and stays authoritative") + + accountZone, ok := zones["corp.internal."] + require.True(t, ok, "account zone must survive") + assert.True(t, accountZone.NonAuthoritative, "an account zone stays match-only, else undefined in-zone names get black-holed") + assert.True(t, accountZone.SearchDomainDisabled) +} + +// TestToDNSConfig_SingleZoneForcedAuthoritative pins the compatibility clause +// in toDNSConfig: a config carrying exactly one zone is treated as +// authoritative no matter what the server said, because servers that predate +// the NonAuthoritative field send only the peer FQDN zone. +// +// The clause can only ever downgrade an explicit true to false, so a server +// that legitimately sends a single non-authoritative zone — an account whose +// only zone is a custom one, with no peer records to build the built-in zone +// from — gets that zone's whole apex black-holed on the client. Real accounts +// always carry the peer zone alongside, which is why this is latent. Narrowing +// it needs a way to tell "unset" from "false" on the wire, or the account +// domain passed down here; until then this test states the contract so a +// change to it is deliberate. +func TestToDNSConfig_SingleZoneForcedAuthoritative(t *testing.T) { + config := toDNSConfig(&mgmProto.DNSConfig{ + ServiceEnable: true, + CustomZones: []*mgmProto.CustomZone{ + { + Domain: "corp.internal.", + NonAuthoritative: true, + Records: []*mgmProto.SimpleRecord{ + {Name: "db.corp.internal.", Type: int64(dns.TypeA), Class: nbdns.DefaultClass, TTL: 300, RData: "10.10.0.5"}, + }, + }, + }, + }, wgaddr.Address{ + IP: netip.MustParseAddr("100.64.0.1"), + Network: netip.MustParsePrefix("100.64.0.0/16"), + }) + + require.NotEmpty(t, config.CustomZones) + assert.Equal(t, "corp.internal.", config.CustomZones[0].Domain) + assert.False(t, config.CustomZones[0].NonAuthoritative, + "a lone zone is forced authoritative for pre-NonAuthoritative servers") + + // The reverse zone the config gains afterwards must not feed back into the + // decision: the compat gate counts the zones the server sent. + require.Len(t, config.CustomZones, 2, "a reverse zone is appended for the overlay prefix") + assert.Equal(t, "64.100.in-addr.arpa.", config.CustomZones[1].Domain) +} diff --git a/integration_tests/management/network_map_db/network_map_data.sql b/integration_tests/management/network_map_db/network_map_data.sql index 9f8e1bdb8..d94e2f4aa 100644 --- a/integration_tests/management/network_map_db/network_map_data.sql +++ b/integration_tests/management/network_map_db/network_map_data.sql @@ -47,8 +47,14 @@ insert into zones (id, account_id, domain, enabled, enable_search_domain, distri VALUES('zone-331','account-33','test-331.com',true,true,'["33-group-one-resource-id"]'); insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) VALUES('zone-332','account-33','disabled-331.com',false,true,'["33-group-one-resource-id"]'); +insert into zones (id, account_id, domain, enabled, enable_search_domain, distribution_groups) + VALUES('zone-333','account-33','search-off-331.com',true,false,'["33-group-two-resources-id"]'); insert into records (id, account_id, zone_id, name, type, ttl, content) VALUES('record-333','account-33','zone-332','test.disabled-331.com','A',1800,'1.1.1.9'); +insert into records (id, account_id, zone_id, name, type, ttl, content) + VALUES('record-334','account-33','zone-333','test.search-off-331.com','A',1800,'1.1.1.3'); +insert into records (id, account_id, zone_id, name, type, ttl, content) + VALUES('record-335','account-33','zone-333','alias.search-off-331.com','CNAME',1800,'test.search-off-331.com'); insert into records (id, account_id, zone_id, name, type, ttl, content) VALUES('record-331','account-33','zone-331','test.test-331.com','A',1800,'1.1.1.1'); insert into records (id, account_id, zone_id, name, type, ttl, content) diff --git a/integration_tests/management/network_map_db/network_map_data_golden.json b/integration_tests/management/network_map_db/network_map_data_golden.json index cff044d89..bb0ccd30b 100644 --- a/integration_tests/management/network_map_db/network_map_data_golden.json +++ b/integration_tests/management/network_map_db/network_map_data_golden.json @@ -513,6 +513,32 @@ "SearchDomainDisabled": false, "NonAuthoritative": true } + }, + { + "DistributionGroups": [ + "33-group-two-resources-id" + ], + "Zone": { + "Domain": "search-off-331.com.", + "Records": [ + { + "Name": "test.search-off-331.com.", + "Type": 1, + "Class": "IN", + "TTL": 1800, + "RData": "1.1.1.3" + }, + { + "Name": "alias.search-off-331.com.", + "Type": 5, + "Class": "IN", + "TTL": 1800, + "RData": "test.search-off-331.com." + } + ], + "SearchDomainDisabled": true, + "NonAuthoritative": true + } } ], "PrivateServiceCandidates": null, diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/case.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/case.json index 4844b02f2..e4c46c63d 100644 --- a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/case.json +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/case.json @@ -1,5 +1,5 @@ { - "description": "Nameserver group and applied custom zone distributed to grp-dev; peer-a (with an extra DNS label) receives them, peer-c outside the group receives neither.", + "description": "Nameserver group and applied custom zones distributed to grp-dev; peer-a (with an extra DNS label) receives them, peer-c is outside that group and receives only the zone distributed to grp-ops. Zone flags travel per zone: both grp-dev zones are match-only (NonAuthoritative), only search-off.internal. disables the search domain, and the built-in peer zone stays authoritative.", "peers": [ "peer-a", "peer-c" diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-a.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-a.json index a06f3c42d..f06a19d9d 100644 --- a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-a.json +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-a.json @@ -45,6 +45,27 @@ } ] }, + { + "Domain": "search-off.internal.", + "SearchDomainDisabled": true, + "NonAuthoritative": true, + "Records": [ + { + "Name": "alias.search-off.internal.", + "Type": "5", + "Class": "IN", + "TTL": "300", + "RData": "app.search-off.internal." + }, + { + "Name": "app.search-off.internal.", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "10.10.0.6" + } + ] + }, { "Domain": "netbird.test.", "Records": [ diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-c.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-c.json index cdb77d464..7e04dca40 100644 --- a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-c.json +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/golden/peer-c.json @@ -22,6 +22,19 @@ "RData": "100.64.0.3" } ] + }, + { + "Domain": "ops-only.internal.", + "NonAuthoritative": true, + "Records": [ + { + "Name": "tool.ops-only.internal.", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "10.10.0.7" + } + ] } ], "ForwarderPort": "22054" diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/nmdata.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/nmdata.json index 6efb3835b..b9741ef16 100644 --- a/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/nmdata.json +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/dns-config/nmdata.json @@ -47,6 +47,28 @@ {"Name": "db.corp.internal.", "Type": 1, "Class": "IN", "TTL": 300, "RData": "10.10.0.5"} ] } + }, + { + "DistributionGroups": ["grp-dev"], + "Zone": { + "Domain": "search-off.internal.", + "NonAuthoritative": true, + "SearchDomainDisabled": true, + "Records": [ + {"Name": "app.search-off.internal.", "Type": 1, "Class": "IN", "TTL": 300, "RData": "10.10.0.6"}, + {"Name": "alias.search-off.internal.", "Type": 5, "Class": "IN", "TTL": 300, "RData": "app.search-off.internal."} + ] + } + }, + { + "DistributionGroups": ["grp-ops"], + "Zone": { + "Domain": "ops-only.internal.", + "NonAuthoritative": true, + "Records": [ + {"Name": "tool.ops-only.internal.", "Type": 1, "Class": "IN", "TTL": 300, "RData": "10.10.0.7"} + ] + } } ] } \ No newline at end of file