acl-agent: make node_name source (hostname vs kubelet cert CN) configurable by bfjelds · Pull Request #839 · microsoft/trident

bfjelds (bfjelds) · 2026-10-09T16:26:16Z

Why

Allow deployments to derive the Node name from the kubelet client certificate instead of relying on the hostname convention.

Changes

Add TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE: hostname (default) or kubelet-cert. An explicit NODE_NAME override takes precedence.
Derive the certificate identity from its system:node:<name> CN and system:nodes organization.
Retry certificate resolution in the orchestrator with cancellation-aware capped exponential backoff. Never fall back to hostname. The one-shot --validate-connection kubernetes diagnostic fails immediately on resolution errors.
Add parsing, precedence and retry unit tests, plus Storm checks using phase-specific API servers and exact resolved-name assertions.

Validation at `777fb02e`

206 ACL-agent unit tests passed. CI build 1221392 succeeded with Storm 8/8; the retry loop is covered by unit tests, not the one-shot Storm checks.

Follow-up

The kubelet certificate path remains fixed at /var/lib/kubelet/pki/kubelet-client-current.pem.

…urable AKS does not guarantee the lowercased-hostname convention trident-acl-agent currently assumes for node_name (per the AKS team). Add TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE (hostname | kubelet-cert) to opt into deriving node_name from the Subject CN (system:node:<name>) of kubelet own client certificate (/var/lib/kubelet/pki/kubelet-client-current.pem) instead - the identity kubelet itself authenticates to the API server with, so it is guaranteed to match the real Node name. Default stays hostname for backward compatibility. kubelet-cert falls back to hostname (with a warning) if the cert cannot be read or parsed, rather than failing startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

azure-pipelines · 2026-10-09T16:26:35Z

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

Adds run-node-name-source to the storm-trident aclagent scenario, proving TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE actually switches node_name resolution between hostname and kubelet-cert. Uses one-shot \ trident-acl-agent --validate-connection kubernetes\ CLI invocations against a self-signed fake kubelet client cert rather than restarting trident-acl-agent.service, so the check stays fast: no service restart, no reboot. Seeds the single-node fake apiserver with the cert Subject CNs node name (distinct from the VMs real hostname), so resolving to the wrong source always 404s - the two sources cannot accidentally agree. Covers: kubelet-cert resolves correctly, hostname/unset resolves to the (non-matching) real hostname and fails, an explicit NODE_NAME override still wins regardless of source, and a missing/unreadable cert degrades kubelet-cert to the hostname behavior with a logged warning instead of erroring out some other way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bfjelds (bfjelds) · 2026-10-09T19:49:54Z

Added to the scenario: proves actually switches node_name resolution between bfjelds-xubuntu and , via one-shot CLI invocations against a self-signed fake kubelet client cert - no service restart or reboot needed, so it stays fast relative to the rest of the scenario.\n\nCovers: kubelet-cert resolves correctly; hostname/unset resolves to the VMs real hostname and fails against this test ' s single-node fake apiserver; an explicit override still wins regardless of source; a missing/unreadable cert degrades kubelet-cert to the hostname behavior with a logged warning rather than erroring out some other way.\n\nVerified: (unchanged), and clean for .

bfjelds (bfjelds) · 2026-10-09T19:50:08Z

Added run-node-name-source to the storm-trident aclagent scenario: proves TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE actually switches node_name resolution between hostname and kubelet-cert, via one-shot --validate-connection kubernetes CLI invocations against a self-signed fake kubelet client cert - no service restart or reboot needed, so it stays fast relative to the rest of the scenario.

Covers: kubelet-cert resolves correctly; hostname/unset resolves to the VM's real hostname and fails against this test's single-node fake apiserver; an explicit NODE_NAME override still wins regardless of source; a missing/unreadable cert degrades kubelet-cert to the hostname behavior with a logged warning rather than erroring out some other way.

Verified: cargo build/fmt/clippy -p trident-acl-agent (unchanged), go build ./... and gofmt -l clean for tools/.

