From f6109a3395f9ce34649cc2af7dc2638d28e1d98e Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:44:10 +0900 Subject: [PATCH 1/3] [client] Remove the empty GPO DNS policy store on Windows teardown (#7563) --- client/internal/dns/host_windows.go | 98 ++++++++++++++--- client/internal/dns/host_windows_test.go | 129 +++++++++++++++++++++++ 2 files changed, 213 insertions(+), 14 deletions(-) diff --git a/client/internal/dns/host_windows.go b/client/internal/dns/host_windows.go index 948000a3d..6462d0c37 100644 --- a/client/internal/dns/host_windows.go +++ b/client/internal/dns/host_windows.go @@ -124,19 +124,9 @@ func newHostManager(wgInterface WGIface) (*registryConfigurator, error) { return nil, err } - var useGPO bool - k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE) - if err != nil { - log.Debugf("failed to open GPO DNS policy root: %v", err) - } else { - closer(k) - useGPO = true - log.Infof("detected GPO DNS policy configuration, using policy store") - } - configurator := ®istryConfigurator{ guid: guid, - gpo: useGPO, + gpo: useGPOPolicyStore(), } origNameservers, err := configurator.captureOriginalNameservers() @@ -576,14 +566,22 @@ func (r *registryConfigurator) setInterfaceRegistryKeyStringValue(key, value str return nil } +// deleteInterfaceRegistryKeyProperty removes a value from the interface key. +// A value that is already gone, or an interface key that is, is not an error: +// the caller asked for the value not to be there, and a cleanup that runs twice +// has to reach its later steps on the second run as well. func (r *registryConfigurator) deleteInterfaceRegistryKeyProperty(propertyKey string) error { regKey, err := r.getInterfaceRegistryKey() - if err != nil { + switch { + case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND): + log.Debugf("interface key of %s does not exist, nothing to delete %s from", r.guid, propertyKey) + return nil + case err != nil: return fmt.Errorf("get interface registry key: %w", err) } defer closer(regKey) - if err := regKey.DeleteValue(propertyKey); err != nil { + if err := regKey.DeleteValue(propertyKey); err != nil && !errors.Is(err, registry.ErrNotExist) { return fmt.Errorf("delete registry key %s: %w", propertyKey, err) } return nil @@ -612,7 +610,12 @@ func (r *registryConfigurator) restoreHostDNS() error { go r.flushDNSCache() - return nil + // Last, and only on the way out, once no rule of ours is left: during a + // session the store is where the rules of this run live, and emptying it + // mid-session would have the next rule recreate it anyway. Propagated so a + // failure keeps the shutdown state for the next run to retry, rather than + // leaving the store to hold up every rule change from here on. + return removeEmptyGPOPolicyStore() } // removeDNSMatchPolicies deletes every NRPT rule this client may have created, @@ -651,6 +654,73 @@ func (r *registryConfigurator) restoreUncleanShutdownDNS() error { return r.restoreHostDNS() } +// useGPOPolicyStore reports whether NRPT rules have to go into the group policy +// store, and clears an empty one out of the way first. +// +// The order is the point. A store left empty by an earlier run would otherwise +// decide this run too, sending its rules somewhere the resolver only reads when +// the policy engine next applies DNS client policy. Removing it before the +// choice is made leaves the local store authoritative for the whole session, +// including the first one after an upgrade. +func useGPOPolicyStore() bool { + if err := removeEmptyGPOPolicyStore(); err != nil { + // Nothing to retry against here: the worst case is the run going + // through the group policy store, which is where it would have gone + // before this check existed. + log.Warnf("%v", err) + } + + k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE) + if err != nil { + log.Debugf("failed to open GPO DNS policy root: %v", err) + return false + } + closer(k) + + log.Infof("detected GPO DNS policy configuration, using policy store") + return true +} + +// removeEmptyGPOPolicyStore deletes the group policy DnsPolicyConfig key once +// nothing is left in it. The key survives the deletion of the last rule it +// held, and the client treats its presence as "group policy configures the +// NRPT", so an empty one left behind keeps every later run writing rules there. +// Rules in that store reach the resolver only when the policy engine next +// applies DNS client policy, and a rule this client writes belongs to no GPO, +// so nothing schedules that application: both adding and removing a rule are +// held up by a minute or more, and for a removal that is a catch-all rule +// resolving every name over an interface that no longer exists. With the store +// absent the local one is authoritative and a change applies at once. +// +// A store that still holds rules, values or subkeys of somebody else's is left +// alone. +func removeEmptyGPOPolicyStore() error { + k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE) + switch { + case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND): + return nil + case err != nil: + return fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err) + } + + info, err := k.Stat() + closer(k) + if err != nil { + return fmt.Errorf("stat HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err) + } + + if info.SubKeyCount != 0 || info.ValueCount != 0 { + return nil + } + + if err := registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot); err != nil { + return fmt.Errorf("delete empty HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err) + } + + log.Infof("removed the empty GPO DNS policy store, leaving the local one authoritative") + return nil +} + // listNRPTRuleKeys returns the names of our NRPT rule keys under a policy store // root. An absent root holds nothing to clean up, which is the normal state of // the GPO store on a machine without DNS Client policy. diff --git a/client/internal/dns/host_windows_test.go b/client/internal/dns/host_windows_test.go index 7aef64590..353f6adbc 100644 --- a/client/internal/dns/host_windows_test.go +++ b/client/internal/dns/host_windows_test.go @@ -8,6 +8,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/sys/windows/registry" + + "github.com/netbirdio/netbird/client/internal/winregistry" ) // TestNRPTEntriesCleanupOnConfigChange tests that old NRPT entries are properly cleaned up @@ -405,3 +407,130 @@ func TestNRPTDomainBatching(t *testing.T) { }) } } + +// TestRemoveEmptyGPOPolicyStore verifies that cleanup takes the GPO policy +// store itself with it once our rules are gone, since the store existing keeps +// the local one from being applied, and that a store with somebody else's rule +// in it is left alone. +func TestRemoveEmptyGPOPolicyStore(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry integration test in short mode") + } + + t.Cleanup(func() { cleanupRegistryKeys(t) }) + cleanupRegistryKeys(t) + + testIP := netip.MustParseAddr("100.64.0.1") + cfg := ®istryConfigurator{gpo: true} + + // a store holding a rule of ours is kept, because the rule is still applied + require.NoError(t, cfg.addDNSMatchPolicy([]string{".example.com"}, testIP)) + exists, err := registryKeyExists(gpoDnsPolicyConfigMatchPath + "-0") + require.NoError(t, err) + require.True(t, exists, "Should write the rule to the GPO policy store") + + require.NoError(t, removeEmptyGPOPolicyStore()) + exists, err = registryKeyExists(GPODNSPolicyConfigRoot) + require.NoError(t, err) + assert.True(t, exists, "Should keep a policy store that still holds a rule") + + // once the rules are gone the store goes with them + require.NoError(t, cfg.removeDNSMatchPolicies()) + require.NoError(t, removeEmptyGPOPolicyStore()) + + exists, err = registryKeyExists(GPODNSPolicyConfigRoot) + require.NoError(t, err) + assert.False(t, exists, "Should remove the GPO policy store once it is empty") + + // A store is not ours to remove while somebody else has a rule in it. The + // rule is written volatile like our own: the rules above created the parent + // chain volatile, and Windows refuses a stable subkey under a volatile + // parent. + foreignRule := GPODNSPolicyConfigRoot + `\{2A3B4C5D-6E7F-4041-8283-84858687888A}` + foreignKey, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, foreignRule, registry.SET_VALUE) + require.NoError(t, err, "Should create a foreign GPO rule") + foreignKey.Close() + t.Cleanup(func() { + _ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignRule) + _ = registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot) + }) + + require.NoError(t, cfg.removeDNSMatchPolicies()) + require.NoError(t, removeEmptyGPOPolicyStore()) + + exists, err = registryKeyExists(foreignRule) + require.NoError(t, err) + assert.True(t, exists, "Should not remove a foreign rule") + exists, err = registryKeyExists(GPODNSPolicyConfigRoot) + require.NoError(t, err) + assert.True(t, exists, "Should keep a policy store that still holds a foreign rule") +} + +// TestDeleteInterfaceRegistryKeyPropertyTwice verifies that removing a value +// that is already gone, or one on an interface key that is, reports success. +// Teardown runs again after a failed cleanup, and the steps that follow this +// one have to be reached on that second run. +func TestDeleteInterfaceRegistryKeyPropertyTwice(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry integration test in short mode") + } + + testGUID := "{12345678-1234-1234-1234-123456789ABC}" + interfacePath := InterfaceConfigPath + `\` + testGUID + testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE) + require.NoError(t, err, "Should create test interface registry key") + testKey.Close() + t.Cleanup(func() { + _ = registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath) + }) + + cfg := ®istryConfigurator{guid: testGUID} + + require.NoError(t, cfg.setInterfaceRegistryKeyStringValue(interfaceConfigSearchListKey, "example.com")) + require.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey)) + assert.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey), + "Should report success for a value that is already gone") + + // and with the interface key itself gone, as it is once the adapter is + require.NoError(t, registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath)) + assert.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey), + "Should report success when the interface key does not exist") +} + +// TestUseGPOPolicyStoreClearsEmptyStore verifies that the store is cleared +// before it is consulted, so an empty one left by an earlier run does not send +// this run's rules to the group policy store. A store somebody else has a rule +// in still decides where the rules go. +func TestUseGPOPolicyStoreClearsEmptyStore(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry integration test in short mode") + } + + t.Cleanup(func() { cleanupRegistryKeys(t) }) + cleanupRegistryKeys(t) + + // the leftover an earlier run used to keep, which the client read as + // "group policy configures the NRPT" for every run after it + emptyStore, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.SET_VALUE) + require.NoError(t, err, "Should create the GPO policy store") + emptyStore.Close() + + assert.False(t, useGPOPolicyStore(), "An empty store should not decide where the rules go") + exists, err := registryKeyExists(GPODNSPolicyConfigRoot) + require.NoError(t, err) + assert.False(t, exists, "Should clear the empty store before consulting it") + + foreignRule := GPODNSPolicyConfigRoot + `\{2A3B4C5D-6E7F-4041-8283-84858687888A}` + foreignKey, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, foreignRule, registry.SET_VALUE) + require.NoError(t, err, "Should create a foreign GPO rule") + foreignKey.Close() + t.Cleanup(func() { + _ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignRule) + _ = registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot) + }) + + assert.True(t, useGPOPolicyStore(), "A store holding a rule should decide where the rules go") + exists, err = registryKeyExists(GPODNSPolicyConfigRoot) + require.NoError(t, err) + assert.True(t, exists, "Should keep a store that holds a rule") +} From 9a5395d3140956389433410192ca8e2753baf7f4 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:16:57 +0200 Subject: [PATCH 2/3] [management] add store support to filter by public id (#7208) --- .../networks/resources/types/resource.go | 2 +- management/server/store/sql_store.go | 68 ++++++++++++++++ management/server/store/sql_store_test.go | 78 +++++++++++++++++++ management/server/store/store.go | 3 + management/server/store/store_mock.go | 45 +++++++++++ management/server/types/policy.go | 2 +- route/route.go | 2 +- 7 files changed, 197 insertions(+), 3 deletions(-) diff --git a/management/server/networks/resources/types/resource.go b/management/server/networks/resources/types/resource.go index 4cf7f7ea3..bb33e00eb 100644 --- a/management/server/networks/resources/types/resource.go +++ b/management/server/networks/resources/types/resource.go @@ -32,7 +32,7 @@ type NetworkResource struct { ID string `gorm:"primaryKey"` NetworkID string `gorm:"index"` AccountID string `gorm:"index"` - PublicID string `json:"-"` + PublicID string `json:"-" gorm:"index"` Name string Description string Type NetworkResourceType diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 1e99251b2..b32be5af9 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -58,6 +58,7 @@ const ( keyQueryCondition = "key = ?" mysqlKeyQueryCondition = "`key` = ?" accountAndIDQueryCondition = "account_id = ? and id = ?" + accountAndAnyIDQueryCondition = "account_id = ? and (id = ? or public_id = ?)" accountAndPeerIDQueryCondition = "account_id = ? and peer_id = ?" accountAndIDsQueryCondition = "account_id = ? AND id IN ?" accountIDCondition = "account_id = ?" @@ -4063,6 +4064,30 @@ func (s *SqlStore) GetPolicyByID(ctx context.Context, lockStrength LockingStreng return policy, nil } +// GetPolicyByIDOrPublicID retrieves a policy by either its ID or its PublicID. Peers report +// whichever of the two the network map they were served carries, so callers resolving a +// peer-reported reference cannot know upfront which namespace it belongs to. +func (s *SqlStore) GetPolicyByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, policyID string) (*types.Policy, error) { + tx := s.db + if lockStrength != LockingStrengthNone { + tx = tx.Clauses(clause.Locking{Strength: string(lockStrength)}) + } + + var policy *types.Policy + + result := tx.Preload(clause.Associations). + Take(&policy, accountAndAnyIDQueryCondition, accountID, policyID, policyID) + if err := result.Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, status.NewPolicyNotFoundError(policyID) + } + log.WithContext(ctx).Errorf("failed to get policy from store: %s", err) + return nil, status.Errorf(status.Internal, "failed to get policy from store") + } + + return policy, nil +} + func (s *SqlStore) CreatePolicy(ctx context.Context, policy *types.Policy) error { result := s.db.Create(policy) if result.Error != nil { @@ -4248,6 +4273,27 @@ func (s *SqlStore) GetRouteByID(ctx context.Context, lockStrength LockingStrengt return route, nil } +// GetRouteByIDOrPublicID retrieves a route by either its ID or its PublicID. See +// GetPolicyByIDOrPublicID for why peer-reported references need both. +func (s *SqlStore) GetRouteByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID string, routeID string) (*route.Route, error) { + tx := s.db + if lockStrength != LockingStrengthNone { + tx = tx.Clauses(clause.Locking{Strength: string(lockStrength)}) + } + + var route *route.Route + result := tx.Take(&route, accountAndAnyIDQueryCondition, accountID, routeID, routeID) + if err := result.Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil, status.NewRouteNotFoundError(routeID) + } + log.WithContext(ctx).Errorf("failed to get route from the store: %s", err) + return nil, status.Errorf(status.Internal, "failed to get route from store") + } + + return route, nil +} + // SaveRoute saves a route to the database. func (s *SqlStore) SaveRoute(ctx context.Context, route *route.Route) error { result := s.db.Save(route) @@ -4642,6 +4688,28 @@ func (s *SqlStore) GetNetworkResourceByID(ctx context.Context, lockStrength Lock return netResources, nil } +// GetNetworkResourceByIDOrPublicID retrieves a network resource by either its ID or its +// PublicID. See GetPolicyByIDOrPublicID for why peer-reported references need both. +func (s *SqlStore) GetNetworkResourceByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, resourceID string) (*resourceTypes.NetworkResource, error) { + tx := s.db + if lockStrength != LockingStrengthNone { + tx = tx.Clauses(clause.Locking{Strength: string(lockStrength)}) + } + + var netResources *resourceTypes.NetworkResource + result := tx. + Take(&netResources, accountAndAnyIDQueryCondition, accountID, resourceID, resourceID) + if result.Error != nil { + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return nil, status.NewNetworkResourceNotFoundError(resourceID) + } + log.WithContext(ctx).Errorf("failed to get network resource from store: %v", result.Error) + return nil, status.Errorf(status.Internal, "failed to get network resource from store") + } + + return netResources, nil +} + func (s *SqlStore) GetNetworkResourceByName(ctx context.Context, lockStrength LockingStrength, accountID, resourceName string) (*resourceTypes.NetworkResource, error) { tx := s.db if lockStrength != LockingStrengthNone { diff --git a/management/server/store/sql_store_test.go b/management/server/store/sql_store_test.go index fbcff5257..73132bf75 100644 --- a/management/server/store/sql_store_test.go +++ b/management/server/store/sql_store_test.go @@ -1972,6 +1972,32 @@ func TestSqlStore_GetPolicyByID(t *testing.T) { } } +func TestSqlStore_GetPolicyByIDOrPublicID(t *testing.T) { + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/store.sql", t.TempDir()) + t.Cleanup(cleanup) + require.NoError(t, err) + + accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + policyID := "cs1tnh0hhcjnqoiuebf0" + + policy, err := store.GetPolicyByID(context.Background(), LockingStrengthNone, accountID, policyID) + require.NoError(t, err) + require.NotEmpty(t, policy.PublicID) + + for _, id := range []string{policyID, policy.PublicID} { + policy, err := store.GetPolicyByIDOrPublicID(context.Background(), LockingStrengthNone, accountID, id) + require.NoError(t, err) + require.Equal(t, policyID, policy.ID) + } + + policy, err = store.GetPolicyByIDOrPublicID(context.Background(), LockingStrengthNone, accountID, "non-existing") + require.Error(t, err) + sErr, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, sErr.Type(), status.NotFound) + require.Nil(t, policy) +} + func TestSqlStore_CreatePolicy(t *testing.T) { store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/store.sql", t.TempDir()) t.Cleanup(cleanup) @@ -2631,6 +2657,32 @@ func TestSqlStore_GetNetworkResourceByID(t *testing.T) { } } +func TestSqlStore_GetNetworkResourceByIDOrPublicID(t *testing.T) { + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/store.sql", t.TempDir()) + t.Cleanup(cleanup) + require.NoError(t, err) + + accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + netResourceID := "ctc4nci7qv9061u6ilfg" + + netResource, err := store.GetNetworkResourceByID(context.Background(), LockingStrengthNone, accountID, netResourceID) + require.NoError(t, err) + require.NotEmpty(t, netResource.PublicID) + + for _, id := range []string{netResourceID, netResource.PublicID} { + netResource, err := store.GetNetworkResourceByIDOrPublicID(context.Background(), LockingStrengthNone, accountID, id) + require.NoError(t, err) + require.Equal(t, netResourceID, netResource.ID) + } + + netResource, err = store.GetNetworkResourceByIDOrPublicID(context.Background(), LockingStrengthNone, accountID, "non-existing") + require.Error(t, err) + sErr, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, sErr.Type(), status.NotFound) + require.Nil(t, netResource) +} + func TestSqlStore_SaveNetworkResource(t *testing.T) { store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/store.sql", t.TempDir()) t.Cleanup(cleanup) @@ -3756,6 +3808,32 @@ func TestSqlStore_GetRouteByID(t *testing.T) { } } +func TestSqlStore_GetRouteByIDOrPublicID(t *testing.T) { + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/extended-store.sql", t.TempDir()) + t.Cleanup(cleanup) + require.NoError(t, err) + + accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + routeID := "ct03t427qv97vmtmglog" + + route, err := store.GetRouteByID(context.Background(), LockingStrengthNone, accountID, routeID) + require.NoError(t, err) + require.NotEmpty(t, route.PublicID) + + for _, id := range []string{routeID, route.PublicID} { + route, err := store.GetRouteByIDOrPublicID(context.Background(), LockingStrengthNone, accountID, id) + require.NoError(t, err) + require.Equal(t, routeID, string(route.ID)) + } + + route, err = store.GetRouteByIDOrPublicID(context.Background(), LockingStrengthNone, accountID, "non-existing") + require.Error(t, err) + sErr, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, sErr.Type(), status.NotFound) + require.Nil(t, route) +} + func TestSqlStore_SaveRoute(t *testing.T) { store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/extended-store.sql", t.TempDir()) t.Cleanup(cleanup) diff --git a/management/server/store/store.go b/management/server/store/store.go index 97da95b4c..b6368f47f 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -138,6 +138,7 @@ type Store interface { GetAccountPolicies(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*types.Policy, error) GetPolicyByID(ctx context.Context, lockStrength LockingStrength, accountID, policyID string) (*types.Policy, error) + GetPolicyByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, policyID string) (*types.Policy, error) CreatePolicy(ctx context.Context, policy *types.Policy) error SavePolicy(ctx context.Context, policy *types.Policy) error DeletePolicy(ctx context.Context, accountID, policyID string) error @@ -208,6 +209,7 @@ type Store interface { GetAccountRoutes(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*route.Route, error) GetRouteByID(ctx context.Context, lockStrength LockingStrength, accountID, routeID string) (*route.Route, error) + GetRouteByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, routeID string) (*route.Route, error) SaveRoute(ctx context.Context, route *route.Route) error DeleteRoute(ctx context.Context, accountID, routeID string) error @@ -248,6 +250,7 @@ type Store interface { GetNetworkResourcesByNetID(ctx context.Context, lockStrength LockingStrength, accountID, netID string) ([]*resourceTypes.NetworkResource, error) GetNetworkResourcesByAccountID(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*resourceTypes.NetworkResource, error) GetNetworkResourceByID(ctx context.Context, lockStrength LockingStrength, accountID, resourceID string) (*resourceTypes.NetworkResource, error) + GetNetworkResourceByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, resourceID string) (*resourceTypes.NetworkResource, error) GetNetworkResourceByName(ctx context.Context, lockStrength LockingStrength, accountID, resourceName string) (*resourceTypes.NetworkResource, error) SaveNetworkResource(ctx context.Context, resource *resourceTypes.NetworkResource) error DeleteNetworkResource(ctx context.Context, accountID, resourceID string) error diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 399a07a19..460ba712b 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -2166,6 +2166,21 @@ func (mr *MockStoreMockRecorder) GetNetworkResourceByID(ctx, lockStrength, accou return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkResourceByID", reflect.TypeOf((*MockStore)(nil).GetNetworkResourceByID), ctx, lockStrength, accountID, resourceID) } +// GetNetworkResourceByIDOrPublicID mocks base method. +func (m *MockStore) GetNetworkResourceByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, resourceID string) (*types0.NetworkResource, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetNetworkResourceByIDOrPublicID", ctx, lockStrength, accountID, resourceID) + ret0, _ := ret[0].(*types0.NetworkResource) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetNetworkResourceByIDOrPublicID indicates an expected call of GetNetworkResourceByIDOrPublicID. +func (mr *MockStoreMockRecorder) GetNetworkResourceByIDOrPublicID(ctx, lockStrength, accountID, resourceID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNetworkResourceByIDOrPublicID", reflect.TypeOf((*MockStore)(nil).GetNetworkResourceByIDOrPublicID), ctx, lockStrength, accountID, resourceID) +} + // GetNetworkResourceByName mocks base method. func (m *MockStore) GetNetworkResourceByName(ctx context.Context, lockStrength LockingStrength, accountID, resourceName string) (*types0.NetworkResource, error) { m.ctrl.T.Helper() @@ -2496,6 +2511,21 @@ func (mr *MockStoreMockRecorder) GetPolicyByID(ctx, lockStrength, accountID, pol return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPolicyByID", reflect.TypeOf((*MockStore)(nil).GetPolicyByID), ctx, lockStrength, accountID, policyID) } +// GetPolicyByIDOrPublicID mocks base method. +func (m *MockStore) GetPolicyByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, policyID string) (*types3.Policy, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPolicyByIDOrPublicID", ctx, lockStrength, accountID, policyID) + ret0, _ := ret[0].(*types3.Policy) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetPolicyByIDOrPublicID indicates an expected call of GetPolicyByIDOrPublicID. +func (mr *MockStoreMockRecorder) GetPolicyByIDOrPublicID(ctx, lockStrength, accountID, policyID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPolicyByIDOrPublicID", reflect.TypeOf((*MockStore)(nil).GetPolicyByIDOrPublicID), ctx, lockStrength, accountID, policyID) +} + // GetPolicyRulesByResourceID mocks base method. func (m *MockStore) GetPolicyRulesByResourceID(ctx context.Context, lockStrength LockingStrength, accountID, peerID string) ([]*types3.PolicyRule, error) { m.ctrl.T.Helper() @@ -2676,6 +2706,21 @@ func (mr *MockStoreMockRecorder) GetRouteByID(ctx, lockStrength, accountID, rout return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRouteByID", reflect.TypeOf((*MockStore)(nil).GetRouteByID), ctx, lockStrength, accountID, routeID) } +// GetRouteByIDOrPublicID mocks base method. +func (m *MockStore) GetRouteByIDOrPublicID(ctx context.Context, lockStrength LockingStrength, accountID, routeID string) (*route.Route, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRouteByIDOrPublicID", ctx, lockStrength, accountID, routeID) + ret0, _ := ret[0].(*route.Route) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetRouteByIDOrPublicID indicates an expected call of GetRouteByIDOrPublicID. +func (mr *MockStoreMockRecorder) GetRouteByIDOrPublicID(ctx, lockStrength, accountID, routeID interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRouteByIDOrPublicID", reflect.TypeOf((*MockStore)(nil).GetRouteByIDOrPublicID), ctx, lockStrength, accountID, routeID) +} + // GetRoutingPeerNetworks mocks base method. func (m *MockStore) GetRoutingPeerNetworks(ctx context.Context, accountID, peerID string) ([]string, error) { m.ctrl.T.Helper() diff --git a/management/server/types/policy.go b/management/server/types/policy.go index 0f7298d18..9786d17b6 100644 --- a/management/server/types/policy.go +++ b/management/server/types/policy.go @@ -29,7 +29,7 @@ type Policy struct { // ID of the policy' ID string `gorm:"primaryKey"` - PublicID string `json:"-"` + PublicID string `json:"-" gorm:"index"` // AccountID is a reference to Account that this object belongs AccountID string `json:"-" gorm:"index"` diff --git a/route/route.go b/route/route.go index 3bdb0a3a1..ef9a39ef7 100644 --- a/route/route.go +++ b/route/route.go @@ -95,7 +95,7 @@ type Route struct { ID ID `gorm:"primaryKey"` // AccountID is a reference to Account that this object belongs AccountID string `gorm:"index"` - PublicID string `json:"-"` + PublicID string `json:"-" gorm:"index"` // Network and Domains are mutually exclusive Network netip.Prefix `gorm:"serializer:json"` Domains domain.List `gorm:"serializer:json"` From cb7ca8ef3f02cf747d35412362598c57784eb79c Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Wed, 23 Sep 2026 11:50:44 +0200 Subject: [PATCH 3/3] [client,management] Skip route firewall rule computation when no firewall (#7624) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [client,management] Skip route firewall rule computation when no firewall A peer that runs with the firewall disabled has no ACL manager and no firewall to program, so nothing ever reads RoutesFirewallRules: the only consumers are acl.Manager, which is reached solely when e.acl is set, and the legacy-management probe in updateNetworkMap, which is guarded by a non-nil firewall. Building those rules is the most expensive part of a sync on a peer that routes many network resources. On a 15k-peer deployment a debug bundle showed getPeerNetworkResourceFirewallRules accounting for 62% of the allocations of Calculate, and Calculate for effectively all of the allocations of handleSync, which was taking 3.2s on average and holding the engine lock for the duration. Let the caller ask Calculate to leave the rules out. The client passes its existing DisableFirewall setting; the management server keeps the default and still produces them. RoutesFirewallRulesIsEmpty is set from the resulting empty list, so a receiver that would otherwise infer legacy management from an empty rule set does not misread the skip. * [client,management] Cover the skip flag through the envelope Review feedback on #7624. The components test compared only the length of the peer firewall rules, so a change to their content would have passed while the message claimed they came out unchanged. Compare the slices. The skip path was also only exercised by setting the field directly on the components, which bypasses the envelope conversion where RoutesFirewallRulesIsEmpty is derived. That bit is what keeps the client from reading skipped rules as a legacy management server, so it gets a test that goes through EnvelopeToNetworkMap with the flag set. * [management] Give the router a peer ACL so the rule comparison bites Review feedback on #7624. peer-router-1 appears in no peer ACL in the shared fixture, so its FirewallRules came out empty and the equality assertion compared two empty slices — it would have passed even if the peer rules were dropped entirely. Add a policy covering the router and require the baseline to be non-empty before comparing. --- client/internal/engine.go | 6 +- .../network_map/nmaptest/runner.go | 2 +- .../types/networkmap_components_test.go | 57 ++++++++ shared/management/networkmap/envelope.go | 8 +- shared/management/networkmap/envelope_test.go | 124 ++++++++++++++++-- .../management/types/networkmap_components.go | 15 ++- 6 files changed, 199 insertions(+), 13 deletions(-) diff --git a/client/internal/engine.go b/client/internal/engine.go index d517d1d68..1613bbbc9 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -1061,7 +1061,11 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error { // back to empty if the FQDN doesn't have the expected shape. dnsName = extractDNSDomainFromFQDN(pc.GetFqdn()) } - result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName) + // With the firewall disabled there is no ACL manager to program, so + // RoutesFirewallRules would be built and then dropped. On a peer that + // routes many network resources that is the single most expensive + // step of the sync. + result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName, e.config.DisableFirewall) if err != nil { return fmt.Errorf("decode network map envelope: %w", err) } diff --git a/management/internals/controllers/network_map/nmaptest/runner.go b/management/internals/controllers/network_map/nmaptest/runner.go index 0d6ac9c18..ffce6483e 100644 --- a/management/internals/controllers/network_map/nmaptest/runner.go +++ b/management/internals/controllers/network_map/nmaptest/runner.go @@ -245,7 +245,7 @@ func computeMode(t *testing.T, ctx context.Context, mode Mode, nmData *networkma peerGroups := maps.Keys(nmData.GetPeerGroups(peerID)) resp := mgmtgrpc.ToComponentSyncResponse(ctx, nil, nil, nil, peer, nil, nil, components, nil, dnsDomain, nil, nmData.AccountSettings, nil, peerGroups, dnsFwdPort) - res, err := networkmap.EnvelopeToNetworkMap(ctx, resp.NetworkMapEnvelope, peer.Key, dnsDomain) + res, err := networkmap.EnvelopeToNetworkMap(ctx, resp.NetworkMapEnvelope, peer.Key, dnsDomain, false) require.NoError(t, err, "expand envelope") return res.NetworkMap default: diff --git a/management/server/types/networkmap_components_test.go b/management/server/types/networkmap_components_test.go index f6d542609..9e76de775 100644 --- a/management/server/types/networkmap_components_test.go +++ b/management/server/types/networkmap_components_test.go @@ -175,6 +175,63 @@ func TestNetworkMapComponents_NetworkResourceRoutes_RouterPeer(t *testing.T) { assert.NotEmpty(t, nm.RoutesFirewallRules, "router peer should have route firewall rules for the resource") } +// A receiver without a firewall asks Calculate to skip the route firewall +// rules. Everything the rest of the sync consumes — routes, peers, peer +// firewall rules — must come out unchanged. +func TestNetworkMapComponents_SkipRouteFirewallRules(t *testing.T) { + ctx := context.Background() + account := createComponentTestAccount() + + // The shared fixture leaves peer-router-1 out of every peer ACL, so its + // FirewallRules would be empty and the comparison below vacuous. Give the + // router a policy of its own. + account.Policies = append(account.Policies, &types.Policy{ + ID: "policy-router", Name: "Router connectivity", Enabled: true, + Rules: []*types.PolicyRule{{ + ID: "rule-router", Name: "Allow all <-> router", Enabled: true, + Action: types.PolicyTrafficActionAccept, Protocol: types.PolicyRuleProtocolALL, + Bidirectional: true, + Sources: []string{"group-all"}, Destinations: []string{"group-all"}, + }}, + }) + + validated := allPeersValidated(account) + + components := account.GetPeerNetworkMapComponents( + ctx, + "peer-router-1", + account.GetPeersCustomZone(ctx, "netbird.io"), + nil, + validated, + account.GetResourcePoliciesMap(), + account.GetResourceRoutersMap(), + account.GetActiveGroupUsers(), + ) + + full := components.Calculate(ctx) + require.NotEmpty(t, full.RoutesFirewallRules, "baseline: router peer must get route firewall rules") + require.NotEmpty(t, full.FirewallRules, "baseline: router peer must get peer firewall rules") + + components.SkipRouteFirewallRules = true + skipped := components.Calculate(ctx) + + assert.Empty(t, skipped.RoutesFirewallRules, "route firewall rules must not be computed when skipped") + assert.ElementsMatch(t, routeNetworks(full.Routes), routeNetworks(skipped.Routes), + "skipping route firewall rules must not change the routes") + assert.ElementsMatch(t, peerIDs(full.Peers), peerIDs(skipped.Peers), + "skipping route firewall rules must not change the peers to connect") + assert.Equal(t, full.FirewallRules, skipped.FirewallRules, + "peer firewall rules are unrelated and must come out unchanged") +} + +func routeNetworks(routes []*nmdata.Route) []string { + networks := make([]string, 0, len(routes)) + for _, r := range routes { + networks = append(networks, r.Network.String()) + } + return networks +} + func TestNetworkMapComponents_NetworkResourceRoutes_UnrelatedPeer(t *testing.T) { account := createComponentTestAccount() validated := allPeersValidated(account) diff --git a/shared/management/networkmap/envelope.go b/shared/management/networkmap/envelope.go index e7961fd7b..fd9dd6bbd 100644 --- a/shared/management/networkmap/envelope.go +++ b/shared/management/networkmap/envelope.go @@ -35,7 +35,12 @@ type EnvelopeResult struct { // // dnsName is the account's DNS domain ("netbird.cloud" etc.); used when // rebuilding the per-peer FQDNs that proto.RemotePeerConfig carries. -func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, localPeerKey, dnsName string) (*EnvelopeResult, error) { +// +// skipRouteFirewallRules leaves RoutesFirewallRules empty. Callers that have +// no firewall to program pass true: the rules are the most expensive part of +// Calculate on a peer that routes many network resources, and nothing reads +// them afterwards. +func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, localPeerKey, dnsName string, skipRouteFirewallRules bool) (*EnvelopeResult, error) { components, err := DecodeEnvelope(ctx, env) if err != nil { return nil, fmt.Errorf("decode envelope: %w", err) @@ -53,6 +58,7 @@ func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, lo return nil, fmt.Errorf("receiving peer (wg_key prefix %q) not found among %d decoded peers — components have no PeerID, Calculate would return empty", trimKey(localPeerKey), len(components.Peers)) } components.PeerID = canonicalKey + components.SkipRouteFirewallRules = skipRouteFirewallRules includeIPv6 := localPeer.SupportsIPv6() && localPeer.IPv6.IsValid() useSourcePrefixes := localPeer.SupportsSourcePrefixes() diff --git a/shared/management/networkmap/envelope_test.go b/shared/management/networkmap/envelope_test.go index 7fe2a5277..98333a3c8 100644 --- a/shared/management/networkmap/envelope_test.go +++ b/shared/management/networkmap/envelope_test.go @@ -9,6 +9,7 @@ import ( "net/netip" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" goproto "google.golang.org/protobuf/proto" @@ -37,7 +38,7 @@ func TestEnvelopeToNetworkMap_RoundTrip(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap") require.NotNil(t, result) require.NotNil(t, result.NetworkMap, "decoded NetworkMap must be non-nil") @@ -78,7 +79,7 @@ func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded)) - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err) require.NotEmpty(t, result.NetworkMap.FirewallRules, "ssh policy should produce firewall rules") for i, fr := range result.NetworkMap.FirewallRules { @@ -88,13 +89,13 @@ func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) { } func TestEnvelopeToNetworkMap_NilEnvelope(t *testing.T) { - _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), nil, "key", "netbird.cloud") + _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), nil, "key", "netbird.cloud", false) require.Error(t, err, "nil envelope must produce an error rather than panic") } func TestEnvelopeToNetworkMap_FullPayloadMissing(t *testing.T) { env := &proto.NetworkMapEnvelope{} - _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), env, "key", "netbird.cloud") + _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), env, "key", "netbird.cloud", false) require.Error(t, err, "envelope with no Full payload must produce an error") } @@ -126,7 +127,7 @@ func TestDecodeEnvelope_MalformedWgKeyPeerSkipped(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap must tolerate one bad peer key") require.NotNil(t, result) require.NotNil(t, result.Components) @@ -195,7 +196,7 @@ func TestEnvelopeRoundTrip_AllGroupShortCircuitParity(t *testing.T) { var decodedEnv proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decodedEnv), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decodedEnv, peers["peer-T"].Key, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decodedEnv, peers["peer-T"].Key, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap") clientNM := result.NetworkMap @@ -253,7 +254,7 @@ func TestEnvelopeToNetworkMap_EmptyComponents(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap must degrade gracefully on empty components") require.Equal(t, uint64(7), result.NetworkMap.Serial) require.Empty(t, result.NetworkMap.RemotePeers, "unvalidated peer connects to nobody") @@ -276,7 +277,7 @@ func TestEnvelopeToNetworkMap_MissingNetwork(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "a missing AccountNetwork must not panic the client") require.NotNil(t, result.Components.Network) require.NotEmpty(t, result.NetworkMap.RemotePeers, "the rest of the snapshot stays usable") @@ -353,3 +354,110 @@ func randomWgKey(t *testing.T) string { require.NoError(t, err) return base64.StdEncoding.EncodeToString(raw[:]) } + +// TestEnvelopeToNetworkMap_SkipRouteFirewallRules covers the flag end to end, +// through the envelope rather than by poking Calculate directly. The +// RoutesFirewallRulesIsEmpty derivation is the part that matters: the client's +// legacy-management probe reads an empty rule list together with that bit, so +// skipping the rules must set it rather than leave it false. +func TestEnvelopeToNetworkMap_SkipRouteFirewallRules(t *testing.T) { + ctx := context.Background() + c, routerKey := buildRoutedResourceComponents(t) + + envelope := mgmtgrpc.EncodeNetworkMapEnvelope(mgmtgrpc.ComponentsEnvelopeInput{ + Components: c, + DNSDomain: "netbird.cloud", + }) + wire, err := goproto.Marshal(envelope) + require.NoError(t, err, "marshal envelope") + var decoded proto.NetworkMapEnvelope + require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") + + full, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decoded, routerKey, "netbird.cloud", false) + require.NoError(t, err, "EnvelopeToNetworkMap without skip") + require.NotEmpty(t, full.NetworkMap.RoutesFirewallRules, + "baseline: the router peer must receive route firewall rules") + require.False(t, full.NetworkMap.RoutesFirewallRulesIsEmpty, + "baseline: the empty bit must be false when rules are present") + + var decodedSkip proto.NetworkMapEnvelope + require.NoError(t, goproto.Unmarshal(wire, &decodedSkip), "unmarshal envelope") + skipped, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decodedSkip, routerKey, "netbird.cloud", true) + require.NoError(t, err, "EnvelopeToNetworkMap with skip") + + assert.Empty(t, skipped.NetworkMap.RoutesFirewallRules, + "route firewall rules must not be computed when skipped") + assert.True(t, skipped.NetworkMap.RoutesFirewallRulesIsEmpty, + "the empty bit must be derived from the skipped list, or the client misreads it as legacy management") + assert.Len(t, skipped.NetworkMap.Routes, len(full.NetworkMap.Routes), + "skipping route firewall rules must not change the routes") + assert.Len(t, skipped.NetworkMap.RemotePeers, len(full.NetworkMap.RemotePeers), + "skipping route firewall rules must not change the remote peers") +} + +// buildRoutedResourceComponents returns components in which the local peer is +// the routing peer for one enabled network resource, reachable by a second +// peer through a resource policy — the minimum shape that yields a non-empty +// RoutesFirewallRules. It also returns the local peer's WG key. +func buildRoutedResourceComponents(t *testing.T) (*types.NetworkMapComponents, string) { + t.Helper() + + routerKey := randomWgKey(t) + peers := map[string]*nmdata.Peer{ + "peer-R": { + ID: "peer-R", Key: routerKey, DNSLabel: "router", + IP: netip.AddrFrom4([4]byte{100, 64, 0, 1}), + Meta: nmdata.PeerSystemMeta{WtVersion: "0.40.0"}, + }, + "peer-S": { + ID: "peer-S", Key: randomWgKey(t), DNSLabel: "source", + IP: netip.AddrFrom4([4]byte{100, 64, 0, 2}), + Meta: nmdata.PeerSystemMeta{WtVersion: "0.40.0"}, + }, + } + + resourcePolicy := &nmdata.Policy{ + ID: "pol-res", PublicID: "10", Enabled: true, + Rules: []*nmdata.PolicyRule{{ + ID: "rule-res", + Enabled: true, + Action: string(types.PolicyTrafficActionAccept), + Protocol: string(types.PolicyRuleProtocolALL), + Sources: []string{"g-src"}, + }}, + } + + c := &types.NetworkMapComponents{ + PeerID: "peer-R", + Network: &nmdata.Network{ + Identifier: "net-routed-resource", + Net: net.IPNet{IP: net.IP{100, 64, 0, 0}, Mask: net.CIDRMask(10, 32)}, + Serial: 1, + }, + AccountSettings: &nmdata.AccountSettingsInfo{}, + DNSSettings: &nmdata.DNSSettings{}, + Peers: peers, + Groups: map[string]*nmdata.Group{ + "g-src": {PublicID: "1", Name: "sources", Peers: []string{"peer-S"}}, + "g-routers": {PublicID: "2", Name: "routers", Peers: []string{"peer-R"}}, + }, + NetworkResources: []*nmdata.NetworkResource{{ + ID: "res-1", NetworkID: "netid-1", PublicID: "100", Name: "res1", + Type: "subnet", + Prefix: netip.MustParsePrefix("10.200.0.0/24"), + Enabled: true, + }}, + RoutersMap: map[string]map[string]*nmdata.NetworkRouter{ + "netid-1": {"peer-R": { + PublicID: "200", PeerGroups: []string{"g-routers"}, Metric: 9999, Enabled: true, + }}, + }, + ResourcePoliciesMap: map[string][]*nmdata.Policy{ + "res-1": {resourcePolicy}, + }, + Policies: []*nmdata.Policy{resourcePolicy}, + NetworkXIDToPublicID: map[string]string{"netid-1": "1"}, + } + + return c, routerKey +} diff --git a/shared/management/types/networkmap_components.go b/shared/management/types/networkmap_components.go index e18db4ec0..7742ca244 100644 --- a/shared/management/types/networkmap_components.go +++ b/shared/management/types/networkmap_components.go @@ -58,6 +58,13 @@ type NetworkMapComponents struct { // domain targets. ForceRoutingPeerDNSResolution bool + // SkipRouteFirewallRules drops the route firewall rule computation from + // Calculate. A receiver without a firewall manager never reads + // RoutesFirewallRules, and on a routing peer with many network resources + // building them dominates the cost of a sync. Defaults to false so the + // management server keeps producing them. + SkipRouteFirewallRules bool + routesByPeerOnce sync.Once routesByPeerIdx map[string][]routeIndexEntry @@ -149,11 +156,15 @@ func (c *NetworkMapComponents) Calculate(ctx context.Context) *NetworkMap { includeIPv6 = p.SupportsIPv6() && p.IPv6.IsValid() } routesUpdate := filterAndExpandRoutes(c.getRoutesToSync(targetPeerID, peersToConnect, peerGroups), includeIPv6) - routesFirewallRules := c.getPeerRoutesFirewallRules(ctx, targetPeerID, includeIPv6) + + var routesFirewallRules []*RouteFirewallRule + if !c.SkipRouteFirewallRules { + routesFirewallRules = c.getPeerRoutesFirewallRules(ctx, targetPeerID, includeIPv6) + } isRouter, networkResourcesRoutes, sourcePeers := c.getNetworkResourcesRoutesToSync(targetPeerID) var networkResourcesFirewallRules []*RouteFirewallRule - if isRouter { + if isRouter && !c.SkipRouteFirewallRules { networkResourcesFirewallRules = c.getPeerNetworkResourceFirewallRules(ctx, targetPeerID, networkResourcesRoutes, includeIPv6) }