From 73df8b79e16b66087ed3e94a1962e6bc457acb67 Mon Sep 17 00:00:00 2001 From: Jordi Gil Date: Thu, 13 Aug 2026 21:08:00 -0400 Subject: [PATCH 1/5] fix(operator): distinguish SecretMissing from KeyMissing in TLS Secret resolution `secret::read_secret_bytes` collapsed two distinct cases into `Ok(None)`: the referenced Secret not existing at all, and the Secret existing but lacking the requested key. `endpoint_tls::read_secret_bytes_for_tls` (shared by InferenceProvider's metrics/health-check TLS resolution and the reconcile-time verify_tls_accessible check) could therefore only ever report KeyMissing when a key was present with an empty value, never when it was absent entirely -- contradicting its own documented behavior and misleading operators diagnosing a live misconfiguration ("Secret doesn't exist" vs. "Secret exists, wrong/missing key name"). Introduce SecretKeyLookup, a three-way Found/SecretMissing/KeyMissing result, and have read_secret_bytes return it directly instead of collapsing to Option>. This also removes a near-duplicate of the same distinction that verify_tls_accessible's private TlsSecretCheck/read_tls_secret_for_verify had already implemented independently against its own kube::Api call -- it now delegates to the same fixed read_secret_bytes. grid_site.rs's gateway-probe path doesn't need the distinction, so it uses SecretKeyLookup::into_bytes() to keep its existing Option>-based control flow unchanged. Adds direct unit tests for read_secret_bytes (secret.rs) and for resolve_tls_config/verify_tls_accessible (endpoint_tls.rs) against a mocked kube::Client, covering: key found, secret absent, secret with no data section, key absent from an existing secret's data (the bug), and key present but empty. Fixes #58 Signed-off-by: Jordi Gil --- Cargo.lock | 1 + operator/Cargo.toml | 3 + operator/src/controller/grid_site.rs | 3 + operator/src/resources/endpoint_tls.rs | 202 ++++++++++++++++++----- operator/src/resources/secret.rs | 212 ++++++++++++++++++++++++- 5 files changed, 374 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 755f678..806bc3f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1518,6 +1518,7 @@ dependencies = [ "time", "tokio", "tokio-rustls", + "tower", "tracing", "tracing-subscriber", "uuid", diff --git a/operator/Cargo.toml b/operator/Cargo.toml index 824d650..ed8a75a 100644 --- a/operator/Cargo.toml +++ b/operator/Cargo.toml @@ -37,5 +37,8 @@ tracing-subscriber = { workspace = true } uuid = { workspace = true } zeroize = { workspace = true } +[dev-dependencies] +tower = { workspace = true } + [lints] workspace = true diff --git a/operator/src/controller/grid_site.rs b/operator/src/controller/grid_site.rs index 5e1a7c7..e09d41f 100644 --- a/operator/src/controller/grid_site.rs +++ b/operator/src/controller/grid_site.rs @@ -263,6 +263,7 @@ async fn build_probe_config_from_secrets( let ca_bytes = read_secret_bytes(client, ca_ref, "ca.crt") .await .map_err(|_err| O::TrustMaterialMissing)? + .into_bytes() .ok_or(O::TrustMaterialMissing)?; let roots = parse_ca_roots(&ca_bytes).map_err(|_err| O::TrustMaterialInvalid)?; @@ -275,11 +276,13 @@ async fn build_probe_config_from_secrets( let cert_bytes = read_secret_bytes(client, secret_ref, "tls.crt") .await .map_err(|_err| O::TrustMaterialMissing)? + .into_bytes() .ok_or(O::TrustMaterialMissing)?; let key_bytes = Zeroizing::new( read_secret_bytes(client, secret_ref, "tls.key") .await .map_err(|_err| O::TrustMaterialMissing)? + .into_bytes() .ok_or(O::TrustMaterialMissing)?, ); let client_certs = parse_client_certs(&cert_bytes).map_err(|_err| O::TrustMaterialInvalid)?; diff --git a/operator/src/resources/endpoint_tls.rs b/operator/src/resources/endpoint_tls.rs index 5520f03..e5849cb 100644 --- a/operator/src/resources/endpoint_tls.rs +++ b/operator/src/resources/endpoint_tls.rs @@ -107,21 +107,21 @@ pub(crate) async fn read_secret_bytes_for_tls( provider_identity: &str, material_desc: &str, ) -> Result, (TlsFailureReason, String)> { - use crate::resources::secret::read_secret_bytes; + use crate::resources::secret::{SecretKeyLookup, read_secret_bytes}; match read_secret_bytes(client, secret_ref, key_name).await { - Ok(Some(bytes)) if !bytes.is_empty() => Ok(bytes), - Ok(Some(_)) => Err(( + Ok(SecretKeyLookup::Found(bytes)) => Ok(bytes), + Ok(SecretKeyLookup::KeyMissing) => Err(( TlsFailureReason::KeyMissing, format!( - "{material_desc} key {key_name:?} in Secret {}/{} is empty for provider {provider_identity}", + "{material_desc} key {key_name:?} in Secret {}/{} is absent or empty for provider {provider_identity}", secret_ref.namespace, secret_ref.name ), )), - Ok(None) => Err(( + Ok(SecretKeyLookup::SecretMissing) => Err(( TlsFailureReason::SecretMissing, format!( - "{material_desc} Secret {}/{} or key {key_name:?} not found for provider {provider_identity}", + "{material_desc} Secret {}/{} not found for provider {provider_identity}", secret_ref.namespace, secret_ref.name ), )), @@ -214,16 +214,6 @@ pub(crate) async fn resolve_tls_config( // TLS validation // --------------------------------------------------------------------------- -/// Result of reading a TLS Secret key for validation. -enum TlsSecretCheck { - /// The key was found and contains non-empty bytes. - Ok(Vec), - /// The Secret does not exist or has no `data` section. - SecretMissing, - /// The expected key is absent or its value is empty. - KeyMissing, -} - /// Verify that TLS Secrets exist, contain the expected keys, and the PEM /// material can be assembled into a valid [`rustls::ClientConfig`]. /// @@ -246,7 +236,6 @@ enum TlsSecretCheck { /// /// [`OperatorError`]: crate::error::OperatorError #[expect( - clippy::too_many_lines, clippy::large_stack_frames, reason = "sequential Secret reads for CA, client cert, and client key with match arms" )] @@ -260,22 +249,19 @@ pub(crate) async fn verify_tls_accessible( let ca_key = tls.ca_secret_ref.key.as_deref().unwrap_or("ca.crt"); let ca_pem = match read_tls_secret_for_verify(client, &tls.ca_secret_ref, ca_key).await? { - TlsSecretCheck::Ok(bytes) => bytes, - TlsSecretCheck::SecretMissing => return Ok(Some(TlsFailureReason::SecretMissing)), - TlsSecretCheck::KeyMissing => return Ok(Some(TlsFailureReason::KeyMissing)), + Ok(bytes) => bytes, + Err(reason) => return Ok(Some(reason)), }; let (client_cert_pem, client_key_pem) = if let Some(client_ref) = &tls.client_certificate_secret_ref { let sref = secret_ref_from_client_cert(client_ref); let cert = match read_tls_secret_for_verify(client, &sref, &client_ref.certificate_key).await? { - TlsSecretCheck::Ok(bytes) => bytes, - TlsSecretCheck::SecretMissing => return Ok(Some(TlsFailureReason::SecretMissing)), - TlsSecretCheck::KeyMissing => return Ok(Some(TlsFailureReason::KeyMissing)), + Ok(bytes) => bytes, + Err(reason) => return Ok(Some(reason)), }; let key = match read_tls_secret_for_verify(client, &sref, &client_ref.private_key_key).await? { - TlsSecretCheck::Ok(bytes) => bytes, - TlsSecretCheck::SecretMissing => return Ok(Some(TlsFailureReason::SecretMissing)), - TlsSecretCheck::KeyMissing => return Ok(Some(TlsFailureReason::KeyMissing)), + Ok(bytes) => bytes, + Err(reason) => return Ok(Some(reason)), }; (Some(cert), Some(key)) } else { @@ -294,8 +280,10 @@ pub(crate) async fn verify_tls_accessible( /// Read raw bytes from a Kubernetes Secret for TLS validation. /// -/// Distinguishes between "Secret not found" and "key not found" to map to -/// the correct [`TlsFailureReason`] variant. +/// Thin wrapper over [`secret::read_secret_bytes`] that maps its +/// [`SecretKeyLookup`](crate::resources::secret::SecretKeyLookup) result +/// onto the [`TlsFailureReason`] this module's callers expect, so "Secret +/// not found" and "key not found" map to the correct variant. /// /// # Errors /// @@ -306,21 +294,14 @@ async fn read_tls_secret_for_verify( client: &kube::Client, secret_ref: &crate::crd::grid_network::SecretRef, key_name: &str, -) -> Result { - let api: kube::Api = - kube::Api::namespaced(client.clone(), &secret_ref.namespace); - let Some(secret) = api.get_opt(&secret_ref.name).await? else { - return Ok(TlsSecretCheck::SecretMissing); - }; - let Some(data) = &secret.data else { - // The Secret exists but has no `data` section — treat it the same - // as a missing Secret since there is nothing to read. - return Ok(TlsSecretCheck::SecretMissing); - }; - match data.get(key_name) { - Some(bytes) if !bytes.0.is_empty() => Ok(TlsSecretCheck::Ok(bytes.0.clone())), - _ => Ok(TlsSecretCheck::KeyMissing), - } +) -> Result, TlsFailureReason>, crate::error::OperatorError> { + use crate::resources::secret::{SecretKeyLookup, read_secret_bytes}; + + Ok(match read_secret_bytes(client, secret_ref, key_name).await? { + SecretKeyLookup::Found(bytes) => Ok(bytes), + SecretKeyLookup::SecretMissing => Err(TlsFailureReason::SecretMissing), + SecretKeyLookup::KeyMissing => Err(TlsFailureReason::KeyMissing), + }) } // --------------------------------------------------------------------------- @@ -331,8 +312,81 @@ async fn read_tls_secret_for_verify( #[expect(clippy::allow_attributes, reason = "blanket test suppressions")] #[allow(clippy::unwrap_used, clippy::expect_used, reason = "tests")] mod tests { + use std::collections::HashMap; + + use k8s_openapi::{ByteString, api::core::v1::Secret}; + use super::*; + // ----------------------------------------------------------------------- + // Test doubles + // ----------------------------------------------------------------------- + + /// Build a `kube::Client` backed by an in-memory map of Secret name to + /// `Secret`, so `resolve_tls_config`/`verify_tls_accessible` can be + /// exercised without a real cluster. Any name not present in the map + /// returns HTTP 404. + #[expect( + clippy::too_many_lines, + reason = "test mock builder: 404-vs-200 branches are the whole point" + )] + fn mock_kube_client_with_secrets(secrets: HashMap<&'static str, Secret>) -> kube::Client { + let service = tower::service_fn(move |req: http::Request| { + let secrets = secrets.clone(); + async move { + let name = req.uri().path().rsplit('/').next().unwrap_or_default().to_owned(); + let response = secrets.get(name.as_str()).map_or_else( + || { + let not_found = serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": format!("secrets \"{name}\" not found"), + "reason": "NotFound", + "code": 404, + }); + http::Response::builder() + .status(404) + .body(kube::client::Body::from( + serde_json::to_vec(¬_found).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + }, + |secret| { + http::Response::builder() + .status(200) + .body(kube::client::Body::from( + serde_json::to_vec(secret).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + }, + ); + Ok::<_, std::convert::Infallible>(response) + } + }); + kube::Client::new(service, "default") + } + + fn secret_with_key(key: &str, value: &[u8]) -> Secret { + let mut data = std::collections::BTreeMap::new(); + data.insert(key.to_owned(), ByteString(value.to_vec())); + Secret { + data: Some(data), + ..Default::default() + } + } + + fn test_tls_config(ca_secret_name: &str) -> EndpointTlsConfig { + EndpointTlsConfig { + ca_secret_ref: crate::crd::grid_network::SecretRef { + name: ca_secret_name.to_owned(), + namespace: "default".to_owned(), + key: None, + }, + client_certificate_secret_ref: None, + } + } + // ----------------------------------------------------------------------- // secret_ref_from_client_cert — field mapping // ----------------------------------------------------------------------- @@ -462,4 +516,66 @@ mod tests { "TLS configured without a kube client must return MaterialInvalid" ); } + + // ----------------------------------------------------------------------- + // resolve_tls_config / verify_tls_accessible — SecretMissing vs + // KeyMissing (grid#58), against a mocked Kubernetes API + // ----------------------------------------------------------------------- + + #[tokio::test] + async fn resolve_tls_config_ca_secret_absent_yields_secret_missing() { + let client = mock_kube_client_with_secrets(HashMap::new()); + let tls = test_tls_config("absent"); + let (reason, _msg) = resolve_tls_config(Some(&tls), Some(&client), "test-provider") + .await + .unwrap_err(); + assert_eq!(reason, TlsFailureReason::SecretMissing); + } + + /// Regression test for grid#58 at the `InferenceProvider` + /// metrics/health-check TLS resolution path (the pipeline the bug was + /// originally reported against): a key absent from an existing CA + /// Secret must surface as `KeyMissing`, not `SecretMissing`. + #[tokio::test] + async fn resolve_tls_config_ca_key_absent_from_existing_secret_yields_key_missing() { + let client = + mock_kube_client_with_secrets(HashMap::from([("ca-secret", secret_with_key("wrong-key", b"bytes"))])); + let tls = test_tls_config("ca-secret"); + let (reason, _msg) = resolve_tls_config(Some(&tls), Some(&client), "test-provider") + .await + .unwrap_err(); + assert_eq!( + reason, + TlsFailureReason::KeyMissing, + "grid#58: a key absent from an existing Secret's data must be KeyMissing, not SecretMissing" + ); + } + + #[tokio::test] + async fn verify_tls_accessible_ca_secret_absent_returns_secret_missing() { + let client = mock_kube_client_with_secrets(HashMap::new()); + let tls = test_tls_config("absent"); + let result = verify_tls_accessible(&client, Some(&tls)).await.expect("no API error"); + assert_eq!(result, Some(TlsFailureReason::SecretMissing)); + } + + #[tokio::test] + async fn verify_tls_accessible_ca_key_absent_from_existing_secret_returns_key_missing() { + let client = + mock_kube_client_with_secrets(HashMap::from([("ca-secret", secret_with_key("wrong-key", b"bytes"))])); + let tls = test_tls_config("ca-secret"); + let result = verify_tls_accessible(&client, Some(&tls)).await.expect("no API error"); + assert_eq!( + result, + Some(TlsFailureReason::KeyMissing), + "grid#58: a key absent from an existing Secret's data must be KeyMissing, not SecretMissing" + ); + } + + #[tokio::test] + async fn verify_tls_accessible_no_tls_config_returns_none() { + let client = mock_kube_client_with_secrets(HashMap::new()); + let result = verify_tls_accessible(&client, None).await.expect("no API error"); + assert!(result.is_none(), "no TLS configured must skip validation"); + } } diff --git a/operator/src/resources/secret.rs b/operator/src/resources/secret.rs index 3b730cc..7151a99 100644 --- a/operator/src/resources/secret.rs +++ b/operator/src/resources/secret.rs @@ -57,9 +57,38 @@ pub async fn read_site_cert_pem( Ok(public_cert_pem_from_secret(&secret)) } +/// Result of looking up a named key within a Kubernetes Secret's `data` map. +/// +/// Distinguishes "the Secret itself is absent" from "the Secret exists but +/// the key is absent (or empty)" so callers can surface an accurate +/// diagnostic instead of collapsing both into a single `None`. See +/// [grid#58](https://github.com/praxis-proxy/grid/issues/58). +#[derive(Debug, Eq, PartialEq)] +pub(crate) enum SecretKeyLookup { + /// The key was found and contains non-empty bytes. + Found(Vec), + /// The Secret does not exist, or exists but has no `data` section. + SecretMissing, + /// The Secret exists but the key is absent from `data`, or its value is + /// empty. + KeyMissing, +} + +impl SecretKeyLookup { + /// Collapse into the historical `Option>` shape for callers that + /// don't need to distinguish "Secret missing" from "key missing". + pub(crate) fn into_bytes(self) -> Option> { + match self { + Self::Found(bytes) => Some(bytes), + Self::SecretMissing | Self::KeyMissing => None, + } + } +} + /// Read raw bytes from a named key within a Kubernetes Secret. /// -/// Returns `Ok(None)` when the Secret or the key does not exist. +/// Returns [`SecretKeyLookup`] so callers can distinguish a missing Secret +/// from a Secret that exists but lacks (or has an empty) requested key. /// Never logs the byte content — callers handle private material. /// /// # Errors @@ -69,12 +98,18 @@ pub(crate) async fn read_secret_bytes( client: &kube::Client, secret_ref: &crate::crd::grid_network::SecretRef, key_name: &str, -) -> Result>, kube::Error> { +) -> Result { let api: kube::Api = kube::Api::namespaced(client.clone(), &secret_ref.namespace); let Some(secret) = api.get_opt(&secret_ref.name).await? else { - return Ok(None); + return Ok(SecretKeyLookup::SecretMissing); + }; + let Some(data) = &secret.data else { + return Ok(SecretKeyLookup::SecretMissing); }; - Ok(secret.data.as_ref().and_then(|d| d.get(key_name)).map(|b| b.0.clone())) + Ok(match data.get(key_name) { + Some(bytes) if !bytes.0.is_empty() => SecretKeyLookup::Found(bytes.0.clone()), + Some(_) | None => SecretKeyLookup::KeyMissing, + }) } /// Extract public certificate PEM from `secret.data["tls.crt"]`. @@ -166,9 +201,178 @@ pub fn site_cert_secret_data(site: &certs::SiteCertOutput) -> BTreeMap) -> kube::Client { + let service = tower::service_fn(move |req: http::Request| { + let secrets = secrets.clone(); + async move { + let name = req.uri().path().rsplit('/').next().unwrap_or_default().to_owned(); + let response = secrets.get(name.as_str()).map_or_else( + || { + let not_found = serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": format!("secrets \"{name}\" not found"), + "reason": "NotFound", + "code": 404, + }); + http::Response::builder() + .status(404) + .body(kube::client::Body::from( + serde_json::to_vec(¬_found).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + }, + |secret| { + http::Response::builder() + .status(200) + .body(kube::client::Body::from( + serde_json::to_vec(secret).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + }, + ); + Ok::<_, std::convert::Infallible>(response) + } + }); + kube::Client::new(service, "default") + } + + fn secret_ref(name: &str) -> crate::crd::grid_network::SecretRef { + crate::crd::grid_network::SecretRef { + name: name.to_owned(), + namespace: "default".to_owned(), + key: None, + } + } + + // ----------------------------------------------------------------------- + // read_secret_bytes — SecretMissing vs KeyMissing (grid#58) + // ----------------------------------------------------------------------- + + #[tokio::test] + async fn read_secret_bytes_found_returns_bytes() { + let mut data = BTreeMap::new(); + data.insert("ca.crt".to_owned(), ByteString(b"ca-bytes".to_vec())); + let client = mock_kube_client_with_secrets(HashMap::from([( + "ca-secret", + Secret { + data: Some(data), + ..Default::default() + }, + )])); + let result = read_secret_bytes(&client, &secret_ref("ca-secret"), "ca.crt") + .await + .expect("mock API call must not fail"); + assert_eq!(result, SecretKeyLookup::Found(b"ca-bytes".to_vec())); + } + + #[tokio::test] + async fn read_secret_bytes_secret_absent_returns_secret_missing() { + let client = mock_kube_client_with_secrets(HashMap::new()); + let result = read_secret_bytes(&client, &secret_ref("absent"), "ca.crt") + .await + .expect("mock API call must not fail"); + assert_eq!(result, SecretKeyLookup::SecretMissing); + } + + #[tokio::test] + async fn read_secret_bytes_secret_with_no_data_section_returns_secret_missing() { + let client = mock_kube_client_with_secrets(HashMap::from([( + "empty-secret", + Secret { + data: None, + ..Default::default() + }, + )])); + let result = read_secret_bytes(&client, &secret_ref("empty-secret"), "ca.crt") + .await + .expect("mock API call must not fail"); + assert_eq!( + result, + SecretKeyLookup::SecretMissing, + "a Secret with no data section at all has nothing to read; treated as missing" + ); + } + + /// Regression test for grid#58: a key absent from an *existing* Secret's + /// `data` map must be reported as `KeyMissing`, not conflated with the + /// Secret itself being absent. + #[tokio::test] + async fn read_secret_bytes_key_absent_from_existing_secret_returns_key_missing() { + let mut data = BTreeMap::new(); + data.insert("wrong-key".to_owned(), ByteString(b"ca-bytes".to_vec())); + let client = mock_kube_client_with_secrets(HashMap::from([( + "ca-secret", + Secret { + data: Some(data), + ..Default::default() + }, + )])); + let result = read_secret_bytes(&client, &secret_ref("ca-secret"), "ca.crt") + .await + .expect("mock API call must not fail"); + assert_eq!( + result, + SecretKeyLookup::KeyMissing, + "grid#58: a key absent from an existing Secret's data must be KeyMissing, not SecretMissing" + ); + } + + #[tokio::test] + async fn read_secret_bytes_key_present_but_empty_returns_key_missing() { + let mut data = BTreeMap::new(); + data.insert("ca.crt".to_owned(), ByteString(Vec::new())); + let client = mock_kube_client_with_secrets(HashMap::from([( + "ca-secret", + Secret { + data: Some(data), + ..Default::default() + }, + )])); + let result = read_secret_bytes(&client, &secret_ref("ca-secret"), "ca.crt") + .await + .expect("mock API call must not fail"); + assert_eq!(result, SecretKeyLookup::KeyMissing); + } + + // ----------------------------------------------------------------------- + // SecretKeyLookup::into_bytes — pure collapse to Option> + // ----------------------------------------------------------------------- + + #[test] + fn into_bytes_found_yields_some() { + assert_eq!(SecretKeyLookup::Found(b"x".to_vec()).into_bytes(), Some(b"x".to_vec())); + } + + #[test] + fn into_bytes_secret_missing_yields_none() { + assert_eq!(SecretKeyLookup::SecretMissing.into_bytes(), None); + } + + #[test] + fn into_bytes_key_missing_yields_none() { + assert_eq!(SecretKeyLookup::KeyMissing.into_bytes(), None); + } + #[test] fn build_creates_secret_with_metadata() { let mut data = BTreeMap::new(); From 1b7d1f2393b846d66f555ac6467d8c935d1d5b62 Mon Sep 17 00:00:00 2001 From: Jordi Gil Date: Thu, 13 Aug 2026 21:42:09 -0400 Subject: [PATCH 2/5] test(operator): close pyramid gaps for grid#58 TLS SecretMissing/KeyMissing Adds the integration and E2E tiers that were missing for the SecretMissing-vs-KeyMissing status.reason distinction fixed in this PR: - Integration: resolve_phase_and_sites (the function reconcile() calls) is now exercised end-to-end against a mocked kube::Client, proving the distinction survives through the actual (phase, status.reason) pair written to the CR, not just the lower-level resolve_tls_config/ read_secret_bytes helpers. - E2E: a new InferenceProvider fixture with a CA Secret that exists but is missing the expected key runs against a live kind cluster and polls status.reason for "HealthCheckTlsKeyMissing" (verified locally: PASS). This gap pre-existed for all four TlsFailureReason variants, not just this fix; the E2E addition here only covers the specific regression this PR guards against, since it needs no live probe endpoint (TLS resolution fails before any health probe is attempted). Signed-off-by: Jordi Gil --- operator/src/controller/inference_provider.rs | 157 ++++++++++++++++++ xtask/src/env/mod.rs | 12 +- xtask/src/env/operator.rs | 126 ++++++++++++++ 3 files changed, 294 insertions(+), 1 deletion(-) diff --git a/operator/src/controller/inference_provider.rs b/operator/src/controller/inference_provider.rs index 4d0b0bf..dfd8947 100644 --- a/operator/src/controller/inference_provider.rs +++ b/operator/src/controller/inference_provider.rs @@ -783,6 +783,163 @@ mod tests { .unwrap_or_else(|_| std::process::abort()) } + // ----------------------------------------------------------------------- + // resolve_phase_and_sites — integration tier: reconcile-path TLS + // failure reasons through a mocked kube::Client (grid#58) + // + // The unit tier (endpoint_tls.rs, secret.rs) already exercises + // resolve_tls_config/read_secret_bytes directly. These tests instead + // drive the same scenario through resolve_phase_and_sites — the actual + // function reconcile() calls — proving the SecretMissing/KeyMissing + // distinction survives all the way to the (phase, status.reason) pair + // reconcile() writes to the CR, not just to an intermediate type. + // ----------------------------------------------------------------------- + + use std::collections::HashMap; + + use k8s_openapi::{ByteString, api::core::v1::Secret}; + + /// Build an HTTP 200 JSON response from any serializable value. + fn json_ok(body: &impl serde::Serialize) -> http::Response { + http::Response::builder() + .status(200) + .body(kube::client::Body::from( + serde_json::to_vec(body).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + } + + /// Build an HTTP 404 Kubernetes `Status` response for a named resource. + /// + /// Must actually set the 404 status (not reuse [`json_ok`]'s 200) — + /// `kube`'s client only maps a response onto `ApiError`/`get_opt: None` + /// when the HTTP status itself is 404; a 200 body shaped like a `Status` + /// object is instead treated as a malformed resource and surfaces as a + /// deserialization error. + fn json_not_found(resource_and_name: &str) -> http::Response { + http::Response::builder() + .status(404) + .body(kube::client::Body::from( + serde_json::to_vec(&serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": format!("{resource_and_name} not found"), + "reason": "NotFound", + "code": 404, + })) + .unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + } + + /// A `kube::Client` that serves just enough of the Kubernetes API surface + /// for `resolve_phase_and_sites` to reach its health-check TLS branch: + /// a `GridNetwork` matching `provider.spec.gridNetworkRef`, an empty + /// `GridSite` list, and the supplied CA Secrets. + fn mock_kube_client_for_health_tls( + grid_network_name: &'static str, + secrets: HashMap<&'static str, Secret>, + ) -> Client { + let service = tower::service_fn(move |req: http::Request| { + let secrets = secrets.clone(); + async move { + let path = req.uri().path().to_owned(); + let name = path.rsplit('/').next().unwrap_or_default().to_owned(); + let response = if path.contains("/secrets/") { + secrets + .get(name.as_str()) + .map_or_else(|| json_not_found(&format!("secrets {name:?}")), json_ok) + } else if path.ends_with("/gridsites") { + json_ok(&serde_json::json!({ + "apiVersion": "grid.praxis-proxy.io/v1alpha1", + "kind": "GridSiteList", + "items": [], + })) + } else if path.contains("/gridnetworks/") && name == grid_network_name { + json_ok(&serde_json::json!({ + "apiVersion": "grid.praxis-proxy.io/v1alpha1", + "kind": "GridNetwork", + "metadata": { "name": grid_network_name }, + "spec": {}, + })) + } else { + json_not_found(&format!("gridnetworks {name:?}")) + }; + Ok::<_, std::convert::Infallible>(response) + } + }); + Client::new(service, "default") + } + + fn secret_with_key(key: &str, value: &[u8]) -> Secret { + let mut data = std::collections::BTreeMap::new(); + data.insert(key.to_owned(), ByteString(value.to_vec())); + Secret { + data: Some(data), + ..Default::default() + } + } + + /// An otherwise-valid provider with `healthCheck.tls.caSecretRef` pointing + /// at `ca_secret_name` in the `default` namespace. + fn provider_with_health_check_tls(network: &str, ca_secret_name: &str) -> InferenceProvider { + serde_json::from_value(serde_json::json!({ + "apiVersion": "grid.praxis-proxy.io/v1alpha1", + "kind": "InferenceProvider", + "metadata": { "name": "prov" }, + "spec": { + "gridNetworkRef": network, + "providerKind": "self_hosted", + "backendKind": "local", + "endpoint": "http://localhost:8000", + "models": [{"name": "model"}], + "healthCheck": { + "tls": { + "caSecretRef": { "name": ca_secret_name, "namespace": "default" } + } + } + } + })) + .unwrap_or_else(|_| std::process::abort()) + } + + #[tokio::test] + async fn resolve_phase_and_sites_health_check_key_absent_from_existing_secret_yields_degraded_key_missing() { + let client = mock_kube_client_for_health_tls( + "net-1", + HashMap::from([("ca-secret", secret_with_key("wrong-key", b"ca-bytes"))]), + ); + let provider = provider_with_health_check_tls("net-1", "ca-secret"); + + let (phase, matching, reason) = resolve_phase_and_sites(&provider, &client) + .await + .expect("mocked API calls must not fail"); + + assert_eq!(phase, ProviderPhase::Degraded); + assert!(matching.is_empty(), "no GridSites exist in this fixture"); + assert_eq!( + reason.as_deref(), + Some("HealthCheckTlsKeyMissing"), + "grid#58: end-to-end through resolve_phase_and_sites (the function reconcile() calls), a key \ + absent from an existing Secret's data must produce status.reason = HealthCheckTlsKeyMissing, \ + not HealthCheckTlsSecretMissing" + ); + } + + #[tokio::test] + async fn resolve_phase_and_sites_health_check_secret_absent_yields_degraded_secret_missing() { + let client = mock_kube_client_for_health_tls("net-1", HashMap::new()); + let provider = provider_with_health_check_tls("net-1", "absent-secret"); + + let (phase, _matching, reason) = resolve_phase_and_sites(&provider, &client) + .await + .expect("mocked API calls must not fail"); + + assert_eq!(phase, ProviderPhase::Degraded); + assert_eq!(reason.as_deref(), Some("HealthCheckTlsSecretMissing")); + } + // ----------------------------------------------------------------------- // validate_provider_config — static validation (items 1-4) // ----------------------------------------------------------------------- diff --git a/xtask/src/env/mod.rs b/xtask/src/env/mod.rs index 40ead29..2e70679 100644 --- a/xtask/src/env/mod.rs +++ b/xtask/src/env/mod.rs @@ -1851,7 +1851,7 @@ fn run_operator_reconcile(context: &str) -> Result Result Result> { @@ -1926,6 +1927,15 @@ fn run_operator_reconcile(context: &str) -> Result Result<(), Box String { + kubectl_jsonpath(context, &format!("inferenceproviders/{name}"), "{.status.reason}").unwrap_or_default() +} + +#[expect( + clippy::disallowed_methods, + reason = "synchronous poll loop in xtask; no async runtime available" +)] +/// Poll until `InferenceProvider` `name` has both the expected `phase` and `status.reason`. +/// +/// Returns `Ok(())` when both match within `timeout`. Returns `Err` if the +/// timeout elapses, reporting the last-observed `(phase, reason)` pair. +pub(crate) fn wait_for_provider_phase_and_reason( + context: &str, + name: &str, + expected_phase: &str, + expected_reason: &str, + timeout: Duration, +) -> Result<(), Box> { + let start = Instant::now(); + loop { + let phase = + kubectl_jsonpath(context, &format!("inferenceproviders/{name}"), "{.status.phase}").unwrap_or_default(); + let reason = read_provider_reason(context, name); + + if phase == expected_phase && reason == expected_reason { + eprintln!(" [OK] {name} phase={phase} reason={reason:?}"); + return Ok(()); + } + if start.elapsed() >= timeout { + return Err(format!( + "timeout waiting for {name} phase={expected_phase:?} reason={expected_reason:?}; \ + last observed: phase={phase:?} reason={reason:?}" + ) + .into()); + } + eprintln!( + " waiting for {name} phase={expected_phase:?} reason={expected_reason:?} \ + (observed: phase={phase:?} reason={reason:?})..." + ); + std::thread::sleep(POLL_INTERVAL); + } +} + #[expect( clippy::disallowed_methods, reason = "synchronous poll loop in xtask; no async runtime available" @@ -2801,6 +2866,67 @@ pub(crate) fn apply_degraded_provider_fixture(context: &str, endpoint: &str) -> Ok(()) } +/// Create [`TLS_KEY_MISSING_CA_SECRET_NAME`] with its data under `wrong-key` +/// instead of the `ca.crt` key `resolve_tls_config` expects. +fn apply_tls_key_missing_ca_secret(context: &str) -> Result<(), Box> { + let manifest = [ + "apiVersion: v1", + "kind: Secret", + "metadata:", + &format!(" name: {TLS_KEY_MISSING_CA_SECRET_NAME}"), + " namespace: default", + "type: Opaque", + "stringData:", + " wrong-key: not-a-real-ca-cert", + "", + ] + .join("\n"); + kubectl::apply_manifest(context, &manifest) +} + +/// Create the CA Secret and apply the `InferenceProvider` fixture for the +/// grid#58 health-check-TLS-key-missing regression scenario. +/// +/// The Secret is created with data under `wrong-key` instead of the expected +/// `ca.crt`, so it exists but the key lookup still fails: `resolve_tls_config` +/// must return `TlsFailureReason::KeyMissing`, and the provider must +/// reconcile to `Degraded` / `status.reason = "HealthCheckTlsKeyMissing"` +/// rather than the pre-fix (incorrect) `"HealthCheckTlsSecretMissing"`. +/// +/// The endpoint is a placeholder — TLS resolution runs and fails before any +/// health probe is attempted (see `resolve_phase_and_sites`), so no live HTTP +/// backend is required for this fixture. +pub(crate) fn apply_provider_with_health_check_tls_key_missing_fixture( + context: &str, +) -> Result<(), Box> { + apply_tls_key_missing_ca_secret(context)?; + + let manifest = serde_json::to_string_pretty(&serde_json::json!({ + "apiVersion": "grid.praxis-proxy.io/v1alpha1", + "kind": "InferenceProvider", + "metadata": { "name": TEST_PROVIDER_TLS_KEY_MISSING }, + "spec": { + "gridNetworkRef": TEST_NETWORK, + "providerKind": "self_hosted", + "backendKind": "local", + "endpoint": "http://127.0.0.1:1", + "models": [{ "name": "model-tls-key-missing" }], + "healthCheck": { + "tls": { + "caSecretRef": { "name": TLS_KEY_MISSING_CA_SECRET_NAME, "namespace": "default" } + } + } + } + })) + .unwrap_or_else(|e| { + eprintln!("tls-key-missing provider fixture serialization failed: {e}"); + std::process::exit(1); + }); + kubectl::apply_manifest(context, &manifest)?; + eprintln!(" [OK] TLS-key-missing provider fixture applied (grid#58 regression)"); + Ok(()) +} + /// Apply the `api_provider` `InferenceProvider` fixture. /// /// Uses `backendKind: "api_provider"` so the scoring engine assigns it a lower From f78c4f37273b34c707f2f12caf66042b123cc6e4 Mon Sep 17 00:00:00 2001 From: Jordi Gil Date: Thu, 13 Aug 2026 21:44:31 -0400 Subject: [PATCH 3/5] test(xtask): add TLS-key-missing fixture to cleanup-coverage doc test Follow-up to the previous commit: the cleanup_includes_all_owned_providers documentation test enforces that every InferenceProvider fixture name is registered for idempotent cleanup between runs. Missed adding the new TEST_PROVIDER_TLS_KEY_MISSING constant when it was introduced. Signed-off-by: Jordi Gil --- xtask/src/env/operator.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/xtask/src/env/operator.rs b/xtask/src/env/operator.rs index d639a6f..a0dbe9d 100644 --- a/xtask/src/env/operator.rs +++ b/xtask/src/env/operator.rs @@ -7857,6 +7857,7 @@ mod tests { TEST_PROVIDER_API, TEST_METRICS_IDLE_PROVIDER, TEST_METRICS_BUSY_PROVIDER, + TEST_PROVIDER_TLS_KEY_MISSING, ]; // Each must be distinct (no duplicate delete). let unique: std::collections::HashSet<_> = all_providers.iter().collect(); From a7c80baa2e1c1319524bee76e0a77adf4f0cb380 Mon Sep 17 00:00:00 2001 From: Jordi Gil Date: Fri, 14 Aug 2026 10:24:13 -0400 Subject: [PATCH 4/5] fix(operator): resolve broken rustdoc intra-doc link in endpoint_tls `[`secret::read_secret_bytes`]` referenced an out-of-scope `secret` path at the doc-comment's location, failing the rustdoc CI check (-D rustdoc::broken-intra-doc-links). Use the same fully-qualified link style already used one line below for SecretKeyLookup. Signed-off-by: Jordi Gil --- operator/src/resources/endpoint_tls.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/operator/src/resources/endpoint_tls.rs b/operator/src/resources/endpoint_tls.rs index e5849cb..84fb4e1 100644 --- a/operator/src/resources/endpoint_tls.rs +++ b/operator/src/resources/endpoint_tls.rs @@ -280,7 +280,8 @@ pub(crate) async fn verify_tls_accessible( /// Read raw bytes from a Kubernetes Secret for TLS validation. /// -/// Thin wrapper over [`secret::read_secret_bytes`] that maps its +/// Thin wrapper over [`read_secret_bytes`](crate::resources::secret::read_secret_bytes) +/// that maps its /// [`SecretKeyLookup`](crate::resources::secret::SecretKeyLookup) result /// onto the [`TlsFailureReason`] this module's callers expect, so "Secret /// not found" and "key not found" map to the correct variant. From 62b145ad3150ae34510a271a655f9769adfd1296 Mon Sep 17 00:00:00 2001 From: Jordi Gil Date: Fri, 14 Aug 2026 11:48:48 -0400 Subject: [PATCH 5/5] refactor(operator): dedupe Secret test doubles, drop test-fn doc comments Addresses grid#59 review feedback (praxis-bot): - mock_kube_client_with_secrets was duplicated verbatim (~40 lines) between secret.rs::tests and endpoint_tls.rs::tests, with secret_with_key also duplicated in spirit. Extracted both into a new resources::test_doubles module (cfg(test)-gated at the mod declaration) so the two copies cannot drift independently. - Removed doc comments on read_secret_bytes_key_absent_from_existing_secret_returns_key_missing (secret.rs) and resolve_tls_config_ca_key_absent_from_existing_secret_yields_key_missing (endpoint_tls.rs) per the project convention (CLAUDE.md, Test Organization: function name is the documentation) -- both were redundant with the grid#58 regression context already carried by each test's assertion message. Signed-off-by: Jordi Gil --- operator/src/resources/endpoint_tls.rs | 65 +------------------------ operator/src/resources/mod.rs | 4 ++ operator/src/resources/secret.rs | 52 +------------------- operator/src/resources/test_doubles.rs | 66 ++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 115 deletions(-) create mode 100644 operator/src/resources/test_doubles.rs diff --git a/operator/src/resources/endpoint_tls.rs b/operator/src/resources/endpoint_tls.rs index 84fb4e1..8fb31db 100644 --- a/operator/src/resources/endpoint_tls.rs +++ b/operator/src/resources/endpoint_tls.rs @@ -315,67 +315,8 @@ async fn read_tls_secret_for_verify( mod tests { use std::collections::HashMap; - use k8s_openapi::{ByteString, api::core::v1::Secret}; - use super::*; - - // ----------------------------------------------------------------------- - // Test doubles - // ----------------------------------------------------------------------- - - /// Build a `kube::Client` backed by an in-memory map of Secret name to - /// `Secret`, so `resolve_tls_config`/`verify_tls_accessible` can be - /// exercised without a real cluster. Any name not present in the map - /// returns HTTP 404. - #[expect( - clippy::too_many_lines, - reason = "test mock builder: 404-vs-200 branches are the whole point" - )] - fn mock_kube_client_with_secrets(secrets: HashMap<&'static str, Secret>) -> kube::Client { - let service = tower::service_fn(move |req: http::Request| { - let secrets = secrets.clone(); - async move { - let name = req.uri().path().rsplit('/').next().unwrap_or_default().to_owned(); - let response = secrets.get(name.as_str()).map_or_else( - || { - let not_found = serde_json::json!({ - "kind": "Status", - "apiVersion": "v1", - "status": "Failure", - "message": format!("secrets \"{name}\" not found"), - "reason": "NotFound", - "code": 404, - }); - http::Response::builder() - .status(404) - .body(kube::client::Body::from( - serde_json::to_vec(¬_found).unwrap_or_else(|_| std::process::abort()), - )) - .unwrap_or_else(|_| std::process::abort()) - }, - |secret| { - http::Response::builder() - .status(200) - .body(kube::client::Body::from( - serde_json::to_vec(secret).unwrap_or_else(|_| std::process::abort()), - )) - .unwrap_or_else(|_| std::process::abort()) - }, - ); - Ok::<_, std::convert::Infallible>(response) - } - }); - kube::Client::new(service, "default") - } - - fn secret_with_key(key: &str, value: &[u8]) -> Secret { - let mut data = std::collections::BTreeMap::new(); - data.insert(key.to_owned(), ByteString(value.to_vec())); - Secret { - data: Some(data), - ..Default::default() - } - } + use crate::resources::test_doubles::{mock_kube_client_with_secrets, secret_with_key}; fn test_tls_config(ca_secret_name: &str) -> EndpointTlsConfig { EndpointTlsConfig { @@ -533,10 +474,6 @@ mod tests { assert_eq!(reason, TlsFailureReason::SecretMissing); } - /// Regression test for grid#58 at the `InferenceProvider` - /// metrics/health-check TLS resolution path (the pipeline the bug was - /// originally reported against): a key absent from an existing CA - /// Secret must surface as `KeyMissing`, not `SecretMissing`. #[tokio::test] async fn resolve_tls_config_ca_key_absent_from_existing_secret_yields_key_missing() { let client = diff --git a/operator/src/resources/mod.rs b/operator/src/resources/mod.rs index 8ce4c98..e89b9b6 100644 --- a/operator/src/resources/mod.rs +++ b/operator/src/resources/mod.rs @@ -35,6 +35,10 @@ pub(crate) mod provider_metrics; pub mod routing_overlay; /// Secret builders for grid TLS certificates. pub mod secret; +/// Shared Kubernetes Secret test doubles, reused by `secret`/`endpoint_tls` +/// unit tests instead of each keeping its own copy of the same mock. +#[cfg(test)] +pub(crate) mod test_doubles; /// Trust bundle management for grid mTLS. pub mod trust_bundle; diff --git a/operator/src/resources/secret.rs b/operator/src/resources/secret.rs index 7151a99..e875d76 100644 --- a/operator/src/resources/secret.rs +++ b/operator/src/resources/secret.rs @@ -207,54 +207,7 @@ mod tests { use std::collections::HashMap; use super::*; - - // ----------------------------------------------------------------------- - // Test doubles - // ----------------------------------------------------------------------- - - /// Build a `kube::Client` backed by an in-memory map of Secret name to - /// `Secret`, so `read_secret_bytes` can be exercised without a real - /// cluster. Any name not present in the map returns HTTP 404. - #[expect( - clippy::too_many_lines, - reason = "test mock builder: 404-vs-200 branches are the whole point" - )] - fn mock_kube_client_with_secrets(secrets: HashMap<&'static str, Secret>) -> kube::Client { - let service = tower::service_fn(move |req: http::Request| { - let secrets = secrets.clone(); - async move { - let name = req.uri().path().rsplit('/').next().unwrap_or_default().to_owned(); - let response = secrets.get(name.as_str()).map_or_else( - || { - let not_found = serde_json::json!({ - "kind": "Status", - "apiVersion": "v1", - "status": "Failure", - "message": format!("secrets \"{name}\" not found"), - "reason": "NotFound", - "code": 404, - }); - http::Response::builder() - .status(404) - .body(kube::client::Body::from( - serde_json::to_vec(¬_found).unwrap_or_else(|_| std::process::abort()), - )) - .unwrap_or_else(|_| std::process::abort()) - }, - |secret| { - http::Response::builder() - .status(200) - .body(kube::client::Body::from( - serde_json::to_vec(secret).unwrap_or_else(|_| std::process::abort()), - )) - .unwrap_or_else(|_| std::process::abort()) - }, - ); - Ok::<_, std::convert::Infallible>(response) - } - }); - kube::Client::new(service, "default") - } + use crate::resources::test_doubles::mock_kube_client_with_secrets; fn secret_ref(name: &str) -> crate::crd::grid_network::SecretRef { crate::crd::grid_network::SecretRef { @@ -313,9 +266,6 @@ mod tests { ); } - /// Regression test for grid#58: a key absent from an *existing* Secret's - /// `data` map must be reported as `KeyMissing`, not conflated with the - /// Secret itself being absent. #[tokio::test] async fn read_secret_bytes_key_absent_from_existing_secret_returns_key_missing() { let mut data = BTreeMap::new(); diff --git a/operator/src/resources/test_doubles.rs b/operator/src/resources/test_doubles.rs new file mode 100644 index 0000000..eaae2e9 --- /dev/null +++ b/operator/src/resources/test_doubles.rs @@ -0,0 +1,66 @@ +//! Shared Kubernetes Secret test doubles for `resources` unit tests. +//! +//! Extracted from `secret.rs`/`endpoint_tls.rs`, which had grown byte-identical +//! copies of the same mocked `kube::Client` builder — kept here once so the +//! two copies cannot drift independently. +//! +//! Declared behind `#[cfg(test)]` at the `mod test_doubles;` site in +//! `resources/mod.rs`, so this whole module compiles out of non-test builds. + +use std::collections::HashMap; + +use k8s_openapi::{ByteString, api::core::v1::Secret}; + +/// Build a `kube::Client` backed by an in-memory map of Secret name to +/// `Secret`, so Secret-reading code can be exercised without a real cluster. +/// Any name not present in the map returns HTTP 404. +#[expect( + clippy::too_many_lines, + reason = "test mock builder: 404-vs-200 branches are the whole point" +)] +pub(crate) fn mock_kube_client_with_secrets(secrets: HashMap<&'static str, Secret>) -> kube::Client { + let service = tower::service_fn(move |req: http::Request| { + let secrets = secrets.clone(); + async move { + let name = req.uri().path().rsplit('/').next().unwrap_or_default().to_owned(); + let response = secrets.get(name.as_str()).map_or_else( + || { + let not_found = serde_json::json!({ + "kind": "Status", + "apiVersion": "v1", + "status": "Failure", + "message": format!("secrets \"{name}\" not found"), + "reason": "NotFound", + "code": 404, + }); + http::Response::builder() + .status(404) + .body(kube::client::Body::from( + serde_json::to_vec(¬_found).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + }, + |secret| { + http::Response::builder() + .status(200) + .body(kube::client::Body::from( + serde_json::to_vec(secret).unwrap_or_else(|_| std::process::abort()), + )) + .unwrap_or_else(|_| std::process::abort()) + }, + ); + Ok::<_, std::convert::Infallible>(response) + } + }); + kube::Client::new(service, "default") +} + +/// Build a Secret with a single `data` key. +pub(crate) fn secret_with_key(key: &str, value: &[u8]) -> Secret { + let mut data = std::collections::BTreeMap::new(); + data.insert(key.to_owned(), ByteString(value.to_vec())); + Secret { + data: Some(data), + ..Default::default() + } +}