From 16d381fc0efff147dd7773882ae4c0ae7787ab53 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Wed, 12 Aug 2026 23:14:04 +0800 Subject: [PATCH] ci(kms): add a nightly live-Vault lane and stop leaking behavior keys (#5999) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No workflow ever set RUSTFS_KMS_VAULT_TOKEN, so live_vault_backends() returned an empty set in every CI run and behavior_rotation.rs never asserted the working half of rotate/versioning; the #[ignore] live-Vault tests had never executed in CI either. nightly-gnu.yml gains a kms-vault-lane job (vault server -dev with KV2 + Transit, full rustfs-kms suite with the lane on, the dev-Vault ignored tests, and the AppRole live script) plus a separate kms-vault-ha-failover job for the three-node Raft failover script, isolated so an election-timing flake cannot mask the main lane's verdict. GitHub-hosted ubuntu-latest rather than the self-hosted fleet: the HA script needs Docker, and e2e-s3tests.yml's banner records how the heterogeneous sm-standard pods burned the last docker-dependent workflow. The behavior harness now records every key TestKms::create_key mints and deletes them after each Vault-backed for_each_backend case, on a fresh manager over the same configuration with the immediate-deletion gate enabled for cleanup only. Transit needs the deletion issued twice (first call parks the key in PendingDeletion, the second destroys it); KV2 destroys on the first call. Verified against a real dev Vault: after a full suite run the server holds zero behavior-* keys. Also fixes test_vault_cancel_key_deletion_persists_state, which was broken by construction — Default::default() never picks up the insecure-dev-defaults env override, so the HTTP dev Vault the test requires was always refused. It now declares development mode on the config, and passes. Refs rustfs/backlog#1774, rustfs/backlog#1562. --- .github/workflows/nightly-gnu.yml | 139 ++++++++++++++++++++++++++++++ crates/kms/src/backends/vault.rs | 6 +- crates/kms/tests/common/mod.rs | 88 ++++++++++++++++++- 3 files changed, 229 insertions(+), 4 deletions(-) diff --git a/.github/workflows/nightly-gnu.yml b/.github/workflows/nightly-gnu.yml index f9543b025..1f7c2d488 100644 --- a/.github/workflows/nightly-gnu.yml +++ b/.github/workflows/nightly-gnu.yml @@ -55,3 +55,142 @@ jobs: - name: Build RustFS run: cargo build --release --locked --target x86_64-unknown-linux-gnu -p rustfs --bins + + # Live-Vault lane for the rustfs-kms suite (rustfs/backlog#1774). + # + # RUSTFS_KMS_VAULT_TOKEN is the single switch that adds the Vault KV2 and + # Vault Transit backends to every for_each_backend spec in + # crates/kms/tests/behavior_*.rs (see crates/kms/AGENTS.md). rotate and + # versioning are advertised only by the Vault backends, so without this lane + # no CI run ever asserts the working half of behavior_rotation.rs — a + # rotation that silently dropped historical key versions would stay green. + # The same lane runs the dev-Vault #[ignore] tests and the two self-hosting + # live scripts (AppRole login, three-node Raft leader failover). + # + # GitHub-hosted ubuntu-latest, deliberately not the self-hosted sm-standard + # fleet: the HA failover script needs a working Docker daemon, and the + # self-hosted fleet is heterogeneous — a docker-dependent workflow has been + # burned by it before (see the banner in e2e-s3tests.yml, rustfs/backlog#1149). + kms-vault-lane: + name: KMS live Vault lane + runs-on: ubuntu-latest + timeout-minutes: 90 + env: + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" + # Root token of the ephemeral loopback dev server. Not a secret: the + # server lives only for this job, listens on 127.0.0.1, and holds only + # keys the tests create. The literal value matters — the dev-Vault + # #[ignore] fixtures in crates/kms/src/backends/vault.rs hardcode it. + VAULT_LANE_TOKEN: dev-only-token + VAULT_LANE_ADDR: http://127.0.0.1:8200 + # Keeps a runner-level proxy from swallowing the loopback dev-server + # traffic (see crates/kms/AGENTS.md). Actions env keys are + # case-insensitive, so only the uppercase form is set; reqwest reads + # either casing. + NO_PROXY: 127.0.0.1,localhost + steps: + - name: Checkout main branch + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + ref: main + + - name: Setup Rust environment + uses: ./.github/actions/setup + with: + # Dedicated key: rust-cache cannot tell runner images apart, so + # sharing a key with an sm-standard lane would let two different + # system images overwrite each other's artifacts (same reasoning as + # ci.yml's ci-uring lane). Saved from this nightly job itself so the + # next night starts warm. + cache-shared-key: kms-vault-lane + cache-save-if: 'true' + install-build-packaging-tools: 'false' + install-test-tools: 'false' + + - name: Install Vault CLI + run: | + set -euo pipefail + wget -qO- https://apt.releases.hashicorp.com/gpg | sudo gpg --dearmor -o /usr/share/keyrings/hashicorp-archive-keyring.gpg + echo "deb [signed-by=/usr/share/keyrings/hashicorp-archive-keyring.gpg] https://apt.releases.hashicorp.com $(lsb_release -cs) main" | sudo tee /etc/apt/sources.list.d/hashicorp.list >/dev/null + sudo apt-get update -qq + sudo apt-get install -y -qq vault + vault version + + - name: Start Vault dev server with KV2 and Transit engines + run: | + set -euo pipefail + nohup vault server -dev \ + -dev-root-token-id="${VAULT_LANE_TOKEN}" \ + -dev-listen-address=127.0.0.1:8200 >/tmp/vault-dev.log 2>&1 & + for _ in $(seq 1 60); do + if curl -fsS "${VAULT_LANE_ADDR}/v1/sys/health" >/dev/null 2>&1; then + break + fi + sleep 1 + done + curl -fsS "${VAULT_LANE_ADDR}/v1/sys/health" + export VAULT_ADDR="${VAULT_LANE_ADDR}" VAULT_TOKEN="${VAULT_LANE_TOKEN}" + # Dev mode mounts KV v2 at secret/ by default; Transit is explicit. + # Prove both engines actually work rather than assuming the defaults. + vault secrets enable transit + vault kv put secret/rustfs-ci-lane-probe value=ok >/dev/null + vault kv get secret/rustfs-ci-lane-probe >/dev/null + vault write -f transit/keys/rustfs-ci-lane-probe >/dev/null + + - name: Run rustfs-kms suite with the Vault lane on + env: + RUSTFS_KMS_VAULT_TOKEN: ${{ env.VAULT_LANE_TOKEN }} + RUSTFS_KMS_VAULT_ADDR: ${{ env.VAULT_LANE_ADDR }} + run: cargo test -p rustfs-kms --locked + + - name: Run dev-Vault ignored tests + env: + RUSTFS_KMS_VAULT_TOKEN: ${{ env.VAULT_LANE_TOKEN }} + RUSTFS_KMS_VAULT_ADDR: ${{ env.VAULT_LANE_ADDR }} + # Filters select the dev-Vault-only #[ignore] tests. The AWS #[ignore] + # tests (backends::aws, service_manager) stay excluded — they need real + # AWS credentials and create billable keys. The AppRole and HA #[ignore] + # tests are excluded here because their own scripts below provision the + # Vault topology they need. + run: | + set -euo pipefail + cargo test -p rustfs-kms --locked --lib backends::contract_tests -- --ignored + cargo test -p rustfs-kms --locked --lib backends::vault -- --ignored + cargo test -p rustfs-kms --locked --test vault_fault_injection -- --ignored + + - name: Run AppRole live checks (self-hosting ephemeral Vault) + run: bash scripts/test/vault_approle_kms_live.sh + + - name: Show Vault dev server log on failure + if: failure() + run: tail -n 200 /tmp/vault-dev.log || true + + # Three-node Raft leader failover (crates/kms/tests/vault_ha_failover_live.rs, + # first validated by rustfs/rustfs#5653). Its own job so an election-timing + # flake cannot mask the main lane's verdict, and vice versa. The script + # provisions and tears down its own Docker cluster. + kms-vault-ha-failover: + name: KMS Vault HA failover lane + runs-on: ubuntu-latest + timeout-minutes: 60 + env: + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" + NO_PROXY: 127.0.0.1,localhost + steps: + - name: Checkout main branch + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + with: + persist-credentials: false + ref: main + + - name: Setup Rust environment + uses: ./.github/actions/setup + with: + cache-shared-key: kms-vault-lane + cache-save-if: 'false' + install-build-packaging-tools: 'false' + install-test-tools: 'false' + + - name: Run HA leader failover live checks (three-node Raft cluster in Docker) + run: bash scripts/test/vault_ha_kms_live.sh diff --git a/crates/kms/src/backends/vault.rs b/crates/kms/src/backends/vault.rs index 188f2fac8..b42ab67ab 100644 --- a/crates/kms/src/backends/vault.rs +++ b/crates/kms/src/backends/vault.rs @@ -2758,10 +2758,14 @@ mod tests { use crate::config::{BackendConfig, KmsConfig}; use crate::types::{CancelKeyDeletionRequest, CreateKeyRequest, DeleteKeyRequest, KeyStatus, KeyUsage}; + // A dev Vault speaks plain HTTP, which validate() refuses unless + // development mode is declared on the config itself — the env override + // is applied by the config loaders, not by Default::default(). let kms_config = KmsConfig { backend_config: BackendConfig::VaultKv2(Box::new(integration_vault_config())), ..Default::default() - }; + } + .with_insecure_development_defaults(); let backend = VaultKmsBackend::new(kms_config).await.expect("backend"); let key_id = format!("cancel-persist-{}", uuid::Uuid::new_v4()); diff --git a/crates/kms/tests/common/mod.rs b/crates/kms/tests/common/mod.rs index 9d2b8d7ec..e795e5f28 100644 --- a/crates/kms/tests/common/mod.rs +++ b/crates/kms/tests/common/mod.rs @@ -32,14 +32,14 @@ use std::collections::HashMap; use std::fmt::Debug; use std::future::Future; use std::path::PathBuf; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use base64::Engine as _; use base64::engine::general_purpose::STANDARD as BASE64; use rustfs_kms::backends::BackendCapabilities; use rustfs_kms::{ - CreateKeyRequest, KeyUsage, KmsConfig, KmsError, KmsManager, KmsServiceManager, KmsServiceStatus, ObjectEncryptionService, - Result, + CreateKeyRequest, DeleteKeyRequest, KeyUsage, KmsConfig, KmsError, KmsManager, KmsServiceManager, KmsServiceStatus, + ObjectEncryptionService, Result, }; use tempfile::TempDir; @@ -126,6 +126,12 @@ pub struct TestKms { manager: Arc, kind: BackendKind, config: KmsConfig, + /// Ids of the keys [`TestKms::create_key`] created, so a run against a + /// persistent Vault can remove them afterwards instead of accumulating + /// `behavior-*` keys forever (rustfs/backlog#1774). Shared through an Arc + /// because the harness instance is consumed by the spec while the cleanup + /// runs after it. + created_keys: Arc>>, /// Held for the harness lifetime so the local key directory outlives a /// simulated process restart. _dir: Option, @@ -147,6 +153,7 @@ impl TestKms { manager, kind: BackendKind::Local, config, + created_keys: Arc::new(Mutex::new(Vec::new())), _dir: Some(dir), } } @@ -166,6 +173,7 @@ impl TestKms { manager, kind: BackendKind::VaultKv2, config, + created_keys: Arc::new(Mutex::new(Vec::new())), _dir: None, } } @@ -180,6 +188,7 @@ impl TestKms { manager, kind: BackendKind::VaultTransit, config, + created_keys: Arc::new(Mutex::new(Vec::new())), _dir: None, } } @@ -192,6 +201,7 @@ impl TestKms { manager, kind: BackendKind::Static, config, + created_keys: Arc::new(Mutex::new(Vec::new())), _dir: None, } } @@ -253,8 +263,75 @@ impl TestKms { .await .unwrap_or_else(|error| panic!("create_key({name}) should succeed on {}: {error:?}", self.kind.name())); assert_eq!(response.key_id, name, "created key id must be the requested name"); + self.created_keys + .lock() + .expect("created-keys lock") + .push(response.key_id.clone()); response.key_id } + + /// Handle to the ids [`Self::create_key`] recorded, for cleanup that runs + /// after a spec consumed the harness instance. + pub fn created_keys_handle(&self) -> Arc>> { + Arc::clone(&self.created_keys) + } + + /// Remove this instance's recorded Vault keys; see [`cleanup_vault_keys`]. + pub async fn cleanup(&self) { + cleanup_vault_keys(self.kind, &self.config, self.created_keys_handle()).await; + } +} + +/// Best-effort removal of the Vault keys a harness instance created, so a +/// persistent dev Vault does not accumulate `behavior-*` keys across runs +/// (rustfs/backlog#1774). A no-op for the Local and Static backends, whose +/// state dies with the per-test temp directory. +/// +/// The deletion runs on a fresh manager over the same configuration — the +/// case's own manager is consumed by the spec and may have been stopped by a +/// restart scenario — with the immediate-deletion gate enabled on the cleanup +/// configuration only, so the configuration under test keeps the gate at its +/// production default and specs asserting the gate's refusal stay honest. +/// +/// Failures are reported but never panic: cleanup runs after the spec's own +/// assertions, and a Vault hiccup here must not turn a green behavior run red. +pub async fn cleanup_vault_keys(kind: BackendKind, config: &KmsConfig, created_keys: Arc>>) { + if !kind.is_vault() { + return; + } + let key_ids: Vec = created_keys.lock().expect("created-keys lock").drain(..).collect(); + if key_ids.is_empty() { + return; + } + let config = config.clone().with_immediate_deletion_allowed(); + let manager = start_manager(&config).await; + let kms = manager.get_manager().await.expect("KMS manager should be running"); + for key_id in key_ids { + // The Transit backend deletes in two steps (first call parks the key in + // PendingDeletion, the next call destroys it); KV2 destroys on the + // first call and reports KeyNotFound on the second. + for _ in 0..2 { + match kms + .delete_key(DeleteKeyRequest { + key_id: key_id.clone(), + pending_window_in_days: None, + force_immediate: Some(true), + confirm_key_id: Some(key_id.clone()), + }) + .await + { + Ok(_) => continue, + Err(KmsError::KeyNotFound { .. }) => break, + Err(error) => { + eprintln!("vault key cleanup: could not delete {key_id}: {error:?}"); + break; + } + } + } + } + if let Err(error) = manager.stop().await { + eprintln!("vault key cleanup: could not stop the cleanup manager: {error:?}"); + } } async fn start_manager(config: &KmsConfig) -> Arc { @@ -332,7 +409,12 @@ where .chain(live_vault_backends()); for kind in kinds { let case = BackendCase::new(kind).await; + // Captured before the spec consumes the case; keys the spec creates + // through the harness afterwards still land in the shared list. + let config = case.kms.config().clone(); + let created_keys = case.kms.created_keys_handle(); spec(case).await; + cleanup_vault_keys(kind, &config, created_keys).await; } }