From 57ccdc7963d48bbd6d4a34bb8e0d8678a65fe238 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Wed, 4 Nov 2026 01:39:19 +0000 Subject: [PATCH 1/2] fix(storage-mongodb): store each access key ID once The MongoDB catalog had no unique index on access_keys.access_key_id, so every import stored a document, also under an ID that another document held. A credential lookup read the first matching document its query met, and a delete removed one document, so a key imported more than once still authenticated after its delete. PostgreSQL, SQLite and Cassandra key the table by the ID and answer such an import with 409. Migration 6, unique_access_key_ids, creates the unique index on MongoDB and does not apply to the other backends. One aggregation scans the collection in natural order and groups the documents by ID. Where one user holds every document of an ID, the body keeps the copy a lookup reads, logs and deletes the others, and records a note with their _id values. To find that copy it reads the record IDs of the ID's copies and sends explain for the lookup's query with that ID, because a partial index can give one ID another plan than the next. When the plan is a forward collection scan, the body keeps the copy with the lowest record ID, which that scan reads first. With any other plan it sends the lookup's own query. A copy deleted or given another ID since the read is neither deleted nor named in the note. detect stops the run with exit 5, and nothing is written, when the documents of an ID belong to more than one user, when two or more documents have no ID or share one that is not a string, or when an ID is an array. It also stops the run for an index that would make the build fail (access_key_id_1 with other options, or the unique index under another name) and for the unique index when it is hidden, which a lookup does not use. Each item names the documents or the index and the mongosh command that fixes it. The body reads again before it writes and refuses the same way. The build commits once a majority of the data-bearing voting members have built the index. Once the index is built, the read looks for array IDs alone, so a reconverge does not group the collection. listIndexes with includeIndexBuildInfo tells a build that is still running from a built index, and while one runs the read groups every document. A generated ID that another key holds now answers an internal error, as on PostgreSQL and SQLite, instead of 404 "User not found". The MongoDB migration tests expect migration 6. Tests on released catalogs cover the body: a golden test, which also checks with the profiler that the body sent no lookup query of its own, refusals of copies and of indexes, a table of index variants measured against MongoDB's own createIndexes, an operator's compound index, a partial index that answers the lookup for some IDs only, query settings that make the lookup scan backward (MongoDB 8 only), the read of array IDs beside the index, the body's own refusal, and a read whose $group spills to disk. Unit tests of the body cover Decimal128 key directions, entries of listIndexes for a running build, the explained command, the explain replies that report a forward scan, and the copy such a scan reads. New pytest cases check, on every backend, that an import of an existing ID answers 409 and keeps the first secret, and that a deleted key stops authenticating. The PostgreSQL CLI migration tests read the not applicable lines of migrate --list. docs/schema/mongodb.txt has the new index, and the upgrade manual describes the migration, its notes, its size limit, the steps that remove extra copies by hand on a large collection, and what to do when it stops. --- CONTRIBUTING.md | 41 +- .../storage-cassandra/src/migrations/mod.rs | 4 + .../storage-mongodb/src/management_store.rs | 9 +- .../migrations/m0006_unique_access_key_ids.rs | 921 +++++++++++++ crates/storage-mongodb/src/migrations/mod.rs | 16 + .../golden/0006_unique_access_key_ids.txt | 22 + .../storage-mongodb/tests/migrations_fresh.rs | 49 +- .../tests/migrations_guards.rs | 11 +- .../tests/migrations_holders.rs | 4 +- .../storage-mongodb/tests/migrations_locks.rs | 6 +- .../tests/migrations_released.rs | 1166 ++++++++++++++++- .../storage-mongodb/tests/migrations_spill.rs | 180 +++ .../tests/migrations_support/mod.rs | 29 +- crates/storage-postgres/src/migrations/mod.rs | 6 +- crates/storage-sqlite/src/migrations/mod.rs | 3 + .../src/migrations/released_tests.rs | 9 +- crates/storage-sqlite/src/migrations/tests.rs | 16 +- crates/storage/src/migrations/registry.rs | 7 +- devtools/run-mongodb-tests | 3 +- docs/adr/0023-catalog-migrations.md | 4 +- docs/getting-started.md | 6 +- docs/manuals/04-quickstart-setup-guide.md | 4 +- docs/manuals/05-admin-guide.md | 2 +- docs/manuals/07-upgrade-manual.md | 10 +- docs/manuals/08-install-linux.md | 2 +- docs/manuals/09-install-macos.md | 2 +- docs/schema/mongodb.txt | 1 + tests/test_auth_permissions.py | 92 ++ tests/test_cli_vector_catalog_migration.py | 28 +- 29 files changed, 2558 insertions(+), 95 deletions(-) create mode 100644 crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs create mode 100644 crates/storage-mongodb/tests/fixtures/golden/0006_unique_access_key_ids.txt create mode 100644 crates/storage-mongodb/tests/migrations_spill.rs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b0d641a98..bfd23f3cb 100755 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -341,7 +341,7 @@ SQLite, PostgreSQL, Cassandra and MongoDB migrate through this framework. ### Example: a nullable column on `tables` -Migration 6, `table_note`, adds a nullable `note TEXT` column to the catalog's +Migration 7, `table_note`, adds a nullable `note TEXT` column to the catalog's `tables` table on SQLite and PostgreSQL. In `crates/storage/src/migrations/registry.rs`, at the end of the @@ -349,18 +349,18 @@ In `crates/storage/src/migrations/registry.rs`, at the end of the ```rust /// Adds a nullable `note` column to the `tables` catalog table. - TableNote = 6, "table_note", + TableNote = 7, "table_note", "add a nullable note column to the tables catalog"; ``` PostgreSQL has `ADD COLUMN IF NOT EXISTS`, so its body is a file, -`crates/storage-postgres/migrations/0006_table_note.sql`: +`crates/storage-postgres/migrations/0007_table_note.sql`: ```sql -- Copyright 2026 ExtendDB contributors -- SPDX-License-Identifier: Apache-2.0 -- --- Migration 6, table_note, PostgreSQL catalog database: a nullable note +-- Migration 7, table_note, PostgreSQL catalog database: a nullable note -- column on tables. IF NOT EXISTS makes a second run change nothing. ALTER TABLE tables ADD COLUMN IF NOT EXISTS note TEXT; @@ -375,7 +375,7 @@ In `crates/storage-postgres/src/migrations/mod.rs`, the arm in `migration()`: bodies: vec![file_body!( StoreKind::Catalog, "migrations", - "0006_table_note.sql" + "0007_table_note.sql" )], }, ``` @@ -387,19 +387,20 @@ and the id in the transactional arm of `runs_in()`: | MigrationId::ResourceTagConditionKeys | MigrationId::SortKeyDefinitions | MigrationId::DropContinuousBackups + | MigrationId::UniqueAccessKeyIds | MigrationId::TableNote => RunsIn::Transaction, ``` SQLite has no `ADD COLUMN IF NOT EXISTS`, and a file with a plain `ADD COLUMN` fails when it runs a second time, so the SQLite body is code that checks for the column first. -`crates/storage-sqlite/src/migrations/m0006_table_note.rs`: +`crates/storage-sqlite/src/migrations/m0007_table_note.rs`: ```rust // Copyright 2026 ExtendDB contributors // SPDX-License-Identifier: Apache-2.0 -//! Migration 6, `table_note`, SQLite: a nullable `note` column on `tables`. +//! Migration 7, `table_note`, SQLite: a nullable `note` column on `tables`. //! //! SQLite has no `ADD COLUMN IF NOT EXISTS`, so the body reads the table's //! columns first and adds the column only when it is missing. A second run @@ -411,10 +412,10 @@ use extenddb_storage::migrations::{CodeBody, Done, MigrationError}; use super::SqliteStores; /// This file, whose SHA-256 the checksum test pins. -pub(crate) const SOURCE: &str = include_str!("m0006_table_note.rs"); +pub(crate) const SOURCE: &str = include_str!("m0007_table_note.rs"); /// Recorded as this migration's checksum. Changes only with a new migration. -const FINGERPRINT: &str = "0006_table_note/sqlite/1"; +const FINGERPRINT: &str = "0007_table_note/sqlite/1"; pub(crate) struct TableNote; @@ -453,7 +454,7 @@ beside the others: ```rust #[rustfmt::skip] #[allow(clippy::all, deprecated)] -mod m0006_table_note; +mod m0007_table_note; ``` and the arm in `migration()`: @@ -464,8 +465,8 @@ and the arm in `migration()`: convergent: true, bodies: vec![StoreBody::code( StoreKind::Catalog, - &m0006_table_note::TableNote, - m0006_table_note::SOURCE, + &m0007_table_note::TableNote, + m0007_table_note::SOURCE, )], }, ``` @@ -477,8 +478,8 @@ rows of `tables`: ```rust #[tokio::test] -async fn migration_6_adds_the_note_column_to_tables() { - let db = temp_db("m6"); +async fn migration_7_adds_the_note_column_to_tables() { + let db = temp_db("m7"); load_dump(&db, DEV_0_1_14).await; run(&db).await.expect("migrate"); let mut text = dump( @@ -488,12 +489,12 @@ async fn migration_6_adds_the_note_column_to_tables() { ) .await; text.push_str(&dump(&db, "SELECT table_name, note FROM tables ORDER BY table_name").await); - golden("0006_table_note", &text); + golden("0007_table_note", &text); } ``` -`EXTENDDB_BLESS_EXPECTED=1 cargo test -p extenddb-storage-sqlite --lib migration_6` -writes `crates/storage-sqlite/tests/fixtures/golden/0006_table_note.txt`, one +`EXTENDDB_BLESS_EXPECTED=1 cargo test -p extenddb-storage-sqlite --lib migration_7` +writes `crates/storage-sqlite/tests/fixtures/golden/0007_table_note.txt`, one tab-separated line per row under each query: ``` @@ -508,15 +509,15 @@ The PostgreSQL body is a file, so it has no golden test. The example declares `Kind::Breaking`. A nullable column can be additive by the rule in step 4, but declaring it additive needs the old-binary job, which this example does not add. As a breaking migration it makes a 1.x binary -without migration 6 refuse a catalog that has it, and a rollback restores the +without migration 7 refuse a catalog that has it, and a rollback restores the backup. The same PR changes the code that reads and writes `tables.note` in each backend's catalog store, and commits the `docs/schema/sqlite.txt` and `docs/schema/postgres.txt` that the integration jobs regenerate. It changes no per-table DDL, because `tables` is a catalog table, and adds no `RELEASED` -entry. `extenddb migrate --list` then shows migration 6, and on a catalog at -migration 5 `extenddb migrate` lists migration 6 as pending and exits 3. +entry. `extenddb migrate --list` then shows migration 7, and on a catalog at +migration 6 `extenddb migrate` lists migration 7 as pending and exits 3. ### Running the migration tests diff --git a/crates/storage-cassandra/src/migrations/mod.rs b/crates/storage-cassandra/src/migrations/mod.rs index aae33b64f..2ba00f79e 100644 --- a/crates/storage-cassandra/src/migrations/mod.rs +++ b/crates/storage-cassandra/src/migrations/mod.rs @@ -127,6 +127,10 @@ pub fn migration(id: MigrationId) -> Migration { "0005_drop_continuous_backups.cql" )], }, + MigrationId::UniqueAccessKeyIds => Migration::NotApplicable( + "access_key_id is the primary key of access_keys, and an import writes with IF NOT \ + EXISTS, so an ID can be stored once", + ), } } diff --git a/crates/storage-mongodb/src/management_store.rs b/crates/storage-mongodb/src/management_store.rs index 95b0f74bd..53f5fcb22 100644 --- a/crates/storage-mongodb/src/management_store.rs +++ b/crates/storage-mongodb/src/management_store.rs @@ -1750,12 +1750,17 @@ impl ManagementStore for MongoCatalogStore { }) .await .map_err(|e| { + // The unique index on access_key_id (migration 6) refuses a + // generated ID that another key holds. PostgreSQL and SQLite + // answer the same collision with an internal error. if is_duplicate_key(&e) { - OpError::NotFound("User not found".to_owned()) + tracing::error!( + "create_access_key: the generated access key ID {access_key_id} is taken" + ); } else { tracing::error!("create_access_key failed: {e}"); - OpError::Internal("Database error".to_owned()) } + OpError::Internal("Database error".to_owned()) })?; Ok(AccessKeyCreated { diff --git a/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs b/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs new file mode 100644 index 000000000..fc4cae748 --- /dev/null +++ b/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs @@ -0,0 +1,921 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Migration 6, `unique_access_key_ids`, MongoDB: give the catalog's +//! `access_keys` collection a unique index on `access_key_id`. +//! +//! PostgreSQL, SQLite and Cassandra key `access_keys` by the ID. Migration 1 +//! gives MongoDB's collection only a non-unique index on `account_id` and +//! `user_name`, so before this migration every import stored a document, +//! including one under an ID that another document held, and a credential +//! lookup, `find_one` on the ID, read whichever of them its query plan met +//! first. +//! +//! One aggregation scans the collection in natural order and groups the +//! documents by ID, keeping the IDs that more than one document holds and +//! the array IDs. `detect` refuses what the body cannot decide: an ID whose +//! documents belong to more than one user, two or more documents that have +//! no ID or share an ID that is not a string, a document whose ID is an +//! array, which a lookup reads for each ID in it, an index that would make +//! the build fail, and this migration's index when it is hidden, which a +//! lookup does not use. `run` reads again and refuses the same before it +//! writes. Then, for each ID that one user holds more than once, it finds +//! the copy a lookup reads, logs the other copies, deletes them and keeps +//! that copy, so every key authenticates as it did before. Last it creates +//! the index, committed once a majority of the replica set's data-bearing +//! voting members have built it. +//! +//! A lookup reads the copy its query plan meets first. For each ID stored +//! more than once the body reads which of its listed copies still hold it, +//! with their record IDs, and sends `explain` for the lookup's command with +//! that ID. When the plan is a forward collection scan, as it is with no +//! index that can answer the lookup and no query settings that change its +//! plan, the lookup reads the copy with the lowest record ID. With any other +//! plan the body sends the lookup's own query. +//! +//! Once the index is built no ID can be stored twice, so a later read +//! groups only the documents whose ID is an array. `listIndexes` also lists +//! an index whose build is running, and until the server reports the build +//! finished the read groups every document. Every request is safe to send +//! again: a second run finds nothing to delete, and creating an index that +//! exists with the same key and options succeeds. +//! +//! The checksum test pins this file once the migration is released. The +//! module is excluded from rustfmt and clippy so neither can require an +//! edit. + +use async_trait::async_trait; +use bson::{Bson, Decimal128, Document, doc}; +use extenddb_storage::migrations::{CatalogStatus, CodeBody, Done, MigrationError}; +use futures::TryStreamExt; +use mongodb::IndexModel; +use mongodb::options::{CommitQuorum, Hint, IndexOptions}; + +use super::MongoStores; + +/// This module, whose SHA-256 the checksum test pins. +pub(crate) const SOURCE: &str = include_str!("m0006_unique_access_key_ids.rs"); + +/// Recorded as this migration's checksum. Changes only with a new migration. +const FINGERPRINT: &str = "0006_unique_access_key_ids/mongodb/1"; + +/// The catalog collection this migration indexes. +const COLLECTION: &str = "access_keys"; + +/// The name of the index this migration creates. +const INDEX: &str = "access_key_id_1"; + +/// NamespaceNotFound: `listIndexes` on a collection that does not exist. +const NAMESPACE_NOT_FOUND: i32 = 26; + +/// Migration 6's catalog body. +pub(crate) struct UniqueAccessKeyIds; + +/// The documents that hold one value of `access_key_id`, when more than one +/// does or the value is an array. +struct Group { + /// The value, as `$group` returned it: null for documents without one. + key: Bson, + /// Whether the value is an array. + array: bool, + /// Every document that holds it, in natural order. + copies: Vec, +} + +/// What the body reads of one document. +struct Stored { + id: Bson, + account_id: Option, + user_name: Option, + created_at: Option, +} + +/// An index of `access_keys`, as `listIndexes` lists it. +struct Listed { + spec: Document, + /// Whether the server reports the index built. It also lists an index + /// whose build is running, with the spec of a built one. + built: bool, +} + +/// Why this migration refuses an index of `access_keys`. +enum Clash { + /// It is the unique index on the ID under another name, and the build + /// refuses a second name for it. + Renamed, + /// It is named `access_key_id_1` and is not the index this migration + /// creates, and the build refuses a second index of the name. + Named, + /// It is this migration's index, hidden, and the query planner does not + /// use a hidden index for a lookup. + Hidden, +} + +/// An index of `access_keys` that this migration refuses. +struct Conflict { + name: String, + spec: Document, + clash: Clash, +} + +/// What `detect` and `run` read. +struct State { + groups: Vec, + conflicts: Vec, +} + +/// What the record IDs of an ID's copies tell about the copy a forward +/// collection scan reads. +#[derive(Debug, PartialEq)] +enum Scanned { + /// The `_id` of the copy with the lowest record ID. + First(Bson), + /// No listed copy holds the ID any more. + Gone, + /// A record ID is not an Int64, so the copies cannot be ordered by it. + Unordered, +} + +impl Stored { + fn describe(&self) -> String { + let field = |value: &Option| value.clone().unwrap_or_else(|| "(none)".to_owned()); + let created = self + .created_at + .as_deref() + .map(|t| format!(", created {t}")) + .unwrap_or_default(); + format!( + "_id {} (account {}, user {}{created})", + self.id, + field(&self.account_id), + field(&self.user_name), + ) + } +} + +/// `_id A, _id B` for `copies`. +fn ids(copies: &[&Stored]) -> String { + copies.iter().map(|c| format!("_id {}", c.id)).collect::>().join(", ") +} + +/// The command that deletes one document of the collection in mongosh. +fn delete_command(db: &str) -> String { + format!("db.getSiblingDB(\"{db}\").{COLLECTION}.deleteOne({{ _id: }})") +} + +impl Group { + /// The ID, when it is a string: the only kind a lookup asks for. + fn access_key_id(&self) -> Option<&str> { + match &self.key { + Bson::String(id) if !self.array => Some(id), + _ => None, + } + } + + /// Whether every copy names the same account and user. + fn one_owner(&self) -> bool { + self.copies.windows(2).all(|pair| { + pair[0].account_id == pair[1].account_id && pair[0].user_name == pair[1].user_name + }) + } + + /// Whether `run` may keep the copy a lookup reads and delete the others. + fn resolvable(&self) -> bool { + self.access_key_id().is_some() && self.one_owner() + } + + /// What `detect` tells the operator about a group the body does not + /// resolve, with the `_id` a lookup reads when it is one of the copies. + /// The runner's advice adds the command to run after the fix. + fn refusal(&self, db: &str, read: Option<&Bson>) -> String { + let n = self.copies.len(); + let copies = self.copies.iter().map(Stored::describe).collect::>().join(", "); + let delete = delete_command(db); + if self.array { + let (them, each) = if n == 1 { ("it", "its") } else { ("each of them", "each one's") }; + return format!( + "the access key ID of {copies} in {db}.{COLLECTION} is the array {}, and a \ + credential lookup reads such a document for each ID in the array; delete \ + {them} with {delete}, or set {each} access_key_id to one string, in mongosh", + self.key + ); + } + match (&self.key, self.access_key_id()) { + (_, Some(id)) => { + let read = read + .map(|r| format!(", and a credential lookup reads _id {r}")) + .unwrap_or_default(); + format!( + "the access key ID {id} is stored {n} times in {db}.{COLLECTION}, for more \ + than one user: {copies}{read}; delete each copy that is not the key \ + owner's with {delete} in mongosh" + ) + } + (Bson::Null, None) => format!( + "{n} documents in {db}.{COLLECTION} have no access key ID, which no credential \ + lookup asks for, and the unique index takes one at most: {copies}; delete the \ + others with {delete} in mongosh" + ), + (key, None) => format!( + "{n} documents in {db}.{COLLECTION} have the access key ID {key}, which is not a \ + string and no credential lookup asks for, and the unique index takes one at \ + most: {copies}; delete the others with {delete} in mongosh" + ), + } + } +} + +impl Conflict { + fn refusal(&self, db: &str) -> String { + let command = |method: &str| { + format!("db.getSiblingDB(\"{db}\").{COLLECTION}.{method}(\"{}\")", self.name) + }; + match self.clash { + Clash::Renamed => format!( + "the unique index {} on {db}.{COLLECTION} ({}) is the index this migration \ + creates under the name {INDEX}, and the build refuses a second name for it; \ + drop it with {} in mongosh", + self.name, + self.spec, + command("dropIndex") + ), + Clash::Named => format!( + "the index {INDEX} on {db}.{COLLECTION} ({}) is not the unique index on \ + access_key_id that this migration creates under that name, and the build \ + refuses a second index of the name; drop it with {} in mongosh", + self.spec, + command("dropIndex") + ), + Clash::Hidden => format!( + "the index {INDEX} on {db}.{COLLECTION} ({}) is the unique index on \ + access_key_id that this migration creates, hidden from the query planner, so \ + a credential lookup does not use it; unhide it with {} in mongosh", + self.spec, + command("unhideIndex") + ), + } + } +} + +/// A Decimal128 as its sign, its digits without leading and trailing +/// zeros, and the power of ten of the last of them, or `None` for NaN and +/// the infinities. A zero has no digits. +fn decimal_parts(value: &Decimal128) -> Option<(bool, String, i64)> { + let text = value.to_string(); + let (negative, unsigned) = match text.strip_prefix('-') { + Some(rest) => (true, rest), + None => (false, text.as_str()), + }; + let (mantissa, exponent) = match unsigned.split_once('E') { + Some((mantissa, exponent)) => (mantissa, exponent.parse::().ok()?), + None => (unsigned, 0), + }; + let (whole, fraction) = mantissa.split_once('.').unwrap_or((mantissa, "")); + let digits = format!("{whole}{fraction}"); + if digits.is_empty() || !digits.bytes().all(|b| b.is_ascii_digit()) { + return None; + } + let digits = digits.trim_start_matches('0'); + let significant = digits.trim_end_matches('0'); + let trailing = digits.len() - significant.len(); + Some((negative, significant.to_owned(), exponent - fraction.len() as i64 + trailing as i64)) +} + +/// Whether the server counts `value` as true, as it reads an index option: +/// false, a zero, null and a missing value are false, and anything else is +/// true. +fn true_value(value: Option<&Bson>) -> bool { + match value { + None | Some(Bson::Null) | Some(Bson::Undefined) => false, + Some(Bson::Boolean(b)) => *b, + Some(Bson::Int32(n)) => *n != 0, + Some(Bson::Int64(n)) => *n != 0, + Some(Bson::Double(n)) => *n != 0.0, + Some(Bson::Decimal128(n)) => decimal_parts(n).map_or(true, |(_, digits, _)| !digits.is_empty()), + Some(_) => true, + } +} + +/// Whether `value` is the number 1, in any numeric type: the server +/// compares key directions by value, a Decimal128 exactly. +fn is_one(value: Option<&Bson>) -> bool { + match value { + Some(Bson::Int32(n)) => *n == 1, + Some(Bson::Int64(n)) => *n == 1, + Some(Bson::Double(n)) => *n == 1.0, + Some(Bson::Decimal128(n)) => decimal_parts(n) == Some((false, "1".to_owned(), 0)), + _ => false, + } +} + +/// Whether `spec` has the key `{ access_key_id: 1 }`. +fn on_the_id(spec: &Document) -> bool { + spec.get_document("key").map_or(false, |key| key.len() == 1 && is_one(key.get("access_key_id"))) +} + +/// The options of `spec` that the build compares: none of `v`, `key`, +/// `name`, `ns` and `background`, which has no effect, and `sparse` and +/// `hidden` only when true, because false is their default. +fn options(spec: &Document) -> Document { + let mut options = spec.clone(); + for field in ["v", "key", "name", "ns", "background"] { + options.remove(field); + } + for field in ["sparse", "hidden"] { + if !true_value(options.get(field)) { + options.remove(field); + } + } + options +} + +/// Whether `spec` is the index this migration creates. +fn is_ours(spec: &Document) -> bool { + let mut options = options(spec); + spec.get_str("name") == Ok(INDEX) + && on_the_id(spec) + && true_value(options.remove("unique").as_ref()) + && options.is_empty() +} + +/// `spec` without its `hidden` option. +fn unhidden(spec: &Document) -> Document { + let mut spec = spec.clone(); + spec.remove("hidden"); + spec +} + +/// The conflict `spec` makes, as `createIndexes` decides on MongoDB 7 and +/// 8: any index named `access_key_id_1` other than the one this migration +/// creates, and the unique index on the ID under another name. A collation, +/// a partial filter or `sparse: true` makes that unique index a different +/// one, which the build creates beside it. This migration's index, hidden, +/// is refused too: MongoDB 8 answers the build with IndexOptionsConflict, +/// and MongoDB 7 answers that the index exists. +fn conflict(spec: &Document) -> Option { + let name = spec.get_str("name").unwrap_or_default().to_owned(); + let mut shown = spec.clone(); + shown.remove("v"); + shown.remove("ns"); + if name == INDEX { + if is_ours(spec) { + return None; + } + let clash = if is_ours(&unhidden(spec)) { Clash::Hidden } else { Clash::Named }; + return Some(Conflict { name, spec: shown, clash }); + } + let options = options(spec); + let distinct = options.contains_key("collation") + || options.contains_key("partialFilterExpression") + || options.contains_key("sparse"); + (on_the_id(spec) && true_value(options.get("unique")) && !distinct) + .then(|| Conflict { name, spec: shown, clash: Clash::Renamed }) +} + +/// Whether an `explain` reply reports a forward collection scan: `LIMIT` +/// over `COLLSCAN` with the direction forward. The plan is the winning plan +/// itself, or its `queryPlan` when the slot-based engine runs it. Any other +/// plan or reply is not one. +fn forward_scan(reply: &Document) -> bool { + let Ok(winning) = reply.get_document("queryPlanner").and_then(|p| p.get_document("winningPlan")) else { + return false; + }; + let mut stage = winning.get_document("queryPlan").unwrap_or(winning); + loop { + match stage.get_str("stage") { + Ok("LIMIT") => match stage.get_document("inputStage") { + Ok(input) => stage = input, + Err(_) => return false, + }, + Ok("COLLSCAN") => return stage.get_str("direction") == Ok("forward"), + _ => return false, + } + } +} + +/// The copy a forward collection scan reads among `found`, an ID's listed +/// copies read with their record IDs: the copy with the lowest record ID. +/// A copy deleted and written again gets a new, higher record ID. +fn scanned_first(found: &[Document]) -> Scanned { + let mut first: Option<(i64, &Bson)> = None; + for copy in found { + let (Ok(record), Some(id)) = (copy.get_i64("$recordId"), copy.get("_id")) else { + return Scanned::Unordered; + }; + if first.map_or(true, |(lowest, _)| record < lowest) { + first = Some((record, id)); + } + } + first.map_or(Scanned::Gone, |(_, id)| Scanned::First(id.clone())) +} + +/// Whether the read needs to group only the documents whose ID is an +/// array: this migration's index, hidden or not, is built and refuses a +/// second copy of any other ID. +fn arrays_only(indexes: &[Listed]) -> bool { + indexes.iter().any(|i| i.built && is_ours(&unhidden(&i.spec))) +} + +/// The server's error code of a failed command, if it reported one. +fn command_code(error: &mongodb::error::Error) -> Option { + match error.kind.as_ref() { + mongodb::error::ErrorKind::Command(command) => Some(command.code), + _ => None, + } +} + +/// One entry of `listIndexes` with `includeIndexBuildInfo`: `{ spec }` for +/// a built index and `{ spec, indexBuildInfo }` for one whose build is +/// running. An entry without a `spec` is read as a spec whose build may be +/// running. +fn listed(entry: &Document) -> Listed { + match entry.get_document("spec") { + Ok(spec) => Listed { spec: spec.clone(), built: !entry.contains_key("indexBuildInfo") }, + Err(_) => Listed { spec: entry.clone(), built: false }, + } +} + +/// The indexes of the collection, or none when the collection does not +/// exist. +async fn indexes(stores: &MongoStores) -> Result, MigrationError> { + let db = &stores.catalog; + let command = doc! { + "listIndexes": COLLECTION, + "cursor": { "batchSize": 1000 }, + "includeIndexBuildInfo": true, + }; + let listed_reply = stores + .request(format!("list the indexes of {}.{COLLECTION}", db.name()), async { + match db.run_command(command.clone()).await { + Err(e) if command_code(&e) == Some(NAMESPACE_NOT_FOUND) => Ok(None), + other => other.map(Some), + } + }) + .await?; + let Some(reply) = listed_reply else { + return Ok(Vec::new()); + }; + let cursor = reply.get_document("cursor").map_err(|e| { + MigrationError::Store(format!("list the indexes of {}.{COLLECTION}: {e}", db.name())) + })?; + if cursor.get_i64("id").unwrap_or(0) != 0 { + return Err(MigrationError::Store(format!( + "list the indexes of {}.{COLLECTION}: the server returned them in more than one batch", + db.name() + ))); + } + Ok(cursor + .get_array("firstBatch") + .map(|batch| batch.iter().filter_map(Bson::as_document).map(listed).collect()) + .unwrap_or_default()) +} + +/// Every value of `access_key_id` that more than one document holds or +/// that is an array, sorted, with its documents in natural order; with +/// `arrays_only`, the array values alone, which needs no grouping of the +/// other documents. The `$natural` hint makes the scan a collection scan in +/// natural order, and `$push` keeps the order in which it meets the +/// documents. `allowDiskUse` lets `$group` spill when its state passes the +/// server's memory limit. +async fn groups(stores: &MongoStores, arrays_only: bool) -> Result, MigrationError> { + let db = &stores.catalog; + let collection = db.collection::(COLLECTION); + let mut pipeline = Vec::new(); + if arrays_only { + pipeline.push(doc! { "$match": { "access_key_id": { "$type": "array" } } }); + } + pipeline.push(doc! { "$group": { + "_id": "$access_key_id", + "count": { "$sum": 1 }, + "array": { "$max": { "$isArray": ["$access_key_id"] } }, + "copies": { "$push": { + "_id": "$_id", + "account_id": "$account_id", + "user_name": "$user_name", + "created_at": "$created_at", + } }, + } }); + if !arrays_only { + pipeline.push(doc! { "$match": { "$or": [{ "count": { "$gt": 1 } }, { "array": true }] } }); + } + pipeline.push(doc! { "$sort": { "_id": 1 } }); + let read = stores + .request(format!("group the documents of {}.{COLLECTION} by access key ID", db.name()), async { + let cursor = collection + .aggregate(pipeline.clone()) + .hint(Hint::Keys(doc! { "$natural": 1 })) + .allow_disk_use(true) + .await?; + cursor.try_collect::>().await + }) + .await?; + Ok(read + .into_iter() + .map(|group| Group { + key: group.get("_id").cloned().unwrap_or(Bson::Null), + array: group.get_bool("array").unwrap_or(false), + copies: group + .get_array("copies") + .map(|copies| { + copies + .iter() + .filter_map(Bson::as_document) + .map(|d| Stored { + id: d.get("_id").cloned().unwrap_or(Bson::Null), + account_id: d.get_str("account_id").ok().map(str::to_owned), + user_name: d.get_str("user_name").ok().map(str::to_owned), + created_at: d + .get_datetime("created_at") + .ok() + .and_then(|t| t.try_to_rfc3339_string().ok()), + }) + .collect() + }) + .unwrap_or_default(), + }) + .collect()) +} + +/// `explain` for a credential lookup's command with `id`: `find` on the ID +/// with `limit` 1 and `singleBatch`, the command the driver sends for the +/// lookup's `find_one`. On MongoDB 8 both fields are part of the query +/// shape that query settings name, so the command keeps them. +fn lookup_explain(id: &str) -> Document { + doc! { + "explain": { "find": COLLECTION, "filter": { "access_key_id": id }, "limit": 1, "singleBatch": true }, + "verbosity": "queryPlanner", + } +} + +/// The copy a credential lookup reads for `id`, with the `_id`s of `group`'s +/// copies that still hold the ID, among them the copy read; `None` when the +/// lookup reads none of them, because a copy was written or deleted since +/// the group was read. The body reads those copies with their record IDs and +/// sends `explain` for the lookup's command with this ID: a partial index +/// can give one ID another plan than the next. On a forward collection scan +/// the lookup reads the copy with the lowest record ID. With any other plan, +/// and when a record ID is not an Int64, the body sends the lookup's own +/// query, `find_one` on the ID with no sort, so the server picks the copy as +/// it does for the lookup. A copy that took the ID since the read is not +/// among those read; the index build fails on it. +async fn looked_up(stores: &MongoStores, id: &str, group: &Group) -> Result)>, MigrationError> { + let db = &stores.catalog; + let collection = db.collection::(COLLECTION); + let what = format!("read the copy of {id} a credential lookup reads in {}.{COLLECTION}", db.name()); + let listed = group.copies.iter().map(|c| c.id.clone()).collect::>(); + let filter = doc! { "_id": { "$in": listed }, "access_key_id": id }; + let held = stores + .request(&what, async { + let cursor = collection + .find(filter.clone()) + .projection(doc! { "_id": 1 }) + .show_record_id(true) + .await?; + cursor.try_collect::>().await + }) + .await?; + let held_ids = held.iter().filter_map(|d| d.get("_id").cloned()).collect::>(); + let command = lookup_explain(id); + let plan = stores + .request(format!("explain the credential lookup of {id} in {}.{COLLECTION}", db.name()), async { + db.run_command(command.clone()).await + }) + .await?; + if forward_scan(&plan) { + match scanned_first(&held) { + Scanned::First(read) => return Ok(Some((read, held_ids))), + Scanned::Gone => return Ok(None), + Scanned::Unordered => {} + } + } + let found = stores + .request(&what, async { collection.find_one(doc! { "access_key_id": id }).await }) + .await?; + Ok(found + .and_then(|d| d.get("_id").cloned()) + .filter(|read| held_ids.contains(read)) + .map(|read| (read, held_ids))) +} + +async fn read(stores: &MongoStores) -> Result { + let indexes = indexes(stores).await?; + Ok(State { + groups: groups(stores, arrays_only(&indexes)).await?, + conflicts: indexes.iter().filter_map(|i| conflict(&i.spec)).collect(), + }) +} + +/// The items `detect` and `run` refuse the migration for. +async fn refusals(stores: &MongoStores, state: &State) -> Result, MigrationError> { + let db = stores.catalog.name(); + let mut items = Vec::new(); + for group in state.groups.iter().filter(|g| !g.resolvable()) { + let read = match group.access_key_id() { + Some(id) => looked_up(stores, id, group).await?.map(|(read, _)| read), + None => None, + }; + items.push(group.refusal(db, read.as_ref())); + } + items.extend(state.conflicts.iter().map(|c| c.refusal(db))); + Ok(items) +} + +#[async_trait] +impl CodeBody for UniqueAccessKeyIds { + fn fingerprint(&self) -> &'static str { + FINGERPRINT + } + + async fn detect(&self, stores: &MongoStores) -> Result, MigrationError> { + let state = read(stores).await?; + refusals(stores, &state).await + } + + async fn run(&self, stores: &MongoStores) -> Result { + let db = &stores.catalog; + let collection = db.collection::(COLLECTION); + let state = read(stores).await?; + // `detect` refused these before the run; what was written since is + // refused the same way, before this body writes anything. + let items = refusals(stores, &state).await?; + if !items.is_empty() { + return Err(MigrationError::Status(CatalogStatus::NeedsAttention { items })); + } + let mut notes = Vec::new(); + for group in state.groups.iter().filter(|g| g.resolvable() && g.copies.len() > 1) { + let Some(id) = group.access_key_id() else { + continue; + }; + // A lookup that reads none of the listed copies, because a copy + // was written or deleted since the read: delete nothing for this + // ID. The build below fails on a copy written since, and the next + // run reads again. + let Some((read, held)) = looked_up(stores, id, group).await? else { + continue; + }; + // Only the listed copies that still hold the ID: a copy deleted + // or given another ID since the read is neither deleted nor + // named. + let (kept, others): (Vec<&Stored>, Vec<&Stored>) = + group.copies.iter().filter(|c| held.contains(&c.id)).partition(|c| c.id == read); + let [kept] = kept.as_slice() else { + continue; + }; + if others.is_empty() { + continue; + } + let owner = format!( + "access key {id} of account {}, user {}", + kept.account_id.as_deref().unwrap_or("(none)"), + kept.user_name.as_deref().unwrap_or("(none)"), + ); + let copies = match others.len() { + 1 => format!("the other copy {}", ids(&others)), + _ => format!("the other copies {}", ids(&others)), + }; + // The ledger records the notes after the body returns; this line + // names the copies when a run stops before then. + tracing::warn!( + "migration 6 unique_access_key_ids: {owner} is stored {} times in {}.{COLLECTION}: \ + deleting {copies} and keeping _id {}, the copy credential lookups read", + others.len() + 1, + db.name(), + kept.id, + ); + // Only those copies, and only while they hold the ID: a copy + // written since is not deleted, and the index build below fails + // on it. + let filter = doc! { + "_id": { "$in": others.iter().map(|c| c.id.clone()).collect::>() }, + "access_key_id": id, + }; + let result = stores + .request(format!("delete the other copies of {id} in {}.{COLLECTION}", db.name()), async { + collection.delete_many(filter.clone()).await + }) + .await?; + let deleted = match usize::try_from(result.deleted_count) { + Ok(n) if n == others.len() => copies, + _ => format!("{} of {copies}", result.deleted_count), + }; + notes.push(format!( + "{owner} was stored {} times: kept _id {}, the copy credential lookups read, and \ + deleted {deleted}", + others.len() + 1, + kept.id, + )); + } + // A copy written after the read above fails the build with a + // duplicate key error, and the runner records nothing. + let model = IndexModel::builder() + .keys(doc! { "access_key_id": 1 }) + .options(IndexOptions::builder().unique(true).build()) + .build(); + stores + .request(format!("create the unique index {INDEX} on {}.{COLLECTION}", db.name()), async { + collection + .create_index(model) + .commit_quorum(CommitQuorum::Majority) + .await + .map(|_| ()) + }) + .await?; + Ok(Done { notes }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn decimal(text: &str) -> Bson { + Bson::Decimal128(text.parse().unwrap_or_else(|e| panic!("{text}: {e:?}"))) + } + + fn ours() -> Document { + doc! { "v": 2, "key": { "access_key_id": 1 }, "name": INDEX, "unique": true } + } + + #[test] + fn a_decimal_key_direction_is_one_only_when_its_value_is_one() { + for text in ["1", "1.0", "1.000", "10E-1", "0.1E+1", "100E-2", "0.0001E+4"] { + assert!(is_one(Some(&decimal(text))), "{text}"); + } + for text in [ + "1.0000000000000000000000000000001", + "0.9999999999999999999999999999999", + "1E+1", + "1E-1", + "11", + "101E-2", + "-1", + "0", + "-0", + "NaN", + "Infinity", + "-Infinity", + ] { + assert!(!is_one(Some(&decimal(text))), "{text}"); + } + assert!(is_one(Some(&Bson::Int64(1))) && is_one(Some(&Bson::Double(1.0)))); + for value in [Bson::Int64(-1), Bson::Double(-1.0), Bson::Double(2.0), Bson::Double(1.000_000_000_000_000_2)] { + assert!(!is_one(Some(&value)), "{value}"); + } + } + + #[test] + fn a_decimal_option_is_true_unless_its_value_is_zero() { + for text in ["1", "-1", "1E-6176", "0.0000001", "NaN", "Infinity"] { + assert!(true_value(Some(&decimal(text))), "{text}"); + } + for text in ["0", "-0", "0E+10", "0.000"] { + assert!(!true_value(Some(&decimal(text))), "{text}"); + } + } + + #[test] + fn list_indexes_tells_a_built_index_from_one_whose_build_runs() { + let built = listed(&doc! { "spec": ours() }); + assert!(built.built); + assert_eq!(built.spec, ours()); + let building = listed(&doc! { + "indexBuildInfo": { + "buildUUID": bson::Binary { subtype: bson::spec::BinarySubtype::Uuid, bytes: vec![7; 16] }, + "method": "Hybrid", + "phase": 1, + "phaseStr": "collection scan", + "replicationState": { "state": "In progress" }, + }, + "spec": ours(), + }); + assert!(!building.built); + assert_eq!(building.spec, ours()); + let unmarked = listed(&ours()); + assert!(!unmarked.built); + assert_eq!(unmarked.spec, ours()); + } + + #[test] + fn only_a_built_index_of_this_migration_limits_the_read_to_array_ids() { + let hidden = doc! { "v": 2, "key": { "access_key_id": 1 }, "name": INDEX, "unique": true, "hidden": true }; + assert!(arrays_only(&[Listed { spec: ours(), built: true }])); + assert!(arrays_only(&[Listed { spec: hidden.clone(), built: true }])); + assert!(!arrays_only(&[Listed { spec: ours(), built: false }])); + assert!(!arrays_only(&[Listed { spec: hidden, built: false }])); + let mut plain = ours(); + plain.remove("unique"); + let mut renamed = ours(); + renamed.insert("name", "akid"); + assert!(!arrays_only(&[ + Listed { spec: plain, built: true }, + Listed { spec: renamed, built: true }, + ])); + assert!(!arrays_only(&[])); + } + + /// An `explain` reply around `winning`, the winning plan. + fn explained(winning: Document) -> Document { + doc! { "queryPlanner": { "namespace": "catalog.access_keys", "winningPlan": winning }, "ok": 1 } + } + + fn scan(direction: &str) -> Document { + doc! { + "stage": "LIMIT", + "limitAmount": 1, + "inputStage": { "stage": "COLLSCAN", "filter": { "access_key_id": { "$eq": "X" } }, "direction": direction }, + } + } + + #[test] + fn explain_reports_a_forward_scan_only_for_a_collection_scan_in_natural_order() { + // MongoDB 7.0.45 and 8.0.34 (which adds isCached), and 7.0.0, whose + // slot-based engine puts the stages under queryPlan. + assert!(forward_scan(&explained(scan("forward")))); + let mut cached = scan("forward"); + cached.insert("isCached", false); + assert!(forward_scan(&explained(cached))); + assert!(forward_scan(&explained(doc! { + "queryPlan": scan("forward"), + "slotBasedPlan": { "slots": "", "stages": "" }, + }))); + // Query settings that allow only { $natural: -1 } on MongoDB 8.0.34. + let mut backward = scan("backward"); + backward.insert("isCached", false); + assert!(!forward_scan(&explained(backward))); + assert!(!forward_scan(&explained(doc! { "queryPlan": scan("backward") }))); + // An index on access_key_id, on 7.0 and on 8.0. + assert!(!forward_scan(&explained(doc! { + "stage": "LIMIT", + "limitAmount": 1, + "inputStage": { "stage": "FETCH", "inputStage": { "stage": "IXSCAN", "indexName": "akid", "direction": "forward" } }, + }))); + assert!(!forward_scan(&explained(doc! { + "isCached": false, + "stage": "EXPRESS_IXSCAN", + "keyPattern": "{ access_key_id: 1, created_at: -1 }", + "indexName": "akid_newest_first", + }))); + assert!(!forward_scan(&explained(doc! { "stage": "COLLSCAN" }))); + assert!(!forward_scan(&explained(doc! { "stage": "LIMIT" }))); + assert!(!forward_scan(&explained(doc! { "stage": "EOF" }))); + assert!(!forward_scan(&doc! { "ok": 1 })); + } + + #[test] + fn the_explained_command_has_the_shape_of_the_lookups_find_one() { + // The server's lookup, as the profiler records it on 7.0.0, 7.0.45 + // and 8.0.34: { find, singleBatch: true, limit: 1, filter }. Query + // settings on 8.0 name a shape that includes limit and singleBatch. + assert_eq!( + lookup_explain("AKIAEXAMPLE"), + doc! { + "explain": { + "find": "access_keys", + "filter": { "access_key_id": "AKIAEXAMPLE" }, + "limit": 1, + "singleBatch": true, + }, + "verbosity": "queryPlanner", + } + ); + } + + #[test] + fn a_forward_scan_reads_the_copy_with_the_lowest_record_id() { + let copy = |id: i32, record: Bson| doc! { "_id": id, "$recordId": record }; + // Copy 1 was deleted and written again after copy 2, so its record + // ID is the higher one. + assert_eq!( + scanned_first(&[copy(1, Bson::Int64(4)), copy(2, Bson::Int64(2)), copy(3, Bson::Int64(3))]), + Scanned::First(Bson::Int32(2)) + ); + assert_eq!(scanned_first(&[copy(5, Bson::Int64(9))]), Scanned::First(Bson::Int32(5))); + assert_eq!(scanned_first(&[]), Scanned::Gone); + assert_eq!( + scanned_first(&[copy(1, Bson::Int64(1)), copy(2, Bson::String("k".to_owned()))]), + Scanned::Unordered + ); + assert_eq!(scanned_first(&[doc! { "$recordId": 1_i64 }]), Scanned::Unordered); + } + + #[test] + fn this_migrations_index_hidden_is_refused_with_the_command_that_unhides_it() { + let hidden = doc! { "v": 2, "key": { "access_key_id": 1 }, "name": INDEX, "unique": true, "hidden": true }; + let refused = conflict(&hidden).expect("refused"); + assert!(matches!(refused.clash, Clash::Hidden)); + assert_eq!( + refused.refusal("cat"), + "the index access_key_id_1 on cat.access_keys ({ \"key\": { \"access_key_id\": 1 }, \ + \"name\": \"access_key_id_1\", \"unique\": true, \"hidden\": true }) is the unique \ + index on access_key_id that this migration creates, hidden from the query planner, \ + so a credential lookup does not use it; unhide it with \ + db.getSiblingDB(\"cat\").access_keys.unhideIndex(\"access_key_id_1\") in mongosh" + ); + let mut hidden_plain = hidden.clone(); + hidden_plain.remove("unique"); + assert!(matches!(conflict(&hidden_plain).map(|c| c.clash), Some(Clash::Named))); + let mut shown = hidden.clone(); + shown.insert("hidden", false); + assert!(conflict(&shown).is_none()); + } +} diff --git a/crates/storage-mongodb/src/migrations/mod.rs b/crates/storage-mongodb/src/migrations/mod.rs index 9a3c88598..851a0d86c 100644 --- a/crates/storage-mongodb/src/migrations/mod.rs +++ b/crates/storage-mongodb/src/migrations/mod.rs @@ -38,6 +38,9 @@ mod m0001_baseline; #[rustfmt::skip] #[allow(clippy::all, deprecated)] mod m0005_drop_continuous_backups; +#[rustfmt::skip] +#[allow(clippy::all, deprecated)] +mod m0006_unique_access_key_ids; mod postcheck; use std::time::Duration; @@ -137,6 +140,19 @@ pub fn migration(id: MigrationId) -> Migration { m0005_drop_continuous_backups::SOURCE, )], }, + MigrationId::UniqueAccessKeyIds => Migration::Applies { + // A unique index is never additive (CONTRIBUTING.md): the + // previous release does not expect the index to refuse a write. + kind: Kind::Breaking, + // Once the index is built no ID has a second copy, so running + // the body again deletes nothing and keeps the index. + convergent: true, + bodies: vec![StoreBody::code( + StoreKind::Catalog, + &m0006_unique_access_key_ids::UniqueAccessKeyIds, + m0006_unique_access_key_ids::SOURCE, + )], + }, } } diff --git a/crates/storage-mongodb/tests/fixtures/golden/0006_unique_access_key_ids.txt b/crates/storage-mongodb/tests/fixtures/golden/0006_unique_access_key_ids.txt new file mode 100644 index 000000000..431c25599 --- /dev/null +++ b/crates/storage-mongodb/tests/fixtures/golden/0006_unique_access_key_ids.txt @@ -0,0 +1,22 @@ +{ + "access_keys": [ + { + "_id": "00000000000000000000000b", + "access_key_id": "AKIAIOSFODNN7EXAMPLE", + "account_id": "123456789012", + "user_name": "ci", + "created_at": "2026-10-29T23:53:08.287Z" + }, + { + "_id": "6ae3dbec99e7d94eaa5c300d", + "access_key_id": "AKIAEXTENDDBAIQC4XUG", + "account_id": "123456789012", + "user_name": "cu-tester", + "created_at": "2026-10-29T23:51:08.287Z" + } + ], + "notes": [ + "access key AKIAEXTENDDBAIQC4XUG of account 123456789012, user cu-tester was stored 3 times: kept _id ObjectId(\"6ae3dbec99e7d94eaa5c300d\"), the copy credential lookups read, and deleted the other copies _id ObjectId(\"000000000000000000000001\"), _id ObjectId(\"000000000000000000000002\")", + "access key AKIAIOSFODNN7EXAMPLE of account 123456789012, user ci was stored 3 times: kept _id ObjectId(\"00000000000000000000000b\"), the copy credential lookups read, and deleted the other copies _id ObjectId(\"00000000000000000000000c\"), _id ObjectId(\"00000000000000000000000d\")" + ] +} diff --git a/crates/storage-mongodb/tests/migrations_fresh.rs b/crates/storage-mongodb/tests/migrations_fresh.rs index 12cf1838e..7e560317c 100644 --- a/crates/storage-mongodb/tests/migrations_fresh.rs +++ b/crates/storage-mongodb/tests/migrations_fresh.rs @@ -16,7 +16,7 @@ use extenddb_storage::migrations::{ use extenddb_storage_mongodb::migrations::{ Definitions, MigrationLock, MongoMigrations, Timing, describe_store, lock, }; -use migrations_support::{Deployment, server}; +use migrations_support::{ACCESS_KEY_ID_INDEX, Deployment, catalog_after_every_migration, server}; const CATALOG_EXPECTED: &str = include_str!("../migrations/catalog/0001_baseline.expected.json"); const DATA_EXPECTED: &str = include_str!("../migrations/data/0001_baseline.expected.json"); @@ -303,7 +303,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { let Some(uri) = server() else { return }; let d = Deployment::new(&uri, "fresh").await; let report = d.create().await; - assert_eq!((report.from, report.to), (None, Some(5))); + assert_eq!((report.from, report.to), (None, Some(6))); let applied: Vec<(u32, String)> = report .applied .iter() @@ -318,6 +318,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { (3, d.target.catalog.clone()), (4, d.target.catalog.clone()), (5, d.target.catalog.clone()), + (6, d.target.catalog.clone()), ] ); let not_applicable: Vec = report @@ -327,7 +328,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { .map(|s| s.id) .collect(); assert_eq!(not_applicable, [3, 4], "{report:?}"); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); let rows = d.catalog_ledger().await; let outcomes: Vec<(u32, Outcome)> = rows.iter().map(|r| (r.id, r.outcome)).collect(); assert_eq!( @@ -338,6 +339,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { (3, Outcome::NotApplicable), (4, Outcome::NotApplicable), (5, Outcome::Applied), + (6, Outcome::Applied), ] ); assert!( @@ -347,7 +349,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { let data_rows = Deployment::documents(&d.data(), "extenddb_migrations").await; assert_eq!(data_rows.len(), 1); assert_eq!(data_rows[0].get_i64("_id"), Ok(1)); - assert_eq!(d.setting("catalog_version").await.as_deref(), Some("5")); + assert_eq!(d.setting("catalog_version").await.as_deref(), Some("6")); assert_eq!(d.setting("resource_tag_migration").await, None); let history = Deployment::documents(&d.catalog(), "schema_history").await; assert_eq!(history.len(), 1); @@ -356,12 +358,16 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { let (catalog, data) = definitions(&d).await; if std::env::var("EXTENDDB_BLESS_EXPECTED").as_deref() == Ok("1") { let dir = env!("CARGO_MANIFEST_DIR"); - // Migration 1's file keeps the collection migration 5 drops. + // Migration 1's file keeps the collection migration 5 drops and + // lacks the index migration 6 creates. let committed: Definitions = serde_json::from_str(CATALOG_EXPECTED).expect("catalog"); let mut catalog_at_1 = catalog.clone(); if let Some(dropped) = committed.get("continuous_backups") { catalog_at_1.insert("continuous_backups".to_owned(), dropped.clone()); } + if let Some(access_keys) = catalog_at_1.get_mut("access_keys") { + access_keys.indexes.remove(ACCESS_KEY_ID_INDEX); + } for (path, defs) in [("catalog", &catalog_at_1), ("data", &data)] { let text = serde_json::to_string_pretty(defs).expect("json") + "\n"; std::fs::write( @@ -371,12 +377,8 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { .expect("write"); } } - let mut expected_catalog: Definitions = - serde_json::from_str(CATALOG_EXPECTED).expect("catalog"); - // Migration 5 drops continuous_backups, which migration 1 creates. - expected_catalog.remove("continuous_backups"); let expected_data: Definitions = serde_json::from_str(DATA_EXPECTED).expect("data"); - assert_eq!(catalog, expected_catalog); + assert_eq!(catalog, catalog_after_every_migration()); assert_eq!(data, expected_data); d.drop_databases().await; } @@ -389,12 +391,12 @@ async fn a_second_run_applies_nothing_and_reconverge_keeps_the_rows() { let before = d.catalog_ledger().await; let again = d.migrate().await.expect("migrate"); assert!(again.applied.is_empty()); - assert_eq!((again.from, again.to), (Some(5), Some(5))); + assert_eq!((again.from, again.to), (Some(6), Some(6))); let reconverged = d.reconverge().await.expect("reconverge"); - // Migration 1's steps in the catalog and the data database, and migration - // 5's in the catalog: 2 is not convergent, and 3 and 4 do not apply on - // MongoDB. + // Migration 1's steps in the catalog and the data database, and the + // steps of migrations 5 and 6 in the catalog: 2 is not convergent, and 3 + // and 4 do not apply on MongoDB. let ran: Vec<(u32, String)> = reconverged .applied .iter() @@ -406,6 +408,7 @@ async fn a_second_run_applies_nothing_and_reconverge_keeps_the_rows() { (1, d.target.catalog.clone()), (1, d.target.data.clone()), (5, d.target.catalog.clone()), + (6, d.target.catalog.clone()), ], "{reconverged:?}" ); @@ -421,7 +424,7 @@ async fn a_second_run_applies_nothing_and_reconverge_keeps_the_rows() { before, "reconverge leaves the rows as they are" ); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); d.drop_databases().await; } @@ -456,7 +459,7 @@ async fn forget_clears_an_unreleased_id_and_migrate_applies_it_again() { assert_eq!(pending.iter().map(|p| p.id).collect::>(), [1, 1]); let report = d.migrate().await.expect("migrate"); assert_eq!(report.applied.len(), 2); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); d.drop_databases().await; } @@ -473,19 +476,19 @@ async fn a_changed_checksum_a_newer_catalog_and_an_older_release_are_reported() doc! { "_id": id, "name": "later", "kind": kind, "checksum": "c", "outcome": "applied", "notes": "", "applied_at": bson::DateTime::now(), "applied_by": "9.9.9" } }; - ledger.insert_one(row(6, "breaking")).await.expect("insert"); + ledger.insert_one(row(7, "breaking")).await.expect("insert"); let status = d.status().await; assert_eq!(status.label(), "AheadIncompatible"); assert_eq!(status.exit_code(), exit::AHEAD_INCOMPATIBLE); ledger - .delete_one(doc! { "_id": 6_i64 }) + .delete_one(doc! { "_id": 7_i64 }) .await .expect("delete"); - ledger.insert_one(row(6, "additive")).await.expect("insert"); + ledger.insert_one(row(7, "additive")).await.expect("insert"); assert_eq!(d.status().await.label(), "AheadCompatible"); assert!(d.migrate().await.expect("migrate").applied.is_empty()); ledger - .delete_one(doc! { "_id": 6_i64 }) + .delete_one(doc! { "_id": 7_i64 }) .await .expect("delete"); @@ -500,7 +503,7 @@ async fn a_changed_checksum_a_newer_catalog_and_an_older_release_are_reported() assert_eq!(refused.exit_code(), exit::ATTENTION); d.reconverge().await.expect("reconverge"); assert_eq!(d.setting("resource_tag_migration").await, None); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); settings .update_one( doc! { "_id": "catalog_version" }, @@ -510,7 +513,7 @@ async fn a_changed_checksum_a_newer_catalog_and_an_older_release_are_reported() .expect("version"); assert_eq!(d.status().await.label(), "WrittenByOlderRelease"); d.reconverge().await.expect("reconverge"); - assert_eq!(d.setting("catalog_version").await.as_deref(), Some("5")); + assert_eq!(d.setting("catalog_version").await.as_deref(), Some("6")); // A recorded checksum this binary's body does not have. ledger @@ -558,6 +561,6 @@ async fn a_ledger_document_this_binary_cannot_read_stops_the_status() { .expect_err("an error"); migrations.close().await; assert!(error.to_string().contains("\"sideways\""), "{error}"); - assert_eq!(MigrationId::LATEST.number(), 5); + assert_eq!(MigrationId::LATEST.number(), 6); d.drop_databases().await; } diff --git a/crates/storage-mongodb/tests/migrations_guards.rs b/crates/storage-mongodb/tests/migrations_guards.rs index 793f65361..6b85f8338 100644 --- a/crates/storage-mongodb/tests/migrations_guards.rs +++ b/crates/storage-mongodb/tests/migrations_guards.rs @@ -14,9 +14,8 @@ use extenddb_storage::migrations::{CatalogStatus, MigrationBackend, Runner}; use extenddb_storage_mongodb::migrations::{ Definitions, MigrationLock, MongoMigrations, Timing, describe_store, lock, }; -use migrations_support::{Deployment, fixture, server}; +use migrations_support::{Deployment, catalog_after_every_migration, fixture, server}; -const CATALOG_EXPECTED: &str = include_str!("../migrations/catalog/0001_baseline.expected.json"); const DATA_EXPECTED: &str = include_str!("../migrations/data/0001_baseline.expected.json"); async fn index_names(db: &mongodb::Database, collection: &str) -> Vec { @@ -65,9 +64,7 @@ async fn assert_fresh_definitions(d: &Deployment) { .await .expect("data"); migrations.close().await; - let mut expected_catalog: Definitions = serde_json::from_str(CATALOG_EXPECTED).expect("json"); - // Migration 5 drops continuous_backups, which migration 1 creates. - expected_catalog.remove("continuous_backups"); + let expected_catalog = catalog_after_every_migration(); assert!(!catalog.contains_key("continuous_backups")); let expected_data: Definitions = serde_json::from_str(DATA_EXPECTED).expect("json"); for (name, want) in &expected_catalog { @@ -207,7 +204,7 @@ async fn migrate_creates_the_indexes_a_deployment_lacks() { .await .contains(&"table_id_1".to_owned()) ); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); assert_fresh_definitions(&d).await; d.drop_databases().await; } @@ -232,7 +229,7 @@ async fn stop_at_every_request(uri: &str, state: Option<&str>) -> usize { } assert_eq!( d.status().await, - CatalogStatus::Current { version: 5 }, + CatalogStatus::Current { version: 6 }, "{requests}" ); assert_fresh_definitions(&d).await; diff --git a/crates/storage-mongodb/tests/migrations_holders.rs b/crates/storage-mongodb/tests/migrations_holders.rs index ed0cd866c..5eba917e5 100644 --- a/crates/storage-mongodb/tests/migrations_holders.rs +++ b/crates/storage-mongodb/tests/migrations_holders.rs @@ -113,7 +113,7 @@ async fn migration_2_keeps_the_lock_through_a_scan_longer_than_the_idle_limit() applied.expect("every migration applies"); // A shorter run would apply without the progress the scan records. assert!(took > timing.idle_limit * 4, "the run took {took:?}"); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); assert!(d.catalog_ledger().await.iter().any(|row| row.id == 2)); d.drop_databases().await; } @@ -151,7 +151,7 @@ async fn the_status_read_under_the_lock_keeps_it_through_a_long_scan() { "the status read took {took:?}" ); applied.expect("every migration applies"); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); d.drop_databases().await; } diff --git a/crates/storage-mongodb/tests/migrations_locks.rs b/crates/storage-mongodb/tests/migrations_locks.rs index 84727523a..3f249d638 100644 --- a/crates/storage-mongodb/tests/migrations_locks.rs +++ b/crates/storage-mongodb/tests/migrations_locks.rs @@ -53,10 +53,10 @@ async fn two_migrators_started_together_apply_once_and_the_other_finds_nothing() applied.sort_unstable(); assert_eq!( applied, - [0, 6], + [0, 7], "one applies every step and the other finds nothing" ); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); d.drop_databases().await; } @@ -204,7 +204,7 @@ async fn migrate_waits_for_a_running_server_and_a_server_refuses_while_a_migrati .is_none() ); // The preview and verify read the status beside the server. - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); serving.release().await; let (held, _) = lock(&d.target, false, Duration::from_secs(2), Timing::PRODUCTION) .await diff --git a/crates/storage-mongodb/tests/migrations_released.rs b/crates/storage-mongodb/tests/migrations_released.rs index c636de7a9..25349ab7d 100644 --- a/crates/storage-mongodb/tests/migrations_released.rs +++ b/crates/storage-mongodb/tests/migrations_released.rs @@ -5,21 +5,24 @@ //! 0.1.12, 0.1.13 and 0.1.14 images (0.1.12 stands for v0.1.4 to v0.1.11, //! whose bootstrapper is the same), and `dotted-ttl`, a 0.1.12 catalog with //! the TTL index releases before v0.1.12 created for a dotted attribute. -//! Each is compared after the upgrade with the definitions migration 1 -//! creates, and its per-table objects with what they were before, and the +//! Each is compared after the upgrade with the definitions of a fresh +//! install, and its per-table objects with what they were before, and the //! code bodies' output with committed golden files. mod migrations_support; use std::collections::BTreeMap; -use bson::{Document, doc}; -use extenddb_storage::migrations::{CatalogStatus, MigrationBackend}; +use bson::oid::ObjectId; +use bson::spec::BinarySubtype; +use bson::{Binary, Bson, Document, doc}; +use extenddb_storage::migrations::{CatalogStatus, MigrationBackend, exit}; use extenddb_storage_mongodb::migrations::{Definitions, MongoMigrations, describe_store}; -use migrations_support::{Deployment, fixture, server}; +use migrations_support::{ + ACCESS_KEY_ID_INDEX, Deployment, catalog_after_every_migration, fixture, server, +}; use serde_json::Value; -const CATALOG_EXPECTED: &str = include_str!("../migrations/catalog/0001_baseline.expected.json"); const DATA_EXPECTED: &str = include_str!("../migrations/data/0001_baseline.expected.json"); const STATES: [&str; 4] = ["0.1.12", "0.1.13", "0.1.14", "dotted-ttl"]; @@ -136,9 +139,7 @@ fn golden(name: &str, value: &Value) { #[tokio::test(flavor = "multi_thread")] async fn every_released_catalog_upgrades_to_the_definitions_of_a_fresh_install() { let Some(uri) = server() else { return }; - let mut expected_catalog: Definitions = serde_json::from_str(CATALOG_EXPECTED).unwrap(); - // Migration 5 drops continuous_backups, which migration 1 creates. - expected_catalog.remove("continuous_backups"); + let expected_catalog = catalog_after_every_migration(); let expected_data: Definitions = serde_json::from_str(DATA_EXPECTED).unwrap(); let fresh = Deployment::new(&uri, "freshkeys").await; fresh.create().await; @@ -164,12 +165,12 @@ async fn every_released_catalog_upgrades_to_the_definitions_of_a_fresh_install() panic!("{state} is not behind"); }; assert_eq!(version, None, "{state} has no ledger"); - assert_eq!(pending.len(), 6, "{state}"); + assert_eq!(pending.len(), 7, "{state}"); let report = d.migrate().await.expect("migrate"); - assert_eq!((report.from, report.to), (None, Some(5)), "{state}"); + assert_eq!((report.from, report.to), (None, Some(6)), "{state}"); assert_eq!( d.status().await, - CatalogStatus::Current { version: 5 }, + CatalogStatus::Current { version: 6 }, "{state}" ); @@ -401,10 +402,1149 @@ async fn a_reconverge_leaves_policies_written_after_migration_2_as_they_are() { .any(|n| n.starts_with("Skipped 2 resource_tag_condition_keys:")), "{report:?}" ); - assert_eq!(d.status().await, CatalogStatus::Current { version: 5 }); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); assert_eq!( documents(Deployment::documents(&d.catalog(), "iam_policies").await), before ); d.drop_databases().await; } + +/// The access key ID of AWS's documented example pair. +const EXAMPLE_KEY_ID: &str = "AKIAIOSFODNN7EXAMPLE"; + +/// The released fixture's one access key: its `_id`, ID, account and +/// creation time. +async fn fixture_key(d: &Deployment) -> (Bson, String, String, i64) { + let key = Deployment::documents(&d.catalog(), "access_keys") + .await + .pop() + .expect("the fixture's access key"); + ( + key.get("_id").cloned().unwrap(), + key.get_str("access_key_id").unwrap().to_owned(), + key.get_str("account_id").unwrap().to_owned(), + key.get_datetime("created_at").unwrap().timestamp_millis(), + ) +} + +/// An access key document as a release before 1.0 stored it, with an `_id` +/// and a creation time the test chooses, and a secret that tells the copies +/// apart. +fn stored_key(n: u8, access_key_id: &str, account: &str, user: &str, created_ms: i64) -> Document { + doc! { + "_id": oid(n), + "access_key_id": access_key_id, + "account_id": account, + "user_name": user, + "secret_key_encrypted": Binary { subtype: BinarySubtype::Generic, bytes: vec![n; 60] }, + "is_active": true, + "created_at": bson::DateTime::from_millis(created_ms), + } +} + +/// The `_id` of the document the server's credential lookup reads for +/// `access_key_id`, by the lookup's own query. +async fn looked_up(d: &Deployment, access_key_id: &str) -> Bson { + d.catalog() + .collection::("access_keys") + .find_one(doc! { "access_key_id": access_key_id }) + .await + .unwrap() + .expect("a key") + .get("_id") + .cloned() + .unwrap() +} + +async fn access_key_indexes(d: &Deployment) -> Vec { + d.catalog() + .collection::("access_keys") + .list_index_names() + .await + .unwrap() +} + +/// Migration 6's body on the oldest released catalog, after the fixture's +/// user imported its access key ID twice more and the user `ci` imported +/// the example pair three times: a release before 1.0 stored a document for +/// each import. Each ID keeps the copy a credential lookup read before the +/// upgrade: the first in natural order, which for the fixture's key is +/// neither the oldest copy nor the one with the lowest `_id`. The unique +/// index then refuses another copy. The committed golden file pins the keys +/// and the notes. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_keeps_the_copy_each_lookup_read_as_the_golden_file_records() { + use futures::TryStreamExt; + use mongodb::error::{ErrorKind, WriteFailure}; + + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "golden6").await; + fixture::load(&d, &fixture::read("0.1.12")).await; + let (fixture_doc, fixture_id, account, created) = fixture_key(&d).await; + let keys = d.catalog().collection::("access_keys"); + keys.insert_many([ + stored_key(1, &fixture_id, &account, "cu-tester", created - 60_000), + stored_key(2, &fixture_id, &account, "cu-tester", created + 60_000), + stored_key(11, EXAMPLE_KEY_ID, &account, "ci", created + 120_000), + stored_key(12, EXAMPLE_KEY_ID, &account, "ci", created + 180_000), + stored_key(13, EXAMPLE_KEY_ID, &account, "ci", created + 240_000), + ]) + .await + .unwrap(); + let ids = [fixture_id.as_str(), EXAMPLE_KEY_ID]; + let mut read_before = Vec::new(); + for id in ids { + read_before.push(looked_up(&d, id).await); + } + assert_eq!(read_before[0], fixture_doc); + + // The profiler records the body's requests on the catalog: with only + // migration 1's indexes the lookup is a forward scan, so the body reads + // record IDs and sends no lookup query of its own. + let catalog = d.catalog(); + catalog + .run_command(doc! { "create": "system.profile", "capped": true, "size": 16 << 20 }) + .await + .unwrap(); + catalog.run_command(doc! { "profile": 2 }).await.unwrap(); + d.migrate().await.expect("migrate"); + catalog.run_command(doc! { "profile": 0 }).await.unwrap(); + let profiled = catalog.collection::("system.profile"); + let count = |filter: Document| { + let profiled = profiled.clone(); + async move { profiled.count_documents(filter).await.unwrap() } + }; + assert_eq!( + count(doc! { + "command.find": "access_keys", + "command.filter._id": { "$exists": false }, + "command.filter.access_key_id": { "$exists": true }, + }) + .await, + 0, + "the body sent a lookup query of its own" + ); + assert_eq!( + count(doc! { "command.find": "access_keys", "command.showRecordId": true }).await, + ids.len() as u64 + ); + assert_eq!( + count(doc! { "command.explain.find": "access_keys" }).await, + ids.len() as u64 + ); + catalog + .collection::("system.profile") + .drop() + .await + .unwrap(); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); + for (id, kept) in ids.iter().zip(&read_before) { + let copies: Vec = keys + .find(doc! { "access_key_id": *id }) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert_eq!(copies.len(), 1, "{id}: {copies:?}"); + assert_eq!(copies[0].get("_id"), Some(kept), "{id}"); + assert_eq!(&looked_up(&d, id).await, kept, "{id}"); + } + assert!( + access_key_indexes(&d) + .await + .contains(&ACCESS_KEY_ID_INDEX.to_owned()) + ); + let refused = keys + .insert_one(stored_key(3, &fixture_id, &account, "cu-tester", created)) + .await + .expect_err("the index refuses another copy"); + assert!( + matches!(*refused.kind, ErrorKind::Write(WriteFailure::WriteError(ref e)) if e.code == 11000), + "{refused}" + ); + + let stored: Vec = Deployment::documents(&d.catalog(), "access_keys") + .await + .iter() + .map(|k| { + serde_json::json!({ + "_id": k.get_object_id("_id").unwrap().to_hex(), + "access_key_id": k.get_str("access_key_id").unwrap(), + "account_id": k.get_str("account_id").unwrap(), + "user_name": k.get_str("user_name").unwrap(), + "created_at": k.get_datetime("created_at").unwrap().try_to_rfc3339_string().unwrap(), + }) + }) + .collect(); + let notes: Vec = d + .catalog_ledger() + .await + .iter() + .find(|row| row.id == 6) + .expect("migration 6's row") + .notes + .lines() + .map(str::to_owned) + .collect(); + golden( + "0006_unique_access_key_ids", + &serde_json::json!({ "access_keys": stored, "notes": notes }), + ); + d.drop_databases().await; +} + +/// Migration 6's `detect` refuses, before any migration writes, an ID that +/// another user of the account holds, an ID that users of one name in two +/// accounts hold, two documents without an ID, and a document whose ID is +/// an array: the status reads NeedsAttention, `migrate --yes` exits 5 and +/// names every copy, and every document is kept, the user `ci`'s two copies +/// of the example pair included, which the body itself would resolve. Once +/// the documents the refusal names are deleted, `migrate --yes` applies +/// every migration. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_refuses_copies_it_cannot_resolve_and_writes_nothing() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "refuse6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + let (owner, fixture_id, account, created) = fixture_key(&d).await; + let keys = d.catalog().collection::("access_keys"); + let without_id = |n: u8| { + let mut key = stored_key(n, "", &account, "broken", created); + key.remove("access_key_id"); + key + }; + let mut array = stored_key(31, "", &account, "listed", created); + array.insert("access_key_id", ARRAY_KEY_IDS.map(Bson::from).to_vec()); + keys.insert_many([ + stored_key(1, &fixture_id, &account, "importer", created + 60_000), + stored_key(2, TWO_ACCOUNTS_KEY_ID, &account, "shared", created + 30_000), + stored_key( + 3, + TWO_ACCOUNTS_KEY_ID, + OTHER_ACCOUNT, + "shared", + created + 90_000, + ), + stored_key(11, EXAMPLE_KEY_ID, &account, "ci", created + 120_000), + stored_key(12, EXAMPLE_KEY_ID, &account, "ci", created + 180_000), + without_id(21), + without_id(22), + array, + ]) + .await + .unwrap(); + let documents = Deployment::documents(&d.catalog(), "access_keys").await; + let settings = Deployment::documents(&d.catalog(), "settings").await; + + let CatalogStatus::NeedsAttention { items } = d.status().await else { + panic!("migration 6 is not refused"); + }; + assert_eq!(items.len(), 4, "{items:?}"); + let error = d.migrate().await.expect_err("refused"); + assert_eq!(error.exit_code(), exit::ATTENTION); + let message = error.to_string(); + let catalog = &d.target.catalog; + let delete = + format!("db.getSiblingDB(\"{catalog}\").access_keys.deleteOne({{ _id: }})"); + for part in [ + format!( + "migration 6 unique_access_key_ids: the access key ID {fixture_id} is stored 2 \ + times in {catalog}.access_keys, for more than one user: _id {owner} (account \ + {account}, user cu-tester, created " + ), + format!( + "), _id {} (account {account}, user importer, created ", + Bson::ObjectId(oid(1)) + ), + format!( + "), and a credential lookup reads _id {owner}; delete each copy that is not the key \ + owner's with {delete} in mongosh" + ), + format!( + "migration 6 unique_access_key_ids: the access key ID {TWO_ACCOUNTS_KEY_ID} is \ + stored 2 times in {catalog}.access_keys, for more than one user: _id {} (account \ + {account}, user shared, created ", + Bson::ObjectId(oid(2)) + ), + format!( + "), _id {} (account {OTHER_ACCOUNT}, user shared, created ", + Bson::ObjectId(oid(3)) + ), + format!( + "), and a credential lookup reads _id {}; delete each copy", + Bson::ObjectId(oid(2)) + ), + format!( + "migration 6 unique_access_key_ids: 2 documents in {catalog}.access_keys have no \ + access key ID" + ), + format!( + "migration 6 unique_access_key_ids: the access key ID of _id {} (account \ + {account}, user listed, created ", + Bson::ObjectId(oid(31)) + ), + format!( + ") in {catalog}.access_keys is the array [\"{}\", \"{}\"], and a credential lookup \ + reads such a document for each ID in the array; delete it with {delete}, or set \ + its access_key_id to one string, in mongosh", + ARRAY_KEY_IDS[0], ARRAY_KEY_IDS[1] + ), + ] { + assert!(message.contains(&part), "{part}\n{message}"); + } + assert!(!message.contains(EXAMPLE_KEY_ID), "{message}"); + assert_eq!( + Deployment::documents(&d.catalog(), "access_keys").await, + documents + ); + assert_eq!( + Deployment::documents(&d.catalog(), "settings").await, + settings + ); + assert!( + Deployment::documents(&d.catalog(), "extenddb_migrations") + .await + .is_empty() + ); + assert_eq!(unique_index(&d).await, None); + + // What the refusal asks for: the importer's copy, the copy of the other + // account, one of the two documents without an ID, which the index + // takes one of, and the array. + for n in [1, 3, 22, 31] { + keys.delete_one(doc! { "_id": oid(n) }).await.unwrap(); + } + d.migrate().await.expect("migrate"); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); + assert_eq!(looked_up(&d, &fixture_id).await, owner); + for (filter, count) in [ + (doc! { "access_key_id": &fixture_id }, 1), + (doc! { "access_key_id": TWO_ACCOUNTS_KEY_ID }, 1), + (doc! { "access_key_id": EXAMPLE_KEY_ID }, 1), + (doc! { "access_key_id": Bson::Null }, 1), + ] { + assert_eq!( + keys.count_documents(filter.clone()).await.unwrap(), + count, + "{filter}" + ); + } + assert_eq!(unique_index(&d).await, Some(true)); + d.drop_databases().await; +} + +/// An index of `access_keys` that would make the build fail is refused +/// before anything is written: `access_key_id_1` without `unique`, which an +/// operator may have created for the lookups, and the unique index under +/// another name. Each refusal prints its `dropIndex` command, and once it +/// has run `migrate --yes` applies migration 6. A non-unique index on the +/// ID under another name stays beside the new one. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_refuses_an_index_the_build_would_fail_on() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "index6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + let (owner, fixture_id, account, created) = fixture_key(&d).await; + let keys = d.catalog().collection::("access_keys"); + keys.insert_one(stored_key( + 1, + &fixture_id, + &account, + "cu-tester", + created + 60_000, + )) + .await + .unwrap(); + keys.create_index(id_index(None, false)).await.unwrap(); + let documents = Deployment::documents(&d.catalog(), "access_keys").await; + let catalog = d.target.catalog.clone(); + let item = refused_index(&d).await; + assert!( + item.starts_with(&format!( + "migration 6 unique_access_key_ids: the index access_key_id_1 on \ + {catalog}.access_keys (" + )) && item.ends_with(&format!( + ") is not the unique index on access_key_id that this migration creates under \ + that name, and the build refuses a second index of the name; drop it with \ + db.getSiblingDB(\"{catalog}\").access_keys.dropIndex(\"access_key_id_1\") in \ + mongosh" + )), + "{item}" + ); + assert_eq!( + d.migrate().await.expect_err("refused").exit_code(), + exit::ATTENTION + ); + assert_eq!( + Deployment::documents(&d.catalog(), "access_keys").await, + documents + ); + keys.drop_index(ACCESS_KEY_ID_INDEX).await.unwrap(); + d.migrate().await.expect("migrate"); + assert_eq!(looked_up(&d, &fixture_id).await, owner); + assert_eq!( + keys.count_documents(doc! { "access_key_id": &fixture_id }) + .await + .unwrap(), + 1 + ); + assert_eq!(unique_index(&d).await, Some(true)); + d.drop_databases().await; + + let d = Deployment::new(&uri, "renamed6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + let keys = d.catalog().collection::("access_keys"); + keys.create_index(id_index(Some("akid"), true)) + .await + .unwrap(); + keys.create_index(id_index(Some("akid_plain"), false)) + .await + .unwrap(); + let catalog = d.target.catalog.clone(); + let item = refused_index(&d).await; + assert!( + item.starts_with(&format!( + "migration 6 unique_access_key_ids: the unique index akid on {catalog}.access_keys (" + )) && item.ends_with(&format!( + ") is the index this migration creates under the name access_key_id_1, and the \ + build refuses a second name for it; drop it with \ + db.getSiblingDB(\"{catalog}\").access_keys.dropIndex(\"akid\") in mongosh" + )), + "{item}" + ); + keys.drop_index("akid").await.unwrap(); + d.migrate().await.expect("migrate"); + assert_eq!(unique_index(&d).await, Some(true)); + assert!( + access_key_indexes(&d) + .await + .contains(&"akid_plain".to_owned()) + ); + d.drop_databases().await; +} + +/// The body's own `run` refuses what `detect` refuses, which the runner's +/// `detect` otherwise stops first: copies and an index written after the +/// runner read the collection. It returns NeedsAttention and writes +/// nothing. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_run_refuses_copies_written_after_detect() { + use extenddb_storage::migrations::MigrationId; + + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "run6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + d.migrate_through(MigrationId::DropContinuousBackups) + .await + .expect("migrations 1 to 5"); + let (_, fixture_id, account, created) = fixture_key(&d).await; + d.catalog() + .collection::("access_keys") + .insert_one(stored_key( + 1, + &fixture_id, + &account, + "importer", + created + 60_000, + )) + .await + .unwrap(); + let documents = Deployment::documents(&d.catalog(), "access_keys").await; + + let items = run_body(&d).await; + assert_eq!(items.len(), 1, "{items:?}"); + assert!( + items[0].starts_with(&format!("the access key ID {fixture_id} is stored 2 times")), + "{items:?}" + ); + assert_eq!( + Deployment::documents(&d.catalog(), "access_keys").await, + documents + ); + assert_eq!(unique_index(&d).await, None); + + // An index the build would fail on, beside an ID the body itself + // resolves: `run` refuses the index and deletes no copy. + let keys = d.catalog().collection::("access_keys"); + keys.delete_one(doc! { "_id": oid(1) }).await.unwrap(); + keys.insert_one(stored_key( + 2, + &fixture_id, + &account, + "cu-tester", + created + 60_000, + )) + .await + .unwrap(); + keys.create_index(id_index(None, false)).await.unwrap(); + let documents = Deployment::documents(&d.catalog(), "access_keys").await; + let items = run_body(&d).await; + assert_eq!(items.len(), 1, "{items:?}"); + assert!( + items[0].starts_with("the index access_key_id_1 on "), + "{items:?}" + ); + assert_eq!( + Deployment::documents(&d.catalog(), "access_keys").await, + documents + ); + assert!( + d.catalog_ledger().await.iter().all(|row| row.id != 6), + "migration 6 is not recorded" + ); + d.drop_databases().await; +} + +/// Migration 6's body `run`, called directly under the lock; the items it +/// refuses the migration for. +async fn run_body(d: &Deployment) -> Vec { + use extenddb_storage::migrations::{Body, MigrationError, MigrationId}; + use extenddb_storage_mongodb::migrations::{MigrationLock, Timing, migration}; + + let held = MigrationLock::acquire( + &d.target, + std::time::Duration::from_secs(10), + Timing::PRODUCTION, + ) + .await + .expect("lock"); + let migrations = MongoMigrations::open(&d.target, Some(&held)) + .await + .expect("open"); + let entry = migration(MigrationId::UniqueAccessKeyIds); + let Body::Code { body, .. } = &entry.bodies()[0].body else { + panic!("migration 6 is a code body"); + }; + let result = body.run(migrations.stores()).await; + migrations.close().await; + held.release().await; + match result { + Err(MigrationError::Status(CatalogStatus::NeedsAttention { items })) => items, + other => panic!("run did not refuse: {:?}", other.map(|done| done.notes)), + } +} + +/// An account other than the fixture's. +const OTHER_ACCOUNT: &str = "210987654321"; + +/// An access key ID that users of one name in two accounts hold. +const TWO_ACCOUNTS_KEY_ID: &str = "AKIATWOACCOUNTS00000"; + +/// The access key IDs of a document whose `access_key_id` is an array. +const ARRAY_KEY_IDS: [&str; 2] = ["AKIAARRAYEXAMPLE000A", "AKIAARRAYEXAMPLE000B"]; + +/// The `_id` [`stored_key`] gives document `n`. +fn oid(n: u8) -> ObjectId { + ObjectId::from_bytes([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, n]) +} + +/// An index on `{ access_key_id: 1 }`, with a name when it is given. +fn id_index(name: Option<&str>, unique: bool) -> mongodb::IndexModel { + let options = match name { + Some(name) => mongodb::options::IndexOptions::builder() + .name(name.to_owned()) + .unique(unique) + .build(), + None => mongodb::options::IndexOptions::builder() + .unique(unique) + .build(), + }; + mongodb::IndexModel::builder() + .keys(doc! { "access_key_id": 1 }) + .options(options) + .build() +} + +/// Whether `access_key_id_1` is unique, or `None` when it does not exist. +async fn unique_index(d: &Deployment) -> Option { + use futures::TryStreamExt; + let indexes: Vec = d + .catalog() + .collection::("access_keys") + .list_indexes() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + indexes + .iter() + .filter_map(|m| m.options.as_ref()) + .find(|o| o.name.as_deref() == Some(ACCESS_KEY_ID_INDEX)) + .map(|o| o.unique == Some(true)) +} + +/// The items for which the status refuses migration 6. +async fn status_items(d: &Deployment) -> Vec { + match d.status().await { + CatalogStatus::NeedsAttention { items } => items, + status => panic!("migration 6 is not refused: {status}"), + } +} + +/// The one item for which the status refuses migration 6. +async fn refused_index(d: &Deployment) -> String { + let mut items = status_items(d).await; + assert_eq!(items.len(), 1, "{items:?}"); + items.pop().unwrap() +} + +/// Once its index is built, migration 6 groups only the documents whose ID +/// is an array, which the unique index takes and a lookup reads for each ID +/// in it; beside an index named `access_key_id_1` that is not its own, it +/// groups every document. A catalog at migration 5 holds an ID of two +/// users, an array ID, a document without an ID and one whose ID is the +/// number 42. Beside a non-unique `access_key_id_1`, the status refuses the +/// ID of two users, the array and the index. With the unique index built by +/// hand and hidden, it refuses the array and the hidden index, with the +/// command that unhides it, and once the index is shown, the array alone. +/// After the array is deleted `migrate --yes` applies 6, and +/// `--reconverge` refuses an array ID written later and nothing else. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_refuses_array_ids_beside_its_own_index() { + use extenddb_storage::migrations::MigrationId; + + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "arrays6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + d.migrate_through(MigrationId::DropContinuousBackups) + .await + .expect("migrations 1 to 5"); + let (_, fixture_id, account, created) = fixture_key(&d).await; + let keys = d.catalog().collection::("access_keys"); + let mut without_id = stored_key(21, "", &account, "broken", created); + without_id.remove("access_key_id"); + let mut numbered = stored_key(41, "", &account, "numbered", created); + numbered.insert("access_key_id", 42); + let array = |n: u8| { + let mut key = stored_key(n, "", &account, "listed", created); + key.insert("access_key_id", ARRAY_KEY_IDS.map(Bson::from).to_vec()); + key + }; + keys.insert_many([ + stored_key(1, &fixture_id, &account, "importer", created + 60_000), + without_id, + numbered, + array(31), + ]) + .await + .unwrap(); + keys.create_index(id_index(None, false)).await.unwrap(); + let catalog = d.target.catalog.clone(); + let prefix = "migration 6 unique_access_key_ids: "; + let refused_array = |n: u8| { + format!( + "{prefix}the access key ID of _id {} (account {account}, user listed, created ", + Bson::ObjectId(oid(n)) + ) + }; + let refused_index = format!("{prefix}the index access_key_id_1 on {catalog}.access_keys ("); + + let items = status_items(&d).await; + assert_eq!(items.len(), 3, "{items:?}"); + for start in [ + format!("{prefix}the access key ID {fixture_id} is stored 2 times"), + refused_array(31), + refused_index.clone(), + ] { + assert!( + items.iter().any(|item| item.starts_with(&start)), + "{start}\n{items:?}" + ); + } + + // The importer's copy and the index go, and the manual's unique index + // is built hidden. + keys.delete_one(doc! { "_id": oid(1) }).await.unwrap(); + keys.drop_index(ACCESS_KEY_ID_INDEX).await.unwrap(); + d.catalog() + .run_command(doc! { "createIndexes": "access_keys", "indexes": [ + { "key": { "access_key_id": 1 }, "name": ACCESS_KEY_ID_INDEX, "unique": true, "hidden": true }, + ], "commitQuorum": "majority" }) + .await + .unwrap(); + let items = status_items(&d).await; + assert_eq!(items.len(), 2, "{items:?}"); + assert!(items[0].starts_with(&refused_array(31)), "{items:?}"); + assert!( + items[1].starts_with(&refused_index) + && items[1].ends_with(&format!( + ") is the unique index on access_key_id that this migration creates, hidden \ + from the query planner, so a credential lookup does not use it; unhide it \ + with db.getSiblingDB(\"{catalog}\").access_keys.unhideIndex(\"access_key_id_1\") \ + in mongosh" + )), + "{items:?}" + ); + + d.catalog() + .run_command(doc! { "collMod": "access_keys", "index": { + "name": ACCESS_KEY_ID_INDEX, "hidden": false, + } }) + .await + .unwrap(); + let items = status_items(&d).await; + assert_eq!(items.len(), 1, "{items:?}"); + assert!(items[0].starts_with(&refused_array(31)), "{items:?}"); + + keys.delete_one(doc! { "_id": oid(31) }).await.unwrap(); + d.migrate().await.expect("migrate"); + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); + for filter in [doc! { "_id": oid(21) }, doc! { "_id": oid(41) }] { + assert_eq!( + keys.count_documents(filter.clone()).await.unwrap(), + 1, + "{filter}" + ); + } + + keys.insert_one(array(32)).await.unwrap(); + let error = d.reconverge().await.expect_err("refused"); + assert_eq!(error.exit_code(), exit::ATTENTION); + let message = error.to_string(); + assert!(message.contains(&refused_array(32)), "{message}"); + for absent in ["have no access key ID", "have the access key ID 42"] { + assert!(!message.contains(absent), "{absent}\n{message}"); + } + d.drop_databases().await; +} + +/// With an operator's index on `{ access_key_id: 1, created_at: -1 }`, a +/// lookup reads an ID's newest copy, which is not its first in natural +/// order. The body keeps the copy the lookup's own query returns and +/// deletes the others, and the operator's index stays. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_keeps_the_copy_a_lookup_reads_through_an_operator_index() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "operator6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + let (first, fixture_id, account, created) = fixture_key(&d).await; + let keys = d.catalog().collection::("access_keys"); + keys.insert_many([ + stored_key(1, &fixture_id, &account, "cu-tester", created + 60_000), + stored_key(2, &fixture_id, &account, "cu-tester", created - 60_000), + ]) + .await + .unwrap(); + keys.create_index( + mongodb::IndexModel::builder() + .keys(doc! { "access_key_id": 1, "created_at": -1 }) + .options( + mongodb::options::IndexOptions::builder() + .name("akid_newest_first".to_owned()) + .build(), + ) + .build(), + ) + .await + .unwrap(); + let newest = Bson::ObjectId(oid(1)); + assert_eq!(looked_up(&d, &fixture_id).await, newest); + + d.migrate().await.expect("migrate"); + let left: Vec = Deployment::documents(&d.catalog(), "access_keys") + .await + .iter() + .filter(|k| k.get_str("access_key_id") == Ok(fixture_id.as_str())) + .map(|k| k.get("_id").cloned().unwrap()) + .collect(); + assert_eq!(left, std::slice::from_ref(&newest)); + let notes = d + .catalog_ledger() + .await + .into_iter() + .find(|row| row.id == 6) + .expect("migration 6's row") + .notes; + assert_eq!( + notes, + format!( + "access key {fixture_id} of account {account}, user cu-tester was stored 3 times: \ + kept _id {newest}, the copy credential lookups read, and deleted the other copies \ + _id {first}, _id {}", + Bson::ObjectId(oid(2)) + ) + ); + assert_eq!(unique_index(&d).await, Some(true)); + assert!( + access_key_indexes(&d) + .await + .contains(&"akid_newest_first".to_owned()) + ); + d.drop_databases().await; +} + +/// A partial index on `access_key_id` gives the lookup an index plan for +/// some IDs and not for others: here `{ access_key_id: 1, created_at: -1 }` +/// for IDs from AKIAM on. The lookup reads an ID below AKIAM by a forward +/// scan, its first copy in natural order, and an ID from AKIAM on through +/// the index, its newest copy. The body keeps, for each ID, the copy its +/// own lookup reads. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_keeps_the_copy_each_lookup_reads_beside_a_partial_index() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "partial6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + let (_, _, account, created) = fixture_key(&d).await; + let keys = d.catalog().collection::("access_keys"); + let (scanned, indexed) = ("AKIAAPARTIAL00000001", "AKIAZPARTIAL00000001"); + keys.insert_many([ + stored_key(1, scanned, &account, "low", created + 60_000), + stored_key(2, indexed, &account, "high", created + 60_000), + stored_key(3, scanned, &account, "low", created + 120_000), + stored_key(4, indexed, &account, "high", created + 120_000), + ]) + .await + .unwrap(); + keys.create_index( + mongodb::IndexModel::builder() + .keys(doc! { "access_key_id": 1, "created_at": -1 }) + .options( + mongodb::options::IndexOptions::builder() + .name("akid_partial_newest_first".to_owned()) + .partial_filter_expression(doc! { "access_key_id": { "$gte": "AKIAM" } }) + .build(), + ) + .build(), + ) + .await + .unwrap(); + let expected = [ + (scanned, Bson::ObjectId(oid(1))), + (indexed, Bson::ObjectId(oid(4))), + ]; + for (id, read) in &expected { + assert_eq!(&looked_up(&d, id).await, read, "{id}"); + } + + d.migrate().await.expect("migrate"); + let documents = Deployment::documents(&d.catalog(), "access_keys").await; + for (id, read) in &expected { + let left: Vec<&Bson> = documents + .iter() + .filter(|k| k.get_str("access_key_id") == Ok(*id)) + .filter_map(|k| k.get("_id")) + .collect(); + assert_eq!(left, [read], "{id}"); + assert_eq!(&looked_up(&d, id).await, read, "{id}"); + } + assert_eq!(unique_index(&d).await, Some(true)); + d.drop_databases().await; +} + +/// On MongoDB 8.0 and later, query settings can give the lookup's command +/// another plan with no index on `access_key_id`: here `allowedIndexes: +/// [{ $natural: -1 }]` for its shape, so a lookup reads an ID's last copy in +/// natural order. `explain` reports a backward scan, and the body keeps the +/// copy the lookup's own query returns. MongoDB 7.0 has no query settings, +/// and there the test stops after it loads the fixture, before it writes +/// the second copy. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_keeps_the_copy_a_lookup_reads_under_query_settings() { + use futures::FutureExt; + + /// CommandNotFound. + const NO_SUCH_COMMAND: i32 = 59; + + fn no_such_command(e: &mongodb::error::Error) -> bool { + matches!(*e.kind, mongodb::error::ErrorKind::Command(ref c) if c.code == NO_SUCH_COMMAND) + } + + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "settings6").await; + let catalog = d.target.catalog.clone(); + let admin = d.client.database("admin"); + fixture::load(&d, &fixture::read("0.1.14")).await; + let (first, fixture_id, account, created) = fixture_key(&d).await; + let set = admin + .run_command(doc! { + "setQuerySettings": { + "find": "access_keys", + "filter": { "access_key_id": &fixture_id }, + "limit": 1, + "singleBatch": true, + "$db": &catalog, + }, + "settings": { "indexHints": { + "ns": { "db": &catalog, "coll": "access_keys" }, + "allowedIndexes": [{ "$natural": -1 }], + } }, + }) + .await; + let hash = match set { + Ok(reply) => reply.get_str("queryShapeHash").unwrap().to_owned(), + Err(e) if no_such_command(&e) => { + eprintln!("query settings need MongoDB 8.0 or later; this server has none"); + d.drop_databases().await; + return; + } + Err(e) => panic!("setQuerySettings: {e}"), + }; + let outcome = std::panic::AssertUnwindSafe(async { + let keys = d.catalog().collection::("access_keys"); + keys.insert_one(stored_key( + 1, + &fixture_id, + &account, + "cu-tester", + created + 60_000, + )) + .await + .unwrap(); + let last = Bson::ObjectId(oid(1)); + assert_eq!(looked_up(&d, &fixture_id).await, last); + + d.migrate().await.expect("migrate"); + let left: Vec = Deployment::documents(&d.catalog(), "access_keys") + .await + .iter() + .filter(|k| k.get_str("access_key_id") == Ok(fixture_id.as_str())) + .map(|k| k.get("_id").cloned().unwrap()) + .collect(); + assert_eq!(left, std::slice::from_ref(&last)); + let notes = d + .catalog_ledger() + .await + .into_iter() + .find(|row| row.id == 6) + .expect("migration 6's row") + .notes; + assert_eq!( + notes, + format!( + "access key {fixture_id} of account {account}, user cu-tester was stored 2 times: \ + kept _id {last}, the copy credential lookups read, and deleted the other copy \ + _id {first}" + ) + ); + }) + .catch_unwind() + .await; + admin + .run_command(doc! { "removeQuerySettings": &hash }) + .await + .unwrap_or_else(|e| panic!("removeQuerySettings {hash}: {e}")); + if let Err(panic) = outcome { + std::panic::resume_unwind(panic); + } + d.drop_databases().await; +} + +/// How migration 6 must treat an index an operator may have on +/// `access_keys`, as measured with `createIndexes` on MongoDB 7.0.45 and +/// 8.0.34: whether `detect` refuses it, and whether the build of +/// `access_key_id_1` fails beside it. The unique hidden `access_key_id_1` +/// is refused, because a lookup does not use a hidden index and MongoDB 8 +/// answers the build with IndexOptionsConflict, although MongoDB 7 answers +/// that the index exists. +const INDEX_VARIANTS: &[IndexVariant] = &[ + ("access_key_id_1, not unique", Refused::BuildFails, || { + doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1" } + }), + ("access_key_id_1, background", Refused::No, || { + doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "background": true } + }), + ("access_key_id_1, sparse: false", Refused::No, || { + doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "sparse": false } + }), + ( + "access_key_id_1, the key direction a Decimal128", + Refused::No, + || { + doc! { "key": { "access_key_id": decimal_one() }, "name": "access_key_id_1", "unique": true } + }, + ), + ( + "access_key_id_1, the key direction the Decimal128 1.0", + Refused::No, + || { + doc! { "key": { "access_key_id": decimal("1.0") }, "name": "access_key_id_1", "unique": true } + }, + ), + ( + "access_key_id_1, the key direction a Decimal128 just above 1", + Refused::BuildFails, + || { + doc! { "key": { "access_key_id": decimal(JUST_ABOVE_ONE) }, "name": "access_key_id_1", "unique": true } + }, + ), + ("access_key_id_1, partial", Refused::BuildFails, || { + doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, + "partialFilterExpression": { "access_key_id": { "$exists": true } } } + }), + ("access_key_id_1, TTL", Refused::BuildFails, || { + doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "expireAfterSeconds": 3600 } + }), + ("access_key_id_1, hidden", Refused::ByVersion, || { + doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "hidden": true } + }), + ("access_key_id_1 on account_id", Refused::BuildFails, || { + doc! { "key": { "account_id": 1 }, "name": "access_key_id_1" } + }), + ("akid", Refused::BuildFails, || { + doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true } + }), + ( + "akid, the key direction a Double", + Refused::BuildFails, + || { + doc! { "key": { "access_key_id": 1.0 }, "name": "akid", "unique": true } + }, + ), + ( + "akid, the key direction an Int64", + Refused::BuildFails, + || { + doc! { "key": { "access_key_id": 1_i64 }, "name": "akid", "unique": true } + }, + ), + ( + "akid, the key direction a Decimal128", + Refused::BuildFails, + || { + doc! { "key": { "access_key_id": decimal_one() }, "name": "akid", "unique": true } + }, + ), + ( + "akid, the key direction a Decimal128 just above 1", + Refused::No, + || { + doc! { "key": { "access_key_id": decimal(JUST_ABOVE_ONE) }, "name": "akid", "unique": true } + }, + ), + ("akid, the key direction the Double 2", Refused::No, || { + doc! { "key": { "access_key_id": 2.0 }, "name": "akid", "unique": true } + }), + ("akid, the key direction the Double -1", Refused::No, || { + doc! { "key": { "access_key_id": -1.0 }, "name": "akid", "unique": true } + }), + ("akid, the key direction the Int64 -1", Refused::No, || { + doc! { "key": { "access_key_id": -1_i64 }, "name": "akid", "unique": true } + }), + ("akid, sparse: false", Refused::BuildFails, || { + doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, "sparse": false } + }), + ("akid, sparse", Refused::No, || { + doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, "sparse": true } + }), + ("akid, partial", Refused::No, || { + doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, + "partialFilterExpression": { "access_key_id": { "$exists": true } } } + }), + ("akid, collation", Refused::No, || { + doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, + "collation": { "locale": "en", "strength": 2 } } + }), + ("akid, descending", Refused::No, || { + doc! { "key": { "access_key_id": -1 }, "name": "akid", "unique": true } + }), + ("akid, not unique", Refused::No, || { + doc! { "key": { "access_key_id": 1 }, "name": "akid" } + }), + ("akid, compound", Refused::No, || { + doc! { "key": { "access_key_id": 1, "account_id": 1 }, "name": "akid", "unique": true } + }), +]; + +/// A variant's label, whether `detect` refuses it, and its spec. +type IndexVariant = (&'static str, Refused, fn() -> Document); + +/// Whether `detect` refuses an index, and why. +#[derive(Clone, Copy, Debug, PartialEq)] +enum Refused { + /// Accepted: the build succeeds beside it. + No, + /// Refused: the build fails beside it. + BuildFails, + /// Refused: the build fails beside it on some versions. + ByVersion, +} + +fn decimal_one() -> Bson { + decimal("1") +} + +fn decimal(text: &str) -> Bson { + Bson::Decimal128(text.parse().unwrap()) +} + +/// A Decimal128 that a Double rounds to 1 and the server compares as +/// another value than 1. +const JUST_ABOVE_ONE: &str = "1.0000000000000000000000000000001"; + +/// Each index of [`INDEX_VARIANTS`], alone beside migration 1's indexes on +/// a catalog at migration 5 that holds one key: `detect` refuses it exactly +/// when it is marked refused. The body's own `createIndexes`, sent beside a +/// refused index, fails, and `migrate --yes` applies migration 6 beside an +/// accepted one. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_refuses_an_index_exactly_when_the_build_fails_on_it() { + use extenddb_storage::migrations::MigrationId; + + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "variants6").await; + fixture::load(&d, &fixture::read("0.1.14")).await; + d.migrate_through(MigrationId::DropContinuousBackups) + .await + .expect("migrations 1 to 5"); + let keys = d.catalog().collection::("access_keys"); + for (label, refused, spec) in INDEX_VARIANTS { + for name in access_key_indexes(&d).await { + if !["_id_", "account_id_1_user_name_1"].contains(&name.as_str()) { + keys.drop_index(name).await.unwrap(); + } + } + d.catalog() + .run_command(doc! { "createIndexes": "access_keys", "indexes": [spec()] }) + .await + .unwrap_or_else(|e| panic!("{label}: {e}")); + let status = d.status().await; + match refused { + Refused::No => { + assert_eq!(status.label(), "Behind", "{label}: {status}"); + d.migrate().await.unwrap_or_else(|e| panic!("{label}: {e}")); + assert_eq!(unique_index(&d).await, Some(true), "{label}"); + forget_6(&d).await; + } + Refused::BuildFails | Refused::ByVersion => { + let CatalogStatus::NeedsAttention { items } = status else { + panic!("{label}: not refused: {status}"); + }; + assert_eq!(items.len(), 1, "{label}: {items:?}"); + let built = d + .catalog() + .run_command(doc! { "createIndexes": "access_keys", "indexes": [ + { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true }, + ], "commitQuorum": "majority" }) + .await; + if *refused == Refused::BuildFails { + assert!(built.is_err(), "{label}: the build succeeds: {built:?}"); + } + } + } + } + d.drop_databases().await; +} + +/// `migrate --forget 6`. +async fn forget_6(d: &Deployment) { + use extenddb_storage::migrations::Runner; + use extenddb_storage_mongodb::migrations::{MigrationLock, Timing}; + + let held = MigrationLock::acquire( + &d.target, + std::time::Duration::from_secs(10), + Timing::PRODUCTION, + ) + .await + .expect("lock"); + let migrations = MongoMigrations::open(&d.target, Some(&held)) + .await + .expect("open"); + let forgot = Runner::new(&migrations, "test").forget(6).await; + migrations.close().await; + held.release().await; + forgot.expect("forget 6"); +} diff --git a/crates/storage-mongodb/tests/migrations_spill.rs b/crates/storage-mongodb/tests/migrations_spill.rs new file mode 100644 index 000000000..9135180e1 --- /dev/null +++ b/crates/storage-mongodb/tests/migrations_spill.rs @@ -0,0 +1,180 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Migration 6's read of `access_keys` when its `$group` state is larger +//! than the server's memory limit for `$group`. The test lowers the limits +//! of the classic and the slot-based engine to 100 KB and sets them back at +//! the end. They are the server's, so the test is alone in its binary: the +//! tests of one binary run at once. A run killed before it sets them back +//! leaves them at 100 KB until mongod restarts, and the next run stops at +//! its start with the commands that set them back. + +mod migrations_support; + +use bson::{Bson, Document, doc}; +use extenddb_storage::migrations::CatalogStatus; +use futures::{FutureExt, TryStreamExt}; +use migrations_support::{Deployment, fixture, server}; + +/// The server parameters that bound the memory of `$group`, for the +/// classic engine and for the slot-based engine. +const LIMITS: [&str; 2] = [ + "internalDocumentSourceGroupMaxMemoryBytes", + "internalQuerySlotBasedExecutionHashAggApproxMemoryUseInBytesBeforeSpill", +]; + +/// What the test sets the limits to while it runs. +const LOWERED: i64 = 100 * 1024; + +/// The limits' default on MongoDB 7.0 and 8.0, 100 MB. +const DEFAULT: i64 = 100 * 1024 * 1024; + +/// QueryExceededMemoryLimitNoDiskUseAllowed. +const NO_DISK_USE: i32 = 292; + +const KEYS: u32 = 3000; +const SHARED: u32 = 20; + +fn key_id(i: u32) -> String { + format!("AKIASPILL{i:011}") +} + +async fn set_limits(client: &mongodb::Client, values: &[Bson]) { + for (name, value) in LIMITS.iter().zip(values) { + client + .database("admin") + .run_command(doc! { "setParameter": 1, *name: value.clone() }) + .await + .unwrap_or_else(|e| panic!("set {name}: {e}")); + } +} + +async fn limits(client: &mongodb::Client) -> Vec { + let mut values = Vec::new(); + for name in LIMITS { + let reply = client + .database("admin") + .run_command(doc! { "getParameter": 1, name: 1 }) + .await + .unwrap_or_else(|e| panic!("get {name}: {e}")); + values.push(reply.get(name).cloned().expect(name)); + } + values +} + +/// Stops the test when a limit is at the value the test sets, which a run +/// killed before it set the limits back leaves: the test would otherwise +/// set back that value at its end. The server returns both limits as Int64. +fn check_not_left_lowered(values: &[Bson]) { + let left: Vec<&str> = LIMITS + .iter() + .zip(values) + .filter(|(_, value)| **value == Bson::Int64(LOWERED)) + .map(|(name, _)| *name) + .collect(); + let (is, it) = if left.len() == 1 { + ("is", "it") + } else { + ("are", "them") + }; + assert!( + left.is_empty(), + "{} {is} at {LOWERED}, the value this test sets while it runs: an earlier run was \ + stopped before it set {it} back. Restart mongod, or set {it} back in mongosh with {}", + left.join(" and "), + left.iter() + .map(|name| format!("db.adminCommand({{ setParameter: 1, {name}: {DEFAULT} }})")) + .collect::>() + .join(" and ") + ); +} + +/// 3,000 keys, and a second copy of 20 of them for the same user: migration +/// 6's `$group` holds more than 100 KB, and its read spills. The migration +/// applies, and each of the 20 IDs keeps the copy a lookup read. +#[tokio::test(flavor = "multi_thread")] +async fn migration_6_reads_a_collection_whose_group_spills_to_disk() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "spill6").await; + let saved = limits(&d.client).await; + check_not_left_lowered(&saved); + fixture::load(&d, &fixture::read("0.1.14")).await; + let keys = d.catalog().collection::("access_keys"); + let account = Deployment::documents(&d.catalog(), "access_keys").await[0] + .get_str("account_id") + .unwrap() + .to_owned(); + let key = |i: u32| { + doc! { + "access_key_id": key_id(i), + "account_id": &account, + "user_name": format!("u{}", i % 10), + "is_active": true, + "created_at": bson::DateTime::from_millis(1_793_235_168_000 + i64::from(i)), + } + }; + keys.insert_many((0..KEYS).map(key)).await.unwrap(); + keys.insert_many((0..SHARED).map(key)).await.unwrap(); + let mut read_before = Vec::new(); + for i in 0..SHARED { + let first = keys + .find_one(doc! { "access_key_id": key_id(i) }) + .await + .unwrap() + .unwrap(); + read_before.push(first.get("_id").cloned().unwrap()); + } + + let small = vec![Bson::Int64(LOWERED); LIMITS.len()]; + set_limits(&d.client, &small).await; + let outcome = std::panic::AssertUnwindSafe(async { + // The state is past the limits: without disk use the same grouping + // fails. + let refused = keys + .aggregate(vec![doc! { "$group": { + "_id": "$access_key_id", + "copies": { "$push": { "_id": "$_id", "user_name": "$user_name" } }, + } }]) + .allow_disk_use(false) + .await; + let code = match refused { + Ok(cursor) => cursor.try_collect::>().await.err(), + Err(e) => Some(e), + } + .and_then(|e| match *e.kind { + mongodb::error::ErrorKind::Command(ref c) => Some(c.code), + _ => None, + }); + assert_eq!(code, Some(NO_DISK_USE), "the group fits in 100 KB"); + + d.migrate().await.expect("migrate"); + }) + .catch_unwind() + .await; + set_limits(&d.client, &saved).await; + if let Err(panic) = outcome { + std::panic::resume_unwind(panic); + } + + assert_eq!(d.status().await, CatalogStatus::Current { version: 6 }); + for (i, read) in (0..SHARED).zip(&read_before) { + let left: Vec = keys + .find(doc! { "access_key_id": key_id(i) }) + .await + .unwrap() + .try_collect() + .await + .unwrap(); + assert_eq!(left.len(), 1, "{}", key_id(i)); + assert_eq!(left[0].get("_id"), Some(read), "{}", key_id(i)); + } + let notes = d + .catalog_ledger() + .await + .into_iter() + .find(|row| row.id == 6) + .expect("migration 6's row") + .notes; + assert_eq!(notes.lines().count(), SHARED as usize, "{notes}"); + d.drop_databases().await; +} diff --git a/crates/storage-mongodb/tests/migrations_support/mod.rs b/crates/storage-mongodb/tests/migrations_support/mod.rs index 06bddd927..6d5cf719d 100644 --- a/crates/storage-mongodb/tests/migrations_support/mod.rs +++ b/crates/storage-mongodb/tests/migrations_support/mod.rs @@ -22,10 +22,37 @@ use extenddb_storage::migrations::{ CatalogStatus, LedgerRow, MigrationError, MigrationId, RunReport, Runner, }; use extenddb_storage_mongodb::migrations::{ - MigrationLock, MongoMigrations, MongoTarget, Timing, catalog_status, lock, migrate_holding, + Definitions, Index, MigrationLock, MongoMigrations, MongoTarget, Timing, catalog_status, lock, + migrate_holding, }; use futures::TryStreamExt; +/// The catalog definitions migration 1 creates in an empty store. +pub const CATALOG_EXPECTED: &str = + include_str!("../../migrations/catalog/0001_baseline.expected.json"); + +/// The name of the index migration 6 creates on `access_keys`. +pub const ACCESS_KEY_ID_INDEX: &str = "access_key_id_1"; + +/// The catalog definitions after every migration: migration 1's, without +/// the collection migration 5 drops, with the index migration 6 creates. +pub fn catalog_after_every_migration() -> Definitions { + let mut catalog: Definitions = serde_json::from_str(CATALOG_EXPECTED).expect("catalog"); + catalog.remove("continuous_backups"); + catalog + .get_mut("access_keys") + .expect("access_keys") + .indexes + .insert( + ACCESS_KEY_ID_INDEX.to_owned(), + Index { + key: vec![("access_key_id".to_owned(), serde_json::json!(1))], + options: [("unique".to_owned(), serde_json::json!(true))].into(), + }, + ); + catalog +} + /// The server the tests run against, or `None` to skip. /// /// # Panics diff --git a/crates/storage-postgres/src/migrations/mod.rs b/crates/storage-postgres/src/migrations/mod.rs index 740c3469a..4405d46fe 100644 --- a/crates/storage-postgres/src/migrations/mod.rs +++ b/crates/storage-postgres/src/migrations/mod.rs @@ -127,6 +127,9 @@ pub fn migration(id: MigrationId) -> Migration { "0005_drop_continuous_backups.sql" )], }, + MigrationId::UniqueAccessKeyIds => Migration::NotApplicable( + "access_key_id is the primary key of access_keys, so an ID can be stored once", + ), } } @@ -148,7 +151,8 @@ pub(crate) const fn runs_in(id: MigrationId) -> RunsIn { MigrationId::Baseline | MigrationId::ResourceTagConditionKeys | MigrationId::SortKeyDefinitions - | MigrationId::DropContinuousBackups => RunsIn::Transaction, + | MigrationId::DropContinuousBackups + | MigrationId::UniqueAccessKeyIds => RunsIn::Transaction, MigrationId::IndexTableBaseKey => RunsIn::Autocommit, } } diff --git a/crates/storage-sqlite/src/migrations/mod.rs b/crates/storage-sqlite/src/migrations/mod.rs index 06647fbb4..68bd470dd 100644 --- a/crates/storage-sqlite/src/migrations/mod.rs +++ b/crates/storage-sqlite/src/migrations/mod.rs @@ -106,6 +106,9 @@ pub fn migration(id: MigrationId) -> Migration { "0005_drop_continuous_backups.sql" )], }, + MigrationId::UniqueAccessKeyIds => Migration::NotApplicable( + "access_key_id is the primary key of access_keys, so an ID can be stored once", + ), } } diff --git a/crates/storage-sqlite/src/migrations/released_tests.rs b/crates/storage-sqlite/src/migrations/released_tests.rs index 71dcee691..cf4556553 100644 --- a/crates/storage-sqlite/src/migrations/released_tests.rs +++ b/crates/storage-sqlite/src/migrations/released_tests.rs @@ -390,11 +390,14 @@ async fn a_catalog_an_older_release_wrote_to_is_reported_and_reconverged() { next_second().await; let report = reconverge(&db).await.expect("reconverge"); - // Every convergent migration ran again; migration 2, which is not - // convergent, is named as skipped. + // Every convergent migration with a body on SQLite ran again; + // migration 2, which is not convergent, is named as skipped. let convergent = MigrationId::ALL .into_iter() - .filter(|id| super::migration(*id).convergent()) + .filter(|id| { + let migration = super::migration(*id); + migration.convergent() && !migration.bodies().is_empty() + }) .count(); assert_eq!(report.applied.len(), convergent, "{trace}: {report:?}"); let skipped = format!("Skipped {}:", MigrationId::ResourceTagConditionKeys); diff --git a/crates/storage-sqlite/src/migrations/tests.rs b/crates/storage-sqlite/src/migrations/tests.rs index a837e802e..34e84be9b 100644 --- a/crates/storage-sqlite/src/migrations/tests.rs +++ b/crates/storage-sqlite/src/migrations/tests.rs @@ -264,13 +264,27 @@ async fn a_fresh_database_is_current_after_one_run_and_seeds_nothing() { status(&db).await, CatalogStatus::Current { version: LATEST } ); + // Every id has a row: `applied` for one with a body on SQLite, and + // `not_applicable` for one that does not apply here. + let applies = MigrationId::ALL + .into_iter() + .filter(|id| !super::migration(*id).bodies().is_empty()) + .count(); assert_eq!( count( &db, "SELECT COUNT(*) FROM extenddb_migrations WHERE outcome = 'applied'" ) .await, - i64::from(LATEST) + i64::try_from(applies).unwrap() + ); + assert_eq!( + count( + &db, + "SELECT COUNT(*) FROM extenddb_migrations WHERE outcome = 'not_applicable'" + ) + .await, + i64::try_from(MigrationId::ALL.len() - applies).unwrap() ); assert_eq!(version_row(&db).await, Some(LATEST.to_string())); assert_eq!( diff --git a/crates/storage/src/migrations/registry.rs b/crates/storage/src/migrations/registry.rs index dc426fd18..9191d348c 100644 --- a/crates/storage/src/migrations/registry.rs +++ b/crates/storage/src/migrations/registry.rs @@ -67,11 +67,16 @@ migration_ids! { /// it. DropContinuousBackups = 5, "drop_continuous_backups", "drop the continuous_backups table, which no request reads or writes since #335"; + /// Makes every access key ID unique. PostgreSQL, SQLite and Cassandra key + /// `access_keys` by the ID; MongoDB had no unique index on it, so an + /// import stored a second key under an ID that a key already had. + UniqueAccessKeyIds = 6, "unique_access_key_ids", + "make every access key ID unique, which the MongoDB catalog did not enforce"; } /// Highest id that has shipped in a release. Bodies at or below it are frozen: /// each backend pins their checksums, kinds and convergent flags, and -/// `forget` refuses them. The v1.0 release PR raises it to 5. +/// `forget` refuses them. The v1.0 release PR raises it to 6. pub const RELEASED_THROUGH: u32 = 0; impl MigrationId { diff --git a/devtools/run-mongodb-tests b/devtools/run-mongodb-tests index a41cc78f0..217b0be19 100755 --- a/devtools/run-mongodb-tests +++ b/devtools/run-mongodb-tests @@ -302,7 +302,8 @@ if [[ " ${RUN_TESTS_ARGS[*]} " == *" --rust"* ]]; then EXTENDDB_TEST_MONGODB_URI="mongodb://localhost:$MONGO_PORT/?replicaSet=rs0&directConnection=true" \ EXTENDDB_TEST_MONGODB_REQUIRED=1 \ cargo test -p extenddb-storage-mongodb --test migrations_fresh --test migrations_guards \ - --test migrations_holders --test migrations_locks --test migrations_released + --test migrations_holders --test migrations_locks --test migrations_released \ + --test migrations_spill echo "" fi echo "=== Running test suite via devtools/run-tests --backend mongodb ===" diff --git a/docs/adr/0023-catalog-migrations.md b/docs/adr/0023-catalog-migrations.md index eeeb98a54..b83f1e3ac 100644 --- a/docs/adr/0023-catalog-migrations.md +++ b/docs/adr/0023-catalog-migrations.md @@ -365,8 +365,8 @@ that each backend implements, as follows. to run. - Rollback after additive migrations runs the previous 1.x binary. Rollback after a breaking migration restores the backup taken before the upgrade. - All five v1.0 migrations are breaking, so rolling back from 1.0 is a - restore. + Each v1.0 migration is breaking on every backend it applies to, so + rolling back from 1.0 is a restore. - Releases before 1.0 refuse a 1.0 catalog or leave a trace in it. PostgreSQL `serve` refuses the integer version row, and v0.1.14 `serve` on every backend refuses a catalog without the `resource_tag_migration` row, which diff --git a/docs/getting-started.md b/docs/getting-started.md index 712f34cfe..b3e6ef366 100755 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -269,7 +269,7 @@ Config: extenddb.toml Catalog database: extenddb_catalog --- Checking catalog migrations... - OK: Current: the catalog is at migration 5 + OK: Current: the catalog is at migration 6 --- Checking catalog connection... OK: Connected to catalog. --- Checking data database... @@ -287,7 +287,7 @@ extenddb runs as a daemon (background process) and logs to syslog. On startup it ```bash ./target/release/extenddb serve --config extenddb.toml -# extenddb 0.1.14 (catalog migration 5) starting on 127.0.0.1:18443 +# extenddb 0.1.14 (catalog migration 6) starting on 127.0.0.1:18443 # storage: postgres (postgresql://extenddb:***@localhost:5432/extenddb_catalog) ``` @@ -1366,7 +1366,7 @@ Each runner requires its tools to be installed. The runner checks prerequisites ```bash ./target/release/extenddb version # extenddb 0.1.14 -# catalog migration 5 (postgres) +# catalog migration 6 (postgres) # commit abc1234 # built 2026-04-17T12:00:00Z ``` diff --git a/docs/manuals/04-quickstart-setup-guide.md b/docs/manuals/04-quickstart-setup-guide.md index 6e8339c6b..08333d9dd 100755 --- a/docs/manuals/04-quickstart-setup-guide.md +++ b/docs/manuals/04-quickstart-setup-guide.md @@ -130,7 +130,7 @@ Check the version: ```bash ./target/release/extenddb version # extenddb 0.1.14 -# catalog migration 5 (postgres) +# catalog migration 6 (postgres) # commit abc1234 # built 2026-04-17T12:00:00Z ``` @@ -172,7 +172,7 @@ Config: extenddb.toml Catalog database: extenddb_catalog --- Checking catalog migrations... - OK: Current: the catalog is at migration 5 + OK: Current: the catalog is at migration 6 --- Checking catalog connection... OK: Connected to catalog. --- Checking data database... diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index e51da206c..e0ff8162f 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -925,7 +925,7 @@ Check that PostgreSQL is running and the connection string in `extenddb.toml` is **The catalog has pending migrations, or is ahead of the binary:** -On PostgreSQL, SQLite, Cassandra and MongoDB, `serve` reads the catalog's migration status before it opens the engine and refuses a catalog it cannot serve, printing the status and the command that resolves it. A catalog with pending migrations exits 3, for example `the catalog is at migration 4, and ExtendDB needs migration 5`, followed by the pending migrations and `extenddb migrate --config extenddb.toml --yes`. A catalog that a newer release migrated with a breaking migration exits 4 and names that release's migrations; upgrade the binary rather than the catalog. The [Upgrade Manual](07-upgrade-manual.md#the-catalog-status) lists every status with its exit code and the stop / migrate / start sequence. +On PostgreSQL, SQLite, Cassandra and MongoDB, `serve` reads the catalog's migration status before it opens the engine and refuses a catalog it cannot serve, printing the status and the command that resolves it. A catalog with pending migrations exits 3, for example `the catalog is at migration 5, and ExtendDB needs migration 6`, followed by the pending migrations and `extenddb migrate --config extenddb.toml --yes`. A catalog that a newer release migrated with a breaking migration exits 4 and names that release's migrations; upgrade the binary rather than the catalog. The [Upgrade Manual](07-upgrade-manual.md#the-catalog-status) lists every status with its exit code and the stop / migrate / start sequence. ### Authentication Errors diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index 147890c71..dd71124e9 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -4,7 +4,7 @@ ## Current Status -This release versions the PostgreSQL, SQLite, Cassandra and MongoDB catalogs by a migration ledger and carries catalog migrations 1 to 5, which `extenddb migrate --list` prints. **Every deployment created by a 0.1.x release runs `extenddb migrate --yes` once** before a server of this release starts on it: the server refuses a catalog with pending migrations. Migration 1 brings the catalog of every 0.1.x release to the v1.0 schema, and migration 5 drops `continuous_backups`, a table and on MongoDB a collection (see [Catalog 0.0.5](#catalog-005)). +This release versions the PostgreSQL, SQLite, Cassandra and MongoDB catalogs by a migration ledger and carries catalog migrations 1 to 6, which `extenddb migrate --list` prints. **Every deployment created by a 0.1.x release runs `extenddb migrate --yes` once** before a server of this release starts on it: the server refuses a catalog with pending migrations. Migration 1 brings the catalog of every 0.1.x release to the v1.0 schema, migration 5 drops `continuous_backups`, a table and on MongoDB a collection (see [Catalog 0.0.5](#catalog-005)), and migration 6 gives the MongoDB `access_keys` collection a unique index on the access key ID (see step 4 of the [general upgrade procedure](#general-upgrade-procedure)). A backup that a 0.1.x release took can restore without its global and local secondary indexes: a SQLite backup taken by 0.1.14 records only the table's key schema and attribute definitions, and its restore creates the table with its items and no index. Take a new backup of each table after upgrading to v1.0, and keep the 0.1.x backups for their items only. @@ -39,7 +39,7 @@ Exit status 6 means the migration lock, or a running server, was still held when `verify` also checks the connection to the data database. When that check fails, `verify` exits 1 on a Behind catalog instead of 3, because `migrate --yes` cannot apply the pending migrations while the data database cannot be reached. It still exits 4 or 5 for the statuses that need a person. -A migration is additive when the previous release serves the catalog correctly with it applied, and breaking otherwise. Migrations 1 to 5 are breaking: do not run a 0.1.x server against a migrated catalog. A 0.1.x binary that writes to a migrated catalog (its `migrate --yes`, or a SQLite dev server) leaves it WrittenByOlderRelease. `migrate --yes --reconverge` repairs it by running the convergent migrations again. Migration 2, which rewrites stored policies, is not convergent: the repair skips it and prints a line that says so (see the v0.1.14 notes below). +A migration is additive when the previous release serves the catalog correctly with it applied, and breaking otherwise. Each migration is breaking on every backend it applies to: do not run a 0.1.x server against a migrated catalog. A 0.1.x binary that writes to a migrated catalog (its `migrate --yes`, or a SQLite dev server) leaves it WrittenByOlderRelease. `migrate --yes --reconverge` repairs it by running the convergent migrations again. Migration 2, which rewrites stored policies, is not convergent: the repair skips it and prints a line that says so (see the v0.1.14 notes below). ## Writing a New Migration @@ -95,6 +95,12 @@ The two transactions that hold the keys, the server's registration and the migra On MongoDB, `migrate` connects with the connection string in the config file, which the server uses too. A user with the `readWrite` role on `extenddb_catalog` and on `extenddb_data` and no other role can run `init`, `migrate --yes`, `verify` and `serve`. `migrate --yes` takes the migration lock, a document in the catalog's `settings` collection, and then waits for the registration every running server keeps in the catalog's `extenddb_servers` collection, and after `--lock-timeout` exits 6 naming each server's host and pid. A server renews its registration every 5 s, and every second after a renewal failed, and deletes it when it stops, once every background worker has ended (it leaves it to expire when one could not be stopped); a server that was killed leaves it until it expires, at most 45 s after its last renewal. A server whose registration was deleted, or that has not renewed it for 30 s, for example because the primary stopped answering, answers new requests with `ServiceUnavailable` and registers again, as a PostgreSQL server does after its session ends (above): it serves again once it holds a new registration and the catalog's status is the one it started with, and it stops with exit 1 when a migration holds the lock, when the status changed, or when 30 s pass without a registration. The lock and the registrations expire by the clock of the replica set's primary, so keep the clocks of its members in sync with NTP. Migration 1 deletes the `resource_tag_migration` settings document in the transaction that records its catalog step, so a 0.1.14 server refuses the migrated catalog, and a run that fails before that transaction leaves the document in place. 0.1.12 and 0.1.13 servers read neither that document nor the catalog version, so they start on a migrated catalog and serve it: do not run them against it. To go back to a 0.1.x release, restore the backup taken before `migrate --yes`. A 0.1.x `migrate` on a migrated catalog reports a pending change, and its `--yes` writes the settings document back; this release's status then reports the catalog as written by an older release, and `migrate --yes --reconverge` repairs it. +On MongoDB, migration 6 gives the catalog's `access_keys` collection a unique index on `access_key_id`; PostgreSQL, SQLite and Cassandra key their `access_keys` table by that ID. Before migration 6, every import of an access key stored a document, also under an access key ID that another document held. A credential lookup read the first of those documents its query plan met: the first in the collection's natural order when the plan scans the collection forward, as it does with no index on `access_key_id` and no query settings for the lookup's query. A delete of the key deleted one of them, so a key imported more than once still authenticated after a delete and was still listed among the user's access keys. Where one user holds every document of an ID, the migration keeps the document a credential lookup reads, deletes the others, and records a note that names the ID and the `_id` of each document it kept and deleted, which `verify` prints. To find that document, it asks the server for the lookup's query plan for each such ID. When the plan scans the collection forward, the migration keeps the first document in natural order; with any other plan, such as one through an index on `access_key_id`, it sends the lookup's own query. It also prints a warning with the same names before each delete, because a `migrate --yes` that stops after its deletes and before the index exists records no note when it runs again. A key that was imported twice and then deleted once before the upgrade has one document left, from the later import: no note names it, and it authenticates with the secret of that import. After the upgrade, review each user's access keys and delete any the user had deleted before. + +Migration 6 cannot decide what to keep when the documents of an ID belong to more than one user, when two or more documents have no access key ID or share one that is not a string, when a document's access key ID is an array, which a credential lookup reads for each ID in it, or when `access_keys` has an index that would make the build fail: an index named `access_key_id_1` that is not the unique index on `access_key_id`, such as one added to speed up lookups, or that unique index under another name. It also stops when the unique index `access_key_id_1` is hidden, because a credential lookup does not use a hidden index. Then the preview, `verify` and `migrate --yes` exit 5, change nothing, and name each document with its `_id`, account, user and creation time, with the `_id` a credential lookup reads for an ID of more than one user, or name the index. Delete each document that is not the key owner's, or drop or unhide the index, in `mongosh` with the command the output prints, then run `migrate --yes` again. The index build commits once a majority of the replica set's data-bearing voting members have built it, so it does not wait for a member that is down. + +Migration 6 groups the documents of `access_keys` in one request, and the runner gives each request 10 s. On a local `mongo:7` replica set, the preview took 8 s on a collection of 1 million keys, and on a collection of 2 million the preview, `verify` and `migrate --yes` stopped after 10 s with `no answer within 10 s`. On a collection that large, delete the extra documents and create the index in `mongosh`, which sets no such limit. This command lists each access key ID stored more than once, with its owners: `db.getSiblingDB("extenddb_catalog").access_keys.aggregate([{ $group: { _id: "$access_key_id", n: { $sum: 1 }, owners: { $addToSet: { account: "$account_id", user: "$user_name" } } } }, { $match: { n: { $gt: 1 } } }], { allowDiskUse: true })`. For an ID of more than one user, delete each document that is not the key owner's, as above. For an ID of one user, keep the document that `findOne({ access_key_id: "" })` returns, which a credential lookup reads, and delete the others. No note records these deletes. Then create the index with `db.getSiblingDB("extenddb_catalog").access_keys.createIndex({ access_key_id: 1 }, { unique: true }, "majority")`, which fails with a duplicate key error that names an ID while one is still stored more than once. Once the index is built, migration 6 reads only the documents whose access key ID is an array. While the build runs, migration 6 groups every document, so run `migrate --yes` after the command returns. + On Cassandra, `migrate` connects as the role the config file names, which the server connects as too. `--cassandra-user ` connects as another role, with its password in `EXTENDDB_CASSANDRA_PASSWORD` or `--cassandra-pass`; a wrong password is reported as a connection timeout. Migration 1 adopts a 0.1.x catalog: it keeps every setting except the `resource_tag_migration` row, which it deletes, and records itself in a ledger in the catalog keyspace and in every account keyspace. On a cluster that runs `CassandraAuthorizer`, `init` grants the application role SELECT, MODIFY, CREATE and ALTER on the catalog keyspace, those four and DROP on the default account's keyspace, and CREATE ON ALL KEYSPACES, which CreateAccount needs to create a keyspace per account and which also lets the role create tables in every keyspace on the cluster; a run of `migrate --yes --cassandra-user ` that creates a missing account keyspace grants the application role the five permissions on it. A 0.1.x `init` granted the application role nothing, so a deployment created before 1.0 on such a cluster needs the statements below before `migrate`. `migrate --yes` takes the migration lock, a row in the catalog's `settings` table written by lightweight transactions, and then waits for the registration every running server of this release keeps in the catalog's `extenddb_servers` table, and after `--lock-timeout` exits 6 naming each server. A server renews its registration every 5 s, and every second after a renewal failed, and deletes it when it stops, once every background worker has ended (it leaves it to expire when one could not be stopped); a server that was killed leaves it until it expires, at most 45 s after its last renewal. A server tries its renewals for 30 s after the last one that succeeded, so it keeps serving through an outage of 25 to 30 s, depending on when the outage starts, for example while the only replica of the catalog restarts. After that, 30 to 35 s after its last successful renewal, it answers new requests with `ServiceUnavailable` and registers again, as a PostgreSQL server does after its session ends (above), and stops with exit 1 when a migration holds the lock, when the status changed, or when 30 s pass without a registration. A migration needs every replica of the catalog keyspace reachable: the lock is written at `SERIAL`, the registrations are read at `ALL`, and after each schema statement the runner waits until every live node reports the same schema version. `serve` refuses a catalog with pending migrations, exiting 3 under `--foreground` with the pending migrations and the `extenddb migrate --config --yes` command to run. A 0.1.x server refuses a migrated catalog. On a deployment created before 1.0 on a cluster that runs `CassandraAuthorizer`, this release's `migrate`, `verify` and `serve`, connected as the application role, fail with `Unauthorized ... has no SELECT permission on _catalog.accounts>` until the role holds its permissions. With every server stopped, run the statements below in `cqlsh` as a superuser; the application role cannot grant them to itself. `` and `` are `username` and `keyspace_prefix` under `[storage.cassandra]` in the config file. Each account has its own keyspace, so list the accounts first: diff --git a/docs/manuals/08-install-linux.md b/docs/manuals/08-install-linux.md index 928ba957d..2b868522e 100755 --- a/docs/manuals/08-install-linux.md +++ b/docs/manuals/08-install-linux.md @@ -118,7 +118,7 @@ Expected: ``` === extenddb verify === ... - OK: Current: the catalog is at migration 5 + OK: Current: the catalog is at migration 6 ... === HEALTHY: All checks passed === ``` diff --git a/docs/manuals/09-install-macos.md b/docs/manuals/09-install-macos.md index 85ba0f1e8..b1ca6de2d 100755 --- a/docs/manuals/09-install-macos.md +++ b/docs/manuals/09-install-macos.md @@ -96,7 +96,7 @@ Expected: ``` === extenddb verify === ... - OK: Current: the catalog is at migration 5 + OK: Current: the catalog is at migration 6 ... === HEALTHY: All checks passed === ``` diff --git a/docs/schema/mongodb.txt b/docs/schema/mongodb.txt index 490403643..c05d67727 100644 --- a/docs/schema/mongodb.txt +++ b/docs/schema/mongodb.txt @@ -1,5 +1,6 @@ collection extenddb_catalog.access_keys type collection options {} index extenddb_catalog.access_keys _id_ key {"_id":1} options {} +index extenddb_catalog.access_keys access_key_id_1 key {"access_key_id":1} options {"unique":true} index extenddb_catalog.access_keys account_id_1_user_name_1 key {"account_id":1,"user_name":1} options {} collection extenddb_catalog.accounts type collection options {} index extenddb_catalog.accounts _id_ key {"_id":1} options {} diff --git a/tests/test_auth_permissions.py b/tests/test_auth_permissions.py index 157f4cec3..c6b01e908 100755 --- a/tests/test_auth_permissions.py +++ b/tests/test_auth_permissions.py @@ -239,6 +239,98 @@ def test_create_access_key_for_nonexistent_user(self): assert resp.status_code == 404 +class TestAccessKeyIdIsStoredOnce: + """An access key ID names one key: an import of an ID that a key already + has answers 409 and leaves that key as it was, on every backend.""" + + @pytest.fixture(autouse=True) + def setup(self, auth_env, mgmt, account_id, region): + self.endpoint = auth_env[0] + self.mgmt = mgmt + self.account_id = account_id + self.region = region + + def _user(self, full_access=False): + name = f"ak-{uuid.uuid4().hex[:8]}" + resp = self.mgmt.create_user(self.account_id, name, "Pass123!") + assert resp.status_code == 201, resp.text + if full_access: + resp = self.mgmt.put_user_policy(self.account_id, name, "full", _full_policy()) + assert resp.status_code in (200, 201, 204), resp.text + return name + + @staticmethod + def _key_id(): + return "AKIA" + uuid.uuid4().hex[:16].upper() + + @staticmethod + def _secret(): + return uuid.uuid4().hex + uuid.uuid4().hex[:8] + + def _listed(self, user): + resp = self.mgmt.list_access_keys(self.account_id, user) + assert resp.status_code == 200, resp.text + return [k["access_key_id"] for k in resp.json()] + + def _authenticates(self, key_id, secret): + """True when a request signed with the pair is allowed, False when + the server refuses the signature or the key.""" + client = _make_client(self.endpoint, key_id, secret, self.region) + try: + client.list_tables() + return True + except ClientError as e: + code = e.response["Error"]["Code"] + assert code in ("UnrecognizedClientException", "InvalidSignatureException"), e + return False + + def test_an_id_another_user_holds_is_refused(self): + owner = self._user(full_access=True) + other = self._user(full_access=True) + try: + key_id, secret = self._key_id(), self._secret() + resp = self.mgmt.import_access_key(self.account_id, owner, key_id, secret) + assert resp.status_code == 201, resp.text + resp = self.mgmt.import_access_key(self.account_id, other, key_id, self._secret()) + assert resp.status_code == 409, resp.text + assert "Access key ID already exists" in resp.text + assert self._listed(owner) == [key_id] + assert key_id not in self._listed(other) + assert self._authenticates(key_id, secret) + finally: + self.mgmt.delete_user(self.account_id, other) + self.mgmt.delete_user(self.account_id, owner) + + def test_a_second_import_of_an_id_keeps_the_first_secret(self): + user = self._user(full_access=True) + try: + key_id, first, second = self._key_id(), self._secret(), self._secret() + resp = self.mgmt.import_access_key(self.account_id, user, key_id, first) + assert resp.status_code == 201, resp.text + resp = self.mgmt.import_access_key(self.account_id, user, key_id, second) + assert resp.status_code == 409, resp.text + assert self._listed(user) == [key_id] + assert self._authenticates(key_id, first) + assert not self._authenticates(key_id, second) + finally: + self.mgmt.delete_user(self.account_id, user) + + def test_a_deleted_key_stops_authenticating_after_a_refused_import(self): + user = self._user(full_access=True) + try: + key_id, secret = self._key_id(), self._secret() + for expected in (201, 409): + resp = self.mgmt.import_access_key(self.account_id, user, key_id, secret) + assert resp.status_code == expected, resp.text + assert self._authenticates(key_id, secret) + resp = self.mgmt.delete_access_key(self.account_id, user, key_id) + assert resp.status_code == 204, resp.text + assert self._listed(user) == [] + assert not self._authenticates(key_id, secret) + finally: + self.mgmt.delete_user(self.account_id, user) + + # --------------------------------------------------------------------------- # Group Management # --------------------------------------------------------------------------- diff --git a/tests/test_cli_vector_catalog_migration.py b/tests/test_cli_vector_catalog_migration.py index f5c4b4ad4..0b35d1a17 100644 --- a/tests/test_cli_vector_catalog_migration.py +++ b/tests/test_cli_vector_catalog_migration.py @@ -61,6 +61,10 @@ # The same lines, for a migration that is not convergent. NOT_CONVERGENT = re.compile(r"^\s*(\d+)\s+\S+\s+(?:breaking|additive)\s+not convergent\s") +# The line of a migration that does not apply on PostgreSQL. The runner +# records it in the catalog's ledger and has no body to run again. +NOT_APPLICABLE = re.compile(r"^\s*(\d+)\s+\S+\s+not applicable: ") + @functools.cache def _not_convergent(): @@ -74,13 +78,25 @@ def _not_convergent(): } +@functools.cache +def _not_applicable(): + """The ids `migrate --list` marks as not applicable on PostgreSQL.""" + listed = _run_extenddb("migrate", "--list") + return { + int(match.group(1)) + for match in map(NOT_APPLICABLE.match, listed.stdout.splitlines()) + if match + } + + @functools.cache def _listed(): """The stores of every migration the binary carries, by id, from `migrate --list`, which opens no database. Adding a migration then needs - no edit here; a literal id in a test means that migration itself.""" + no edit here; a literal id in a test means that migration itself. A + migration that does not apply records its row in the catalog only.""" listed = _run_extenddb("migrate", "--list") - stores = {} + stores = {i: ["catalog"] for i in _not_applicable()} for line in listed.stdout.splitlines(): match = LISTED.match(line) if match: @@ -345,8 +361,8 @@ def test_reconverge_runs_every_convergent_migration_again_on_a_current_catalog( ): """`migrate --yes --reconverge` runs each convergent migration again on a catalog at the latest migration, names each one that is not - convergent as skipped, and leaves the catalog current, with its ledger - rows as they were. + convergent as skipped, names none that does not apply, and leaves the + catalog current, with its ledger rows as they were. This is the command an operator runs after a release before 1.0 wrote to a migrated catalog. Migration 1's check of its result does not run @@ -367,7 +383,9 @@ def test_reconverge_runs_every_convergent_migration_again_on_a_current_catalog( assert rerun.returncode == 0, output assert 2 in _not_convergent(), _not_convergent() for i in _ids_in("catalog"): - if i in _not_convergent(): + if i in _not_applicable(): + assert not re.search(rf"^\s*(?:Skipped|Ran again) {i} ", output, re.M), output + elif i in _not_convergent(): assert re.search(rf"^\s*Skipped {i} \S+: ", output, re.M), output assert f"Ran again {i} " not in output, output else: From 99953b1da1e20d24cb3b157df60038848f93989f Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:31:22 +0000 Subject: [PATCH 2/2] fix(storage-mongodb): delete the index refusals from migration 6 Migration 6 no longer refuses an index that an operator added to access_keys. Before this change, detect stopped the run with exit 5 when access_keys had one of these indexes: - an access_key_id_1 other than the index the migration creates - the unique index on access_key_id under another name - the unique access_key_id_1 when hidden Each refusal printed the mongosh command that drops or unhides the index. ExtendDB owns the collections, tables and indexes of its catalog and data databases, and a change to them is not supported. The migration does not refuse such a change. This change deletes Clash, Conflict, conflict() and the tests of the index refusals. It also deletes the Decimal128 handling of key directions and index options. The only index that the migration reads as its own is the one that the upgrade manual's command builds, which has an Int32 direction. Beside an index whose key direction is a Decimal128, the read now groups the whole collection, which gives the same groups and is slower. true_value() needs no Decimal128 case: on 7.0.45 the server lists unique and sparse as booleans, whatever number created them. This change also deletes the live test that applied migration 6 beside 14 index variants. Each variant ran on a catalog that holds one key, so the migration had nothing to delete. The test checked only that the index build succeeds with that index in place. Ten of the variants were indexes that an operator adds under another name. ExtendDB does not support such indexes. The rest of the migration behaves as before. The read groups only the documents with an array ID when access_key_id_1 is built with the key and options of the manual's command. The same applies when that index is hidden. detect refuses an ID that more than one user holds, and an array ID. It also refuses two or more documents that have no ID or share an ID that is not a string. run refuses copies written after detect. When one user holds an ID more than once, the body sends explain for the lookup's query and keeps the copy that the lookup reads. On mongo:7 (server 7.0.45) and mongo:8.0 (server 8.0.34) replica sets, a non-unique access_key_id_1 stops migrate --yes with exit 1 and IndexKeySpecsConflict. The run deletes the extra copies before the build fails, and only its warnings name them. A unique index on access_key_id under another name stops migrate --yes with exit 1 and IndexOptionsConflict. After the operator drops either index, migrate --yes applies migration 6. With a hidden access_key_id_1, migrate --yes on 7.0.45 applies migration 6 and the index stays hidden. On 8.0.34 it exits 1 with IndexOptionsConflict, and it applies migration 6 after the index is unhidden. The [storage] section of the admin guide's configuration reference now says that ExtendDB owns these collections, tables and indexes. It adds that a change to them is not supported and can make a later migration fail. The guide's advice for slow PostgreSQL queries no longer tells the operator to add indexes. The upgrade manual no longer lists the index refusals. It names the two added indexes that made the build fail. It tells the operator to keep the output of a failed run, whose warnings are the only record of the copies that run deleted. A new unit test checks which forms of access_key_id_1 the migration reads as its own. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> --- .../migrations/m0006_unique_access_key_ids.rs | 302 +++++-------- .../tests/migrations_released.rs | 419 +----------------- docs/manuals/05-admin-guide.md | 4 +- docs/manuals/07-upgrade-manual.md | 4 +- 4 files changed, 128 insertions(+), 601 deletions(-) diff --git a/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs b/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs index fc4cae748..0c3e8fa7b 100644 --- a/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs +++ b/crates/storage-mongodb/src/migrations/m0006_unique_access_key_ids.rs @@ -15,15 +15,13 @@ //! documents by ID, keeping the IDs that more than one document holds and //! the array IDs. `detect` refuses what the body cannot decide: an ID whose //! documents belong to more than one user, two or more documents that have -//! no ID or share an ID that is not a string, a document whose ID is an -//! array, which a lookup reads for each ID in it, an index that would make -//! the build fail, and this migration's index when it is hidden, which a -//! lookup does not use. `run` reads again and refuses the same before it -//! writes. Then, for each ID that one user holds more than once, it finds -//! the copy a lookup reads, logs the other copies, deletes them and keeps -//! that copy, so every key authenticates as it did before. Last it creates -//! the index, committed once a majority of the replica set's data-bearing -//! voting members have built it. +//! no ID or share an ID that is not a string, and a document whose ID is an +//! array, which a lookup reads for each ID in it. `run` reads again and +//! refuses the same before it writes. Then, for each ID that one user holds +//! more than once, it finds the copy a lookup reads, logs the other copies, +//! deletes them and keeps that copy, so every key authenticates as it did +//! before. Last it creates the index, committed once a majority of the +//! replica set's data-bearing voting members have built it. //! //! A lookup reads the copy its query plan meets first. For each ID stored //! more than once the body reads which of its listed copies still hold it, @@ -45,7 +43,7 @@ //! edit. use async_trait::async_trait; -use bson::{Bson, Decimal128, Document, doc}; +use bson::{Bson, Document, doc}; use extenddb_storage::migrations::{CatalogStatus, CodeBody, Done, MigrationError}; use futures::TryStreamExt; use mongodb::IndexModel; @@ -98,32 +96,6 @@ struct Listed { built: bool, } -/// Why this migration refuses an index of `access_keys`. -enum Clash { - /// It is the unique index on the ID under another name, and the build - /// refuses a second name for it. - Renamed, - /// It is named `access_key_id_1` and is not the index this migration - /// creates, and the build refuses a second index of the name. - Named, - /// It is this migration's index, hidden, and the query planner does not - /// use a hidden index for a lookup. - Hidden, -} - -/// An index of `access_keys` that this migration refuses. -struct Conflict { - name: String, - spec: Document, - clash: Clash, -} - -/// What `detect` and `run` read. -struct State { - groups: Vec, - conflicts: Vec, -} - /// What the record IDs of an ID's copies tell about the copy a forward /// collection scan reads. #[derive(Debug, PartialEq)] @@ -225,65 +197,9 @@ impl Group { } } -impl Conflict { - fn refusal(&self, db: &str) -> String { - let command = |method: &str| { - format!("db.getSiblingDB(\"{db}\").{COLLECTION}.{method}(\"{}\")", self.name) - }; - match self.clash { - Clash::Renamed => format!( - "the unique index {} on {db}.{COLLECTION} ({}) is the index this migration \ - creates under the name {INDEX}, and the build refuses a second name for it; \ - drop it with {} in mongosh", - self.name, - self.spec, - command("dropIndex") - ), - Clash::Named => format!( - "the index {INDEX} on {db}.{COLLECTION} ({}) is not the unique index on \ - access_key_id that this migration creates under that name, and the build \ - refuses a second index of the name; drop it with {} in mongosh", - self.spec, - command("dropIndex") - ), - Clash::Hidden => format!( - "the index {INDEX} on {db}.{COLLECTION} ({}) is the unique index on \ - access_key_id that this migration creates, hidden from the query planner, so \ - a credential lookup does not use it; unhide it with {} in mongosh", - self.spec, - command("unhideIndex") - ), - } - } -} - -/// A Decimal128 as its sign, its digits without leading and trailing -/// zeros, and the power of ten of the last of them, or `None` for NaN and -/// the infinities. A zero has no digits. -fn decimal_parts(value: &Decimal128) -> Option<(bool, String, i64)> { - let text = value.to_string(); - let (negative, unsigned) = match text.strip_prefix('-') { - Some(rest) => (true, rest), - None => (false, text.as_str()), - }; - let (mantissa, exponent) = match unsigned.split_once('E') { - Some((mantissa, exponent)) => (mantissa, exponent.parse::().ok()?), - None => (unsigned, 0), - }; - let (whole, fraction) = mantissa.split_once('.').unwrap_or((mantissa, "")); - let digits = format!("{whole}{fraction}"); - if digits.is_empty() || !digits.bytes().all(|b| b.is_ascii_digit()) { - return None; - } - let digits = digits.trim_start_matches('0'); - let significant = digits.trim_end_matches('0'); - let trailing = digits.len() - significant.len(); - Some((negative, significant.to_owned(), exponent - fraction.len() as i64 + trailing as i64)) -} - -/// Whether the server counts `value` as true, as it reads an index option: -/// false, a zero, null and a missing value are false, and anything else is -/// true. +/// Whether `value` is true as an index option. False, null, a missing +/// value and a zero Int32, Int64 or Double are false, and any other value +/// is true. fn true_value(value: Option<&Bson>) -> bool { match value { None | Some(Bson::Null) | Some(Bson::Undefined) => false, @@ -291,19 +207,18 @@ fn true_value(value: Option<&Bson>) -> bool { Some(Bson::Int32(n)) => *n != 0, Some(Bson::Int64(n)) => *n != 0, Some(Bson::Double(n)) => *n != 0.0, - Some(Bson::Decimal128(n)) => decimal_parts(n).map_or(true, |(_, digits, _)| !digits.is_empty()), Some(_) => true, } } -/// Whether `value` is the number 1, in any numeric type: the server -/// compares key directions by value, a Decimal128 exactly. +/// Whether `value` is the number 1 as an Int32, an Int64 or a Double. An +/// index whose key direction has another type, such as a Decimal128, is not +/// this migration's index here. The read beside it groups every document. fn is_one(value: Option<&Bson>) -> bool { match value { Some(Bson::Int32(n)) => *n == 1, Some(Bson::Int64(n)) => *n == 1, Some(Bson::Double(n)) => *n == 1.0, - Some(Bson::Decimal128(n)) => decimal_parts(n) == Some((false, "1".to_owned(), 0)), _ => false, } } @@ -313,23 +228,22 @@ fn on_the_id(spec: &Document) -> bool { spec.get_document("key").map_or(false, |key| key.len() == 1 && is_one(key.get("access_key_id"))) } -/// The options of `spec` that the build compares: none of `v`, `key`, -/// `name`, `ns` and `background`, which has no effect, and `sparse` and -/// `hidden` only when true, because false is their default. +/// The options of `spec` that [`is_ours`] compares: every field but `v`, +/// `key`, `name`, `ns`, `background` and `hidden`. `background` has no +/// effect, and a hidden index refuses a second copy of an ID as a visible +/// one does. `sparse` counts only when true, because false is its default. fn options(spec: &Document) -> Document { let mut options = spec.clone(); - for field in ["v", "key", "name", "ns", "background"] { + for field in ["v", "key", "name", "ns", "background", "hidden"] { options.remove(field); } - for field in ["sparse", "hidden"] { - if !true_value(options.get(field)) { - options.remove(field); - } + if !true_value(options.get("sparse")) { + options.remove("sparse"); } options } -/// Whether `spec` is the index this migration creates. +/// Whether `spec` is the index this migration creates, hidden or not. fn is_ours(spec: &Document) -> bool { let mut options = options(spec); spec.get_str("name") == Ok(INDEX) @@ -338,40 +252,6 @@ fn is_ours(spec: &Document) -> bool { && options.is_empty() } -/// `spec` without its `hidden` option. -fn unhidden(spec: &Document) -> Document { - let mut spec = spec.clone(); - spec.remove("hidden"); - spec -} - -/// The conflict `spec` makes, as `createIndexes` decides on MongoDB 7 and -/// 8: any index named `access_key_id_1` other than the one this migration -/// creates, and the unique index on the ID under another name. A collation, -/// a partial filter or `sparse: true` makes that unique index a different -/// one, which the build creates beside it. This migration's index, hidden, -/// is refused too: MongoDB 8 answers the build with IndexOptionsConflict, -/// and MongoDB 7 answers that the index exists. -fn conflict(spec: &Document) -> Option { - let name = spec.get_str("name").unwrap_or_default().to_owned(); - let mut shown = spec.clone(); - shown.remove("v"); - shown.remove("ns"); - if name == INDEX { - if is_ours(spec) { - return None; - } - let clash = if is_ours(&unhidden(spec)) { Clash::Hidden } else { Clash::Named }; - return Some(Conflict { name, spec: shown, clash }); - } - let options = options(spec); - let distinct = options.contains_key("collation") - || options.contains_key("partialFilterExpression") - || options.contains_key("sparse"); - (on_the_id(spec) && true_value(options.get("unique")) && !distinct) - .then(|| Conflict { name, spec: shown, clash: Clash::Renamed }) -} - /// Whether an `explain` reply reports a forward collection scan: `LIMIT` /// over `COLLSCAN` with the direction forward. The plan is the winning plan /// itself, or its `queryPlan` when the slot-based engine runs it. Any other @@ -410,10 +290,10 @@ fn scanned_first(found: &[Document]) -> Scanned { } /// Whether the read needs to group only the documents whose ID is an -/// array: this migration's index, hidden or not, is built and refuses a -/// second copy of any other ID. +/// array. It does when this migration's index is built, because that index +/// refuses a second copy of any other ID. fn arrays_only(indexes: &[Listed]) -> bool { - indexes.iter().any(|i| i.built && is_ours(&unhidden(&i.spec))) + indexes.iter().any(|i| i.built && is_ours(&i.spec)) } /// The server's error code of a failed command, if it reported one. @@ -597,26 +477,23 @@ async fn looked_up(stores: &MongoStores, id: &str, group: &Group) -> Result Result { +/// What `detect` and `run` read. +async fn read(stores: &MongoStores) -> Result, MigrationError> { let indexes = indexes(stores).await?; - Ok(State { - groups: groups(stores, arrays_only(&indexes)).await?, - conflicts: indexes.iter().filter_map(|i| conflict(&i.spec)).collect(), - }) + groups(stores, arrays_only(&indexes)).await } /// The items `detect` and `run` refuse the migration for. -async fn refusals(stores: &MongoStores, state: &State) -> Result, MigrationError> { +async fn refusals(stores: &MongoStores, groups: &[Group]) -> Result, MigrationError> { let db = stores.catalog.name(); let mut items = Vec::new(); - for group in state.groups.iter().filter(|g| !g.resolvable()) { + for group in groups.iter().filter(|g| !g.resolvable()) { let read = match group.access_key_id() { Some(id) => looked_up(stores, id, group).await?.map(|(read, _)| read), None => None, }; items.push(group.refusal(db, read.as_ref())); } - items.extend(state.conflicts.iter().map(|c| c.refusal(db))); Ok(items) } @@ -627,22 +504,22 @@ impl CodeBody for UniqueAccessKeyIds { } async fn detect(&self, stores: &MongoStores) -> Result, MigrationError> { - let state = read(stores).await?; - refusals(stores, &state).await + let groups = read(stores).await?; + refusals(stores, &groups).await } async fn run(&self, stores: &MongoStores) -> Result { let db = &stores.catalog; let collection = db.collection::(COLLECTION); - let state = read(stores).await?; + let groups = read(stores).await?; // `detect` refused these before the run; what was written since is // refused the same way, before this body writes anything. - let items = refusals(stores, &state).await?; + let items = refusals(stores, &groups).await?; if !items.is_empty() { return Err(MigrationError::Status(CatalogStatus::NeedsAttention { items })); } let mut notes = Vec::new(); - for group in state.groups.iter().filter(|g| g.resolvable() && g.copies.len() > 1) { + for group in groups.iter().filter(|g| g.resolvable() && g.copies.len() > 1) { let Some(id) = group.access_key_id() else { continue; }; @@ -737,40 +614,52 @@ mod tests { } #[test] - fn a_decimal_key_direction_is_one_only_when_its_value_is_one() { - for text in ["1", "1.0", "1.000", "10E-1", "0.1E+1", "100E-2", "0.0001E+4"] { - assert!(is_one(Some(&decimal(text))), "{text}"); + fn a_key_direction_is_one_only_as_the_number_1() { + for value in [Bson::Int32(1), Bson::Int64(1), Bson::Double(1.0)] { + assert!(is_one(Some(&value)), "{value}"); } - for text in [ - "1.0000000000000000000000000000001", - "0.9999999999999999999999999999999", - "1E+1", - "1E-1", - "11", - "101E-2", - "-1", - "0", - "-0", - "NaN", - "Infinity", - "-Infinity", + for value in [ + Bson::Int32(0), + Bson::Int32(-1), + Bson::Int32(2), + Bson::Int64(0), + Bson::Int64(-1), + Bson::Int64(2), + Bson::Double(0.0), + Bson::Double(-1.0), + Bson::Double(2.0), + Bson::Double(1.5), + Bson::Double(1.000_000_000_000_000_2), + decimal("1"), ] { - assert!(!is_one(Some(&decimal(text))), "{text}"); - } - assert!(is_one(Some(&Bson::Int64(1))) && is_one(Some(&Bson::Double(1.0)))); - for value in [Bson::Int64(-1), Bson::Double(-1.0), Bson::Double(2.0), Bson::Double(1.000_000_000_000_000_2)] { assert!(!is_one(Some(&value)), "{value}"); } + assert!(!is_one(None)); } #[test] - fn a_decimal_option_is_true_unless_its_value_is_zero() { - for text in ["1", "-1", "1E-6176", "0.0000001", "NaN", "Infinity"] { - assert!(true_value(Some(&decimal(text))), "{text}"); + fn an_option_is_false_only_when_false_zero_null_or_missing() { + for value in [ + Bson::Boolean(true), + Bson::Int32(1), + Bson::Int32(-1), + Bson::Int64(1), + Bson::Double(0.5), + ] { + assert!(true_value(Some(&value)), "{value}"); } - for text in ["0", "-0", "0E+10", "0.000"] { - assert!(!true_value(Some(&decimal(text))), "{text}"); + for value in [ + Bson::Boolean(false), + Bson::Int32(0), + Bson::Int64(0), + Bson::Double(0.0), + Bson::Double(-0.0), + Bson::Null, + Bson::Undefined, + ] { + assert!(!true_value(Some(&value)), "{value}"); } + assert!(!true_value(None)); } #[test] @@ -899,23 +788,34 @@ mod tests { } #[test] - fn this_migrations_index_hidden_is_refused_with_the_command_that_unhides_it() { - let hidden = doc! { "v": 2, "key": { "access_key_id": 1 }, "name": INDEX, "unique": true, "hidden": true }; - let refused = conflict(&hidden).expect("refused"); - assert!(matches!(refused.clash, Clash::Hidden)); - assert_eq!( - refused.refusal("cat"), - "the index access_key_id_1 on cat.access_keys ({ \"key\": { \"access_key_id\": 1 }, \ - \"name\": \"access_key_id_1\", \"unique\": true, \"hidden\": true }) is the unique \ - index on access_key_id that this migration creates, hidden from the query planner, \ - so a credential lookup does not use it; unhide it with \ - db.getSiblingDB(\"cat\").access_keys.unhideIndex(\"access_key_id_1\") in mongosh" - ); - let mut hidden_plain = hidden.clone(); - hidden_plain.remove("unique"); - assert!(matches!(conflict(&hidden_plain).map(|c| c.clash), Some(Clash::Named))); - let mut shown = hidden.clone(); - shown.insert("hidden", false); - assert!(conflict(&shown).is_none()); + fn the_index_built_by_hand_is_ours_with_options_that_do_not_change_it() { + // The upgrade manual builds the index by hand on a large collection, + // and the read then groups the array IDs alone. + let with = |field: &str, value: Bson| { + let mut spec = ours(); + spec.insert(field, value); + spec + }; + let keyed = |direction: Bson| with("key", doc! { "access_key_id": direction }.into()); + for spec in [ + ours(), + with("background", true.into()), + with("sparse", false.into()), + with("hidden", true.into()), + ] { + assert!(is_ours(&spec), "{spec}"); + } + for spec in [ + with("unique", false.into()), + with("sparse", true.into()), + with("expireAfterSeconds", 3600.into()), + with("partialFilterExpression", doc! { "access_key_id": { "$exists": true } }.into()), + with("name", "akid".into()), + with("key", doc! { "account_id": 1 }.into()), + keyed(decimal("1")), + keyed((-1).into()), + ] { + assert!(!is_ours(&spec), "{spec}"); + } } } diff --git a/crates/storage-mongodb/tests/migrations_released.rs b/crates/storage-mongodb/tests/migrations_released.rs index 25349ab7d..70689cd14 100644 --- a/crates/storage-mongodb/tests/migrations_released.rs +++ b/crates/storage-mongodb/tests/migrations_released.rs @@ -735,100 +735,9 @@ async fn migration_6_refuses_copies_it_cannot_resolve_and_writes_nothing() { d.drop_databases().await; } -/// An index of `access_keys` that would make the build fail is refused -/// before anything is written: `access_key_id_1` without `unique`, which an -/// operator may have created for the lookups, and the unique index under -/// another name. Each refusal prints its `dropIndex` command, and once it -/// has run `migrate --yes` applies migration 6. A non-unique index on the -/// ID under another name stays beside the new one. -#[tokio::test(flavor = "multi_thread")] -async fn migration_6_refuses_an_index_the_build_would_fail_on() { - let Some(uri) = server() else { return }; - let d = Deployment::new(&uri, "index6").await; - fixture::load(&d, &fixture::read("0.1.14")).await; - let (owner, fixture_id, account, created) = fixture_key(&d).await; - let keys = d.catalog().collection::("access_keys"); - keys.insert_one(stored_key( - 1, - &fixture_id, - &account, - "cu-tester", - created + 60_000, - )) - .await - .unwrap(); - keys.create_index(id_index(None, false)).await.unwrap(); - let documents = Deployment::documents(&d.catalog(), "access_keys").await; - let catalog = d.target.catalog.clone(); - let item = refused_index(&d).await; - assert!( - item.starts_with(&format!( - "migration 6 unique_access_key_ids: the index access_key_id_1 on \ - {catalog}.access_keys (" - )) && item.ends_with(&format!( - ") is not the unique index on access_key_id that this migration creates under \ - that name, and the build refuses a second index of the name; drop it with \ - db.getSiblingDB(\"{catalog}\").access_keys.dropIndex(\"access_key_id_1\") in \ - mongosh" - )), - "{item}" - ); - assert_eq!( - d.migrate().await.expect_err("refused").exit_code(), - exit::ATTENTION - ); - assert_eq!( - Deployment::documents(&d.catalog(), "access_keys").await, - documents - ); - keys.drop_index(ACCESS_KEY_ID_INDEX).await.unwrap(); - d.migrate().await.expect("migrate"); - assert_eq!(looked_up(&d, &fixture_id).await, owner); - assert_eq!( - keys.count_documents(doc! { "access_key_id": &fixture_id }) - .await - .unwrap(), - 1 - ); - assert_eq!(unique_index(&d).await, Some(true)); - d.drop_databases().await; - - let d = Deployment::new(&uri, "renamed6").await; - fixture::load(&d, &fixture::read("0.1.14")).await; - let keys = d.catalog().collection::("access_keys"); - keys.create_index(id_index(Some("akid"), true)) - .await - .unwrap(); - keys.create_index(id_index(Some("akid_plain"), false)) - .await - .unwrap(); - let catalog = d.target.catalog.clone(); - let item = refused_index(&d).await; - assert!( - item.starts_with(&format!( - "migration 6 unique_access_key_ids: the unique index akid on {catalog}.access_keys (" - )) && item.ends_with(&format!( - ") is the index this migration creates under the name access_key_id_1, and the \ - build refuses a second name for it; drop it with \ - db.getSiblingDB(\"{catalog}\").access_keys.dropIndex(\"akid\") in mongosh" - )), - "{item}" - ); - keys.drop_index("akid").await.unwrap(); - d.migrate().await.expect("migrate"); - assert_eq!(unique_index(&d).await, Some(true)); - assert!( - access_key_indexes(&d) - .await - .contains(&"akid_plain".to_owned()) - ); - d.drop_databases().await; -} - /// The body's own `run` refuses what `detect` refuses, which the runner's -/// `detect` otherwise stops first: copies and an index written after the -/// runner read the collection. It returns NeedsAttention and writes -/// nothing. +/// `detect` otherwise stops first: copies written after the runner read the +/// collection. It returns NeedsAttention and writes nothing. #[tokio::test(flavor = "multi_thread")] async fn migration_6_run_refuses_copies_written_after_detect() { use extenddb_storage::migrations::MigrationId; @@ -864,32 +773,6 @@ async fn migration_6_run_refuses_copies_written_after_detect() { documents ); assert_eq!(unique_index(&d).await, None); - - // An index the build would fail on, beside an ID the body itself - // resolves: `run` refuses the index and deletes no copy. - let keys = d.catalog().collection::("access_keys"); - keys.delete_one(doc! { "_id": oid(1) }).await.unwrap(); - keys.insert_one(stored_key( - 2, - &fixture_id, - &account, - "cu-tester", - created + 60_000, - )) - .await - .unwrap(); - keys.create_index(id_index(None, false)).await.unwrap(); - let documents = Deployment::documents(&d.catalog(), "access_keys").await; - let items = run_body(&d).await; - assert_eq!(items.len(), 1, "{items:?}"); - assert!( - items[0].starts_with("the index access_key_id_1 on "), - "{items:?}" - ); - assert_eq!( - Deployment::documents(&d.catalog(), "access_keys").await, - documents - ); assert!( d.catalog_ledger().await.iter().all(|row| row.id != 6), "migration 6 is not recorded" @@ -940,23 +823,6 @@ fn oid(n: u8) -> ObjectId { ObjectId::from_bytes([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, n]) } -/// An index on `{ access_key_id: 1 }`, with a name when it is given. -fn id_index(name: Option<&str>, unique: bool) -> mongodb::IndexModel { - let options = match name { - Some(name) => mongodb::options::IndexOptions::builder() - .name(name.to_owned()) - .unique(unique) - .build(), - None => mongodb::options::IndexOptions::builder() - .unique(unique) - .build(), - }; - mongodb::IndexModel::builder() - .keys(doc! { "access_key_id": 1 }) - .options(options) - .build() -} - /// Whether `access_key_id_1` is unique, or `None` when it does not exist. async fn unique_index(d: &Deployment) -> Option { use futures::TryStreamExt; @@ -984,24 +850,15 @@ async fn status_items(d: &Deployment) -> Vec { } } -/// The one item for which the status refuses migration 6. -async fn refused_index(d: &Deployment) -> String { - let mut items = status_items(d).await; - assert_eq!(items.len(), 1, "{items:?}"); - items.pop().unwrap() -} - -/// Once its index is built, migration 6 groups only the documents whose ID -/// is an array, which the unique index takes and a lookup reads for each ID -/// in it; beside an index named `access_key_id_1` that is not its own, it -/// groups every document. A catalog at migration 5 holds an ID of two -/// users, an array ID, a document without an ID and one whose ID is the -/// number 42. Beside a non-unique `access_key_id_1`, the status refuses the -/// ID of two users, the array and the index. With the unique index built by -/// hand and hidden, it refuses the array and the hidden index, with the -/// command that unhides it, and once the index is shown, the array alone. -/// After the array is deleted `migrate --yes` applies 6, and -/// `--reconverge` refuses an array ID written later and nothing else. +/// A catalog at migration 5 holds an ID of two users and an array ID. It +/// also holds a document without an ID and one whose ID is the number 42. +/// Beside a non-unique `access_key_id_1`, the status refuses the ID of two +/// users and the array. With the second user's copy deleted and that index +/// replaced by the unique index built by hand, the status refuses the array +/// alone. After the array is deleted, `migrate --yes` applies 6 and keeps +/// the document without an ID and the one with the number 42. Then +/// `--reconverge` refuses an array ID written later, and it refuses neither +/// of those two documents. #[tokio::test(flavor = "multi_thread")] async fn migration_6_refuses_array_ids_beside_its_own_index() { use extenddb_storage::migrations::MigrationId; @@ -1031,8 +888,13 @@ async fn migration_6_refuses_array_ids_beside_its_own_index() { ]) .await .unwrap(); - keys.create_index(id_index(None, false)).await.unwrap(); - let catalog = d.target.catalog.clone(); + keys.create_index( + mongodb::IndexModel::builder() + .keys(doc! { "access_key_id": 1 }) + .build(), + ) + .await + .unwrap(); let prefix = "migration 6 unique_access_key_ids: "; let refused_array = |n: u8| { format!( @@ -1040,14 +902,12 @@ async fn migration_6_refuses_array_ids_beside_its_own_index() { Bson::ObjectId(oid(n)) ) }; - let refused_index = format!("{prefix}the index access_key_id_1 on {catalog}.access_keys ("); let items = status_items(&d).await; - assert_eq!(items.len(), 3, "{items:?}"); + assert_eq!(items.len(), 2, "{items:?}"); for start in [ format!("{prefix}the access key ID {fixture_id} is stored 2 times"), refused_array(31), - refused_index.clone(), ] { assert!( items.iter().any(|item| item.starts_with(&start)), @@ -1056,36 +916,16 @@ async fn migration_6_refuses_array_ids_beside_its_own_index() { } // The importer's copy and the index go, and the manual's unique index - // is built hidden. + // is built by hand. keys.delete_one(doc! { "_id": oid(1) }).await.unwrap(); keys.drop_index(ACCESS_KEY_ID_INDEX).await.unwrap(); d.catalog() .run_command(doc! { "createIndexes": "access_keys", "indexes": [ - { "key": { "access_key_id": 1 }, "name": ACCESS_KEY_ID_INDEX, "unique": true, "hidden": true }, + { "key": { "access_key_id": 1 }, "name": ACCESS_KEY_ID_INDEX, "unique": true }, ], "commitQuorum": "majority" }) .await .unwrap(); let items = status_items(&d).await; - assert_eq!(items.len(), 2, "{items:?}"); - assert!(items[0].starts_with(&refused_array(31)), "{items:?}"); - assert!( - items[1].starts_with(&refused_index) - && items[1].ends_with(&format!( - ") is the unique index on access_key_id that this migration creates, hidden \ - from the query planner, so a credential lookup does not use it; unhide it \ - with db.getSiblingDB(\"{catalog}\").access_keys.unhideIndex(\"access_key_id_1\") \ - in mongosh" - )), - "{items:?}" - ); - - d.catalog() - .run_command(doc! { "collMod": "access_keys", "index": { - "name": ACCESS_KEY_ID_INDEX, "hidden": false, - } }) - .await - .unwrap(); - let items = status_items(&d).await; assert_eq!(items.len(), 1, "{items:?}"); assert!(items[0].starts_with(&refused_array(31)), "{items:?}"); @@ -1331,220 +1171,3 @@ async fn migration_6_keeps_the_copy_a_lookup_reads_under_query_settings() { } d.drop_databases().await; } - -/// How migration 6 must treat an index an operator may have on -/// `access_keys`, as measured with `createIndexes` on MongoDB 7.0.45 and -/// 8.0.34: whether `detect` refuses it, and whether the build of -/// `access_key_id_1` fails beside it. The unique hidden `access_key_id_1` -/// is refused, because a lookup does not use a hidden index and MongoDB 8 -/// answers the build with IndexOptionsConflict, although MongoDB 7 answers -/// that the index exists. -const INDEX_VARIANTS: &[IndexVariant] = &[ - ("access_key_id_1, not unique", Refused::BuildFails, || { - doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1" } - }), - ("access_key_id_1, background", Refused::No, || { - doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "background": true } - }), - ("access_key_id_1, sparse: false", Refused::No, || { - doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "sparse": false } - }), - ( - "access_key_id_1, the key direction a Decimal128", - Refused::No, - || { - doc! { "key": { "access_key_id": decimal_one() }, "name": "access_key_id_1", "unique": true } - }, - ), - ( - "access_key_id_1, the key direction the Decimal128 1.0", - Refused::No, - || { - doc! { "key": { "access_key_id": decimal("1.0") }, "name": "access_key_id_1", "unique": true } - }, - ), - ( - "access_key_id_1, the key direction a Decimal128 just above 1", - Refused::BuildFails, - || { - doc! { "key": { "access_key_id": decimal(JUST_ABOVE_ONE) }, "name": "access_key_id_1", "unique": true } - }, - ), - ("access_key_id_1, partial", Refused::BuildFails, || { - doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, - "partialFilterExpression": { "access_key_id": { "$exists": true } } } - }), - ("access_key_id_1, TTL", Refused::BuildFails, || { - doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "expireAfterSeconds": 3600 } - }), - ("access_key_id_1, hidden", Refused::ByVersion, || { - doc! { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true, "hidden": true } - }), - ("access_key_id_1 on account_id", Refused::BuildFails, || { - doc! { "key": { "account_id": 1 }, "name": "access_key_id_1" } - }), - ("akid", Refused::BuildFails, || { - doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true } - }), - ( - "akid, the key direction a Double", - Refused::BuildFails, - || { - doc! { "key": { "access_key_id": 1.0 }, "name": "akid", "unique": true } - }, - ), - ( - "akid, the key direction an Int64", - Refused::BuildFails, - || { - doc! { "key": { "access_key_id": 1_i64 }, "name": "akid", "unique": true } - }, - ), - ( - "akid, the key direction a Decimal128", - Refused::BuildFails, - || { - doc! { "key": { "access_key_id": decimal_one() }, "name": "akid", "unique": true } - }, - ), - ( - "akid, the key direction a Decimal128 just above 1", - Refused::No, - || { - doc! { "key": { "access_key_id": decimal(JUST_ABOVE_ONE) }, "name": "akid", "unique": true } - }, - ), - ("akid, the key direction the Double 2", Refused::No, || { - doc! { "key": { "access_key_id": 2.0 }, "name": "akid", "unique": true } - }), - ("akid, the key direction the Double -1", Refused::No, || { - doc! { "key": { "access_key_id": -1.0 }, "name": "akid", "unique": true } - }), - ("akid, the key direction the Int64 -1", Refused::No, || { - doc! { "key": { "access_key_id": -1_i64 }, "name": "akid", "unique": true } - }), - ("akid, sparse: false", Refused::BuildFails, || { - doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, "sparse": false } - }), - ("akid, sparse", Refused::No, || { - doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, "sparse": true } - }), - ("akid, partial", Refused::No, || { - doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, - "partialFilterExpression": { "access_key_id": { "$exists": true } } } - }), - ("akid, collation", Refused::No, || { - doc! { "key": { "access_key_id": 1 }, "name": "akid", "unique": true, - "collation": { "locale": "en", "strength": 2 } } - }), - ("akid, descending", Refused::No, || { - doc! { "key": { "access_key_id": -1 }, "name": "akid", "unique": true } - }), - ("akid, not unique", Refused::No, || { - doc! { "key": { "access_key_id": 1 }, "name": "akid" } - }), - ("akid, compound", Refused::No, || { - doc! { "key": { "access_key_id": 1, "account_id": 1 }, "name": "akid", "unique": true } - }), -]; - -/// A variant's label, whether `detect` refuses it, and its spec. -type IndexVariant = (&'static str, Refused, fn() -> Document); - -/// Whether `detect` refuses an index, and why. -#[derive(Clone, Copy, Debug, PartialEq)] -enum Refused { - /// Accepted: the build succeeds beside it. - No, - /// Refused: the build fails beside it. - BuildFails, - /// Refused: the build fails beside it on some versions. - ByVersion, -} - -fn decimal_one() -> Bson { - decimal("1") -} - -fn decimal(text: &str) -> Bson { - Bson::Decimal128(text.parse().unwrap()) -} - -/// A Decimal128 that a Double rounds to 1 and the server compares as -/// another value than 1. -const JUST_ABOVE_ONE: &str = "1.0000000000000000000000000000001"; - -/// Each index of [`INDEX_VARIANTS`], alone beside migration 1's indexes on -/// a catalog at migration 5 that holds one key: `detect` refuses it exactly -/// when it is marked refused. The body's own `createIndexes`, sent beside a -/// refused index, fails, and `migrate --yes` applies migration 6 beside an -/// accepted one. -#[tokio::test(flavor = "multi_thread")] -async fn migration_6_refuses_an_index_exactly_when_the_build_fails_on_it() { - use extenddb_storage::migrations::MigrationId; - - let Some(uri) = server() else { return }; - let d = Deployment::new(&uri, "variants6").await; - fixture::load(&d, &fixture::read("0.1.14")).await; - d.migrate_through(MigrationId::DropContinuousBackups) - .await - .expect("migrations 1 to 5"); - let keys = d.catalog().collection::("access_keys"); - for (label, refused, spec) in INDEX_VARIANTS { - for name in access_key_indexes(&d).await { - if !["_id_", "account_id_1_user_name_1"].contains(&name.as_str()) { - keys.drop_index(name).await.unwrap(); - } - } - d.catalog() - .run_command(doc! { "createIndexes": "access_keys", "indexes": [spec()] }) - .await - .unwrap_or_else(|e| panic!("{label}: {e}")); - let status = d.status().await; - match refused { - Refused::No => { - assert_eq!(status.label(), "Behind", "{label}: {status}"); - d.migrate().await.unwrap_or_else(|e| panic!("{label}: {e}")); - assert_eq!(unique_index(&d).await, Some(true), "{label}"); - forget_6(&d).await; - } - Refused::BuildFails | Refused::ByVersion => { - let CatalogStatus::NeedsAttention { items } = status else { - panic!("{label}: not refused: {status}"); - }; - assert_eq!(items.len(), 1, "{label}: {items:?}"); - let built = d - .catalog() - .run_command(doc! { "createIndexes": "access_keys", "indexes": [ - { "key": { "access_key_id": 1 }, "name": "access_key_id_1", "unique": true }, - ], "commitQuorum": "majority" }) - .await; - if *refused == Refused::BuildFails { - assert!(built.is_err(), "{label}: the build succeeds: {built:?}"); - } - } - } - } - d.drop_databases().await; -} - -/// `migrate --forget 6`. -async fn forget_6(d: &Deployment) { - use extenddb_storage::migrations::Runner; - use extenddb_storage_mongodb::migrations::{MigrationLock, Timing}; - - let held = MigrationLock::acquire( - &d.target, - std::time::Duration::from_secs(10), - Timing::PRODUCTION, - ) - .await - .expect("lock"); - let migrations = MongoMigrations::open(&d.target, Some(&held)) - .await - .expect("open"); - let forgot = Runner::new(&migrations, "test").forget(6).await; - migrations.close().await; - held.release().await; - forgot.expect("forget 6"); -} diff --git a/docs/manuals/05-admin-guide.md b/docs/manuals/05-admin-guide.md index e0ff8162f..ec2860fb5 100755 --- a/docs/manuals/05-admin-guide.md +++ b/docs/manuals/05-admin-guide.md @@ -98,6 +98,8 @@ These settings require a server restart to take effect. |-----|---------|-------------| | `backend` | `postgres` | Storage backend (only `postgres` supported) | +ExtendDB owns the collections, tables and indexes of its catalog and data databases. A change to them, such as an added index, is not supported and can make a later migration fail. + #### [storage.postgres] | Key | Default | Description | @@ -945,7 +947,7 @@ The IAM policy does not allow the operation. Check attached policies with `list- **Slow queries:** -Check PostgreSQL query performance with `EXPLAIN ANALYZE`. Ensure indexes exist on key columns. +Check PostgreSQL query performance with `EXPLAIN ANALYZE`. **High connection count:** diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index dd71124e9..444786d80 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -97,7 +97,9 @@ On MongoDB, `migrate` connects with the connection string in the config file, wh On MongoDB, migration 6 gives the catalog's `access_keys` collection a unique index on `access_key_id`; PostgreSQL, SQLite and Cassandra key their `access_keys` table by that ID. Before migration 6, every import of an access key stored a document, also under an access key ID that another document held. A credential lookup read the first of those documents its query plan met: the first in the collection's natural order when the plan scans the collection forward, as it does with no index on `access_key_id` and no query settings for the lookup's query. A delete of the key deleted one of them, so a key imported more than once still authenticated after a delete and was still listed among the user's access keys. Where one user holds every document of an ID, the migration keeps the document a credential lookup reads, deletes the others, and records a note that names the ID and the `_id` of each document it kept and deleted, which `verify` prints. To find that document, it asks the server for the lookup's query plan for each such ID. When the plan scans the collection forward, the migration keeps the first document in natural order; with any other plan, such as one through an index on `access_key_id`, it sends the lookup's own query. It also prints a warning with the same names before each delete, because a `migrate --yes` that stops after its deletes and before the index exists records no note when it runs again. A key that was imported twice and then deleted once before the upgrade has one document left, from the later import: no note names it, and it authenticates with the secret of that import. After the upgrade, review each user's access keys and delete any the user had deleted before. -Migration 6 cannot decide what to keep when the documents of an ID belong to more than one user, when two or more documents have no access key ID or share one that is not a string, when a document's access key ID is an array, which a credential lookup reads for each ID in it, or when `access_keys` has an index that would make the build fail: an index named `access_key_id_1` that is not the unique index on `access_key_id`, such as one added to speed up lookups, or that unique index under another name. It also stops when the unique index `access_key_id_1` is hidden, because a credential lookup does not use a hidden index. Then the preview, `verify` and `migrate --yes` exit 5, change nothing, and name each document with its `_id`, account, user and creation time, with the `_id` a credential lookup reads for an ID of more than one user, or name the index. Delete each document that is not the key owner's, or drop or unhide the index, in `mongosh` with the command the output prints, then run `migrate --yes` again. The index build commits once a majority of the replica set's data-bearing voting members have built it, so it does not wait for a member that is down. +Migration 6 cannot decide what to keep when the documents of an ID belong to more than one user, when two or more documents have no access key ID or share one that is not a string, or when a document's access key ID is an array, which a credential lookup reads for each ID in it. Then the preview, `verify` and `migrate --yes` exit 5, change nothing, and name each document with its `_id`, account, user and creation time, with the `_id` a credential lookup reads for an ID of more than one user. In `mongosh`, delete or correct the documents as the output says, with the command it prints, then run `migrate --yes` again. The index build commits once a majority of the replica set's data-bearing voting members have built it, so it does not wait for a member that is down. + +An index that an operator adds to `access_keys`, other than the unique index `access_key_id_1` that the next paragraph creates, can make the index build fail. On `mongo:7` and `mongo:8.0` replica sets, a non-unique index named `access_key_id_1` made `migrate --yes` exit 1 with the server's error. A unique index on `access_key_id` under another name also made it exit 1. When the build fails, the run has already deleted the extra documents of each ID that one user holds. Only its warnings name them, so keep its output. With either index dropped, the next `migrate --yes` applied migration 6. Migration 6 groups the documents of `access_keys` in one request, and the runner gives each request 10 s. On a local `mongo:7` replica set, the preview took 8 s on a collection of 1 million keys, and on a collection of 2 million the preview, `verify` and `migrate --yes` stopped after 10 s with `no answer within 10 s`. On a collection that large, delete the extra documents and create the index in `mongosh`, which sets no such limit. This command lists each access key ID stored more than once, with its owners: `db.getSiblingDB("extenddb_catalog").access_keys.aggregate([{ $group: { _id: "$access_key_id", n: { $sum: 1 }, owners: { $addToSet: { account: "$account_id", user: "$user_name" } } } }, { $match: { n: { $gt: 1 } } }], { allowDiskUse: true })`. For an ID of more than one user, delete each document that is not the key owner's, as above. For an ID of one user, keep the document that `findOne({ access_key_id: "" })` returns, which a credential lookup reads, and delete the others. No note records these deletes. Then create the index with `db.getSiblingDB("extenddb_catalog").access_keys.createIndex({ access_key_id: 1 }, { unique: true }, "majority")`, which fails with a duplicate key error that names an ID while one is still stored more than once. Once the index is built, migration 6 reads only the documents whose access key ID is an array. While the build runs, migration 6 groups every document, so run `migrate --yes` after the command returns.