feat(datastore): add container's encrypted credential columns (S04-A per-format columns plan: 6/11)

What this step does

Adds Container's encrypted upstream-credential columns to container_remote_repositories: the key-wrap trio (wrapped_dek, ns_key_id, ns_key_version) and the ciphertext pair (encrypted_username, encrypted_password), all five nullable, guarded as one unit by check_container_remote_repositories_enc_credentials_all_or_none, with index_container_remote_repositories_on_ns_id_ns_key_id over (namespace_id, ns_key_id) and the composite fk_container_remote_repositories_ns_key_id_ns_enc_keys referencing namespace_encryption_keys (id, namespace_id) ON DELETE RESTRICT.

Schema only. Nothing populates the new columns and no Go code reads or writes them yet. Encryption needs the Go crypto stack and the root key, so a plaintext-to-ciphertext conversion cannot be a migration statement, and ADR-006 rejects post-deployment migrations. The interim tmp_plaintext_* pair stays the live credential store and keeps its own constraint until a later release drops both together. Step 7 lands Container's reads and writes.

The NOT VALID add and the VALIDATE scan ship as two migrations, so the scan takes SHARE UPDATE EXCLUSIVE instead of running under the add's ACCESS EXCLUSIVE, and a failed scan retries without redoing the add.

Merge order

