[bfjelds (bfjelds)]

Fixes trident-acl-agent so a stale/recycled Node name can't be mistaken for this machine's own Node object. Opt-in, disabled by default.

Change

• NodeClient::get_node and NodeClient::watch_node can verify the fetched Node's status.nodeInfo.systemUUID against this host's own /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 every caller - connection check, startup recovery, reconcile loop, and the long-lived watch stream - treats a misidentified Node exactly like a missing one.
• Verification is skipped (logged at warn), not treated as a mismatch, when the Node's systemUUID is empty or the local product_uuid can't be read - either case only means the UUID is undetermined, not that the Node describes a different machine.
• Gated behind TRIDENT_ACL_AGENT_KUBERNETES_VALIDATE_NODE_UUID, a real boolean config field (loaded via envy, like every other trident-acl-agent setting) that must be set to the literal string true to enable verification. Disabled (false) by default: the assumption that kubelet's systemUUID always traces back to this machine's product_uuid hasn't been confirmed across every environment this agent runs in, so it's opt-in per-deployment rather than unconditional.
• Extracted the DMI product_uuid path/read into a shared osutils::dmi helper, now reused by trident-acl-agent and trident's tracestream metadata instead of each defining its own copy of the path and read-with-fallback logic.
• Documented the config field and behavior (including watch-stream coverage) in docs/Explanation/Trident-ACL-Agent.md.

Testing

• https://dev.azure.com/mariner-org/ECF/_build/results?buildId=1220485&view=results
• Re-validated end-to-end against a live VM after the env-var-to-envy-config migration: rebuilt the trident-acl RPM and VM image, ran the full storm-trident run aclagent suite - 7/7 PASS, including run-node-resilience's systemUUID mismatch phases.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

Watched Node objects bypass identity verification, allowing reconciliation against recycled Nodes.

2 open findings

• Watch events bypass Node identity verification · New
• Move product UUID probing into osutils · New

What changed in this PR

Adds hardware UUID validation to prevent the ACL agent from accepting a stale Kubernetes Node object.

Changes:

• Compares Node and local DMI system UUIDs.
• Treats mismatches as NodeGone.
• Adds focused identity-verification tests.

File	Description
crates/​trident-acl-agent/​src/​annotations/​k8s.rs	Adds Node identity verification and tests.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Watch events bypass identity verification and can still trigger servicing for a same-name replacement Node.

2 open findings

• Watch events bypass Node identity verification
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Added run-node-resilience phase 3 (storm E2E) + fast Go unit tests covering the systemUUID-mismatch case end-to-end: sets a mismatched systemUUID on the fake Node, restarts trident-acl-agent, and confirms it (a) logs the mismatch distinctly from a plain 404 and (b) treats it the same as NodeGone — no crash, same PID, resumes once the systemUUID matches again. See tools/storm/aclagent/tests/node_resilience.go and proxies/apiserver_test.go.

[Copilot]

🔵 Needs a closer look

Long-lived watch events bypass identity verification and can still trigger operations for mismatched hardware.

2 open findings

• Watch events bypass Node identity verification
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Pushed a follow-up fix after investigating whether AKS nodes guarantee status.nodeInfo.systemUUID matches /sys/class/dmi/id/product_uuid:

Confirmed via cadvisor source that kubelet populates systemUUID by reading the exact same file we check against. However, if cadvisor fails to read that file on the kubelet side, it logs an error but still lets node registration proceed with an empty systemUUID rather than failing. An empty value only means kubelet could not determine the UUID - it is not evidence the Node describes a different machine.

Updated verify_node_identity to skip verification (warn + Ok) when the Node reports an empty systemUUID, same as when our local file is unreadable, instead of treating it as a mismatch. Without this, a rare kubelet-side DMI read failure could have falsely locked us out of an otherwise-healthy node forever.

Updated/added unit tests accordingly (missing_node_system_info_skips_verification, empty_node_system_uuid_skips_verification).

[Copilot]

🟡 Changes recommended

Watch-delivered Nodes still bypass identity verification and can trigger reconciliation for mismatched hardware.

3 open findings

• Watch events bypass Node identity verification
• Update empty systemUUID verification comments · New
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Fixed the storm run-node-resilience failure from build 1220454: readVmProductUUID ran a plain cat /sys/class/dmi/id/product_uuid over SSH as the non-root test user, which hit Permission denied (file is 0400 root-only) and aborted the whole aclagent scenario before any assertions ran. Every other privileged command in this harness already uses sudo; added it here too.

