diff --git a/crates/osutils/src/dmi.rs b/crates/osutils/src/dmi.rs new file mode 100644 index 0000000000..a0598cf8a1 --- /dev/null +++ b/crates/osutils/src/dmi.rs @@ -0,0 +1,54 @@ +//! Helpers for reading hardware identity information exposed by the kernel's +//! DMI (Desktop Management Interface) sysfs tree. + +use std::path::Path; + +use anyhow::{Context, Error}; + +use sysdefs::osuuid::OsUuid; + +use crate::files; + +/// Path to the hardware product UUID exposed by the kernel. kubelet +/// populates a Node's `status.nodeInfo.systemUUID` from this same file, and +/// Trident's tracing metadata uses it as an asset identifier. +pub const PRODUCT_UUID_PATH: &str = "/sys/class/dmi/id/product_uuid"; + +/// Reads and trims the hardware product UUID from [`PRODUCT_UUID_PATH`]. +pub fn read_product_uuid() -> Result { + read_uuid_from(PRODUCT_UUID_PATH) +} + +fn read_uuid_from(path: impl AsRef) -> Result { + files::read_file_trim(&path.as_ref()) + .map(OsUuid::from) + .with_context(|| format!("Failed to read UUID from '{}'", path.as_ref().display())) +} + +#[cfg(test)] +mod tests { + use super::*; + + use std::fs; + + #[test] + fn reads_and_trims_product_uuid() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("product_uuid"); + for value in ["1234-ABCD", "6BA7B810-9DAD-11D1-80B4-00C04FD430C8", ""] { + fs::write(&path, format!(" {value}\n")).unwrap(); + assert_eq!(read_uuid_from(&path).unwrap(), OsUuid::from(value)); + } + } + + #[test] + fn errors_for_missing_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("missing"); + let error = read_uuid_from(&path).unwrap_err(); + assert!( + error.to_string().contains(&path.display().to_string()), + "{error:#}" + ); + } +} diff --git a/crates/osutils/src/lib.rs b/crates/osutils/src/lib.rs index 48a28b9d4e..6efde3b846 100644 --- a/crates/osutils/src/lib.rs +++ b/crates/osutils/src/lib.rs @@ -5,6 +5,7 @@ pub mod chroot; pub mod container; pub mod dependencies; pub mod df; +pub mod dmi; pub mod e2fsck; pub mod efibootmgr; pub mod efivar; diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index 86c794d260..53210133ea 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -30,10 +30,14 @@ use kube::{ }, Api, Client, Config, Error as KubeError, }; +use log::warn; use reqwest::StatusCode; use serde_json::json; use thiserror::Error; +use osutils::dmi; +use sysdefs::osuuid::OsUuid; + use crate::core::config::KubernetesConfig; /// Floor for the Kubernetes watch request's `timeoutSeconds`, decoupled from @@ -59,6 +63,10 @@ pub struct NodeClient { api: Api, poll_interval: Duration, cluster_url: String, + /// Mirrors [`KubernetesConfig::validate_node_uuid`]: enables + /// [`verify_node_identity`]'s `systemUUID`-vs-local-hardware check in + /// both [`NodeClient::get_node`] and [`NodeClient::watch_node`]. + validate_identity: bool, } impl NodeClient { @@ -70,6 +78,7 @@ impl NodeClient { api: Api::all(client), poll_interval: config.watch_poll_interval, cluster_url, + validate_identity: config.validate_node_uuid, }) } @@ -78,7 +87,11 @@ impl NodeClient { } pub async fn get_node(&self, name: &str) -> Result { - self.api.get(name).await.map_err(map_kube_error) + let node = self.api.get(name).await.map_err(map_kube_error)?; + if self.validate_identity { + verify_node_identity(&node, name, dmi::read_product_uuid())?; + } + Ok(node) } pub async fn patch_node_labels( @@ -143,10 +156,26 @@ impl NodeClient { .fields(&format!("metadata.name={name}")) .timeout(timeout_secs); + // Applied per watch event below, mirroring get_node: a Node emitted + // by the watch whose systemUUID doesn't match this machine is just + // as stale/wrong as one returned by a direct get, and must be routed + // into the same NodeGone handling rather than silently reconciling + // against the wrong node. + let validate_identity = self.validate_identity; + watcher::watcher(self.api.clone(), watcher_config) .default_backoff() .touched_objects() .map_err(map_watch_error) + .and_then(move |node| { + let name = name.clone(); + async move { + if validate_identity { + verify_node_identity(&node, &name, dmi::read_product_uuid())?; + } + Ok(node) + } + }) .boxed() } } @@ -161,6 +190,71 @@ fn is_not_found(err: &KubeError) -> bool { matches!(err, KubeError::Api(resp) if is_not_found_response(resp)) } +/// Confirms `node`'s reported `status.nodeInfo.systemUUID` matches this +/// machine's own hardware product UUID from [`dmi::read_product_uuid`]. +/// A mismatch means the Node object we +/// fetched by name does not describe this machine - e.g. the Node name was +/// recycled onto different hardware - so it's treated identically to the +/// Node not existing ([`K8sClientError::NodeGone`]). +/// +/// If the local product UUID can't be read, the check is skipped (logged at +/// warn) rather than failing closed, so a host without DMI data (e.g. some +/// VM/container test environments) doesn't lose all Node access. +/// +/// Likewise, if the Node's reported `systemUUID` is empty, the check is +/// skipped. kubelet populates this field via cadvisor reading the same +/// `product_uuid` file; if that read ever fails on the kubelet's side, +/// cadvisor logs an error but still lets node registration proceed with an +/// empty `systemUUID` rather than surfacing the error. An empty value is +/// therefore evidence kubelet couldn't determine the UUID - not evidence the +/// node is a different machine - so treating it as a mismatch would risk a +/// false positive that locks us out of an otherwise-healthy node forever. +fn verify_node_identity( + node: &Node, + name: &str, + local_uuid: Result, +) -> Result<(), K8sClientError> { + let local_uuid = match local_uuid { + Ok(uuid) => uuid, + Err(err) => { + warn!( + "failed to read local product uuid, skipping Node {name:?} identity verification: {err:#}" + ); + return Ok(()); + } + }; + + let local_uuid = local_uuid.to_string(); + if local_uuid.trim().is_empty() { + warn!("Local product uuid is empty, skipping Node {name:?} identity verification"); + return Ok(()); + } + + let node_uuid = node + .status + .as_ref() + .and_then(|status| status.node_info.as_ref()) + .map(|node_info| node_info.system_uuid.as_str()) + .unwrap_or_default(); + + if node_uuid.is_empty() { + warn!("Node {name:?} status.nodeInfo.systemUUID is empty, skipping identity verification"); + return Ok(()); + } + + if !OsUuid::from(node_uuid) + .to_string() + .eq_ignore_ascii_case(&local_uuid) + { + warn!( + "Node {name:?} status.nodeInfo.systemUUID {node_uuid:?} does not match local product uuid {local_uuid:?}; treating node as not found" + ); + return Err(K8sClientError::NodeGone); + } + + Ok(()) +} + fn map_kube_error(err: KubeError) -> K8sClientError { if is_not_found(&err) { K8sClientError::NodeGone @@ -273,4 +367,71 @@ mod tests { assert!(matches!(map_watch_error(err), K8sClientError::Watch(_))); } + + fn node_with_system_uuid(uuid: &str) -> Node { + use k8s_openapi::api::core::v1::{NodeStatus, NodeSystemInfo}; + + Node { + status: Some(NodeStatus { + node_info: Some(NodeSystemInfo { + system_uuid: uuid.to_string(), + ..Default::default() + }), + ..Default::default() + }), + ..Default::default() + } + } + + #[test] + fn matching_system_uuid_is_ok() { + let node = node_with_system_uuid("1234-ABCD"); + verify_node_identity(&node, "n", Ok(OsUuid::from("1234-ABCD"))).unwrap(); + } + + #[test] + fn matching_system_uuid_is_case_insensitive() { + for (local, remote) in [ + ("1234-abcd", "1234-ABCD"), + ( + "6BA7B810-9DAD-11D1-80B4-00C04FD430C8", + "6ba7b810-9dad-11d1-80b4-00c04fd430c8", + ), + ] { + let node = node_with_system_uuid(remote); + verify_node_identity(&node, "n", Ok(OsUuid::from(local))).unwrap(); + } + } + + #[test] + fn mismatched_system_uuid_is_node_gone() { + let node = node_with_system_uuid("5678-EFGH"); + let error = verify_node_identity(&node, "n", Ok(OsUuid::from("1234-ABCD"))).unwrap_err(); + assert!(matches!(error, K8sClientError::NodeGone), "got {error:?}"); + } + + #[test] + fn missing_node_system_info_skips_verification() { + verify_node_identity(&Node::default(), "n", Ok(OsUuid::from("1234-ABCD"))).unwrap(); + } + + #[test] + fn empty_node_system_uuid_skips_verification() { + let node = node_with_system_uuid(""); + verify_node_identity(&node, "n", Ok(OsUuid::from("1234-ABCD"))).unwrap(); + } + + #[test] + fn unreadable_local_uuid_skips_verification() { + let node = node_with_system_uuid("5678-EFGH"); + verify_node_identity(&node, "n", Err(Error::msg("UUID file unavailable"))).unwrap(); + } + + #[test] + fn empty_local_uuid_skips_verification() { + let node = node_with_system_uuid("1234-ABCD"); + for local in ["", " \n"] { + verify_node_identity(&node, "n", Ok(OsUuid::from(local))).unwrap(); + } + } } diff --git a/crates/trident-acl-agent/src/core/config.rs b/crates/trident-acl-agent/src/core/config.rs index 2f25af9303..d66e34987f 100644 --- a/crates/trident-acl-agent/src/core/config.rs +++ b/crates/trident-acl-agent/src/core/config.rs @@ -116,6 +116,7 @@ impl AgentConfig { annotation_prefix: kubernetes .annotation_prefix .unwrap_or_else(|| DEFAULT_ANNOTATION_PREFIX.to_string()), + validate_node_uuid: kubernetes.validate_node_uuid.unwrap_or(false), }, trident: TridentConfig { socket: trident @@ -168,6 +169,8 @@ struct RawKubernetesConfig { node_name: Option, #[serde(deserialize_with = "empty_string_as_none")] annotation_prefix: Option, + #[serde(deserialize_with = "empty_bool_as_none")] + validate_node_uuid: Option, } /// Mirrors [`TridentConfig`] (see [`RawNebraskaConfig`]). @@ -254,6 +257,21 @@ where .transpose() } +fn empty_bool_as_none<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + empty_as_none(String::deserialize(deserializer)?) + .map(|value| { + value.parse::().map_err(|_| { + D::Error::custom(format!( + "invalid boolean {value:?} (expected \"true\" or \"false\")" + )) + }) + }) + .transpose() +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct NebraskaConfig { pub endpoint: Option, @@ -291,6 +309,16 @@ pub struct KubernetesConfig { /// `TRIDENT_ACL_AGENT_KUBERNETES_ANNOTATION_PREFIX` so a deployment can /// pick its own namespace instead. pub annotation_prefix: String, + /// Enables [`NodeClient`](crate::annotations::k8s::NodeClient)'s + /// `systemUUID`-vs-local-hardware identity check on every Node read + /// (both explicit `get_node` calls and each Node delivered by the + /// long-lived watch). Disabled by default: the check's assumption + /// (kubelet's `systemUUID` always traces back to this same machine's + /// `/sys/class/dmi/id/product_uuid`) hold for the environments this was + /// validated against, but hasn't been confirmed across every deployment + /// trident-acl-agent runs in, so it starts opt-in rather than risking + /// false-positive `NodeGone` loops on a healthy cluster. + pub validate_node_uuid: bool, } impl Default for KubernetesConfig { @@ -301,6 +329,7 @@ impl Default for KubernetesConfig { node_name: default_node_name(), watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, annotation_prefix: DEFAULT_ANNOTATION_PREFIX.to_string(), + validate_node_uuid: false, } } } @@ -466,6 +495,10 @@ mod tests { config.kubernetes.annotation_prefix, DEFAULT_ANNOTATION_PREFIX ); + assert!( + !config.kubernetes.validate_node_uuid, + "Node systemUUID verification should be disabled by default" + ); } #[test] @@ -503,6 +536,7 @@ mod tests { "TRIDENT_ACL_AGENT_KUBERNETES_ANNOTATION_PREFIX", "contoso.example.com", ), + ("TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID", "true"), ])) .unwrap(); @@ -544,6 +578,10 @@ mod tests { Some(Duration::from_secs(30 * 60)) ); assert_eq!(config.kubernetes.annotation_prefix, "contoso.example.com"); + assert!( + config.kubernetes.validate_node_uuid, + "validate_node_uuid should be true when explicitly set" + ); } #[test] @@ -600,4 +638,24 @@ mod tests { .unwrap_err(); assert!(format!("{err:#}").contains("not a duration"), "{err:#}"); } + + #[test] + fn validate_node_uuid_unset_by_empty_value_defaults_to_false() { + let config = AgentConfig::from_vars(vars(&[( + "TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID", + "", + )])) + .unwrap(); + assert!(!config.kubernetes.validate_node_uuid); + } + + #[test] + fn malformed_validate_node_uuid_is_a_parse_error() { + let err = AgentConfig::from_vars(vars(&[( + "TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID", + "yes", + )])) + .unwrap_err(); + assert!(format!("{err:#}").contains("yes"), "{err:#}"); + } } diff --git a/crates/trident/src/logging/tracestream.rs b/crates/trident/src/logging/tracestream.rs index 1326968337..c7ffe05fc8 100644 --- a/crates/trident/src/logging/tracestream.rs +++ b/crates/trident/src/logging/tracestream.rs @@ -1,6 +1,6 @@ use std::{ collections::BTreeMap, - fs::{self, File}, + fs::File, io::Write, sync::{Arc, RwLock}, time::Instant, @@ -19,15 +19,14 @@ use tracing::{ use tracing_subscriber::{layer::Layer, registry::LookupSpan}; use osutils::{ - files, + dmi, files, osrelease::{OsRelease, OS_RELEASE_PATH}, uname, }; +use sysdefs::osuuid::OsUuid; use crate::{TRIDENT_METRICS_FILE_PATH, TRIDENT_VERSION}; -/// The product uuid is used to identify the hardware that Trident is running on. -const PRODUCT_UUID_FILE: &str = "/sys/class/dmi/id/product_uuid"; lazy_static::lazy_static! { static ref ADDITIONAL_FIELDS: BTreeMap = populate_additional_fields(); pub static ref PLATFORM_INFO: BTreeMap = populate_platform_info(); @@ -349,15 +348,14 @@ where } } -/// Obtain product uuid of the hardware Trident is running on -fn read_product_uuid(filepath: String) -> String { - match fs::read_to_string(filepath.clone()) { - Ok(uuid) => uuid.trim().to_string(), - Err(_) => { - debug!("Failed to read product uuid from {}", filepath); - "unknown".into() - } - } +/// Obtain product uuid of the hardware Trident is running on. Falls back to +/// "unknown" (rather than failing) since this value is purely informational +/// metadata attached to trace entries. +fn product_uuid_or_unknown(uuid: Result) -> String { + uuid.map(|uuid| uuid.to_string()).unwrap_or_else(|err| { + debug!("Failed to read product uuid: {err:#}"); + "unknown".into() + }) } fn populate_additional_fields() -> BTreeMap { @@ -393,7 +391,7 @@ fn populate_platform_info() -> BTreeMap { sys.refresh_all(); platform_info.insert( "asset_id".to_string(), - json!(read_product_uuid(PRODUCT_UUID_FILE.into())), + json!(product_uuid_or_unknown(dmi::read_product_uuid())), ); platform_info.insert("os_release".to_string(), json!(get_os_release())); platform_info.insert("total_cpu".to_string(), json!(sys.cpus().len())); @@ -417,8 +415,6 @@ fn populate_platform_info() -> BTreeMap { mod tests { use super::*; - use std::{fs::File, io::Write}; - #[test] fn test_tracestream() { let tracestream = TraceStream::default(); @@ -463,17 +459,13 @@ mod tests { #[test] fn test_read_product_uuid_unknown() { - let uuid = read_product_uuid("unknown".to_string()); + let uuid = product_uuid_or_unknown(Err(anyhow!("UUID file unavailable"))); assert_eq!(uuid, "unknown"); } #[test] fn test_read_product_uuid_exists() { - let temp_dir = tempfile::tempdir().unwrap(); - let filepath = temp_dir.path().join("product_uuid"); - let mut file = File::create(&filepath).unwrap(); - file.write_all("test_uuid".as_bytes()).unwrap(); - let uuid = read_product_uuid(filepath.to_str().unwrap().to_string()); + let uuid = product_uuid_or_unknown(Ok(OsUuid::from("test_uuid"))); assert_eq!(uuid, "test_uuid"); } } @@ -536,7 +528,7 @@ mod functional_test { let mut expected_platform_info = BTreeMap::new(); expected_platform_info.insert( "asset_id".to_string(), - json!(read_product_uuid(PRODUCT_UUID_FILE.into())), + json!(product_uuid_or_unknown(dmi::read_product_uuid())), ); expected_platform_info.insert("os_release".to_string(), json!(get_os_release())); expected_platform_info.insert("total_cpu".to_string(), json!(4)); diff --git a/docs/Development/Testing/TridentAclAgent-Tests.md b/docs/Development/Testing/TridentAclAgent-Tests.md index e11dae9e5f..8d69c15932 100644 --- a/docs/Development/Testing/TridentAclAgent-Tests.md +++ b/docs/Development/Testing/TridentAclAgent-Tests.md @@ -268,7 +268,12 @@ The scenario runs these test cases in order: Both phases assert the service's `systemd` `MainPID` stays unchanged throughout (i.e. it never crashes or restarts) and that it resumes once `NodeStore.RestoreNode` brings the Node back — all before any fake - Nebraska/image-server mocks or real update/rollback traffic is involved + Nebraska/image-server mocks or real update/rollback traffic is involved. + **Phase 3** additionally checks a one-shot GET with UUID validation explicitly + enabled: a matching UUID succeeds, a mismatch fails with both the UUID-mismatch + and NodeGone diagnostics from that invocation, and restoration succeeds. + This isolates GET validation from daemon/watch logs. **Phase 4** exercises + mismatched UUID detection through the long-lived watch. 4. **run-ab-update** — Starts the fake apiserver and fake Nebraska/Omaha endpoints in-process (the latter over HTTPS — see [Nebraska/Image Server TLS](#nebraskaimage-server-tls)), delivers a fake kubeconfig and restarts diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 6777d721b9..afb844829b 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -267,6 +267,7 @@ naming the offending variable. | `TRIDENT_ACL_AGENT_ORCHESTRATION_FINALIZE_TIMEOUT` | `10m` | How long a `finalize` is allowed to run before it's considered failed. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_HEARTBEAT_INTERVAL` | `60s` | Refresh cadence for the `InProgress` status heartbeat. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_NODE_GONE_MAX_WAIT` | unset (wait forever) | How long the agent will keep waiting for its own Node object to reappear after a 404 before giving up and exiting (a [`humantime`](https://docs.rs/humantime) duration, e.g. `30m`, `1h`). Unset means the agent never gives up on its own — see below. | +| `TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID` | `false` | When `true`, every Node read verifies `status.nodeInfo.systemUUID` against this machine's own `/sys/class/dmi/id/product_uuid`, treating a mismatch the same as the Node not existing (`NodeGone`) — see below. | Kubernetes API server connectivity (both the startup/recovery Node read and the long-lived watch loop) is retried indefinitely by default; there @@ -294,6 +295,30 @@ exit-on-Node-gone behavior back (e.g. to let an external supervisor/alert fire instead) can opt into a bound via `TRIDENT_ACL_AGENT_ORCHESTRATION_NODE_GONE_MAX_WAIT`. +### Node identity verification + +When `TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID` is set to `true`, +every Node read - the +startup/recovery GET, `--validate-connection`, and each Node delivered by +the long-lived watch stream - additionally checks that the fetched Node's +`status.nodeInfo.systemUUID` matches this machine's own +`/sys/class/dmi/id/product_uuid`. kubelet populates `systemUUID` by reading +that same file, so on a healthy, correctly-identified node the two values +always match; a mismatch means the Node object fetched by name doesn't +actually describe this machine (e.g. a stale or recycled Node name) and is +treated identically to the Node not existing (`NodeGone`), including the +same retry/backoff and `TRIDENT_ACL_AGENT_ORCHESTRATION_NODE_GONE_MAX_WAIT` +handling described above. An empty `systemUUID` on the Node, or an +unreadable local `product_uuid` file, skips the check (logged at `warn`) +rather than treating it as a mismatch, since either case only means the +UUID couldn't be determined, not that the Node describes a different +machine. The check is disabled (`false`) by default because, while +kubelet is known to source `systemUUID` from this same file in general, +this hasn't been confirmed as a hard guarantee across every environment +this agent runs in — opt in once that's been validated for a given +deployment. + + ### Setting env vars via a systemd drop-in The agent ships as `trident-acl-agent.service`, with no `Environment=` diff --git a/tests/images/trident-vm-testimage/base/files/trident-acl-agent-override.conf b/tests/images/trident-vm-testimage/base/files/trident-acl-agent-override.conf index 9529661983..0bc4624f79 100644 --- a/tests/images/trident-vm-testimage/base/files/trident-acl-agent-override.conf +++ b/tests/images/trident-vm-testimage/base/files/trident-acl-agent-override.conf @@ -8,3 +8,4 @@ Environment=TRIDENT_ACL_AGENT_CURRENT_VERSION_KEY=IMAGE_VERSION Environment=TRIDENT_ACL_AGENT_ORCHESTRATION_MODE=annotations Environment=TRIDENT_ACL_AGENT_CURRENT_VERSION_PATH=/etc/os-release Environment=TRIDENT_ACL_AGENT_CURRENT_VERSION_FALLBACK=always +Environment=TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID=true diff --git a/tools/storm/aclagent/README.md b/tools/storm/aclagent/README.md index 58d010a0cb..12cf51c80e 100644 --- a/tools/storm/aclagent/README.md +++ b/tools/storm/aclagent/README.md @@ -23,7 +23,11 @@ There is intentionally no fake `tridentd`. object disappearing (HTTP 404) instead of exiting, and resumes once it reappears, both via a restart-triggered startup read and via an already-running agent's in-flight status PATCH; runs before - `run-ab-update` so it exercises a clean, no-pending-state agent + `run-ab-update` so it exercises a clean, no-pending-state agent. + UUID coverage includes a one-shot GET with validation explicitly enabled: + matching UUIDs succeed, a mismatched UUID fails with its own mismatch and + NodeGone diagnostics, and the restored UUID succeeds. These assertions do + not rely on daemon logs; the separate watch phase retains its own coverage. - `run-ab-update` - `run-rollback` - `collect-logs` diff --git a/tools/storm/aclagent/proxies/apiserver.go b/tools/storm/aclagent/proxies/apiserver.go index 56d8924dbb..b12b73e1e0 100644 --- a/tools/storm/aclagent/proxies/apiserver.go +++ b/tools/storm/aclagent/proxies/apiserver.go @@ -39,7 +39,17 @@ type NodeStore struct { deleteOnNextPatch bool } -func NewSeedNode(name string, labels map[string]string) *corev1.Node { +// NewSeedNode builds the fake apiserver's initial Node object. systemUUID +// must be the real VM's hardware product UUID +// (/sys/class/dmi/id/product_uuid) - when TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID +// is set, trident-acl-agent's NodeClient::get_node/watch_node +// (crates/trident-acl-agent/src/annotations/k8s.rs) compare a fetched Node's +// status.nodeInfo.systemUUID against that local file and treat a non-empty +// mismatch the same as the Node not existing at all (an empty systemUUID is +// NOT treated as a mismatch; verification is skipped instead), so leaving +// this unset here would make every get_node/watch_node call against the +// fake apiserver fail as NodeGone once verification is enabled. +func NewSeedNode(name string, labels map[string]string, systemUUID string) *corev1.Node { seed := &corev1.Node{ TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Node"}, ObjectMeta: metav1.ObjectMeta{ @@ -47,6 +57,11 @@ func NewSeedNode(name string, labels map[string]string) *corev1.Node { Labels: map[string]string{}, Annotations: map[string]string{}, }, + Status: corev1.NodeStatus{ + NodeInfo: corev1.NodeSystemInfo{ + SystemUUID: systemUUID, + }, + }, } for key, value := range labels { seed.Labels[key] = value @@ -203,6 +218,24 @@ func (s *NodeStore) SetReadyCondition(ready bool) *corev1.Node { return s.node.DeepCopy() } +// SetSystemUUID overwrites the Node's status.nodeInfo.systemUUID. Used by +// run-node-resilience to exercise trident-acl-agent's systemUUID +// verification (NodeClient::get_node/watch_node in +// crates/trident-acl-agent/src/annotations/k8s.rs): when +// TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID is set, setting a non-empty value +// that doesn't match the VM's real /sys/class/dmi/id/product_uuid makes the +// next get_node/watch_node call treat the Node as not found, exactly like +// DeleteNode does, while setting it back to the real value lets the agent +// "find" the Node again without an actual DeleteNode/RestoreNode cycle. +func (s *NodeStore) SetSystemUUID(uuid string) *corev1.Node { + s.mu.Lock() + defer s.mu.Unlock() + s.node.Status.NodeInfo.SystemUUID = uuid + s.bumpLocked() + s.broadcastLocked() + return s.node.DeepCopy() +} + // DeleteNode simulates the Node object being deleted from the API server: // subsequent GET/PATCH requests for it return HTTP 404, and LIST requests // return an empty NodeList - used by run-node-resilience to exercise diff --git a/tools/storm/aclagent/proxies/apiserver_test.go b/tools/storm/aclagent/proxies/apiserver_test.go index c084c8e443..898968b18f 100644 --- a/tools/storm/aclagent/proxies/apiserver_test.go +++ b/tools/storm/aclagent/proxies/apiserver_test.go @@ -1,9 +1,12 @@ package proxies import ( + "encoding/json" "net/http" "net/http/httptest" "testing" + + corev1 "k8s.io/api/core/v1" ) // newTestAPIServer spins up an in-process httptest.Server backed by a fresh @@ -12,7 +15,7 @@ import ( func newTestAPIServer(t *testing.T) (*httptest.Server, *NodeStore) { t.Helper() const nodeName = "test-node" - store := NewNodeStore(NewSeedNode(nodeName, map[string]string{})) + store := NewNodeStore(NewSeedNode(nodeName, map[string]string{}, "test-system-uuid")) server := NewAPIServer(nodeName, store) ts := httptest.NewServer(server.Handler()) t.Cleanup(ts.Close) @@ -110,3 +113,51 @@ func TestNodeStoreRestoreNodeUndoesDeleteNode(t *testing.T) { t.Fatalf("expected label set before DeleteNode to survive the delete/restore cycle, got %+v", snapshot.Labels) } } + +// TestNodeStoreSeedsSystemUUID confirms a GET serves the seeded +// status.nodeInfo.systemUUID verbatim - the field trident-acl-agent's +// NodeClient::get_node (crates/trident-acl-agent/src/annotations/k8s.rs) +// compares against the VM's own /sys/class/dmi/id/product_uuid. +func TestNodeStoreSeedsSystemUUID(t *testing.T) { + ts, _ := newTestAPIServer(t) + + resp, err := http.Get(ts.URL + "/api/v1/nodes/test-node") + if err != nil { + t.Fatalf("unexpected error on GET: %v", err) + } + defer resp.Body.Close() + + var node corev1.Node + if err := json.NewDecoder(resp.Body).Decode(&node); err != nil { + t.Fatalf("failed to decode Node response: %v", err) + } + if node.Status.NodeInfo.SystemUUID != "test-system-uuid" { + t.Fatalf("expected systemUUID %q, got %q", "test-system-uuid", node.Status.NodeInfo.SystemUUID) + } +} + +// TestNodeStoreSetSystemUUID confirms SetSystemUUID is reflected on the +// very next GET - run-node-resilience's phase 3 uses this to simulate a +// Node that GETs successfully but whose systemUUID doesn't match the VM's +// hardware, which NodeClient::get_node must treat the same as NodeGone. +func TestNodeStoreSetSystemUUID(t *testing.T) { + ts, store := newTestAPIServer(t) + store.SetSystemUUID("mismatched-uuid") + + resp, err := http.Get(ts.URL + "/api/v1/nodes/test-node") + if err != nil { + t.Fatalf("unexpected error on GET after SetSystemUUID: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected 200 after SetSystemUUID (the Node itself is not gone), got %d", resp.StatusCode) + } + + var node corev1.Node + if err := json.NewDecoder(resp.Body).Decode(&node); err != nil { + t.Fatalf("failed to decode Node response: %v", err) + } + if node.Status.NodeInfo.SystemUUID != "mismatched-uuid" { + t.Fatalf("expected systemUUID %q, got %q", "mismatched-uuid", node.Status.NodeInfo.SystemUUID) + } +} diff --git a/tools/storm/aclagent/tests/node_resilience.go b/tools/storm/aclagent/tests/node_resilience.go index 7347761db5..89885436d8 100644 --- a/tools/storm/aclagent/tests/node_resilience.go +++ b/tools/storm/aclagent/tests/node_resilience.go @@ -12,6 +12,8 @@ import ( stormssh "tridenttools/storm/utils/ssh" stormvm "tridenttools/storm/utils/vm" stormvmconfig "tridenttools/storm/utils/vm/config" + + "github.com/sirupsen/logrus" ) // aclAgentService is the systemd unit trident-acl-agent runs as, factored @@ -49,6 +51,15 @@ const aclAgentService = "trident-acl-agent.service" // watch loop (is_node_gone_error on a raw stream error) exists purely // as defense in depth for an error shape the real API server's // documented semantics don't actually produce. +// +// Phase 3 below additionally proves NodeClient::get_node's systemUUID +// verification (also in k8s.rs): a Node object that GETs successfully but +// whose status.nodeInfo.systemUUID doesn't match this VM's own +// /sys/class/dmi/id/product_uuid must be logged and then treated exactly +// like NodeGone (not acted on, not crashed on) by the very same startup +// Node read phase 1 exercises. Phase 4 proves NodeClient::watch_node +// performs the identical check on each Node delivered by the long-lived +// watch stream, without ever restarting the agent. func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllVMConfig) error { vmIP, err := stormvm.GetVmIP(vmConfig) if err != nil { @@ -63,6 +74,11 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon ctx, cancel := context.WithCancel(context.Background()) defer cancel() + productUUID, err := readVmProductUUID(vmConfig.VMConfig, vmIP) + if err != nil { + return err + } + // Only the fake apiserver is needed for phase 1. Phase 2 patches in a // "stage" request to reach handle_stage's first publish_status call, // but is deleted before the agent would ever actually dial the @@ -70,7 +86,7 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon // image-server mock is needed either. trident-acl-agent never gets a // config file at all (see prepareVmForAclAgent's doc comment in // update.go). - nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{})) + nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{}, productUUID)) apiServer := stormproxies.NewAPIServer(testConfig.NodeName, nodeStore) _, apiServerStop, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("%s:%d", testConfig.HostEndpointIP, testConfig.APIServerPort)) if err != nil { @@ -99,6 +115,7 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon const nodeGoneLogSubstring = "no longer exists; waiting for it to reappear" const nodeReappearedLogSubstring = "reappeared after" + const nodeUUIDMismatchLogSubstring = "does not match local product uuid" // --- Phase 1: NodeGone from the startup Node read --- // @@ -230,7 +247,7 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon nodeStore.PatchAnnotations(map[string]string{stormproxies.UpdateRequestAnnotation: ""}) nodeStore.RestoreNode() - if _, err := waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeReappearedLogSubstring, reappearedCount, 60*time.Second, journalSince); err != nil { + if reappearedCount, err = waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeReappearedLogSubstring, reappearedCount, 60*time.Second, journalSince); err != nil { return fmt.Errorf("phase 2: agent did not log resuming after the Node reappeared: %w", err) } if err := assertServiceMainPIDUnchanged(vmConfig.VMConfig, vmIP, aclAgentService, pid, 15*time.Second); err != nil { @@ -244,6 +261,107 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon return fmt.Errorf("phase 2: post-recovery validate-connection check failed: %w", err) } + // --- Phase 3: NodeGone from a systemUUID mismatch on the startup Node read --- + // + // Unlike phases 1-2 (the Node is literally absent, a 404), here the GET + // itself succeeds - the Node object comes back, but its + // status.nodeInfo.systemUUID doesn't match this VM's own + // /sys/class/dmi/id/product_uuid (see NodeClient::get_node and + // verify_node_identity in crates/trident-acl-agent/src/annotations/ + // k8s.rs). This proves that mismatch is (a) logged distinctly from a + // plain 404, and (b) still funneled into the exact same NodeGone / + // await_node_recreation path as phases 1-2, rather than being acted on + // or crashing the agent. Reuses phase 1's restart-based trigger to + // exercise the explicit-GET path (get_node_with_retry at startup). + // Phase 4 below covers the long-lived watch stream's identical check. + uuidCheckEnv := map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME": testConfig.NodeName, + "TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID": "true", + } + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, uuidCheckEnv); err != nil { + return fmt.Errorf("phase 3: matching-UUID GET check failed: %w", err) + } + nodeStore.SetSystemUUID("00000000-0000-0000-0000-000000000000") + // This process performs a GET, not a watch. Its own diagnostics must prove + // the UUID mismatch, so daemon journal messages cannot mask a missing check. + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", false, uuidCheckEnv, + nodeUUIDMismatchLogSubstring, "node object no longer exists"); err != nil { + return fmt.Errorf("phase 3: mismatched-UUID GET check failed: %w", err) + } + logrus.Info("phase 3: isolated GET rejected the mismatched systemUUID") + if _, err := stormssh.SshCommandCombinedOutput(vmConfig.VMConfig, vmIP, fmt.Sprintf("sudo systemctl restart %s", aclAgentService)); err != nil { + return fmt.Errorf("phase 3: failed to restart %s: %w", aclAgentService, err) + } + + mismatchCount, err := waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeUUIDMismatchLogSubstring, 0, 30*time.Second, journalSince) + if err != nil { + return fmt.Errorf("phase 3: agent did not log the systemUUID mismatch: %w", err) + } + logrus.Infof("phase 3: observed %d systemUUID mismatch log line(s)", mismatchCount) + if nodeGoneCount, err = waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeGoneLogSubstring, nodeGoneCount, 30*time.Second, journalSince); err != nil { + return fmt.Errorf("phase 3: agent did not treat the systemUUID mismatch as the node-gone resilience path: %w", err) + } + + pid, err = readServiceMainPID(vmConfig.VMConfig, vmIP, aclAgentService) + if err != nil { + return fmt.Errorf("phase 3: failed to read MainPID while the Node's systemUUID is mismatched: %w", err) + } + if err := assertServiceMainPIDUnchanged(vmConfig.VMConfig, vmIP, aclAgentService, pid, 20*time.Second); err != nil { + return fmt.Errorf("phase 3: %s did not survive the systemUUID mismatch: %w", aclAgentService, err) + } + + // Restore the real systemUUID and confirm the agent notices and + // resumes, exactly as phase 1 does after RestoreNode. + nodeStore.SetSystemUUID(productUUID) + if reappearedCount, err = waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeReappearedLogSubstring, reappearedCount, 60*time.Second, journalSince); err != nil { + return fmt.Errorf("phase 3: agent did not log resuming after the systemUUID started matching again: %w", err) + } + if err := assertServiceMainPIDUnchanged(vmConfig.VMConfig, vmIP, aclAgentService, pid, 15*time.Second); err != nil { + return fmt.Errorf("phase 3: %s did not remain stable after the systemUUID started matching again: %w", aclAgentService, err) + } + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, uuidCheckEnv); err != nil { + return fmt.Errorf("phase 3: post-recovery validate-connection check failed: %w", err) + } + + // --- Phase 4: NodeGone from a systemUUID mismatch delivered via the watch stream, no restart --- + // + // Same underlying check as phase 3 (verify_node_identity), but this + // time triggered purely through the already-open watch connection + // (NodeClient::watch_node in k8s.rs), with no agent restart at all - + // mirroring how phase 2 proves the PATCH-triggered NodeGone path + // without a restart. SetSystemUUID broadcasts a MODIFIED event to + // every active watcher (see its doc comment in proxies/apiserver.go), + // which the agent's long-lived watch picks up directly; reusing phase + // 3's pid baseline (captured after its one restart, used unchanged + // since) means a stable PID here proves this mismatch was caught by + // the existing watch loop, not by some other restart this test isn't + // aware of. + nodeStore.SetSystemUUID("11111111-1111-1111-1111-111111111111") + + if mismatchCount, err = waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeUUIDMismatchLogSubstring, mismatchCount, 30*time.Second, journalSince); err != nil { + return fmt.Errorf("phase 4: agent did not log the systemUUID mismatch delivered via the watch stream: %w", err) + } + logrus.Infof("phase 4: observed %d systemUUID mismatch log line(s)", mismatchCount) + if nodeGoneCount, err = waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeGoneLogSubstring, nodeGoneCount, 30*time.Second, journalSince); err != nil { + return fmt.Errorf("phase 4: agent did not treat the watch-delivered systemUUID mismatch as the node-gone resilience path: %w", err) + } + if err := assertServiceMainPIDUnchanged(vmConfig.VMConfig, vmIP, aclAgentService, pid, 20*time.Second); err != nil { + return fmt.Errorf("phase 4: %s did not survive the watch-delivered systemUUID mismatch without a restart: %w", aclAgentService, err) + } + + // Restore the real systemUUID; recovery GET polling detects the fix + // after the mismatched event ended the watch, without restarting the agent. + nodeStore.SetSystemUUID(productUUID) + if reappearedCount, err = waitForJournalOccurrenceCountAbove(vmConfig.VMConfig, vmIP, aclAgentService, nodeReappearedLogSubstring, reappearedCount, 60*time.Second, journalSince); err != nil { + return fmt.Errorf("phase 4: agent did not log resuming after recovery GET detected the matching systemUUID: %w", err) + } + if err := assertServiceMainPIDUnchanged(vmConfig.VMConfig, vmIP, aclAgentService, pid, 15*time.Second); err != nil { + return fmt.Errorf("phase 4: %s did not remain stable after the systemUUID started matching again: %w", aclAgentService, err) + } + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, nil); err != nil { + return fmt.Errorf("phase 4: post-recovery validate-connection check failed: %w", err) + } + return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) } diff --git a/tools/storm/aclagent/tests/rollback.go b/tools/storm/aclagent/tests/rollback.go index 8e884a44c0..de17c108fc 100644 --- a/tools/storm/aclagent/tests/rollback.go +++ b/tools/storm/aclagent/tests/rollback.go @@ -35,6 +35,11 @@ func RunRollback(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.Al ctx, cancel := context.WithCancel(context.Background()) defer cancel() + productUUID, err := readVmProductUUID(vmConfig.VMConfig, vmIP) + if err != nil { + return err + } + // Rollback doesn't stage a new image from Nebraska - it re-activates the // previously-finalized volume trident already has on disk - so only the // fake apiserver is needed here, not the Nebraska/image-server mocks @@ -42,7 +47,7 @@ func RunRollback(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.Al // all (see prepareVmForAclAgent); rollback's PatchSteps leave // `server`/`appId` unset too, since Nebraska is never queried during a // rollback request. - nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{})) + nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{}, productUUID)) apiServer := stormproxies.NewAPIServer(testConfig.NodeName, nodeStore) _, apiServerStop, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("%s:%d", testConfig.HostEndpointIP, testConfig.APIServerPort)) if err != nil { diff --git a/tools/storm/aclagent/tests/update.go b/tools/storm/aclagent/tests/update.go index 8735425fc1..637ea4c9fe 100644 --- a/tools/storm/aclagent/tests/update.go +++ b/tools/storm/aclagent/tests/update.go @@ -122,8 +122,9 @@ func sha384CosiMetadata(path string) (string, error) { // defaults; after prepareVmForAclAgent delivers it, kubernetes succeeds // too. Nebraska has no static config at all, so proving it can succeed // requires passing envVars explicitly (see the env-var-override checks in -// RunABUpdate). -func expectValidateConnection(cfg stormvmconfig.VMConfig, vmIP, mode string, wantSuccess bool, envVars map[string]string) error { +// RunABUpdate). expectedOutput optionally asserts diagnostics from this +// invocation, independently of the background service's journal. +func expectValidateConnection(cfg stormvmconfig.VMConfig, vmIP, mode string, wantSuccess bool, envVars map[string]string, expectedOutput ...string) error { var prefix strings.Builder prefix.WriteString("sudo") if len(envVars) > 0 { @@ -138,6 +139,16 @@ func expectValidateConnection(cfg stormvmconfig.VMConfig, vmIP, mode string, wan if gotSuccess != wantSuccess { return fmt.Errorf("--validate-connection %s (envVars=%v): expected success=%v, got success=%v (err=%v, output=%s)", mode, envVars, wantSuccess, gotSuccess, err, out) } + // Failed SSH commands retain their combined output in the wrapped error. + diagnostics := out + if err != nil { + diagnostics += "\n" + err.Error() + } + for _, expected := range expectedOutput { + if !strings.Contains(diagnostics, expected) { + return fmt.Errorf("--validate-connection %s: expected diagnostic %q, got %s", mode, expected, diagnostics) + } + } return nil } @@ -152,10 +163,15 @@ func RunABUpdate(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.Al } } + productUUID, err := readVmProductUUID(vmConfig.VMConfig, vmIP) + if err != nil { + return err + } + ctx, cancel := context.WithCancel(context.Background()) defer cancel() - nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{})) + nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{}, productUUID)) apiServer := stormproxies.NewAPIServer(testConfig.NodeName, nodeStore) // Bind on HostEndpointIP (not 127.0.0.1) so the VM can reach the fake // apiserver directly over the libvirt NAT network, instead of relying diff --git a/tools/storm/aclagent/tests/vm.go b/tools/storm/aclagent/tests/vm.go index de8a911feb..4b44564b79 100644 --- a/tools/storm/aclagent/tests/vm.go +++ b/tools/storm/aclagent/tests/vm.go @@ -2,8 +2,10 @@ package tests import ( "fmt" + "strings" stormaclconfig "tridenttools/storm/aclagent/utils/config" + stormssh "tridenttools/storm/utils/ssh" stormvm "tridenttools/storm/utils/vm" stormvmconfig "tridenttools/storm/utils/vm/config" @@ -45,3 +47,20 @@ func CleanupVM(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllV } return nil } + +// readVmProductUUID reads the VM's hardware product UUID +// (/sys/class/dmi/id/product_uuid). Every fake Node seeded for +// trident-acl-agent (stormproxies.NewSeedNode) must carry this exact value +// as its status.nodeInfo.systemUUID: when TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID +// is set, trident-acl-agent's NodeClient::get_node/watch_node +// (crates/trident-acl-agent/src/annotations/k8s.rs) compare the two and +// treat a non-empty mismatch as the Node not existing. An empty +// systemUUID is NOT treated as a mismatch (verification is skipped), so +// only a WRONG systemUUID here would make the agent never find its Node. +func readVmProductUUID(cfg stormvmconfig.VMConfig, vmIP string) (string, error) { + out, err := stormssh.SshCommandCombinedOutput(cfg, vmIP, "sudo cat /sys/class/dmi/id/product_uuid") + if err != nil { + return "", fmt.Errorf("failed to read VM product uuid: %w", err) + } + return strings.TrimSpace(out), nil +}