You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
acl-agent: make node_name source (hostname vs kubelet cert CN) configurable - #839
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.
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.
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 .
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/.
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.
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.
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.
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 sameNODE_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.
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.
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.
Issue: Certificate-derived identity does not enforce the required system:nodes organization. Evidence: This code reads only the first Subject CN, so a certificate with CN=system:node:<name> and a missing or different O is accepted even though the PR contract requires both fields. Suggestion: Reject certificates whose Subject has no system:nodes organization and add a negative test for a valid-looking CN with the wrong O.
Issue: This testing guide says no service restart is involved, but shared setup performs one unconditional restart before the phases. Evidence:RunNodeNameSource invokes prepareVmForAclAgent, whose command includes systemctl restart trident-acl-agent.service. Suggestion: Clarify that no additional restart or reboot occurs during the assertion phases.
Correct scenario description of shared service restart
tools/storm/aclagent/README.md:155
Issue: The scenario description incorrectly says the service is never touched or restarted. Evidence:RunNodeNameSource calls prepareVmForAclAgent, which unconditionally runs systemctl restart trident-acl-agent.service; only the individual assertion phases avoid further restarts. Suggestion: Document the one shared setup restart while retaining the no-per-phase-restart distinction.
Issue: The certificate source does not enforce the required system:nodes Subject organization. Evidence: The PR contract derives identity from both CN=system:node:<name> and O=system:nodes, but this code inspects only the CN, so any certificate with the prefix is accepted even if it is not a kubelet node credential. Suggestion: Require at least one Subject organization exactly equal to system:nodes before returning the CN suffix, and add a rejection test for a matching CN with the wrong/missing organization.
Correct test description to mention shared service restart
tools/storm/aclagent/README.md:155
Issue: This description still says the test never touches or restarts the service. Evidence:RunNodeNameSource calls prepareVmForAclAgent, which unconditionally runs systemctl restart trident-acl-agent.service (tools/storm/aclagent/tests/update.go:654-657); only the phase assertions themselves are independent one-shot CLI calls. Suggestion: State that shared setup restarts the service once, while no per-phase restart/reboot is needed, and make the same correction in the testing guide.
Issue: Certificate-derived identity ignores the required system:nodes organization. Evidence: This parser inspects only COMMONNAME, so any certificate with a system:node:<name> CN is accepted even when it is not a kubelet node certificate; both the PR contract and linked #839 require the CN andO=system:nodes. Suggestion: Require the subject to contain an exact system:nodes organization before returning the CN suffix, and add regression coverage for missing/wrong organizations.
Correct documentation about service restart during node name tests
tools/storm/aclagent/README.md:155
Issue: This remaining description still says the test never touches or restarts the service. Evidence:RunNodeNameSource calls prepareVmForAclAgent, which unconditionally runs systemctl restart trident-acl-agent.service (tools/storm/aclagent/tests/update.go:654-657); the new Go comments correctly acknowledge that one shared-setup restart. The same inaccurate claim remains in docs/Development/Testing/TridentAclAgent-Tests.md:275-277. Suggestion: State that setup restarts the service once, while phase assertions are standalone CLI calls requiring no additional restart or reboot.
…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>
Copilot resolve the merge conflicts in this pull request
Merged main and resolved the main.rs conflict, preserving cancellation-aware node-name resolution and upstream shutdown logging. Merge commit: 36cb7d3f.
the effective config log does not include kubernetes.node_name_source or kubernetes.kubelet_cert_path. if resolution fails, we can see that no explicit override was set, but not whether it tried hostname vs certificate or which certificate path it used. please include both fields in redacted_for_logging().
retrying is good when the kubelet certificate is temporarily missing during boot. my concern is that a wrong path, corrupt certificate, or invalid CN can never recover, but the agent still retries forever. systemd then reports the service as running even though it never connects to Kubernetes, watches its Node, or processes updates. should permanent resolution failures eventually fail the service or expose a readiness signal?
the effective config log does not include kubernetes.node_name_source or kubernetes.kubelet_cert_path. if resolution fails, we can see that no explicit override was set, but not whether it tried hostname vs certificate or which certificate path it used. please include both fields in redacted_for_logging().
Great comment. Added kubernetes.node_name_source and kubernetes.kubelet_cert_path to redacted_for_logging() in b7640d9. The startup log now shows the configured source and certificate path without reading or logging certificate contents. Regression coverage checks both defaults and environment overrides.
retrying is good when the kubelet certificate is temporarily missing during boot. my concern is that a wrong path, corrupt certificate, or invalid CN can never recover, but the agent still retries forever. systemd then reports the service as running even though it never connects to Kubernetes, watches its Node, or processes updates. should permanent resolution failures eventually fail the service or expose a readiness signal?
i'm not super confident on identifying "permanent resolution failures" (versus "reading a partially written file" or something i haven't thought of).
is this something you want to block merging on or something that can be refined later?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Allow deployments to derive the Node name from the kubelet client certificate instead of relying on the hostname convention.
Changes
TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE:hostname(default) orkubelet-cert. An explicitNODE_NAMEoverride takes precedence.system:node:<name>CN.--validate-connection kubernetesdiagnostic fails immediately on resolution errors.Validation at
777fb02eCI build 1221392 succeeded