Merges after Step 2 (Maven's columns). goose.WithAllowOutofOrder(false) means a filesystem version below the database's max applied version is treated as missing and the whole Up pass is refused, so the two pairs must land in timestamp order. lint:migration-ordering enforces this and has no allow_failure: whichever pair merges second is failed and re-stamped rather than shipping a deploy goose would refuse. Both this MR and Step 2 also hand-edit knownHeadVersion and regenerate structure.sql, so they conflict textually whichever way round they go.

Because that re-stamp is the enforced remedy, the adding migration cites its Maven sibling by basename glob rather than by version prefix: a prefix written into a file that is frozen on merge would name a file that never existed.

Plan: Per-format encrypted credential columns, Step 6 of 11. Step 1 (!2349 (merged)) has merged.

Governing ADR

ADR-007 publishes the three remote-repository tables' column lists. It did not declare the key-wrap machinery, so the plan gated Steps 2, 6 and 9 on a handbook amendment. That amendment has landed (handbook 417f7b765, 2026-09-04, "Artifact Registry ADR 007: publish the credential key-wrap columns") and publishes wrapped_dek, ns_key_id, ns_key_version, the (namespace_id, ns_key_id) index and the composite ON DELETE RESTRICT reference for container_remote_repositories exactly as built here. The local docs/adr/ mirror is a daily-synced copy and is one commit behind at the time of writing, so review the schema against the upstream file, not the mirror; ./scripts/adr-freshness.sh reports the gap.

Review fixes carried in this branch

The last two commits address findings from a review of the first four.

TestContainerRemoteConstraints_NsKeyFKRestrictsKeyRowDelete asserted SQLSTATE 23503 exactly for a refused DELETE against the referenced side of an ON DELETE RESTRICT reference. PostgreSQL reports that as restrict_violation from 18 on, so the [POSTGRES, 18] leg would have been red while every local run on 16 stayed green. Measured against this exact shape, a hash-partitioned referencing table and a hash-partitioned referenced one:

direction 16.15 17.11 18.6
INSERT, absent key 23503, declared name 23503, declared name 23503, declared name
DELETE, RESTRICT 23503, <table>_<cols>_fkey[N] 23503, same shape 23001, declared name plus _N

internal/datastore/sqlstate.go already stated the rule and isKeyStillReferencedErr already maps both codes, so the fix brings the test in line with the production mapper. The delete direction now uses assertRestrictOrForeignKeyViolation; the three insert-direction call sites keep the exact-23503 helper.

Also corrected: four justifications that read as verified but do not operate (the index's totality, "puts the scan under an operator's control", the row count bounding the lock hold, and the concurrent-index directive omitting the transaction-block refusal), and three merged comments this branch falsifies (CountRowsForVersion, RetireKeyVersion, and the key-row recovery note in namespace_encryption_keys_schema_notes.md). Each commit body has the detail.

What this does not reach

Two arms the new FK opens that no caller handles, both unreachable while nothing writes ns_key_id, named in internal/datastore/migrations/container_remote_credential_columns.md and owned by #417 (closed)'s per-format wiring steps:

  • an insert-side 23503 naming the new FK falls through classifyContainerRemoteRepositoryWriteError to a generic write error;
  • deleteNamespaceKeysTx, the re-enable hard-delete, wraps the refusal without a sentinel, unlike RetireKeyVersion.

Testing

Twelve new tests. The all-or-none unit gets all ten half-set rejections (each of the five columns held back alone, and each set alone) plus the accept arms; the FK's pg_get_constraintdef, confdeltype, confmatchtype and convalidated are pinned; the CHECK and the FK are each counted onto all 64 partitions; the constraint name is measured at exactly 63 characters, PostgreSQL's NAMEDATALEN - 1; and two static tests parse the migration files for the add/validate split and the dated row-count justification. All three auth_status values get a positive hit. The twelfth, TestForeignKeyViolationHelpers_DisagreeOnRestrictViolation, pins the one SQLSTATE the two reference-violation assertion helpers disagree on, and needs no database, so it asserts that on every version rather than only on the leg whose PostgreSQL reports 23001.

Ran against real databases rather than only locally: the container-remote suite is 194 pass, 0 fail on PostgreSQL 18.6 and 194 / 0 on 16.15. The SQLSTATE guard was falsified by reverting the single call site, which fails on 18.6 with must surface as foreign_key_violation (23503); got 23001.

golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false reports 127 findings in internal/datastore/migrations at this branch's head 1f5ad2ed1, and the same 127, the same set line for line, with the two helper-test files put back to the state they were in before the FK-helper test commit: that test and the assertT interface it needs add none. Two of the 127 do sit in a file this branch touches, schema_helpers_test.go (wsl and wsl_v5 on one append); the code they land on is byte-identical across that commit and only its line number moved. The branch also edits four files in internal/datastore itself, all comment or subtest-name changes; the same command over ./internal/datastore/ reports 5448 findings at the head, 82 of them in those four files, mostly contextcheck on the shared seed and cleanup helpers. Both figures were higher when measured at 87254e070, three commits back (129 and 5462), and the merge-base comparison recorded then has not been re-run at this head. Two //nolint tokens went in the simplification pass, gosec on insertContainerRemoteRepositoryCredentialsSQL and dupl on TestContainerRemoteRepositoriesSchema_Columns; each was dropped and the command re-run, and neither reports a finding. The gosec one was also checked at the merge base with the directive deleted and the identifier and value untouched, where it still does not fire, so it was already dead on main. squawk is clean, and every one of the ten squawk-ignore-file tokens was dropped individually and re-run to confirm it fires.

Database Review Evidence

Migrations

Note

Timings are from CI (db:migrate matrix, goose verbose) against an empty database, in apply / rollback order per PG version. Production-scale validation via Database Lab is not yet available. See Database review evidence for the matrix rationale and how to read the numbers.

Migration PG 16 PG 17 PG 18
20260904140000_add_container_remote_encrypted_credentials.sql OK (414.23ms / 195.74ms) OK (334.07ms / 186.47ms) OK (185.33ms / 78ms)
20260904140100_validate_container_remote_enc_credentials_check.sql OK (14.2ms / 51.37ms) OK (16.11ms / 53.9ms) OK (10.96ms / 22.73ms)

Collected from pipeline #2825943024, a merged-results pipeline on f6b5a7e14, which merges this branch's head 381c47ba0 into main bf451f30f. All three matrix legs passed. The head has since moved to 1f5ad2ed1, a test-only commit touching no file under internal/datastore/migrations/sql/, so the table stands at that head too.

Migration notes:

  • Neither migration reaches 1s on any leg, so neither is near the five-minute Up budget runner.go caps boot at. The slowest apply is the adding migration's 414.23ms on PG 16.
  • No version-specific regression. For both migrations the slowest leg is within 1.25x of the second-slowest (adding: 414.23ms against 334.07ms; validating: 16.11ms against 14.2ms), against the 2x that would flag one.
  • The validating migration's rollback runs 2.1x to 3.6x its own apply (51.37ms against 14.2ms on 16, 53.9ms against 16.11ms on 17, 22.73ms against 10.96ms on 18). That is the asymmetry the migration's own Down comment predicts rather than a surprise: the Up is one VALIDATE CONSTRAINT under SHARE UPDATE EXCLUSIVE, while the Down drops the constraint and re-adds it NOT VALID, two ALTER TABLEs that take ACCESS EXCLUSIVE on the parent and all 64 partitions.

What this evidence does not cover, stated because the table reads as complete and is not:

  • No query plans, because query mode does not apply. No changed hand-written, non-test Go file adds or modifies a query-producing method. The two internal/datastore files in the diff, namespace_encryption_keys.go and namespace_encryption_keys_rotation.go, are comment-only, and internal/datastore/sqlstate.go dispatches no statement, so there is no new or changed statement to EXPLAIN.
  • No lock measurement, and none of these numbers back the adding migration's Lock budget paragraph. These are apply and rollback durations on an empty database. They do not measure the lock hold, the unbounded wait to be granted it (that migration sets no lock_timeout, per #548), or any row count at production scale. The relation counts in that paragraph are arithmetic off the DDL and can be checked without a measurement (1 parent + 64 partitions + the new index + its 64 partition clones = 130 under ACCESS EXCLUSIVE; both foreign-key sides' parents + their 128 partitions = 130 under SHARE ROW EXCLUSIVE). Its "measured on PostgreSQL 16.15" wording is the part nothing here stands behind, and this section does not change that wording.
  • No at-scale validation. Lock-acquisition timing and row counts at production scale need Database Lab, which this project does not have; see the note above the table.

e2e scenarios

No scenario in docs/testing/ is added or affected. The change is schema only, with no route, no request or response shape and no behaviour reachable from an e2e client: the columns are nullable, nothing reads or writes them, and the interim plaintext store still serves every credential path. Step 7 is where an e2e-visible change first appears.

Diff size

2216 reviewable LOC across 18 files, past the 500 the development model asks a justification for. Splitting would not help: the migration, the schema dump it regenerates and the tests that pin it are one reversible unit, and a split would leave a migration on main with no test asserting its shape. By group:

group LOC note
tests 841 the eleven new tests and their fixtures
structure.sql 651 regenerated dump, mechanical: 1 parent + 64 partitions per object
sidecar docs 456 comment-cap overflow, prose that cannot live in _test.go
migrations SQL 220 the two migrations, mostly the squawk justifications
go-jet output 24 regenerated, DO NOT EDIT
Go comments 24 namespace_encryption_keys{,_rotation}.go, comment-only
other Go 24 doc comments only, no statement changes

Hand-written and reviewable, excluding the two regenerated groups: 1541 LOC.

Plan Status table

Per the step-MR guardrail this MR does not edit the plan. Its Status row is to be recorded in the standing docs(plans) batch MR.

Related to #417 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading