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
23503naming the new FK falls throughclassifyContainerRemoteRepositoryWriteErrorto a generic write error; deleteNamespaceKeysTx, the re-enable hard-delete, wraps the refusal without a sentinel, unlikeRetireKeyVersion.
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
Upbudgetrunner.gocaps 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
Downcomment predicts rather than a surprise: theUpis oneVALIDATE CONSTRAINTunderSHARE UPDATE EXCLUSIVE, while theDowndrops the constraint and re-adds itNOT VALID, twoALTER TABLEs that takeACCESS EXCLUSIVEon 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/datastorefiles in the diff,namespace_encryption_keys.goandnamespace_encryption_keys_rotation.go, are comment-only, andinternal/datastore/sqlstate.godispatches no statement, so there is no new or changed statement toEXPLAIN. - No lock measurement, and none of these numbers back the adding migration's
Lock budgetparagraph. 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 nolock_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 underACCESS EXCLUSIVE; both foreign-key sides' parents + their 128 partitions = 130 underSHARE 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)