fix(cluster): /cluster/status reported a total partition on a healthy fleet
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
The status aggregator probed every peer's /cluster/status/local with NO credential. That route is token-gated, so on any cluster with TIDAL_API_KEY set each peer answered 401, and every peer row collapsed to the honest-unknown placeholder: reachable false, partitioned true, applied_events null, lag_events null, version "". Only the OWN region survived, because it is served in-process with no HTTP hop. The result: the one surface an operator reads to clear the N/N+1 version skew before a rolling upgrade - and the deploy runbook's own step 4 - reported the whole cluster partitioned while it was perfectly healthy, with every peer's version blank so the skew check was blind. Observed on the GKE cluster: all three pods answered /health 200 under leader tidaldb-1, replication applied, and a curl between the exact same pod FQDNs returned 200, while /cluster/status insisted both siblings were unreachable. security::bearer_from_env documents this precise trap - a node that "dials an authenticated peer with NO credential" - and count_alive_other_voters already attaches the bearer. This forwards the CALLER's Authorization header instead of reaching for creds.bearer(), matching the relayed-operator-hop convention that /cluster/promote already uses, so a weakly-authenticated caller cannot borrow the node's own credential to read peers it could not read directly. Why it escaped: every other multi-process test runs with no TIDAL_API_KEY, where a credential-less probe succeeds - including cluster_multiproc's all-reachable assertion. The new test carries the key. Verified differential: it fails on the reverted code with exactly the observed shape (us-east reachable, both peers null/false/empty) and passes with the fix. cluster_multiproc still 5/5.
This commit is contained in:
parent
e117333fec
commit
67a175e19a
@ -6050,6 +6050,7 @@ pub struct AggregatedRegionStatus {
|
|||||||
#[allow(clippy::significant_drop_tightening)]
|
#[allow(clippy::significant_drop_tightening)]
|
||||||
pub async fn cluster_status(
|
pub async fn cluster_status(
|
||||||
State(node): State<Arc<ClusterNode>>,
|
State(node): State<Arc<ClusterNode>>,
|
||||||
|
headers: HeaderMap,
|
||||||
) -> std::result::Result<Json<AggregatedStatusResponse>, ClusterAppError> {
|
) -> std::result::Result<Json<AggregatedStatusResponse>, ClusterAppError> {
|
||||||
let state = node.replica_for(None)?;
|
let state = node.replica_for(None)?;
|
||||||
let leader_name = state.leader_name();
|
let leader_name = state.leader_name();
|
||||||
@ -6059,9 +6060,24 @@ pub async fn cluster_status(
|
|||||||
// in-process (no HTTP hop); peers over the async client with the per-peer
|
// in-process (no HTTP hop); peers over the async client with the per-peer
|
||||||
// budget. Each entry: (RegionId, name, Option<local-status-json>).
|
// budget. Each entry: (RegionId, name, Option<local-status-json>).
|
||||||
let client = state.client.clone();
|
let client = state.client.clone();
|
||||||
|
// A relayed operator hop: pass the caller's credential straight through, the
|
||||||
|
// same way `/cluster/promote` does. Without it every peer answers 401 and
|
||||||
|
// this aggregate reported `reachable: false`, `partitioned: true`,
|
||||||
|
// `applied_events: null` and `version: ""` for EVERY peer on an
|
||||||
|
// authenticated cluster — a total-partition reading taken off a perfectly
|
||||||
|
// healthy fleet, from the one surface an operator reads to clear the N/N+1
|
||||||
|
// version skew before a rolling upgrade (and the runbook's own deploy
|
||||||
|
// step 4). `security::bearer_from_env` documents this exact trap: "node then
|
||||||
|
// dials an authenticated peer with NO credential".
|
||||||
|
//
|
||||||
|
// FORWARDING the caller's header, rather than reaching for
|
||||||
|
// `creds.bearer()`, keeps a weakly-authenticated caller from borrowing this
|
||||||
|
// node's own credential to read peers it could not read directly.
|
||||||
|
let auth = forwarded_auth(&headers);
|
||||||
let own_status = state.local_status().ok();
|
let own_status = state.local_status().ok();
|
||||||
let futures = regions.into_iter().map(|(rid, name, http)| {
|
let futures = regions.into_iter().map(|(rid, name, http)| {
|
||||||
let client = client.clone();
|
let client = client.clone();
|
||||||
|
let auth = auth.clone();
|
||||||
let own_status = if rid == state.region {
|
let own_status = if rid == state.region {
|
||||||
own_status.clone()
|
own_status.clone()
|
||||||
} else {
|
} else {
|
||||||
@ -6082,12 +6098,11 @@ pub async fn cluster_status(
|
|||||||
return (rid, name, None); // unreachable: no declared addr
|
return (rid, name, None); // unreachable: no declared addr
|
||||||
};
|
};
|
||||||
let url = peer_url(&http_addr, "/cluster/status/local");
|
let url = peer_url(&http_addr, "/cluster/status/local");
|
||||||
let body = client
|
let mut req = client.get(&url).timeout(forward::STATUS_PEER_TIMEOUT);
|
||||||
.get(&url)
|
if let Some(auth) = auth {
|
||||||
.timeout(forward::STATUS_PEER_TIMEOUT)
|
req = req.header(axum::http::header::AUTHORIZATION, auth);
|
||||||
.send()
|
}
|
||||||
.await
|
let body = req.send().await.ok();
|
||||||
.ok();
|
|
||||||
let json = match body {
|
let json = match body {
|
||||||
Some(resp) if resp.status().is_success() => {
|
Some(resp) if resp.status().is_success() => {
|
||||||
resp.json::<serde_json::Value>().await.ok()
|
resp.json::<serde_json::Value>().await.ok()
|
||||||
|
|||||||
@ -1145,6 +1145,89 @@ fn runbook_auth_protected_routes_401_probes_open() {
|
|||||||
println!("[auth] protected routes 401 bare, probes + /openapi.json open, valid bearer 204");
|
println!("[auth] protected routes 401 bare, probes + /openapi.json open, valid bearer 204");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The runbook's deploy step 4 — `GET /cluster/status | jq '.regions[] | {name,
|
||||||
|
/// lag_events, reachable}'` — must tell the TRUTH on an authenticated cluster.
|
||||||
|
///
|
||||||
|
/// The aggregate probes every peer's `/cluster/status/local` over HTTP, and that
|
||||||
|
/// route is token-gated (asserted by
|
||||||
|
/// `runbook_auth_protected_routes_401_probes_open` above). A probe carrying no
|
||||||
|
/// credential therefore reads 401 from every peer, and each peer row degrades to
|
||||||
|
/// the "honest unknown": `reachable: false`, `partitioned: true`,
|
||||||
|
/// `applied_events: null`, `version: ""`. The result is a TOTAL-PARTITION
|
||||||
|
/// reading taken off a perfectly healthy fleet, on the one surface an operator
|
||||||
|
/// reads to clear the N/N+1 version skew before a rolling upgrade.
|
||||||
|
/// `security::bearer_from_env` documents this exact trap: a node that "dials an
|
||||||
|
/// authenticated peer with NO credential".
|
||||||
|
///
|
||||||
|
/// Every OTHER multi-process test — including `cluster_multiproc`'s
|
||||||
|
/// all-reachable assertion — runs with no `TIDAL_API_KEY`, where a
|
||||||
|
/// credential-less probe succeeds. That is precisely why this was invisible for
|
||||||
|
/// so long, so this test carries the key.
|
||||||
|
#[test]
|
||||||
|
fn runbook_cluster_status_aggregate_reaches_peers_under_auth() {
|
||||||
|
const KEY: &str = "runbook-status-aggregate-key";
|
||||||
|
let opts = (0..3).fold(ClusterOptions::new(3), |opts, i| {
|
||||||
|
opts.with_env(i, "TIDAL_API_KEY", KEY)
|
||||||
|
});
|
||||||
|
let cluster = MultiProcCluster::start_with(opts);
|
||||||
|
|
||||||
|
let status_url = format!("{}/cluster/status", cluster.node(LEADER));
|
||||||
|
let fetch = || -> serde_json::Value {
|
||||||
|
let resp = cluster
|
||||||
|
.client()
|
||||||
|
.get(&status_url)
|
||||||
|
.header("Authorization", format!("Bearer {KEY}"))
|
||||||
|
.send()
|
||||||
|
.expect("GET /cluster/status");
|
||||||
|
assert_eq!(
|
||||||
|
resp.status().as_u16(),
|
||||||
|
200,
|
||||||
|
"an operator read of /cluster/status must still serve with the bearer"
|
||||||
|
);
|
||||||
|
resp.json().expect("/cluster/status body is JSON")
|
||||||
|
};
|
||||||
|
|
||||||
|
// The fan-out gets the convergence budget to report every peer.
|
||||||
|
let deadline = Instant::now() + convergence_budget();
|
||||||
|
let settled = loop {
|
||||||
|
let snap = fetch();
|
||||||
|
let all_reachable = snap["regions"].as_array().is_some_and(|rows| {
|
||||||
|
rows.len() == 3 && rows.iter().all(|r| r["reachable"].as_bool() == Some(true))
|
||||||
|
});
|
||||||
|
if all_reachable {
|
||||||
|
break snap;
|
||||||
|
}
|
||||||
|
assert!(
|
||||||
|
Instant::now() < deadline,
|
||||||
|
"/cluster/status never reported all 3 regions reachable on an \
|
||||||
|
AUTHENTICATED cluster — a peer probe that carries no credential \
|
||||||
|
reads its 401 as `unreachable`. Last snapshot: {snap}"
|
||||||
|
);
|
||||||
|
std::thread::sleep(Duration::from_millis(250));
|
||||||
|
};
|
||||||
|
|
||||||
|
// Each peer row must carry REAL values, never the unreachable placeholder.
|
||||||
|
for row in settled["regions"].as_array().expect("regions array") {
|
||||||
|
let name = row["name"].as_str().unwrap_or("<unnamed>");
|
||||||
|
assert_eq!(
|
||||||
|
row["partitioned"].as_bool(),
|
||||||
|
Some(false),
|
||||||
|
"{name}: a reachable region is not partitioned"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!row["applied_events"].is_null(),
|
||||||
|
"{name}: applied_events must be a known number, not the \
|
||||||
|
unreachable-peer null"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
!row["version"].as_str().unwrap_or_default().is_empty(),
|
||||||
|
"{name}: must report its build version — the pre-upgrade N/N+1 skew \
|
||||||
|
check reads this field, and an unreachable peer reports it empty"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
println!("[auth] /cluster/status aggregated all 3 regions through the forwarded bearer");
|
||||||
|
}
|
||||||
|
|
||||||
// ── Auth: the admin key separates operator authority from data-plane access ────
|
// ── Auth: the admin key separates operator authority from data-plane access ────
|
||||||
|
|
||||||
/// With BOTH `TIDAL_API_KEY` and `TIDAL_ADMIN_KEY` set, a data-plane bearer is
|
/// With BOTH `TIDAL_API_KEY` and `TIDAL_ADMIN_KEY` set, a data-plane bearer is
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user