…warning stormssh.SshCommandCombinedOutput discards the captured remote output on a non-zero exit (innerSshCommand returns (\ \, err)); the actual stdout/stderr only survives inside the wrapped errors own message. Check err.Error() instead of the (always-empty-on-error) output string. Verified end-to-end on bfjelds-xubuntu: full storm-trident aclagent scenario now passes 8/8, including run-node-name-source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bfjelds (bfjelds) · 2026-10-09T20:24:01Z

Rebuilt binaries/RPMs/VM images and ran the full storm-trident aclagent scenario end-to-end on bfjelds-xubuntu to validate the new test:

=== SUMMARY of storm-trident::scenario::aclagent ===
  deploy-vm...........: PASS
  check-deployment....: PASS
  run-node-resilience.: PASS
  run-node-name-source: PASS
  run-ab-update.......: PASS
  run-rollback........: PASS
  collect-logs........: PASS
  cleanup-vm..........: PASS
=== RESULT ===
OK: passed: 8; total: 8

Along the way, fixed a bug in run-node-name-source's case 4 (missing-cert fallback check): stormssh.SshCommandCombinedOutput discards the captured remote output on a non-zero exit (innerSshCommand returns ("", err)) - the actual stdout/stderr only survives inside the wrapped error's own message. The test now checks err.Error() instead of the (always-empty-on-error) output string. First run caught this for real (failed with an empty "got:" message); fixed and reran clean.

…nd exact-value assertions Replaces the single shared fake apiserver (one node, mixed pass/fail-based assertions) with three sequential phases, each its own single-node apiserver seeded with exactly the identity that phase is expected to resolve to: - kubelet-cert source -> the certs Subject CN - hostname source (explicit + default), and kubelet-cert with a missing/unreadable cert -> the VMs real hostname, read live via SSH rather than assumed equal to --node-name - explicit NODE_NAME override -> a third, unrelated literal Every check now asserts the *exact* Node name validate-connections own success output reports fetching (via a new expectValidateConnectionOutput helper), not just pass/fail - a phase can only pass by resolving to the value it claims, never by coincidentally matching another phases answer or by the absence of a negative case. Addresses feedback that the explicit-override check previously reused the same literal as the cert-derived name, and that the hostname checks should assert the VMs actual hostname rather than only prove source selection via expected failures. Verified end-to-end on bfjelds-xubuntu: full storm-trident aclagent scenario passes 8/8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bfjelds (bfjelds) · 2026-10-09T21:47:34Z

Addressed review feedback on run-node-name-source (two good catches: the explicit-override check reused the same node name as the cert-derived test, making its success ambiguous in logs; and the hostname checks only proved source-selection via expected failures rather than asserting the actual resolved value).

Redesigned into three sequential phases, each with its own single-node fake apiserver (the fake only ever serves one Node name at a time) seeded with exactly the identity that phase expects:

kubelet-cert source -> apiserver seeded with the cert's Subject CN
hostname source (explicit + default) -> apiserver seeded with the VM's real hostname, read live via ssh ... hostname rather than assumed equal to --node-name; a missing/unreadable cert's fallback is checked against this same phase, proving it degrades to the real hostname specifically
explicit NODE_NAME override -> apiserver seeded with a third, unrelated literal (node-name-source-hostname-override-node)

Every check now asserts the exact Node name --validate-connection's own success output reports fetching (fetched Node "<name>"), via a new expectValidateConnectionOutput helper - not just pass/fail. A phase can only pass by resolving to the value it specifically claims.

Verified end-to-end on bfjelds-xubuntu again: full storm-trident aclagent scenario, 8/8 PASS.

bfjelds (bfjelds) · 2026-10-09T22:26:48Z

CI validation: build 1221319 on [GITHUB]-trident-pr-e2e, commit e566aae9 — SUCCEEDED, full storm-trident aclagent scenario 8/8 PASS.

run-node-name-source confirmed doing exactly what's intended, straight from the log:

Phase	Env vars	Resolved to
kubelet-cert	`NODE_NAME_SOURCE=kubelet-cert`	`"node-name-source-test-cert-node"`
hostname	`NODE_NAME_SOURCE=hostname`	`"trident-acl-agent-testimg"` (VM's real hostname)
missing-cert fallback	cert hidden, `NODE_NAME_SOURCE=kubelet-cert`	`WARN ... falling back to hostname` → `"trident-acl-agent-testimg"`
explicit override	`NODE_NAME=node-name-source-hostname-override-node`, `SOURCE=hostname`	`"node-name-source-hostname-override-node"`

All three distinct literals resolved correctly and independently, matching the per-phase apiserver design. No further changes needed.

…ostname Per feedback: if TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE=kubelet-cert is explicitly requested and the cert cant be read/parsed, treat it as a fatal config error - not a silent degrade to hostname. An operator who opted into kubelet-cert did so because hostname is not a trustworthy Node identity on their platform; silently falling back to it on cert failure risks the agent reconciling against the wrong Node entirely, same reasoning the agent already applies to its own Node disappearing (NodeGone is fatal, not papered over). - default_node_name() is now fallible (Result<String, Error>); AgentConfig::from_vars propagates kubelet-cert resolution failure as a wrapped failed to determine node_name config error, the same severity as any other malformed/unsatisfiable TRIDENT_ACL_AGENT_* setting. - impl Default for KubernetesConfig calls hostname_node_name() directly (NodeNameSource::default() is always Hostname, which cant fail), avoiding a fake unwrap on a Default impl that must be infallible. - Updated storm run-node-name-source test case: missing/unreadable cert with kubelet-cert source now asserts a fatal failed to determine node_name error (via err.Error(), since stormssh.SshCommandCombinedOutput discards output on non-zero exit) instead of a successful fallback-to-hostname resolution. - Updated docs (Trident-ACL-Agent.md, storm README, TridentAclAgent-Tests.md) to describe fail-hard behavior instead of the old fallback. Verified end-to-end on bfjelds-xubuntu: cargo test/fmt/clippy clean (16 config tests pass), full storm-trident aclagent scenario 8/8 PASS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bfjelds (bfjelds) · 2026-10-09T22:56:08Z

Addressed feedback: kubelet-cert source failing to read/parse the cert should not fall back to hostname — it should be treated like a "node not found" condition and fail hard instead.

Changed:

default_node_name() is now fallible (Result<String, Error>); AgentConfig::from_vars propagates a kubelet-cert resolution failure as a fatal failed to determine node_name config error — same severity as any other malformed/unsatisfiable TRIDENT_ACL_AGENT_* setting, no silent degrade to a possibly-wrong identity.
impl Default for KubernetesConfig now calls hostname_node_name() directly (NodeNameSource::default() is always Hostname, which can't fail), since Default::default() itself must stay infallible.
run-node-name-source's missing-cert check now asserts this fatal error (via the error text, since stormssh.SshCommandCombinedOutput discards output on a non-zero exit) instead of a successful hostname fallback.
Updated Trident-ACL-Agent.md, the storm README, and TridentAclAgent-Tests.md to describe the fail-hard behavior.

Verified on bfjelds-xubuntu: cargo test/fmt/clippy -p trident-acl-agent clean (16 config tests), full storm-trident aclagent scenario 8/8 PASS with the new behavior exercised end-to-end.

… handling Per feedback: a missing/unreadable kubelet-cert at startup should be treated the same way as the agents own Node object not existing yet - retried with capped exponential backoff, not a fatal process crash. - KubernetesConfig.node_name renamed to node_name_override (Option<String>, explicit override only); node_name is no longer eagerly resolved during AgentConfig::from_vars, which is infallible again for this field. - New KubernetesConfig::resolve_node_name_once(): single, fail-fast resolution (override, else node_name_source). Used directly by --validate-connection kubernetes (connection_check.rs), where fail-fast remains correct for a one-shot diagnostic. - Orchestrator gains its own resolved node_name: String field and Orchestrator::resolve_node_name_with_retry(), which calls resolve_node_name_once() in a loop with the same NODE_READ_BACKOFF/NODE_READ_BACKOFF_MAX capped-exponential-backoff policy as get_node_with_retry, racing every sleep against shutdown - exactly the same treatment already given to a Node 404 (NodeGone). Orchestrator::from_config now takes &CancellationToken and returns Result<Option<Self>, Error> (None on shutdown during retry), mirroring recover_from_trident_state/await_node_recreations own Ok(None)/Ok(false) early-exit shape. - main.rs updated for the new from_config signature/return type. - New Rust unit tests in orchestrator.rs prove the retry loop actually retries (Ok(None) on a cancelled shutdown, not an immediate Err) for KubeletCert, and resolves without any retry for Hostname/explicit override. - Updated run-node-name-source storm test: missing-cert phase now checks --validate-connections fail-fast error text (failed to resolve node_name) instead of the old fatal-config-error text: deliberately does NOT attempt to prove the long-running retry loop from Go/storm, since that needs a controllable cert path (out of scope here) to deny-then-grant without flaking on real timing. - Updated docs (Trident-ACL-Agent.md, storm README, TridentAclAgent-Tests.md) to describe the retry behavior. Verified end-to-end on bfjelds-xubuntu: cargo test -p trident-acl-agent (206 tests, incl. 3 new retry-behavior tests) + fmt + clippy all clean; full storm-trident aclagent scenario 8/8 PASS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bfjelds (bfjelds) · 2026-10-09T23:43:10Z

To directly answer the question: previously, no — exponential backoff retry was NOT happening. A missing/unreadable kubelet-cert cert made AgentConfig::from_vars return Err at config-load time in main.rs (AgentConfig::from_env()?), which exits the process immediately - before the orchestrator, get_node_with_retry, or await_node_recreation (the existing NodeGone retry machinery) ever runs. systemd's Restart=on-failure/fixed RestartSec=5 would then crash-loop at a constant interval, not exponential backoff.

Fixed: an unreadable kubelet-cert at startup is now treated the same way the agent already treats its own Node object not existing yet (NodeGone) - a plausible, expected startup race (kubelet's own TLS bootstrapping not finished yet) to retry and wait out, not a fatal crash.

Changes:

KubernetesConfig.node_name → node_name_override: Option<String> (explicit override only). node_name is no longer eagerly resolved during config loading, which is infallible again for this field.
New KubernetesConfig::resolve_node_name_once(): a single, fail-fast resolution (override, else node_name_source). Used directly by --validate-connection kubernetes, where fail-fast remains correct for a one-shot diagnostic.
Orchestrator gains its own resolved node_name: String field and resolve_node_name_with_retry(), which calls resolve_node_name_once() in a loop using the same NODE_READ_BACKOFF/NODE_READ_BACKOFF_MAX capped-exponential-backoff policy as get_node_with_retry - racing every sleep against shutdown, exactly like the existing NodeGone handling.
Orchestrator::from_config now takes &CancellationToken and returns Result<Option<Self>, Error> (None on shutdown during retry), mirroring recover_from_trident_state/await_node_recreation's own early-exit shape.
New Rust unit tests prove the retry loop actually retries (Ok(None) on a cancelled shutdown, not an immediate Err) for kubelet-cert, and resolves without any retry for hostname/explicit override.
Updated run-node-name-source's missing-cert phase to check --validate-connection's fail-fast error text (failed to resolve node_name) - the long-running retry loop itself is covered by the new Rust unit tests instead of the Go/storm scenario, since proving an indefinite retry needs a controllable cert path (deliberately out of scope here per the earlier discussion) to flip from missing→present without flaking on real timing.
Docs updated (Trident-ACL-Agent.md, storm README, TridentAclAgent-Tests.md).

Verified on bfjelds-xubuntu: cargo test -p trident-acl-agent (206 tests, including 3 new retry-behavior tests) + fmt/clippy all clean; full storm-trident aclagent scenario 8/8 PASS.

bfjelds (bfjelds) · 2026-10-10T00:36:27Z

CI validation: build 1221392 on [GITHUB]-trident-pr-e2e, commit 777fb02e (the retry-with-backoff change) — SUCCEEDED.

Full storm-trident aclagent scenario: 8/8 PASS.

=== RESULT ===
OK: passed: 8; total: 8

run-node-name-source passed in ~5s, confirming --validate-connection kubernetes's fail-fast behavior is unchanged after moving node_name resolution out of eager config-loading (KubernetesConfig::resolve_node_name_once) — no regression from the refactor. As expected, this VM scenario doesn't exercise the new Orchestrator::resolve_node_name_with_retry backoff loop itself; that's covered by the 3 new Rust unit tests in this same commit (cargo test -p trident-acl-agent), run separately in CI's build/test stage.

Copilot

🟡 Changes recommended

The description contradicts the final behavior, and several tests depend on the host lacking a readable kubelet certificate.

2 open findings

What changed in this PR

Adds configurable ACL-agent Kubernetes node-name resolution using either the hostname or kubelet certificate CN.

Changes:

Adds node-name source configuration and certificate parsing.
Adds retry behavior and connection-validation handling.
Adds unit, Storm integration, and documentation coverage.

File	Description
`tools/storm/aclagent/trident.go`	Registers the new Storm test.
`tools/storm/aclagent/tests/node_name_source.go`	Tests node-name source behavior end to end.
`tools/storm/aclagent/README.md`	Documents the Storm test.
`docs/Explanation/Trident-ACL-Agent.md`	Documents the environment variable.
`docs/Development/Testing/TridentAclAgent-Tests.md`	Updates scenario documentation.
`crates/trident-acl-agent/src/main.rs`	Handles shutdown during resolution.
`crates/trident-acl-agent/src/core/config.rs`	Implements source configuration and certificate parsing.
`crates/trident-acl-agent/src/connection_check.rs`	Resolves node names for diagnostics.
`crates/trident-acl-agent/src/annotations/orchestrator.rs`	Adds retrying node-name resolution.
`crates/trident-acl-agent/Cargo.toml`	Adds the OpenSSL dependency.
`Cargo.lock`	Records the dependency update.

🧠 Review effort: Balanced

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

+    fn default_node_name_errors_when_cert_source_requested_but_unreadable() {
+        // DEFAULT_KUBELET_CLIENT_CERT won't exist on a dev/test machine, so
+        // requesting KubeletCert must error out - no silent fallback to
+        // Hostname (see NodeNameSource::KubeletCert's doc comment for why).
+        let err = default_node_name(NodeNameSource::KubeletCert).unwrap_err();


Copilot

🟡 Changes recommended

Certificate validation and Storm test isolation contain unresolved correctness issues.

5 open findings

🧠 Review effort: Balanced

+    cn.strip_prefix(KUBELET_CERT_CN_NODE_PREFIX)
+        .map(ToString::to_string)
+        .ok_or_else(|| {
+            anyhow!(
+                "certificate at {cert_path:?} has Subject CN {cn:?}, \
+                 which doesn't start with {KUBELET_CERT_CN_NODE_PREFIX:?}"
+            )
+        })
+}


+	// needs to happen once, up front. trident-acl-agent.service itself is
+	// never touched by this test, since every check is a direct one-shot
+	// CLI invocation.
+	if err := prepareVmForAclAgent(vmConfig.VMConfig, vmIP, testConfig, nil); err != nil {


bfjelds (bfjelds) requested a balanced review from Copilot October 10, 2026 00:42

Copilot started reviewing on behalf of bfjelds (bfjelds) October 10, 2026 00:43 View session

Copilot AI reviewed Oct 10, 2026

View reviewed changes

bfjelds (bfjelds) requested a balanced review from Copilot October 10, 2026 15:20

Copilot started reviewing on behalf of bfjelds (bfjelds) October 10, 2026 15:21 View session

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Navigation Menu

Repository navigation

acl-agent: make node_name source (hostname vs kubelet cert CN) configurable - #839

acl-agent: make node_name source (hostname vs kubelet cert CN) configurable#839
bfjelds (bfjelds) wants to merge 6 commits into
mainfrom
user/bfjelds/acl-agent-node-name-source

bfjelds (bfjelds) commented Oct 9, 2026 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bfjelds (bfjelds) commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Changes

Validation at 777fb02e

Follow-up

Uh oh!

azure-pipelines Bot commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 9, 2026

Uh oh!

bfjelds (bfjelds) commented Oct 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bfjelds (bfjelds) commented Oct 9, 2026 •

edited

Loading

Validation at `777fb02e`