From 7b374d8cf16654efc88829e6d3f58eca71cb9bd0 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Wed, 5 Aug 2026 21:20:39 +0000 Subject: [PATCH 1/3] trident-acl-agent: add --validate-connection for diagnosing a single dependency Adds a `--validate-connection ` CLI flag that checks reachability of exactly one dependency and exits immediately (0 on success, non-zero with a descriptive error on failure), instead of running the agent. Useful for on-node troubleshooting or a systemd ExecStartPre-style check without invoking the full orchestrator loop. Each target uses the simplest available check: - kubernetes: fetches this node's own Node object (the same get_node() call orchestrator.rs's reconcile loop already makes on startup). - tridentd: connects to its gRPC Unix socket. Connecting is sufficient proof of reachability - no RPC call is needed, since Endpoint::connect() fails immediately if nothing is listening. - nebraska: issues a real Omaha update-check query. Any well-formed response (including "no update available") counts as success; only a network/transport failure is treated as unreachable. Found and fixed along the way: query_for_update() is a blocking call (reqwest::blocking under the hood in omaha::send) - calling it directly from this async fn panics ("Cannot drop a runtime in a context where blocking is not allowed") because reqwest::blocking spins up its own inner Tokio runtime per call, which isn't safe to tear down from inside an already-running async task. Wrapped in tokio::task::spawn_blocking for the new nebraska check. Note: this same blocking-in-async pattern also exists in the pre-existing run_omaha_only() and orchestrator.rs::handle_stage() call sites - out of scope for this change, not touched here, but worth a follow-up if it's ever hit in practice. Verified: cargo test -p trident-acl-agent (78 passed), cargo clippy --all-targets -- -D warnings (clean), cargo fmt --check (clean). Manually exercised all three failure paths (no tridentd socket, no kubeconfig, no configured Nebraska endpoint - each exits 1 with a clear error) and both success paths achievable on this dev host (nebraska, against a local mock Omaha server; kubernetes' get_node() and tridentd's TridentClient::connect() are already exercised successfully by the full storm-trident E2E suite). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86 --- crates/trident-acl-agent/src/main.rs | 107 ++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/crates/trident-acl-agent/src/main.rs b/crates/trident-acl-agent/src/main.rs index 3c07aceec..d9ac103bd 100644 --- a/crates/trident-acl-agent/src/main.rs +++ b/crates/trident-acl-agent/src/main.rs @@ -1,12 +1,16 @@ use std::path::PathBuf; +use anyhow::Context; use clap::Parser; use log::{LevelFilter, Log, Metadata, Record}; use trident_acl_agent::{ config::{AgentConfig, GoalSource, DEFAULT_CONFIG_PATH}, + k8s::NodeClient, orchestrator::Orchestrator, - run_omaha_only, + query_for_update, run_omaha_only, + trident::TridentClient, + IdSource, DEFAULT_NEBRASKA_TRACK, }; /// Module/target prefixes for the underlying HTTP/gRPC/watch client stack. @@ -91,6 +95,103 @@ struct Args { /// clear error. #[arg()] url: Option, + + /// Validate connectivity to a single dependency and exit immediately, + /// instead of running the agent. Useful for troubleshooting one + /// connection in isolation (e.g. a systemd ExecStartPre check, or manual + /// diagnostics on-node) without running the full orchestrator loop. + /// Exits with status 0 if the connection could be established, non-zero + /// (with an error message) otherwise. + #[arg(long, value_enum)] + validate_connection: Option, +} + +/// A single dependency `--validate-connection` can check. +#[derive(Clone, Copy, Debug, clap::ValueEnum)] +enum ConnectionTarget { + /// Validates reachability of the Kubernetes API server by fetching this + /// node's own Node object (the same access the agent's reconcile loop + /// already requires). + Kubernetes, + /// Validates reachability of tridentd by connecting to its gRPC Unix + /// socket. Connecting is sufficient - no RPC call is needed, since the + /// connection itself fails immediately if nothing is listening. + Tridentd, + /// Validates reachability of the Nebraska/Omaha server by issuing a real + /// update-check query. Any well-formed Omaha response (including "no + /// update available") counts as success - only a network/transport + /// failure is treated as unreachable. + Nebraska, +} + +/// Checks connectivity to exactly one of `target`'s dependencies and returns +/// `Ok(())` on success. The caller (`main`) surfaces any `Err` the normal way +/// (`anyhow`'s `Termination` impl prints the error and exits non-zero), so +/// this function only needs to produce a descriptive error on failure - no +/// explicit `process::exit` is required. +async fn validate_connection( + target: ConnectionTarget, + config: &AgentConfig, +) -> Result<(), anyhow::Error> { + match target { + ConnectionTarget::Kubernetes => { + let client = NodeClient::new(&config.kubernetes) + .await + .context("failed to build Kubernetes client")?; + client + .get_node(&config.kubernetes.node_name) + .await + .with_context(|| { + format!( + "failed to reach Kubernetes API server at {} (get Node {:?})", + config.kubernetes.api_server, config.kubernetes.node_name + ) + })?; + log::info!( + "kubernetes: reached API server at {} and fetched Node {:?}", + config.kubernetes.api_server, + config.kubernetes.node_name + ); + } + ConnectionTarget::Tridentd => { + TridentClient::connect(&config.trident.socket) + .await + .with_context(|| { + format!("failed to reach tridentd at {}", config.trident.socket) + })?; + log::info!("tridentd: connected to {}", config.trident.socket); + } + ConnectionTarget::Nebraska => { + let endpoint = config.nebraska.endpoint.clone().ok_or_else(|| { + anyhow::anyhow!( + "nebraska.endpoint is not configured (set [nebraska].endpoint in config.toml, or pass a URL on the CLI)" + ) + })?; + let app_id = config.nebraska.app_id.clone(); + // query_for_update() is a blocking call (reqwest::blocking under + // the hood, see omaha::send) - calling it directly from this + // async fn can panic ("Cannot drop a runtime in a context where + // blocking is not allowed") because reqwest::blocking spins up + // its own inner Tokio runtime per call, which isn't safe to tear + // down from inside an already-running async task. Run it on a + // dedicated blocking thread instead. + let endpoint_for_task = endpoint.clone(); + tokio::task::spawn_blocking(move || { + query_for_update( + &endpoint_for_task, + &app_id, + DEFAULT_NEBRASKA_TRACK, + &semver::Version::new(0, 0, 0), + IdSource::MachineIdHashed, + ) + }) + .await + .context("Nebraska connectivity check task panicked")? + .with_context(|| format!("failed to reach Nebraska server at {endpoint}"))?; + log::info!("nebraska: reached server at {endpoint}"); + } + } + Ok(()) } #[tokio::main] @@ -131,6 +232,10 @@ async fn main() -> Result<(), anyhow::Error> { let config = AgentConfig::load(&config_path, explicit_config)?.unwrap_or_default(); let config = config.with_cli_endpoint(args.url.clone()); + if let Some(target) = args.validate_connection { + return validate_connection(target, &config).await; + } + match config.orchestration.goal_source { // Historical one-shot flow: query Nebraska once, apply an update if // offered, and exit. No Kubernetes/annotation involvement. From 1eec8cdac9c221e20e6c0808283e92e179d5e760 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 6 Aug 2026 00:12:27 +0000 Subject: [PATCH 2/3] fix Copilot-flagged issues on PR #732 - add check_nebraska_reachable(): a pure transport/schema reachability check that does not apply query_for_update's app-level semantic validation (app ID match, non-error app/update-check status), so the nebraska validate-connection target matches its documented behavior of treating any well-formed Omaha response as success - replace hardcoded "config.toml" wording in main.rs with trident-acl-agent.conf, matching the real default config path - add a unit test documenting the behavior difference between check_nebraska_reachable and query_for_update on the same error-status response Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86 --- crates/trident-acl-agent/src/lib.rs | 68 ++++++++++++++++++++++++++++ crates/trident-acl-agent/src/main.rs | 23 ++++++---- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/crates/trident-acl-agent/src/lib.rs b/crates/trident-acl-agent/src/lib.rs index 64cb21243..c8414220c 100644 --- a/crates/trident-acl-agent/src/lib.rs +++ b/crates/trident-acl-agent/src/lib.rs @@ -108,6 +108,27 @@ pub async fn run_omaha_only(config: &config::AgentConfig) -> Result<(), anyhow:: } } +/// Checks that the Omaha/Nebraska server at `url` is reachable and speaking +/// the Omaha protocol, without treating any app-level result (including a +/// non-OK app/update-check status) as a failure. Unlike [`query_for_update`], +/// this only fails on network/transport problems or a response that isn't +/// well-formed Omaha XML -- it's meant for a pure "can we talk to this +/// server at all" check (e.g. `--validate-connection nebraska`), not for +/// deciding whether an update is available. +pub fn check_nebraska_reachable( + url: &Url, + app_id: &str, + track: &str, + machine_id_source: IdSource, +) -> Result<(), HarpoonError> { + let request = Request::default().with_app( + AppRequest::new(app_id, Version::new(0, 0, 0), track, machine_id_source)? + .with_update_check(), + ); + omaha::send(url, &request)?; + Ok(()) +} + /// Query the Omaha server at the given URL for the given app and track. pub fn query_for_update( url: &Url, @@ -470,4 +491,51 @@ mod tests { omaha_mock.assert(); assert!(matches!(response.result, QueryResult::NoUpdate)); } + + #[test] + fn test_check_nebraska_reachable_succeeds_on_error_app_status() { + // check_nebraska_reachable() is meant to be a pure "can we reach + // this server and does it speak Omaha" check, unlike + // query_for_update() which also validates app-level semantics. A + // well-formed response with a non-OK app status should still count + // as "reachable" here, even though query_for_update() would reject + // the same response as a QueryError. + let mut server = mockito::Server::new(); + + let omaha_mock = server + .mock("POST", "/") + .with_status(200) + .match_body(Matcher::Regex(".* + + + + + + "#}) + .expect(2) + .create(); + + check_nebraska_reachable( + &Url::parse(&server.url()).unwrap(), + "test", + "track", + IdSource::MachineIdHashed, + ) + .unwrap(); + + // Confirm the same response *would* be rejected by query_for_update(), + // to document the intentional behavior difference. + assert!(query_for_update( + &Url::parse(&server.url()).unwrap(), + "test", + "track", + &Version::new(0, 1, 0), + IdSource::MachineIdHashed, + ) + .is_err()); + + omaha_mock.assert(); + } } diff --git a/crates/trident-acl-agent/src/main.rs b/crates/trident-acl-agent/src/main.rs index d9ac103bd..06e1c9587 100644 --- a/crates/trident-acl-agent/src/main.rs +++ b/crates/trident-acl-agent/src/main.rs @@ -5,10 +5,11 @@ use clap::Parser; use log::{LevelFilter, Log, Metadata, Record}; use trident_acl_agent::{ + check_nebraska_reachable, config::{AgentConfig, GoalSource, DEFAULT_CONFIG_PATH}, k8s::NodeClient, orchestrator::Orchestrator, - query_for_update, run_omaha_only, + run_omaha_only, trident::TridentClient, IdSource, DEFAULT_NEBRASKA_TRACK, }; @@ -91,8 +92,8 @@ struct Args { config: Option, /// Optional Omaha/Nebraska URL override. When omitted, Harpoon uses the - /// endpoint from config.toml. When both are missing, startup fails with a - /// clear error. + /// endpoint from trident-acl-agent.conf. When both are missing, startup + /// fails with a clear error. #[arg()] url: Option, @@ -164,24 +165,30 @@ async fn validate_connection( ConnectionTarget::Nebraska => { let endpoint = config.nebraska.endpoint.clone().ok_or_else(|| { anyhow::anyhow!( - "nebraska.endpoint is not configured (set [nebraska].endpoint in config.toml, or pass a URL on the CLI)" + "nebraska.endpoint is not configured (set [nebraska].endpoint in trident-acl-agent.conf, or pass a URL on the CLI)" ) })?; let app_id = config.nebraska.app_id.clone(); - // query_for_update() is a blocking call (reqwest::blocking under - // the hood, see omaha::send) - calling it directly from this + // check_nebraska_reachable() is a blocking call (reqwest::blocking + // under the hood, see omaha::send) - calling it directly from this // async fn can panic ("Cannot drop a runtime in a context where // blocking is not allowed") because reqwest::blocking spins up // its own inner Tokio runtime per call, which isn't safe to tear // down from inside an already-running async task. Run it on a // dedicated blocking thread instead. + // + // Deliberately uses check_nebraska_reachable() rather than + // query_for_update(): the latter also validates app-level + // semantics (app ID match, non-error app/update-check status), + // which would make this a "can we get a valid update check" test + // rather than the pure reachability check documented on + // ConnectionTarget::Nebraska above. let endpoint_for_task = endpoint.clone(); tokio::task::spawn_blocking(move || { - query_for_update( + check_nebraska_reachable( &endpoint_for_task, &app_id, DEFAULT_NEBRASKA_TRACK, - &semver::Version::new(0, 0, 0), IdSource::MachineIdHashed, ) }) From 9b8ea3ae3920272d328f637a3f3d5e3aa8cf08a2 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Thu, 6 Aug 2026 00:18:18 +0000 Subject: [PATCH 3/3] trident-acl-agent: use configured nebraska.track in validate-connection check_nebraska_reachable() was called with the hardcoded DEFAULT_NEBRASKA_TRACK instead of config.nebraska.track, so --validate-connection nebraska would validate against the wrong track whenever a deployment overrides it in trident-acl-agent.conf. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c06585d-a82d-475f-a802-83fdfa012d86 --- crates/trident-acl-agent/src/main.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/trident-acl-agent/src/main.rs b/crates/trident-acl-agent/src/main.rs index 06e1c9587..a7fa777a6 100644 --- a/crates/trident-acl-agent/src/main.rs +++ b/crates/trident-acl-agent/src/main.rs @@ -11,7 +11,7 @@ use trident_acl_agent::{ orchestrator::Orchestrator, run_omaha_only, trident::TridentClient, - IdSource, DEFAULT_NEBRASKA_TRACK, + IdSource, }; /// Module/target prefixes for the underlying HTTP/gRPC/watch client stack. @@ -184,11 +184,12 @@ async fn validate_connection( // rather than the pure reachability check documented on // ConnectionTarget::Nebraska above. let endpoint_for_task = endpoint.clone(); + let track = config.nebraska.track.clone(); tokio::task::spawn_blocking(move || { check_nebraska_reachable( &endpoint_for_task, &app_id, - DEFAULT_NEBRASKA_TRACK, + &track, IdSource::MachineIdHashed, ) })