From 45c2a78247721a2903506de201b4a9fb483c070a Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 16:54:30 +0000 Subject: [PATCH 01/13] trident-acl-agent: verify Node systemUUID matches local hardware NodeClient::get_node now compares the fetched Node's status.nodeInfo.systemUUID against this host's own hardware identity (/sys/class/dmi/id/product_uuid, the same source kubelet uses to populate systemUUID). On mismatch, logs a warning with both UUIDs and returns K8sClientError::NodeGone, so the agent treats a misidentified Node the same as a missing one (retry/await recreation) instead of acting on a Node object that does not describe this machine. If the local product UUID cannot be read, verification is skipped (logged at warn) rather than failing closed. --- .../trident-acl-agent/src/annotations/k8s.rs | 129 +++++++++++++++++- 1 file changed, 127 insertions(+), 2 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index 86c794d26..b732622ed 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -15,7 +15,7 @@ //! after a dropped or failed watch; that is governed entirely by //! `kube::runtime::watcher`'s built-in `default_backoff()`. -use std::{collections::BTreeMap, time::Duration}; +use std::{collections::BTreeMap, path::Path, time::Duration}; use anyhow::{Context, Error}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; @@ -30,10 +30,13 @@ use kube::{ }, Api, Client, Config, Error as KubeError, }; +use log::warn; use reqwest::StatusCode; use serde_json::json; use thiserror::Error; +use osutils::files::read_file_trim; + use crate::core::config::KubernetesConfig; /// Floor for the Kubernetes watch request's `timeoutSeconds`, decoupled from @@ -42,6 +45,15 @@ use crate::core::config::KubernetesConfig; /// reconnect churn on an otherwise-healthy watch. const WATCH_TIMEOUT_SECS: u32 = 290; +/// Path to the hardware product UUID exposed by the kernel. kubelet +/// populates a Node's `status.nodeInfo.systemUUID` from this same file, so +/// on a healthy, correctly-identified node the two values always match. +/// A mismatch means the Node object fetched from the API server does not +/// actually describe the machine this agent is running on (e.g. a stale or +/// recycled Node name), so it must be treated the same as the Node not +/// existing at all. +const PRODUCT_UUID_PATH: &str = "/sys/class/dmi/id/product_uuid"; + #[derive(Debug, Error)] pub enum K8sClientError { #[error("failed to build Kubernetes client config: {0}")] @@ -78,7 +90,9 @@ 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)?; + verify_node_identity(&node, name, Path::new(PRODUCT_UUID_PATH))?; + Ok(node) } pub async fn patch_node_labels( @@ -161,6 +175,49 @@ 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 (read from `product_uuid_path`, +/// normally [`PRODUCT_UUID_PATH`]). 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. +fn verify_node_identity( + node: &Node, + name: &str, + product_uuid_path: &Path, +) -> Result<(), K8sClientError> { + let local_uuid = match read_file_trim(&product_uuid_path) { + Ok(uuid) => uuid, + Err(err) => { + warn!( + "failed to read local product uuid from {}, skipping Node {name:?} identity verification: {err:#}", + product_uuid_path.display() + ); + 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.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 +330,72 @@ 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() + } + } + + fn write_uuid_file(uuid: &str) -> tempfile::NamedTempFile { + use std::io::Write; + + let mut file = tempfile::NamedTempFile::new().expect("failed to create temp file"); + writeln!(file, "{uuid}").expect("failed to write temp file"); + file + } + + #[test] + fn matching_system_uuid_is_ok() { + let uuid_file = write_uuid_file("1234-ABCD"); + let node = node_with_system_uuid("1234-ABCD"); + + assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + } + + #[test] + fn matching_system_uuid_is_case_insensitive() { + let uuid_file = write_uuid_file("1234-abcd"); + let node = node_with_system_uuid("1234-ABCD"); + + assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + } + + #[test] + fn mismatched_system_uuid_is_node_gone() { + let uuid_file = write_uuid_file("1234-ABCD"); + let node = node_with_system_uuid("5678-EFGH"); + + assert!(matches!( + verify_node_identity(&node, "n", uuid_file.path()), + Err(K8sClientError::NodeGone) + )); + } + + #[test] + fn missing_node_system_info_is_node_gone() { + let uuid_file = write_uuid_file("1234-ABCD"); + let node = Node::default(); + + assert!(matches!( + verify_node_identity(&node, "n", uuid_file.path()), + Err(K8sClientError::NodeGone) + )); + } + + #[test] + fn unreadable_local_uuid_skips_verification() { + let node = node_with_system_uuid("5678-EFGH"); + + assert!(verify_node_identity(&node, "n", Path::new("/does/not/exist")).is_ok()); + } } From 17e942f5bae9e1c40bd7924af814d7071169fce2 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 17:59:15 +0000 Subject: [PATCH 02/13] storm aclagent tests: seed fake Node with real VM product uuid NodeClient's new systemUUID verification (previous commit) treats any Node whose status.nodeInfo.systemUUID doesn't match the local /sys/class/dmi/id/product_uuid as not found. The storm E2E harness's fake apiserver never set systemUUID on its seeded Node, so every get_node call against it would now return NodeGone and run-ab-update, run-rollback, and run-node-resilience would all fail immediately. Fix: NewSeedNode takes the real systemUUID to seed onto the fake Node, and each of the three call sites (RunABUpdate, RunRollback, RunNodeResilience) reads it from the VM over SSH (/sys/class/dmi/id/product_uuid) before constructing the node store. --- tools/storm/aclagent/proxies/apiserver.go | 15 ++++++++++++++- tools/storm/aclagent/proxies/apiserver_test.go | 2 +- tools/storm/aclagent/tests/node_resilience.go | 7 ++++++- tools/storm/aclagent/tests/rollback.go | 7 ++++++- tools/storm/aclagent/tests/update.go | 7 ++++++- tools/storm/aclagent/tests/vm.go | 17 +++++++++++++++++ 6 files changed, 50 insertions(+), 5 deletions(-) diff --git a/tools/storm/aclagent/proxies/apiserver.go b/tools/storm/aclagent/proxies/apiserver.go index 56d8924db..9733076e0 100644 --- a/tools/storm/aclagent/proxies/apiserver.go +++ b/tools/storm/aclagent/proxies/apiserver.go @@ -39,7 +39,15 @@ 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) - trident-acl-agent's NodeClient::get_node +// (crates/trident-acl-agent/src/annotations/k8s.rs) compares a fetched +// Node's status.nodeInfo.systemUUID against that local file and treats any +// mismatch (including an empty systemUUID) the same as the Node not existing +// at all, so leaving this unset here would make every get_node call against +// the fake apiserver fail as NodeGone. +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 +55,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 diff --git a/tools/storm/aclagent/proxies/apiserver_test.go b/tools/storm/aclagent/proxies/apiserver_test.go index c084c8e44..d34de80c4 100644 --- a/tools/storm/aclagent/proxies/apiserver_test.go +++ b/tools/storm/aclagent/proxies/apiserver_test.go @@ -12,7 +12,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) diff --git a/tools/storm/aclagent/tests/node_resilience.go b/tools/storm/aclagent/tests/node_resilience.go index 7347761db..ef0c6aa5b 100644 --- a/tools/storm/aclagent/tests/node_resilience.go +++ b/tools/storm/aclagent/tests/node_resilience.go @@ -63,6 +63,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 +75,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 { diff --git a/tools/storm/aclagent/tests/rollback.go b/tools/storm/aclagent/tests/rollback.go index 8e884a44c..de17c108f 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 8735425fc..4a0ec33e6 100644 --- a/tools/storm/aclagent/tests/update.go +++ b/tools/storm/aclagent/tests/update.go @@ -152,10 +152,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 de8a911fe..deee44e52 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,18 @@ 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: trident-acl-agent's +// NodeClient::get_node (crates/trident-acl-agent/src/annotations/k8s.rs) +// compares the two and treats any mismatch as the Node not existing, so an +// unset or 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, "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 +} From 48d816496ee50947389c551ca013d6f5fb5aae76 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 18:04:31 +0000 Subject: [PATCH 03/13] storm aclagent tests: cover systemUUID-mismatch node-gone handling Adds phase 3 to run-node-resilience: sets the fake Node's status.nodeInfo.systemUUID to a value that does not match the VM's real /sys/class/dmi/id/product_uuid, restarts trident-acl-agent, and confirms the startup Node read (a) logs the mismatch distinctly from a plain 404 and (b) still funnels into the same NodeGone / await_node_recreation path as phases 1-2 (no crash, same PID, resumes once the systemUUID matches again). NodeStore gains SetSystemUUID for this, mirroring SetReadyCondition. New fast unit tests in apiserver_test.go cover the fake apiserver seeding/serving systemUUID correctly, independent of the full VM harness. --- tools/storm/aclagent/proxies/apiserver.go | 17 ++++++ .../storm/aclagent/proxies/apiserver_test.go | 51 ++++++++++++++++ tools/storm/aclagent/tests/node_resilience.go | 60 ++++++++++++++++++- 3 files changed, 127 insertions(+), 1 deletion(-) diff --git a/tools/storm/aclagent/proxies/apiserver.go b/tools/storm/aclagent/proxies/apiserver.go index 9733076e0..e9179051b 100644 --- a/tools/storm/aclagent/proxies/apiserver.go +++ b/tools/storm/aclagent/proxies/apiserver.go @@ -216,6 +216,23 @@ 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 in +// crates/trident-acl-agent/src/annotations/k8s.rs): setting a value that +// doesn't match the VM's real /sys/class/dmi/id/product_uuid makes the next +// get_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 d34de80c4..898968b18 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 @@ -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 ef0c6aa5b..7d50be544 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,13 @@ 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. func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllVMConfig) error { vmIP, err := stormvm.GetVmIP(vmConfig) if err != nil { @@ -104,6 +113,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 --- // @@ -235,7 +245,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 { @@ -249,6 +259,54 @@ 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 since + // the mismatch can only be observed on an explicit GET + // (get_node_with_retry at startup), not on the long-lived watch. + nodeStore.SetSystemUUID("00000000-0000-0000-0000-000000000000") + 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, nil); err != nil { + return fmt.Errorf("phase 3: post-recovery validate-connection check failed: %w", err) + } + return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) } From 7db8410ad1635972a434b2742a66a3a973d34c1e Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 18:27:35 +0000 Subject: [PATCH 04/13] trident-acl-agent: skip systemUUID check when Node reports empty value cadvisor (which kubelet uses to populate status.nodeInfo.systemUUID) reads /sys/class/dmi/id/product_uuid directly, same as our local check. But if cadvisor fails to read that file on the kubelet side, it logs an error and still lets node registration proceed with an empty systemUUID rather than failing - an empty value is evidence kubelet could not determine the UUID, not evidence the Node describes a different machine. Treat it the same as an unreadable local file: skip verification instead of treating it as a mismatch, to avoid a false positive that would otherwise lock us out of a healthy node forever. --- .../trident-acl-agent/src/annotations/k8s.rs | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index b732622ed..eeefa2120 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -185,6 +185,15 @@ fn is_not_found(err: &KubeError) -> bool { /// 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, @@ -208,6 +217,13 @@ fn verify_node_identity( .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 !node_uuid.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" @@ -382,14 +398,19 @@ mod tests { } #[test] - fn missing_node_system_info_is_node_gone() { + fn missing_node_system_info_skips_verification() { let uuid_file = write_uuid_file("1234-ABCD"); let node = Node::default(); - assert!(matches!( - verify_node_identity(&node, "n", uuid_file.path()), - Err(K8sClientError::NodeGone) - )); + assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + } + + #[test] + fn empty_node_system_uuid_skips_verification() { + let uuid_file = write_uuid_file("1234-ABCD"); + let node = node_with_system_uuid(""); + + assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); } #[test] From aaf1289b78664f4df11c7d3acf3a0a18c9bc815a Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 18:40:35 +0000 Subject: [PATCH 05/13] storm aclagent tests: read VM product_uuid with sudo /sys/class/dmi/id/product_uuid is root-only (0400). readVmProductUUID ran a plain cat over SSH as the non-root test user, which failed with Permission denied and aborted run-node-resilience before it could even start (build 1220454: \cat: /sys/class/dmi/id/product_uuid: Permission denied\). Every other privileged command in this harness is already prefixed with sudo; this one was missed. --- tools/storm/aclagent/tests/vm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/storm/aclagent/tests/vm.go b/tools/storm/aclagent/tests/vm.go index deee44e52..351bbe744 100644 --- a/tools/storm/aclagent/tests/vm.go +++ b/tools/storm/aclagent/tests/vm.go @@ -56,7 +56,7 @@ func CleanupVM(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllV // compares the two and treats any mismatch as the Node not existing, so an // unset or 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, "cat /sys/class/dmi/id/product_uuid") + 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) } From eea50792ae1e7cd5f1cfe11858a54a41241033ba Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 18:59:37 +0000 Subject: [PATCH 06/13] trident-acl-agent: fix rustfmt formatting cargo fmt --check failed in CI (build 1220474, Check amd64 job) on the single-line warn! added in the previous commit - rustfmt collapses it onto one line rather than wrapping it. --- crates/trident-acl-agent/src/annotations/k8s.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index eeefa2120..4f5994114 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -218,9 +218,7 @@ fn verify_node_identity( .unwrap_or_default(); if node_uuid.is_empty() { - warn!( - "Node {name:?} status.nodeInfo.systemUUID is empty, skipping identity verification" - ); + warn!("Node {name:?} status.nodeInfo.systemUUID is empty, skipping identity verification"); return Ok(()); } From 44d2bd9b70d0b27d5e962ec5979649dfc5f89a14 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 19:10:13 +0000 Subject: [PATCH 07/13] trident-acl-agent: gate Node systemUUID verification behind an env var Make the systemUUID-vs-local-product_uuid check opt-in via TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID (presence-only, same convention as osutils::container::DOCKER_ENVIRONMENT). Disabled by default: the assumption that kubelet systemUUID always traces back to this machines own product_uuid is well-supported in general but has not been confirmed across every environment this agent runs in, so it should be validated and opted into per-deployment rather than enabled unconditionally. Enabled it for the storm aclagent E2E scenario via the trident-acl-agent.service override.conf drop-in baked into both the base and update VM images, so run-node-resilience Phase 3 (and the general NodeGone handling) continues to exercise this path. Documented the new env var and behavior in docs/Explanation/Trident-ACL-Agent.md. --- .../trident-acl-agent/src/annotations/k8s.rs | 15 +++++++++++- docs/Explanation/Trident-ACL-Agent.md | 23 +++++++++++++++++++ .../files/trident-acl-agent-override.conf | 1 + 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index 4f5994114..66d939a75 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -54,6 +54,17 @@ const WATCH_TIMEOUT_SECS: u32 = 290; /// existing at all. const PRODUCT_UUID_PATH: &str = "/sys/class/dmi/id/product_uuid"; +/// Environment variable that enables [`verify_node_identity`]'s +/// `systemUUID`-vs-local-hardware check in [`NodeClient::get_node`]. Value is +/// not important - only presence is checked, matching +/// `osutils::container::DOCKER_ENVIRONMENT`'s convention. Unset by default: +/// the check's assumptions (kubelet's `systemUUID` always traces back to this +/// same machine's `product_uuid`) hold for the environments this was +/// validated against, but haven'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 const ENV_VALIDATE_NODE_UUID: &str = "TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID"; + #[derive(Debug, Error)] pub enum K8sClientError { #[error("failed to build Kubernetes client config: {0}")] @@ -91,7 +102,9 @@ impl NodeClient { pub async fn get_node(&self, name: &str) -> Result { let node = self.api.get(name).await.map_err(map_kube_error)?; - verify_node_identity(&node, name, Path::new(PRODUCT_UUID_PATH))?; + if std::env::var(ENV_VALIDATE_NODE_UUID).is_ok() { + verify_node_identity(&node, name, Path::new(PRODUCT_UUID_PATH))?; + } Ok(node) } diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 6777d721b..8f6c763e1 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_VALIDATE_NODE_UUID` | unset (disabled) | Presence-only flag (value is ignored). When set, 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,28 @@ 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_VALIDATE_NODE_UUID` is set, every explicit Node GET +(the startup/recovery read and `--validate-connection`; the long-lived +watch stream is not covered) 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 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 952966198..00ed7b9e6 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_VALIDATE_NODE_UUID=1 From 02c7bf30a7a2ae1fe7f6cb2ec9016af51d8d9ffc Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 21:03:20 +0000 Subject: [PATCH 08/13] acl-agent: cover watch_node identity check, dedupe UUID probe, fix comments Addresses review feedback on PR #837: - NodeClient::watch_node now applies verify_node_identity to each Node delivered by the watch stream (gated by the same TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID opt-in), matching get_node. Previously only explicit GETs were covered, so a misidentified Node delivered via the long-lived watch was never caught. - Extracted the duplicated DMI product_uuid path + read-and-fallback logic (previously redefined separately in k8s.rs and tracestream.rs) into a shared osutils::dmi module, consumed by both. - Fixed stale comments in the storm test harness (apiserver.go, vm.go) that claimed an empty systemUUID is treated as a mismatch and did not mention the verification is opt-in; both now match actual behavior (empty is skipped, non-empty mismatch -> NodeGone, gated by the env var). - Updated docs/Explanation/Trident-ACL-Agent.md and the PR description to state the watch stream is now covered, since that limitation (truthfully documented before this change) no longer applies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osutils/src/dmi.rs | 49 +++++++++++++++++++ crates/osutils/src/lib.rs | 1 + .../trident-acl-agent/src/annotations/k8s.rs | 30 +++++++----- crates/trident/src/logging/tracestream.rs | 30 ++++++------ docs/Explanation/Trident-ACL-Agent.md | 6 +-- tools/storm/aclagent/proxies/apiserver.go | 27 +++++----- tools/storm/aclagent/tests/vm.go | 10 ++-- 7 files changed, 107 insertions(+), 46 deletions(-) create mode 100644 crates/osutils/src/dmi.rs diff --git a/crates/osutils/src/dmi.rs b/crates/osutils/src/dmi.rs new file mode 100644 index 000000000..a172019a6 --- /dev/null +++ b/crates/osutils/src/dmi.rs @@ -0,0 +1,49 @@ +//! 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 crate::files::read_file_trim; + +/// 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_product_uuid_from(PRODUCT_UUID_PATH) +} + +/// Reads and trims the hardware product UUID from an arbitrary path. Exposed +/// (rather than only the [`PRODUCT_UUID_PATH`]-bound [`read_product_uuid`]) +/// so callers can inject a fake path in unit tests. +pub fn read_product_uuid_from(path: impl AsRef) -> Result { + read_file_trim(&path.as_ref()).with_context(|| { + format!( + "Failed to read product UUID from '{}'", + path.as_ref().display() + ) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reads_and_trims_product_uuid() { + let dir = tempfile::tempdir().expect("failed to create temp dir"); + let path = dir.path().join("product_uuid"); + std::fs::write(&path, "1234-ABCD\n").expect("failed to write test file"); + + assert_eq!(read_product_uuid_from(&path).unwrap(), "1234-ABCD"); + } + + #[test] + fn errors_for_missing_file() { + assert!(read_product_uuid_from("/nonexistent/product_uuid-for-test").is_err()); + } +} diff --git a/crates/osutils/src/lib.rs b/crates/osutils/src/lib.rs index 48a28b9d4..6efde3b84 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 66d939a75..3d7b48545 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -35,7 +35,7 @@ use reqwest::StatusCode; use serde_json::json; use thiserror::Error; -use osutils::files::read_file_trim; +use osutils::{dmi::PRODUCT_UUID_PATH, files::read_file_trim}; use crate::core::config::KubernetesConfig; @@ -45,17 +45,9 @@ use crate::core::config::KubernetesConfig; /// reconnect churn on an otherwise-healthy watch. const WATCH_TIMEOUT_SECS: u32 = 290; -/// Path to the hardware product UUID exposed by the kernel. kubelet -/// populates a Node's `status.nodeInfo.systemUUID` from this same file, so -/// on a healthy, correctly-identified node the two values always match. -/// A mismatch means the Node object fetched from the API server does not -/// actually describe the machine this agent is running on (e.g. a stale or -/// recycled Node name), so it must be treated the same as the Node not -/// existing at all. -const PRODUCT_UUID_PATH: &str = "/sys/class/dmi/id/product_uuid"; - /// Environment variable that enables [`verify_node_identity`]'s -/// `systemUUID`-vs-local-hardware check in [`NodeClient::get_node`]. Value is +/// `systemUUID`-vs-local-hardware check in [`NodeClient::get_node`] and +/// [`NodeClient::watch_node`]. Value is /// not important - only presence is checked, matching /// `osutils::container::DOCKER_ENVIRONMENT`'s convention. Unset by default: /// the check's assumptions (kubelet's `systemUUID` always traces back to this @@ -170,10 +162,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 = std::env::var(ENV_VALIDATE_NODE_UUID).is_ok(); + 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, Path::new(PRODUCT_UUID_PATH))?; + } + Ok(node) + } + }) .boxed() } } diff --git a/crates/trident/src/logging/tracestream.rs b/crates/trident/src/logging/tracestream.rs index 132696833..0b564a1b4 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,6 +19,7 @@ use tracing::{ use tracing_subscriber::{layer::Layer, registry::LookupSpan}; use osutils::{ + dmi::read_product_uuid_from, files, osrelease::{OsRelease, OS_RELEASE_PATH}, uname, @@ -26,8 +27,6 @@ use osutils::{ 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 read_product_uuid(filepath: &str) -> String { + read_product_uuid_from(filepath).unwrap_or_else(|err| { + debug!("Failed to read product uuid from {filepath}: {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!(read_product_uuid(osutils::dmi::PRODUCT_UUID_PATH)), ); platform_info.insert("os_release".to_string(), json!(get_os_release())); platform_info.insert("total_cpu".to_string(), json!(sys.cpus().len())); @@ -463,7 +461,7 @@ mod tests { #[test] fn test_read_product_uuid_unknown() { - let uuid = read_product_uuid("unknown".to_string()); + let uuid = read_product_uuid("/nonexistent/product_uuid-for-test"); assert_eq!(uuid, "unknown"); } @@ -473,7 +471,7 @@ mod tests { 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 = read_product_uuid(filepath.to_str().unwrap()); assert_eq!(uuid, "test_uuid"); } } @@ -536,7 +534,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!(read_product_uuid(osutils::dmi::PRODUCT_UUID_PATH)), ); 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/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 8f6c763e1..d3328f687 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -297,9 +297,9 @@ fire instead) can opt into a bound via ### Node identity verification -When `TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID` is set, every explicit Node GET -(the startup/recovery read and `--validate-connection`; the long-lived -watch stream is not covered) additionally checks that the fetched Node's +When `TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID` is set, 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 diff --git a/tools/storm/aclagent/proxies/apiserver.go b/tools/storm/aclagent/proxies/apiserver.go index e9179051b..2d3d72b7f 100644 --- a/tools/storm/aclagent/proxies/apiserver.go +++ b/tools/storm/aclagent/proxies/apiserver.go @@ -41,12 +41,14 @@ type NodeStore struct { // 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) - trident-acl-agent's NodeClient::get_node -// (crates/trident-acl-agent/src/annotations/k8s.rs) compares a fetched -// Node's status.nodeInfo.systemUUID against that local file and treats any -// mismatch (including an empty systemUUID) the same as the Node not existing -// at all, so leaving this unset here would make every get_node call against -// the fake apiserver fail as NodeGone. +// (/sys/class/dmi/id/product_uuid) - when TRIDENT_ACL_AGENT_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"}, @@ -218,12 +220,13 @@ func (s *NodeStore) SetReadyCondition(ready bool) *corev1.Node { // SetSystemUUID overwrites the Node's status.nodeInfo.systemUUID. Used by // run-node-resilience to exercise trident-acl-agent's systemUUID -// verification (NodeClient::get_node in -// crates/trident-acl-agent/src/annotations/k8s.rs): setting a value that -// doesn't match the VM's real /sys/class/dmi/id/product_uuid makes the next -// get_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. +// verification (NodeClient::get_node/watch_node in +// crates/trident-acl-agent/src/annotations/k8s.rs): when +// TRIDENT_ACL_AGENT_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() diff --git a/tools/storm/aclagent/tests/vm.go b/tools/storm/aclagent/tests/vm.go index 351bbe744..3c68d1bd4 100644 --- a/tools/storm/aclagent/tests/vm.go +++ b/tools/storm/aclagent/tests/vm.go @@ -51,10 +51,12 @@ func CleanupVM(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllV // 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: trident-acl-agent's -// NodeClient::get_node (crates/trident-acl-agent/src/annotations/k8s.rs) -// compares the two and treats any mismatch as the Node not existing, so an -// unset or wrong systemUUID here would make the agent never find its Node. +// as its status.nodeInfo.systemUUID: when TRIDENT_ACL_AGENT_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 { From 9882e27ce6788000a727572fd1de98bce479ad3d Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 21:11:08 +0000 Subject: [PATCH 09/13] acl-agent: use shared dmi reader in verify_node_identity, fix stale e2e comments Follow-up to the previous commit on this PR, from a fresh review pass: - verify_node_identity still called osutils::files::read_file_trim directly instead of the newly-added osutils::dmi::read_product_uuid_from, so it was not actually using the shared DMI helper it was supposed to deduplicate onto. Switched it over (read_product_uuid_from is already generic over any path, so this is a drop-in replacement). - Fixed stale comments in tools/storm/aclagent/tests/node_resilience.go that claimed a systemUUID mismatch can only be observed on an explicit GET, never on the long-lived watch - no longer true now that NodeClient::watch_node also verifies identity. Clarified that the watch-stream path is a real, but currently test-uncovered, code path rather than an implementation limitation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/annotations/k8s.rs | 4 ++-- tools/storm/aclagent/tests/node_resilience.go | 20 +++++++++++++++---- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index 3d7b48545..3c197e358 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -35,7 +35,7 @@ use reqwest::StatusCode; use serde_json::json; use thiserror::Error; -use osutils::{dmi::PRODUCT_UUID_PATH, files::read_file_trim}; +use osutils::dmi::{read_product_uuid_from, PRODUCT_UUID_PATH}; use crate::core::config::KubernetesConfig; @@ -220,7 +220,7 @@ fn verify_node_identity( name: &str, product_uuid_path: &Path, ) -> Result<(), K8sClientError> { - let local_uuid = match read_file_trim(&product_uuid_path) { + let local_uuid = match read_product_uuid_from(product_uuid_path) { Ok(uuid) => uuid, Err(err) => { warn!( diff --git a/tools/storm/aclagent/tests/node_resilience.go b/tools/storm/aclagent/tests/node_resilience.go index 7d50be544..1d8cfcb4a 100644 --- a/tools/storm/aclagent/tests/node_resilience.go +++ b/tools/storm/aclagent/tests/node_resilience.go @@ -57,7 +57,10 @@ const aclAgentService = "trident-acl-agent.service" // 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. +// Node read phase 1 exercises. NodeClient::watch_node performs the +// identical check on each Node delivered by the long-lived watch stream, +// but that path is NOT exercised by this test - see the note on phase 3 +// below for why. func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllVMConfig) error { vmIP, err := stormvm.GetVmIP(vmConfig) if err != nil { @@ -269,9 +272,18 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon // 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 since - // the mismatch can only be observed on an explicit GET - // (get_node_with_retry at startup), not on the long-lived watch. + // or crashing the agent. Reuses phase 1's restart-based trigger to + // exercise the explicit-GET path (get_node_with_retry at startup). + // + // NodeClient::watch_node applies the identical verify_node_identity + // check to each Node delivered by the long-lived watch stream (see + // k8s.rs), so the same mismatch handling should also be reachable there + // without an agent restart - but that path is NOT exercised by this + // test: it would require this fake apiserver to deliver a MODIFIED + // watch event with a mismatched systemUUID while the agent is already + // watching, which SetSystemUUID's broadcast (see its doc comment in + // proxies/apiserver.go) is capable of triggering, but doing so has not + // been validated end-to-end here yet. nodeStore.SetSystemUUID("00000000-0000-0000-0000-000000000000") 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) From 973dd395981a3d80402b76fc2e55b129dce5a198 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 21:49:15 +0000 Subject: [PATCH 10/13] acl-agent: add e2e phase exercising watch-stream identity verification Addresses the remaining open item on PR #837: RunNodeResilience now has a Phase 4 that triggers a systemUUID mismatch via NodeStore.SetSystemUUID with the agent already running and watching - no restart - proving NodeClient::watch_node catches it via verify_node_identity exactly like phase 3 proves for the explicit-GET path, and that the same NodeGone/recovery handling applies with a provably stable MainPID throughout (no restart happened). Validated end-to-end against a live VM: rebuilt trident-acl RPM from this branch (v9882e27c), rebuilt trident-vm-acl-agent-testimage.qcow2 via Image Customizer (the prior artifact predated TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID being baked into the image config and could not have exercised phase 3 or 4 at all), and ran the full aclagent storm suite: deploy-vm..........: PASS check-deployment...: PASS run-node-resilience: PASS (all 4 phases, including the new one) run-ab-update......: PASS run-rollback.......: PASS collect-logs.......: PASS cleanup-vm.........: PASS Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/storm/aclagent/tests/node_resilience.go | 57 ++++++++++++++----- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/tools/storm/aclagent/tests/node_resilience.go b/tools/storm/aclagent/tests/node_resilience.go index 1d8cfcb4a..90e944910 100644 --- a/tools/storm/aclagent/tests/node_resilience.go +++ b/tools/storm/aclagent/tests/node_resilience.go @@ -57,10 +57,9 @@ const aclAgentService = "trident-acl-agent.service" // 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. NodeClient::watch_node performs the -// identical check on each Node delivered by the long-lived watch stream, -// but that path is NOT exercised by this test - see the note on phase 3 -// below for why. +// 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 { @@ -274,16 +273,7 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon // 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). - // - // NodeClient::watch_node applies the identical verify_node_identity - // check to each Node delivered by the long-lived watch stream (see - // k8s.rs), so the same mismatch handling should also be reachable there - // without an agent restart - but that path is NOT exercised by this - // test: it would require this fake apiserver to deliver a MODIFIED - // watch event with a mismatched systemUUID while the agent is already - // watching, which SetSystemUUID's broadcast (see its doc comment in - // proxies/apiserver.go) is capable of triggering, but doing so has not - // been validated end-to-end here yet. + // Phase 4 below covers the long-lived watch stream's identical check. nodeStore.SetSystemUUID("00000000-0000-0000-0000-000000000000") 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) @@ -319,6 +309,45 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon 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; the watch stream should deliver the fix + // the same way it delivered the mismatch, with no restart either. + 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 the watch-delivered systemUUID started matching again: %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) } From 7dc03048e30117078a17e8b4eb06b3f82cc06fb7 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 8 Oct 2026 23:04:35 +0000 Subject: [PATCH 11/13] acl-agent: migrate node-identity env var to the envy config system Addresses review feedback on PR #837: TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID was implemented as a raw std::env::var(...).is_ok() presence check in k8s.rs, bypassing the envy-based AgentConfig/KubernetesConfig system every other trident-acl-agent setting goes through. Two consequences: - Non-Unicode safety: std::env::var() returns Err for a present value that isn't valid UTF-8, silently disabling verification despite the documented "presence-only, value ignored" contract. - The var's name didn't follow the established TRIDENT_ACL_AGENT_
_ convention (no section segment), because there was no existing boolean-flag precedent in this config system to follow in the first place. Renamed to TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID and added a real `validate_node_uuid: bool` field to KubernetesConfig/ RawKubernetesConfig, loaded through the same envy::prefixed(...) mechanism as every other Kubernetes setting (explicit "true"/"false", default false, empty value falls back to the default like every other field here). NodeClient now stores this as a plain bool at construction time instead of re-reading the environment on every get_node/watch_node call. Updated the baked-in systemd override.conf, docs, and storm e2e harness comments to the new name. Validated end-to-end against a live VM: rebuilt the trident-acl RPM and trident-vm-acl-agent-testimage.qcow2 with this change, and re-ran the full aclagent storm suite - all 7 test cases pass, including run-node-resilience's systemUUID mismatch phases (3 and 4), now driven by the new config field instead of the old env var. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../trident-acl-agent/src/annotations/k8s.rs | 21 +++---- crates/trident-acl-agent/src/core/config.rs | 58 +++++++++++++++++++ docs/Explanation/Trident-ACL-Agent.md | 14 +++-- .../files/trident-acl-agent-override.conf | 2 +- tools/storm/aclagent/proxies/apiserver.go | 4 +- tools/storm/aclagent/tests/vm.go | 2 +- 6 files changed, 77 insertions(+), 24 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index 3c197e358..8d9f9ce4c 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -45,18 +45,6 @@ use crate::core::config::KubernetesConfig; /// reconnect churn on an otherwise-healthy watch. const WATCH_TIMEOUT_SECS: u32 = 290; -/// Environment variable that enables [`verify_node_identity`]'s -/// `systemUUID`-vs-local-hardware check in [`NodeClient::get_node`] and -/// [`NodeClient::watch_node`]. Value is -/// not important - only presence is checked, matching -/// `osutils::container::DOCKER_ENVIRONMENT`'s convention. Unset by default: -/// the check's assumptions (kubelet's `systemUUID` always traces back to this -/// same machine's `product_uuid`) hold for the environments this was -/// validated against, but haven'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 const ENV_VALIDATE_NODE_UUID: &str = "TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID"; - #[derive(Debug, Error)] pub enum K8sClientError { #[error("failed to build Kubernetes client config: {0}")] @@ -74,6 +62,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 { @@ -85,6 +77,7 @@ impl NodeClient { api: Api::all(client), poll_interval: config.watch_poll_interval, cluster_url, + validate_identity: config.validate_node_uuid, }) } @@ -94,7 +87,7 @@ impl NodeClient { pub async fn get_node(&self, name: &str) -> Result { let node = self.api.get(name).await.map_err(map_kube_error)?; - if std::env::var(ENV_VALIDATE_NODE_UUID).is_ok() { + if self.validate_identity { verify_node_identity(&node, name, Path::new(PRODUCT_UUID_PATH))?; } Ok(node) @@ -167,7 +160,7 @@ impl NodeClient { // 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 = std::env::var(ENV_VALIDATE_NODE_UUID).is_ok(); + let validate_identity = self.validate_identity; watcher::watcher(self.api.clone(), watcher_config) .default_backoff() diff --git a/crates/trident-acl-agent/src/core/config.rs b/crates/trident-acl-agent/src/core/config.rs index 2f25af930..d66e34987 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/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index d3328f687..afb844829 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -267,7 +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_VALIDATE_NODE_UUID` | unset (disabled) | Presence-only flag (value is ignored). When set, 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. | +| `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 @@ -297,7 +297,8 @@ fire instead) can opt into a bound via ### Node identity verification -When `TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID` is set, every Node read - the +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 @@ -311,10 +312,11 @@ 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 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. +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 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 00ed7b9e6..0bc4624f7 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,4 +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_VALIDATE_NODE_UUID=1 +Environment=TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID=true diff --git a/tools/storm/aclagent/proxies/apiserver.go b/tools/storm/aclagent/proxies/apiserver.go index 2d3d72b7f..b12b73e1e 100644 --- a/tools/storm/aclagent/proxies/apiserver.go +++ b/tools/storm/aclagent/proxies/apiserver.go @@ -41,7 +41,7 @@ type NodeStore struct { // 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_VALIDATE_NODE_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 @@ -222,7 +222,7 @@ func (s *NodeStore) SetReadyCondition(ready bool) *corev1.Node { // 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_VALIDATE_NODE_UUID is set, setting a non-empty value +// 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 diff --git a/tools/storm/aclagent/tests/vm.go b/tools/storm/aclagent/tests/vm.go index 3c68d1bd4..4b44564b7 100644 --- a/tools/storm/aclagent/tests/vm.go +++ b/tools/storm/aclagent/tests/vm.go @@ -51,7 +51,7 @@ func CleanupVM(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllV // 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_VALIDATE_NODE_UUID +// 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 From eda6e7df74678b2170942e1286e2fa2578b7a73d Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 10 Oct 2026 17:01:51 +0000 Subject: [PATCH 12/13] fix: centralize typed DMI UUID reads and handle missing identity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d61c42e7-3c43-4fe5-8c89-89d38ff9528b --- crates/osutils/src/dmi.rs | 103 +++++++++--------- .../trident-acl-agent/src/annotations/k8s.rs | 83 +++++++------- crates/trident/src/logging/tracestream.rs | 24 ++-- tools/storm/aclagent/tests/node_resilience.go | 6 +- 4 files changed, 109 insertions(+), 107 deletions(-) diff --git a/crates/osutils/src/dmi.rs b/crates/osutils/src/dmi.rs index a172019a6..a0598cf8a 100644 --- a/crates/osutils/src/dmi.rs +++ b/crates/osutils/src/dmi.rs @@ -1,49 +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 crate::files::read_file_trim; - -/// 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_product_uuid_from(PRODUCT_UUID_PATH) -} - -/// Reads and trims the hardware product UUID from an arbitrary path. Exposed -/// (rather than only the [`PRODUCT_UUID_PATH`]-bound [`read_product_uuid`]) -/// so callers can inject a fake path in unit tests. -pub fn read_product_uuid_from(path: impl AsRef) -> Result { - read_file_trim(&path.as_ref()).with_context(|| { - format!( - "Failed to read product UUID from '{}'", - path.as_ref().display() - ) - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn reads_and_trims_product_uuid() { - let dir = tempfile::tempdir().expect("failed to create temp dir"); - let path = dir.path().join("product_uuid"); - std::fs::write(&path, "1234-ABCD\n").expect("failed to write test file"); - - assert_eq!(read_product_uuid_from(&path).unwrap(), "1234-ABCD"); - } - - #[test] - fn errors_for_missing_file() { - assert!(read_product_uuid_from("/nonexistent/product_uuid-for-test").is_err()); - } -} +//! 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/trident-acl-agent/src/annotations/k8s.rs b/crates/trident-acl-agent/src/annotations/k8s.rs index 8d9f9ce4c..53210133e 100644 --- a/crates/trident-acl-agent/src/annotations/k8s.rs +++ b/crates/trident-acl-agent/src/annotations/k8s.rs @@ -15,7 +15,7 @@ //! after a dropped or failed watch; that is governed entirely by //! `kube::runtime::watcher`'s built-in `default_backoff()`. -use std::{collections::BTreeMap, path::Path, time::Duration}; +use std::{collections::BTreeMap, time::Duration}; use anyhow::{Context, Error}; use futures::{stream::BoxStream, StreamExt, TryStreamExt}; @@ -35,7 +35,8 @@ use reqwest::StatusCode; use serde_json::json; use thiserror::Error; -use osutils::dmi::{read_product_uuid_from, PRODUCT_UUID_PATH}; +use osutils::dmi; +use sysdefs::osuuid::OsUuid; use crate::core::config::KubernetesConfig; @@ -88,7 +89,7 @@ impl NodeClient { pub async fn get_node(&self, name: &str) -> Result { let node = self.api.get(name).await.map_err(map_kube_error)?; if self.validate_identity { - verify_node_identity(&node, name, Path::new(PRODUCT_UUID_PATH))?; + verify_node_identity(&node, name, dmi::read_product_uuid())?; } Ok(node) } @@ -170,7 +171,7 @@ impl NodeClient { let name = name.clone(); async move { if validate_identity { - verify_node_identity(&node, &name, Path::new(PRODUCT_UUID_PATH))?; + verify_node_identity(&node, &name, dmi::read_product_uuid())?; } Ok(node) } @@ -190,8 +191,8 @@ fn is_not_found(err: &KubeError) -> bool { } /// Confirms `node`'s reported `status.nodeInfo.systemUUID` matches this -/// machine's own hardware product UUID (read from `product_uuid_path`, -/// normally [`PRODUCT_UUID_PATH`]). A mismatch means the Node object we +/// 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`]). @@ -211,19 +212,24 @@ fn is_not_found(err: &KubeError) -> bool { fn verify_node_identity( node: &Node, name: &str, - product_uuid_path: &Path, + local_uuid: Result, ) -> Result<(), K8sClientError> { - let local_uuid = match read_product_uuid_from(product_uuid_path) { + let local_uuid = match local_uuid { Ok(uuid) => uuid, Err(err) => { warn!( - "failed to read local product uuid from {}, skipping Node {name:?} identity verification: {err:#}", - product_uuid_path.display() + "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() @@ -236,7 +242,10 @@ fn verify_node_identity( return Ok(()); } - if !node_uuid.eq_ignore_ascii_case(&local_uuid) { + 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" ); @@ -374,61 +383,55 @@ mod tests { } } - fn write_uuid_file(uuid: &str) -> tempfile::NamedTempFile { - use std::io::Write; - - let mut file = tempfile::NamedTempFile::new().expect("failed to create temp file"); - writeln!(file, "{uuid}").expect("failed to write temp file"); - file - } - #[test] fn matching_system_uuid_is_ok() { - let uuid_file = write_uuid_file("1234-ABCD"); let node = node_with_system_uuid("1234-ABCD"); - - assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + verify_node_identity(&node, "n", Ok(OsUuid::from("1234-ABCD"))).unwrap(); } #[test] fn matching_system_uuid_is_case_insensitive() { - let uuid_file = write_uuid_file("1234-abcd"); - let node = node_with_system_uuid("1234-ABCD"); - - assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + 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 uuid_file = write_uuid_file("1234-ABCD"); let node = node_with_system_uuid("5678-EFGH"); - - assert!(matches!( - verify_node_identity(&node, "n", uuid_file.path()), - Err(K8sClientError::NodeGone) - )); + 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() { - let uuid_file = write_uuid_file("1234-ABCD"); - let node = Node::default(); - - assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + verify_node_identity(&Node::default(), "n", Ok(OsUuid::from("1234-ABCD"))).unwrap(); } #[test] fn empty_node_system_uuid_skips_verification() { - let uuid_file = write_uuid_file("1234-ABCD"); let node = node_with_system_uuid(""); - - assert!(verify_node_identity(&node, "n", uuid_file.path()).is_ok()); + 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(); + } - assert!(verify_node_identity(&node, "n", Path::new("/does/not/exist")).is_ok()); + #[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/src/logging/tracestream.rs b/crates/trident/src/logging/tracestream.rs index 0b564a1b4..c7ffe05fc 100644 --- a/crates/trident/src/logging/tracestream.rs +++ b/crates/trident/src/logging/tracestream.rs @@ -19,11 +19,11 @@ use tracing::{ use tracing_subscriber::{layer::Layer, registry::LookupSpan}; use osutils::{ - dmi::read_product_uuid_from, - files, + dmi, files, osrelease::{OsRelease, OS_RELEASE_PATH}, uname, }; +use sysdefs::osuuid::OsUuid; use crate::{TRIDENT_METRICS_FILE_PATH, TRIDENT_VERSION}; @@ -351,9 +351,9 @@ where /// 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 read_product_uuid(filepath: &str) -> String { - read_product_uuid_from(filepath).unwrap_or_else(|err| { - debug!("Failed to read product uuid from {filepath}: {err:#}"); +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() }) } @@ -391,7 +391,7 @@ fn populate_platform_info() -> BTreeMap { sys.refresh_all(); platform_info.insert( "asset_id".to_string(), - json!(read_product_uuid(osutils::dmi::PRODUCT_UUID_PATH)), + 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())); @@ -415,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(); @@ -461,17 +459,13 @@ mod tests { #[test] fn test_read_product_uuid_unknown() { - let uuid = read_product_uuid("/nonexistent/product_uuid-for-test"); + 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()); + let uuid = product_uuid_or_unknown(Ok(OsUuid::from("test_uuid"))); assert_eq!(uuid, "test_uuid"); } } @@ -534,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(osutils::dmi::PRODUCT_UUID_PATH)), + 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/tools/storm/aclagent/tests/node_resilience.go b/tools/storm/aclagent/tests/node_resilience.go index 90e944910..e84eac19d 100644 --- a/tools/storm/aclagent/tests/node_resilience.go +++ b/tools/storm/aclagent/tests/node_resilience.go @@ -335,11 +335,11 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon return fmt.Errorf("phase 4: %s did not survive the watch-delivered systemUUID mismatch without a restart: %w", aclAgentService, err) } - // Restore the real systemUUID; the watch stream should deliver the fix - // the same way it delivered the mismatch, with no restart either. + // 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 the watch-delivered systemUUID started matching again: %w", err) + 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) From 0c112a4713655c955684aecc95ff18aed08d88a3 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Sat, 10 Oct 2026 18:16:56 +0000 Subject: [PATCH 13/13] test: isolate GET-side Node UUID validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d61c42e7-3c43-4fe5-8c89-89d38ff9528b --- .../Development/Testing/TridentAclAgent-Tests.md | 7 ++++++- tools/storm/aclagent/README.md | 6 +++++- tools/storm/aclagent/tests/node_resilience.go | 16 +++++++++++++++- tools/storm/aclagent/tests/update.go | 15 +++++++++++++-- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/docs/Development/Testing/TridentAclAgent-Tests.md b/docs/Development/Testing/TridentAclAgent-Tests.md index e11dae9e5..8d69c1593 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/tools/storm/aclagent/README.md b/tools/storm/aclagent/README.md index 58d010a0c..12cf51c80 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/tests/node_resilience.go b/tools/storm/aclagent/tests/node_resilience.go index e84eac19d..89885436d 100644 --- a/tools/storm/aclagent/tests/node_resilience.go +++ b/tools/storm/aclagent/tests/node_resilience.go @@ -274,7 +274,21 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon // 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) } @@ -305,7 +319,7 @@ func RunNodeResilience(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon 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, nil); err != nil { + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, uuidCheckEnv); err != nil { return fmt.Errorf("phase 3: post-recovery validate-connection check failed: %w", err) } diff --git a/tools/storm/aclagent/tests/update.go b/tools/storm/aclagent/tests/update.go index 4a0ec33e6..637ea4c9f 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 }