From 06251bb69e9a958726b10b0e64855d571f326d1b 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] 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 | 155 +++++++++--------- .../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, 156 insertions(+), 160 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 e99768655..0a43e2189 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,39 +285,54 @@ 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} { + for _, isDeleting := range []bool{true, false} { + 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, + } + if isDeleting { + account = assert.JSONObject{ + "auth_tenant_id": "tenant1", + "rbac_policies": rbacPoliciesJSON, + } + accountExpect = assert.JSONObject{ + "name": "second", + "auth_tenant_id": "tenant1", + "in_maintenance": false, + "metadata": nil, + "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": assert.JSONObject{ - "auth_tenant_id": "tenant1", - "in_maintenance": inMaintenance, - "rbac_policies": rbacPoliciesJSON, - }, + "account": account, }, ExpectStatus: http.StatusOK, ExpectBody: assert.JSONObject{ - "account": assert.JSONObject{ - "name": "second", - "auth_tenant_id": "tenant1", - "in_maintenance": inMaintenance, - "metadata": assert.JSONObject{}, - "rbac_policies": rbacPoliciesJSON, - }, + "account": accountExpect, }, }.Check(t, h) @@ -346,15 +345,15 @@ func TestAccountsAPI(t *testing.T) { "account": assert.JSONObject{ "name": "second", "auth_tenant_id": "tenant1", - "in_maintenance": inMaintenance, - "metadata": assert.JSONObject{}, + "in_maintenance": false, + "metadata": nil, "rbac_policies": rbacPoliciesJSON, }, }, }.Check(t, h) // the first pass also generates an audit event since we're touching the GCPolicies - if inMaintenance { + if isDeleting { s.Auditor.ExpectEvents(t, cadf.Event{ RequestPath: "/keppel/v1/accounts/second", @@ -394,10 +393,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 +400,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 +409,7 @@ func TestAccountsAPI(t *testing.T) { "name": "second", "auth_tenant_id": "tenant1", "in_maintenance": false, - "metadata": newMetadataJSON, + "metadata": nil, "rbac_policies": newRBACPoliciesJSON, }, }, @@ -460,7 +453,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 +482,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 +562,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 +1055,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 +1076,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 +1092,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 +1111,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 +1130,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 +1148,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 +1169,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 +1189,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 +1264,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 +1362,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 +1389,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 +1424,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 +1560,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 +1590,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 +1627,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 +1652,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 +1669,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 +1691,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 +1753,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 +1837,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", IsDeleting: true, NextBlobSweepedAt: &nextBlobSweepAt, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, + {Name: "test2", AuthTenantID: "tenant2", IsDeleting: true, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, + {Name: "test3", AuthTenantID: "tenant3", IsDeleting: true, GCPoliciesJSON: "[]", SecurityScanPoliciesJSON: "[]"}, } for _, account := range accounts { mustInsert(t, s.DB, account) @@ -2034,7 +2035,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", @@ -2072,7 +2073,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{ @@ -2094,6 +2095,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", @@ -2110,7 +2113,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..547db0468 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, is_deleting) VALUES ('test1', 'tenant1', 200, TRUE); +INSERT INTO accounts (name, auth_tenant_id, is_deleting) VALUES ('test2', 'tenant2', TRUE); +INSERT INTO accounts (name, auth_tenant_id, is_deleting) VALUES ('test3', 'tenant3', TRUE); 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" {