diff --git a/crates/commons-servers/src/backup_secrets.rs b/crates/commons-servers/src/backup_secrets.rs index dc362e410..3748c6e81 100644 --- a/crates/commons-servers/src/backup_secrets.rs +++ b/crates/commons-servers/src/backup_secrets.rs @@ -145,13 +145,21 @@ impl BackupSecrets { Ok(()) } Self::Memory(store) => { - store - .lock() - .unwrap() - .entry(secret_name.to_string()) - .or_default() - .insert(key.to_string(), value.to_string()); - Ok(()) + use std::collections::btree_map::Entry; + + match store.lock().unwrap().entry(secret_name.to_string()) { + // Kube answers 409 here. The double has to as well, or the + // double-create path passes in tests and 502s in production + // — and the callers' rollback of a failed create can't be + // exercised at all. + Entry::Occupied(_) => Err(AppError::Upstream(format!( + "secret create failed: {secret_name} already exists" + ))), + Entry::Vacant(slot) => { + slot.insert(BTreeMap::from([(key.to_string(), value.to_string())])); + Ok(()) + } + } } } } @@ -312,6 +320,33 @@ mod tests { // Deleting an already-absent Secret is a no-op success. secrets.delete_password("backup-repo-x").await.unwrap(); } + + /// `create_password` is create-if-absent (Kube answers 409). The double + /// has to reject too — otherwise a double-create passes in tests and 502s + /// in production, and no test can reach a caller's rollback path. + #[tokio::test] + async fn memory_create_password_rejects_an_existing_secret() { + let secrets = BackupSecrets::memory(); + secrets + .create_password("backup-repo-y", "password", "first") + .await + .unwrap(); + assert!( + secrets + .create_password("backup-repo-y", "password", "second") + .await + .is_err(), + "creating over an existing secret must fail", + ); + assert_eq!( + secrets + .read_password("backup-repo-y", "password") + .await + .unwrap(), + "first", + "the rejected create must not have overwritten anything", + ); + } } /// Generate a strong repo passphrase: 8 words from the EFF large wordlist diff --git a/crates/private-server/src/fns/backups.rs b/crates/private-server/src/fns/backups.rs index ba84c1c3c..c7d9121e7 100644 --- a/crates/private-server/src/fns/backups.rs +++ b/crates/private-server/src/fns/backups.rs @@ -1076,12 +1076,22 @@ pub async fn upsert( }, ) .await?; - kube.create_password( - &repo_password_ref, - REPO_PASSWORD_SECRET_KEY, - &commons_servers::backup_secrets::generate_passphrase(), - ) - .await?; + // Same all-or-nothing invariant as `create`: a failed Secret create + // must not leave a half-created config stuck in `provisioning` with + // a `repo_password_ref` pointing at nothing. Without the rollback + // every retry takes the *update* path — which never creates the + // Secret — so the group could never be provisioned again. + if let Err(e) = kube + .create_password( + &repo_password_ref, + REPO_PASSWORD_SECRET_KEY, + &commons_servers::backup_secrets::generate_passphrase(), + ) + .await + { + let _ = ServerGroupBackupConfig::delete(&mut conn, args.server_group_id).await; + return Err(e); + } } } diff --git a/crates/private-server/tests/it/backups.rs b/crates/private-server/tests/it/backups.rs index 19d059001..13cf891f0 100644 --- a/crates/private-server/tests/it/backups.rs +++ b/crates/private-server/tests/it/backups.rs @@ -856,6 +856,52 @@ async fn update_region_and_delete() { .await; } +/// Onboarding is all-or-nothing on both create paths. A config row left behind +/// by a failed Secret create is unrecoverable: it points its +/// `repo_password_ref` at nothing, and every retry now sees a config and takes +/// the *update* path, which never creates the Secret. +#[tokio::test(flavor = "multi_thread")] +async fn upsert_rolls_back_the_config_when_the_secret_cannot_be_created() { + commons_tests::server::run(async |mut conn, _public, private| { + let group_id = seed_group(&mut conn).await; + + // Onboard once so the group's passphrase Secret exists... + let resp = private + .post("/api/backups/create") + .json(&serde_json::json!({ + "server_group_id": group_id, + "bucket": "bes-iac", + "target_role_arn": "arn:aws:iam::123:role/dev", + "maintenance_role_arn": "arn:aws:iam::123:role/maint", + "mode": "from_birth", + })) + .await; + resp.assert_status_ok(); + + // ...then drop only the config row, leaving the Secret behind. `upsert` + // now takes the create path, and its create-if-absent Secret write + // fails — the shape of any transient secret-store failure. + conn.batch_execute(&format!( + "DELETE FROM server_group_backup_config WHERE group_id = '{group_id}'" + )) + .await + .expect("drop config row"); + + let resp = private + .post("/api/backups/upsert") + .json(&serde_json::json!({ + "server_group_id": group_id, + "bucket": "bes-iac", + "target_role_arn": "arn:aws:iam::123:role/dev", + "maintenance_role_arn": "arn:aws:iam::123:role/maint", + })) + .await; + resp.assert_status(axum::http::StatusCode::BAD_GATEWAY); + assert_no_config!(private, group_id); + }) + .await; +} + #[tokio::test(flavor = "multi_thread")] async fn upsert_creates_then_reapplies_idempotently() { commons_tests::server::run(async |mut conn, _public, private| {