From 979571a99ff03a6bf1aea587c1509bbcec3f3d54 Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Mon, 28 Sep 2026 14:14:32 +0200 Subject: [PATCH] [management] Prevent deleting custom domains used by services (#7515) Deleting a custom domain released its name while services still pointed at it, leaving them on a namespace the account no longer held. Deletion now refuses with 412 when a service in the same account uses the domain or a subdomain, including disabled ones. Service writes revalidate authorization inside their transaction and hold a shared lock on the matching registrations, so a delete racing a create cannot strand either. The dependency lookup is account-scoped: registrations are unique by name, so another account can hold team.example.com under example.com and its services are authorized by its own registration. --- docs/custom-domain-validation.md | 5 + .../domain/manager/deletion_test.go | 80 ++++++++ .../reverseproxy/domain/manager/manager.go | 20 ++ .../service/manager/domain_validation_test.go | 50 +++++ .../reverseproxy/service/manager/manager.go | 17 ++ .../service/manager/manager_test.go | 4 + .../server/store/sql_store_custom_domain.go | 48 ++++- .../store/sql_store_custom_domain_test.go | 182 ++++++++++++++++++ .../store/sql_store_domain_expiration.go | 5 +- management/server/store/store.go | 2 + management/server/store/store_mock.go | 16 ++ shared/management/http/api/openapi.yml | 5 +- 12 files changed, 424 insertions(+), 10 deletions(-) create mode 100644 management/internals/modules/reverseproxy/domain/manager/deletion_test.go create mode 100644 management/server/store/sql_store_custom_domain_test.go diff --git a/docs/custom-domain-validation.md b/docs/custom-domain-validation.md index a70388e4d..4b0f95ae3 100644 --- a/docs/custom-domain-validation.md +++ b/docs/custom-domain-validation.md @@ -26,3 +26,8 @@ window. Restarting management does not extend a previously assigned deadline. Registrations with existing services, including services using subdomains, are retained for operator review. Management logs their account and domain IDs so an operator can identify and resolve those dependencies before cleanup. + +Manual deletion is also refused while any service uses the domain or a subdomain, +including disabled services. Delete those services or move them to another domain +before removing the registration. A refused deletion returns HTTP 412 and leaves +the domain and its services unchanged; no deletion activity event is recorded. diff --git a/management/internals/modules/reverseproxy/domain/manager/deletion_test.go b/management/internals/modules/reverseproxy/domain/manager/deletion_test.go new file mode 100644 index 000000000..b47a2bca0 --- /dev/null +++ b/management/internals/modules/reverseproxy/domain/manager/deletion_test.go @@ -0,0 +1,80 @@ +package manager + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" + "github.com/netbirdio/netbird/management/server/activity" + nbcontext "github.com/netbirdio/netbird/management/server/context" + nbstore "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/shared/auth" +) + +func TestDeleteDomain_ServiceDependencies(t *testing.T) { + for _, tt := range []struct { + name string + domainName string + serviceHost string + accountID string + enabled bool + protected bool + }{ + {"exact", "example.com", "example.com", accountA, true, true}, + {"subdomain", "example.com", "deep.app.example.com", accountA, true, true}, + {"disabled", "example.com", "app.example.com", accountA, false, true}, + // A service is authorized by its own account's registration, so another + // account's service under this namespace is not a dependency of it. + {"other account", "example.com", "app.example.com", accountB, true, false}, + {"case and trailing dot", "example.com", "APP.EXAMPLE.COM.", accountA, true, true}, + {"suffix boundary", "example.com", "notexample.com", accountA, true, false}, + {"literal underscore", "a_b.example.com", "app.a_b.example.com", accountA, true, true}, + {"underscore wildcard", "a_b.example.com", "app.axb.example.com", accountA, true, false}, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + env := setupDomainTest(t) + events := captureDomainEvents(env) + d, err := env.store.CreateCustomDomain(ctx, accountA, tt.domainName, testCluster, true) + require.NoError(t, err) + svc := &rpservice.Service{ + ID: "dependent", AccountID: tt.accountID, Domain: tt.serviceHost, + Enabled: tt.enabled, ProxyCluster: testCluster, + } + require.NoError(t, env.store.CreateService(ctx, svc)) + router := mux.NewRouter() + RegisterEndpoints(router, env.manager) + deleteDomain := func() *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodDelete, "/domains/"+d.ID, nil) + req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{AccountId: accountA, UserId: accountAUser}) + response := httptest.NewRecorder() + router.ServeHTTP(response, req) + return response + } + + response := deleteDomain() + if tt.protected { + require.Equal(t, http.StatusPreconditionFailed, response.Code, "dependent services must block deletion: %s", response.Body.String()) + assert.NotContains(t, response.Body.String(), tt.accountID, "the error must not reveal the service's account") + assert.NotNil(t, storedDomain(t, env.store, accountA, d.Domain), "the namespace must remain reserved") + assert.Empty(t, events.get(), "rejected deletion must not emit DomainDeleted") + stored, err := env.store.GetServiceByID(ctx, nbstore.LockingStrengthNone, tt.accountID, svc.ID) + require.NoError(t, err) + assert.Equal(t, svc.Enabled, stored.Enabled, "rejected deletion must preserve the service") + require.NoError(t, env.store.DeleteService(ctx, tt.accountID, svc.ID)) + response = deleteDomain() + } + require.Equal(t, http.StatusNoContent, response.Code, "deletion must succeed without dependencies: %s", response.Body.String()) + assert.Nil(t, storedDomain(t, env.store, accountA, d.Domain), "the registration must be deleted") + captured := events.get() + require.Len(t, captured, 1, "only successful deletion may emit an event") + assert.Equal(t, activity.DomainDeleted, captured[0].Activity, "the event must describe the successful deletion") + }) + } +} diff --git a/management/internals/modules/reverseproxy/domain/manager/manager.go b/management/internals/modules/reverseproxy/domain/manager/manager.go index c0fb12e9c..1e9ecab43 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager.go @@ -357,6 +357,26 @@ func (m Manager) DeriveClusterFromDomain(ctx context.Context, accountID, domain return "", fmt.Errorf("domain %s does not match any available proxy cluster", domain) } +// ValidateServiceDomain holds custom domain authorization through a service write transaction. +func (m Manager) ValidateServiceDomain(ctx context.Context, tx nbstore.Store, accountID, serviceDomain, cluster string) error { + if _, ok := ExtractClusterFromFreeDomain(serviceDomain, []string{cluster}); ok { + return nil + } + name, err := nbdomain.FromString(serviceDomain) + if err != nil { + return status.Errorf(status.InvalidArgument, "invalid service domain: %v", err) + } + customDomains, err := tx.LockCustomDomains(ctx, accountID, name) + if err != nil { + return err + } + target, match := extractClusterFromCustomDomains(serviceDomain, customDomains) + if match != customDomainValidated || target != cluster { + return status.Errorf(status.PreconditionFailed, "custom domain authorization changed; retry the service operation") + } + return nil +} + func (m Manager) getClusterAllowList(ctx context.Context, accountID string) ([]string, error) { byopAddresses, err := m.proxyManager.GetActiveClusterAddressesForAccount(ctx, accountID) if err != nil { diff --git a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go index 6f641b73b..f491d01c3 100644 --- a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go +++ b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go @@ -125,3 +125,53 @@ func TestUpdateService_RefusesMoveToUnvalidatedDomain(t *testing.T) { require.NoError(t, err) assert.Equal(t, "app.proven.example.com", stored.Domain, "the service must keep its original domain") } + +func TestCreateService_DomainDeletedBeforeWrite(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + + d, err := testStore.CreateCustomDomain(ctx, testAccountID, "proven.example.com", validationTestCluster, true) + require.NoError(t, err) + svc := newTestService("app.proven.example.com") + require.NoError(t, mgr.initializeServiceForCreate(ctx, testAccountID, svc)) + + // Delete after the initial authorization check, before the service transaction starts. + require.NoError(t, testStore.DeleteCustomDomain(ctx, testAccountID, d.ID)) + err = mgr.persistNewService(ctx, testAccountID, svc) + require.Error(t, err, "an earlier validation result must not authorize a deleted registration") + sErr, ok := status.FromError(err) + require.True(t, ok, "the caller must receive a typed precondition error") + assert.Equal(t, status.PreconditionFailed, sErr.Type(), "the service must require current domain authorization") + services, err := testStore.GetAccountServices(ctx, store.LockingStrengthNone, testAccountID) + require.NoError(t, err) + assert.Empty(t, services, "the failed write must not leave a service") +} + +func TestUpdateService_DomainDeletedBeforeWrite(t *testing.T) { + ctx := context.Background() + mgr, testStore := setupIntegrationTest(t) + withRealDomainManager(t, mgr, testStore) + _, err := testStore.CreateCustomDomain(ctx, testAccountID, "original.example.com", validationTestCluster, true) + require.NoError(t, err) + d, err := testStore.CreateCustomDomain(ctx, testAccountID, "destination.example.com", validationTestCluster, true) + require.NoError(t, err) + svc, err := mgr.CreateService(ctx, testAccountID, testUserID, newTestService("app.original.example.com")) + require.NoError(t, err) + moved := svc.Copy() + moved.Domain = "app.destination.example.com" + cluster, err := mgr.resolveEffectiveCluster(ctx, testAccountID, moved) + require.NoError(t, err) + + require.NoError(t, testStore.DeleteCustomDomain(ctx, testAccountID, d.ID)) + err = testStore.ExecuteInTransaction(ctx, func(tx store.Store) error { + return mgr.executeServiceUpdate(ctx, tx, testAccountID, moved, &serviceUpdateInfo{}, nil, cluster) + }) + require.Error(t, err, "a domain deleted after cluster resolution must reject the update") + sErr, ok := status.FromError(err) + require.True(t, ok, "the caller must receive a typed precondition error") + assert.Equal(t, status.PreconditionFailed, sErr.Type(), "the move must require current domain authorization") + stored, err := testStore.GetServiceByID(ctx, store.LockingStrengthNone, testAccountID, svc.ID) + require.NoError(t, err) + assert.Equal(t, svc.Domain, stored.Domain, "the service must retain its authorized domain") +} diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index 9c7f95eb4..62897c9ae 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -74,6 +74,7 @@ const unknownHostPlaceholder = "unknown" // ClusterDeriver derives the proxy cluster from a domain. type ClusterDeriver interface { DeriveClusterFromDomain(ctx context.Context, accountID, domain string) (string, error) + ValidateServiceDomain(ctx context.Context, tx store.Store, accountID, domain, cluster string) error GetClusterDomains() []string } @@ -332,6 +333,9 @@ func (m *Manager) persistNewService(ctx context.Context, accountID string, svc * } return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + if err := m.validateServiceDomain(ctx, transaction, accountID, svc, svc.ProxyCluster); err != nil { + return err + } if svc.Domain != "" { if err := m.checkDomainAvailable(ctx, transaction, svc.Domain, ""); err != nil { return err @@ -461,6 +465,9 @@ func (m *Manager) persistNewEphemeralService(ctx context.Context, accountID, pee } return m.store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + if err := m.validateServiceDomain(ctx, transaction, accountID, svc, svc.ProxyCluster); err != nil { + return err + } if err := m.validateEphemeralPreconditions(ctx, transaction, accountID, peerID, svc); err != nil { return err } @@ -622,6 +629,9 @@ func (m *Manager) resolveEffectiveCluster(ctx context.Context, accountID string, } func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.Store, accountID string, service *service.Service, updateInfo *serviceUpdateInfo, customPorts *bool, effectiveCluster string) error { + if err := m.validateServiceDomain(ctx, transaction, accountID, service, effectiveCluster); err != nil { + return err + } existingService, err := transaction.GetServiceByID(ctx, store.LockingStrengthUpdate, accountID, service.ID) if err != nil { return err @@ -677,6 +687,13 @@ func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.St return nil } +func (m *Manager) validateServiceDomain(ctx context.Context, tx store.Store, accountID string, svc *service.Service, cluster string) error { + if m.clusterDeriver == nil { + return nil + } + return m.clusterDeriver.ValidateServiceDomain(ctx, tx, accountID, svc.Domain, cluster) +} + // validateL4PortDiffOnClusterDiff checks if custom L4 ports are configured and validates port changes across clusters. // It ensures no port changes if custom ports are unsupported for a given cluster and protocol mode. // Returns an error if validation fails, otherwise returns nil. diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index dd0edec60..9704796be 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -655,6 +655,10 @@ func (d *testClusterDeriver) GetClusterDomains() []string { return d.domains } +func (d *testClusterDeriver) ValidateServiceDomain(context.Context, store.Store, string, string, string) error { + return nil +} + const ( testAccountID = "test-account" testPeerID = "test-peer-1" diff --git a/management/server/store/sql_store_custom_domain.go b/management/server/store/sql_store_custom_domain.go index 9eaaf2b10..5b7cbf519 100644 --- a/management/server/store/sql_store_custom_domain.go +++ b/management/server/store/sql_store_custom_domain.go @@ -4,13 +4,16 @@ import ( "context" "errors" "fmt" + "strings" "time" "github.com/rs/xid" log "github.com/sirupsen/logrus" "gorm.io/gorm" + "gorm.io/gorm/clause" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" + nbdomain "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/shared/management/status" ) @@ -131,16 +134,45 @@ func (s *SqlStore) UpdateCustomDomain(ctx context.Context, accountID string, d * return d, nil } -func (s *SqlStore) DeleteCustomDomain(ctx context.Context, accountID string, domainID string) error { - result := s.db.Delete(domain.Domain{}, accountAndIDQueryCondition, accountID, domainID) - if result.Error != nil { - log.WithContext(ctx).Errorf("failed to delete reverse proxy custom domain from store: %v", result.Error) - return status.Errorf(status.Internal, "failed to delete reverse proxy custom domain from store") +// LockCustomDomains holds shared locks on registrations covering a service until commit. +func (s *SqlStore) LockCustomDomains(ctx context.Context, accountID string, serviceDomain nbdomain.Domain) ([]*domain.Domain, error) { + var names []string + for name := serviceDomain.PunycodeString(); name != ""; { + names = append(names, name) + _, name, _ = strings.Cut(name, ".") } - if result.RowsAffected == 0 { - return status.Errorf(status.NotFound, "reverse proxy custom domain %s not found", domainID) + var domains []*domain.Domain + if err := s.db.WithContext(ctx).Clauses(clause.Locking{Strength: string(LockingStrengthShare)}). + Where(accountIDCondition, accountID).Where("domain IN ?", names). + Order("id").Find(&domains).Error; err != nil { + return nil, fmt.Errorf("lock custom domains: %w", err) } + return domains, nil +} - return nil +// DeleteCustomDomain removes a registration only when no service uses its namespace. +func (s *SqlStore) DeleteCustomDomain(ctx context.Context, accountID string, domainID string) error { + return s.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + var d domain.Domain + // Service writes hold a shared lock on this row through commit, so neither + // operation can proceed against the other's outdated view of the domain. + if err := tx.Clauses(clause.Locking{Strength: string(LockingStrengthUpdate)}). + Take(&d, accountAndIDQueryCondition, accountID, domainID).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return status.Errorf(status.NotFound, "custom domain not found") + } + return fmt.Errorf("lock custom domain for deletion: %w", err) + } + + result := tx.Where(accountAndIDQueryCondition, accountID, domainID). + Where("NOT EXISTS (?)", customDomainServices(tx, &d).Select("1")).Delete(&domain.Domain{}) + if result.Error != nil { + return fmt.Errorf("delete custom domain: %w", result.Error) + } + if result.RowsAffected == 0 { + return status.Errorf(status.PreconditionFailed, "custom domain has dependent services; delete or move them before deleting the domain") + } + return nil + }) } diff --git a/management/server/store/sql_store_custom_domain_test.go b/management/server/store/sql_store_custom_domain_test.go new file mode 100644 index 000000000..a274c7271 --- /dev/null +++ b/management/server/store/sql_store_custom_domain_test.go @@ -0,0 +1,182 @@ +package store + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" + "github.com/netbirdio/netbird/management/server/types" + nbdomain "github.com/netbirdio/netbird/shared/management/domain" + "github.com/netbirdio/netbird/shared/management/status" +) + +func TestLockCustomDomains_ConcurrentServices(t *testing.T) { + runTestForAllEngines(t, "", func(t *testing.T, store Store) { + if store.GetStoreEngine() == types.SqliteStoreEngine { + t.Skip("SQLite serializes transactions on one connection") + } + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", ""))) + _, err := store.CreateCustomDomain(ctx, "owner", "one.example.com", "cluster", true) + require.NoError(t, err) + _, err = store.CreateCustomDomain(ctx, "owner", "two.example.com", "cluster", true) + require.NoError(t, err) + + locked := make(chan error, 1) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- store.ExecuteInTransaction(ctx, func(tx Store) error { + _, err := tx.LockCustomDomains(ctx, "owner", "app.one.example.com") + locked <- err + if err != nil { + return err + } + select { + case <-release: + return nil + case <-ctx.Done(): + return ctx.Err() + } + }) + }() + var lockErr error + select { + case lockErr = <-locked: + case err := <-done: + t.Fatalf("transaction ended before locking: %v", err) + } + writeCtx, writeCancel := context.WithTimeout(ctx, 3*time.Second) + defer writeCancel() + var writeErr error + for _, name := range []nbdomain.Domain{"app.one.example.com", "app.two.example.com"} { + writeErr = store.ExecuteInTransaction(writeCtx, func(tx Store) error { + if _, err := tx.LockCustomDomains(writeCtx, "owner", name); err != nil { + return err + } + return tx.CreateService(writeCtx, &rpservice.Service{ + ID: name.PunycodeString(), AccountID: "owner", Domain: name.PunycodeString(), + }) + }) + if writeErr != nil { + break + } + } + close(release) + require.NoError(t, <-done) + require.NoError(t, lockErr) + require.NoError(t, writeErr, "domain authorization locks must allow concurrent service writes") + services, err := store.GetAccountServices(ctx, LockingStrengthNone, "owner") + require.NoError(t, err) + assert.Len(t, services, 2, "both services must commit while the first domain is locked") + }) +} + +func TestDeleteCustomDomain_ServiceDependencies(t *testing.T) { + runTestForAllEngines(t, "", func(t *testing.T, store Store) { + ctx := context.Background() + require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", ""))) + d, err := store.CreateCustomDomain(ctx, "owner", "example.com", "cluster", true) + require.NoError(t, err) + svc := &rpservice.Service{ID: "service", AccountID: "owner", Domain: "APP.EXAMPLE.COM."} + require.NoError(t, store.CreateService(ctx, svc)) + + err = store.DeleteCustomDomain(ctx, "other", d.ID) + require.Error(t, err) + sErr, ok := status.FromError(err) + require.True(t, ok, "cross-account deletion must return a typed error") + assert.Equal(t, status.NotFound, sErr.Type(), "cross-account deletion must not reveal dependencies") + + err = store.DeleteCustomDomain(ctx, "owner", d.ID) + require.Error(t, err) + sErr, ok = status.FromError(err) + require.True(t, ok, "dependent services must return a typed error") + assert.Equal(t, status.PreconditionFailed, sErr.Type(), "deletion must fail until services are removed") + stored, err := store.GetCustomDomain(ctx, "owner", d.ID) + require.NoError(t, err) + assert.True(t, stored.Validated, "rejected deletion must preserve validation") + + require.NoError(t, store.DeleteService(ctx, "owner", svc.ID)) + require.NoError(t, store.DeleteCustomDomain(ctx, "owner", d.ID)) + _, err = store.GetCustomDomain(ctx, "owner", d.ID) + require.Error(t, err, "the registration must be gone after successful deletion") + }) +} + +func TestDeleteCustomDomain_OtherAccountSubdomainIsNotADependency(t *testing.T) { + runTestForAllEngines(t, "", func(t *testing.T, store Store) { + ctx := context.Background() + require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", ""))) + require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "other", "admin", ""))) + + // Registrations are unique by name, so a second account can hold a + // subdomain of the first account's registration and serve from it. + parent, err := store.CreateCustomDomain(ctx, "owner", "example.com", "cluster", true) + require.NoError(t, err) + _, err = store.CreateCustomDomain(ctx, "other", "team.example.com", "cluster", true) + require.NoError(t, err) + require.NoError(t, store.CreateService(ctx, &rpservice.Service{ + ID: "service", AccountID: "other", Domain: "app.team.example.com", + })) + + require.NoError(t, store.DeleteCustomDomain(ctx, "owner", parent.ID), + "another account's service must not hold the registration open") + }) +} + +func TestDeleteCustomDomain_ConcurrentServiceCreation(t *testing.T) { + runTestForAllEngines(t, "", func(t *testing.T, store Store) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + require.NoError(t, store.SaveAccount(ctx, newAccountWithId(ctx, "owner", "admin", ""))) + for i := range 10 { + d, err := store.CreateCustomDomain(ctx, "owner", fmt.Sprintf("app%d.example.com", i), "cluster", true) + require.NoError(t, err) + svc := &rpservice.Service{ID: fmt.Sprintf("service-%d", i), AccountID: "owner", Domain: "nested." + d.Domain} + start := make(chan struct{}) + created := make(chan error, 1) + deleted := make(chan error, 1) + go func() { + <-start + created <- store.ExecuteInTransaction(ctx, func(tx Store) error { + domains, err := tx.LockCustomDomains(ctx, "owner", nbdomain.Domain(svc.Domain)) + if err != nil { + return err + } + for _, candidate := range domains { + if candidate.ID == d.ID && candidate.Validated { + return tx.CreateService(ctx, svc) + } + } + return status.Errorf(status.PreconditionFailed, "registration was deleted") + }) + }() + go func() { + <-start + deleted <- store.DeleteCustomDomain(ctx, "owner", d.ID) + }() + close(start) + createErr, deleteErr := <-created, <-deleted + require.True(t, createErr == nil || deleteErr == nil, "one operation must succeed: create=%v, delete=%v", createErr, deleteErr) + if createErr == nil { + require.Error(t, deleteErr, "a committed service must block deletion") + stored, err := store.GetCustomDomain(ctx, "owner", d.ID) + require.NoError(t, err) + assert.True(t, stored.Validated, "the service must retain its authorization") + require.NoError(t, store.DeleteService(ctx, "owner", svc.ID)) + require.NoError(t, store.DeleteCustomDomain(ctx, "owner", d.ID)) + continue + } + require.NoError(t, deleteErr) + services, err := store.GetAccountServices(ctx, LockingStrengthNone, "owner") + require.NoError(t, err) + assert.Empty(t, services, "a deleted registration must not leave a new service") + } + }) +} diff --git a/management/server/store/sql_store_domain_expiration.go b/management/server/store/sql_store_domain_expiration.go index 525c54084..e7b09f4ad 100644 --- a/management/server/store/sql_store_domain_expiration.go +++ b/management/server/store/sql_store_domain_expiration.go @@ -53,7 +53,10 @@ func customDomainServices(db *gorm.DB, d *domain.Domain) *gorm.DB { // Shared domain validation permits underscores, and older rows may contain // other LIKE metacharacters. escaped := strings.NewReplacer("!", "!!", "%", "!%", "_", "!_").Replace(name) - return db.Model(&rpservice.Service{}).Where( + // Registrations are unique by name, so another account can hold a subdomain + // of this one and serve from it. Its services derive their cluster from that + // account's own registration and are not dependents of this one. + return db.Model(&rpservice.Service{}).Where(accountIDCondition, d.AccountID).Where( "LOWER(domain) IN ? OR LOWER(domain) LIKE ? ESCAPE '!' OR LOWER(domain) LIKE ? ESCAPE '!'", []string{name, name + "."}, "%."+escaped, "%."+escaped+".", ) diff --git a/management/server/store/store.go b/management/server/store/store.go index b6368f47f..ed53690a5 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -36,6 +36,7 @@ import ( "github.com/netbirdio/netbird/management/server/telemetry" "github.com/netbirdio/netbird/management/server/testutil" "github.com/netbirdio/netbird/management/server/types" + nbdomain "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/util" "github.com/netbirdio/netbird/util/crypt" @@ -307,6 +308,7 @@ type Store interface { GetCustomDomain(ctx context.Context, accountID string, domainID string) (*domain.Domain, error) ListFreeDomains(ctx context.Context, accountID string) ([]string, error) ListCustomDomains(ctx context.Context, accountID string) ([]*domain.Domain, error) + LockCustomDomains(ctx context.Context, accountID string, serviceDomain nbdomain.Domain) ([]*domain.Domain, error) GetCustomDomainByName(ctx context.Context, domainName string) (*domain.Domain, error) CreateCustomDomain(ctx context.Context, accountID string, domainName string, targetCluster string, validated bool) (*domain.Domain, error) UpdateCustomDomain(ctx context.Context, accountID string, d *domain.Domain) (*domain.Domain, error) diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 460ba712b..2db74fb86 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -31,6 +31,7 @@ import ( posture "github.com/netbirdio/netbird/management/server/posture" types3 "github.com/netbirdio/netbird/management/server/types" route "github.com/netbirdio/netbird/route" + domain0 "github.com/netbirdio/netbird/shared/management/domain" crypt "github.com/netbirdio/netbird/util/crypt" gomock "go.uber.org/mock/gomock" ) @@ -3302,6 +3303,21 @@ func (mr *MockStoreMockRecorder) ListFreeDomains(ctx, accountID any) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListFreeDomains", reflect.TypeOf((*MockStore)(nil).ListFreeDomains), ctx, accountID) } +// LockCustomDomains mocks base method. +func (m *MockStore) LockCustomDomains(ctx context.Context, accountID string, serviceDomain domain0.Domain) ([]*domain.Domain, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LockCustomDomains", ctx, accountID, serviceDomain) + ret0, _ := ret[0].([]*domain.Domain) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LockCustomDomains indicates an expected call of LockCustomDomains. +func (mr *MockStoreMockRecorder) LockCustomDomains(ctx, accountID, serviceDomain any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LockCustomDomains", reflect.TypeOf((*MockStore)(nil).LockCustomDomains), ctx, accountID, serviceDomain) +} + // MarkAccountPrimary mocks base method. func (m *MockStore) MarkAccountPrimary(ctx context.Context, accountID string) error { m.ctrl.T.Helper() diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 5ad682c6b..c19df4fcf 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -13347,7 +13347,7 @@ paths: /api/reverse-proxies/domains/{domainId}: delete: summary: Delete a Custom domain - description: Delete an existing service custom domain + description: Delete an existing service custom domain after removing or moving all services that use it or its subdomains, including disabled services. tags: [ Services ] security: - BearerAuth: [ ] @@ -13370,6 +13370,9 @@ paths: "$ref": "#/components/responses/forbidden" '404': "$ref": "#/components/responses/not_found" + '412': + description: The domain or one of its subdomains is still used by a service + content: { } '500': "$ref": "#/components/responses/internal_error" /api/reverse-proxies/domains/{domainId}/validate: