From a9181c5aa74fbd8ed31b0fdff8ddd19e9ad43f1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 21 Oct 2024 18:10:31 +0200 Subject: [PATCH 01/19] Add accounts.{is_deleting,next_deletion_attempt_at} --- internal/keppel/database.go | 9 +++++++++ internal/models/account.go | 3 +++ 2 files changed, 12 insertions(+) diff --git a/internal/keppel/database.go b/internal/keppel/database.go index 9896b98c2..59c59b524 100644 --- a/internal/keppel/database.go +++ b/internal/keppel/database.go @@ -304,6 +304,15 @@ var sqlMigrations = map[string]string{ ALTER TABLE peers DROP COLUMN use_for_pull_delegation; `, + "043_add_accounts_is_deleting.up.sql": ` + ALTER TABLE accounts + ADD COLUMN is_deleting BOOLEAN NOT NULL DEFAULT FALSE, + ADD COLUMN next_deletion_attempt_at TIMESTAMPTZ DEFAULT NULL; + `, + "043_add_accounts_is_deleting.down.sql": ` + ALTER TABLE accounts + DROP COLUMN is_deleting, next_deletion_attempt_at; + `, } // DB adds convenience functions on top of gorp.DbMap. diff --git a/internal/models/account.go b/internal/models/account.go index 87a55ae21..266bf5375 100644 --- a/internal/models/account.go +++ b/internal/models/account.go @@ -48,6 +48,8 @@ type Account struct { RequiredLabels string `db:"required_labels"` // InMaintenance indicates whether the account is in maintenance mode (as defined in the API spec). InMaintenance bool `db:"in_maintenance"` + // IsDeleting indicates whether the account is currently being deleted. + IsDeleting bool `db:"is_deleting"` // IsManaged indicates if the account was created by AccountManagementDriver IsManaged bool `db:"is_managed"` @@ -61,6 +63,7 @@ type Account struct { SecurityScanPoliciesJSON string `db:"security_scan_policies_json"` NextBlobSweepedAt *time.Time `db:"next_blob_sweep_at"` // see tasks.BlobSweepJob + NextDeletionAttempt *time.Time `db:"next_deletion_attempt_at"` // see tasks.AccountDeletionJob NextEnforcementAt *time.Time `db:"next_enforcement_at"` // see tasks.CreateManagedAccountsJob NextStorageSweepedAt *time.Time `db:"next_storage_sweep_at"` // see tasks.StorageSweepJob NextFederationAnnouncementAt *time.Time `db:"next_federation_announcement_at"` // see tasks.AnnounceAccountToFederationJob From 11c75ef0aa07c46654c7408d91e74e78b8df5018 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 21 Oct 2024 18:49:00 +0200 Subject: [PATCH 02/19] Mark accounts for deletion instead of complaining about things to delete --- internal/api/keppel/accounts.go | 15 +++++++++------ internal/processor/accounts.go | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/internal/api/keppel/accounts.go b/internal/api/keppel/accounts.go index f1892c4cc..5414cc468 100644 --- a/internal/api/keppel/accounts.go +++ b/internal/api/keppel/accounts.go @@ -158,18 +158,21 @@ func (a *API) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { return } - resp, err := a.processor().DeleteAccount(r.Context(), *account, keppel.AuditContext{ + if account.IsDeleting { + respondwith.JSON(w, http.StatusConflict, struct { + Error string `json:"error,omitempty"` + }{Error: "account is already set to be deleted"}) + } + + err := a.processor().MarkAccountForDeletion(*account, keppel.AuditContext{ UserIdentity: authz.UserIdentity, Request: r, }) if respondwith.ErrorText(w, err) { return } - if resp == nil { - w.WriteHeader(http.StatusNoContent) - } else { - respondwith.JSON(w, http.StatusConflict, resp) - } + + w.WriteHeader(http.StatusNoContent) } func (a *API) handlePostAccountSublease(w http.ResponseWriter, r *http.Request) { diff --git a/internal/processor/accounts.go b/internal/processor/accounts.go index 39281fc86..515f21cb3 100644 --- a/internal/processor/accounts.go +++ b/internal/processor/accounts.go @@ -367,8 +367,29 @@ var ( deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` deleteAccountMarkAllBlobsForDeletionQuery = `UPDATE blobs SET can_be_deleted_at = $2 WHERE account_name = $1` + markAccountForDeletion = `UPDATE accounts SET is_deleting = true WHERE name = $1` ) +func (p *Processor) MarkAccountForDeletion(account models.Account, actx keppel.AuditContext) error { + _, err := p.db.Exec(markAccountForDeletion, account.Name) + if err != nil { + return err + } + + if userInfo := actx.UserIdentity.UserInfo(); userInfo != nil { + p.auditor.Record(audittools.EventParameters{ + Time: p.timeNow(), + Request: actx.Request, + User: userInfo, + ReasonCode: http.StatusOK, + Action: cadf.DeleteAction, + Target: AuditAccount{Account: account}, + }) + } + + return nil +} + func (p *Processor) DeleteAccount(ctx context.Context, account models.Account, actx keppel.AuditContext) (*DeleteAccountResponse, error) { if !account.InMaintenance { return &DeleteAccountResponse{ From cd3db03861ffa742ba14065dcbf5697225e2720b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 21 Oct 2024 18:49:28 +0200 Subject: [PATCH 03/19] Consider an accounts deletion status --- internal/processor/accounts.go | 4 ++-- internal/tasks/manifests.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/processor/accounts.go b/internal/processor/accounts.go index 515f21cb3..8154e24bd 100644 --- a/internal/processor/accounts.go +++ b/internal/processor/accounts.go @@ -391,9 +391,9 @@ func (p *Processor) MarkAccountForDeletion(account models.Account, actx keppel.A } func (p *Processor) DeleteAccount(ctx context.Context, account models.Account, actx keppel.AuditContext) (*DeleteAccountResponse, error) { - if !account.InMaintenance { + if account.IsDeleting { return &DeleteAccountResponse{ - Error: "account must be set in maintenance first", + Error: "account is already set to be deleted", }, nil } diff --git a/internal/tasks/manifests.go b/internal/tasks/manifests.go index fcf851906..68bb2ec86 100644 --- a/internal/tasks/manifests.go +++ b/internal/tasks/manifests.go @@ -181,7 +181,7 @@ func (j *Janitor) syncManifestsInReplicaRepo(ctx context.Context, repo models.Re } // do not perform manifest sync while account is in maintenance (maintenance mode blocks all kinds of replication) - if !account.InMaintenance { + if !account.InMaintenance && !account.IsDeleting { syncPayload, err := j.getReplicaSyncPayload(ctx, *account, repo) if err != nil { return err @@ -640,7 +640,7 @@ func (j *Janitor) doSecurityCheck(ctx context.Context, securityInfo *models.Triv // skip validation while account is in maintenance (maintenance mode blocks // all kinds of activity on an account's contents) - if account.InMaintenance { + if account.InMaintenance || account.IsDeleting { securityInfo.NextCheckAt = j.timeNow().Add(j.addJitter(1 * time.Hour)) return nil } From 81577a3737adbd03b12ec591ee9cb16601249409 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 21 Oct 2024 18:49:47 +0200 Subject: [PATCH 04/19] Update EnforceManagedAccountsJob to reflect that it also updates accounts --- internal/tasks/account_management.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/tasks/account_management.go b/internal/tasks/account_management.go index 73df10940..52c08fabc 100644 --- a/internal/tasks/account_management.go +++ b/internal/tasks/account_management.go @@ -43,10 +43,10 @@ import ( func (j *Janitor) EnforceManagedAccountsJob(registerer prometheus.Registerer) jobloop.Job { return (&jobloop.ProducerConsumerJob[models.AccountName]{ Metadata: jobloop.JobMetadata{ - ReadableName: "create new managed accounts", + ReadableName: "create and update managed accounts", CounterOpts: prometheus.CounterOpts{ Name: "keppel_managed_account_creations", - Help: "Counter for managed account creations.", + Help: "Counter for managed account creations and updates.", }, }, DiscoverTask: j.discoverManagedAccount, From e7d064941f6d7256af642faf58c8b96e01233964 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 22 Oct 2024 15:52:48 +0200 Subject: [PATCH 05/19] Add DeleteAccountsJob by re-using most bits from EnforceManagedAccountsJob --- cmd/janitor/main.go | 1 + internal/processor/accounts.go | 147 +------------------- internal/tasks/account_deletion.go | 208 +++++++++++++++++++++++++++++ 3 files changed, 211 insertions(+), 145 deletions(-) create mode 100644 internal/tasks/account_deletion.go diff --git a/cmd/janitor/main.go b/cmd/janitor/main.go index 7dceddab6..d2b652cbb 100644 --- a/cmd/janitor/main.go +++ b/cmd/janitor/main.go @@ -69,6 +69,7 @@ func run(cmd *cobra.Command, args []string) { janitor := tasks.NewJanitor(cfg, fd, sd, icd, db, amd, auditor) go janitor.AccountFederationAnnouncementJob(nil).Run(ctx) go janitor.AbandonedUploadCleanupJob(nil).Run(ctx) + go janitor.DeleteAccountsJob(nil).Run(ctx) go janitor.EnforceManagedAccountsJob(nil).Run(ctx) go janitor.ManifestGarbageCollectionJob(nil).Run(ctx) go janitor.BlobMountSweepJob(nil).Run(ctx) diff --git a/internal/processor/accounts.go b/internal/processor/accounts.go index 8154e24bd..19f88fdb8 100644 --- a/internal/processor/accounts.go +++ b/internal/processor/accounts.go @@ -321,57 +321,12 @@ func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Ac return targetAccount, nil } -// DeleteAccountRemainingManifest appears in type DeleteAccountResponse. -type DeleteAccountRemainingManifest struct { - RepositoryName string `json:"repository"` - Digest string `json:"digest"` -} - -// DeleteAccountRemainingManifests appears in type DeleteAccountResponse. -type DeleteAccountRemainingManifests struct { - Count uint64 `json:"count"` - Next []DeleteAccountRemainingManifest `json:"next"` -} - -// DeleteAccountRemainingBlobs appears in type DeleteAccountResponse. -type DeleteAccountRemainingBlobs struct { - Count uint64 `json:"count"` -} - -// DeleteAccountResponse is returned by Processor.DeleteAccount(). -// It is the structure of the response to an account deletion API call. -type DeleteAccountResponse struct { - RemainingManifests *DeleteAccountRemainingManifests `json:"remaining_manifests,omitempty"` - RemainingBlobs *DeleteAccountRemainingBlobs `json:"remaining_blobs,omitempty"` - Error string `json:"error,omitempty"` -} - var ( - deleteAccountFindManifestsQuery = sqlext.SimplifyWhitespace(` - SELECT r.name, m.digest - FROM manifests m - JOIN repos r ON m.repo_id = r.id - JOIN accounts a ON a.name = r.account_name - LEFT OUTER JOIN manifest_manifest_refs mmr ON mmr.repo_id = r.id AND m.digest = mmr.child_digest - WHERE a.name = $1 AND parent_digest IS NULL - LIMIT 10 - `) - deleteAccountCountManifestsQuery = sqlext.SimplifyWhitespace(` - SELECT COUNT(m.digest) - FROM manifests m - JOIN repos r ON m.repo_id = r.id - JOIN accounts a ON a.name = r.account_name - WHERE a.name = $1 - `) - deleteAccountReposQuery = `DELETE FROM repos WHERE account_name = $1` - deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` - deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` - deleteAccountMarkAllBlobsForDeletionQuery = `UPDATE blobs SET can_be_deleted_at = $2 WHERE account_name = $1` - markAccountForDeletion = `UPDATE accounts SET is_deleting = true WHERE name = $1` + markAccountForDeletion = `UPDATE accounts SET is_deleting = TRUE, next_deletion_attempt_at = $1 WHERE name = $2` ) func (p *Processor) MarkAccountForDeletion(account models.Account, actx keppel.AuditContext) error { - _, err := p.db.Exec(markAccountForDeletion, account.Name) + _, err := p.db.Exec(markAccountForDeletion, p.timeNow(), account.Name) if err != nil { return err } @@ -389,101 +344,3 @@ func (p *Processor) MarkAccountForDeletion(account models.Account, actx keppel.A return nil } - -func (p *Processor) DeleteAccount(ctx context.Context, account models.Account, actx keppel.AuditContext) (*DeleteAccountResponse, error) { - if account.IsDeleting { - return &DeleteAccountResponse{ - Error: "account is already set to be deleted", - }, nil - } - - // can only delete account when user has deleted all manifests from it - var nextManifests []DeleteAccountRemainingManifest - err := sqlext.ForeachRow(p.db, deleteAccountFindManifestsQuery, []any{account.Name}, - func(rows *sql.Rows) error { - var m DeleteAccountRemainingManifest - err := rows.Scan(&m.RepositoryName, &m.Digest) - nextManifests = append(nextManifests, m) - return err - }, - ) - if err != nil { - return nil, err - } - if len(nextManifests) > 0 { - manifestCount, err := p.db.SelectInt(deleteAccountCountManifestsQuery, account.Name) - return &DeleteAccountResponse{ - RemainingManifests: &DeleteAccountRemainingManifests{ - Count: keppel.AtLeastZero(manifestCount), - Next: nextManifests, - }, - }, err - } - - // delete all repos (and therefore, all blob mounts), so that blob sweeping - // can immediately take place - _, err = p.db.Exec(deleteAccountReposQuery, account.Name) - if err != nil { - return nil, err - } - - // can only delete account when all blobs have been deleted - blobCount, err := p.db.SelectInt(deleteAccountCountBlobsQuery, account.Name) - if err != nil { - return nil, err - } - if blobCount > 0 { - // make sure that blob sweep runs immediately - _, err := p.db.Exec(deleteAccountMarkAllBlobsForDeletionQuery, account.Name, p.timeNow()) - if err != nil { - return nil, err - } - _, err = p.db.Exec(deleteAccountScheduleBlobSweepQuery, account.Name, p.timeNow()) - if err != nil { - return nil, err - } - return &DeleteAccountResponse{ - RemainingBlobs: &DeleteAccountRemainingBlobs{Count: keppel.AtLeastZero(blobCount)}, - }, nil - } - - // start deleting the account in a transaction - tx, err := p.db.Begin() - if err != nil { - return nil, err - } - defer sqlext.RollbackUnlessCommitted(tx) - _, err = tx.Delete(&account) - if err != nil { - return nil, err - } - - // before committing the transaction, confirm account deletion with the - // storage driver and the federation driver - err = p.sd.CleanupAccount(ctx, account.Reduced()) - if err != nil { - return nil, fmt.Errorf("while cleaning up storage for account: %w", err) - } - err = p.fd.ForfeitAccountName(ctx, account) - if err != nil { - return nil, fmt.Errorf("while cleaning up name claim for account: %w", err) - } - - err = tx.Commit() - if err != nil { - return nil, err - } - - if userInfo := actx.UserIdentity.UserInfo(); userInfo != nil { - p.auditor.Record(audittools.EventParameters{ - Time: p.timeNow(), - Request: actx.Request, - User: userInfo, - ReasonCode: http.StatusOK, - Action: cadf.DeleteAction, - Target: AuditAccount{Account: account}, - }) - } - - return nil, nil -} diff --git a/internal/tasks/account_deletion.go b/internal/tasks/account_deletion.go new file mode 100644 index 000000000..d8adfd6f8 --- /dev/null +++ b/internal/tasks/account_deletion.go @@ -0,0 +1,208 @@ +/****************************************************************************** +* +* Copyright 2024 SAP SE +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +* +******************************************************************************/ + +package tasks + +import ( + "context" + "database/sql" + "errors" + "fmt" + "net/http" + "time" + + "github.com/opencontainers/go-digest" + "github.com/prometheus/client_golang/prometheus" + "github.com/sapcc/go-api-declarations/cadf" + "github.com/sapcc/go-bits/audittools" + "github.com/sapcc/go-bits/jobloop" + "github.com/sapcc/go-bits/logg" + "github.com/sapcc/go-bits/sqlext" + + "github.com/sapcc/keppel/internal/keppel" + "github.com/sapcc/keppel/internal/models" + "github.com/sapcc/keppel/internal/processor" +) + +// EnforceManagedAccounts is a job. Each task creates newly discovered accounts from the driver. +func (j *Janitor) DeleteAccountsJob(registerer prometheus.Registerer) jobloop.Job { + return (&jobloop.ProducerConsumerJob[models.AccountName]{ + Metadata: jobloop.JobMetadata{ + ReadableName: "delete accounts marked for deletion", + CounterOpts: prometheus.CounterOpts{ + Name: "keppel_account_deletion", + Help: "Counter for deleted accounts.", + }, + }, + DiscoverTask: j.discoverAccountForDeletion, + ProcessTask: j.deleteMarkedAccount, + }).Setup(registerer) +} + +var ( + accountDeletionSelectQuery = sqlext.SimplifyWhitespace(` + SELECT name FROM accounts + WHERE is_deleting AND next_deletion_attempt_at < $1 + ORDER BY next_deletion_attempt_at ASC, name ASC + `) +) + +func (j *Janitor) discoverAccountForDeletion(_ context.Context, _ prometheus.Labels) (accountName models.AccountName, err error) { + err = j.db.SelectOne(&accountName, accountDeletionSelectQuery, j.timeNow()) + return accountName, err +} + +func (j *Janitor) deleteMarkedAccount(ctx context.Context, accountName models.AccountName, labels prometheus.Labels) error { + accountModel, err := keppel.FindAccount(j.db, accountName) + if errors.Is(err, sql.ErrNoRows) { + // assume the account got already deleted + return nil + } + if err != nil { + return err + } + + actx := keppel.AuditContext{ + UserIdentity: janitorUserIdentity{TaskName: "account-deletion"}, + Request: janitorDummyRequest, + } + + // can only delete account when all manifests from it are deleted + err = sqlext.ForeachRow(j.db, deleteAccountFindManifestsQuery, []any{accountModel.Name}, + func(rows *sql.Rows) error { + var m deleteAccountRemainingManifest + err := rows.Scan(&m.RepositoryName, &m.Digest) + if err != nil { + return err + } + + parsedDigest, err := digest.Parse(m.Digest) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: could not parse digest: %w", + m.Digest, m.RepositoryName, err) + } + repo, err := keppel.FindRepository(j.db, m.RepositoryName, accountModel.Name) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: could not find repository in DB: %w", + m.Digest, m.RepositoryName, err) + } + err = j.processor().DeleteManifest(ctx, accountModel.Reduced(), *repo, parsedDigest, actx) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: %w", + m.Digest, m.RepositoryName, err) + } + + return nil + }, + ) + if err != nil { + return err + } + + // delete all repos (and therefore, all blob mounts), so that blob sweeping can immediately take place + _, err = j.db.Exec(deleteAccountReposQuery, accountModel.Name) + if err != nil { + return err + } + + // can only delete account when all blobs have been deleted + blobCount, err := j.db.SelectInt(deleteAccountCountBlobsQuery, accountModel.Name) + if err != nil { + return err + } + if blobCount > 0 { + // make sure that blob sweep runs immediately + // TODO: how to prevent resetting time stamp if already set? + _, err := j.db.Exec(deleteAccountMarkAllBlobsForDeletionQuery, accountModel.Name, j.timeNow()) + if err != nil { + return err + } + + _, err = j.db.Exec(deleteAccountScheduleBlobSweepQuery, accountModel.Name, j.timeNow()) + if err != nil { + return err + } + + _, err = j.db.Exec(`UPDATE accounts SET next_deletion_attempt_at = $1 WHERE name = $2`, j.timeNow().Add(1*time.Minute), accountModel.Name) + if err != nil { + return err + } + logg.Info("cleaning up managed account %q: waiting for %d blobs to be deleted", accountModel.Name, blobCount) + return nil + } + + // start deleting the account in a transaction + tx, err := j.db.Begin() + if err != nil { + return err + } + defer sqlext.RollbackUnlessCommitted(tx) + _, err = tx.Delete(accountModel) + if err != nil { + return err + } + + // before committing the transaction, confirm account deletion with the + // storage driver and the federation driver + err = j.sd.CleanupAccount(ctx, accountModel.Reduced()) + if err != nil { + return fmt.Errorf("while cleaning up storage for account: %w", err) + } + err = j.fd.ForfeitAccountName(ctx, *accountModel) + if err != nil { + return fmt.Errorf("while cleaning up name claim for account: %w", err) + } + + err = tx.Commit() + if err != nil { + return err + } + + if userInfo := actx.UserIdentity.UserInfo(); userInfo != nil { + j.auditor.Record(audittools.EventParameters{ + Time: j.timeNow(), + Request: actx.Request, + User: userInfo, + ReasonCode: http.StatusOK, + Action: cadf.DeleteAction, + Target: processor.AuditAccount{Account: *accountModel}, + }) + } + + return nil +} + +var ( + deleteAccountFindManifestsQuery = sqlext.SimplifyWhitespace(` + SELECT r.name, m.digest + FROM manifests m + JOIN repos r ON m.repo_id = r.id + JOIN accounts a ON a.name = r.account_name + LEFT OUTER JOIN manifest_manifest_refs mmr ON mmr.repo_id = r.id AND m.digest = mmr.child_digest + WHERE a.name = $1 AND parent_digest IS NULL + `) + deleteAccountReposQuery = `DELETE FROM repos WHERE account_name = $1` + deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` + deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` + deleteAccountMarkAllBlobsForDeletionQuery = `UPDATE blobs SET can_be_deleted_at = $2 WHERE account_name = $1` +) + +type deleteAccountRemainingManifest struct { + RepositoryName string + Digest string +} From 0ba7c9c2642c7f8a8ed40a3e89e7694d5c18c987 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 22 Oct 2024 15:54:57 +0200 Subject: [PATCH 06/19] Mark account for deletion in enforceManagedAccount --- internal/tasks/account_management.go | 95 ++++++---------------------- 1 file changed, 19 insertions(+), 76 deletions(-) diff --git a/internal/tasks/account_management.go b/internal/tasks/account_management.go index 52c08fabc..29c5341dc 100644 --- a/internal/tasks/account_management.go +++ b/internal/tasks/account_management.go @@ -27,10 +27,8 @@ import ( "slices" "time" - "github.com/opencontainers/go-digest" "github.com/prometheus/client_golang/prometheus" "github.com/sapcc/go-bits/jobloop" - "github.com/sapcc/go-bits/logg" "github.com/sapcc/go-bits/sqlext" "github.com/sapcc/keppel/internal/auth" @@ -97,16 +95,25 @@ func (j *Janitor) enforceManagedAccount(ctx context.Context, accountName models. // the error returned from either tryDeleteManagedAccount or createOrUpdateManagedAccount is the main error that this method returns... var nextCheckDuration time.Duration if account == nil { - var done bool - done, err = j.tryDeleteManagedAccount(ctx, accountName) - switch { - case done: - nextCheckDuration = 0 // account has been deleted -> next check not necessary - case err == nil: - nextCheckDuration = 1 * time.Minute // we are making progress to delete this account -> recheck soon - default: - err = fmt.Errorf("could not delete managed account %q: %w", accountName, err) - nextCheckDuration = 5 * time.Minute // default interval for recheck after error + var accountModel *models.Account + accountModel, err = keppel.FindAccount(j.db, accountName) + if err != nil { + return err + } + if errors.Is(err, sql.ErrNoRows) { + nextCheckDuration = 0 // assume the account got already deleted + } else { + actx := keppel.AuditContext{ + UserIdentity: janitorUserIdentity{TaskName: "managed-account-enforcement"}, + Request: janitorDummyRequest, + } + err = j.processor().MarkAccountForDeletion(*accountModel, actx) + if err == nil { + nextCheckDuration = 1 * time.Hour // account will be deleted -> defer next check until probably after it was deleted + } else { + err = fmt.Errorf("could not mark account %q for deletion: %w", accountName, err) + nextCheckDuration = 5 * time.Minute // default interval for recheck after error + } } } else { err = j.createOrUpdateManagedAccount(ctx, *account, securityScanPolicies) @@ -130,70 +137,6 @@ func (j *Janitor) enforceManagedAccount(ctx context.Context, accountName models. } } -func (j *Janitor) tryDeleteManagedAccount(ctx context.Context, accountName models.AccountName) (done bool, err error) { - accountModel, err := keppel.FindAccount(j.db, accountName) - if err != nil { - return false, err - } - if errors.Is(err, sql.ErrNoRows) { - // assume the account got already deleted - return true, nil - } - - _, err = j.db.Exec("UPDATE accounts SET in_maintenance = TRUE WHERE name = $1", accountName) - if err != nil { - return false, err - } - // avoid quering the account twice and we set this field just above via SQL - accountModel.InMaintenance = true - - proc := j.processor() - actx := keppel.AuditContext{ - UserIdentity: janitorUserIdentity{TaskName: "account-sync"}, - Request: janitorDummyRequest, - } - resp, err := proc.DeleteAccount(ctx, *accountModel, actx) - if err != nil { - return false, err - } - if resp == nil { - // deletion was completed - return true, nil - } - if resp.Error != "" { - return false, errors.New(resp.Error) - } - - // deletion was not completed yet -> check if we need to do something on our side - if resp.RemainingManifests != nil { - remainder := *resp.RemainingManifests - logg.Info("cleaning up managed account %q: need to delete %d manifests in this cycle", accountName, remainder.Count) - for _, rm := range remainder.Next { - parsedDigest, err := digest.Parse(rm.Digest) - if err != nil { - return false, fmt.Errorf("while deleting manifest %q in repository %q: could not parse digest: %w", - rm.Digest, rm.RepositoryName, err) - } - repo, err := keppel.FindRepository(j.db, rm.RepositoryName, accountName) - if err != nil { - return false, fmt.Errorf("while deleting manifest %q in repository %q: could not find repository in DB: %w", - rm.Digest, rm.RepositoryName, err) - } - err = proc.DeleteManifest(ctx, accountModel.Reduced(), *repo, parsedDigest, actx) - if err != nil { - return false, fmt.Errorf("while deleting manifest %q in repository %q: %w", - rm.Digest, rm.RepositoryName, err) - } - } - } - if resp.RemainingBlobs != nil { - logg.Info("cleaning up managed account %q: waiting for %d blobs to be deleted", accountName, resp.RemainingBlobs.Count) - } - - // since the deletion was not finished, we will retry in the next cycle - return false, nil -} - func (j *Janitor) createOrUpdateManagedAccount(ctx context.Context, account keppel.Account, securityScanPolicies []keppel.SecurityScanPolicy) error { userIdentity := janitorUserIdentity{TaskName: "account-management"} From 2f9b2314f4534097b203c5e23b90fee70ad58416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 21 Oct 2024 19:43:26 +0200 Subject: [PATCH 07/19] Update tests with all the changes --- internal/api/keppel/accounts_test.go | 127 +++--------------- internal/api/keppel/api.go | 13 +- .../keppel/fixtures/delete-account-001.sql | 16 ++- .../keppel/fixtures/delete-account-002.sql | 7 - .../keppel/fixtures/delete-account-003.sql | 6 - internal/api/registry/api.go | 2 +- internal/processor/processor.go | 4 +- internal/tasks/account_management_test.go | 104 ++++++++------ internal/tasks/janitor.go | 2 +- internal/tasks/shared_test.go | 3 +- internal/test/setup.go | 2 +- 11 files changed, 111 insertions(+), 175 deletions(-) delete mode 100644 internal/api/keppel/fixtures/delete-account-002.sql delete mode 100644 internal/api/keppel/fixtures/delete-account-003.sql diff --git a/internal/api/keppel/accounts_test.go b/internal/api/keppel/accounts_test.go index 9590fea5f..8666901e2 100644 --- a/internal/api/keppel/accounts_test.go +++ b/internal/api/keppel/accounts_test.go @@ -1933,138 +1933,43 @@ func TestDeleteAccount(t *testing.T) { }.Check(t, h) // failure case: account not in maintenance - _, err = s.DB.Exec(`UPDATE accounts SET in_maintenance = FALSE`) - if err != nil { - t.Fatal(err.Error()) - } - assert.HTTPRequest{ - Method: "DELETE", - Path: "/keppel/v1/accounts/test1", - Header: map[string]string{"X-Test-Perms": "view:tenant1,change:tenant1"}, - ExpectStatus: http.StatusConflict, - ExpectBody: assert.JSONObject{ - "error": "account must be set in maintenance first", - }, - }.Check(t, h) - _, err = s.DB.Exec(`UPDATE accounts SET in_maintenance = TRUE`) + _, err = s.DB.Exec(`UPDATE accounts SET is_deleting = FALSE`) if err != nil { t.Fatal(err.Error()) } - // phase 1: DELETE on account should complain about remaining manifests + // phase 1: DELETE on account should immediately mark it for deletion assert.HTTPRequest{ Method: "DELETE", Path: "/keppel/v1/accounts/test1", Header: map[string]string{"X-Test-Perms": "view:tenant1,change:tenant1"}, - ExpectStatus: http.StatusConflict, - ExpectBody: assert.JSONObject{ - "remaining_manifests": assert.JSONObject{ - "count": 2, - "next": []assert.JSONObject{{ - "repository": repos[0].Name, - "digest": imageList.Manifest.Digest.String(), - }}, - }, - }, - }.Check(t, h) - - // that didn't touch the DB - easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-000.sql") - - // as indicated by the response, we need to delete the specified manifest to - // proceed with the account deletion - assert.HTTPRequest{ - Method: "DELETE", - Path: fmt.Sprintf( - "/keppel/v1/accounts/test1/repositories/%s/_manifests/%s", - repos[0].Name, imageList.Manifest.Digest.String(), - ), - Header: map[string]string{"X-Test-Perms": "view:tenant1,delete:tenant1"}, ExpectStatus: http.StatusNoContent, }.Check(t, h) + // account is already set to be deleted, so nothing happens assert.HTTPRequest{ Method: "DELETE", Path: "/keppel/v1/accounts/test1", Header: map[string]string{"X-Test-Perms": "view:tenant1,change:tenant1"}, - ExpectStatus: http.StatusConflict, - ExpectBody: assert.JSONObject{ - "remaining_manifests": assert.JSONObject{ - "count": 1, - "next": []assert.JSONObject{{ - "repository": repos[0].Name, - "digest": image.Manifest.Digest.String(), - }}, - }, - }, - }.Check(t, h) - - assert.HTTPRequest{ - Method: "DELETE", - Path: fmt.Sprintf( - "/keppel/v1/accounts/test1/repositories/%s/_manifests/%s", - repos[0].Name, image.Manifest.Digest.String(), - ), - Header: map[string]string{"X-Test-Perms": "view:tenant1,delete:tenant1"}, ExpectStatus: http.StatusNoContent, }.Check(t, h) - easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-001.sql") - // phase 2: DELETE on account should complain about remaining blobs - assert.HTTPRequest{ - Method: "DELETE", - Path: "/keppel/v1/accounts/test1", - Header: map[string]string{"X-Test-Perms": "view:tenant1,change:tenant1"}, - ExpectStatus: http.StatusConflict, - ExpectBody: assert.JSONObject{ - "remaining_blobs": assert.JSONObject{"count": 3}, + s.Auditor.ExpectEvents(t, + cadf.Event{ + RequestPath: "/keppel/v1/accounts/test1", + Action: cadf.DeleteAction, + Outcome: "success", + Reason: test.CADFReasonOK, + Target: cadf.Resource{ + TypeURI: "docker-registry/account", + ID: "test1", + ProjectID: "tenant1", + }, }, - }.Check(t, h) - - // but this will have cleaned up the blob mounts and scheduled a GC pass - // (replace time.Now() with a deterministic time before diffing the DB) - _, err = s.DB.Exec( - `UPDATE accounts SET next_blob_sweep_at = $1 WHERE next_blob_sweep_at > $2 AND next_blob_sweep_at <= $3`, - time.Unix(300, 0), - time.Now().Add(-5*time.Second), - time.Now(), ) - if err != nil { - t.Fatal(err.Error()) - } - // also all blobs will be marked for deletion - _, err = s.DB.Exec( - `UPDATE blobs SET can_be_deleted_at = $1 WHERE can_be_deleted_at > $2 AND can_be_deleted_at <= $3`, - time.Unix(300, 0), - time.Now().Add(-5*time.Second), - time.Now(), - ) - if err != nil { - t.Fatal(err.Error()) - } - easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-002.sql") - // phase 3: all blobs have been cleaned up, so the account can finally be - // deleted (we use fresh accounts for this because that's easier than - // running the blob sweep) - assert.HTTPRequest{ - Method: "DELETE", - Path: "/keppel/v1/accounts/test2", - Header: map[string]string{"X-Test-Perms": "view:tenant2,change:tenant2"}, - ExpectStatus: http.StatusNoContent, - }.Check(t, h) - - s.FD.ForfeitFails = true - assert.HTTPRequest{ - Method: "DELETE", - Path: "/keppel/v1/accounts/test3", - Header: map[string]string{"X-Test-Perms": "view:tenant3,change:tenant3"}, - ExpectStatus: http.StatusInternalServerError, - ExpectBody: assert.StringData("while cleaning up name claim for account: ForfeitAccountName failed as requested\n"), - }.Check(t, h) - - // account "test2" should be gone now - easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-003.sql") + // that didn't touch the DB + easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-001.sql") } //nolint:unparam diff --git a/internal/api/keppel/api.go b/internal/api/keppel/api.go index 351816b51..cf27f8bfc 100644 --- a/internal/api/keppel/api.go +++ b/internal/api/keppel/api.go @@ -28,6 +28,7 @@ import ( "net/url" "strconv" "strings" + "time" "github.com/gorilla/mux" "github.com/sapcc/go-bits/respondwith" @@ -48,11 +49,19 @@ type API struct { db *keppel.DB auditor keppel.Auditor rle *keppel.RateLimitEngine // may be nil + // non-pure functions that can be replaced by deterministic doubles for unit tests + timeNow func() time.Time } // NewAPI constructs a new API instance. func NewAPI(cfg keppel.Configuration, ad keppel.AuthDriver, fd keppel.FederationDriver, sd keppel.StorageDriver, icd keppel.InboundCacheDriver, db *keppel.DB, auditor keppel.Auditor, rle *keppel.RateLimitEngine) *API { - return &API{cfg, ad, fd, sd, icd, db, auditor, rle} + return &API{cfg, ad, fd, sd, icd, db, auditor, rle, time.Now} +} + +// OverrideTimeNow replaces time.Now with a test double. +func (a *API) OverrideTimeNow(timeNow func() time.Time) *API { + a.timeNow = timeNow + return a } // AddTo implements the api.API interface. @@ -91,7 +100,7 @@ func (a *API) AddTo(r *mux.Router) { } func (a *API) processor() *processor.Processor { - return processor.New(a.cfg, a.db, a.sd, a.icd, a.auditor, a.fd) + return processor.New(a.cfg, a.db, a.sd, a.icd, a.auditor, a.fd, a.timeNow) } func (a *API) handleGetAPIInfo(w http.ResponseWriter, r *http.Request) { diff --git a/internal/api/keppel/fixtures/delete-account-001.sql b/internal/api/keppel/fixtures/delete-account-001.sql index acb116d9c..f42d4bb3e 100644 --- a/internal/api/keppel/fixtures/delete-account-001.sql +++ b/internal/api/keppel/fixtures/delete-account-001.sql @@ -1,4 +1,4 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, in_maintenance) VALUES ('test1', 'tenant1', 200, TRUE); +INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, in_maintenance, is_deleting, next_deletion_attempt_at) VALUES ('test1', 'tenant1', 200, TRUE, TRUE, 0); INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test2', 'tenant2', TRUE); INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test3', 'tenant3', TRUE); @@ -10,5 +10,19 @@ INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (2, 'test1', 'sha256:3ae14a50df760250f0e97faf429cc4541c832ed0de61ad5b6ac25d1d695d1a6e', 1048919, 'd4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35', 1, 604801); INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (3, 'test1', 'sha256:92b29e540b6fcadd4e07525af1546c7eff1bb9a8ef0ef249e0b234cdb13dbea3', 1412, '4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce', 2, 604802); +INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 1); +INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 2); +INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 3); + +INSERT INTO manifest_contents (repo_id, digest, content) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', '{"manifests":[{"digest":"sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c","mediaType":"application/vnd.docker.distribution.manifest.v2+json","platform":{"architecture":"amd64","os":"linux"},"size":592}],"mediaType":"application/vnd.docker.distribution.manifest.list.v2+json","schemaVersion":2}'); + +INSERT INTO manifest_manifest_refs (repo_id, parent_digest, child_digest) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c'); + +INSERT INTO manifests (repo_id, digest, media_type, size_bytes, pushed_at, next_validation_at) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'application/vnd.docker.distribution.manifest.list.v2+json', 909, 0, 86400); +INSERT INTO manifests (repo_id, digest, media_type, size_bytes, pushed_at, next_validation_at) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 'application/vnd.docker.distribution.manifest.v2+json', 592, 100, 86500); + INSERT INTO repos (id, account_name, name) VALUES (1, 'test1', 'foo/bar'); INSERT INTO repos (id, account_name, name) VALUES (2, 'test1', 'something-else'); + +INSERT INTO trivy_security_info (repo_id, digest, vuln_status, message, next_check_at) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'Pending', '', 0); +INSERT INTO trivy_security_info (repo_id, digest, vuln_status, message, next_check_at) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 'Pending', '', 0); diff --git a/internal/api/keppel/fixtures/delete-account-002.sql b/internal/api/keppel/fixtures/delete-account-002.sql deleted file mode 100644 index d51e9a638..000000000 --- a/internal/api/keppel/fixtures/delete-account-002.sql +++ /dev/null @@ -1,7 +0,0 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, in_maintenance) VALUES ('test1', 'tenant1', 300, TRUE); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test2', 'tenant2', TRUE); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test3', 'tenant3', TRUE); - -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, can_be_deleted_at, next_validation_at) VALUES (1, 'test1', 'sha256:442f91fa9998460f28e8ff7023e5ddca679f7d2b51dc5498e8aba249678cc7f8', 1048919, '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b', 0, 300, 604800); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, can_be_deleted_at, next_validation_at) VALUES (2, 'test1', 'sha256:3ae14a50df760250f0e97faf429cc4541c832ed0de61ad5b6ac25d1d695d1a6e', 1048919, 'd4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35', 1, 300, 604801); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, can_be_deleted_at, next_validation_at) VALUES (3, 'test1', 'sha256:92b29e540b6fcadd4e07525af1546c7eff1bb9a8ef0ef249e0b234cdb13dbea3', 1412, '4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce', 2, 300, 604802); diff --git a/internal/api/keppel/fixtures/delete-account-003.sql b/internal/api/keppel/fixtures/delete-account-003.sql deleted file mode 100644 index 97207bc53..000000000 --- a/internal/api/keppel/fixtures/delete-account-003.sql +++ /dev/null @@ -1,6 +0,0 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, in_maintenance) VALUES ('test1', 'tenant1', 300, TRUE); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test3', 'tenant3', TRUE); - -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, can_be_deleted_at, next_validation_at) VALUES (1, 'test1', 'sha256:442f91fa9998460f28e8ff7023e5ddca679f7d2b51dc5498e8aba249678cc7f8', 1048919, '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b', 0, 300, 604800); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, can_be_deleted_at, next_validation_at) VALUES (2, 'test1', 'sha256:3ae14a50df760250f0e97faf429cc4541c832ed0de61ad5b6ac25d1d695d1a6e', 1048919, 'd4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35', 1, 300, 604801); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, can_be_deleted_at, next_validation_at) VALUES (3, 'test1', 'sha256:92b29e540b6fcadd4e07525af1546c7eff1bb9a8ef0ef249e0b234cdb13dbea3', 1412, '4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce', 2, 300, 604802); diff --git a/internal/api/registry/api.go b/internal/api/registry/api.go index 1f9638991..0499b2810 100644 --- a/internal/api/registry/api.go +++ b/internal/api/registry/api.go @@ -114,7 +114,7 @@ func (a *API) AddTo(r *mux.Router) { } func (a *API) processor() *processor.Processor { - return processor.New(a.cfg, a.db, a.sd, a.icd, a.auditor, a.fd).OverrideTimeNow(a.timeNow).OverrideGenerateStorageID(a.generateStorageID) + return processor.New(a.cfg, a.db, a.sd, a.icd, a.auditor, a.fd, a.timeNow).OverrideTimeNow(a.timeNow).OverrideGenerateStorageID(a.generateStorageID) } // This implements the GET /v2/ endpoint. diff --git a/internal/processor/processor.go b/internal/processor/processor.go index 2635a184e..1ba6ec076 100644 --- a/internal/processor/processor.go +++ b/internal/processor/processor.go @@ -52,8 +52,8 @@ type Processor struct { } // New creates a new Processor. -func New(cfg keppel.Configuration, db *keppel.DB, sd keppel.StorageDriver, icd keppel.InboundCacheDriver, auditor keppel.Auditor, fd keppel.FederationDriver) *Processor { - return &Processor{cfg, db, fd, sd, icd, auditor, make(map[string]*client.RepoClient), time.Now, keppel.GenerateStorageID} +func New(cfg keppel.Configuration, db *keppel.DB, sd keppel.StorageDriver, icd keppel.InboundCacheDriver, auditor keppel.Auditor, fd keppel.FederationDriver, timenow func() time.Time) *Processor { + return &Processor{cfg, db, fd, sd, icd, auditor, make(map[string]*client.RepoClient), timenow, keppel.GenerateStorageID} } // OverrideTimeNow replaces time.Now with a test double. diff --git a/internal/tasks/account_management_test.go b/internal/tasks/account_management_test.go index d04ff7332..a60201765 100644 --- a/internal/tasks/account_management_test.go +++ b/internal/tasks/account_management_test.go @@ -37,11 +37,12 @@ func TestAccountManagementBasic(t *testing.T) { tr, tr0 := easypg.NewTracker(t, s.DB.DbMap.Db) tr0.Ignore() - job := j.EnforceManagedAccountsJob(s.Registry) + managedAccountsJob := j.EnforceManagedAccountsJob(s.Registry) + deleteAccountsJob := j.DeleteAccountsJob(s.Registry) // we haven't configured any account, so it should do nothing s.AMD.ConfigPath = "./fixtures/account_management_empty.json" - expectError(t, sql.ErrNoRows.Error(), job.ProcessOne(s.Ctx)) + expectError(t, sql.ErrNoRows.Error(), managedAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEmpty() // configure an account to create @@ -49,9 +50,9 @@ func TestAccountManagementBasic(t *testing.T) { s.Clock.StepBy(1 * time.Hour) // we only have one account defined which should be created - expectSuccess(t, job.ProcessOne(s.Ctx)) + expectSuccess(t, managedAccountsJob.ProcessOne(s.Ctx)) // since we are enforcing that account, no error is returned - expectError(t, sql.ErrNoRows.Error(), job.ProcessOne(s.Ctx)) + expectError(t, sql.ErrNoRows.Error(), managedAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEqualf(` INSERT INTO accounts (name, auth_tenant_id, required_labels, external_peer_url, gc_policies_json, security_scan_policies_json, rbac_policies_json, is_managed, next_enforcement_at) VALUES ('abcde', '12345', 'important-label,some-label', 'registry-tertiary.example.org', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":6,"unit":"h"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":".*","match_vulnerability_id":".*","except_fix_released":true,"action":{"assessment":"risk accepted: vulnerabilities without an available fix are not actionable","ignore":true}}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]', TRUE, %d); `, @@ -60,15 +61,25 @@ func TestAccountManagementBasic(t *testing.T) { // test if errors are propagated s.AMD.ConfigPath = "./fixtures/account_management_error.json" s.Clock.StepBy(2 * time.Hour) - expectError(t, fmt.Sprintf("could not configure managed account %q: %s", "", processor.ErrAccountNameEmpty.Error()), job.ProcessOne(s.Ctx)) + expectError(t, fmt.Sprintf("could not configure managed account %q: %s", "", processor.ErrAccountNameEmpty.Error()), managedAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEmpty() // and delete the account again s.AMD.ConfigPath = "./fixtures/account_management_empty.json" s.Clock.StepBy(2 * time.Hour) - expectSuccess(t, job.ProcessOne(s.Ctx)) - expectError(t, sql.ErrNoRows.Error(), job.ProcessOne(s.Ctx)) + // now the account is being marked for deletion + expectSuccess(t, managedAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEqualf(` + UPDATE accounts SET next_enforcement_at = %d, is_deleting = TRUE, next_deletion_attempt_at = %d WHERE name = 'abcde'; + `, + s.Clock.Now().Add(1*time.Hour).Unix(), + s.Clock.Now().Unix(), + ) + + // and we can immeadetaly delete it yet because it has no manifests and blobs attached to it + s.Clock.StepBy(1 * time.Hour) + expectSuccess(t, deleteAccountsJob.ProcessOne(s.Ctx)) + tr.DBChanges().AssertEqual(` DELETE FROM accounts WHERE name = 'abcde'; `) } @@ -104,15 +115,17 @@ func TestAccountManagementWithReplicaCreation(t *testing.T) { func TestAccountManagementWithComplexDeletion(t *testing.T) { j, s := setup(t) - job := j.EnforceManagedAccountsJob(s.Registry) + managedAccountsJob := j.EnforceManagedAccountsJob(s.Registry) + deleteAccountsJob := j.DeleteAccountsJob(s.Registry) + blobSweepJob := j.BlobSweepJob(s.Registry) tr, tr0 := easypg.NewTracker(t, s.DB.DbMap.Db) tr0.Ignore() // create a managed account named "abcde" s.AMD.ConfigPath = "./fixtures/account_management_basic.json" - expectSuccess(t, job.ProcessOne(s.Ctx)) - expectError(t, sql.ErrNoRows.Error(), job.ProcessOne(s.Ctx)) + expectSuccess(t, managedAccountsJob.ProcessOne(s.Ctx)) + expectError(t, sql.ErrNoRows.Error(), managedAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().Ignore() // give quota to its auth tenant @@ -133,38 +146,56 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { image.MustUpload(t, s, repo, "latest") tr.DBChanges().Ignore() - // try to delete the managed account: first attempt will set in_maintenance and delete the image, but nothing more + // remove the managed account: this will set is_deleting, but nothing more s.AMD.ConfigPath = "./fixtures/account_management_empty.json" s.Clock.StepBy(2 * time.Hour) - expectSuccess(t, job.ProcessOne(s.Ctx)) + expectSuccess(t, managedAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEqualf(` - UPDATE accounts SET in_maintenance = TRUE, next_enforcement_at = %[1]d WHERE name = 'abcde'; - DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[2]s' AND blob_id = 1; - DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[2]s' AND blob_id = 2; - DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[2]s' AND blob_id = 3; - DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[2]s'; - DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[2]s'; - DELETE FROM tags WHERE repo_id = 2 AND name = 'latest'; - DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[2]s'; + UPDATE accounts SET next_enforcement_at = %d, is_deleting = TRUE, next_deletion_attempt_at = %d WHERE name = 'abcde'; `, - s.Clock.Now().Add(1*time.Minute).Unix(), - image.Manifest.Digest.String(), + s.Clock.Now().Add(1*time.Hour).Unix(), + s.Clock.Now().Unix(), ) - // second try: since there are no manifests left... + // the deleteAccountsJob will delete: + // - all manifests // - all repos (and thus blob mounts) will be deleted // - all blobs are marked for immediate deletion, and a blob GC is scheduled - s.Clock.StepBy(2 * time.Minute) - expectSuccess(t, job.ProcessOne(s.Ctx)) + s.Clock.StepBy(1 * time.Minute) + expectSuccess(t, deleteAccountsJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEqualf(` - UPDATE accounts SET next_blob_sweep_at = %[1]d, next_enforcement_at = %[2]d WHERE name = 'abcde'; + UPDATE accounts SET next_blob_sweep_at = %[1]d, next_deletion_attempt_at = %[2]d WHERE name = 'abcde'; DELETE FROM blob_mounts WHERE blob_id = 1 AND repo_id = 2; DELETE FROM blob_mounts WHERE blob_id = 2 AND repo_id = 2; DELETE FROM blob_mounts WHERE blob_id = 3 AND repo_id = 2; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 1 AND account_name = 'abcde' AND digest = '%[4]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 2 AND account_name = 'abcde' AND digest = '%[5]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 3 AND account_name = 'abcde' AND digest = '%[6]s'; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[3]s' AND blob_id = 1; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[3]s' AND blob_id = 2; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[3]s' AND blob_id = 3; + DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[3]s'; + DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[3]s'; + DELETE FROM repos WHERE id = 2 AND account_name = 'abcde' AND name = 'foo'; + DELETE FROM tags WHERE repo_id = 2 AND name = 'latest'; + DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[3]s'; + `, + s.Clock.Now().Unix(), + s.Clock.Now().Add(1*time.Minute).Unix(), + image.Manifest.Digest.String(), + image.Layers[0].Digest.String(), + image.Layers[1].Digest.String(), + image.Config.Digest.String(), + ) + + // TODO: fix the can_be_deleted_at reset + s.Clock.StepBy(3 * time.Minute) + expectSuccess(t, deleteAccountsJob.ProcessOne(s.Ctx)) + tr.DBChanges().AssertEqualf(` + UPDATE accounts SET next_blob_sweep_at = %[1]d, next_deletion_attempt_at = %[2]d WHERE name = 'abcde'; UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 1 AND account_name = 'abcde' AND digest = '%[3]s'; UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 2 AND account_name = 'abcde' AND digest = '%[4]s'; UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 3 AND account_name = 'abcde' AND digest = '%[5]s'; - DELETE FROM repos WHERE id = 2 AND account_name = 'abcde' AND name = 'foo'; `, s.Clock.Now().Unix(), s.Clock.Now().Add(1*time.Minute).Unix(), @@ -173,12 +204,10 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { image.Config.Digest.String(), ) - // to make further progress, the scheduled blob GC needs to go through first - // (we need to run this twice because the common test setup includes another account that is irrelevant to this test) - s.Clock.StepBy(1 * time.Second) - blobGCJob := j.BlobSweepJob(s.Registry) - expectSuccess(t, blobGCJob.ProcessOne(s.Ctx)) - expectSuccess(t, blobGCJob.ProcessOne(s.Ctx)) + s.Clock.StepBy(1 * time.Minute) + // we need to run this twice because the common test setup includes another account that is irrelevant to this test + expectSuccess(t, blobSweepJob.ProcessOne(s.Ctx)) + expectSuccess(t, blobSweepJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEqualf(` UPDATE accounts SET next_blob_sweep_at = %[1]d WHERE name = 'abcde'; UPDATE accounts SET next_blob_sweep_at = %[1]d WHERE name = 'test1'; @@ -186,16 +215,9 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { DELETE FROM blobs WHERE id = 2 AND account_name = 'abcde' AND digest = '%[3]s'; DELETE FROM blobs WHERE id = 3 AND account_name = 'abcde' AND digest = '%[4]s'; `, - s.Clock.Now().Add(1*time.Hour).Unix(), + s.Clock.Now().Add(60*time.Minute).Unix(), image.Layers[0].Digest.String(), image.Layers[1].Digest.String(), image.Config.Digest.String(), ) - - // third try: this time, the account deletion can go through - s.Clock.StepBy(2 * time.Minute) - expectSuccess(t, job.ProcessOne(s.Ctx)) - tr.DBChanges().AssertEqualf(` - DELETE FROM accounts WHERE name = 'abcde'; - `) } diff --git a/internal/tasks/janitor.go b/internal/tasks/janitor.go index 7dabe1e8b..12bada4bb 100644 --- a/internal/tasks/janitor.go +++ b/internal/tasks/janitor.go @@ -96,7 +96,7 @@ func addJitter(duration time.Duration) time.Duration { } func (j *Janitor) processor() *processor.Processor { - return processor.New(j.cfg, j.db, j.sd, j.icd, j.auditor, j.fd).OverrideTimeNow(j.timeNow).OverrideGenerateStorageID(j.generateStorageID) + return processor.New(j.cfg, j.db, j.sd, j.icd, j.auditor, j.fd, j.timeNow).OverrideTimeNow(j.timeNow).OverrideGenerateStorageID(j.generateStorageID) } //////////////////////////////////////////////////////////////////////////////// diff --git a/internal/tasks/shared_test.go b/internal/tasks/shared_test.go index b4dfe0740..291185d8f 100644 --- a/internal/tasks/shared_test.go +++ b/internal/tasks/shared_test.go @@ -46,8 +46,7 @@ func setup(t *testing.T, opts ...test.SetupOption) (*Janitor, test.Setup) { return j, s } -//nolint:unparam -func forAllReplicaTypes(t *testing.T, action func(string)) { +func forAllReplicaTypes(_ *testing.T, action func(string)) { action("on_first_use") action("from_external_on_first_use") } diff --git a/internal/test/setup.go b/internal/test/setup.go index 3bc4a8e02..07b36c7bc 100644 --- a/internal/test/setup.go +++ b/internal/test/setup.go @@ -344,7 +344,7 @@ func NewSetup(t *testing.T, opts ...SetupOption) Setup { authapi.NewAPI(s.Config, ad, fd, s.DB), } if params.WithKeppelAPI { - apis = append(apis, keppelv1.NewAPI(s.Config, ad, fd, sd, icd, s.DB, s.Auditor, params.RateLimitEngine)) + apis = append(apis, keppelv1.NewAPI(s.Config, ad, fd, sd, icd, s.DB, s.Auditor, params.RateLimitEngine).OverrideTimeNow(s.Clock.Now)) } if params.WithPeerAPI { apis = append(apis, peerv1.NewAPI(s.Config, ad, s.DB)) From 5a48a5b55366e2466bbb635a5d1bb529d69aa9fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 22 Oct 2024 19:02:28 +0200 Subject: [PATCH 08/19] Almost drop unused metadata field, almost drop in_maintenance in the API and replace it with state --- docs/api-spec.md | 15 +- internal/api/keppel/accounts.go | 5 +- internal/api/keppel/accounts_test.go | 227 ++++++++---------- .../keppel/fixtures/delete-account-000.sql | 6 +- .../keppel/fixtures/delete-account-001.sql | 6 +- internal/api/registry/blobs_test.go | 22 +- internal/api/registry/manifests.go | 6 +- internal/api/registry/manifests_test.go | 6 +- internal/api/registry/replication_test.go | 4 +- internal/api/registry/shared_test.go | 6 +- internal/api/registry/uploads.go | 4 +- .../drivers/basic/account_management_test.go | 1 - internal/keppel/account.go | 23 +- internal/keppel/database.go | 17 +- internal/keppel/database_helpers.go | 4 +- internal/models/account.go | 8 +- internal/processor/accounts.go | 12 +- internal/tasks/manifests.go | 6 +- internal/tasks/manifests_test.go | 10 +- 19 files changed, 181 insertions(+), 207 deletions(-) diff --git a/docs/api-spec.md b/docs/api-spec.md index 1eed3d7f6..6741bd22d 100644 --- a/docs/api-spec.md +++ b/docs/api-spec.md @@ -74,7 +74,6 @@ On success, returns 200 and a JSON response body like this: { "name": "firstaccount", "auth_tenant_id": "firsttenant", - "metadata": {}, "rbac_policies": [ { "match_repository": "library/.*", @@ -109,9 +108,6 @@ On success, returns 200 and a JSON response body like this: { "name": "secondaccount", "auth_tenant_id": "secondtenant", - "metadata": { - "priority": "just an example" - }, "rbac_policies": [], "replication": { "strategy": "on_first_use", @@ -142,8 +138,7 @@ The following fields may be returned: | `accounts[].gc_policies[].time_constraint.oldest`
`accounts[].gc_policies[].time_constraint.newest` | integer or omitted | If set, the GC policy only applies to at most that many images within each repository, specifically to those that are oldest/newest ones when ordered by the timestamp attribute specified in the `time_constraint.on` key. These constraints are forbidden for policies with action "delete" to ensure that GC runs are idempotent. | | `accounts[].gc_policies[].time_constraint.older_than`
`accounts[].gc_policies[].time_constraint.newer_than` | duration or omitted | If set, the GC policy only applies to at most images whose timestamp (as selected by the `time_constraint.on` key) is older/newer than the given age. Durations are given as a JSON object with the keys `value` (integer) and `unit` (string), e.g. `{"value": 4, "unit": "d"}` for 4 days. The units `s` (second), `m` (minute), `h` (hour), `d` (day), `w` (7 days) and `y` (365 days) are understood. | | `accounts[].gc_policies[].action` | string | One of: `delete` (to delete matching images) or `protect` (to not delete matching images, even if another policy with a lower priority would want to). | -| `accounts[].in_maintenance` | bool | Whether this account is in maintenance mode. [See below](#maintenance-mode) for details. | -| `accounts[].metadata` | object of strings | Free-form metadata maintained by the user. The contents of this field are not interpreted by Keppel, but may trigger special behavior in applications using this API. | +| `accounts[].state` | string | The state of the account. Only shown when there is a specific state to report. [See below](#account-state) for possible values and details. | | `accounts[].rbac_policies` | list of objects | Policies for rule-based access control (RBAC) to repositories in this account. RBAC policies are evaluated in addition to the permissions granted by the auth tenant. | | `accounts[].rbac_policies[].match_cidr` | string | The RBAC policy applies to requests which originate from an IP address that matches the CIDR. | | `accounts[].rbac_policies[].match_repository` | string | The RBAC policy applies to all repositories in this account whose name matches this regex. The leading account name and slash is stripped from the repository name before matching. The notes on regexes below apply. | @@ -204,18 +199,16 @@ The following fields are shown on accounts configured with this strategy: Note that the `accounts[].replication.upstream.password` field is omitted from GET responses for security reasons. -### Maintenance mode +### Account state -When `accounts[].in_maintenance` is true, the following differences in behavior apply to this account: +When `accounts[].state` is `deleting`, the following differences in behavior apply to this account: - For primary accounts (i.e. accounts that are not replicas), no new blobs or manifests may be pushed. Only pulling and deleting are allowed. - For replica accounts, no new blobs or manifests will be replicated. Pulling is still allowed, but it becomes possible to delete blobs and manifests. -Maintenance mode is a significant part of the account deletion workflow: Sending a DELETE request on an account is only -allowed while the account is in maintenance mode, and the caller must have deleted all manifests from the account before -attempting to DELETE it. +Sending a DELETE request on an account moves it into `state = "deleting"` and schedules the deletion of everything that belongs to the account, including manifests and blobs. ## GET /keppel/v1/accounts/:name diff --git a/internal/api/keppel/accounts.go b/internal/api/keppel/accounts.go index 5414cc468..6ad21903b 100644 --- a/internal/api/keppel/accounts.go +++ b/internal/api/keppel/accounts.go @@ -159,9 +159,8 @@ func (a *API) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { } if account.IsDeleting { - respondwith.JSON(w, http.StatusConflict, struct { - Error string `json:"error,omitempty"` - }{Error: "account is already set to be deleted"}) + w.WriteHeader(http.StatusNoContent) + return } err := a.processor().MarkAccountForDeletion(*account, keppel.AuditContext{ diff --git a/internal/api/keppel/accounts_test.go b/internal/api/keppel/accounts_test.go index 8666901e2..902f639ea 100644 --- a/internal/api/keppel/accounts_test.go +++ b/internal/api/keppel/accounts_test.go @@ -78,10 +78,6 @@ func TestAccountsAPI(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "metadata": assert.JSONObject{ - "bar": "barbar", - "foo": "foofoo", - }, }, }, ExpectStatus: http.StatusOK, @@ -90,11 +86,8 @@ func TestAccountsAPI(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{ - "bar": "barbar", - "foo": "foofoo", - }, - "rbac_policies": []assert.JSONObject{}, + "metadata": nil, + "rbac_policies": []assert.JSONObject{}, }, }, }.Check(t, h) @@ -128,11 +121,8 @@ func TestAccountsAPI(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{ - "bar": "barbar", - "foo": "foofoo", - }, - "rbac_policies": []assert.JSONObject{}, + "metadata": nil, + "rbac_policies": []assert.JSONObject{}, }}, }, }.Check(t, h) @@ -146,11 +136,8 @@ func TestAccountsAPI(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{ - "bar": "barbar", - "foo": "foofoo", - }, - "rbac_policies": []assert.JSONObject{}, + "metadata": nil, + "rbac_policies": []assert.JSONObject{}, }, }, }.Check(t, h) @@ -223,7 +210,7 @@ func TestAccountsAPI(t *testing.T) { "auth_tenant_id": "tenant1", "gc_policies": gcPoliciesJSON, "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": rbacPoliciesJSON, }, }, @@ -273,18 +260,15 @@ func TestAccountsAPI(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{ - "bar": "barbar", - "foo": "foofoo", - }, - "rbac_policies": []assert.JSONObject{}, + "metadata": nil, + "rbac_policies": []assert.JSONObject{}, }, { "name": "second", "auth_tenant_id": "tenant1", "gc_policies": gcPoliciesJSON, "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": rbacPoliciesJSON, }, }, @@ -301,82 +285,75 @@ func TestAccountsAPI(t *testing.T) { "auth_tenant_id": "tenant1", "gc_policies": gcPoliciesJSON, "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": rbacPoliciesJSON, }, }, }.Check(t, h) tr.DBChanges().AssertEqual(` - INSERT INTO accounts (name, auth_tenant_id, metadata_json) VALUES ('first', 'tenant1', '{"bar":"barbar","foo":"foofoo"}'); + INSERT INTO accounts (name, auth_tenant_id) VALUES ('first', 'tenant1'); INSERT INTO accounts (name, auth_tenant_id, gc_policies_json, rbac_policies_json) VALUES ('second', 'tenant1', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":10,"unit":"d"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]'); `) - // check editing of InMaintenance flag (this also tests editing of GC policies - // since we don't give any and thus clear the field) - for _, inMaintenance := range []bool{true, false} { - assert.HTTPRequest{ - Method: "PUT", - Path: "/keppel/v1/accounts/second", - Header: map[string]string{"X-Test-Perms": "change:tenant1"}, - Body: assert.JSONObject{ - "account": assert.JSONObject{ - "auth_tenant_id": "tenant1", - "in_maintenance": inMaintenance, - "rbac_policies": rbacPoliciesJSON, - }, - }, - ExpectStatus: http.StatusOK, - ExpectBody: assert.JSONObject{ - "account": assert.JSONObject{ - "name": "second", - "auth_tenant_id": "tenant1", - "in_maintenance": inMaintenance, - "metadata": assert.JSONObject{}, - "rbac_policies": rbacPoliciesJSON, - }, - }, - }.Check(t, h) + account := assert.JSONObject{ + "auth_tenant_id": "tenant1", + "rbac_policies": rbacPoliciesJSON, + } + accountExpect := assert.JSONObject{ + "name": "second", + "in_maintenance": false, + "metadata": nil, + "auth_tenant_id": "tenant1", + "rbac_policies": rbacPoliciesJSON, + } - assert.HTTPRequest{ - Method: "GET", - Path: "/keppel/v1/accounts/second", - Header: map[string]string{"X-Test-Perms": "view:tenant1"}, - ExpectStatus: http.StatusOK, - ExpectBody: assert.JSONObject{ - "account": assert.JSONObject{ - "name": "second", - "auth_tenant_id": "tenant1", - "in_maintenance": inMaintenance, - "metadata": assert.JSONObject{}, - "rbac_policies": rbacPoliciesJSON, - }, + assert.HTTPRequest{ + Method: "PUT", + Path: "/keppel/v1/accounts/second", + Header: map[string]string{"X-Test-Perms": "change:tenant1"}, + Body: assert.JSONObject{ + "account": account, + }, + ExpectStatus: http.StatusOK, + ExpectBody: assert.JSONObject{ + "account": accountExpect, + }, + }.Check(t, h) + + assert.HTTPRequest{ + Method: "GET", + Path: "/keppel/v1/accounts/second", + Header: map[string]string{"X-Test-Perms": "view:tenant1"}, + ExpectStatus: http.StatusOK, + ExpectBody: assert.JSONObject{ + "account": assert.JSONObject{ + "name": "second", + "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, + "rbac_policies": rbacPoliciesJSON, }, - }.Check(t, h) + }, + }.Check(t, h) - // the first pass also generates an audit event since we're touching the GCPolicies - if inMaintenance { - s.Auditor.ExpectEvents(t, - cadf.Event{ - RequestPath: "/keppel/v1/accounts/second", - Action: cadf.UpdateAction, - Outcome: "success", - Reason: test.CADFReasonOK, - Target: cadf.Resource{ - TypeURI: "docker-registry/account", - ID: "second", - ProjectID: "tenant1", - Attachments: []cadf.Attachment{{ - Name: "rbac-policies", - TypeURI: "mime:application/json", - Content: toJSONVia[[]keppel.RBACPolicy](rbacPoliciesJSON), - }}, - }, - }, - ) - } else { - s.Auditor.ExpectEvents(t /*, nothing */) - } - } + s.Auditor.ExpectEvents(t, + cadf.Event{ + RequestPath: "/keppel/v1/accounts/second", + Action: cadf.UpdateAction, + Outcome: "success", + Reason: test.CADFReasonOK, + Target: cadf.Resource{ + TypeURI: "docker-registry/account", + ID: "second", + ProjectID: "tenant1", + Attachments: []cadf.Attachment{{ + Name: "rbac-policies", + TypeURI: "mime:application/json", + Content: toJSONVia[[]keppel.RBACPolicy](rbacPoliciesJSON), + }}, + }, + }, + ) // check editing of metadata and RBAC policies newRBACPoliciesJSON := []assert.JSONObject{ @@ -394,10 +371,6 @@ func TestAccountsAPI(t *testing.T) { "permissions": []string{"pull", "delete"}, }, } - newMetadataJSON := assert.JSONObject{ - "foo": "bingo", - "bar": "buz", - } assert.HTTPRequest{ Method: "PUT", Path: "/keppel/v1/accounts/second", @@ -405,9 +378,7 @@ func TestAccountsAPI(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - // add metadata - "metadata": newMetadataJSON, - "rbac_policies": newRBACPoliciesJSON, + "rbac_policies": newRBACPoliciesJSON, }, }, ExpectStatus: http.StatusOK, @@ -416,7 +387,7 @@ func TestAccountsAPI(t *testing.T) { "name": "second", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": newMetadataJSON, + "metadata": nil, "rbac_policies": newRBACPoliciesJSON, }, }, @@ -460,7 +431,7 @@ func TestAccountsAPI(t *testing.T) { "name": "second", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": newRBACPoliciesJSON, "validation": assert.JSONObject{ "required_labels": []string{"foo", "bar"}, @@ -489,7 +460,7 @@ func TestAccountsAPI(t *testing.T) { "name": "second", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": newRBACPoliciesJSON, }, }, @@ -569,7 +540,7 @@ func TestPutAccountErrorCases(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, }, }, @@ -1062,7 +1033,7 @@ func TestPutAccountErrorCases(t *testing.T) { "account": assert.JSONObject{ "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "name": "first", "rbac_policies": []assert.JSONObject{{ "match_cidr": "1.2.0.0/16", @@ -1083,7 +1054,7 @@ func TestPutAccountErrorCases(t *testing.T) { "account": assert.JSONObject{ "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "name": "first", "rbac_policies": []assert.JSONObject{{ "match_cidr": "1.2.0.0/16", @@ -1099,6 +1070,8 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "library/.+", "match_username": "foo", @@ -1116,6 +1089,8 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "library/.+", "permissions": []string{"anonymous_first_pull"}, @@ -1133,6 +1108,8 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "*/library", "permissions": []string{"anonymous_pull"}, @@ -1149,6 +1126,8 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "library/.+", "match_username": "[a-z]++@tenant2", @@ -1168,6 +1147,8 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "platform_filter": []assert.JSONObject{{ "os": "linux", "architecture": "amd64", @@ -1186,6 +1167,8 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "platform_filter": []assert.JSONObject{{ "os": "linux", "architecture": "amd64", @@ -1259,7 +1242,7 @@ func TestGetPutAccountReplicationOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, }, }, @@ -1357,7 +1340,7 @@ func TestGetPutAccountReplicationOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "on_first_use", @@ -1384,7 +1367,7 @@ func TestGetPutAccountReplicationOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "on_first_use", @@ -1419,7 +1402,7 @@ func TestGetPutAccountReplicationOnFirstUse(t *testing.T) { "name": "second", "auth_tenant_id": "tenant2", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, }, }, @@ -1555,7 +1538,7 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -1585,7 +1568,7 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -1622,7 +1605,7 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -1647,8 +1630,6 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": assert.JSONObject{}, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -1666,7 +1647,7 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -1688,8 +1669,6 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": assert.JSONObject{}, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -1752,7 +1731,7 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { "name": "second", "auth_tenant_id": "tenant2", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, }, }, @@ -1836,9 +1815,9 @@ func TestDeleteAccount(t *testing.T) { // setup test accounts and repositories nextBlobSweepAt := time.Unix(200, 0) accounts := []*models.Account{ - {Name: "test1", AuthTenantID: "tenant1", InMaintenance: true, NextBlobSweepedAt: &nextBlobSweepAt, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, - {Name: "test2", AuthTenantID: "tenant2", InMaintenance: true, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, - {Name: "test3", AuthTenantID: "tenant3", InMaintenance: true, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, + {Name: "test1", AuthTenantID: "tenant1", NextBlobSweepedAt: &nextBlobSweepAt, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, + {Name: "test2", AuthTenantID: "tenant2", GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, + {Name: "test3", AuthTenantID: "tenant3", GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, } for _, account := range accounts { mustInsert(t, s.DB, account) @@ -2047,7 +2026,7 @@ func TestReplicaAccountsInheritPlatformFilter(t *testing.T) { "name": name, "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ "strategy": "from_external_on_first_use", @@ -2085,7 +2064,7 @@ func TestReplicaAccountsInheritPlatformFilter(t *testing.T) { "name": "first", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "platform_filter": testPlatformFilter, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ @@ -2107,6 +2086,8 @@ func TestReplicaAccountsInheritPlatformFilter(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, "platform_filter": []assert.JSONObject{{ "os": "linux", "architecture": "amd64", @@ -2123,7 +2104,7 @@ func TestReplicaAccountsInheritPlatformFilter(t *testing.T) { "name": "second", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": assert.JSONObject{}, + "metadata": nil, "platform_filter": testPlatformFilter, "rbac_policies": []assert.JSONObject{}, "replication": assert.JSONObject{ diff --git a/internal/api/keppel/fixtures/delete-account-000.sql b/internal/api/keppel/fixtures/delete-account-000.sql index ed6a1c6fb..6adf13bb3 100644 --- a/internal/api/keppel/fixtures/delete-account-000.sql +++ b/internal/api/keppel/fixtures/delete-account-000.sql @@ -1,6 +1,6 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, in_maintenance) VALUES ('test1', 'tenant1', 200, TRUE); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test2', 'tenant2', TRUE); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test3', 'tenant3', TRUE); +INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at) VALUES ('test1', 'tenant1', 200); +INSERT INTO accounts (name, auth_tenant_id) VALUES ('test2', 'tenant2'); +INSERT INTO accounts (name, auth_tenant_id) VALUES ('test3', 'tenant3'); INSERT INTO blob_mounts (blob_id, repo_id) VALUES (1, 1); INSERT INTO blob_mounts (blob_id, repo_id) VALUES (2, 1); diff --git a/internal/api/keppel/fixtures/delete-account-001.sql b/internal/api/keppel/fixtures/delete-account-001.sql index f42d4bb3e..d88c04acc 100644 --- a/internal/api/keppel/fixtures/delete-account-001.sql +++ b/internal/api/keppel/fixtures/delete-account-001.sql @@ -1,6 +1,6 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, in_maintenance, is_deleting, next_deletion_attempt_at) VALUES ('test1', 'tenant1', 200, TRUE, TRUE, 0); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test2', 'tenant2', TRUE); -INSERT INTO accounts (name, auth_tenant_id, in_maintenance) VALUES ('test3', 'tenant3', TRUE); +INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, is_deleting, next_deletion_attempt_at) VALUES ('test1', 'tenant1', 200, TRUE, 0); +INSERT INTO accounts (name, auth_tenant_id) VALUES ('test2', 'tenant2'); +INSERT INTO accounts (name, auth_tenant_id) VALUES ('test3', 'tenant3'); INSERT INTO blob_mounts (blob_id, repo_id) VALUES (1, 1); INSERT INTO blob_mounts (blob_id, repo_id) VALUES (2, 1); diff --git a/internal/api/registry/blobs_test.go b/internal/api/registry/blobs_test.go index 8184a2193..f81d2d5a7 100644 --- a/internal/api/registry/blobs_test.go +++ b/internal/api/registry/blobs_test.go @@ -57,7 +57,7 @@ func TestBlobMonolithicUpload(t *testing.T) { }.Check(t, h) // test failure cases: account is in maintenance - testWithAccountInMaintenance(t, s.DB, "test1", func() { + testWithAccountIsDeleting(t, s.DB, "test1", func() { assert.HTTPRequest{ Method: "POST", Path: "/v2/test1/foo/blobs/uploads/?digest=" + blob.Digest.String(), @@ -69,9 +69,12 @@ func TestBlobMonolithicUpload(t *testing.T) { Body: assert.ByteData(blob.Contents), ExpectStatus: http.StatusMethodNotAllowed, ExpectHeader: test.VersionHeader, - ExpectBody: test.ErrorCodeWithMessage{ - Code: keppel.ErrUnsupported, - Message: "account is in maintenance", + ExpectBody: assert.JSONObject{ + "errors": []assert.JSONObject{{ + "code": "UNSUPPORTED", + "message": "account is being deleted", + "detail": nil, + }}, }, }.Check(t, h) }) @@ -269,7 +272,7 @@ func TestBlobStreamedAndChunkedUpload(t *testing.T) { }.Check(t, h) // test failure cases during POST: account is in maintenance - testWithAccountInMaintenance(t, s.DB, "test1", func() { + testWithAccountIsDeleting(t, s.DB, "test1", func() { assert.HTTPRequest{ Method: "POST", Path: "/v2/test1/foo/blobs/uploads/", @@ -281,9 +284,12 @@ func TestBlobStreamedAndChunkedUpload(t *testing.T) { Body: assert.ByteData(blob.Contents), ExpectStatus: http.StatusMethodNotAllowed, ExpectHeader: test.VersionHeader, - ExpectBody: test.ErrorCodeWithMessage{ - Code: keppel.ErrUnsupported, - Message: "account is in maintenance", + ExpectBody: assert.JSONObject{ + "errors": []assert.JSONObject{{ + "code": "UNSUPPORTED", + "message": "account is being deleted", + "detail": nil, + }}, }, }.Check(t, h) }) diff --git a/internal/api/registry/manifests.go b/internal/api/registry/manifests.go index 29af485f8..ae46d8c1e 100644 --- a/internal/api/registry/manifests.go +++ b/internal/api/registry/manifests.go @@ -71,7 +71,7 @@ func (a *API) handleGetOrHeadManifest(w http.ResponseWriter, r *http.Request) { // see the true 404 to properly replicate the non-existence of the manifest // from this account into the replica account) userType := authz.UserIdentity.UserType() - if (account.UpstreamPeerHostName != "" || account.ExternalPeerURL != "") && !account.InMaintenance && (userType != keppel.PeerUser && userType != keppel.TrivyUser) { + if (account.UpstreamPeerHostName != "" || account.ExternalPeerURL != "") && !account.IsDeleting && (userType != keppel.PeerUser && userType != keppel.TrivyUser) { // when replicating from external, only authenticated users can trigger the replication if account.ExternalPeerURL != "" && userType != keppel.RegularUser { if !authz.ScopeSet.Contains(auth.Scope{ @@ -335,8 +335,8 @@ func (a *API) handlePutManifest(w http.ResponseWriter, r *http.Request) { } // forbid pushing during maintenance - if account.InMaintenance { - keppel.ErrUnsupported.With("account is in maintenance").WithStatus(http.StatusMethodNotAllowed).WriteAsRegistryV2ResponseTo(w, r) + if account.IsDeleting { + keppel.ErrUnsupported.With("account is being deleted").WithStatus(http.StatusMethodNotAllowed).WriteAsRegistryV2ResponseTo(w, r) return } diff --git a/internal/api/registry/manifests_test.go b/internal/api/registry/manifests_test.go index 3c6631dce..24477c235 100644 --- a/internal/api/registry/manifests_test.go +++ b/internal/api/registry/manifests_test.go @@ -96,8 +96,8 @@ func TestImageManifestLifecycle(t *testing.T) { ExpectBody: test.ErrorCode(keppel.ErrDenied), }.Check(t, h) - // PUT failure case: cannot push while account is in maintenance - testWithAccountInMaintenance(t, s.DB, "test1", func() { + // PUT failure case: cannot push while account is being deleted + testWithAccountIsDeleting(t, s.DB, "test1", func() { assert.HTTPRequest{ Method: "PUT", Path: "/v2/test1/foo/manifests/" + ref, @@ -109,7 +109,7 @@ func TestImageManifestLifecycle(t *testing.T) { ExpectStatus: http.StatusMethodNotAllowed, ExpectBody: test.ErrorCodeWithMessage{ Code: keppel.ErrUnsupported, - Message: "account is in maintenance", + Message: "account is being deleted", }, }.Check(t, h) }) diff --git a/internal/api/registry/replication_test.go b/internal/api/registry/replication_test.go index 0da0e91cb..ea761fc90 100644 --- a/internal/api/registry/replication_test.go +++ b/internal/api/registry/replication_test.go @@ -48,7 +48,7 @@ func TestReplicationSimpleImage(t *testing.T) { if firstPass { // replication will not take place while the account is in maintenance - testWithAccountInMaintenance(t, s2.DB, "test1", func() { + testWithAccountIsDeleting(t, s2.DB, "test1", func() { assert.HTTPRequest{ Method: "GET", Path: "/v2/test1/foo/manifests/" + image.Manifest.Digest.String(), @@ -60,7 +60,7 @@ func TestReplicationSimpleImage(t *testing.T) { }) } else { // if manifest is already present locally, we don't care about the maintenance mode - testWithAccountInMaintenance(t, s2.DB, "test1", func() { + testWithAccountIsDeleting(t, s2.DB, "test1", func() { expectManifestExists(t, h2, token, "test1/foo", image.Manifest, image.Manifest.Digest.String(), nil) }) } diff --git a/internal/api/registry/shared_test.go b/internal/api/registry/shared_test.go index d8670b87b..a0f229de3 100644 --- a/internal/api/registry/shared_test.go +++ b/internal/api/registry/shared_test.go @@ -270,13 +270,13 @@ func expectStorageEmpty(t *testing.T, sd *trivial.StorageDriver, db *keppel.DB) } //nolint:unparam -func testWithAccountInMaintenance(t *testing.T, db *keppel.DB, accountName models.AccountName, action func()) { - _, err := db.Exec("UPDATE accounts SET in_maintenance = TRUE WHERE name = $1", accountName) +func testWithAccountIsDeleting(t *testing.T, db *keppel.DB, accountName models.AccountName, action func()) { + _, err := db.Exec("UPDATE accounts SET is_deleting = TRUE WHERE name = $1", accountName) if err != nil { t.Fatal(err.Error()) } action() - _, err = db.Exec("UPDATE accounts SET in_maintenance = FALSE WHERE name = $1", accountName) + _, err = db.Exec("UPDATE accounts SET is_deleting = FALSE WHERE name = $1", accountName) if err != nil { t.Fatal(err.Error()) } diff --git a/internal/api/registry/uploads.go b/internal/api/registry/uploads.go index d91a79988..e73bac2b9 100644 --- a/internal/api/registry/uploads.go +++ b/internal/api/registry/uploads.go @@ -80,8 +80,8 @@ func (a *API) handleStartBlobUpload(w http.ResponseWriter, r *http.Request) { } // forbid pushing during maintenance - if account.InMaintenance { - keppel.ErrUnsupported.With("account is in maintenance").WithStatus(http.StatusMethodNotAllowed).WriteAsRegistryV2ResponseTo(w, r) + if account.IsDeleting { + keppel.ErrUnsupported.With("account is being deleted").WithStatus(http.StatusMethodNotAllowed).WriteAsRegistryV2ResponseTo(w, r) return } diff --git a/internal/drivers/basic/account_management_test.go b/internal/drivers/basic/account_management_test.go index 3cf16f3a1..91d9be58e 100644 --- a/internal/drivers/basic/account_management_test.go +++ b/internal/drivers/basic/account_management_test.go @@ -63,7 +63,6 @@ func TestConfigureAccount(t *testing.T) { RepositoryRx: ".*", }, }, - Metadata: map[string]string(nil), RBACPolicies: []keppel.RBACPolicy{ { Permissions: []keppel.RBACPermission{"anonymous_pull"}, diff --git a/internal/keppel/account.go b/internal/keppel/account.go index 0f817a885..179890e8a 100644 --- a/internal/keppel/account.go +++ b/internal/keppel/account.go @@ -19,9 +19,6 @@ package keppel import ( - "encoding/json" - "fmt" - "github.com/sapcc/keppel/internal/models" ) @@ -30,12 +27,15 @@ type Account struct { Name models.AccountName `json:"name"` AuthTenantID string `json:"auth_tenant_id"` GCPolicies []GCPolicy `json:"gc_policies,omitempty"` - InMaintenance bool `json:"in_maintenance"` - Metadata map[string]string `json:"metadata"` RBACPolicies []RBACPolicy `json:"rbac_policies"` ReplicationPolicy *ReplicationPolicy `json:"replication,omitempty"` + State string `json:"state,omitempty"` ValidationPolicy *ValidationPolicy `json:"validation,omitempty"` PlatformFilter models.PlatformFilter `json:"platform_filter,omitempty"` + + // TODO: deprecated, and remove + InMaintenance bool `json:"in_maintenance"` + Metadata *map[string]string `json:"metadata"` } // RenderAccount converts an account model from the DB into the API representation. @@ -52,21 +52,16 @@ func RenderAccount(dbAccount models.Account) (Account, error) { // do not render "null" in this field rbacPolicies = []RBACPolicy{} } - - metadata := make(map[string]string) - if dbAccount.MetadataJSON != "" { - err := json.Unmarshal([]byte(dbAccount.MetadataJSON), &metadata) - if err != nil { - return Account{}, fmt.Errorf("malformed metadata JSON: %q", dbAccount.MetadataJSON) - } + var state string + if dbAccount.IsDeleting { + state = "deleting" } return Account{ Name: dbAccount.Name, AuthTenantID: dbAccount.AuthTenantID, GCPolicies: gcPolicies, - InMaintenance: dbAccount.InMaintenance, - Metadata: metadata, + State: state, RBACPolicies: rbacPolicies, ReplicationPolicy: RenderReplicationPolicy(dbAccount), ValidationPolicy: RenderValidationPolicy(dbAccount.Reduced()), diff --git a/internal/keppel/database.go b/internal/keppel/database.go index 59c59b524..4269641bf 100644 --- a/internal/keppel/database.go +++ b/internal/keppel/database.go @@ -307,11 +307,24 @@ var sqlMigrations = map[string]string{ "043_add_accounts_is_deleting.up.sql": ` ALTER TABLE accounts ADD COLUMN is_deleting BOOLEAN NOT NULL DEFAULT FALSE, - ADD COLUMN next_deletion_attempt_at TIMESTAMPTZ DEFAULT NULL; + ADD COLUMN next_deletion_attempt_at TIMESTAMPTZ DEFAULT NULL; + + UPDATE accounts SET is_deleting = TRUE WHERE in_maintenance; + + ALTER TABLE accounts + DROP COLUMN in_maintenance, + DROP COLUMN metadata_json; `, "043_add_accounts_is_deleting.down.sql": ` ALTER TABLE accounts - DROP COLUMN is_deleting, next_deletion_attempt_at; + ADD COLUMN in_maintenance BOOLEAN NOT NULL DEFAULT FALSE, + ADD COLUMN metadata_json TEXT NOT NULL DEFAULT ''; + + UPDATE accounts SET in_maintenance = TRUE WHERE is_deleting; + + ALTER TABLE accounts + DROP COLUMN is_deleting, + DROP COLUMN next_deletion_attempt_at; `, } diff --git a/internal/keppel/database_helpers.go b/internal/keppel/database_helpers.go index 4eb3030af..eef7016b5 100644 --- a/internal/keppel/database_helpers.go +++ b/internal/keppel/database_helpers.go @@ -44,7 +44,7 @@ func FindAccount(db gorp.SqlExecutor, name models.AccountName) (*models.Account, var reducedAccountGetByNameQuery = sqlext.SimplifyWhitespace(` SELECT auth_tenant_id, upstream_peer_hostname, external_peer_url, external_peer_username, external_peer_password, - platform_filter, required_labels, in_maintenance + platform_filter, required_labels, is_deleting FROM accounts WHERE name = $1 `) @@ -56,7 +56,7 @@ func FindReducedAccount(db gorp.SqlExecutor, name models.AccountName) (*models.R err := db.QueryRow(reducedAccountGetByNameQuery, name).Scan( &a.AuthTenantID, &a.UpstreamPeerHostName, &a.ExternalPeerURL, &a.ExternalPeerUserName, &a.ExternalPeerPassword, - &a.PlatformFilter, &a.RequiredLabels, &a.InMaintenance, + &a.PlatformFilter, &a.RequiredLabels, &a.IsDeleting, ) if errors.Is(err, sql.ErrNoRows) { return nil, nil diff --git a/internal/models/account.go b/internal/models/account.go index 266bf5375..577c4869d 100644 --- a/internal/models/account.go +++ b/internal/models/account.go @@ -46,15 +46,11 @@ type Account struct { // RequiredLabels is a comma-separated list of labels that must be present on // all image manifests in this account. RequiredLabels string `db:"required_labels"` - // InMaintenance indicates whether the account is in maintenance mode (as defined in the API spec). - InMaintenance bool `db:"in_maintenance"` // IsDeleting indicates whether the account is currently being deleted. IsDeleting bool `db:"is_deleting"` // IsManaged indicates if the account was created by AccountManagementDriver IsManaged bool `db:"is_managed"` - // MetadataJSON contains a JSON string of a map[string]string, or the empty string. - MetadataJSON string `db:"metadata_json"` // RBACPoliciesJSON contains a JSON string of []keppel.RBACPolicy, or the empty string. RBACPoliciesJSON string `db:"rbac_policies_json"` // GCPoliciesJSON contains a JSON string of []keppel.GCPolicy, or the empty string. @@ -80,7 +76,7 @@ func (a Account) Reduced() ReducedAccount { ExternalPeerPassword: a.ExternalPeerPassword, PlatformFilter: a.PlatformFilter, RequiredLabels: a.RequiredLabels, - InMaintenance: a.InMaintenance, + IsDeleting: a.IsDeleting, } } @@ -100,7 +96,7 @@ type ReducedAccount struct { // validation policy, status RequiredLabels string - InMaintenance bool + IsDeleting bool // NOTE: When adding or removing fields, always adjust Account.Reduced() and keppel.FindReducedAccount() too! } diff --git a/internal/processor/accounts.go b/internal/processor/accounts.go index 19f88fdb8..d0936934e 100644 --- a/internal/processor/accounts.go +++ b/internal/processor/accounts.go @@ -104,7 +104,7 @@ func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Ac } // validate and update fields as requested - targetAccount.InMaintenance = account.InMaintenance + targetAccount.IsDeleting = account.State == "deleting" // validate GC policies if len(account.GCPolicies) == 0 { @@ -120,14 +120,6 @@ func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Ac targetAccount.GCPoliciesJSON = string(buf) } - // serialize metadata - if len(account.Metadata) == 0 { - targetAccount.MetadataJSON = "" - } else { - buf, _ := json.Marshal(account.Metadata) - targetAccount.MetadataJSON = string(buf) - } - // validate replication policy (for OnFirstUseStrategy, the peer hostname is // checked for correctness down below when validating the platform filter) var originalStrategy keppel.ReplicationStrategy @@ -304,7 +296,7 @@ func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Ac // audit log is necessary for all changes except to InMaintenance if userInfo != nil { - originalAccount.InMaintenance = targetAccount.InMaintenance + originalAccount.IsDeleting = targetAccount.IsDeleting if !reflect.DeepEqual(*originalAccount, targetAccount) { p.auditor.Record(audittools.EventParameters{ Time: p.timeNow(), diff --git a/internal/tasks/manifests.go b/internal/tasks/manifests.go index 68bb2ec86..8ef54b331 100644 --- a/internal/tasks/manifests.go +++ b/internal/tasks/manifests.go @@ -180,8 +180,8 @@ func (j *Janitor) syncManifestsInReplicaRepo(ctx context.Context, repo models.Re return fmt.Errorf("cannot find account for repo %s: %w", repo.FullName(), err) } - // do not perform manifest sync while account is in maintenance (maintenance mode blocks all kinds of replication) - if !account.InMaintenance && !account.IsDeleting { + // do not perform manifest sync while account is in deletion (deletion mode blocks all kinds of replication) + if !account.IsDeleting { syncPayload, err := j.getReplicaSyncPayload(ctx, *account, repo) if err != nil { return err @@ -640,7 +640,7 @@ func (j *Janitor) doSecurityCheck(ctx context.Context, securityInfo *models.Triv // skip validation while account is in maintenance (maintenance mode blocks // all kinds of activity on an account's contents) - if account.InMaintenance || account.IsDeleting { + if account.IsDeleting { securityInfo.NextCheckAt = j.timeNow().Add(j.addJitter(1 * time.Hour)) return nil } diff --git a/internal/tasks/manifests_test.go b/internal/tasks/manifests_test.go index 21af253ff..fccea4995 100644 --- a/internal/tasks/manifests_test.go +++ b/internal/tasks/manifests_test.go @@ -337,10 +337,10 @@ func TestManifestSyncJob(t *testing.T) { // ManifestSyncJob on the replica side should not do anything while // the account is in maintenance; only the timestamp is updated to make sure // that the job loop progresses to the next repo - mustExec(t, s2.DB, `UPDATE accounts SET in_maintenance = TRUE`) + mustExec(t, s2.DB, `UPDATE accounts SET is_deleting = TRUE`) expectSuccess(t, syncManifestsJob2.ProcessOne(s2.Ctx)) tr.DBChanges().AssertEqualf(` - UPDATE accounts SET in_maintenance = TRUE WHERE name = 'test1'; + UPDATE accounts SET is_deleting = TRUE WHERE name = 'test1'; UPDATE repos SET next_manifest_sync_at = %d WHERE id = 1 AND account_name = 'test1' AND name = 'foo'; `, s1.Clock.Now().Add(1*time.Hour).Unix(), @@ -348,9 +348,9 @@ func TestManifestSyncJob(t *testing.T) { expectError(t, sql.ErrNoRows.Error(), syncManifestsJob2.ProcessOne(s2.Ctx)) tr.DBChanges().AssertEmpty() - // end maintenance - mustExec(t, s2.DB, `UPDATE accounts SET in_maintenance = FALSE`) - tr.DBChanges().AssertEqual(`UPDATE accounts SET in_maintenance = FALSE WHERE name = 'test1';`) + // end deletion + mustExec(t, s2.DB, `UPDATE accounts SET is_deleting = FALSE`) + tr.DBChanges().AssertEqual(`UPDATE accounts SET is_deleting = FALSE WHERE name = 'test1';`) // test that replication from external uses the inbound cache if strategy == "from_external_on_first_use" { From c3a5e0c10e196886a0379c4ebbae0a23004245b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Tue, 29 Oct 2024 18:32:34 +0100 Subject: [PATCH 09/19] Prevent any API requests on accounts that are in deleting --- internal/api/keppel/accounts.go | 7 ++++++- internal/api/keppel/api.go | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/api/keppel/accounts.go b/internal/api/keppel/accounts.go index 6ad21903b..74d70966a 100644 --- a/internal/api/keppel/accounts.go +++ b/internal/api/keppel/accounts.go @@ -112,7 +112,12 @@ func (a *API) handlePutAccount(w http.ResponseWriter, r *http.Request) { http.Error(w, `malformed attribute "account.name" in request body is not allowed here`, http.StatusUnprocessableEntity) return } - // ... transfer it here into the struct, to make the below code simpler + // ... or state ... + if req.Account.State != "" { + http.Error(w, `malformed attribute "account.state" in request body is not allowed here`, http.StatusUnprocessableEntity) + return + } + // ... and transfer the name here into the struct, to make the below code simpler req.Account.Name = models.AccountName(mux.Vars(r)["account"]) // check permission to create account diff --git a/internal/api/keppel/api.go b/internal/api/keppel/api.go index cf27f8bfc..1b7ca4b1b 100644 --- a/internal/api/keppel/api.go +++ b/internal/api/keppel/api.go @@ -184,6 +184,10 @@ func (a *API) findAccountFromRequest(w http.ResponseWriter, r *http.Request, _ * http.Error(w, "account not found", http.StatusNotFound) return nil } + if account.IsDeleting && r.Method == http.MethodGet { + http.Error(w, "account is being deleted", http.StatusConflict) + return nil + } return account } From b08d061b0c8df006686fdb2d5c8b1778fd996476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 11 Nov 2024 17:00:00 +0100 Subject: [PATCH 10/19] Drop kinda duplicated audit event --- internal/tasks/account_deletion.go | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/internal/tasks/account_deletion.go b/internal/tasks/account_deletion.go index d8adfd6f8..c1495517d 100644 --- a/internal/tasks/account_deletion.go +++ b/internal/tasks/account_deletion.go @@ -23,20 +23,16 @@ import ( "database/sql" "errors" "fmt" - "net/http" "time" "github.com/opencontainers/go-digest" "github.com/prometheus/client_golang/prometheus" - "github.com/sapcc/go-api-declarations/cadf" - "github.com/sapcc/go-bits/audittools" "github.com/sapcc/go-bits/jobloop" "github.com/sapcc/go-bits/logg" "github.com/sapcc/go-bits/sqlext" "github.com/sapcc/keppel/internal/keppel" "github.com/sapcc/keppel/internal/models" - "github.com/sapcc/keppel/internal/processor" ) // EnforceManagedAccounts is a job. Each task creates newly discovered accounts from the driver. @@ -168,23 +164,7 @@ func (j *Janitor) deleteMarkedAccount(ctx context.Context, accountName models.Ac return fmt.Errorf("while cleaning up name claim for account: %w", err) } - err = tx.Commit() - if err != nil { - return err - } - - if userInfo := actx.UserIdentity.UserInfo(); userInfo != nil { - j.auditor.Record(audittools.EventParameters{ - Time: j.timeNow(), - Request: actx.Request, - User: userInfo, - ReasonCode: http.StatusOK, - Action: cadf.DeleteAction, - Target: processor.AuditAccount{Account: *accountModel}, - }) - } - - return nil + return tx.Commit() } var ( From 43070b962fb2fe62be39195fc973b47943e3a577 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 11 Nov 2024 17:44:58 +0100 Subject: [PATCH 11/19] Don't add new occurences of metadata, in_maintenance, reject them in the API --- internal/api/keppel/accounts.go | 10 +++++++ internal/api/keppel/accounts_test.go | 42 ++++++++++++++++++++-------- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/internal/api/keppel/accounts.go b/internal/api/keppel/accounts.go index 74d70966a..414b57e49 100644 --- a/internal/api/keppel/accounts.go +++ b/internal/api/keppel/accounts.go @@ -117,6 +117,16 @@ func (a *API) handlePutAccount(w http.ResponseWriter, r *http.Request) { http.Error(w, `malformed attribute "account.state" in request body is not allowed here`, http.StatusUnprocessableEntity) return } + // ... or in_maintenance ... + if req.Account.InMaintenance { + http.Error(w, `malformed attribute "account.in_maintenance" in request body is not allowed here`, http.StatusUnprocessableEntity) + return + } + // ... or metadata ... + if req.Account.Metadata != nil && len(*req.Account.Metadata) > 0 { + http.Error(w, `malformed attribute "account.metadata" in request body is not allowed here`, http.StatusUnprocessableEntity) + return + } // ... and transfer the name here into the struct, to make the below code simpler req.Account.Name = models.AccountName(mux.Vars(r)["account"]) diff --git a/internal/api/keppel/accounts_test.go b/internal/api/keppel/accounts_test.go index 902f639ea..39807eeb4 100644 --- a/internal/api/keppel/accounts_test.go +++ b/internal/api/keppel/accounts_test.go @@ -1070,8 +1070,6 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "library/.+", "match_username": "foo", @@ -1089,8 +1087,6 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "library/.+", "permissions": []string{"anonymous_first_pull"}, @@ -1108,8 +1104,6 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "*/library", "permissions": []string{"anonymous_pull"}, @@ -1126,8 +1120,6 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, "rbac_policies": []assert.JSONObject{{ "match_repository": "library/.+", "match_username": "[a-z]++@tenant2", @@ -1147,8 +1139,6 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, "platform_filter": []assert.JSONObject{{ "os": "linux", "architecture": "amd64", @@ -1167,8 +1157,6 @@ func TestPutAccountErrorCases(t *testing.T) { Body: assert.JSONObject{ "account": assert.JSONObject{ "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, "platform_filter": []assert.JSONObject{{ "os": "linux", "architecture": "amd64", @@ -1205,6 +1193,36 @@ func TestPutAccountErrorCases(t *testing.T) { ExpectBody: assert.StringData("no permission for keppel_account:unknown:change\n"), }.Check(t, h) + assert.HTTPRequest{ + Method: "PUT", + Path: "/keppel/v1/accounts/first", + Header: map[string]string{"X-Test-Perms": "change:tenant1"}, + Body: assert.JSONObject{ + "account": assert.JSONObject{ + "auth_tenant_id": "tenant1", + "in_maintenance": true, + }, + }, + ExpectStatus: http.StatusUnprocessableEntity, + ExpectBody: assert.StringData("malformed attribute \"account.in_maintenance\" in request body is not allowed here\n"), + }.Check(t, h) + + assert.HTTPRequest{ + Method: "PUT", + Path: "/keppel/v1/accounts/first", + Header: map[string]string{"X-Test-Perms": "change:tenant1"}, + Body: assert.JSONObject{ + "account": assert.JSONObject{ + "auth_tenant_id": "tenant1", + "metadata": assert.JSONObject{ + "foo": "bar", + }, + }, + }, + ExpectStatus: http.StatusUnprocessableEntity, + ExpectBody: assert.StringData("malformed attribute \"account.metadata\" in request body is not allowed here\n"), + }.Check(t, h) + // test protection for managed accounts mustExec(t, s.DB, "UPDATE accounts SET is_managed = TRUE WHERE name = $1", "first") assert.HTTPRequest{ From 65968ff71f943d71590b602c0840c79975fd0463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 11 Nov 2024 18:26:33 +0100 Subject: [PATCH 12/19] Handle nested manifests --- internal/tasks/account_deletion.go | 81 +++++++++++++++++++----------- 1 file changed, 52 insertions(+), 29 deletions(-) diff --git a/internal/tasks/account_deletion.go b/internal/tasks/account_deletion.go index c1495517d..b4c8bdb3b 100644 --- a/internal/tasks/account_deletion.go +++ b/internal/tasks/account_deletion.go @@ -79,35 +79,51 @@ func (j *Janitor) deleteMarkedAccount(ctx context.Context, accountName models.Ac } // can only delete account when all manifests from it are deleted - err = sqlext.ForeachRow(j.db, deleteAccountFindManifestsQuery, []any{accountModel.Name}, - func(rows *sql.Rows) error { - var m deleteAccountRemainingManifest - err := rows.Scan(&m.RepositoryName, &m.Digest) - if err != nil { - return err - } - - parsedDigest, err := digest.Parse(m.Digest) - if err != nil { - return fmt.Errorf("while deleting manifest %q in repository %q: could not parse digest: %w", - m.Digest, m.RepositoryName, err) - } - repo, err := keppel.FindRepository(j.db, m.RepositoryName, accountModel.Name) - if err != nil { - return fmt.Errorf("while deleting manifest %q in repository %q: could not find repository in DB: %w", - m.Digest, m.RepositoryName, err) - } - err = j.processor().DeleteManifest(ctx, accountModel.Reduced(), *repo, parsedDigest, actx) - if err != nil { - return fmt.Errorf("while deleting manifest %q in repository %q: %w", - m.Digest, m.RepositoryName, err) - } - - return nil - }, - ) - if err != nil { - return err + tries := 0 + for { + if tries >= 3 { + return fmt.Errorf("could not delete all manifests references by %s", accountModel.Name) + } + tries++ + + err = sqlext.ForeachRow(j.db, deleteAccountFindManifestsQuery, []any{accountModel.Name}, + func(rows *sql.Rows) error { + var m deleteAccountRemainingManifest + err := rows.Scan(&m.RepositoryName, &m.Digest) + if err != nil { + return err + } + + parsedDigest, err := digest.Parse(m.Digest) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: could not parse digest: %w", + m.Digest, m.RepositoryName, err) + } + repo, err := keppel.FindRepository(j.db, m.RepositoryName, accountModel.Name) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: could not find repository in DB: %w", + m.Digest, m.RepositoryName, err) + } + err = j.processor().DeleteManifest(ctx, accountModel.Reduced(), *repo, parsedDigest, actx) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: %w", + m.Digest, m.RepositoryName, err) + } + + return nil + }, + ) + if err != nil { + return err + } + + manifestCount, err := j.db.SelectInt(deleteAccountCountManifestsQuery, accountModel.Name) + if err != nil { + return err + } + if manifestCount == 0 { + break + } } // delete all repos (and therefore, all blob mounts), so that blob sweeping can immediately take place @@ -176,6 +192,13 @@ var ( LEFT OUTER JOIN manifest_manifest_refs mmr ON mmr.repo_id = r.id AND m.digest = mmr.child_digest WHERE a.name = $1 AND parent_digest IS NULL `) + deleteAccountCountManifestsQuery = sqlext.SimplifyWhitespace(` + SELECT COUNT(m.digest) + FROM manifests m + JOIN repos r ON m.repo_id = r.id + JOIN accounts a ON a.name = r.account_name + WHERE a.name = $1 + `) deleteAccountReposQuery = `DELETE FROM repos WHERE account_name = $1` deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` From 235985c9a688eeff895f9dce3c4f091dd216ca06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sandro=20J=C3=A4ckel?= Date: Mon, 11 Nov 2024 19:12:04 +0100 Subject: [PATCH 13/19] Test deletion with imageList --- internal/tasks/account_management_test.go | 69 +++++++++++++++++++---- 1 file changed, 59 insertions(+), 10 deletions(-) diff --git a/internal/tasks/account_management_test.go b/internal/tasks/account_management_test.go index a60201765..438fb9fe4 100644 --- a/internal/tasks/account_management_test.go +++ b/internal/tasks/account_management_test.go @@ -144,6 +144,15 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { test.GenerateExampleLayer(2), ) image.MustUpload(t, s, repo, "latest") + + // also setup some manifest-manifest refs to test with + images := make([]test.Image, 2) + for idx := range images { + images[idx] = test.GenerateImage(test.GenerateExampleLayer(int64(idx + 2))) + images[idx].MustUpload(t, s, repo, "") + } + imageList := test.GenerateImageList(images[0], images[1]) + imageList.MustUpload(t, s, repo, "") tr.DBChanges().Ignore() // remove the managed account: this will set is_deleting, but nothing more @@ -168,24 +177,51 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { DELETE FROM blob_mounts WHERE blob_id = 1 AND repo_id = 2; DELETE FROM blob_mounts WHERE blob_id = 2 AND repo_id = 2; DELETE FROM blob_mounts WHERE blob_id = 3 AND repo_id = 2; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 1 AND account_name = 'abcde' AND digest = '%[4]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 2 AND account_name = 'abcde' AND digest = '%[5]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 3 AND account_name = 'abcde' AND digest = '%[6]s'; - DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[3]s' AND blob_id = 1; - DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[3]s' AND blob_id = 2; - DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[3]s' AND blob_id = 3; - DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[3]s'; - DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[3]s'; + DELETE FROM blob_mounts WHERE blob_id = 4 AND repo_id = 2; + DELETE FROM blob_mounts WHERE blob_id = 5 AND repo_id = 2; + DELETE FROM blob_mounts WHERE blob_id = 6 AND repo_id = 2; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 1 AND account_name = 'abcde' AND digest = '%[3]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 2 AND account_name = 'abcde' AND digest = '%[4]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 3 AND account_name = 'abcde' AND digest = '%[5]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 4 AND account_name = 'abcde' AND digest = '%[6]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 5 AND account_name = 'abcde' AND digest = '%[7]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 6 AND account_name = 'abcde' AND digest = '%[8]s'; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[9]s' AND blob_id = 2; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[9]s' AND blob_id = 4; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[10]s' AND blob_id = 5; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[10]s' AND blob_id = 6; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[11]s' AND blob_id = 1; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[11]s' AND blob_id = 2; + DELETE FROM manifest_blob_refs WHERE repo_id = 2 AND digest = '%[11]s' AND blob_id = 3; + DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[9]s'; + DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[10]s'; + DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[11]s'; + DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[12]s'; + DELETE FROM manifest_manifest_refs WHERE repo_id = 2 AND parent_digest = '%[12]s' AND child_digest = '%[9]s'; + DELETE FROM manifest_manifest_refs WHERE repo_id = 2 AND parent_digest = '%[12]s' AND child_digest = '%[10]s'; + DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[9]s'; + DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[10]s'; + DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[11]s'; + DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[12]s'; DELETE FROM repos WHERE id = 2 AND account_name = 'abcde' AND name = 'foo'; DELETE FROM tags WHERE repo_id = 2 AND name = 'latest'; - DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[3]s'; + DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[9]s'; + DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[10]s'; + DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[11]s'; + DELETE FROM trivy_security_info WHERE repo_id = 2 AND digest = '%[12]s'; `, s.Clock.Now().Unix(), s.Clock.Now().Add(1*time.Minute).Unix(), - image.Manifest.Digest.String(), image.Layers[0].Digest.String(), image.Layers[1].Digest.String(), image.Config.Digest.String(), + images[0].Config.Digest.String(), + images[1].Layers[0].Digest.String(), + images[1].Config.Digest.String(), + images[0].Manifest.Digest.String(), + images[1].Manifest.Digest.String(), + image.Manifest.Digest.String(), + imageList.Manifest.Digest.String(), ) // TODO: fix the can_be_deleted_at reset @@ -196,12 +232,19 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 1 AND account_name = 'abcde' AND digest = '%[3]s'; UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 2 AND account_name = 'abcde' AND digest = '%[4]s'; UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 3 AND account_name = 'abcde' AND digest = '%[5]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 4 AND account_name = 'abcde' AND digest = '%[6]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 5 AND account_name = 'abcde' AND digest = '%[7]s'; + UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 6 AND account_name = 'abcde' AND digest = '%[8]s'; `, s.Clock.Now().Unix(), s.Clock.Now().Add(1*time.Minute).Unix(), image.Layers[0].Digest.String(), image.Layers[1].Digest.String(), image.Config.Digest.String(), + images[0].Config.Digest.String(), + images[1].Layers[0].Digest.String(), + images[1].Config.Digest.String(), + images[0].Config.Digest.String(), ) s.Clock.StepBy(1 * time.Minute) @@ -214,10 +257,16 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { DELETE FROM blobs WHERE id = 1 AND account_name = 'abcde' AND digest = '%[2]s'; DELETE FROM blobs WHERE id = 2 AND account_name = 'abcde' AND digest = '%[3]s'; DELETE FROM blobs WHERE id = 3 AND account_name = 'abcde' AND digest = '%[4]s'; + DELETE FROM blobs WHERE id = 4 AND account_name = 'abcde' AND digest = '%[5]s'; + DELETE FROM blobs WHERE id = 5 AND account_name = 'abcde' AND digest = '%[6]s'; + DELETE FROM blobs WHERE id = 6 AND account_name = 'abcde' AND digest = '%[7]s'; `, s.Clock.Now().Add(60*time.Minute).Unix(), image.Layers[0].Digest.String(), image.Layers[1].Digest.String(), image.Config.Digest.String(), + images[0].Config.Digest.String(), + images[1].Layers[0].Digest.String(), + images[1].Config.Digest.String(), ) } From 1f71c6ed52d135edbbc46eb13e303cd2bb8e60fc Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Tue, 12 Nov 2024 13:42:17 +0100 Subject: [PATCH 14/19] internal/api/keppel: remove leftovers of obsolete tests --- internal/api/keppel/accounts_test.go | 220 ++---------------- .../keppel/fixtures/delete-account-000.sql | 28 --- .../keppel/fixtures/delete-account-001.sql | 28 --- internal/api/keppel/repos_test.go | 7 - 4 files changed, 23 insertions(+), 260 deletions(-) delete mode 100644 internal/api/keppel/fixtures/delete-account-000.sql delete mode 100644 internal/api/keppel/fixtures/delete-account-001.sql diff --git a/internal/api/keppel/accounts_test.go b/internal/api/keppel/accounts_test.go index 39807eeb4..edcc0b887 100644 --- a/internal/api/keppel/accounts_test.go +++ b/internal/api/keppel/accounts_test.go @@ -20,14 +20,12 @@ package keppelv1_test import ( - "bytes" "encoding/base64" "encoding/json" "fmt" "net/http" "strings" "testing" - "time" "github.com/sapcc/go-api-declarations/cadf" "github.com/sapcc/go-bits/assert" @@ -295,67 +293,7 @@ func TestAccountsAPI(t *testing.T) { INSERT INTO accounts (name, auth_tenant_id, gc_policies_json, rbac_policies_json) VALUES ('second', 'tenant1', '[{"match_repository":".*/database","except_repository":"archive/.*","time_constraint":{"on":"pushed_at","newer_than":{"value":10,"unit":"d"}},"action":"protect"},{"match_repository":".*","only_untagged":true,"action":"delete"}]', '[{"match_repository":"library/.*","permissions":["anonymous_pull"]},{"match_repository":"library/alpine","match_username":".*@tenant2","permissions":["pull","push"]}]'); `) - account := assert.JSONObject{ - "auth_tenant_id": "tenant1", - "rbac_policies": rbacPoliciesJSON, - } - accountExpect := assert.JSONObject{ - "name": "second", - "in_maintenance": false, - "metadata": nil, - "auth_tenant_id": "tenant1", - "rbac_policies": rbacPoliciesJSON, - } - - assert.HTTPRequest{ - Method: "PUT", - Path: "/keppel/v1/accounts/second", - Header: map[string]string{"X-Test-Perms": "change:tenant1"}, - Body: assert.JSONObject{ - "account": account, - }, - ExpectStatus: http.StatusOK, - ExpectBody: assert.JSONObject{ - "account": accountExpect, - }, - }.Check(t, h) - - assert.HTTPRequest{ - Method: "GET", - Path: "/keppel/v1/accounts/second", - Header: map[string]string{"X-Test-Perms": "view:tenant1"}, - ExpectStatus: http.StatusOK, - ExpectBody: assert.JSONObject{ - "account": assert.JSONObject{ - "name": "second", - "auth_tenant_id": "tenant1", - "in_maintenance": false, - "metadata": nil, - "rbac_policies": rbacPoliciesJSON, - }, - }, - }.Check(t, h) - - s.Auditor.ExpectEvents(t, - cadf.Event{ - RequestPath: "/keppel/v1/accounts/second", - Action: cadf.UpdateAction, - Outcome: "success", - Reason: test.CADFReasonOK, - Target: cadf.Resource{ - TypeURI: "docker-registry/account", - ID: "second", - ProjectID: "tenant1", - Attachments: []cadf.Attachment{{ - Name: "rbac-policies", - TypeURI: "mime:application/json", - Content: toJSONVia[[]keppel.RBACPolicy](rbacPoliciesJSON), - }}, - }, - }, - ) - - // check editing of metadata and RBAC policies + // check editing of RBAC policies newRBACPoliciesJSON := []assert.JSONObject{ // rbacPoliciesJSON[0] is deleted // rbacPoliciesJSON[1] is updated as follows: @@ -1799,126 +1737,15 @@ func TestGetPutAccountReplicationFromExternalOnFirstUse(t *testing.T) { }.Check(t, h) } -func uploadManifest(t *testing.T, s test.Setup, account *models.Account, repo *models.Repository, manifest test.Bytes, sizeBytes uint64) models.Manifest { - t.Helper() - - dbManifest := models.Manifest{ - RepositoryID: repo.ID, - Digest: manifest.Digest, - MediaType: manifest.MediaType, - SizeBytes: sizeBytes, - PushedAt: s.Clock.Now(), - NextValidationAt: s.Clock.Now().Add(models.ManifestValidationInterval), - } - mustDo(t, s.DB.Insert(&dbManifest)) - mustDo(t, s.DB.Insert(&models.TrivySecurityInfo{ - RepositoryID: repo.ID, - Digest: manifest.Digest, - NextCheckAt: time.Unix(0, 0), - VulnerabilityStatus: models.PendingVulnerabilityStatus, - })) - mustDo(t, s.DB.Insert(&models.ManifestContent{ - RepositoryID: repo.ID, - Digest: manifest.Digest.String(), - Content: manifest.Contents, - })) - mustDo(t, s.SD.WriteManifest(s.Ctx, account.Reduced(), repo.Name, manifest.Digest, manifest.Contents)) - return dbManifest -} - func TestDeleteAccount(t *testing.T) { - s := test.NewSetup(t, test.WithKeppelAPI) - h := s.Handler - - // setup test accounts and repositories - nextBlobSweepAt := time.Unix(200, 0) - accounts := []*models.Account{ - {Name: "test1", AuthTenantID: "tenant1", NextBlobSweepedAt: &nextBlobSweepAt, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, - {Name: "test2", AuthTenantID: "tenant2", GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, - {Name: "test3", AuthTenantID: "tenant3", GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, - } - for _, account := range accounts { - mustInsert(t, s.DB, account) - } - repos := []*models.Repository{ - {AccountName: "test1", Name: "foo/bar"}, - {AccountName: "test1", Name: "something-else"}, - } - for _, repo := range repos { - mustInsert(t, s.DB, repo) - } - - // upload a test image - image := test.GenerateImage( - test.GenerateExampleLayer(1), - test.GenerateExampleLayer(2), - ) - - sidGen := test.StorageIDGenerator{} - var blobs []models.Blob - for idx, testBlob := range append(image.Layers, image.Config) { - storageID := sidGen.Next() - blob := models.Blob{ - AccountName: accounts[0].Name, - Digest: testBlob.Digest, - SizeBytes: uint64(len(testBlob.Contents)), - StorageID: storageID, - PushedAt: time.Unix(int64(idx), 0), - NextValidationAt: time.Unix(int64(idx), 0).Add(models.BlobValidationInterval), - } - mustInsert(t, s.DB, &blob) - blobs = append(blobs, blob) - - err := s.SD.AppendToBlob(s.Ctx, accounts[0].Reduced(), storageID, 1, &blob.SizeBytes, bytes.NewReader(testBlob.Contents)) - if err != nil { - t.Fatal(err.Error()) - } - err = s.SD.FinalizeBlob(s.Ctx, accounts[0].Reduced(), storageID, 1) - if err != nil { - t.Fatal(err.Error()) - } - err = keppel.MountBlobIntoRepo(s.DB, blob, *repos[0]) - if err != nil { - t.Fatal(err.Error()) - } - } - - mustInsert(t, s.DB, &models.Manifest{ - RepositoryID: repos[0].ID, - Digest: image.Manifest.Digest, - MediaType: image.Manifest.MediaType, - SizeBytes: uint64(len(image.Manifest.Contents)), - PushedAt: time.Unix(100, 0), - NextValidationAt: time.Unix(100, 0).Add(models.ManifestValidationInterval), - }) - mustInsert(t, s.DB, &models.TrivySecurityInfo{ - RepositoryID: repos[0].ID, - Digest: image.Manifest.Digest, - NextCheckAt: time.Unix(0, 0), - VulnerabilityStatus: models.PendingVulnerabilityStatus, - }) - err := s.SD.WriteManifest(s.Ctx, accounts[0].Reduced(), repos[0].Name, image.Manifest.Digest, image.Manifest.Contents) - if err != nil { - t.Fatal(err.Error()) - } - for _, blob := range blobs { - _, err := s.DB.Exec( - `INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES ($1, $2, $3)`, - repos[0].ID, image.Manifest.Digest.String(), blob.ID, - ) - if err != nil { - t.Fatal(err.Error()) - } - } - - imageList := test.GenerateImageList(image) - uploadManifest(t, s, accounts[0], repos[0], imageList.Manifest, imageList.SizeBytes()) - mustExec(t, s.DB, - `INSERT INTO manifest_manifest_refs (repo_id, parent_digest, child_digest) VALUES ($1, $2, $3)`, - repos[0].ID, imageList.Manifest.Digest.String(), image.Manifest.Digest.String(), + s := test.NewSetup(t, + test.WithKeppelAPI, + test.WithAccount(models.Account{Name: "test1", AuthTenantID: "tenant1", GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}), ) + h := s.Handler - easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-000.sql") + tr, tr0 := easypg.NewTracker(t, s.DB.DbMap.Db) + tr0.Ignore() // failure case: insufficient permissions (the "delete" permission refers to // manifests within the account, not the account itself) @@ -1929,21 +1756,7 @@ func TestDeleteAccount(t *testing.T) { ExpectStatus: http.StatusForbidden, }.Check(t, h) - // failure case: account not in maintenance - _, err = s.DB.Exec(`UPDATE accounts SET is_deleting = FALSE`) - if err != nil { - t.Fatal(err.Error()) - } - - // phase 1: DELETE on account should immediately mark it for deletion - assert.HTTPRequest{ - Method: "DELETE", - Path: "/keppel/v1/accounts/test1", - Header: map[string]string{"X-Test-Perms": "view:tenant1,change:tenant1"}, - ExpectStatus: http.StatusNoContent, - }.Check(t, h) - - // account is already set to be deleted, so nothing happens + // DELETE on account should immediately mark it for deletion assert.HTTPRequest{ Method: "DELETE", Path: "/keppel/v1/accounts/test1", @@ -1951,6 +1764,11 @@ func TestDeleteAccount(t *testing.T) { ExpectStatus: http.StatusNoContent, }.Check(t, h) + tr.DBChanges().AssertEqualf(` + UPDATE accounts SET is_deleting = TRUE, next_deletion_attempt_at = %[1]d WHERE name = 'test1'; + `, + s.Clock.Now().Unix(), + ) s.Auditor.ExpectEvents(t, cadf.Event{ RequestPath: "/keppel/v1/accounts/test1", @@ -1965,8 +1783,16 @@ func TestDeleteAccount(t *testing.T) { }, ) - // that didn't touch the DB - easypg.AssertDBContent(t, s.DB.DbMap.Db, "fixtures/delete-account-001.sql") + // account is already set to be deleted, so nothing happens + assert.HTTPRequest{ + Method: "DELETE", + Path: "/keppel/v1/accounts/test1", + Header: map[string]string{"X-Test-Perms": "view:tenant1,change:tenant1"}, + ExpectStatus: http.StatusNoContent, + }.Check(t, h) + + tr.DBChanges().AssertEmpty() + s.Auditor.ExpectEvents(t /*, nothing */) } //nolint:unparam diff --git a/internal/api/keppel/fixtures/delete-account-000.sql b/internal/api/keppel/fixtures/delete-account-000.sql deleted file mode 100644 index 6adf13bb3..000000000 --- a/internal/api/keppel/fixtures/delete-account-000.sql +++ /dev/null @@ -1,28 +0,0 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at) VALUES ('test1', 'tenant1', 200); -INSERT INTO accounts (name, auth_tenant_id) VALUES ('test2', 'tenant2'); -INSERT INTO accounts (name, auth_tenant_id) VALUES ('test3', 'tenant3'); - -INSERT INTO blob_mounts (blob_id, repo_id) VALUES (1, 1); -INSERT INTO blob_mounts (blob_id, repo_id) VALUES (2, 1); -INSERT INTO blob_mounts (blob_id, repo_id) VALUES (3, 1); - -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (1, 'test1', 'sha256:442f91fa9998460f28e8ff7023e5ddca679f7d2b51dc5498e8aba249678cc7f8', 1048919, '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b', 0, 604800); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (2, 'test1', 'sha256:3ae14a50df760250f0e97faf429cc4541c832ed0de61ad5b6ac25d1d695d1a6e', 1048919, 'd4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35', 1, 604801); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (3, 'test1', 'sha256:92b29e540b6fcadd4e07525af1546c7eff1bb9a8ef0ef249e0b234cdb13dbea3', 1412, '4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce', 2, 604802); - -INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 1); -INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 2); -INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 3); - -INSERT INTO manifest_contents (repo_id, digest, content) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', '{"manifests":[{"digest":"sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c","mediaType":"application/vnd.docker.distribution.manifest.v2+json","platform":{"architecture":"amd64","os":"linux"},"size":592}],"mediaType":"application/vnd.docker.distribution.manifest.list.v2+json","schemaVersion":2}'); - -INSERT INTO manifest_manifest_refs (repo_id, parent_digest, child_digest) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c'); - -INSERT INTO manifests (repo_id, digest, media_type, size_bytes, pushed_at, next_validation_at) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'application/vnd.docker.distribution.manifest.list.v2+json', 909, 0, 86400); -INSERT INTO manifests (repo_id, digest, media_type, size_bytes, pushed_at, next_validation_at) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 'application/vnd.docker.distribution.manifest.v2+json', 592, 100, 86500); - -INSERT INTO repos (id, account_name, name) VALUES (1, 'test1', 'foo/bar'); -INSERT INTO repos (id, account_name, name) VALUES (2, 'test1', 'something-else'); - -INSERT INTO trivy_security_info (repo_id, digest, vuln_status, message, next_check_at) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'Pending', '', 0); -INSERT INTO trivy_security_info (repo_id, digest, vuln_status, message, next_check_at) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 'Pending', '', 0); diff --git a/internal/api/keppel/fixtures/delete-account-001.sql b/internal/api/keppel/fixtures/delete-account-001.sql deleted file mode 100644 index d88c04acc..000000000 --- a/internal/api/keppel/fixtures/delete-account-001.sql +++ /dev/null @@ -1,28 +0,0 @@ -INSERT INTO accounts (name, auth_tenant_id, next_blob_sweep_at, is_deleting, next_deletion_attempt_at) VALUES ('test1', 'tenant1', 200, TRUE, 0); -INSERT INTO accounts (name, auth_tenant_id) VALUES ('test2', 'tenant2'); -INSERT INTO accounts (name, auth_tenant_id) VALUES ('test3', 'tenant3'); - -INSERT INTO blob_mounts (blob_id, repo_id) VALUES (1, 1); -INSERT INTO blob_mounts (blob_id, repo_id) VALUES (2, 1); -INSERT INTO blob_mounts (blob_id, repo_id) VALUES (3, 1); - -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (1, 'test1', 'sha256:442f91fa9998460f28e8ff7023e5ddca679f7d2b51dc5498e8aba249678cc7f8', 1048919, '6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b', 0, 604800); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (2, 'test1', 'sha256:3ae14a50df760250f0e97faf429cc4541c832ed0de61ad5b6ac25d1d695d1a6e', 1048919, 'd4735e3a265e16eee03f59718b9b5d03019c07d8b6c51f90da3a666eec13ab35', 1, 604801); -INSERT INTO blobs (id, account_name, digest, size_bytes, storage_id, pushed_at, next_validation_at) VALUES (3, 'test1', 'sha256:92b29e540b6fcadd4e07525af1546c7eff1bb9a8ef0ef249e0b234cdb13dbea3', 1412, '4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fce', 2, 604802); - -INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 1); -INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 2); -INSERT INTO manifest_blob_refs (repo_id, digest, blob_id) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 3); - -INSERT INTO manifest_contents (repo_id, digest, content) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', '{"manifests":[{"digest":"sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c","mediaType":"application/vnd.docker.distribution.manifest.v2+json","platform":{"architecture":"amd64","os":"linux"},"size":592}],"mediaType":"application/vnd.docker.distribution.manifest.list.v2+json","schemaVersion":2}'); - -INSERT INTO manifest_manifest_refs (repo_id, parent_digest, child_digest) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c'); - -INSERT INTO manifests (repo_id, digest, media_type, size_bytes, pushed_at, next_validation_at) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'application/vnd.docker.distribution.manifest.list.v2+json', 909, 0, 86400); -INSERT INTO manifests (repo_id, digest, media_type, size_bytes, pushed_at, next_validation_at) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 'application/vnd.docker.distribution.manifest.v2+json', 592, 100, 86500); - -INSERT INTO repos (id, account_name, name) VALUES (1, 'test1', 'foo/bar'); -INSERT INTO repos (id, account_name, name) VALUES (2, 'test1', 'something-else'); - -INSERT INTO trivy_security_info (repo_id, digest, vuln_status, message, next_check_at) VALUES (1, 'sha256:6aa9f3d5659c999fecab6df26efb864792763a2c7ae7580edf5dc11df2882ea5', 'Pending', '', 0); -INSERT INTO trivy_security_info (repo_id, digest, vuln_status, message, next_check_at) VALUES (1, 'sha256:e255ca60e7cfef94adfcd95d78f1eb44404c4f5887cbf506dd5799489a42606c', 'Pending', '', 0); diff --git a/internal/api/keppel/repos_test.go b/internal/api/keppel/repos_test.go index 1fd62f79a..721a4e036 100644 --- a/internal/api/keppel/repos_test.go +++ b/internal/api/keppel/repos_test.go @@ -40,13 +40,6 @@ func mustInsert(t *testing.T, db *keppel.DB, obj any) { } } -func mustDo(t *testing.T, err error) { - t.Helper() - if err != nil { - t.Fatal(err.Error()) - } -} - func mustExec(t *testing.T, db *keppel.DB, query string, args ...any) { t.Helper() _, err := db.Exec(query, args...) From c3cafaab452eadb207ee44a5ef35870283940650 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Tue, 12 Nov 2024 13:49:21 +0100 Subject: [PATCH 15/19] fix whitespace --- internal/keppel/database.go | 32 +++++++++++------------ internal/tasks/account_deletion.go | 14 +++++----- internal/tasks/account_management_test.go | 4 +-- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/internal/keppel/database.go b/internal/keppel/database.go index 4269641bf..8d876727d 100644 --- a/internal/keppel/database.go +++ b/internal/keppel/database.go @@ -305,27 +305,27 @@ var sqlMigrations = map[string]string{ DROP COLUMN use_for_pull_delegation; `, "043_add_accounts_is_deleting.up.sql": ` - ALTER TABLE accounts - ADD COLUMN is_deleting BOOLEAN NOT NULL DEFAULT FALSE, - ADD COLUMN next_deletion_attempt_at TIMESTAMPTZ DEFAULT NULL; + ALTER TABLE accounts + ADD COLUMN is_deleting BOOLEAN NOT NULL DEFAULT FALSE, + ADD COLUMN next_deletion_attempt_at TIMESTAMPTZ DEFAULT NULL; - UPDATE accounts SET is_deleting = TRUE WHERE in_maintenance; + UPDATE accounts SET is_deleting = TRUE WHERE in_maintenance; - ALTER TABLE accounts - DROP COLUMN in_maintenance, - DROP COLUMN metadata_json; - `, + ALTER TABLE accounts + DROP COLUMN in_maintenance, + DROP COLUMN metadata_json; + `, "043_add_accounts_is_deleting.down.sql": ` - ALTER TABLE accounts - ADD COLUMN in_maintenance BOOLEAN NOT NULL DEFAULT FALSE, - ADD COLUMN metadata_json TEXT NOT NULL DEFAULT ''; + ALTER TABLE accounts + ADD COLUMN in_maintenance BOOLEAN NOT NULL DEFAULT FALSE, + ADD COLUMN metadata_json TEXT NOT NULL DEFAULT ''; - UPDATE accounts SET in_maintenance = TRUE WHERE is_deleting; + UPDATE accounts SET in_maintenance = TRUE WHERE is_deleting; - ALTER TABLE accounts - DROP COLUMN is_deleting, - DROP COLUMN next_deletion_attempt_at; - `, + ALTER TABLE accounts + DROP COLUMN is_deleting, + DROP COLUMN next_deletion_attempt_at; + `, } // DB adds convenience functions on top of gorp.DbMap. diff --git a/internal/tasks/account_deletion.go b/internal/tasks/account_deletion.go index b4c8bdb3b..83c858b51 100644 --- a/internal/tasks/account_deletion.go +++ b/internal/tasks/account_deletion.go @@ -190,15 +190,15 @@ var ( JOIN repos r ON m.repo_id = r.id JOIN accounts a ON a.name = r.account_name LEFT OUTER JOIN manifest_manifest_refs mmr ON mmr.repo_id = r.id AND m.digest = mmr.child_digest - WHERE a.name = $1 AND parent_digest IS NULL + WHERE a.name = $1 AND parent_digest IS NULL `) deleteAccountCountManifestsQuery = sqlext.SimplifyWhitespace(` - SELECT COUNT(m.digest) - FROM manifests m - JOIN repos r ON m.repo_id = r.id - JOIN accounts a ON a.name = r.account_name - WHERE a.name = $1 - `) + SELECT COUNT(m.digest) + FROM manifests m + JOIN repos r ON m.repo_id = r.id + JOIN accounts a ON a.name = r.account_name + WHERE a.name = $1 + `) deleteAccountReposQuery = `DELETE FROM repos WHERE account_name = $1` deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` diff --git a/internal/tasks/account_management_test.go b/internal/tasks/account_management_test.go index 438fb9fe4..72ae9c480 100644 --- a/internal/tasks/account_management_test.go +++ b/internal/tasks/account_management_test.go @@ -197,8 +197,8 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[10]s'; DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[11]s'; DELETE FROM manifest_contents WHERE repo_id = 2 AND digest = '%[12]s'; - DELETE FROM manifest_manifest_refs WHERE repo_id = 2 AND parent_digest = '%[12]s' AND child_digest = '%[9]s'; - DELETE FROM manifest_manifest_refs WHERE repo_id = 2 AND parent_digest = '%[12]s' AND child_digest = '%[10]s'; + DELETE FROM manifest_manifest_refs WHERE repo_id = 2 AND parent_digest = '%[12]s' AND child_digest = '%[9]s'; + DELETE FROM manifest_manifest_refs WHERE repo_id = 2 AND parent_digest = '%[12]s' AND child_digest = '%[10]s'; DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[9]s'; DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[10]s'; DELETE FROM manifests WHERE repo_id = 2 AND digest = '%[11]s'; From dc27b0e1f9461209071a9dbdfd24c66fcb6b3f58 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Tue, 12 Nov 2024 13:50:05 +0100 Subject: [PATCH 16/19] review: task metric names should be plural --- internal/tasks/account_deletion.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/tasks/account_deletion.go b/internal/tasks/account_deletion.go index 83c858b51..d6ffcd29f 100644 --- a/internal/tasks/account_deletion.go +++ b/internal/tasks/account_deletion.go @@ -41,8 +41,8 @@ func (j *Janitor) DeleteAccountsJob(registerer prometheus.Registerer) jobloop.Jo Metadata: jobloop.JobMetadata{ ReadableName: "delete accounts marked for deletion", CounterOpts: prometheus.CounterOpts{ - Name: "keppel_account_deletion", - Help: "Counter for deleted accounts.", + Name: "keppel_account_deletions", + Help: "Counter for attempts to cleanup a deleted account..", }, }, DiscoverTask: j.discoverAccountForDeletion, From 72f93ce0ab741d8e2c20e8e3c882090868d1dc81 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Tue, 12 Nov 2024 14:00:33 +0100 Subject: [PATCH 17/19] remove fixed number of retries on recursive manifest deletion --- internal/tasks/account_deletion.go | 139 ++++++++++++++--------------- 1 file changed, 69 insertions(+), 70 deletions(-) diff --git a/internal/tasks/account_deletion.go b/internal/tasks/account_deletion.go index d6ffcd29f..876cb2994 100644 --- a/internal/tasks/account_deletion.go +++ b/internal/tasks/account_deletion.go @@ -63,6 +63,28 @@ func (j *Janitor) discoverAccountForDeletion(_ context.Context, _ prometheus.Lab return accountName, err } +var ( + deleteAccountFindManifestsQuery = sqlext.SimplifyWhitespace(` + SELECT r.name, m.digest + FROM manifests m + JOIN repos r ON m.repo_id = r.id + JOIN accounts a ON a.name = r.account_name + LEFT OUTER JOIN manifest_manifest_refs mmr ON mmr.repo_id = r.id AND m.digest = mmr.child_digest + WHERE a.name = $1 AND parent_digest IS NULL + `) + deleteAccountCountManifestsQuery = sqlext.SimplifyWhitespace(` + SELECT COUNT(m.digest) + FROM manifests m + JOIN repos r ON m.repo_id = r.id + JOIN accounts a ON a.name = r.account_name + WHERE a.name = $1 + `) + deleteAccountReposQuery = `DELETE FROM repos WHERE account_name = $1` + deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` + deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` + deleteAccountMarkAllBlobsForDeletionQuery = `UPDATE blobs SET can_be_deleted_at = $2 WHERE account_name = $1` +) + func (j *Janitor) deleteMarkedAccount(ctx context.Context, accountName models.AccountName, labels prometheus.Labels) error { accountModel, err := keppel.FindAccount(j.db, accountName) if errors.Is(err, sql.ErrNoRows) { @@ -79,50 +101,54 @@ func (j *Janitor) deleteMarkedAccount(ctx context.Context, accountName models.Ac } // can only delete account when all manifests from it are deleted - tries := 0 - for { - if tries >= 3 { - return fmt.Errorf("could not delete all manifests references by %s", accountModel.Name) - } - tries++ - - err = sqlext.ForeachRow(j.db, deleteAccountFindManifestsQuery, []any{accountModel.Name}, - func(rows *sql.Rows) error { - var m deleteAccountRemainingManifest - err := rows.Scan(&m.RepositoryName, &m.Digest) - if err != nil { - return err - } - - parsedDigest, err := digest.Parse(m.Digest) - if err != nil { - return fmt.Errorf("while deleting manifest %q in repository %q: could not parse digest: %w", - m.Digest, m.RepositoryName, err) - } - repo, err := keppel.FindRepository(j.db, m.RepositoryName, accountModel.Name) - if err != nil { - return fmt.Errorf("while deleting manifest %q in repository %q: could not find repository in DB: %w", - m.Digest, m.RepositoryName, err) - } - err = j.processor().DeleteManifest(ctx, accountModel.Reduced(), *repo, parsedDigest, actx) - if err != nil { - return fmt.Errorf("while deleting manifest %q in repository %q: %w", - m.Digest, m.RepositoryName, err) - } - - return nil - }, - ) - if err != nil { - return err - } + deletedManifestCount := 0 + err = sqlext.ForeachRow(j.db, deleteAccountFindManifestsQuery, []any{accountModel.Name}, + func(rows *sql.Rows) error { + var ( + repoName string + digestStr string + ) + err := rows.Scan(&repoName, &digestStr) + if err != nil { + return err + } + + parsedDigest, err := digest.Parse(digestStr) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: could not parse digest: %w", + digestStr, repoName, err) + } + repo, err := keppel.FindRepository(j.db, repoName, accountModel.Name) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: could not find repository in DB: %w", + digestStr, repoName, err) + } + err = j.processor().DeleteManifest(ctx, accountModel.Reduced(), *repo, parsedDigest, actx) + if err != nil { + return fmt.Errorf("while deleting manifest %q in repository %q: %w", + digestStr, repoName, err) + } + deletedManifestCount++ + + return nil + }, + ) + if err != nil { + return err + } - manifestCount, err := j.db.SelectInt(deleteAccountCountManifestsQuery, accountModel.Name) - if err != nil { - return err - } - if manifestCount == 0 { - break + // the section above could only delete manifests that are not referenced by others; + // if there is stuff left over, restart the loop + manifestCount, err := j.db.SelectInt(deleteAccountCountManifestsQuery, accountModel.Name) + if err != nil { + return err + } + if manifestCount > 0 { + if deletedManifestCount > 0 { + return j.deleteMarkedAccount(ctx, accountName, labels) + } else { + return fmt.Errorf("cannot make progress on deleting account %q: %d manifests remain, but none are ready to delete", + accountName, manifestCount) } } @@ -182,30 +208,3 @@ func (j *Janitor) deleteMarkedAccount(ctx context.Context, accountName models.Ac return tx.Commit() } - -var ( - deleteAccountFindManifestsQuery = sqlext.SimplifyWhitespace(` - SELECT r.name, m.digest - FROM manifests m - JOIN repos r ON m.repo_id = r.id - JOIN accounts a ON a.name = r.account_name - LEFT OUTER JOIN manifest_manifest_refs mmr ON mmr.repo_id = r.id AND m.digest = mmr.child_digest - WHERE a.name = $1 AND parent_digest IS NULL - `) - deleteAccountCountManifestsQuery = sqlext.SimplifyWhitespace(` - SELECT COUNT(m.digest) - FROM manifests m - JOIN repos r ON m.repo_id = r.id - JOIN accounts a ON a.name = r.account_name - WHERE a.name = $1 - `) - deleteAccountReposQuery = `DELETE FROM repos WHERE account_name = $1` - deleteAccountCountBlobsQuery = `SELECT COUNT(id) FROM blobs WHERE account_name = $1` - deleteAccountScheduleBlobSweepQuery = `UPDATE accounts SET next_blob_sweep_at = $2 WHERE name = $1` - deleteAccountMarkAllBlobsForDeletionQuery = `UPDATE blobs SET can_be_deleted_at = $2 WHERE account_name = $1` -) - -type deleteAccountRemainingManifest struct { - RepositoryName string - Digest string -} From 7af4f4f26f46547dfc714e3fb4aa05209695b175 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Tue, 12 Nov 2024 14:21:18 +0100 Subject: [PATCH 18/19] fix unreachable branch --- internal/tasks/account_management.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/tasks/account_management.go b/internal/tasks/account_management.go index 29c5341dc..097379ef2 100644 --- a/internal/tasks/account_management.go +++ b/internal/tasks/account_management.go @@ -98,10 +98,11 @@ func (j *Janitor) enforceManagedAccount(ctx context.Context, accountName models. var accountModel *models.Account accountModel, err = keppel.FindAccount(j.db, accountName) if err != nil { - return err - } - if errors.Is(err, sql.ErrNoRows) { - nextCheckDuration = 0 // assume the account got already deleted + if errors.Is(err, sql.ErrNoRows) { + nextCheckDuration = 0 // assume the account got already deleted + } else { + return err + } } else { actx := keppel.AuditContext{ UserIdentity: janitorUserIdentity{TaskName: "managed-account-enforcement"}, From 8e6c77a96a37a6a50c5514873f9ae0e95a38b952 Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Tue, 12 Nov 2024 14:29:51 +0100 Subject: [PATCH 19/19] finish implementation of complex deletion test --- internal/tasks/account_management_test.go | 32 ++++++----------------- 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/internal/tasks/account_management_test.go b/internal/tasks/account_management_test.go index 72ae9c480..d61b6f1ad 100644 --- a/internal/tasks/account_management_test.go +++ b/internal/tasks/account_management_test.go @@ -224,33 +224,11 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { imageList.Manifest.Digest.String(), ) - // TODO: fix the can_be_deleted_at reset - s.Clock.StepBy(3 * time.Minute) - expectSuccess(t, deleteAccountsJob.ProcessOne(s.Ctx)) - tr.DBChanges().AssertEqualf(` - UPDATE accounts SET next_blob_sweep_at = %[1]d, next_deletion_attempt_at = %[2]d WHERE name = 'abcde'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 1 AND account_name = 'abcde' AND digest = '%[3]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 2 AND account_name = 'abcde' AND digest = '%[4]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 3 AND account_name = 'abcde' AND digest = '%[5]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 4 AND account_name = 'abcde' AND digest = '%[6]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 5 AND account_name = 'abcde' AND digest = '%[7]s'; - UPDATE blobs SET can_be_deleted_at = %[1]d WHERE id = 6 AND account_name = 'abcde' AND digest = '%[8]s'; - `, - s.Clock.Now().Unix(), - s.Clock.Now().Add(1*time.Minute).Unix(), - image.Layers[0].Digest.String(), - image.Layers[1].Digest.String(), - image.Config.Digest.String(), - images[0].Config.Digest.String(), - images[1].Layers[0].Digest.String(), - images[1].Config.Digest.String(), - images[0].Config.Digest.String(), - ) - - s.Clock.StepBy(1 * time.Minute) // we need to run this twice because the common test setup includes another account that is irrelevant to this test + s.Clock.StepBy(1 * time.Minute) expectSuccess(t, blobSweepJob.ProcessOne(s.Ctx)) expectSuccess(t, blobSweepJob.ProcessOne(s.Ctx)) + expectError(t, sql.ErrNoRows.Error(), blobSweepJob.ProcessOne(s.Ctx)) tr.DBChanges().AssertEqualf(` UPDATE accounts SET next_blob_sweep_at = %[1]d WHERE name = 'abcde'; UPDATE accounts SET next_blob_sweep_at = %[1]d WHERE name = 'test1'; @@ -269,4 +247,10 @@ func TestAccountManagementWithComplexDeletion(t *testing.T) { images[1].Layers[0].Digest.String(), images[1].Config.Digest.String(), ) + + // now account deletion can go through + s.Clock.StepBy(1 * time.Minute) + expectSuccess(t, deleteAccountsJob.ProcessOne(s.Ctx)) + expectError(t, sql.ErrNoRows.Error(), deleteAccountsJob.ProcessOne(s.Ctx)) + tr.DBChanges().AssertEqualf(`DELETE FROM accounts WHERE name = 'abcde';`) }