Skip to content

[management] update account delete with proper proxy domain and service cleanup #5817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pascal-fischer merged 5 commits into from
Apr 10, 2026
Merged

[management] update account delete with proper proxy domain and service cleanup #5817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34e720b
update account delete with proper proxy domain and service cleanup
pascal-fischer Apr 7, 2026
15d3043
fix tests
pascal-fischer Apr 7, 2026
b706444
fix tests
pascal-fischer Apr 7, 2026
8f973d2
fix tests
pascal-fischer Apr 7, 2026
93fc09a
test service and domain delete
pascal-fischer Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions management/internals/modules/reverseproxy/domain/domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,8 @@ func (d *Domain) EventMeta() map[string]any {
"validated": d.Validated,
}
}

func (d *Domain) Copy() *Domain {
dCopy := *d
return &dCopy
}
5 changes: 0 additions & 5 deletions management/server/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,11 +742,6 @@ 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)
}

err = am.serviceManager.DeleteAllServices(ctx, accountID, userID)
if err != nil {
return status.Errorf(status.Internal, "failed to delete service %s: %v", accountID, err)
}

for _, otherUser := range account.Users {
if otherUser.Id == userID {
continue
Expand Down
11 changes: 10 additions & 1 deletion management/server/account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,16 @@ import (
"time"

"github.com/golang/mock/gomock"
"github.com/netbirdio/netbird/shared/management/status"
"github.com/prometheus/client_golang/prometheus/push"
log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/otel/metric/noop"
"golang.zx2c4.com/wireguard/wgctrl/wgtypes"

"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
"github.com/netbirdio/netbird/shared/management/status"

nbdns "github.com/netbirdio/netbird/dns"
"github.com/netbirdio/netbird/management/internals/controllers/network_map"
"github.com/netbirdio/netbird/management/internals/controllers/network_map/controller"
Expand Down Expand Up @@ -1815,6 +1817,13 @@ func TestAccount_Copy(t *testing.T) {
Targets: []*service.Target{},
},
},
Domains: []*domain.Domain{
{
ID: "domain1",
Domain: "test.com",
AccountID: "account1",
},
},
NetworkMapCache: &types.NetworkMapBuilder{},
}
account.InitOnce()
Expand Down
5 changes: 5 additions & 0 deletions management/server/store/sql_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,11 @@ func (s *SqlStore) DeleteAccount(ctx context.Context, account *types.Account) er
return result.Error
}

result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id)
if result.Error != nil {
return result.Error
}

result = tx.Select(clause.Associations).Delete(account)
Comment on lines +399 to 404
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Account model associations =="
rg -n 'Services\s+\[\]\*service\.Service|Domains\s+\[\]\*proxydomain\.Domain' management/server/types/account.go

echo
echo "== Domain model delete constraint check =="
rg -n 'type Domain struct|AccountID|constraint|OnDelete' management/internals/modules/reverseproxy/domain/domain.go -A8 -B2

echo
echo "== DeleteAccount cleanup steps =="
rg -n 'func \(s \*SqlStore\) DeleteAccount|Delete\(account\.Policies|Delete\(account\.UsersG|Delete\(account\.Services|Delete\(account\.Domains|Delete\(account\)' management/server/store/sql_store.go -A30 -B5

Repository: netbirdio/netbird

Length of output: 4423

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Full DeleteAccount function context =="
sed -n '385,419p' management/server/store/sql_store.go

echo
echo "== Checking for any domain cleanup elsewhere =="
rg -n 'Domains.*Delete|DeleteDomain|domain.*orphan|cascade.*domain' management/server/store/sql_store.go

echo
echo "== Checking Account model for Domains association details =="
sed -n '100,110p' management/server/types/account.go

echo
echo "== Checking if there are transaction hooks or other cleanup paths =="
rg -n 'func.*Delete|OnDelete|cascade' management/internals/modules/reverseproxy/domain/domain.go

Repository: netbirdio/netbird

Length of output: 1117

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Checking cascade delete behavior in other delete operations =="
rg -n 'Select\(clause\.Associations\)\.Delete' management/server/store/sql_store.go -B2 -A2

