From ddee6ff45268217eca552c47ebca06c4611c66c0 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 9 Oct 2026 16:06:12 +0000 Subject: [PATCH 1/6] acl-agent: make node_name source (hostname vs kubelet cert CN) configurable 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:) 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> --- Cargo.lock | 1 + crates/trident-acl-agent/Cargo.toml | 1 + crates/trident-acl-agent/src/core/config.rs | 260 +++++++++++++++++++- docs/Explanation/Trident-ACL-Agent.md | 3 +- 4 files changed, 251 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5b985ce79..85f817cde 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4038,6 +4038,7 @@ dependencies = [ "log", "maplit", "mockito", + "openssl", "osutils", "quick-xml", "reqwest", diff --git a/crates/trident-acl-agent/Cargo.toml b/crates/trident-acl-agent/Cargo.toml index 1c3c75cb0..31340a257 100644 --- a/crates/trident-acl-agent/Cargo.toml +++ b/crates/trident-acl-agent/Cargo.toml @@ -20,6 +20,7 @@ hex = { workspace = true } k8s-openapi = { workspace = true } kube = { workspace = true } log = { workspace = true } +openssl = { workspace = true } quick-xml = { workspace = true, features = ["serialize"] } reqwest = { workspace = true } semver = { workspace = true } diff --git a/crates/trident-acl-agent/src/core/config.rs b/crates/trident-acl-agent/src/core/config.rs index 2f25af930..8647c6f40 100644 --- a/crates/trident-acl-agent/src/core/config.rs +++ b/crates/trident-acl-agent/src/core/config.rs @@ -24,10 +24,16 @@ //! behavior) remains available as an explicit opt-out via //! `TRIDENT_ACL_AGENT_ORCHESTRATION_MODE=omaha-only`. -use std::{path::PathBuf, str::FromStr, time::Duration}; +use std::{ + path::{Path, PathBuf}, + str::FromStr, + time::Duration, +}; use anyhow::{anyhow, Context, Error}; use const_format::formatcp; +use log::warn; +use openssl::{nid::Nid, x509::X509}; use serde::{de::Error as _, Deserialize, Deserializer}; use trident_proto::TRIDENT_DEFAULT_SOCKET_URI; use url::Url; @@ -61,6 +67,14 @@ const DEFAULT_NODE_GONE_MAX_WAIT: Option = None; pub const STATE_FILE_NAME: &str = "state.json"; pub const DEFAULT_STATE_PATH: &str = formatcp!("/var/lib/trident-acl-agent/{STATE_FILE_NAME}"); pub const DEFAULT_KUBELET_KUBECONFIG: &str = "/var/lib/kubelet/kubeconfig"; +/// Default path to kubelet's own client certificate, used to derive +/// `node_name` when `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` is +/// `kubelet-cert`. Not independently overridable (yet) - only the source +/// selection is. +const DEFAULT_KUBELET_CLIENT_CERT: &str = "/var/lib/kubelet/pki/kubelet-client-current.pem"; +/// Prefix kubelet's client cert Subject CN always carries +/// (`system:node:`), per the Kubernetes TLS bootstrapping spec. +const KUBELET_CERT_CN_NODE_PREFIX: &str = "system:node:"; /// Default annotation-key prefix. /// Override with `TRIDENT_ACL_AGENT_KUBERNETES_ANNOTATION_PREFIX`. pub const DEFAULT_ANNOTATION_PREFIX: &str = "acl.microsoft.com"; @@ -106,16 +120,22 @@ impl AgentConfig { .track .unwrap_or_else(|| DEFAULT_NEBRASKA_TRACK.to_string()), }, - kubernetes: KubernetesConfig { - api_server: kubernetes.api_server, - kubeconfig: kubernetes - .kubeconfig - .unwrap_or_else(|| PathBuf::from(DEFAULT_KUBELET_KUBECONFIG)), - node_name: kubernetes.node_name.unwrap_or_else(default_node_name), - watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, - annotation_prefix: kubernetes - .annotation_prefix - .unwrap_or_else(|| DEFAULT_ANNOTATION_PREFIX.to_string()), + kubernetes: { + let node_name_source = kubernetes.node_name_source.unwrap_or_default(); + KubernetesConfig { + api_server: kubernetes.api_server, + kubeconfig: kubernetes + .kubeconfig + .unwrap_or_else(|| PathBuf::from(DEFAULT_KUBELET_KUBECONFIG)), + node_name: kubernetes + .node_name + .unwrap_or_else(|| default_node_name(node_name_source)), + node_name_source, + watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, + annotation_prefix: kubernetes + .annotation_prefix + .unwrap_or_else(|| DEFAULT_ANNOTATION_PREFIX.to_string()), + } }, trident: TridentConfig { socket: trident @@ -166,6 +186,8 @@ struct RawKubernetesConfig { kubeconfig: Option, #[serde(deserialize_with = "empty_string_as_none")] node_name: Option, + #[serde(deserialize_with = "empty_node_name_source_as_none")] + node_name_source: Option, #[serde(deserialize_with = "empty_string_as_none")] annotation_prefix: Option, } @@ -254,6 +276,17 @@ where .transpose() } +fn empty_node_name_source_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)) + .transpose() +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct NebraskaConfig { pub endpoint: Option, @@ -283,6 +316,11 @@ pub struct KubernetesConfig { pub api_server: Option, pub kubeconfig: PathBuf, pub node_name: String, + /// Which source `node_name` was (or would be) derived from when no + /// explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` override is set. + /// See [`NodeNameSource`] for the two options and why hostname remains + /// the default despite not being AKS-guaranteed. + pub node_name_source: NodeNameSource, pub watch_poll_interval: Duration, /// Annotation-key prefix used for the request/status/commit-status /// annotations (e.g. `acl.microsoft.com` in @@ -295,10 +333,12 @@ pub struct KubernetesConfig { impl Default for KubernetesConfig { fn default() -> Self { + let node_name_source = NodeNameSource::default(); Self { api_server: None, kubeconfig: PathBuf::from(DEFAULT_KUBELET_KUBECONFIG), - node_name: default_node_name(), + node_name: default_node_name(node_name_source), + node_name_source, watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, annotation_prefix: DEFAULT_ANNOTATION_PREFIX.to_string(), } @@ -357,6 +397,42 @@ impl FromStr for Mode { } } +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum NodeNameSource { + /// Lowercased `hostname()`. This matches cloud-provider-azure's current + /// AKS convention (the Node object is registered under the lowercased + /// hostname; see AgentBaker's `cse_config.sh`), but per the AKS team + /// that convention is an implementation detail and is **not** + /// guaranteed to hold. Kept as the default anyway, for backward + /// compatibility with existing deployments - switch to `KubeletCert` + /// explicitly where the guarantee matters. + #[default] + Hostname, + /// Subject CN of kubelet's own client certificate + /// (`/var/lib/kubelet/pki/kubelet-client-current.pem`), stripped of its + /// `system:node:` prefix. This is the identity kubelet itself + /// authenticates to the API server with, so it is guaranteed to match + /// the real Node name regardless of hostname conventions. Opt in with + /// `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE=kubelet-cert`. If the + /// cert can't be read or parsed, falls back to `Hostname` (logging a + /// warning) rather than failing startup outright. + KubeletCert, +} + +impl FromStr for NodeNameSource { + type Err = Error; + + fn from_str(s: &str) -> Result { + match s { + "hostname" => Ok(Self::Hostname), + "kubelet-cert" => Ok(Self::KubeletCert), + other => Err(anyhow!( + "unknown node_name_source {other:?} (expected \"hostname\" or \"kubelet-cert\")" + )), + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct OrchestrationConfig { pub mode: Mode, @@ -401,7 +477,23 @@ fn default_nebraska_endpoint() -> Url { .expect("invariant: DEFAULT_NEBRASKA_ENDPOINT is a compile-time-valid URL") } -fn default_node_name() -> String { +fn default_node_name(source: NodeNameSource) -> String { + match source { + NodeNameSource::Hostname => hostname_node_name(), + NodeNameSource::KubeletCert => node_name_from_kubelet_cert(Path::new( + DEFAULT_KUBELET_CLIENT_CERT, + )) + .unwrap_or_else(|err| { + warn!( + "failed to derive node_name from kubelet client cert at \ + {DEFAULT_KUBELET_CLIENT_CERT} ({err:#}); falling back to hostname" + ); + hostname_node_name() + }), + } +} + +fn hostname_node_name() -> String { // Kubernetes Node names must be valid RFC 1123 DNS labels, which are // lowercase-only; kubelet itself lowercases the hostname when it // registers the Node object. Match that behavior here so a mixed-case @@ -414,6 +506,29 @@ fn default_node_name() -> String { .to_lowercase() } +/// Derives `node_name` from kubelet's own client cert's Subject CN +/// (`system:node:`), stripping the `system:node:` prefix. +fn node_name_from_kubelet_cert(cert_path: &Path) -> Result { + let pem = std::fs::read(cert_path).with_context(|| format!("failed to read {cert_path:?}"))?; + let cert = X509::from_pem(&pem) + .with_context(|| format!("failed to parse X.509 certificate from {cert_path:?}"))?; + let cn = cert + .subject_name() + .entries_by_nid(Nid::COMMONNAME) + .next() + .and_then(|entry| entry.data().as_utf8().ok()) + .map(|cn| cn.to_string()) + .ok_or_else(|| anyhow!("certificate at {cert_path:?} has no Subject CN"))?; + 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:?}" + ) + }) +} + #[cfg(test)] mod tests { use super::*; @@ -466,6 +581,11 @@ mod tests { config.kubernetes.annotation_prefix, DEFAULT_ANNOTATION_PREFIX ); + assert_eq!( + config.kubernetes.node_name_source, + NodeNameSource::Hostname, + "hostname must remain the default node_name_source for backward compatibility" + ); } #[test] @@ -486,6 +606,10 @@ mod tests { "/etc/trident-acl-agent/kubeconfig", ), ("TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME", "node-42"), + ( + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", + "kubelet-cert", + ), ( "TRIDENT_ACL_AGENT_TRIDENT_SOCKET", "unix:///custom/trident.sock", @@ -521,6 +645,10 @@ mod tests { PathBuf::from("/etc/trident-acl-agent/kubeconfig") ); assert_eq!(config.kubernetes.node_name, "node-42"); + assert_eq!( + config.kubernetes.node_name_source, + NodeNameSource::KubeletCert + ); assert_eq!(config.trident.socket, "unix:///custom/trident.sock"); assert_eq!(config.orchestration.mode, Mode::OmahaOnly); assert_eq!( @@ -600,4 +728,110 @@ mod tests { .unwrap_err(); assert!(format!("{err:#}").contains("not a duration"), "{err:#}"); } + + #[test] + fn malformed_node_name_source_is_a_parse_error() { + let err = AgentConfig::from_vars(vars(&[( + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", + "bogus", + )])) + .unwrap_err(); + assert!(format!("{err:#}").contains("bogus"), "{err:#}"); + } + + #[test] + fn empty_node_name_source_falls_back_to_hostname_default() { + let config = AgentConfig::from_vars(vars(&[( + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", + "", + )])) + .unwrap(); + assert_eq!(config.kubernetes.node_name_source, NodeNameSource::Hostname); + } + + // CN "system:node:aks-nodepool1-36435499-vmss000000", O "system:nodes" - + // the shape a real AKS kubelet client cert has. + const TEST_CERT_NODE_CN: &str = "-----BEGIN CERTIFICATE----- +MIIDfzCCAmegAwIBAgIUd01WEzdpjKsn3oATTeMT8zBL5cIwDQYJKoZIhvcNAQEL +BQAwTzEVMBMGA1UECgwMc3lzdGVtOm5vZGVzMTYwNAYDVQQDDC1zeXN0ZW06bm9k +ZTpha3Mtbm9kZXBvb2wxLTM2NDM1NDk5LXZtc3MwMDAwMDAwHhcNMjYxMDA5MTYw +MTUwWhcNMzYxMDA2MTYwMTUwWjBPMRUwEwYDVQQKDAxzeXN0ZW06bm9kZXMxNjA0 +BgNVBAMMLXN5c3RlbTpub2RlOmFrcy1ub2RlcG9vbDEtMzY0MzU0OTktdm1zczAw +MDAwMDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAL6Zd1AjlXLtYCFb +JTomb9M94zkzT49It6TqBKOiETnnGIhWS3F917ueaUJhhcSzMwKdGIaUU3FM5NPV +Tsd2zBnKs8/YkrVbP+Fyn6K4u5Q/8erilf3A3I4Dh/soxauK0H4HNh3UzEnwdvks +HasXEcVbb8488ld8p0eUmx8ycHXTnTVA00oiyp6wIbRClwOfIgox3a+0Fl0u49Wv +TIhGbUPj1Hkpm6BrTk5zoVKFOKtjmWBJ/tK/Oi1OcGF3cOW/tnJ+yhKYbk+a7Pok +EHqpFwhpJ28xgi9IVxIPWR9bskGdbX39r5EwXRhM3eguI7ReiupwAjR2UHbuHjx0 +5KIp8ucCAwEAAaNTMFEwHQYDVR0OBBYEFPrWvCYXfgGQ+8AskqoFpLVwxFsTMB8G +A1UdIwQYMBaAFPrWvCYXfgGQ+8AskqoFpLVwxFsTMA8GA1UdEwEB/wQFMAMBAf8w +DQYJKoZIhvcNAQELBQADggEBAIn8Qv+hKXd1mCw+9QY1r4t07rSY5pcrEJh2H56g +67c72HvW+6xhizPyctkpFqH8bFkr5Ob+jASvlJenV1FRENYEdw9esMzxNWd0RQdu +9EYHZInD1XBF9n3R1yavtNT8NkCCReB33R3qPySqeFFZ0ATjRThoc4676V/34/Vt +hLURihiYkJJiIPByX4RxoB1dq1Etk6pGtlGquMjmNN5eo+YKqhejRkjdk5YQmuws +G3/d/F+SqLfGBUcGRBU1bw7Wu4KmkBMBgPOCUyWcspQsB4l0fhCOs4VyYB2DJjPb +wN+7zVpbIrXaPJX/yoPlLodQ6TbM/08QfJ6jRBTZ3iLjm+o= +-----END CERTIFICATE----- +"; + + // CN "not-a-node-identity" - a well-formed cert that isn't a kubelet + // client cert, to exercise the "no system:node: prefix" error path. + const TEST_CERT_BAD_CN: &str = "-----BEGIN CERTIFICATE----- +MIIDHTCCAgWgAwIBAgIUUuLrFudHzqPoZiQlWpn0pwkQfekwDQYJKoZIhvcNAQEL +BQAwHjEcMBoGA1UEAwwTbm90LWEtbm9kZS1pZGVudGl0eTAeFw0yNjEwMDkxNjAx +NTdaFw0zNjEwMDYxNjAxNTdaMB4xHDAaBgNVBAMME25vdC1hLW5vZGUtaWRlbnRp +dHkwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCh4AGuv6pTVUVSeoCd +hjhUNpuNOFzgdvOvFn3CdqjgBrV6emS0jf3eRBKKzyrhU0zSOHr0/ulCXGGE4+ah +bak666z/23F17ilSM9Z5iJdrDfSKmmTLpswv5/UyJwrdUQJjrqH4zweWOHSDj81P +xX6RjIfMaLOh3GWXrrSb4IJtQ+ms4zPiz73mXxoxDNG9Z8PV8Rk9YeyqANKs1ovi +zhOI+fBTMQbnYCyWIxGHjeJWTBtVjUtU1eiU/iXlFIWQgNGxYgL94qPITNGLNA/p +7TV8UfsGZukNKOImW4TgGyghGoSY6vda6eIsKHr/ezgRRFnN/AQ1fzYX/eIzYcZl +wRA3AgMBAAGjUzBRMB0GA1UdDgQWBBT1y6XPmHYq/vxuuM66ZzLhoPZDGjAfBgNV +HSMEGDAWgBT1y6XPmHYq/vxuuM66ZzLhoPZDGjAPBgNVHRMBAf8EBTADAQH/MA0G +CSqGSIb3DQEBCwUAA4IBAQB0/859B0zfEujTcUX8sS19minrLuXk/FhttIoH1Wl0 +DOF4kbsN1mkiaBWXFqOivTbBFVkVVv+yzvPRyv0NKVzKyFOy05Q2Mz79ZnXTEDTN +naxxy4nls8EAcarecR9XKkLbJgE+pUZ7bOL+qUNyRBIF7RdDSIyYcNYLg5XoA982 +xFeJ1m79RhbveFGkhVCsdvaf07z+8JZtEH+apEUjtBHbRU+eXt+ovD3yoxufw8RW +P1bXRwozJvvsUsxTGkU7z3y54wDQWGRf43vRNYcXOiY45sgqOX46m56S1R0Xumpd +gf0TbLqnNti3MeMftKcEa8VQ2rE6Np02EETxr94lsiNK +-----END CERTIFICATE----- +"; + + #[test] + fn node_name_from_kubelet_cert_strips_system_node_prefix() { + let dir = tempfile::tempdir().unwrap(); + let cert_path = dir.path().join("kubelet-client-current.pem"); + std::fs::write(&cert_path, TEST_CERT_NODE_CN).unwrap(); + + let node_name = node_name_from_kubelet_cert(&cert_path).unwrap(); + assert_eq!(node_name, "aks-nodepool1-36435499-vmss000000"); + } + + #[test] + fn node_name_from_kubelet_cert_errors_without_system_node_prefix() { + let dir = tempfile::tempdir().unwrap(); + let cert_path = dir.path().join("not-a-kubelet-cert.pem"); + std::fs::write(&cert_path, TEST_CERT_BAD_CN).unwrap(); + + let err = node_name_from_kubelet_cert(&cert_path).unwrap_err(); + assert!(format!("{err:#}").contains("system:node:"), "{err:#}"); + } + + #[test] + fn node_name_from_kubelet_cert_errors_on_missing_file() { + let err = node_name_from_kubelet_cert(Path::new("/nonexistent/kubelet-client-current.pem")) + .unwrap_err(); + assert!(format!("{err:#}").contains("failed to read"), "{err:#}"); + } + + #[test] + fn default_node_name_falls_back_to_hostname_when_cert_source_requested_but_unreadable() { + // DEFAULT_KUBELET_CLIENT_CERT won't exist on a dev/test machine, so + // requesting KubeletCert must degrade to the same value Hostname + // would produce, rather than erroring or returning an empty string. + assert_eq!( + default_node_name(NodeNameSource::KubeletCert), + default_node_name(NodeNameSource::Hostname) + ); + } } diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 6777d721b..2370dc244 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -260,7 +260,8 @@ naming the offending variable. | `TRIDENT_ACL_AGENT_CURRENT_VERSION_FALLBACK` | `always` | Controls what happens when `TRIDENT_ACL_AGENT_CURRENT_VERSION_KEY` isn't present at `TRIDENT_ACL_AGENT_CURRENT_VERSION_PATH` (e.g. a dev/test host, or an image that hasn't started stamping that key yet). `always` reports `0.0.0` as the node's current version — a sentinel that can never collide with a real release version and cause a false `AlreadyAtTarget`. `error` fails the operation instead of using a placeholder version. Any other value is used verbatim as the current version, with no format validation. | | `TRIDENT_ACL_AGENT_KUBERNETES_API_SERVER` | unset | Explicit override for the Kubernetes API server URL. When unset, the server embedded in the kubeconfig is used as-is. | | `TRIDENT_ACL_AGENT_KUBERNETES_KUBECONFIG` | `/var/lib/kubelet/kubeconfig` | Path to the kubeconfig used to reach the Kubernetes API server and authenticate as this node. | -| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` | The node's own hostname, lowercased | The Node object this agent watches/patches. | +| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` | derived from `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | The Node object this agent watches/patches. | +| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | `hostname` | How `node_name` is derived when `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` is unset. `hostname` uses the node's own hostname, lowercased — matches AKS's current (but not guaranteed) convention of registering Nodes under the lowercased hostname. `kubelet-cert` instead reads the Subject CN (`system:node:`) from kubelet's own client certificate (`/var/lib/kubelet/pki/kubelet-client-current.pem`) — the identity kubelet itself authenticates to the API server with, so it's guaranteed to match the real Node name. If the cert can't be read or parsed, the agent logs a warning and falls back to the hostname behavior instead of failing startup. | | `TRIDENT_ACL_AGENT_TRIDENT_SOCKET` | `unix:///run/trident/trident.sock` | The gRPC Unix socket URI used to reach `tridentd`. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_STATE_PATH` | `/var/lib/trident-acl-agent/state.json` | Path to the agent's persistent state file bridging the pre-reboot and post-reboot halves of `finalize`/`rollback` across the reboot. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_STAGE_TIMEOUT` | `20m` | How long a `stage` is allowed to run (a [`humantime`](https://docs.rs/humantime) duration, e.g. `20m`, `1h`) before it's considered failed. | From ccc212b8deaf5512d1f9b8e5e481c2f20c8c4dcb Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 9 Oct 2026 19:47:26 +0000 Subject: [PATCH 2/6] acl-agent: add fast storm test for node_name_source 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> --- .../Testing/TridentAclAgent-Tests.md | 19 +- tools/storm/aclagent/README.md | 26 ++ .../storm/aclagent/tests/node_name_source.go | 236 ++++++++++++++++++ tools/storm/aclagent/trident.go | 4 + 4 files changed, 281 insertions(+), 4 deletions(-) create mode 100644 tools/storm/aclagent/tests/node_name_source.go diff --git a/docs/Development/Testing/TridentAclAgent-Tests.md b/docs/Development/Testing/TridentAclAgent-Tests.md index e11dae9e5..69fa2380b 100644 --- a/docs/Development/Testing/TridentAclAgent-Tests.md +++ b/docs/Development/Testing/TridentAclAgent-Tests.md @@ -269,22 +269,33 @@ The scenario runs these test cases in order: 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 -4. **run-ab-update** — Starts the fake apiserver and fake Nebraska/Omaha +4. **run-node-name-source** — Proves + `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` (`hostname` vs + `kubelet-cert`) actually switches which identity `node_name` resolves + from, via one-shot `trident-acl-agent --validate-connection kubernetes` + CLI invocations against a self-signed fake kubelet client cert — no + service restart or reboot is involved, so this stays fast. Checks that + `kubelet-cert` resolves to the cert's Subject CN, that `hostname` (and + the unset default) instead resolves to the VM's real hostname, that an + explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` still wins regardless + of source, and that an unreadable cert degrades `kubelet-cert` to the + hostname behavior with a logged warning rather than erroring out +5. **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 `trident-acl-agent.service`, patches the `acl.azure.com/update-request` annotation, and waits for `trident-acl-agent` to drive a real Trident A/B update to completion (including a real reboot) -5. **run-rollback** — Exercises the `rollback` annotation end-to-end against +6. **run-rollback** — Exercises the `rollback` annotation end-to-end against tridentd's `RollbackService` gRPC API, followed by a real reboot and post-reboot commit; must run after `run-ab-update` in the same VM lifetime, since it rolls back to the volume active before that update -6. **collect-logs** — Fetches `trident-acl-agent` and Trident logs from the +7. **collect-logs** — Fetches `trident-acl-agent` and Trident logs from the VM via SSH; also runs automatically (with a `journalctl` dump for `trident-acl-agent.service`) if an update/rollback/resilience step times out waiting for the service to become active, to make crash-loops self-diagnosing -7. **cleanup-vm** — Destroys the QEMU VM +8. **cleanup-vm** — Destroys the QEMU VM ### Flags diff --git a/tools/storm/aclagent/README.md b/tools/storm/aclagent/README.md index 58d010a0c..07b6afd62 100644 --- a/tools/storm/aclagent/README.md +++ b/tools/storm/aclagent/README.md @@ -24,6 +24,11 @@ There is intentionally no fake `tridentd`. 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-node-name-source` — proves `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` + (`hostname` vs `kubelet-cert`) actually switches which identity + `node_name` resolves from, via one-shot `--validate-connection + kubernetes` CLI invocations against a fake kubelet client cert - no + service restart/reboot needed, so it stays fast - `run-ab-update` - `run-rollback` - `collect-logs` @@ -141,6 +146,27 @@ to `HostEndpointIP` (the libvirt NAT gateway address), which the VM can reach directly — this avoids relying on reverse SSH tunnels, which don't survive the VM actually going down for a real reboot. +## `run-node-name-source` + +Proves `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` (microsoft/trident#839) +actually switches which identity `node_name` resolves from, entirely via +one-shot `trident-acl-agent --validate-connection kubernetes` CLI +invocations — it never touches `trident-acl-agent.service` (no restart, no +reboot), so it runs fast relative to the rest of this scenario. It seeds the +fake apiserver with a single Node whose name matches a self-signed fake +kubelet client cert's Subject CN (`system:node:`), uploaded to +`/var/lib/kubelet/pki/kubelet-client-current.pem`, and checks: + +- `kubelet-cert` source resolves to that cert's CN and succeeds +- `hostname` source (and the unset/default case) resolves to the VM's real + hostname instead, which this fake apiserver doesn't serve, and fails +- an explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` still wins outright + regardless of `NODE_NAME_SOURCE` +- with the cert hidden, `kubelet-cert` source degrades to the hostname + behavior (same failure shape as above) and logs a "falling back to + hostname" warning, rather than erroring out some other way + + ## `run-rollback` `run-rollback` exercises the `rollback` annotation end-to-end against diff --git a/tools/storm/aclagent/tests/node_name_source.go b/tools/storm/aclagent/tests/node_name_source.go new file mode 100644 index 000000000..c4380cce7 --- /dev/null +++ b/tools/storm/aclagent/tests/node_name_source.go @@ -0,0 +1,236 @@ +package tests + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "fmt" + "math/big" + "os" + "path/filepath" + "strings" + "time" + + stormproxies "tridenttools/storm/aclagent/proxies" + stormaclconfig "tridenttools/storm/aclagent/utils/config" + stormssh "tridenttools/storm/utils/ssh" + stormvm "tridenttools/storm/utils/vm" + stormvmconfig "tridenttools/storm/utils/vm/config" +) + +// kubeletClientCertPath mirrors trident-acl-agent's own +// DEFAULT_KUBELET_CLIENT_CERT (crates/trident-acl-agent/src/core/config.rs). +const kubeletClientCertPath = "/var/lib/kubelet/pki/kubelet-client-current.pem" + +// certDerivedNodeName is the Node name this test's fake kubelet client cert +// claims via its Subject CN (system:node:), deliberately different +// from testConfig.NodeName (the VM's real, lowercased hostname). The fake +// apiserver (see proxies.NewAPIServer) only ever serves a single Node name - +// whatever its own nodeName argument is - so seeding it with +// certDerivedNodeName instead of testConfig.NodeName makes the two +// node_name sources mutually exclusive: resolving to the wrong one always +// 404s against this fake, there is no overlap that could make this test +// pass vacuously. +const certDerivedNodeName = "node-name-source-test-cert-node" + +// RunNodeNameSource proves TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE +// (microsoft/trident#839) actually switches which identity +// trident-acl-agent resolves node_name from, entirely through one-shot +// `trident-acl-agent --validate-connection kubernetes` invocations - no +// service restart or reboot needed, since validate-connection is a +// standalone CLI invocation of the same binary, not a call into the +// long-running trident-acl-agent.service. That keeps this test fast +// relative to the rest of this VM-based scenario. +func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllVMConfig) error { + vmIP, err := stormvm.GetVmIP(vmConfig) + if err != nil { + return fmt.Errorf("failed to get VM IP: %w", err) + } + if testConfig.OutputPath != "" { + if err := os.MkdirAll(testConfig.OutputPath, 0o755); err != nil { + return err + } + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Only the fake apiserver is needed - no Nebraska/image-server mock, + // exactly like RunNodeResilience (see its own doc comment for why). + nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(certDerivedNodeName, map[string]string{})) + apiServer := stormproxies.NewAPIServer(certDerivedNodeName, nodeStore) + _, apiServerStop, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("%s:%d", testConfig.HostEndpointIP, testConfig.APIServerPort)) + if err != nil { + return fmt.Errorf("failed to start fake apiserver: %w", err) + } + // Deferred instead of relying on ctx cancellation alone - the next test + // case binds the same HostEndpointIP:APIServerPort and must not race + // this apiserver's teardown, same reasoning as RunABUpdate/ + // RunNodeResilience's own apiServerStop defer. + defer apiServerStop() + nodeStore.SetReadyCondition(true) + + // Only need the fake kubeconfig this writes, so the agent binary's own + // compiled-in default kubeconfig path resolves to the fake apiserver + // above - trident-acl-agent.service itself is never touched by this + // test, since every check below is a direct one-shot CLI invocation. + if err := prepareVmForAclAgent(vmConfig.VMConfig, vmIP, testConfig, nil); err != nil { + return err + } + + certPEM, err := generateFakeKubeletClientCertPEM(certDerivedNodeName) + if err != nil { + return fmt.Errorf("failed to generate fake kubelet client cert: %w", err) + } + if err := uploadKubeletClientCert(vmConfig.VMConfig, vmIP, certPEM); err != nil { + return err + } + + // 1. kubelet-cert source resolves node_name from the cert's Subject CN, + // which IS this fake apiserver's one known Node - must succeed. + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", + }); err != nil { + return fmt.Errorf("kubelet-cert source: %w", err) + } + + // 2. hostname source (explicit, and separately the compiled-in default + // below) resolves to the VM's real hostname, which this fake + // apiserver does NOT serve - must fail. Proves the two sources are + // genuinely different code paths, not just hostname silently used + // both times. + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", false, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "hostname", + }); err != nil { + return fmt.Errorf("hostname source: %w", err) + } + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", false, nil); err != nil { + return fmt.Errorf("default (unset NODE_NAME_SOURCE) source: %w", err) + } + + // 3. An explicit TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME must still win + // outright regardless of NODE_NAME_SOURCE - proves override + // precedence over source selection. + if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME": certDerivedNodeName, + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "hostname", + }); err != nil { + return fmt.Errorf("explicit node_name override: %w", err) + } + + // 4. An unreadable/missing cert must make kubelet-cert source degrade + // to the hostname behavior (logging a warning), not error out + // outright - confirmed two ways: the CLI invocation fails with + // exactly the same (hostname-not-served) shape as case 2 above, and + // the fallback warning is actually logged (trident-acl-agent's + // default --verbosity is Debug, so a Warn-level log is never + // filtered out here). + if _, err := stormssh.SshCommandCombinedOutput(vmConfig.VMConfig, vmIP, fmt.Sprintf("sudo mv %s %s.bak", kubeletClientCertPath, kubeletClientCertPath)); err != nil { + return fmt.Errorf("failed to hide kubelet client cert: %w", err) + } + // Deferred (not a plain call at the end) so the cert is restored + // regardless of which return path below is taken, leaving the VM in + // the same state this test case found it in for anyone re-running + // just this test case in isolation. + defer func() { + if err := restoreKubeletClientCert(vmConfig.VMConfig, vmIP); err != nil { + fmt.Fprintf(os.Stderr, "run-node-name-source: %v\n", err) + } + }() + + out, err := runValidateConnectionKubernetes(vmConfig.VMConfig, vmIP, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", + }) + if err == nil { + return fmt.Errorf("kubelet-cert source with missing cert: expected failure (falls back to hostname, which this fake apiserver doesn't serve), got success") + } + if !strings.Contains(out, "falling back to hostname") { + return fmt.Errorf("kubelet-cert source with missing cert: expected a 'falling back to hostname' warning in output, got: %s", out) + } + + return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) +} + +// uploadKubeletClientCert writes certPEM to kubeletClientCertPath on the VM, +// mirroring how prepareVmForAclAgent delivers the fake kubeconfig. +func uploadKubeletClientCert(cfg stormvmconfig.VMConfig, vmIP string, certPEM []byte) error { + localCertFile, err := os.CreateTemp("", "trident-acl-agent-kubelet-client-cert-*.pem") + if err != nil { + return fmt.Errorf("failed to create local temp file for fake kubelet client cert: %w", err) + } + defer os.Remove(localCertFile.Name()) + if _, err := localCertFile.Write(certPEM); err != nil { + localCertFile.Close() + return fmt.Errorf("failed to write local temp fake kubelet client cert: %w", err) + } + if err := localCertFile.Close(); err != nil { + return fmt.Errorf("failed to close local temp fake kubelet client cert: %w", err) + } + if _, err := stormssh.SshCommandCombinedOutput(cfg, vmIP, fmt.Sprintf("sudo mkdir -p %s", filepath.Dir(kubeletClientCertPath))); err != nil { + return fmt.Errorf("failed to create kubelet pki directory on VM: %w", err) + } + if err := stormssh.ScpUploadFileWithSudo(cfg, vmIP, localCertFile.Name(), kubeletClientCertPath); err != nil { + return fmt.Errorf("failed to upload fake kubelet client cert to VM: %w", err) + } + return nil +} + +// restoreKubeletClientCert undoes the "hide the cert" step in case 4 above, +// so this test case leaves the VM in the same state it found it in (a +// valid cert present) regardless of pass/fail, for anyone re-running just +// this test case in isolation. +func restoreKubeletClientCert(cfg stormvmconfig.VMConfig, vmIP string) error { + if _, err := stormssh.SshCommandCombinedOutput(cfg, vmIP, fmt.Sprintf("sudo mv %s.bak %s", kubeletClientCertPath, kubeletClientCertPath)); err != nil { + return fmt.Errorf("failed to restore kubelet client cert: %w", err) + } + return nil +} + +// generateFakeKubeletClientCertPEM builds a minimal self-signed certificate +// shaped like a real AKS kubelet client cert: Subject CN +// "system:node:", Organization "system:nodes" - the only two +// fields node_name_from_kubelet_cert (crates/trident-acl-agent/src/core/ +// config.rs) actually reads. +func generateFakeKubeletClientCertPEM(nodeName string) ([]byte, error) { + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, fmt.Errorf("failed to generate test key: %w", err) + } + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + CommonName: "system:node:" + nodeName, + Organization: []string{"system:nodes"}, + }, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(10 * 365 * 24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &priv.PublicKey, priv) + if err != nil { + return nil, fmt.Errorf("failed to create test certificate: %w", err) + } + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), nil +} + +// runValidateConnectionKubernetes is like expectValidateConnection (see +// update.go) but returns the combined output too, for the one case in this +// file (kubelet-cert source with an unreadable cert) that needs to assert +// on a specific log message, not just the exit status. +func runValidateConnectionKubernetes(cfg stormvmconfig.VMConfig, vmIP string, envVars map[string]string) (string, error) { + var prefix strings.Builder + prefix.WriteString("sudo") + if len(envVars) > 0 { + prefix.WriteString(" env") + for k, v := range envVars { + fmt.Fprintf(&prefix, " %s=%q", k, v) + } + } + command := fmt.Sprintf("%s trident-acl-agent --validate-connection kubernetes", prefix.String()) + return stormssh.SshCommandCombinedOutput(cfg, vmIP, command) +} diff --git a/tools/storm/aclagent/trident.go b/tools/storm/aclagent/trident.go index 724d50bbb..18880030e 100644 --- a/tools/storm/aclagent/trident.go +++ b/tools/storm/aclagent/trident.go @@ -45,6 +45,7 @@ func (s *TridentAclAgentScenario) RegisterTestCases(r storm.TestRegistrar) error r.RegisterTestCase("deploy-vm", s.deployVm) r.RegisterTestCase("check-deployment", s.checkDeployment) r.RegisterTestCase("run-node-resilience", s.runNodeResilience) + r.RegisterTestCase("run-node-name-source", s.runNodeNameSource) r.RegisterTestCase("run-ab-update", s.runABUpdate) r.RegisterTestCase("run-rollback", s.runRollback) r.RegisterTestCase("collect-logs", s.collectLogs) @@ -84,6 +85,9 @@ func (s *TridentAclAgentScenario) checkDeployment(tc storm.TestCase) error { func (s *TridentAclAgentScenario) runNodeResilience(tc storm.TestCase) error { return s.runTestCase(tc, stormtests.RunNodeResilience) } +func (s *TridentAclAgentScenario) runNodeNameSource(tc storm.TestCase) error { + return s.runTestCase(tc, stormtests.RunNodeNameSource) +} func (s *TridentAclAgentScenario) runABUpdate(tc storm.TestCase) error { return s.runTestCase(tc, stormtests.RunABUpdate) } From 145c2eaea6aded15a546af0ab67188a095734583 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 9 Oct 2026 20:23:38 +0000 Subject: [PATCH 3/6] acl-agent: fix run-node-name-source to check error text for fallback 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> --- tools/storm/aclagent/tests/node_name_source.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/storm/aclagent/tests/node_name_source.go b/tools/storm/aclagent/tests/node_name_source.go index c4380cce7..675ac7e0a 100644 --- a/tools/storm/aclagent/tests/node_name_source.go +++ b/tools/storm/aclagent/tests/node_name_source.go @@ -141,14 +141,19 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon } }() - out, err := runValidateConnectionKubernetes(vmConfig.VMConfig, vmIP, map[string]string{ + _, err = runValidateConnectionKubernetes(vmConfig.VMConfig, vmIP, map[string]string{ "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", }) if err == nil { return fmt.Errorf("kubelet-cert source with missing cert: expected failure (falls back to hostname, which this fake apiserver doesn't serve), got success") } - if !strings.Contains(out, "falling back to hostname") { - return fmt.Errorf("kubelet-cert source with missing cert: expected a 'falling back to hostname' warning in output, got: %s", out) + // On failure, stormssh.SshCommandCombinedOutput returns an empty string + // (see innerSshCommand) - the actual remote stdout/stderr is only ever + // present inside the wrapped error's own message ("...\nOutput: ..."), + // so the fallback warning must be looked for there instead of in the + // (always-empty-on-error) first return value. + if !strings.Contains(err.Error(), "falling back to hostname") { + return fmt.Errorf("kubelet-cert source with missing cert: expected a 'falling back to hostname' warning in output, got: %w", err) } return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) From e566aae98fa2f0d1d6bed14a6f7493bbf2ca701b Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 9 Oct 2026 21:47:15 +0000 Subject: [PATCH 4/6] acl-agent: strengthen run-node-name-source with per-phase apiserver and 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> --- .../storm/aclagent/tests/node_name_source.go | 246 +++++++++++------- 1 file changed, 148 insertions(+), 98 deletions(-) diff --git a/tools/storm/aclagent/tests/node_name_source.go b/tools/storm/aclagent/tests/node_name_source.go index 675ac7e0a..487fb2fc0 100644 --- a/tools/storm/aclagent/tests/node_name_source.go +++ b/tools/storm/aclagent/tests/node_name_source.go @@ -26,16 +26,19 @@ import ( const kubeletClientCertPath = "/var/lib/kubelet/pki/kubelet-client-current.pem" // certDerivedNodeName is the Node name this test's fake kubelet client cert -// claims via its Subject CN (system:node:), deliberately different -// from testConfig.NodeName (the VM's real, lowercased hostname). The fake -// apiserver (see proxies.NewAPIServer) only ever serves a single Node name - -// whatever its own nodeName argument is - so seeding it with -// certDerivedNodeName instead of testConfig.NodeName makes the two -// node_name sources mutually exclusive: resolving to the wrong one always -// 404s against this fake, there is no overlap that could make this test -// pass vacuously. +// claims via its Subject CN (system:node:). Deliberately distinct from +// both the VM's real hostname and explicitOverrideNodeName below, so each +// phase's assertion is a genuine positive match on a specific value, never +// a coincidental overlap between two phases. const certDerivedNodeName = "node-name-source-test-cert-node" +// explicitOverrideNodeName is a third literal, distinct from both +// certDerivedNodeName and the VM's real hostname, used only for the +// explicit TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME override check - so that +// check's success can only be explained by the override actually being +// honored, never by it happening to match another phase's expected value. +const explicitOverrideNodeName = "node-name-source-hostname-override-node" + // RunNodeNameSource proves TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE // (microsoft/trident#839) actually switches which identity // trident-acl-agent resolves node_name from, entirely through one-shot @@ -44,6 +47,15 @@ const certDerivedNodeName = "node-name-source-test-cert-node" // standalone CLI invocation of the same binary, not a call into the // long-running trident-acl-agent.service. That keeps this test fast // relative to the rest of this VM-based scenario. +// +// The fake apiserver (see proxies.NewAPIServer) only ever serves a single +// Node name - whatever its own nodeName argument is - so this test runs +// three sequential phases, each with its own single-node apiserver +// instance seeded with the one specific identity that phase expects to +// resolve to. Every check then asserts the *exact* Node name +// validate-connection's own success output reports fetching, not just +// pass/fail - so a phase can only pass by actually resolving to the value +// it claims, never by vacuously matching a different phase's answer. func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmconfig.AllVMConfig) error { vmIP, err := stormvm.GetVmIP(vmConfig) if err != nil { @@ -58,105 +70,120 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon ctx, cancel := context.WithCancel(context.Background()) defer cancel() - // Only the fake apiserver is needed - no Nebraska/image-server mock, - // exactly like RunNodeResilience (see its own doc comment for why). - nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(certDerivedNodeName, map[string]string{})) - apiServer := stormproxies.NewAPIServer(certDerivedNodeName, nodeStore) - _, apiServerStop, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("%s:%d", testConfig.HostEndpointIP, testConfig.APIServerPort)) - if err != nil { - return fmt.Errorf("failed to start fake apiserver: %w", err) - } - // Deferred instead of relying on ctx cancellation alone - the next test - // case binds the same HostEndpointIP:APIServerPort and must not race - // this apiserver's teardown, same reasoning as RunABUpdate/ - // RunNodeResilience's own apiServerStop defer. - defer apiServerStop() - nodeStore.SetReadyCondition(true) - - // Only need the fake kubeconfig this writes, so the agent binary's own - // compiled-in default kubeconfig path resolves to the fake apiserver - // above - trident-acl-agent.service itself is never touched by this - // test, since every check below is a direct one-shot CLI invocation. + // Delivers the fake kubeconfig pointing trident-acl-agent's compiled-in + // default kubeconfig path at HostEndpointIP:APIServerPort. That address + // stays constant across every phase below - only which single Node the + // fake apiserver bound there currently serves changes - so this only + // 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 { return err } - certPEM, err := generateFakeKubeletClientCertPEM(certDerivedNodeName) - if err != nil { - return fmt.Errorf("failed to generate fake kubelet client cert: %w", err) + // --- Phase 1: kubelet-cert source resolves to the cert's Subject CN --- + if err := withNodeNameSourceApiServer(ctx, testConfig, certDerivedNodeName, func() error { + certPEM, err := generateFakeKubeletClientCertPEM(certDerivedNodeName) + if err != nil { + return fmt.Errorf("failed to generate fake kubelet client cert: %w", err) + } + if err := uploadKubeletClientCert(vmConfig.VMConfig, vmIP, certPEM); err != nil { + return err + } + return expectValidateConnectionOutput(vmConfig.VMConfig, vmIP, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", + }, fmt.Sprintf("fetched Node %q", certDerivedNodeName)) + }); err != nil { + return fmt.Errorf("phase kubelet-cert: %w", err) } - if err := uploadKubeletClientCert(vmConfig.VMConfig, vmIP, certPEM); err != nil { + + // --- Phase 2: hostname source (explicit and default) resolves to the + // VM's own real hostname, and a missing/unreadable cert degrades + // kubelet-cert source to that exact same resolution --- + realHostname, err := readVmHostname(vmConfig.VMConfig, vmIP) + if err != nil { return err } + if err := withNodeNameSourceApiServer(ctx, testConfig, realHostname, func() error { + if err := expectValidateConnectionOutput(vmConfig.VMConfig, vmIP, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "hostname", + }, fmt.Sprintf("fetched Node %q", realHostname)); err != nil { + return fmt.Errorf("explicit hostname source: %w", err) + } + if err := expectValidateConnectionOutput(vmConfig.VMConfig, vmIP, nil, + fmt.Sprintf("fetched Node %q", realHostname)); err != nil { + return fmt.Errorf("default (unset NODE_NAME_SOURCE) source: %w", err) + } - // 1. kubelet-cert source resolves node_name from the cert's Subject CN, - // which IS this fake apiserver's one known Node - must succeed. - if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, map[string]string{ - "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", + // An unreadable/missing cert must make kubelet-cert source degrade + // to the hostname behavior (logging a warning), not error out + // outright - confirmed by asserting the very same successful + // resolution to realHostname as above, plus the fallback warning + // actually being logged (trident-acl-agent's default --verbosity + // is Debug, so a Warn-level log is never filtered out here). + if _, err := stormssh.SshCommandCombinedOutput(vmConfig.VMConfig, vmIP, fmt.Sprintf("sudo mv %s %s.bak", kubeletClientCertPath, kubeletClientCertPath)); err != nil { + return fmt.Errorf("failed to hide kubelet client cert: %w", err) + } + defer func() { + if err := restoreKubeletClientCert(vmConfig.VMConfig, vmIP); err != nil { + fmt.Fprintf(os.Stderr, "run-node-name-source: %v\n", err) + } + }() + return expectValidateConnectionOutput(vmConfig.VMConfig, vmIP, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", + }, "falling back to hostname", fmt.Sprintf("fetched Node %q", realHostname)) }); err != nil { - return fmt.Errorf("kubelet-cert source: %w", err) + return fmt.Errorf("phase hostname/missing-cert-fallback: %w", err) } - // 2. hostname source (explicit, and separately the compiled-in default - // below) resolves to the VM's real hostname, which this fake - // apiserver does NOT serve - must fail. Proves the two sources are - // genuinely different code paths, not just hostname silently used - // both times. - if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", false, map[string]string{ - "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "hostname", + // --- Phase 3: an explicit TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME must + // win outright regardless of NODE_NAME_SOURCE. Uses a third literal + // distinct from both phases above, so success here can only be + // explained by the override itself, not by coincidentally matching + // the cert-derived name or the real hostname. --- + if err := withNodeNameSourceApiServer(ctx, testConfig, explicitOverrideNodeName, func() error { + return expectValidateConnectionOutput(vmConfig.VMConfig, vmIP, map[string]string{ + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME": explicitOverrideNodeName, + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "hostname", + }, fmt.Sprintf("fetched Node %q", explicitOverrideNodeName)) }); err != nil { - return fmt.Errorf("hostname source: %w", err) - } - if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", false, nil); err != nil { - return fmt.Errorf("default (unset NODE_NAME_SOURCE) source: %w", err) + return fmt.Errorf("phase explicit node_name override: %w", err) } - // 3. An explicit TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME must still win - // outright regardless of NODE_NAME_SOURCE - proves override - // precedence over source selection. - if err := expectValidateConnection(vmConfig.VMConfig, vmIP, "kubernetes", true, map[string]string{ - "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME": certDerivedNodeName, - "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "hostname", - }); err != nil { - return fmt.Errorf("explicit node_name override: %w", err) - } - - // 4. An unreadable/missing cert must make kubelet-cert source degrade - // to the hostname behavior (logging a warning), not error out - // outright - confirmed two ways: the CLI invocation fails with - // exactly the same (hostname-not-served) shape as case 2 above, and - // the fallback warning is actually logged (trident-acl-agent's - // default --verbosity is Debug, so a Warn-level log is never - // filtered out here). - if _, err := stormssh.SshCommandCombinedOutput(vmConfig.VMConfig, vmIP, fmt.Sprintf("sudo mv %s %s.bak", kubeletClientCertPath, kubeletClientCertPath)); err != nil { - return fmt.Errorf("failed to hide kubelet client cert: %w", err) - } - // Deferred (not a plain call at the end) so the cert is restored - // regardless of which return path below is taken, leaving the VM in - // the same state this test case found it in for anyone re-running - // just this test case in isolation. - defer func() { - if err := restoreKubeletClientCert(vmConfig.VMConfig, vmIP); err != nil { - fmt.Fprintf(os.Stderr, "run-node-name-source: %v\n", err) - } - }() + return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) +} - _, err = runValidateConnectionKubernetes(vmConfig.VMConfig, vmIP, map[string]string{ - "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", - }) - if err == nil { - return fmt.Errorf("kubelet-cert source with missing cert: expected failure (falls back to hostname, which this fake apiserver doesn't serve), got success") - } - // On failure, stormssh.SshCommandCombinedOutput returns an empty string - // (see innerSshCommand) - the actual remote stdout/stderr is only ever - // present inside the wrapped error's own message ("...\nOutput: ..."), - // so the fallback warning must be looked for there instead of in the - // (always-empty-on-error) first return value. - if !strings.Contains(err.Error(), "falling back to hostname") { - return fmt.Errorf("kubelet-cert source with missing cert: expected a 'falling back to hostname' warning in output, got: %w", err) +// withNodeNameSourceApiServer starts a fresh single-node fake apiserver +// bound to testConfig.HostEndpointIP:APIServerPort, seeded with exactly one +// Node named nodeName, runs body, then synchronously stops that apiserver +// before returning - so each of RunNodeNameSource's phases gets a clean +// apiserver instance serving only the one identity that phase expects, +// with no overlap or leftover state from a previous phase, and the next +// phase (or the next test case entirely) never races this one's teardown +// for the same bind address. +func withNodeNameSourceApiServer(ctx context.Context, testConfig stormaclconfig.TestConfig, nodeName string, body func() error) error { + nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(nodeName, map[string]string{})) + apiServer := stormproxies.NewAPIServer(nodeName, nodeStore) + _, stop, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("%s:%d", testConfig.HostEndpointIP, testConfig.APIServerPort)) + if err != nil { + return fmt.Errorf("failed to start fake apiserver for node %q: %w", nodeName, err) } + defer stop() + nodeStore.SetReadyCondition(true) + return body() +} - return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) +// readVmHostname reads the VM's real hostname (the same OS-level value +// trident-acl-agent's own hostname-source fallback reads via hostname::get() +// - see hostname_node_name() in crates/trident-acl-agent/src/core/ +// config.rs), lowercased the same way that function lowercases it before +// using it as a Kubernetes Node name. +func readVmHostname(cfg stormvmconfig.VMConfig, vmIP string) (string, error) { + out, err := stormssh.SshCommandCombinedOutput(cfg, vmIP, "hostname") + if err != nil { + return "", fmt.Errorf("failed to read VM hostname: %w", err) + } + return strings.ToLower(strings.TrimSpace(out)), nil } // uploadKubeletClientCert writes certPEM to kubeletClientCertPath on the VM, @@ -183,10 +210,10 @@ func uploadKubeletClientCert(cfg stormvmconfig.VMConfig, vmIP string, certPEM [] return nil } -// restoreKubeletClientCert undoes the "hide the cert" step in case 4 above, -// so this test case leaves the VM in the same state it found it in (a -// valid cert present) regardless of pass/fail, for anyone re-running just -// this test case in isolation. +// restoreKubeletClientCert undoes the "hide the cert" step in phase 2 +// above, so this test case leaves the VM in the same state it found it in +// (a valid cert present) regardless of pass/fail, for anyone re-running +// just this test case in isolation. func restoreKubeletClientCert(cfg stormvmconfig.VMConfig, vmIP string) error { if _, err := stormssh.SshCommandCombinedOutput(cfg, vmIP, fmt.Sprintf("sudo mv %s.bak %s", kubeletClientCertPath, kubeletClientCertPath)); err != nil { return fmt.Errorf("failed to restore kubelet client cert: %w", err) @@ -223,10 +250,33 @@ func generateFakeKubeletClientCertPEM(nodeName string) ([]byte, error) { return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), nil } -// runValidateConnectionKubernetes is like expectValidateConnection (see -// update.go) but returns the combined output too, for the one case in this -// file (kubelet-cert source with an unreadable cert) that needs to assert -// on a specific log message, not just the exit status. +// expectValidateConnectionOutput runs `trident-acl-agent --validate-connection +// kubernetes` with envVars, asserts it succeeds, and asserts its combined +// output contains every string in wantSubstrings - used wherever this test +// needs to confirm not just that resolution succeeded, but which specific +// Node name it resolved to (e.g. "fetched Node \"\"", the exact +// phrase connection_check.rs's validate_connection logs on success). +func expectValidateConnectionOutput(cfg stormvmconfig.VMConfig, vmIP string, envVars map[string]string, wantSubstrings ...string) error { + out, err := runValidateConnectionKubernetes(cfg, vmIP, envVars) + if err != nil { + return fmt.Errorf("expected success (envVars=%v): %w", envVars, err) + } + for _, want := range wantSubstrings { + if !strings.Contains(out, want) { + return fmt.Errorf("expected output to contain %q (envVars=%v), got: %s", want, envVars, out) + } + } + return nil +} + +// runValidateConnectionKubernetes runs `trident-acl-agent --validate-connection +// kubernetes` with envVars and returns its combined output. Note: +// stormssh.SshCommandCombinedOutput (see ssh.go's innerSshCommand) discards +// the captured remote output on a non-zero exit, returning ("", err) +// instead - the real stdout/stderr only survives inside the wrapped +// error's own message ("...\nOutput: ..."). Callers that need to inspect +// output from an *expected failure* must check err.Error(), not the first +// return value. func runValidateConnectionKubernetes(cfg stormvmconfig.VMConfig, vmIP string, envVars map[string]string) (string, error) { var prefix strings.Builder prefix.WriteString("sudo") From c8ffd82c0ce7671f53ae5023967064d5228740a8 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 9 Oct 2026 22:55:48 +0000 Subject: [PATCH 5/6] acl-agent: kubelet-cert node_name source fails hard, no fallback to hostname 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); 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> --- crates/trident-acl-agent/src/core/config.rs | 91 +++++++++++++------ .../Testing/TridentAclAgent-Tests.md | 15 +-- docs/Explanation/Trident-ACL-Agent.md | 2 +- tools/storm/aclagent/README.md | 32 ++++--- .../storm/aclagent/tests/node_name_source.go | 36 +++++--- 5 files changed, 118 insertions(+), 58 deletions(-) diff --git a/crates/trident-acl-agent/src/core/config.rs b/crates/trident-acl-agent/src/core/config.rs index 8647c6f40..9d27110bc 100644 --- a/crates/trident-acl-agent/src/core/config.rs +++ b/crates/trident-acl-agent/src/core/config.rs @@ -32,7 +32,6 @@ use std::{ use anyhow::{anyhow, Context, Error}; use const_format::formatcp; -use log::warn; use openssl::{nid::Nid, x509::X509}; use serde::{de::Error as _, Deserialize, Deserializer}; use trident_proto::TRIDENT_DEFAULT_SOCKET_URI; @@ -122,14 +121,22 @@ impl AgentConfig { }, kubernetes: { let node_name_source = kubernetes.node_name_source.unwrap_or_default(); + let node_name = match kubernetes.node_name { + Some(name) => name, + None => default_node_name(node_name_source).with_context(|| { + format!( + "failed to determine node_name for \ + {ENV_PREFIX_KUBERNETES}NODE_NAME_SOURCE={node_name_source:?}; set \ + {ENV_PREFIX_KUBERNETES}NODE_NAME explicitly to override" + ) + })?, + }; KubernetesConfig { api_server: kubernetes.api_server, kubeconfig: kubernetes .kubeconfig .unwrap_or_else(|| PathBuf::from(DEFAULT_KUBELET_KUBECONFIG)), - node_name: kubernetes - .node_name - .unwrap_or_else(|| default_node_name(node_name_source)), + node_name, node_name_source, watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, annotation_prefix: kubernetes @@ -333,12 +340,17 @@ pub struct KubernetesConfig { impl Default for KubernetesConfig { fn default() -> Self { - let node_name_source = NodeNameSource::default(); + // Calls hostname_node_name() directly rather than going through + // the now-fallible default_node_name(): NodeNameSource::default() + // is always Hostname, which hostname_node_name() can always + // satisfy (it has its own internal fallback to DEFAULT_NODE_NAME + // on a hostname-read failure), so this Default impl never needs + // the KubeletCert branch that can actually return Err. Self { api_server: None, kubeconfig: PathBuf::from(DEFAULT_KUBELET_KUBECONFIG), - node_name: default_node_name(node_name_source), - node_name_source, + node_name: hostname_node_name(), + node_name_source: NodeNameSource::default(), watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, annotation_prefix: DEFAULT_ANNOTATION_PREFIX.to_string(), } @@ -414,8 +426,12 @@ pub enum NodeNameSource { /// authenticates to the API server with, so it is guaranteed to match /// the real Node name regardless of hostname conventions. Opt in with /// `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE=kubelet-cert`. If the - /// cert can't be read or parsed, falls back to `Hostname` (logging a - /// warning) rather than failing startup outright. + /// cert can't be read or parsed, config loading fails outright (same + /// severity as any other malformed/unsatisfiable config) - deliberately + /// no fallback to `Hostname`: an operator who opted into this source + /// did so because hostname is *not* a trustworthy Node identity on + /// their platform, so silently substituting it back in on cert failure + /// would risk the agent reconciling against the wrong Node entirely. KubeletCert, } @@ -477,19 +493,18 @@ fn default_nebraska_endpoint() -> Url { .expect("invariant: DEFAULT_NEBRASKA_ENDPOINT is a compile-time-valid URL") } -fn default_node_name(source: NodeNameSource) -> String { +fn default_node_name(source: NodeNameSource) -> Result { match source { - NodeNameSource::Hostname => hostname_node_name(), - NodeNameSource::KubeletCert => node_name_from_kubelet_cert(Path::new( - DEFAULT_KUBELET_CLIENT_CERT, - )) - .unwrap_or_else(|err| { - warn!( - "failed to derive node_name from kubelet client cert at \ - {DEFAULT_KUBELET_CLIENT_CERT} ({err:#}); falling back to hostname" - ); - hostname_node_name() - }), + NodeNameSource::Hostname => Ok(hostname_node_name()), + // No fallback to Hostname on failure here, by design - see + // NodeNameSource::KubeletCert's own doc comment for why resolving + // to the *wrong* Node silently is worse than refusing to start. + // The caller (AgentConfig::from_vars) propagates this as a fatal + // config error, the same severity as any other unsatisfiable + // TRIDENT_ACL_AGENT_* setting. + NodeNameSource::KubeletCert => { + node_name_from_kubelet_cert(Path::new(DEFAULT_KUBELET_CLIENT_CERT)) + } } } @@ -825,13 +840,35 @@ gf0TbLqnNti3MeMftKcEa8VQ2rE6Np02EETxr94lsiNK } #[test] - fn default_node_name_falls_back_to_hostname_when_cert_source_requested_but_unreadable() { + 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 degrade to the same value Hostname - // would produce, rather than erroring or returning an empty string. - assert_eq!( - default_node_name(NodeNameSource::KubeletCert), - default_node_name(NodeNameSource::Hostname) + // 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(); + assert!(format!("{err:#}").contains("failed to read"), "{err:#}"); + } + + #[test] + fn default_node_name_hostname_source_always_succeeds() { + assert!(!default_node_name(NodeNameSource::Hostname) + .unwrap() + .is_empty()); + } + + #[test] + fn missing_cert_with_kubelet_cert_source_is_a_fatal_config_error() { + let err = AgentConfig::from_vars(vars(&[( + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", + "kubelet-cert", + )])) + .unwrap_err(); + // No TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME override set, and + // DEFAULT_KUBELET_CLIENT_CERT won't exist on a dev/test machine - + // from_vars must propagate node_name resolution failure as a fatal + // config error, not silently fall back to hostname. + assert!( + format!("{err:#}").contains("failed to determine node_name"), + "{err:#}" ); } } diff --git a/docs/Development/Testing/TridentAclAgent-Tests.md b/docs/Development/Testing/TridentAclAgent-Tests.md index 69fa2380b..28f30992c 100644 --- a/docs/Development/Testing/TridentAclAgent-Tests.md +++ b/docs/Development/Testing/TridentAclAgent-Tests.md @@ -274,12 +274,15 @@ The scenario runs these test cases in order: `kubelet-cert`) actually switches which identity `node_name` resolves from, via one-shot `trident-acl-agent --validate-connection kubernetes` CLI invocations against a self-signed fake kubelet client cert — no - service restart or reboot is involved, so this stays fast. Checks that - `kubelet-cert` resolves to the cert's Subject CN, that `hostname` (and - the unset default) instead resolves to the VM's real hostname, that an - explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` still wins regardless - of source, and that an unreadable cert degrades `kubelet-cert` to the - hostname behavior with a logged warning rather than erroring out + service restart or reboot is involved, so this stays fast. Runs as three + phases, each with its own single-node fake apiserver seeded with the + identity that phase expects, asserting the *exact* resolved Node name: + `kubelet-cert` resolves to the cert's Subject CN; `hostname` (and the + unset default) resolves to the VM's real hostname; an explicit + `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` wins regardless of source; and + an unreadable cert with `kubelet-cert` source is a **fatal config + error** (deliberately no fallback to hostname — resolving to the wrong + Node silently would be worse than refusing to start) 5. **run-ab-update** — Starts the fake apiserver and fake Nebraska/Omaha endpoints in-process (the latter over HTTPS — see [Nebraska/Image Server TLS](#nebraskaimage-server-tls)), delivers a fake kubeconfig and restarts diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 2370dc244..32b98fdd5 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -261,7 +261,7 @@ naming the offending variable. | `TRIDENT_ACL_AGENT_KUBERNETES_API_SERVER` | unset | Explicit override for the Kubernetes API server URL. When unset, the server embedded in the kubeconfig is used as-is. | | `TRIDENT_ACL_AGENT_KUBERNETES_KUBECONFIG` | `/var/lib/kubelet/kubeconfig` | Path to the kubeconfig used to reach the Kubernetes API server and authenticate as this node. | | `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` | derived from `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | The Node object this agent watches/patches. | -| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | `hostname` | How `node_name` is derived when `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` is unset. `hostname` uses the node's own hostname, lowercased — matches AKS's current (but not guaranteed) convention of registering Nodes under the lowercased hostname. `kubelet-cert` instead reads the Subject CN (`system:node:`) from kubelet's own client certificate (`/var/lib/kubelet/pki/kubelet-client-current.pem`) — the identity kubelet itself authenticates to the API server with, so it's guaranteed to match the real Node name. If the cert can't be read or parsed, the agent logs a warning and falls back to the hostname behavior instead of failing startup. | +| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | `hostname` | How `node_name` is derived when `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` is unset. `hostname` uses the node's own hostname, lowercased — matches AKS's current (but not guaranteed) convention of registering Nodes under the lowercased hostname. `kubelet-cert` instead reads the Subject CN (`system:node:`) from kubelet's own client certificate (`/var/lib/kubelet/pki/kubelet-client-current.pem`) — the identity kubelet itself authenticates to the API server with, so it's guaranteed to match the real Node name. If the cert can't be read or parsed, the agent fails to start (a fatal config error) rather than silently falling back to the hostname behavior — resolving to the *wrong* Node is worse than refusing to start. Set `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` explicitly to override either source. | | `TRIDENT_ACL_AGENT_TRIDENT_SOCKET` | `unix:///run/trident/trident.sock` | The gRPC Unix socket URI used to reach `tridentd`. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_STATE_PATH` | `/var/lib/trident-acl-agent/state.json` | Path to the agent's persistent state file bridging the pre-reboot and post-reboot halves of `finalize`/`rollback` across the reboot. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_STAGE_TIMEOUT` | `20m` | How long a `stage` is allowed to run (a [`humantime`](https://docs.rs/humantime) duration, e.g. `20m`, `1h`) before it's considered failed. | diff --git a/tools/storm/aclagent/README.md b/tools/storm/aclagent/README.md index 07b6afd62..c237b62ca 100644 --- a/tools/storm/aclagent/README.md +++ b/tools/storm/aclagent/README.md @@ -152,19 +152,25 @@ Proves `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` (microsoft/trident#839) actually switches which identity `node_name` resolves from, entirely via one-shot `trident-acl-agent --validate-connection kubernetes` CLI invocations — it never touches `trident-acl-agent.service` (no restart, no -reboot), so it runs fast relative to the rest of this scenario. It seeds the -fake apiserver with a single Node whose name matches a self-signed fake -kubelet client cert's Subject CN (`system:node:`), uploaded to -`/var/lib/kubelet/pki/kubelet-client-current.pem`, and checks: - -- `kubelet-cert` source resolves to that cert's CN and succeeds -- `hostname` source (and the unset/default case) resolves to the VM's real - hostname instead, which this fake apiserver doesn't serve, and fails -- an explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` still wins outright - regardless of `NODE_NAME_SOURCE` -- with the cert hidden, `kubelet-cert` source degrades to the hostname - behavior (same failure shape as above) and logs a "falling back to - hostname" warning, rather than erroring out some other way +reboot), so it runs fast relative to the rest of this scenario. The fake +apiserver only ever serves a single Node name at a time, so this runs as +three sequential phases, each with its own apiserver instance seeded with +exactly the one identity that phase expects, and asserts the *exact* +resolved Node name reported in `--validate-connection`'s own success output +(not just pass/fail): + +- **kubelet-cert phase**: a self-signed fake kubelet client cert (Subject CN + `system:node:`) is uploaded to + `/var/lib/kubelet/pki/kubelet-client-current.pem`; `kubelet-cert` source + resolves to that cert's CN +- **hostname phase**: the VM's real hostname is read live via SSH; both + `hostname` source (explicit) and the unset/default case resolve to it. + With the cert hidden, `kubelet-cert` source is a **fatal config error** + (not a fallback to hostname — resolving to the wrong Node silently would + be worse than refusing to start) +- **explicit override phase**: `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` set + to a third, unrelated literal still wins outright regardless of + `NODE_NAME_SOURCE` ## `run-rollback` diff --git a/tools/storm/aclagent/tests/node_name_source.go b/tools/storm/aclagent/tests/node_name_source.go index 487fb2fc0..963727e8d 100644 --- a/tools/storm/aclagent/tests/node_name_source.go +++ b/tools/storm/aclagent/tests/node_name_source.go @@ -98,8 +98,11 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon } // --- Phase 2: hostname source (explicit and default) resolves to the - // VM's own real hostname, and a missing/unreadable cert degrades - // kubelet-cert source to that exact same resolution --- + // VM's own real hostname. kubelet-cert source with a + // missing/unreadable cert is a separate, fatal config error - NOT + // a fallback to hostname (see NodeNameSource::KubeletCert's own doc + // comment in config.rs) - checked in the same phase since it needs + // the same cert-hide/restore plumbing. --- realHostname, err := readVmHostname(vmConfig.VMConfig, vmIP) if err != nil { return err @@ -115,12 +118,12 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon return fmt.Errorf("default (unset NODE_NAME_SOURCE) source: %w", err) } - // An unreadable/missing cert must make kubelet-cert source degrade - // to the hostname behavior (logging a warning), not error out - // outright - confirmed by asserting the very same successful - // resolution to realHostname as above, plus the fallback warning - // actually being logged (trident-acl-agent's default --verbosity - // is Debug, so a Warn-level log is never filtered out here). + // An unreadable/missing cert with kubelet-cert source explicitly + // requested must be a fatal config error - deliberately NO + // fallback to hostname (see NodeNameSource::KubeletCert's doc + // comment in config.rs: resolving to the *wrong* Node silently is + // worse than refusing to start, the same reasoning the agent + // already applies to its own Node disappearing/NodeGone). if _, err := stormssh.SshCommandCombinedOutput(vmConfig.VMConfig, vmIP, fmt.Sprintf("sudo mv %s %s.bak", kubeletClientCertPath, kubeletClientCertPath)); err != nil { return fmt.Errorf("failed to hide kubelet client cert: %w", err) } @@ -129,11 +132,22 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon fmt.Fprintf(os.Stderr, "run-node-name-source: %v\n", err) } }() - return expectValidateConnectionOutput(vmConfig.VMConfig, vmIP, map[string]string{ + _, err := runValidateConnectionKubernetes(vmConfig.VMConfig, vmIP, map[string]string{ "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", - }, "falling back to hostname", fmt.Sprintf("fetched Node %q", realHostname)) + }) + if err == nil { + return fmt.Errorf("kubelet-cert source with missing cert: expected a fatal config error, got success") + } + // stormssh.SshCommandCombinedOutput discards the captured remote + // output on a non-zero exit - see runValidateConnectionKubernetes's + // own doc comment - so the actual stderr only survives inside + // err.Error(). + if !strings.Contains(err.Error(), "failed to determine node_name") { + return fmt.Errorf("kubelet-cert source with missing cert: expected a 'failed to determine node_name' config error, got: %w", err) + } + return nil }); err != nil { - return fmt.Errorf("phase hostname/missing-cert-fallback: %w", err) + return fmt.Errorf("phase hostname/missing-cert-is-fatal: %w", err) } // --- Phase 3: an explicit TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME must From 777fb02eaa8d5ef8d3b750107aa9f54d43f5d94d Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Fri, 9 Oct 2026 23:42:46 +0000 Subject: [PATCH 6/6] acl-agent: retry node_name resolution with backoff, matching NodeGone 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, 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, 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> --- .../src/annotations/orchestrator.rs | 180 +++++++++++++++--- .../trident-acl-agent/src/connection_check.rs | 18 +- crates/trident-acl-agent/src/core/config.rs | 167 ++++++++++------ crates/trident-acl-agent/src/main.rs | 9 +- .../Testing/TridentAclAgent-Tests.md | 13 +- docs/Explanation/Trident-ACL-Agent.md | 2 +- tools/storm/aclagent/README.md | 11 +- .../storm/aclagent/tests/node_name_source.go | 32 ++-- 8 files changed, 326 insertions(+), 106 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/orchestrator.rs b/crates/trident-acl-agent/src/annotations/orchestrator.rs index f2e6b61df..d1236fd42 100644 --- a/crates/trident-acl-agent/src/annotations/orchestrator.rs +++ b/crates/trident-acl-agent/src/annotations/orchestrator.rs @@ -33,7 +33,7 @@ use crate::{ SCHEMA_VERSION, }, core::{ - config::AgentConfig, + config::{AgentConfig, KubernetesConfig}, nebraska::{CheckOutcome, Client as NebraskaClient, ProgressEvent}, trident::{CompletedResponse, TridentClient, TridentClientError}, version::{current_active_version, FALLBACK_ALWAYS_VERSION}, @@ -106,21 +106,86 @@ impl NebraskaReport { pub struct Orchestrator { config: AgentConfig, + /// The agent's own Node name, resolved once at construction time (see + /// `from_config`'s retry-with-backoff resolution) and held fixed for + /// the orchestrator's whole lifetime - the identity this process + /// reconciles against doesn't change without a restart. + node_name: String, k8s: NodeClient, state: StateStore, annotation_keys: AnnotationKeys, } impl Orchestrator { - pub async fn from_config(config: AgentConfig) -> Result { + /// Builds the orchestrator, including resolving `node_name` - retrying + /// indefinitely with the same capped-exponential-backoff-with-jitter + /// policy as `get_node_with_retry`/`await_node_recreation` if + /// `node_name_source` is `kubelet-cert` and the cert isn't readable yet + /// (see `resolve_node_name_with_retry`). Returns `Ok(None)` if + /// `shutdown` fires during that retry, mirroring + /// `recover_from_trident_state`/`await_node_recreation`'s own + /// `Ok(None)`/`Ok(false)` so `main.rs` can exit promptly instead of + /// blocking indefinitely. + pub async fn from_config( + config: AgentConfig, + shutdown: &CancellationToken, + ) -> Result, Error> { + let Some(node_name) = + Self::resolve_node_name_with_retry(&config.kubernetes, shutdown).await? + else { + return Ok(None); + }; let k8s = NodeClient::new(&config.kubernetes).await?; let annotation_keys = AnnotationKeys::new(&config.kubernetes.annotation_prefix); - Ok(Self { + Ok(Some(Self { state: StateStore::new(config.orchestration.state_path.clone()), config, + node_name, k8s, annotation_keys, - }) + })) + } + + /// Resolves `node_name` once, retrying indefinitely (capped exponential + /// backoff + jitter, same `NODE_READ_BACKOFF`/`NODE_READ_BACKOFF_MAX` + /// policy as `get_node_with_retry`) if resolution fails - e.g. + /// `node_name_source=kubelet-cert` and kubelet hasn't finished writing + /// its client cert yet. Treated the same way a missing Node object + /// (`NodeGone`) already is: a plausible, expected startup race to wait + /// out, not a reason to crash the process. Races every sleep against + /// `shutdown`, returning `Ok(None)` if it fires first. + async fn resolve_node_name_with_retry( + kubernetes: &KubernetesConfig, + shutdown: &CancellationToken, + ) -> Result, Error> { + let mut backoff = ExponentialBackoffBuilder::new() + .with_initial_interval(NODE_READ_BACKOFF) + .with_max_interval(NODE_READ_BACKOFF_MAX) + .with_multiplier(2.0) + .with_randomization_factor(1.0) + .with_max_elapsed_time(None) + .build(); + loop { + match kubernetes.resolve_node_name_once() { + Ok(node_name) => return Ok(Some(node_name)), + Err(err) => { + // `max_elapsed_time(None)` means `next_backoff()` never + // returns `None` in practice, but fall back to the cap + // instead of unwrapping, so this can't panic even if + // that invariant ever changes. + let delay = backoff.next_backoff().unwrap_or(NODE_READ_BACKOFF_MAX); + warn!( + "failed to resolve node_name (node_name_source={:?}), retrying in \ + {delay:?}: {err:#}", + kubernetes.node_name_source + ); + select! { + _ = shutdown.cancelled() => return Ok(None), + _ = time::sleep(delay) => {} + } + } + } + } } /// Issues a real `systemctl reboot`. Routed through the repo's @@ -187,9 +252,7 @@ impl Orchestrator { return Err(err); } } - let mut stream = self - .k8s - .watch_node(self.config.kubernetes.node_name.clone()); + let mut stream = self.k8s.watch_node(self.node_name.clone()); // Watch-stream errors are tolerated indefinitely, so a transient // reconnect hiccup doesn't abort the whole orchestrator the way a // bare `node?` would. No extra sleep here (unlike @@ -220,7 +283,7 @@ impl Orchestrator { _ = shutdown.cancelled() => { info!( "shutdown requested while watching node {}", - self.config.kubernetes.node_name + self.node_name ); return Ok(()); } @@ -243,7 +306,7 @@ impl Orchestrator { errors_since_last_node_event += 1; warn!( "transient error watching node {} (errors since last node event: {errors_since_last_node_event}): {err:#}", - self.config.kubernetes.node_name + self.node_name ); continue; } @@ -254,7 +317,7 @@ impl Orchestrator { "shutdown requested while reconciling node {}; exiting (any \ in-flight stage/finalize/rollback/commit resumes from \ state.json on next start)", - self.config.kubernetes.node_name + self.node_name ); return Ok(()); } @@ -299,7 +362,7 @@ impl Orchestrator { /// `NODE_READ_BACKOFF_MAX`, an acceptable idle cost in exchange for not /// permanently killing the agent on a transient 404 or a delete+recreate. async fn await_node_recreation(&self, shutdown: &CancellationToken) -> Result { - let node_name = &self.config.kubernetes.node_name; + let node_name = &self.node_name; info!("node {node_name} no longer exists; waiting for it to reappear"); let started = time::Instant::now(); let mut last_announced = started; @@ -424,10 +487,7 @@ impl Orchestrator { return Ok(Some(LoopControl::Continue)); } - let Some(node) = self - .get_node_with_retry(&self.config.kubernetes.node_name, shutdown) - .await? - else { + let Some(node) = self.get_node_with_retry(&self.node_name, shutdown).await? else { return Ok(None); // shutdown requested while retrying }; let snapshot = Snapshot::from_node(&node, &self.annotation_keys); @@ -1406,14 +1466,10 @@ impl Orchestrator { ); info!( "sending {annotation_key} annotation to node {}: {status:?}", - self.config.kubernetes.node_name + self.node_name ); self.k8s - .patch_node_metadata( - &self.config.kubernetes.node_name, - BTreeMap::new(), - annotations, - ) + .patch_node_metadata(&self.node_name, BTreeMap::new(), annotations) .await?; Ok(()) } @@ -1425,7 +1481,7 @@ impl Orchestrator { Err(err) if self.is_node_gone_error(&err) => { info!( "stopping terminal status publish because node {} no longer exists", - self.config.kubernetes.node_name + self.node_name ); return; } @@ -1512,7 +1568,7 @@ impl Orchestrator { if self.is_node_gone_error(&err) { info!( "stopping heartbeats because node {} no longer exists", - self.config.kubernetes.node_name + self.node_name ); stop_heartbeats = true; } else { @@ -2216,9 +2272,85 @@ mod tests { const MOCK_RPC_TIMEOUT: Duration = Duration::from_secs(5); use crate::{ annotations::{RequestedOperation, SCHEMA_VERSION}, - core::trident::mock::{connect_mock_client, MockTridentdConfig, Outcome}, + core::{ + config::NodeNameSource, + trident::mock::{connect_mock_client, MockTridentdConfig, Outcome}, + }, }; + // --- node_name resolution retry (microsoft/trident#839 follow-up) --- + + /// Proves resolve_node_name_with_retry actually *retries* (keeps + /// looping/sleeping) rather than bailing out with an error on the + /// first failed resolution attempt - the whole point of treating an + /// unreadable kubelet-cert as a transient startup race, analogous to + /// the Node object itself not existing yet, rather than a fatal + /// condition. DEFAULT_KUBELET_CLIENT_CERT won't exist on this test + /// machine, so KubeletCert source fails deterministically every + /// attempt; cancelling shutdown shortly after starting (well under + /// NODE_READ_BACKOFF's 2s initial interval) and asserting Ok(None) - + /// not an Err - proves the loop was still patiently retrying (parked + /// in its backoff sleep) rather than having already given up. + #[tokio::test] + async fn resolve_node_name_with_retry_keeps_retrying_instead_of_erroring() { + let kubernetes = KubernetesConfig { + node_name_source: NodeNameSource::KubeletCert, + ..Default::default() + }; + let shutdown = CancellationToken::new(); + let shutdown_clone = shutdown.clone(); + tokio::spawn(async move { + time::sleep(Duration::from_millis(50)).await; + shutdown_clone.cancel(); + }); + let result = Orchestrator::resolve_node_name_with_retry(&kubernetes, &shutdown).await; + assert!( + matches!(result, Ok(None)), + "expected Ok(None) (shutdown interrupted an in-progress retry), got {result:?}" + ); + } + + /// Hostname source must resolve immediately on the very first attempt - + /// no retry loop, no backoff sleep - since hostname_node_name() always + /// succeeds (it has its own internal fallback). Bounds the whole call + /// well under NODE_READ_BACKOFF's 2s initial interval to prove no sleep + /// was entered at all. + #[tokio::test] + async fn resolve_node_name_with_retry_hostname_source_resolves_without_retrying() { + let kubernetes = KubernetesConfig::default(); // Hostname is the default source + let shutdown = CancellationToken::new(); + let result = time::timeout( + Duration::from_millis(500), + Orchestrator::resolve_node_name_with_retry(&kubernetes, &shutdown), + ) + .await + .expect("must resolve well under one NODE_READ_BACKOFF interval") + .unwrap(); + assert!(matches!(result, Some(name) if !name.is_empty())); + } + + /// An explicit override must resolve immediately too, regardless of + /// node_name_source - same "no retry needed" guarantee as the hostname + /// case above, via a different code path + /// (KubernetesConfig::resolve_node_name_once's override branch). + #[tokio::test] + async fn resolve_node_name_with_retry_explicit_override_resolves_without_retrying() { + let kubernetes = KubernetesConfig { + node_name_override: Some("node-42".to_string()), + node_name_source: NodeNameSource::KubeletCert, + ..Default::default() + }; + let shutdown = CancellationToken::new(); + let result = time::timeout( + Duration::from_millis(500), + Orchestrator::resolve_node_name_with_retry(&kubernetes, &shutdown), + ) + .await + .expect("must resolve well under one NODE_READ_BACKOFF interval") + .unwrap(); + assert_eq!(result, Some("node-42".to_string())); + } + fn request(operation: RequestedOperation) -> UpdateRequest { UpdateRequest { schema_version: SCHEMA_VERSION.to_string(), diff --git a/crates/trident-acl-agent/src/connection_check.rs b/crates/trident-acl-agent/src/connection_check.rs index 62f04f00f..b9296d36f 100644 --- a/crates/trident-acl-agent/src/connection_check.rs +++ b/crates/trident-acl-agent/src/connection_check.rs @@ -25,24 +25,28 @@ pub async fn validate_connection( let client = NodeClient::new(&config.kubernetes) .await .context("failed to build Kubernetes client")?; + // Single, fail-fast attempt - appropriate for a one-shot + // diagnostic (unlike the long-running orchestrator, which + // retries a transient kubelet-cert failure with backoff; see + // KubernetesConfig::resolve_node_name_once's doc comment). + let node_name = config + .kubernetes + .resolve_node_name_once() + .context("failed to resolve node_name")?; // Report the actually-resolved server (kubeconfig's own server, // unless overridden by kubernetes.api_server), not a value // guessed from config - the two only match when an override is // set. let cluster_url = client.cluster_url(); client - .get_node(&config.kubernetes.node_name) + .get_node(&node_name) .await .with_context(|| { format!( - "failed to reach Kubernetes API server at {cluster_url} (get Node {:?})", - config.kubernetes.node_name + "failed to reach Kubernetes API server at {cluster_url} (get Node {node_name:?})" ) })?; - info!( - "kubernetes: reached API server at {cluster_url} and fetched Node {:?}", - config.kubernetes.node_name - ); + info!("kubernetes: reached API server at {cluster_url} and fetched Node {node_name:?}"); } ConnectionTarget::Tridentd => { TridentClient::connect(&config.trident.socket) diff --git a/crates/trident-acl-agent/src/core/config.rs b/crates/trident-acl-agent/src/core/config.rs index 9d27110bc..a6920d12f 100644 --- a/crates/trident-acl-agent/src/core/config.rs +++ b/crates/trident-acl-agent/src/core/config.rs @@ -119,30 +119,17 @@ impl AgentConfig { .track .unwrap_or_else(|| DEFAULT_NEBRASKA_TRACK.to_string()), }, - kubernetes: { - let node_name_source = kubernetes.node_name_source.unwrap_or_default(); - let node_name = match kubernetes.node_name { - Some(name) => name, - None => default_node_name(node_name_source).with_context(|| { - format!( - "failed to determine node_name for \ - {ENV_PREFIX_KUBERNETES}NODE_NAME_SOURCE={node_name_source:?}; set \ - {ENV_PREFIX_KUBERNETES}NODE_NAME explicitly to override" - ) - })?, - }; - KubernetesConfig { - api_server: kubernetes.api_server, - kubeconfig: kubernetes - .kubeconfig - .unwrap_or_else(|| PathBuf::from(DEFAULT_KUBELET_KUBECONFIG)), - node_name, - node_name_source, - watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, - annotation_prefix: kubernetes - .annotation_prefix - .unwrap_or_else(|| DEFAULT_ANNOTATION_PREFIX.to_string()), - } + kubernetes: KubernetesConfig { + api_server: kubernetes.api_server, + kubeconfig: kubernetes + .kubeconfig + .unwrap_or_else(|| PathBuf::from(DEFAULT_KUBELET_KUBECONFIG)), + node_name_override: kubernetes.node_name, + node_name_source: kubernetes.node_name_source.unwrap_or_default(), + watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, + annotation_prefix: kubernetes + .annotation_prefix + .unwrap_or_else(|| DEFAULT_ANNOTATION_PREFIX.to_string()), }, trident: TridentConfig { socket: trident @@ -322,11 +309,21 @@ pub struct KubernetesConfig { /// kubeconfig has no reason to contain. pub api_server: Option, pub kubeconfig: PathBuf, - pub node_name: String, - /// Which source `node_name` was (or would be) derived from when no - /// explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` override is set. - /// See [`NodeNameSource`] for the two options and why hostname remains - /// the default despite not being AKS-guaranteed. + /// Explicit override for `node_name` (`TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME`). + /// When unset, [`Self::resolve_node_name_once`] derives it from + /// `node_name_source` instead. Deliberately *not* resolved eagerly at + /// config-load time: the long-running orchestrator needs to retry a + /// transient `kubelet-cert` resolution failure with backoff (treating + /// it like a startup race, analogous to the Node object itself not + /// existing yet) rather than fail the whole process outright, while + /// `--validate-connection kubernetes` wants the opposite - a single, + /// fail-fast attempt. Each caller decides which by how it drives + /// `resolve_node_name_once`, not by config loading itself. + pub node_name_override: Option, + /// Which source `node_name` is derived from when + /// `node_name_override` is unset. See [`NodeNameSource`] for the two + /// options and why hostname remains the default despite not being + /// AKS-guaranteed. pub node_name_source: NodeNameSource, pub watch_poll_interval: Duration, /// Annotation-key prefix used for the request/status/commit-status @@ -338,18 +335,31 @@ pub struct KubernetesConfig { pub annotation_prefix: String, } +impl KubernetesConfig { + /// Resolves the effective `node_name`: `node_name_override` if set, + /// otherwise a single, non-retried attempt via `node_name_source`. This + /// is the fail-fast building block both callers share - + /// `--validate-connection kubernetes` uses this directly (a single bad + /// attempt should report failure immediately), while the long-running + /// orchestrator wraps repeated calls to this same method in its own + /// capped-exponential-backoff retry loop (see + /// `Orchestrator::resolve_node_name_with_retry`) so a transient + /// `kubelet-cert` failure - e.g. kubelet hasn't finished writing the + /// cert yet at boot - doesn't crash the whole agent. + pub fn resolve_node_name_once(&self) -> Result { + match &self.node_name_override { + Some(name) => Ok(name.clone()), + None => default_node_name(self.node_name_source), + } + } +} + impl Default for KubernetesConfig { fn default() -> Self { - // Calls hostname_node_name() directly rather than going through - // the now-fallible default_node_name(): NodeNameSource::default() - // is always Hostname, which hostname_node_name() can always - // satisfy (it has its own internal fallback to DEFAULT_NODE_NAME - // on a hostname-read failure), so this Default impl never needs - // the KubeletCert branch that can actually return Err. Self { api_server: None, kubeconfig: PathBuf::from(DEFAULT_KUBELET_KUBECONFIG), - node_name: hostname_node_name(), + node_name_override: None, node_name_source: NodeNameSource::default(), watch_poll_interval: DEFAULT_KUBERNETES_POLL_INTERVAL, annotation_prefix: DEFAULT_ANNOTATION_PREFIX.to_string(), @@ -425,13 +435,23 @@ pub enum NodeNameSource { /// `system:node:` prefix. This is the identity kubelet itself /// authenticates to the API server with, so it is guaranteed to match /// the real Node name regardless of hostname conventions. Opt in with - /// `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE=kubelet-cert`. If the - /// cert can't be read or parsed, config loading fails outright (same - /// severity as any other malformed/unsatisfiable config) - deliberately - /// no fallback to `Hostname`: an operator who opted into this source - /// did so because hostname is *not* a trustworthy Node identity on - /// their platform, so silently substituting it back in on cert failure - /// would risk the agent reconciling against the wrong Node entirely. + /// `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE=kubelet-cert`. + /// + /// Deliberately no fallback to `Hostname` if the cert can't be read or + /// parsed: an operator who opted into this source did so because + /// hostname is *not* a trustworthy Node identity on their platform, so + /// silently substituting it back in on cert failure would risk the + /// agent reconciling against the wrong Node entirely. Instead, the + /// long-running orchestrator treats an unreadable cert as a startup + /// race analogous to the Node object itself not existing yet - e.g. + /// kubelet hasn't finished its own TLS bootstrapping and written the + /// cert before this agent started - and retries resolution with the + /// same capped-exponential-backoff policy used for a missing Node (see + /// `Orchestrator::resolve_node_name_with_retry`), rather than crashing + /// the process. `--validate-connection kubernetes`, by contrast, makes + /// a single fail-fast attempt via + /// [`KubernetesConfig::resolve_node_name_once`] and reports failure + /// immediately - appropriate for a one-shot diagnostic. KubeletCert, } @@ -659,7 +679,10 @@ mod tests { config.kubernetes.kubeconfig, PathBuf::from("/etc/trident-acl-agent/kubeconfig") ); - assert_eq!(config.kubernetes.node_name, "node-42"); + assert_eq!( + config.kubernetes.node_name_override.as_deref(), + Some("node-42") + ); assert_eq!( config.kubernetes.node_name_source, NodeNameSource::KubeletCert @@ -856,19 +879,55 @@ gf0TbLqnNti3MeMftKcEa8VQ2rE6Np02EETxr94lsiNK } #[test] - fn missing_cert_with_kubelet_cert_source_is_a_fatal_config_error() { - let err = AgentConfig::from_vars(vars(&[( + fn config_loading_succeeds_even_when_kubelet_cert_is_unreadable() { + // Unlike the old design, AgentConfig::from_vars must NOT fail just + // because node_name_source=kubelet-cert and the cert can't be read + // yet - resolution is deferred to resolve_node_name_once/ + // resolve_node_name_with_retry (see KubernetesConfig's doc + // comments), so config loading itself stays infallible for this. + let config = AgentConfig::from_vars(vars(&[( "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", "kubelet-cert", )])) - .unwrap_err(); - // No TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME override set, and - // DEFAULT_KUBELET_CLIENT_CERT won't exist on a dev/test machine - - // from_vars must propagate node_name resolution failure as a fatal - // config error, not silently fall back to hostname. - assert!( - format!("{err:#}").contains("failed to determine node_name"), - "{err:#}" + .unwrap(); + assert_eq!( + config.kubernetes.node_name_source, + NodeNameSource::KubeletCert + ); + assert_eq!(config.kubernetes.node_name_override, None); + } + + #[test] + fn resolve_node_name_once_errors_when_kubelet_cert_is_unreadable() { + // DEFAULT_KUBELET_CLIENT_CERT won't exist on a dev/test machine, so + // requesting kubelet-cert source with no override must error out - + // no silent fallback to hostname (see NodeNameSource::KubeletCert's + // doc comment for why). + let config = AgentConfig::from_vars(vars(&[( + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", + "kubelet-cert", + )])) + .unwrap(); + let err = config.kubernetes.resolve_node_name_once().unwrap_err(); + assert!(format!("{err:#}").contains("failed to read"), "{err:#}"); + } + + #[test] + fn resolve_node_name_once_prefers_explicit_override_over_source() { + // Even with node_name_source=kubelet-cert (which would otherwise + // read an unreadable cert and error), an explicit override must + // win outright and never even attempt source-based resolution. + let config = AgentConfig::from_vars(vars(&[ + ("TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME", "node-42"), + ( + "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE", + "kubelet-cert", + ), + ])) + .unwrap(); + assert_eq!( + config.kubernetes.resolve_node_name_once().unwrap(), + "node-42" ); } } diff --git a/crates/trident-acl-agent/src/main.rs b/crates/trident-acl-agent/src/main.rs index a57251850..e09b39168 100644 --- a/crates/trident-acl-agent/src/main.rs +++ b/crates/trident-acl-agent/src/main.rs @@ -122,7 +122,14 @@ async fn main() -> Result<(), Error> { } }); } - Orchestrator::from_config(config).await?.run(shutdown).await + match Orchestrator::from_config(config, &shutdown).await? { + Some(orchestrator) => orchestrator.run(shutdown).await, + // shutdown fired while retrying node_name resolution (e.g. + // waiting on kubelet's cert to be written) - exit cleanly, + // mirroring Orchestrator::run's own early-exit behavior for + // the same signal. + None => Ok(()), + } } } } diff --git a/docs/Development/Testing/TridentAclAgent-Tests.md b/docs/Development/Testing/TridentAclAgent-Tests.md index 28f30992c..4cd440222 100644 --- a/docs/Development/Testing/TridentAclAgent-Tests.md +++ b/docs/Development/Testing/TridentAclAgent-Tests.md @@ -280,9 +280,16 @@ The scenario runs these test cases in order: `kubelet-cert` resolves to the cert's Subject CN; `hostname` (and the unset default) resolves to the VM's real hostname; an explicit `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` wins regardless of source; and - an unreadable cert with `kubelet-cert` source is a **fatal config - error** (deliberately no fallback to hostname — resolving to the wrong - Node silently would be worse than refusing to start) + an unreadable cert with `kubelet-cert` source makes this one-shot + diagnostic fail immediately (fail-fast, no fallback to hostname — see + [Trident ACL Agent](../../Explanation/Trident-ACL-Agent.md)). The + long-running agent's different behavior for that same condition - + retrying indefinitely with capped exponential backoff instead of + crashing, the same way it already waits out its own Node object not + existing yet - is covered by Rust unit tests + (`resolve_node_name_with_retry` in `orchestrator.rs`) rather than this + VM scenario, since proving an indefinite retry loop needs a controllable + cert path, not a real filesystem race 5. **run-ab-update** — Starts the fake apiserver and fake Nebraska/Omaha endpoints in-process (the latter over HTTPS — see [Nebraska/Image Server TLS](#nebraskaimage-server-tls)), delivers a fake kubeconfig and restarts diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 32b98fdd5..45b45efeb 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -261,7 +261,7 @@ naming the offending variable. | `TRIDENT_ACL_AGENT_KUBERNETES_API_SERVER` | unset | Explicit override for the Kubernetes API server URL. When unset, the server embedded in the kubeconfig is used as-is. | | `TRIDENT_ACL_AGENT_KUBERNETES_KUBECONFIG` | `/var/lib/kubelet/kubeconfig` | Path to the kubeconfig used to reach the Kubernetes API server and authenticate as this node. | | `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` | derived from `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | The Node object this agent watches/patches. | -| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | `hostname` | How `node_name` is derived when `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` is unset. `hostname` uses the node's own hostname, lowercased — matches AKS's current (but not guaranteed) convention of registering Nodes under the lowercased hostname. `kubelet-cert` instead reads the Subject CN (`system:node:`) from kubelet's own client certificate (`/var/lib/kubelet/pki/kubelet-client-current.pem`) — the identity kubelet itself authenticates to the API server with, so it's guaranteed to match the real Node name. If the cert can't be read or parsed, the agent fails to start (a fatal config error) rather than silently falling back to the hostname behavior — resolving to the *wrong* Node is worse than refusing to start. Set `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` explicitly to override either source. | +| `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE` | `hostname` | How `node_name` is derived when `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` is unset. `hostname` uses the node's own hostname, lowercased — matches AKS's current (but not guaranteed) convention of registering Nodes under the lowercased hostname. `kubelet-cert` instead reads the Subject CN (`system:node:`) from kubelet's own client certificate (`/var/lib/kubelet/pki/kubelet-client-current.pem`) — the identity kubelet itself authenticates to the API server with, so it's guaranteed to match the real Node name. There is deliberately no fallback to the hostname behavior if the cert can't be read or parsed — resolving to the *wrong* Node is worse than waiting. Instead, the long-running agent treats this the same way it treats its own Node object not existing yet: it retries indefinitely with capped exponential backoff (same policy used while waiting for the Node to reappear), since an unreadable cert at startup is typically just kubelet's own TLS bootstrapping not having finished yet. `trident-acl-agent --validate-connection kubernetes`, by contrast, makes one fail-fast attempt and reports failure immediately — appropriate for a one-shot diagnostic. Set `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` explicitly to override either source. | | `TRIDENT_ACL_AGENT_TRIDENT_SOCKET` | `unix:///run/trident/trident.sock` | The gRPC Unix socket URI used to reach `tridentd`. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_STATE_PATH` | `/var/lib/trident-acl-agent/state.json` | Path to the agent's persistent state file bridging the pre-reboot and post-reboot halves of `finalize`/`rollback` across the reboot. | | `TRIDENT_ACL_AGENT_ORCHESTRATION_STAGE_TIMEOUT` | `20m` | How long a `stage` is allowed to run (a [`humantime`](https://docs.rs/humantime) duration, e.g. `20m`, `1h`) before it's considered failed. | diff --git a/tools/storm/aclagent/README.md b/tools/storm/aclagent/README.md index c237b62ca..3c6fa0097 100644 --- a/tools/storm/aclagent/README.md +++ b/tools/storm/aclagent/README.md @@ -165,9 +165,14 @@ resolved Node name reported in `--validate-connection`'s own success output resolves to that cert's CN - **hostname phase**: the VM's real hostname is read live via SSH; both `hostname` source (explicit) and the unset/default case resolve to it. - With the cert hidden, `kubelet-cert` source is a **fatal config error** - (not a fallback to hostname — resolving to the wrong Node silently would - be worse than refusing to start) + With the cert hidden, `kubelet-cert` source makes + `--validate-connection kubernetes` fail immediately (fail-fast is correct + for this one-shot CLI diagnostic — see the Rust-level + `resolve_node_name_with_retry` unit tests in `orchestrator.rs` for + coverage of the long-running agent's different behavior: it retries this + same condition indefinitely with capped exponential backoff instead of + crashing, since an unreadable cert at startup is typically just kubelet's + own TLS bootstrapping not having finished yet) - **explicit override phase**: `TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME` set to a third, unrelated literal still wins outright regardless of `NODE_NAME_SOURCE` diff --git a/tools/storm/aclagent/tests/node_name_source.go b/tools/storm/aclagent/tests/node_name_source.go index 963727e8d..24d6a9e2d 100644 --- a/tools/storm/aclagent/tests/node_name_source.go +++ b/tools/storm/aclagent/tests/node_name_source.go @@ -99,10 +99,10 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon // --- Phase 2: hostname source (explicit and default) resolves to the // VM's own real hostname. kubelet-cert source with a - // missing/unreadable cert is a separate, fatal config error - NOT - // a fallback to hostname (see NodeNameSource::KubeletCert's own doc - // comment in config.rs) - checked in the same phase since it needs - // the same cert-hide/restore plumbing. --- + // missing/unreadable cert fails --validate-connection immediately - + // NOT a fallback to hostname (see NodeNameSource::KubeletCert's own + // doc comment in config.rs) - checked in the same phase since it + // needs the same cert-hide/restore plumbing. --- realHostname, err := readVmHostname(vmConfig.VMConfig, vmIP) if err != nil { return err @@ -119,11 +119,17 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon } // An unreadable/missing cert with kubelet-cert source explicitly - // requested must be a fatal config error - deliberately NO - // fallback to hostname (see NodeNameSource::KubeletCert's doc - // comment in config.rs: resolving to the *wrong* Node silently is - // worse than refusing to start, the same reasoning the agent - // already applies to its own Node disappearing/NodeGone). + // requested must fail --validate-connection immediately - + // deliberately NO fallback to hostname (see + // NodeNameSource::KubeletCert's doc comment in config.rs: resolving + // to the *wrong* Node silently is worse than refusing to start). + // Note this is specific to the one-shot --validate-connection CLI + // path (KubernetesConfig::resolve_node_name_once's single, + // fail-fast attempt) - the long-running orchestrator instead + // retries this same condition with exponential backoff, treating + // it like a startup race analogous to the Node object itself not + // existing yet (see Orchestrator::resolve_node_name_with_retry), + // which isn't exercised by this fast, non-service test case. if _, err := stormssh.SshCommandCombinedOutput(vmConfig.VMConfig, vmIP, fmt.Sprintf("sudo mv %s %s.bak", kubeletClientCertPath, kubeletClientCertPath)); err != nil { return fmt.Errorf("failed to hide kubelet client cert: %w", err) } @@ -136,18 +142,18 @@ func RunNodeNameSource(testConfig stormaclconfig.TestConfig, vmConfig stormvmcon "TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME_SOURCE": "kubelet-cert", }) if err == nil { - return fmt.Errorf("kubelet-cert source with missing cert: expected a fatal config error, got success") + return fmt.Errorf("kubelet-cert source with missing cert: expected validate-connection to fail, got success") } // stormssh.SshCommandCombinedOutput discards the captured remote // output on a non-zero exit - see runValidateConnectionKubernetes's // own doc comment - so the actual stderr only survives inside // err.Error(). - if !strings.Contains(err.Error(), "failed to determine node_name") { - return fmt.Errorf("kubelet-cert source with missing cert: expected a 'failed to determine node_name' config error, got: %w", err) + if !strings.Contains(err.Error(), "failed to resolve node_name") { + return fmt.Errorf("kubelet-cert source with missing cert: expected a 'failed to resolve node_name' error, got: %w", err) } return nil }); err != nil { - return fmt.Errorf("phase hostname/missing-cert-is-fatal: %w", err) + return fmt.Errorf("phase hostname/missing-cert-fails-fast: %w", err) } // --- Phase 3: an explicit TRIDENT_ACL_AGENT_KUBERNETES_NODE_NAME must