Skip to content

trident-acl-agent: add --validate-connection - #732

Draft
bfjelds wants to merge 3 commits into
user/bfjelds/acl-agent-rollback-grpc-rustfrom
user/bfjelds/acl-agent-connection-check
Draft

trident-acl-agent: add --validate-connection#732
bfjelds wants to merge 3 commits into
user/bfjelds/acl-agent-rollback-grpc-rustfrom
user/bfjelds/acl-agent-connection-check

Conversation

@bfjelds

@bfjelds bfjelds commented Aug 5, 2026

Copy link
Copy Markdown
Member

Add --validate-connection <kubernetes|tridentd|nebraska> CLI flag that checks reachability of exactly one service based on trident-acl-agent config 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.

Unclear if this should merge or not.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a --validate-connection <kubernetes|tridentd|nebraska> mode to trident-acl-agent that performs a single dependency reachability check based on config/CLI overrides and then exits, instead of running the normal agent/orchestrator loop. This is aimed at on-node troubleshooting and systemd ExecStartPre-style preflight checks.

Changes:

  • Introduces a ConnectionTarget clap ValueEnum and a new --validate-connection CLI flag.
  • Implements validate_connection() to check Kubernetes (fetch Node), tridentd (connect gRPC socket), or Nebraska (run an Omaha update-check via spawn_blocking).
  • Short-circuits main() to run the validation path and exit early when the flag is provided.
Suppressed comments (1)

crates/trident-acl-agent/src/main.rs:190

  • spawn_blocking(...).await can fail with a JoinError for cancellation as well as panic. The current context string says the task "panicked", which may be inaccurate and confusing during troubleshooting.
            .await
            .context("Nebraska connectivity check task panicked")?
            .with_context(|| format!("failed to reach Nebraska server at {endpoint}"))?;

Comment thread crates/trident-acl-agent/src/main.rs
Comment thread crates/trident-acl-agent/src/main.rs
…dependency

Adds a `--validate-connection <kubernetes|tridentd|nebraska>` 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
@bfjelds
bfjelds force-pushed the user/bfjelds/acl-agent-connection-check branch from 75dd1f6 to 7b374d8 Compare August 6, 2026 00:01
- 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/trident-acl-agent/src/main.rs:124

  • The help text for ConnectionTarget::Nebraska says "only a network/transport failure is treated as unreachable", but the implementation also fails on non-success HTTP status codes (via error_for_status()) and malformed/non-Omaha responses (XML parse/protocol validation). This mismatch can mislead users about what failures to expect.
    /// 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.

crates/trident-acl-agent/src/lib.rs:117

  • check_nebraska_reachable() docs say it "only fails on network/transport problems" or malformed XML, but omaha::send() also treats non-success HTTP status codes as errors (via error_for_status()). The doc should reflect that it requires a successful HTTP response as well.
/// 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

crates/trident-acl-agent/src/main.rs:194

  • --validate-connection nebraska uses DEFAULT_NEBRASKA_TRACK instead of the configured [nebraska].track. This means the reachability check may exercise a different request than the agent will actually use (and could succeed/fail for the wrong reason), which contradicts the PR description that validation is based on the agent config.
                    &endpoint_for_task,
                    &app_id,
                    DEFAULT_NEBRASKA_TRACK,
                    IdSource::MachineIdHashed,
                )

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/trident-acl-agent/src/main.rs:124

  • The Nebraska help text says “Any well‑formed Omaha response … counts as success – only a network/transport failure is treated as unreachable”, but --validate-connection nebraska calls check_nebraska_reachable() which goes through omaha::send() and will fail on non‑2xx HTTP statuses (error_for_status()), even if the body is valid Omaha XML. This makes the CLI help misleading about what failures are expected.

Suggestion: either relax check_nebraska_reachable()/omaha::send() to parse valid Omaha XML even on non‑2xx responses, or (simpler) update this help text (and the check_nebraska_reachable docstring) to explicitly include HTTP status failures in the “unreachable” bucket.

    /// 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.

crates/trident-acl-agent/src/main.rs:198

  • The join error from spawn_blocking(...).await can also be caused by task cancellation (not only a panic). Using .context("... panicked") can produce a misleading error message.

Suggestion: use a more general message (or include the JoinError detail) so failures are accurately described.

            .await
            .context("Nebraska connectivity check task panicked")?
            .with_context(|| format!("failed to reach Nebraska server at {endpoint}"))?;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants