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/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 2f25af930..a6920d12f 100644 --- a/crates/trident-acl-agent/src/core/config.rs +++ b/crates/trident-acl-agent/src/core/config.rs @@ -24,10 +24,15 @@ //! 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 openssl::{nid::Nid, x509::X509}; use serde::{de::Error as _, Deserialize, Deserializer}; use trident_proto::TRIDENT_DEFAULT_SOCKET_URI; use url::Url; @@ -61,6 +66,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"; @@ -111,7 +124,8 @@ impl AgentConfig { kubeconfig: kubernetes .kubeconfig .unwrap_or_else(|| PathBuf::from(DEFAULT_KUBELET_KUBECONFIG)), - node_name: kubernetes.node_name.unwrap_or_else(default_node_name), + 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 @@ -166,6 +180,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 +270,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, @@ -282,7 +309,22 @@ pub struct KubernetesConfig { /// kubeconfig has no reason to contain. pub api_server: Option, pub kubeconfig: PathBuf, - pub node_name: String, + /// 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 /// annotations (e.g. `acl.microsoft.com` in @@ -293,12 +335,32 @@ 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 { Self { api_server: None, kubeconfig: PathBuf::from(DEFAULT_KUBELET_KUBECONFIG), - node_name: default_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(), } @@ -357,6 +419,56 @@ 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`. + /// + /// 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, +} + +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 +513,22 @@ 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) -> Result { + match source { + 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)) + } + } +} + +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 +541,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 +616,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 +641,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", @@ -520,7 +679,14 @@ 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 + ); assert_eq!(config.trident.socket, "unix:///custom/trident.sock"); assert_eq!(config.orchestration.mode, Mode::OmahaOnly); assert_eq!( @@ -600,4 +766,168 @@ 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_errors_when_cert_source_requested_but_unreadable() { + // DEFAULT_KUBELET_CLIENT_CERT won't exist on a dev/test machine, so + // requesting KubeletCert must error out - no silent fallback to + // Hostname (see NodeNameSource::KubeletCert's doc comment for why). + let err = default_node_name(NodeNameSource::KubeletCert).unwrap_err(); + 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 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(); + 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 e11dae9e5..4cd440222 100644 --- a/docs/Development/Testing/TridentAclAgent-Tests.md +++ b/docs/Development/Testing/TridentAclAgent-Tests.md @@ -269,22 +269,43 @@ 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. 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 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 `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/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 6777d721b..45b45efeb 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. 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 58d010a0c..3c6fa0097 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,38 @@ 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. 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 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` + + ## `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..24d6a9e2d --- /dev/null +++ b/tools/storm/aclagent/tests/node_name_source.go @@ -0,0 +1,311 @@ +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 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 +// `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. +// +// 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 { + 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() + + // 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 + } + + // --- 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) + } + + // --- Phase 2: hostname source (explicit and default) resolves to the + // VM's own real hostname. kubelet-cert source with a + // 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 + } + 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) + } + + // An unreadable/missing cert with kubelet-cert source explicitly + // 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) + } + defer func() { + if err := restoreKubeletClientCert(vmConfig.VMConfig, vmIP); err != nil { + fmt.Fprintf(os.Stderr, "run-node-name-source: %v\n", err) + } + }() + _, 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 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 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-fails-fast: %w", err) + } + + // --- 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("phase explicit node_name override: %w", err) + } + + return collectAclArtifacts(vmConfig.VMConfig, vmIP, testConfig.OutputPath) +} + +// 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() +} + +// 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, +// 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 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) + } + 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 +} + +// 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") + 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) }