[management] Clean up after account deletion (#7812)

Deleting an account left state behind that DeleteAccount's store
associations don't reach. The Agent Network tables outlived the account,
keeping its gateway domain claimed and its provider API keys stored. The
proxies kept serving its gateway until they next resynced. Cloud-side
state, such as managed proxy deployments, had no way to be cleaned up at
all.

Account deletion now runs registered hooks after the permission check
and before any users or data are removed. A failing hook aborts the
deletion. Agent Network registers one that tells the proxies to drop the
account's gateway mappings. The account's settings, providers, policies,
guardrails and budget rules are deleted in the account's transaction.
Consumption counters, and the access logs of deleted accounts, are left
to the background cleanup; usage records are kept.
This commit is contained in:
Brad Ison
2026-09-30 18:25:38 +02:00
committed by GitHub
parent 6c453a0f97
commit fd1a0203c7
19 changed files with 1023 additions and 27 deletions
+33
View File
@@ -112,6 +112,9 @@ type DefaultAccountManager struct {
permissionsManager permissions.Manager
disableDefaultPolicy bool
deletionHooksMu sync.RWMutex
deletionHooks []account.DeletionHook
}
var _ account.Manager = (*DefaultAccountManager)(nil)
@@ -120,6 +123,32 @@ func (am *DefaultAccountManager) SetServiceManager(serviceManager service.Manage
am.serviceManager = serviceManager
}
// AddAccountDeletionHook registers hook to run on every account deletion. Hooks run in
// registration order, and the first one to fail stops the rest and aborts the deletion.
// It panics on a nil hook: dropping one silently would skip that hook's cleanup on every
// deletion, so the wiring bug surfaces at startup instead.
func (am *DefaultAccountManager) AddAccountDeletionHook(hook account.DeletionHook) {
if hook == nil {
panic("nil account deletion hook")
}
am.deletionHooksMu.Lock()
defer am.deletionHooksMu.Unlock()
am.deletionHooks = append(am.deletionHooks, hook)
}
func (am *DefaultAccountManager) runAccountDeletionHooks(ctx context.Context, accountID string) error {
am.deletionHooksMu.RLock()
hooks := slices.Clone(am.deletionHooks)
am.deletionHooksMu.RUnlock()
for _, hook := range hooks {
if err := hook(ctx, accountID); err != nil {
return fmt.Errorf("account deletion hook: %w", err)
}
}
return nil
}
func isUniqueConstraintError(err error) bool {
switch {
case strings.Contains(err.Error(), "(SQLSTATE 23505)"),
@@ -889,6 +918,10 @@ func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, u
return status.Errorf(status.Internal, "failed to build user infos for account %s: %v", accountID, err)
}
if err = am.runAccountDeletionHooks(ctx, accountID); err != nil {
return err
}
if err = am.deleteAccountUsers(ctx, accountID, userID, account.Users, userInfosMap); err != nil {
return err
}
@@ -0,0 +1,14 @@
package account
import "context"
// DeletionHook runs when an account is deleted, after the caller's permission to delete
// it has been checked and before any of its users or data are removed. It lets code that
// keeps per-account state outside the store tear that state down while the account still
// exists.
//
// A hook that returns an error aborts the deletion and the account is kept. The caller
// sees the error, so a hook that wants a specific response returns a status error. A
// retried deletion runs every hook again, and a later step can still fail after the hooks
// succeed, so a hook must be idempotent and must tolerate the account surviving it.
type DeletionHook func(ctx context.Context, accountID string) error
+95
View File
@@ -958,6 +958,101 @@ func TestAccountManager_DeleteAccount(t *testing.T) {
assert.Len(t, pats, 0)
}
func TestAccountManager_DeleteAccount_RunsDeletionHooks(t *testing.T) {
manager, _, err := createManager(t)
require.NoError(t, err)
ownerID := "account_creator"
account, err := createAccount(manager, "test_account", ownerID, "")
require.NoError(t, err)
// Each hook records its call and checks the account is still in the store, which is
// the point of running before deletion: a hook must be able to read what it cleans up.
var calls []string
hook := func(name string) nbAccount.DeletionHook {
return func(ctx context.Context, accountID string) error {
calls = append(calls, name+":"+accountID)
_, err := manager.Store.GetAccount(ctx, accountID)
assert.NoError(t, err, "account should still exist while hook %s runs", name)
return nil
}
}
manager.AddAccountDeletionHook(hook("first"))
manager.AddAccountDeletionHook(hook("second"))
require.NoError(t, manager.DeleteAccount(context.Background(), account.Id, ownerID))
assert.Equal(t, []string{"first:" + account.Id, "second:" + account.Id}, calls,
"hooks should run once each, in registration order, with the deleted account's ID")
_, err = manager.Store.GetAccount(context.Background(), account.Id)
assert.Error(t, err, "account should be deleted after the hooks succeed")
}
func TestAccountManager_DeleteAccount_DeletionHookErrorAbortsDeletion(t *testing.T) {
manager, _, err := createManager(t)
require.NoError(t, err)
ownerID := "account_creator"
account, err := createAccount(manager, "test_account", ownerID, "")
require.NoError(t, err)
manager.AddAccountDeletionHook(func(context.Context, string) error {
return status.Errorf(status.PreconditionFailed, "teardown refused")
})
secondCalled := false
manager.AddAccountDeletionHook(func(context.Context, string) error {
secondCalled = true
return nil
})
err = manager.DeleteAccount(context.Background(), account.Id, ownerID)
require.Error(t, err)
// The hook's status type has to survive the wrapping, since the HTTP layer maps it
// to the response code.
sErr, ok := status.FromError(err)
require.True(t, ok, "error should carry the hook's status error, got %v", err)
assert.Equal(t, status.PreconditionFailed, sErr.Type(), "status type should be the hook's")
assert.False(t, secondCalled, "hooks after a failing one should not run")
_, err = manager.Store.GetAccount(context.Background(), account.Id)
assert.NoError(t, err, "account should survive a failing hook")
_, err = manager.Store.GetUserByUserID(context.Background(), store.LockingStrengthNone, ownerID)
assert.NoError(t, err, "account owner should survive a failing hook")
}
func TestAccountManager_AddAccountDeletionHook_RejectsNil(t *testing.T) {
manager, _, err := createManager(t)
require.NoError(t, err)
assert.PanicsWithValue(t, "nil account deletion hook", func() {
manager.AddAccountDeletionHook(nil)
}, "registering a nil hook should panic instead of breaking a later deletion")
}
func TestAccountManager_DeleteAccount_DeletionHooksSkippedWithoutPermission(t *testing.T) {
manager, _, err := createManager(t)
require.NoError(t, err)
ownerID := "account_creator"
account, err := createAccount(manager, "test_account", ownerID, "")
require.NoError(t, err)
adminID := "regular_admin"
account.Users[adminID] = types.NewAdminUser(adminID)
require.NoError(t, manager.Store.SaveAccount(context.Background(), account))
called := false
manager.AddAccountDeletionHook(func(context.Context, string) error {
called = true
return nil
})
err = manager.DeleteAccount(context.Background(), account.Id, adminID)
require.Error(t, err, "only the owner may delete the account")
assert.False(t, called, "hooks should not run for a caller who may not delete the account")
}
func BenchmarkTest_GetAccountWithclaims(b *testing.B) {
claims := auth.UserAuth{
Domain: "example.com",
@@ -18,6 +18,7 @@ import (
"gorm.io/gorm/clause"
nbdns "github.com/netbirdio/netbird/dns"
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
nbpeer "github.com/netbirdio/netbird/management/server/peer"
"github.com/netbirdio/netbird/management/server/types"
"github.com/netbirdio/netbird/route"
@@ -162,6 +163,10 @@ func (s *SqlStore) DeleteAccount(ctx context.Context, account *types.Account) er
return result.Error
}
if err := deleteAgentNetworkAccountConfig(tx, account.Id); err != nil {
return err
}
result = tx.Select(clause.Associations).Delete(account)
if result.Error != nil {
return result.Error
@@ -179,6 +184,29 @@ func (s *SqlStore) DeleteAccount(ctx context.Context, account *types.Account) er
return err
}
// deleteAgentNetworkAccountConfig removes the account's agent network configuration. These
// tables are not account associations, so deleting the account does not reach them. The
// settings row holds the account's globally unique gateway domain and the provider rows
// hold its upstream API keys. Tables that grow with traffic are left out: consumption
// counters and access logs are swept in the background, and usage records are kept.
func deleteAgentNetworkAccountConfig(tx *gorm.DB, accountID string) error {
// Dependents first: policies point at providers and guardrails, and settings
// go last, as DeleteSettings refuses while providers exist.
models := []any{
&agentNetworkTypes.Policy{},
&agentNetworkTypes.Provider{},
&agentNetworkTypes.Guardrail{},
&agentNetworkTypes.AccountBudgetRule{},
&agentNetworkTypes.Settings{},
}
for _, model := range models {
if err := tx.Delete(model, "account_id = ?", accountID).Error; err != nil {
return fmt.Errorf("delete %T rows: %w", model, err)
}
}
return nil
}
func (s *SqlStore) UpdateAccountDomainAttributes(ctx context.Context, accountID string, domain string, category string, isPrimaryDomain bool) error {
accountCopy := types.Account{
Domain: domain,
@@ -16,6 +16,7 @@ import (
"github.com/stretchr/testify/require"
nbdns "github.com/netbirdio/netbird/dns"
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
proxydomain "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types"
@@ -401,6 +402,25 @@ func TestSqlite_DeleteAccount(t *testing.T) {
require.NoError(t, err)
require.Equal(t, o.AccountID, account.Id)
err = store.CreateAgentNetworkSettings(context.Background(), &agentNetworkTypes.Settings{
AccountID: account.Id,
Domain: "gw.example.com",
ProxyAddress: "gw.example.com",
})
require.NoError(t, err)
agentNetworkConfig := []any{
&agentNetworkTypes.Provider{ID: "an_provider", AccountID: account.Id, APIKey: "sk-test"},
&agentNetworkTypes.Policy{ID: "an_policy", AccountID: account.Id},
&agentNetworkTypes.Guardrail{ID: "an_guardrail", AccountID: account.Id},
&agentNetworkTypes.AccountBudgetRule{ID: "an_budget_rule", AccountID: account.Id},
}
for _, row := range agentNetworkConfig {
require.NoError(t, store.(*SqlStore).db.Create(row).Error, "creating %T", row)
}
otherProvider := &agentNetworkTypes.Provider{ID: "other_provider", AccountID: "other_account"}
require.NoError(t, store.(*SqlStore).db.Create(otherProvider).Error)
err = store.DeleteAccount(context.Background(), account)
require.NoError(t, err)
@@ -465,6 +485,32 @@ func TestSqlite_DeleteAccount(t *testing.T) {
err = store.(*SqlStore).db.Model(&rpservice.Target{}).Find(&targets, "account_id = ?", account.Id).Error
require.NoError(t, err, "expecting no error after DeleteAccount when searching for service targets")
require.Len(t, targets, 0, "expecting no service targets to be found after DeleteAccount")
_, err = store.GetAgentNetworkSettings(context.Background(), LockingStrengthNone, account.Id)
require.Error(t, err, "expecting agent network settings to be deleted with the account")
sErr, ok := status.FromError(err)
require.True(t, ok, "expecting a status error when getting agent network settings, got %v", err)
require.Equal(t, status.NotFound, sErr.Type(), "expecting agent network settings to be deleted with the account")
// The domain is globally unique, so a leftover row would keep it from another account.
err = store.CreateAgentNetworkSettings(context.Background(), &agentNetworkTypes.Settings{
AccountID: "other_account",
Domain: "gw.example.com",
ProxyAddress: "gw.example.com",
})
require.NoError(t, err, "expecting the deleted account's gateway domain to be free for another account")
for _, row := range agentNetworkConfig {
var count int64
err = store.(*SqlStore).db.Model(row).Where("account_id = ?", account.Id).Count(&count).Error
require.NoError(t, err, "counting %T rows after DeleteAccount", row)
assert.Zero(t, count, "expecting no %T rows to be found after DeleteAccount", row)
}
var otherProviders int64
err = store.(*SqlStore).db.Model(&agentNetworkTypes.Provider{}).Where("account_id = ?", "other_account").Count(&otherProviders).Error
require.NoError(t, err)
assert.Equal(t, int64(1), otherProviders, "expecting another account's agent network provider to survive DeleteAccount")
}
func Test_GetAccount(t *testing.T) {
@@ -69,6 +69,23 @@ func (s *SqlStore) DeleteOldAgentNetworkAccessLogs(ctx context.Context, accountI
return deleted, nil
}
// GetDeletedAccountIDsWithAgentNetworkAccessLogs returns the IDs of accounts that no
// longer exist but still have access-log rows. The retention sweep is driven by settings
// rows, which are deleted with the account, so it uses this to find logs it would
// otherwise never expire.
func (s *SqlStore) GetDeletedAccountIDsWithAgentNetworkAccessLogs(ctx context.Context) ([]string, error) {
var accountIDs []string
err := s.db.Model(&agentNetworkTypes.AgentNetworkAccessLog{}).
Distinct("account_id").
Where("NOT EXISTS (SELECT 1 FROM accounts WHERE accounts.id = agent_network_access_log.account_id)").
Pluck("account_id", &accountIDs).Error
if err != nil {
log.WithContext(ctx).Errorf("failed to get deleted accounts with agent-network access logs: %v", err)
return nil, status.Errorf(status.Internal, "failed to get deleted accounts with agent-network access logs")
}
return accountIDs, nil
}
// GetAgentNetworkAccessLogs retrieves flattened agent-network access logs for
// an account with server-side pagination, filtering and sorting. Authorising
// group ids are hydrated from the group child table for the returned page.
@@ -663,6 +663,21 @@ func (s *SqlStore) IncrementAgentNetworkConsumptionBatch(
return nil
}
// DeleteAgentNetworkConsumptionOfDeletedAccounts deletes every consumption counter whose
// account no longer exists and returns the number of rows deleted. Counters grow with
// traffic, so they are swept in the background instead of in the account-deletion
// transaction, and the sweep also catches counters a proxy writes after the deletion.
func (s *SqlStore) DeleteAgentNetworkConsumptionOfDeletedAccounts(ctx context.Context) (int64, error) {
res := s.db.
Where("NOT EXISTS (SELECT 1 FROM accounts WHERE accounts.id = agent_network_consumption.account_id)").
Delete(&agentNetworkTypes.Consumption{})
if res.Error != nil {
log.WithContext(ctx).Errorf("failed to delete agent-network consumption of deleted accounts: %v", res.Error)
return 0, status.Errorf(status.Internal, "failed to delete agent-network consumption of deleted accounts")
}
return res.RowsAffected, nil
}
// ListAgentNetworkConsumption returns every consumption row recorded
// for the account, ordered by window_start descending. Backs the
// dashboard's basic counter view.
@@ -9,6 +9,7 @@ import (
"github.com/stretchr/testify/require"
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
"github.com/netbirdio/netbird/management/server/types"
)
// TestAgentNetworkUsage_RealStore_RoundTrip drives CreateAgentNetworkUsage and
@@ -300,3 +301,37 @@ func TestDeleteOldAgentNetworkAccessLogs(t *testing.T) {
require.NoError(t, err)
require.Len(t, usage, 1, "usage record for the deleted log must survive")
}
// TestDeleteAgentNetworkConsumptionOfDeletedAccounts verifies that the sweep removes the
// consumption counters of accounts that no longer exist and leaves live accounts' counters,
// including those of a live account without a settings row.
func TestDeleteAgentNetworkConsumptionOfDeletedAccounts(t *testing.T) {
runTestForAllEngines(t, "", func(t *testing.T, s Store) {
ctx := context.Background()
const (
liveAccountID = "acc-anet-consumption-live"
deletedAccountID = "acc-anet-consumption-deleted"
)
require.NoError(t, s.SaveAccount(ctx, &types.Account{Id: liveAccountID}))
windowStart := time.Now().UTC().Truncate(time.Hour)
for _, accountID := range []string{liveAccountID, deletedAccountID} {
for _, dimID := range []string{"user-1", "user-2"} {
require.NoError(t, s.IncrementAgentNetworkConsumption(ctx, accountID,
agentNetworkTypes.DimensionUser, dimID, 3600, windowStart, 10, 5, 0.01))
}
}
deleted, err := s.DeleteAgentNetworkConsumptionOfDeletedAccounts(ctx)
require.NoError(t, err)
assert.Equal(t, int64(2), deleted, "both of the deleted account's counters should be removed")
rows, err := s.ListAgentNetworkConsumption(ctx, LockingStrengthNone, deletedAccountID)
require.NoError(t, err)
assert.Empty(t, rows, "the deleted account should have no consumption counters left")
rows, err = s.ListAgentNetworkConsumption(ctx, LockingStrengthNone, liveAccountID)
require.NoError(t, err)
assert.Len(t, rows, 2, "the live account's consumption counters should survive")
})
}
+2
View File
@@ -320,6 +320,7 @@ type Store interface {
GetAgentNetworkAccessLogSessions(ctx context.Context, lockStrength LockingStrength, accountID string, filter agentNetworkTypes.AgentNetworkAccessLogFilter) ([]*agentNetworkTypes.AgentNetworkAccessLogSession, int64, error)
GetAgentNetworkUsageRows(ctx context.Context, lockStrength LockingStrength, accountID string, filter agentNetworkTypes.AgentNetworkAccessLogFilter) ([]*agentNetworkTypes.AgentNetworkUsage, error)
DeleteOldAgentNetworkAccessLogs(ctx context.Context, accountID string, olderThan time.Time) (int64, error)
GetDeletedAccountIDsWithAgentNetworkAccessLogs(ctx context.Context) ([]string, error)
GetServiceTargetByTargetID(ctx context.Context, lockStrength LockingStrength, accountID string, targetID string) (*rpservice.Target, error)
GetTargetsByServiceID(ctx context.Context, lockStrength LockingStrength, accountID string, serviceID string) ([]*rpservice.Target, error)
DeleteTarget(ctx context.Context, accountID string, serviceID string, targetID uint) error
@@ -388,6 +389,7 @@ type Store interface {
GetAgentNetworkConsumption(ctx context.Context, lockStrength LockingStrength, accountID string, kind agentNetworkTypes.ConsumptionDimension, dimID string, windowSeconds int64, windowStart time.Time) (*agentNetworkTypes.Consumption, error)
GetAgentNetworkConsumptionBatch(ctx context.Context, lockStrength LockingStrength, accountID string, keys []agentNetworkTypes.ConsumptionKey) (map[agentNetworkTypes.ConsumptionKey]*agentNetworkTypes.Consumption, error)
ListAgentNetworkConsumption(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*agentNetworkTypes.Consumption, error)
DeleteAgentNetworkConsumptionOfDeletedAccounts(ctx context.Context) (int64, error)
GetAccountAgentNetworkBudgetRules(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*agentNetworkTypes.AccountBudgetRule, error)
GetAgentNetworkBudgetRuleByID(ctx context.Context, lockStrength LockingStrength, accountID, ruleID string) (*agentNetworkTypes.AccountBudgetRule, error)
SaveAgentNetworkBudgetRule(ctx context.Context, rule *agentNetworkTypes.AccountBudgetRule) error
+30
View File
@@ -457,6 +457,21 @@ func (mr *MockStoreMockRecorder) DeleteAgentNetworkBudgetRule(ctx, accountID, ru
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAgentNetworkBudgetRule", reflect.TypeOf((*MockStore)(nil).DeleteAgentNetworkBudgetRule), ctx, accountID, ruleID)
}
// DeleteAgentNetworkConsumptionOfDeletedAccounts mocks base method.
func (m *MockStore) DeleteAgentNetworkConsumptionOfDeletedAccounts(ctx context.Context) (int64, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "DeleteAgentNetworkConsumptionOfDeletedAccounts", ctx)
ret0, _ := ret[0].(int64)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// DeleteAgentNetworkConsumptionOfDeletedAccounts indicates an expected call of DeleteAgentNetworkConsumptionOfDeletedAccounts.
func (mr *MockStoreMockRecorder) DeleteAgentNetworkConsumptionOfDeletedAccounts(ctx any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAgentNetworkConsumptionOfDeletedAccounts", reflect.TypeOf((*MockStore)(nil).DeleteAgentNetworkConsumptionOfDeletedAccounts), ctx)
}
// DeleteAgentNetworkGuardrail mocks base method.
func (m *MockStore) DeleteAgentNetworkGuardrail(ctx context.Context, accountID, guardrailID string) error {
m.ctrl.T.Helper()
@@ -1972,6 +1987,21 @@ func (mr *MockStoreMockRecorder) GetDNSRecordByID(ctx, lockStrength, accountID,
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDNSRecordByID", reflect.TypeOf((*MockStore)(nil).GetDNSRecordByID), ctx, lockStrength, accountID, zoneID, recordID)
}
// GetDeletedAccountIDsWithAgentNetworkAccessLogs mocks base method.
func (m *MockStore) GetDeletedAccountIDsWithAgentNetworkAccessLogs(ctx context.Context) ([]string, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetDeletedAccountIDsWithAgentNetworkAccessLogs", ctx)
ret0, _ := ret[0].([]string)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetDeletedAccountIDsWithAgentNetworkAccessLogs indicates an expected call of GetDeletedAccountIDsWithAgentNetworkAccessLogs.
func (mr *MockStoreMockRecorder) GetDeletedAccountIDsWithAgentNetworkAccessLogs(ctx any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDeletedAccountIDsWithAgentNetworkAccessLogs", reflect.TypeOf((*MockStore)(nil).GetDeletedAccountIDsWithAgentNetworkAccessLogs), ctx)
}
// GetEmbeddedProxyPeerIDsByCluster mocks base method.
func (m *MockStore) GetEmbeddedProxyPeerIDsByCluster(ctx context.Context, accountID string) (map[string][]string, error) {
m.ctrl.T.Helper()