From e83886b77bf96534d61072cf0855c5977a3aa5a5 Mon Sep 17 00:00:00 2001 From: pascal Date: Fri, 21 Aug 2026 11:03:58 +0200 Subject: [PATCH] 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)