echo
echo "== Looking for account deletion tests =="
fd -t f '.*test.*\.go$' | xargs rg -l 'DeleteAccount' | head -5

echo
echo "== Checking if Domains have any cascade constraints defined =="
rg -n 'type Domain struct' management/internals/modules/reverseproxy/domain/domain.go -A30 | head -40

echo
echo "== Checking recent changes or migrations related to domains =="
fd -t f '*.go' management/server/store | xargs rg -l 'Domain' | grep -i migrat

Repository: netbirdio/netbird

Length of output: 2975

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== DeleteAccount test cases =="
rg -n 'DeleteAccount|func Test.*Delete' management/server/store/sql_store_test.go -A20 | head -80

echo
echo "== Domain-related assertions in tests =="
rg -n 'Domain.*assert|assert.*Domain|orphan' management/server/store/sql_store_test.go

echo
echo "== Check if Domains cleanup is tested =="
rg -n 'Domains|proxydomain' management/server/store/sql_store_test.go | head -10

Repository: netbirdio/netbird

Length of output: 4631

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check if there's a test for domain orphaning (searching for domain-related test) =="
rg -n 'Domain|proxydomain' management/server/store/sql_store_test.go -B2 -A5 | grep -A5 -B5 'Delete'

echo
echo "== Looking for any domain cleanup or cascading logic =="
rg -n 'type Account struct' management/server/types/account.go -A50 | grep -E 'Domains|Services'

echo
echo "== Checking if there are database migrations or constraints for domains =="
fd -t f 'migrat' management/server/store | xargs cat 2>/dev/null | grep -i 'domain\|account_id' | head -20

Repository: netbirdio/netbird

Length of output: 1384

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Finding migration/schema files =="
find . -type f \( -name '*migrat*' -o -name '*.sql' \) | grep -v node_modules | grep -v '.git' | head -20

echo
echo "== Checking if GORM cascade delete works with the foreignKey pattern =="
rg -n 'onDelete|ON DELETE|cascade' management/server/types/ management/internals/modules/reverseproxy/domain/

echo
echo "== Looking for when Services deletion was added (search for Services delete pattern) =="
rg -n 'Delete\(account\.Services' management/server/store/sql_store.go -B10 -A5

Repository: netbirdio/netbird

Length of output: 1205

🏁 Script executed:

#!/bin/bash
set -euco pipefail

echo "== Checking migration.go for domain table definition =="
cat management/server/store/migration.go | grep -A20 'type domains\|Domain table' || echo "Searching differently..."

echo
echo "== Searching for Domain table creation in migrations =="
rg -n 'CreateTable.*domain\|domains.*table' management/server/store/ -i

echo
echo "== Checking GORM migration syntax for cascade constraints =="
rg -n 'ForeignKey.*onDelete|OnDelete\|constraint' management/server/store/migration.go -B2 -A2

echo
echo "== Looking at how Services table is created vs Domains =="
rg -n 'CreateTable.*service\|CreateTable.*domain' management/server/store/migration.go -i -A10 | head -50

Repository: netbirdio/netbird

Length of output: 677

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Checking migration_up.go =="
ls -la management/cmd/migration_up.go && wc -l management/cmd/migration_up.go

echo
echo "== Searching for Domain table AutoMigrate in store code =="
rg -n 'AutoMigrate.*Domain\|Domain.*AutoMigrate' management/server/store/ -B2 -A2

echo
echo "== Looking for main store initialization =="
rg -n 'func.*New.*SqlStore\|AutoMigrate' management/server/store/sql_store.go -A30 | head -60

echo
echo "== Finding where models are migrated =="
fd -t f 'sql_store.go' management/server/store/ | xargs rg -n 'AutoMigrate' -B5 -A10

Repository: netbirdio/netbird

Length of output: 265

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Checking how db is initialized and tables created =="
rg -n 'func.*Open.*\|gorm\.Open\|AutoMigrate\|CreateTable' management/server/store/sqlite.go -B2 -A8 | head -80