[Copilot]

🔵 Needs a closer look

Watch-delivered Nodes remain unverified and can still trigger servicing against recycled hardware.

3 open findings

• Watch events bypass Node identity verification
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Watch-delivered Nodes still bypass identity validation and can trigger reconciliation for recycled hardware.

3 open findings

• Watch events bypass Node identity verification
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[bfjelds (bfjelds)]

Made the systemUUID verification opt-in: added TRIDENT_ACL_AGENT_VALIDATE_NODE_UUID (presence-only env var, same convention as osutils::container::DOCKER_ENVIRONMENT). Disabled by default - kubelet sourcing systemUUID from the same product_uuid file is well-supported in general but not a confirmed guarantee across every environment this agent runs in, so it is opt-in per deployment rather than unconditional.

Enabled it for the storm aclagent E2E scenario via the existing trident-acl-agent.service override.conf drop-in (baked into both base and update VM images), so run-node-resilience Phase 3 and the general NodeGone handling keep exercising this path.

Documented the new env var and behavior in docs/Explanation/Trident-ACL-Agent.md.

[Copilot]

🟡 Changes recommended

Watch events bypass identity validation, allowing a replacement Node to reach reconciliation.

4 open findings

• Watch events bypass Node identity verification
• Enable Node identity verification by default · New
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Watched Node objects bypass UUID validation and can still trigger servicing operations.

4 open findings

• Watch events bypass Node identity verification
• Enable Node identity verification by default
• Update empty systemUUID verification comments
• Move product UUID probing into osutils

🧠 Review effort: Balanced

[Copilot]

🟢 Approval recommended

No unresolved blocking issues remain in the opt-in validation, shared readers, configuration, or test changes.

No open issues 🎉

_{Review effort: Balanced}

[bfjelds (bfjelds)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved the conflict, retaining both branch-specific settings alongside main's override. Merge commit: ff54168.

[Copilot]

🟢 Approval recommended

The opt-in checks cover GET and watch paths, preserve existing fallback and telemetry behavior, and leave no identified blocking issues.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

The opt-in checks consistently cover GET and watch reads, preserve fallback and telemetry behavior, and have no identified unresolved blocking issues.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

No unresolved blockers were identified in the opt-in identity checks, recovery integration, telemetry preservation, or associated tests.

No open issues 🎉

_{Review effort: Balanced}

[bfjelds (bfjelds)]

Copilot resolve the merge conflicts in this pull request

[Copilot]

🟢 Approval recommended

No unresolved blocking issues remain in the opt-in validation, recovery integration, shared UUID reader, or supporting tests.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

Copilot resolve the merge conflicts in this pull request

Resolved the conflicts against current main and committed the merge in 61a4f07. The commit preserves both branches’ config, documentation, and Storm scenario changes; validation passed for the ACL-agent Rust and Storm Go tests.

[Paco Huelsz (frhuelsz)]

LGTM, agent reviewed and brought up these non-blocking concerns:

Concern	Scenario and impact	Proposed resolution
Status PATCH race	A Node could be replaced after it was verified but before an in-progress or terminal status PATCH. The old VM could write stale status onto the replacement, but cannot service or reboot it; operation IDs also help the controller reject unrelated results.	Follow-up hardening: atomically test the verified Node UID when PATCHing and treat a failed test as NodeGone. UID is preferable to resourceVersion, which changes during normal Node updates.
Pending-commit recovery	If a Node name is reassigned while the old VM reboots—or the old VM later returns as a zombie—it can publish commit status before another verified read. The local commit must remain independent of Kubernetes to avoid firmware fallback.	Commit and persist locally first, then verify the Node before publishing. If identity does not match, retain the result instead of patching the replacement.
Empty Node UUID	An empty UUID means kubelet could not determine hardware identity; it does not prove a mismatch. Failing closed could permanently disable updates on an otherwise valid node.	Keep the documented warning-and-skip behavior, or add an explicit strict mode where an empty UUID is treated as NodeGone.

None of these should block this PR. The main request-processing race is fixed, and the remaining cases require a broader split-brain failure where infrastructure reuses a Node name without fencing the old VM. I recommend tracking conditional PATCH protection as follow-up hardening; the pending-recovery and empty-UUID behaviors are reasonable accepted tradeoffs.

Separate nit: git diff --check reports trailing whitespace in the testing documentation.