diff --git a/internal/api/api.go b/internal/api/api.go index 581f4f3..4f12703 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -48,7 +48,10 @@ type TenantStore interface { // domain's zone name (for the generated template's name) before creating it. GetDomain(ctx context.Context, id, projectID uuid.UUID) (store.Domain, error) DeleteDomain(ctx context.Context, id, projectID uuid.UUID) error - ImportDomains(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, error) + // ImportDomains synchronises an account's zones with its domains: new + // zones become domains, domains of vanished zones are deleted. Returns + // (created, removed). + ImportDomains(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, []store.Domain, error) SetDomainTemplate(ctx context.Context, domainID, projectID uuid.UUID, templateID *uuid.UUID) (store.Domain, error) // SetDomainStatus persists the outcome of a manual check (handleCheck) so // the domain's badge reflects reality immediately, instead of staying diff --git a/internal/api/tenant_dto.go b/internal/api/tenant_dto.go index 1ec0b6c..4176d19 100644 --- a/internal/api/tenant_dto.go +++ b/internal/api/tenant_dto.go @@ -79,6 +79,14 @@ func toDomainResponse(d store.Domain) domainResponse { return resp } +// importResponse splits an import into what appeared and what vanished at +// the provider. Both slices are initialised so they marshal as [] and never +// as null — clients call .length/.map on them. +type importResponse struct { + Created []domainResponse `json:"created"` + Removed []domainResponse `json:"removed"` +} + // parseOptionalUUID parses s (may be nil/empty) into *uuid.UUID; returns ok=false on invalid input. func parseOptionalUUID(s *string) (*uuid.UUID, bool) { if s == nil || *s == "" { diff --git a/internal/api/tenant_handlers.go b/internal/api/tenant_handlers.go index 4a93a68..bc47e00 100644 --- a/internal/api/tenant_handlers.go +++ b/internal/api/tenant_handlers.go @@ -130,19 +130,26 @@ func (a *API) handleImportZones(w http.ResponseWriter, r *http.Request) { writeErr(w, http.StatusInternalServerError, "internal error") return } - // Imported atomically: either every zone becomes a domain or none does, - // so a mid-batch provider/DB error never leaves a partial import behind. - doms, err := a.Store.ImportDomains(r.Context(), pid, aid, zones) + // Imported atomically: either every zone becomes a domain (and vanished + // zones' domains are deleted) or none of it happens, so a mid-batch + // provider/DB error never leaves a partial sync behind. + doms, gone, err := a.Store.ImportDomains(r.Context(), pid, aid, zones) if err != nil { - log.Printf("api: import: create domains failed: %v", err) + log.Printf("api: import: sync domains failed: %v", err) writeErr(w, http.StatusInternalServerError, "internal error") return } - created := make([]domainResponse, 0, len(doms)) - for _, d := range doms { - created = append(created, toDomainResponse(d)) + resp := importResponse{ + Created: make([]domainResponse, 0, len(doms)), + Removed: make([]domainResponse, 0, len(gone)), } - writeJSON(w, http.StatusCreated, created) + for _, d := range doms { + resp.Created = append(resp.Created, toDomainResponse(d)) + } + for _, d := range gone { + resp.Removed = append(resp.Removed, toDomainResponse(d)) + } + writeJSON(w, http.StatusCreated, resp) } // --- templates --- diff --git a/internal/api/tenant_test.go b/internal/api/tenant_test.go index 1db85a6..baedfc6 100644 --- a/internal/api/tenant_test.go +++ b/internal/api/tenant_test.go @@ -38,8 +38,13 @@ type mockTenantStore struct { createDomains int importDomains []store.Domain + importRemoved []store.Domain importDomainsErr error importCalled bool + // importDomainsFn, when set, overrides the default fake behavior of + // ImportDomains — used by tests that need to control created/removed + // independently (e.g. TestImportZonesReturnsCreatedAndRemoved). + importDomainsFn func(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, []store.Domain, error) setDomainTemplateErr error @@ -186,10 +191,16 @@ func (m *mockTenantStore) DeleteCustomRecord(_ context.Context, domainID, projec return m.deleteCustomRecordErr } -func (m *mockTenantStore) ImportDomains(_ context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, error) { +func (m *mockTenantStore) ImportDomains(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, []store.Domain, error) { m.importCalled = true + if m.importDomainsFn != nil { + created, removed, err := m.importDomainsFn(ctx, projectID, accountID, zones) + m.importDomains, m.importRemoved = created, removed + m.domains = append(m.domains, created...) + return created, removed, err + } if m.importDomainsErr != nil { - return nil, m.importDomainsErr + return nil, nil, m.importDomainsErr } out := make([]store.Domain, 0, len(zones)) for _, z := range zones { @@ -198,7 +209,7 @@ func (m *mockTenantStore) ImportDomains(_ context.Context, projectID, accountID } m.domains = append(m.domains, out...) m.importDomains = out - return out, nil + return out, nil, nil } type mockCipher struct{} @@ -431,12 +442,15 @@ func TestImportZones_CreatesDomainPerZone(t *testing.T) { if len(ts.importDomains) != 2 { t.Fatalf("expected 2 domains created via ImportDomains, got %d", len(ts.importDomains)) } - var resp []domainResponse + var resp importResponse if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { t.Fatal(err) } - if len(resp) != 2 { - t.Fatalf("expected 2 domains in response, got %d", len(resp)) + if len(resp.Created) != 2 { + t.Fatalf("expected 2 domains in response, got %d", len(resp.Created)) + } + if len(resp.Removed) != 0 { + t.Fatalf("expected 0 removed domains in response, got %d", len(resp.Removed)) } } @@ -471,6 +485,39 @@ func TestImportZones_AtomicRollbackOnError(t *testing.T) { } } +// TestImportZonesReturnsCreatedAndRemoved covers the sync response shape: +// the endpoint now reports both what appeared (created) and what vanished +// at the provider (removed) rather than a bare array of created domains. +func TestImportZonesReturnsCreatedAndRemoved(t *testing.T) { + a, ts := newTenantTestAPI() + accID := uuid.New() + ts.accounts = []store.Account{{ID: accID, Provider: "selectel", SecretEnc: "ENC(token)"}} + ts.importDomainsFn = func(_ context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, []store.Domain, error) { + return []store.Domain{{ID: uuid.New(), ProjectID: projectID, ProviderAccountID: accountID, ZoneName: "new.example.com.", ZoneID: "z1"}}, + []store.Domain{{ID: uuid.New(), ProjectID: projectID, ProviderAccountID: accountID, ZoneName: "gone.example.com.", ZoneID: "z9"}}, + nil + } + router := NewRouter(a) + + req := requestWithSessionCookie(http.MethodPost, "/api/v1/projects/"+testPID+"/accounts/"+accID.String()+"/import", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String()) + } + var got importResponse + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatalf("response must be an object with created/removed: %v (%s)", err, w.Body.String()) + } + if len(got.Created) != 1 || got.Created[0].ZoneName != "new.example.com." { + t.Fatalf("unexpected created: %+v", got.Created) + } + if len(got.Removed) != 1 || got.Removed[0].ZoneName != "gone.example.com." { + t.Fatalf("unexpected removed: %+v", got.Removed) + } +} + func TestImportZones_BadAccountUUID(t *testing.T) { a, _ := newTenantTestAPI() router := NewRouter(a) diff --git a/internal/store/db/domains.sql.go b/internal/store/db/domains.sql.go index 25e2a07..a9afa71 100644 --- a/internal/store/db/domains.sql.go +++ b/internal/store/db/domains.sql.go @@ -75,6 +75,47 @@ func (q *Queries) DeleteDomain(ctx context.Context, arg DeleteDomainParams) erro return err } +const deleteDomainsNotInZones = `-- name: DeleteDomainsNotInZones :many +DELETE FROM domains +WHERE project_id = $1 AND provider_account_id = $2 AND zone_id <> ALL($3::text[]) +RETURNING id, project_id, provider_account_id, zone_name, zone_id, template_id, created_at, last_check_status +` + +type DeleteDomainsNotInZonesParams struct { + ProjectID uuid.UUID `json:"project_id"` + ProviderAccountID uuid.UUID `json:"provider_account_id"` + ZoneIds []string `json:"zone_ids"` +} + +func (q *Queries) DeleteDomainsNotInZones(ctx context.Context, arg DeleteDomainsNotInZonesParams) ([]Domain, error) { + rows, err := q.db.Query(ctx, deleteDomainsNotInZones, arg.ProjectID, arg.ProviderAccountID, arg.ZoneIds) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Domain + for rows.Next() { + var i Domain + if err := rows.Scan( + &i.ID, + &i.ProjectID, + &i.ProviderAccountID, + &i.ZoneName, + &i.ZoneID, + &i.TemplateID, + &i.CreatedAt, + &i.LastCheckStatus, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getDomain = `-- name: GetDomain :one SELECT id, project_id, provider_account_id, zone_name, zone_id, template_id, created_at, last_check_status FROM domains WHERE id = $1 AND project_id = $2 ` diff --git a/internal/store/queries/domains.sql b/internal/store/queries/domains.sql index ba0829d..8dc7f5c 100644 --- a/internal/store/queries/domains.sql +++ b/internal/store/queries/domains.sql @@ -37,3 +37,8 @@ UPDATE domains SET last_check_status = $2 WHERE id = $1 AND project_id = $3; -- name: CountDriftDomains :one SELECT count(*) FROM domains WHERE last_check_status = 'drift'; + +-- name: DeleteDomainsNotInZones :many +DELETE FROM domains +WHERE project_id = $1 AND provider_account_id = $2 AND zone_id <> ALL($3::text[]) +RETURNING *; diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 0955278..89fd964 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -103,13 +103,16 @@ func TestImportDomains_CommitsAllOnSuccess(t *testing.T) { {ID: "z1", Name: "a.example.com"}, {ID: "z2", Name: "b.example.com"}, } - doms, err := s.ImportDomains(ctx, defaultProject, acc.ID, zones) + doms, removed, err := s.ImportDomains(ctx, defaultProject, acc.ID, zones) if err != nil { t.Fatal(err) } if len(doms) != 2 { t.Fatalf("expected 2 domains returned, got %d", len(doms)) } + if len(removed) != 0 { + t.Fatalf("expected 0 removed domains, got %d", len(removed)) + } list, err := s.ListDomains(ctx, defaultProject) if err != nil { @@ -131,7 +134,7 @@ func TestImportDomains_RollsBackAllOnError(t *testing.T) { {ID: "z1", Name: "a.example.com"}, {ID: "z2", Name: "b.example.com"}, } - if _, err := s.ImportDomains(ctx, defaultProject, bogusAccountID, zones); err == nil { + if _, _, err := s.ImportDomains(ctx, defaultProject, bogusAccountID, zones); err == nil { t.Fatal("expected FK violation error, got nil") } @@ -161,7 +164,7 @@ func TestImportDomains_IdempotentOnRepeat(t *testing.T) { {ID: "z1", Name: "a.example.com"}, {ID: "z2", Name: "b.example.com"}, } - first, err := s.ImportDomains(ctx, defaultProject, acc.ID, zones) + first, _, err := s.ImportDomains(ctx, defaultProject, acc.ID, zones) if err != nil { t.Fatal(err) } @@ -169,13 +172,16 @@ func TestImportDomains_IdempotentOnRepeat(t *testing.T) { t.Fatalf("expected 2 domains on first import, got %d", len(first)) } - second, err := s.ImportDomains(ctx, defaultProject, acc.ID, zones) + second, removed, err := s.ImportDomains(ctx, defaultProject, acc.ID, zones) if err != nil { t.Fatalf("expected repeat import to succeed idempotently, got error: %v", err) } if len(second) != 0 { t.Fatalf("expected 0 newly-created domains on repeat import, got %d", len(second)) } + if len(removed) != 0 { + t.Fatalf("expected 0 removed domains on repeat import with the same zones, got %d", len(removed)) + } list, err := s.ListDomains(ctx, defaultProject) if err != nil { @@ -207,7 +213,7 @@ func TestSetDomainTemplate_ClosesImportCheckLoop(t *testing.T) { if err != nil { t.Fatal(err) } - doms, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{{ID: "z1", Name: "a.example.com"}}) + doms, _, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{{ID: "z1", Name: "a.example.com"}}) if err != nil { t.Fatal(err) } @@ -255,7 +261,7 @@ func TestSetDomainTemplate_RejectsForeignProjectTemplate(t *testing.T) { if err != nil { t.Fatal(err) } - doms, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{{ID: "z1", Name: "a.example.com"}}) + doms, _, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{{ID: "z1", Name: "a.example.com"}}) if err != nil { t.Fatal(err) } @@ -289,7 +295,7 @@ func seedDomain(t *testing.T, s *Store, ctx context.Context) Domain { if err != nil { t.Fatal(err) } - doms, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{{ID: "z1", Name: "a.example.com"}}) + doms, _, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{{ID: "z1", Name: "a.example.com"}}) if err != nil { t.Fatal(err) } @@ -363,3 +369,140 @@ func TestCustomRecordsDeleteAndCascade(t *testing.T) { t.Fatalf("expected cascade to remove keys with the domain, got %+v", keys) } } + +// seedAccount creates an additional provider account for an existing +// project — used to test that ImportDomains scopes its deletions to a +// single account, not the whole project. +func seedAccount(t *testing.T, s *Store, ctx context.Context, projectID uuid.UUID) Account { + t.Helper() + acc, err := s.Queries().CreateAccount(ctx, db.CreateAccountParams{ + ID: uuid.New(), ProjectID: projectID, Provider: "selectel", SecretEnc: "enc-blob", + }) + if err != nil { + t.Fatal(err) + } + return accountFromDB(acc) +} + +// TestImportDomainsRemovesVanishedZones verifies the sync behavior added to +// ImportDomains: a domain whose zone no longer appears in the provider's +// zone list is deleted on the next import, closing the orphan-domain gap +// (previously such a domain sat forever in status "error"). +func TestImportDomainsRemovesVanishedZones(t *testing.T) { + s, ctx := newStore(t) + acc, err := s.Queries().CreateAccount(ctx, db.CreateAccountParams{ + ID: uuid.New(), ProjectID: defaultProject, Provider: "selectel", SecretEnc: "enc-blob", + }) + if err != nil { + t.Fatal(err) + } + + created, removed, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{ + {ID: "z1", Name: "one.example.com."}, + {ID: "z2", Name: "two.example.com."}, + }) + if err != nil { + t.Fatal(err) + } + if len(created) != 2 || len(removed) != 0 { + t.Fatalf("first import: expected 2 created / 0 removed, got %d/%d", len(created), len(removed)) + } + + // z2 disappeared at the provider — re-import must remove its domain. + created, removed, err = s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{ + {ID: "z1", Name: "one.example.com."}, + }) + if err != nil { + t.Fatal(err) + } + if len(created) != 0 { + t.Fatalf("expected nothing new to be created, got %+v", created) + } + if len(removed) != 1 || removed[0].ZoneID != "z2" { + t.Fatalf("expected the vanished zone's domain to be removed, got %+v", removed) + } + left, err := s.ListDomains(ctx, defaultProject) + if err != nil { + t.Fatal(err) + } + if len(left) != 1 || left[0].ZoneID != "z1" { + t.Fatalf("expected only z1 to survive, got %+v", left) + } +} + +// TestImportDomainsEmptyZoneListRemovesNothing verifies the safety guard: an +// empty zone list from the provider is indistinguishable from a temporary +// loss of access, so it must never delete anything. +func TestImportDomainsEmptyZoneListRemovesNothing(t *testing.T) { + s, ctx := newStore(t) + acc, err := s.Queries().CreateAccount(ctx, db.CreateAccountParams{ + ID: uuid.New(), ProjectID: defaultProject, Provider: "selectel", SecretEnc: "enc-blob", + }) + if err != nil { + t.Fatal(err) + } + + if _, _, err := s.ImportDomains(ctx, defaultProject, acc.ID, []provider.Zone{ + {ID: "z1", Name: "one.example.com."}, + }); err != nil { + t.Fatal(err) + } + + created, removed, err := s.ImportDomains(ctx, defaultProject, acc.ID, nil) + if err != nil { + t.Fatal(err) + } + if len(created) != 0 || len(removed) != 0 { + t.Fatalf("empty zone list must be a no-op, got %d created / %d removed", len(created), len(removed)) + } + left, err := s.ListDomains(ctx, defaultProject) + if err != nil { + t.Fatal(err) + } + if len(left) != 1 { + t.Fatalf("expected the domain to survive an empty zone list, got %+v", left) + } +} + +// TestImportDomainsOnlyTouchesItsOwnAccount verifies deletion is scoped to +// the account being imported: a project may hold several provider accounts, +// and one account's zone list says nothing about another account's domains. +func TestImportDomainsOnlyTouchesItsOwnAccount(t *testing.T) { + s, ctx := newStore(t) + accA, err := s.Queries().CreateAccount(ctx, db.CreateAccountParams{ + ID: uuid.New(), ProjectID: defaultProject, Provider: "selectel", SecretEnc: "enc-blob", + }) + if err != nil { + t.Fatal(err) + } + accB := seedAccount(t, s, ctx, defaultProject) + + if _, _, err := s.ImportDomains(ctx, defaultProject, accB.ID, []provider.Zone{ + {ID: "zb", Name: "b.example.com."}, + }); err != nil { + t.Fatal(err) + } + if _, _, err := s.ImportDomains(ctx, defaultProject, accA.ID, []provider.Zone{ + {ID: "za", Name: "a.example.com."}, + }); err != nil { + t.Fatal(err) + } + + // Re-importing accA with the same single zone must not touch accB's domains. + _, removed, err := s.ImportDomains(ctx, defaultProject, accA.ID, []provider.Zone{ + {ID: "za", Name: "a.example.com."}, + }) + if err != nil { + t.Fatal(err) + } + if len(removed) != 0 { + t.Fatalf("import for one account must not remove another account's domains, got %+v", removed) + } + left, err := s.ListDomains(ctx, defaultProject) + if err != nil { + t.Fatal(err) + } + if len(left) != 2 { + t.Fatalf("expected both accounts' domains to survive, got %+v", left) + } +} diff --git a/internal/store/tenant.go b/internal/store/tenant.go index 612fd9f..3030d0f 100644 --- a/internal/store/tenant.go +++ b/internal/store/tenant.go @@ -187,25 +187,30 @@ func (s *Store) GetDomain(ctx context.Context, id, projectID uuid.UUID) (Domain, return domainFromDB(d), nil } -// ImportDomains creates one domain per zone inside a single transaction: if -// any zone fails to be created, the whole batch is rolled back so callers -// never observe a partially-imported set of domains. +// ImportDomains synchronises a provider account's zones with the domains +// stored for it, inside a single transaction: zones without a domain are +// created (ON CONFLICT DO NOTHING keeps repeated imports idempotent), and +// domains whose zone no longer exists at the provider are deleted. Deletion +// is scoped to accountID — a project may hold several provider accounts, and +// one account's zone list says nothing about another's domains. // -// Import is idempotent: zones that already have a domain for this project -// (enforced by the domains_project_zone_uniq constraint) are silently -// skipped via ON CONFLICT DO NOTHING rather than erroring or duplicating — -// so a repeated POST .../import never creates duplicate domains. Only the -// zones that were actually newly created are returned. -func (s *Store) ImportDomains(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]Domain, error) { +// An empty zone list removes nothing: it is indistinguishable from an account +// that temporarily lost access to its zones, and the cost of being wrong is +// every domain of the account gone along with its check history. +// +// Returns the domains actually created and the ones actually removed. +func (s *Store) ImportDomains(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]Domain, []Domain, error) { tx, err := s.pool.Begin(ctx) if err != nil { - return nil, err + return nil, nil, err } defer tx.Rollback(ctx) // no-op once Commit has succeeded q := s.q.WithTx(tx) - out := make([]Domain, 0, len(zones)) + created := make([]Domain, 0, len(zones)) + zoneIDs := make([]string, 0, len(zones)) for _, z := range zones { + zoneIDs = append(zoneIDs, z.ID) d, err := q.ImportDomain(ctx, db.ImportDomainParams{ ID: uuid.New(), ProjectID: projectID, ProviderAccountID: accountID, ZoneName: z.Name, ZoneID: z.ID, TemplateID: nil, @@ -216,14 +221,28 @@ func (s *Store) ImportDomains(ctx context.Context, projectID, accountID uuid.UUI // for this project — skip it rather than fail the batch. continue } - return nil, err + return nil, nil, err } - out = append(out, domainFromDB(d)) + created = append(created, domainFromDB(d)) } + + removed := make([]Domain, 0) + if len(zoneIDs) > 0 { + gone, err := q.DeleteDomainsNotInZones(ctx, db.DeleteDomainsNotInZonesParams{ + ProjectID: projectID, ProviderAccountID: accountID, ZoneIds: zoneIDs, + }) + if err != nil { + return nil, nil, err + } + for _, d := range gone { + removed = append(removed, domainFromDB(d)) + } + } + if err := tx.Commit(ctx); err != nil { - return nil, err + return nil, nil, err } - return out, nil + return created, removed, nil } // SetDomainTemplate attaches (or clears, when templateID is nil) the DNS