echo
echo "== Looking for all GORM model tags for Account associations =="
rg -n 'Services.*gorm\|Domains.*gorm' management/server/types/account.go -B1 -A1

echo
echo "== Checking if there are onDelete tags on the Domain or Service models =="
rg -n 'onDelete\|OnDelete' management/server/types/ management/internals/modules/reverseproxy/ -i

Repository: netbirdio/netbird

Length of output: 207

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== List files in store directory =="
ls -la management/server/store/ | grep -E '\.go$|^total'

echo
echo "== Find where DB tables are created/migrated =="
rg -n 'gorm\.DB\|Migrator\|CreateTable' management/server/store/*.go | head -30

echo
echo "== Check if GORM's Select(clause.Associations) cascades on Account.Domains =="
rg -n 'Select\(clause\.Associations\)\.Delete' management/server/store/*.go -B5 -A5

Repository: netbirdio/netbird

Length of output: 867

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Looking at sql_store.go initialization =="
head -100 management/server/store/sql_store.go

echo
echo "== Searching for how DB handles cascade deletes in current code =="
rg -n 'Select\(clause\.Associations\)|clause\.Associations' management/server/store/sql_store.go

echo
echo "== Check the Account model associations more carefully =="
sed -n '95,115p' management/server/types/account.go

Repository: netbirdio/netbird

Length of output: 6699

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check constraint tags on Account associations =="
rg -n 'gorm:' management/server/types/account.go | grep -E 'Services|Domains|Policies|Users|Onboarding'

echo
echo "== Check if Domain cleanup is actually tested =="
rg -n 'GetDomain\|proxydomain\|Domains' management/server/store/sql_store_test.go | grep -i 'delete\|account'

echo
echo "== Verify if domains would be orphaned after account deletion =="
rg -n 'Domains\|domain.*delete\|orphan' management/server/store/sql_store_test.go

Repository: netbirdio/netbird

Length of output: 792

Add explicit Domain deletion in DeleteAccount transaction.

Lines 399-404 clean up Services, Policies, and UsersG before deleting the account, but Domains are missing. Without the OnDelete:CASCADE constraint (unlike Onboarding), Domain rows will be orphaned when the account is deleted.

Proposed fix 
		result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id)
		if result.Error != nil {
			return result.Error
		}

+		result = tx.Select(clause.Associations).Delete(account.Domains, "account_id = ?", account.Id)
+		if result.Error != nil {
+			return result.Error
+		}
+
		result = tx.Select(clause.Associations).Delete(account)
		if result.Error != nil {
			return result.Error
		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id)
if result.Error != nil {
return result.Error
}
result = tx.Select(clause.Associations).Delete(account)
result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id)
if result.Error != nil {
return result.Error
}
result = tx.Select(clause.Associations).Delete(account.Domains, "account_id = ?", account.Id)
if result.Error != nil {
return result.Error
}
result = tx.Select(clause.Associations).Delete(account)
if result.Error != nil {
return result.Error
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@management/server/store/sql_store.go` around lines 399 - 404, The transaction
that removes an Account (in DeleteAccount) currently deletes Services, Policies,
and UsersG but omits Domains, leaving orphaned Domain rows; add an explicit
deletion of account.Domains via the same
tx.Select(clause.Associations).Delete(...) pattern (filtering by account.Id or
account_id) before the final tx.Delete(account) so Domains are removed in the
same transaction; update the DeleteAccount transaction to call the deletion for
account.Domains (similar to how account.Services is deleted) and check/return
result.Error as done for the other deletes.

if result.Error != nil {
return result.Error
Expand Down
45 changes: 45 additions & 0 deletions management/server/store/sql_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"github.com/stretchr/testify/require"

nbdns "github.com/netbirdio/netbird/dns"
proxydomain "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
"github.com/netbirdio/netbird/management/internals/modules/zones"
"github.com/netbirdio/netbird/management/internals/modules/zones/records"
resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types"
Expand Down Expand Up @@ -350,6 +352,35 @@ func TestSqlite_DeleteAccount(t *testing.T) {
},
}

account.Services = []*rpservice.Service{
{
ID: "service_id",
AccountID: account.Id,
Name: "test service",
Domain: "svc.example.com",
Enabled: true,
Targets: []*rpservice.Target{
{
AccountID: account.Id,
ServiceID: "service_id",
Host: "localhost",
Port: 8080,
Protocol: "http",
Enabled: true,
},
},
},
}

account.Domains = []*proxydomain.Domain{
{
ID: "domain_id",
Domain: "custom.example.com",
AccountID: account.Id,
Validated: true,
},
}

err = store.SaveAccount(context.Background(), account)
require.NoError(t, err)

Expand Down Expand Up @@ -411,6 +442,20 @@ func TestSqlite_DeleteAccount(t *testing.T) {
require.NoError(t, err, "expecting no error after removing DeleteAccount when searching for network resources")
require.Len(t, resources, 0, "expecting no network resources to be found after DeleteAccount")
}

domains, err := store.ListCustomDomains(context.Background(), account.Id)
require.NoError(t, err, "expecting no error after DeleteAccount when searching for custom domains")
require.Len(t, domains, 0, "expecting no custom domains to be found after DeleteAccount")

var services []*rpservice.Service
err = store.(*SqlStore).db.Model(&rpservice.Service{}).Find(&services, "account_id = ?", account.Id).Error
require.NoError(t, err, "expecting no error after DeleteAccount when searching for services")
require.Len(t, services, 0, "expecting no services to be found after DeleteAccount")

var targets []*rpservice.Target
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")
}

func Test_GetAccount(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions management/server/store/sqlstore_bench_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/stretchr/testify/assert"

nbdns "github.com/netbirdio/netbird/dns"
"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types"
routerTypes "github.com/netbirdio/netbird/management/server/networks/routers/types"
Expand Down Expand Up @@ -265,6 +266,7 @@ func setupBenchmarkDB(b testing.TB) (*SqlStore, func(), string) {
&nbdns.NameServerGroup{}, &posture.Checks{}, &networkTypes.Network{},
&routerTypes.NetworkRouter{}, &resourceTypes.NetworkResource{},
&types.AccountOnboarding{}, &service.Service{}, &service.Target{},
&domain.Domain{},
}

for i := len(models) - 1; i >= 0; i-- {
Expand Down
8 changes: 8 additions & 0 deletions management/server/types/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (

"github.com/netbirdio/netbird/client/ssh/auth"
nbdns "github.com/netbirdio/netbird/dns"
proxydomain "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain"
"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
"github.com/netbirdio/netbird/management/internals/modules/zones"
"github.com/netbirdio/netbird/management/internals/modules/zones/records"
Expand Down Expand Up @@ -101,6 +102,7 @@ type Account struct {
DNSSettings DNSSettings `gorm:"embedded;embeddedPrefix:dns_settings_"`
PostureChecks []*posture.Checks `gorm:"foreignKey:AccountID;references:id"`
Services []*service.Service `gorm:"foreignKey:AccountID;references:id"`
Domains []*proxydomain.Domain `gorm:"foreignKey:AccountID;references:id"`
// Settings is a dictionary of Account settings
Settings *Settings `gorm:"embedded;embeddedPrefix:settings_"`
Networks []*networkTypes.Network `gorm:"foreignKey:AccountID;references:id"`
Expand Down Expand Up @@ -911,6 +913,11 @@ func (a *Account) Copy() *Account {
services = append(services, svc.Copy())
}

domains := []*proxydomain.Domain{}
for _, domain := range a.Domains {
domains = append(domains, domain.Copy())
}

return &Account{
Id: a.Id,
CreatedBy: a.CreatedBy,
Expand All @@ -936,6 +943,7 @@ func (a *Account) Copy() *Account {
Onboarding: a.Onboarding,
NetworkMapCache: a.NetworkMapCache,
nmapInitOnce: a.nmapInitOnce,
Domains: domains,
}
}

Expand Down
Loading