Repository navigation
acl: trident-acl-agent utilize cookie for Nebraska communication - #854
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Cookie handling needs endpoint isolation, preservation of unchanged cookies, and retention of updates received on failed HTTP requests.
3 open findings
What changed in this PR
Adds persistent Nebraska affinity-cookie handling to the ACL agent across requests, process restarts, and reboots.
Changes:
- Captures and sends cookies through the Nebraska client.
- Adds configurable cookie storage shared by orchestration and connectivity probes.
- Adds unit tests and Storm assertions for cookie reuse and reboot persistence.
| File | Description |
|---|---|
| tools/storm/aclagent/tests/update.go | Checks cookie reuse and reboot persistence. |
| tools/storm/aclagent/proxies/nebraska.go | Simulates affinity cookies and counts request classifications. |
| tools/storm/aclagent/proxies/nebraska_test.go | Tests simulated cookie issuance and echoing. |
| crates/trident-acl-agent/src/lib.rs | Loads and saves cookies for connectivity probes. |
| crates/trident-acl-agent/src/core/nebraska/transport.rs | Sends cookies and extracts response cookies. |
| crates/trident-acl-agent/src/core/nebraska/mod.rs | Exports the cookie store. |
| crates/trident-acl-agent/src/core/nebraska/cookie_store.rs | Adds atomic, owner-only cookie persistence. |
| crates/trident-acl-agent/src/core/nebraska/client.rs | Retains cookies between client requests. |
| crates/trident-acl-agent/src/core/config.rs | Configures the cookie storage path. |
| crates/trident-acl-agent/src/connection_check.rs | Connects probes to shared cookie storage. |
| crates/trident-acl-agent/src/annotations/orchestrator.rs | Persists cookies around update checks and reports. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f933ff8 to
0c7f2f2
Compare
Nebraska issues a load-balancer affinity cookie (e.g. an App-Service-style ASLBSA cookie) via Set-Cookie on any request that does not already carry one, and expects it echoed back on every later request/response. - Transport::post_xml now takes an optional outgoing cookie and returns any captured Set-Cookie value (raw name=value pairs only, attributes like Path/Secure dropped), so ReqwestTransport sends/receives it over the wire without relying on reqwest's short-lived built-in cookie jar. - Client carries the cookie across its own calls via interior mutability (RefCell), with with_cookie()/cookie() to seed/read it across process and reboot boundaries, since callers (check_for_update, report_progress, complete_after_reboot, report_failure) build a fresh Client per call. - New CookieStore (core::nebraska::cookie_store) persists the cookie to its own file (/var/lib/trident-acl-agent/nebraska-cookie by default), deliberately separate from annotations::state's state.json: that file is purpose-built to bridge one update operation's pre-/post-reboot halves, while the cookie is a host-level attribute unrelated to any operation. - Orchestrator loads/saves the cookie around both Nebraska call sites (handle_stage's check_for_update, report_nebraska_event's progress/terminal reports), best-effort: a load/save failure only logs a warning and proceeds cookie-less, never fails the underlying update. - New OrchestrationConfig::cookie_path (TRIDENT_ACL_AGENT_ORCHESTRATION_COOKIE_PATH), defaulting to /var/lib/trident-acl-agent/nebraska-cookie. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c26d0a1-b43f-4ccb-a684-0d92c41f1321
Extends the earlier trident-acl-agent cookie-persistence change with storm coverage. Real Nebraska's own Omaha handler never sets the Azure-App-Service-style affinity cookie production is fronted by - it's an infrastructure-level artifact, not something Nebraska's application code emits - so storm's fake Nebraska (which wraps the real upstream Omaha handler) never exercised this path before. - NebraskaProxy.Handler() now simulates that cookie: issues a stable, per-instance value on any request lacking one, and classifies every request as missing/correct/wrong via new RequestsMissingCookie(), RequestsWithCorrectCookie(), RequestsWithWrongCookie(), and AffinityCookie() accessors. Purely additive to the test harness; the real Omaha protocol handling is untouched. - New nebraska_test.go subtest drives the simulated cookie directly over HTTP (no VM needed): asserts it's issued once, not re-issued once echoed back, and that a stale cookie is classified as wrong. - RunABUpdate (tools/storm/aclagent/tests/update.go) now asserts, after its existing stage/finalize/reboot/commit run: exactly one request arrived with no cookie (the agent's first-ever contact), none arrived with a wrong cookie, at least one echoed the correct cookie, and - the strongest check - the cookie persisted on disk (/var/lib/trident-acl-agent/nebraska-cookie, the same persistent partition the Nebraska CA cert is pinned to) matches exactly across the real "systemctl reboot" finalize triggers, proving cross-process, cross-reboot persistence with the real compiled binary under systemd. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c26d0a1-b43f-4ccb-a684-0d92c41f1321
…tore Fixes a real CI failure on this branch (ADO build 1225069, [GITHUB]-trident-pr-e2e, storm aclagent run-ab-update): storm-trident::scenario::aclagent::run-ab-update -> FAIL (expected exactly 1 Nebraska request with no affinity cookie (the agent's first-ever contact), got 2) Root cause: RunABUpdate's storm test runs a successful "--validate-connection nebraska" check (via check_nebraska_reachable()) against the real Nebraska endpoint before the stage/finalize scenario even starts, to prove the env-var config override path works. That probe built a bare Client with no cookie_store wiring at all - it never loaded an existing cookie nor persisted the one Nebraska issued - so it always counted as its own permanent "first contact". The real scenario's own first check_for_update then saw no persisted cookie either, making 2 first-contacts instead of 1. Fix: check_nebraska_reachable() now takes a &CookieStore, loads/seeds the probe's Client with any persisted cookie, and persists back whatever the probe's response sets - mirroring the orchestrator's load_nebraska_cookie()/save_nebraska_cookie() pattern. Best-effort, like every other cookie_store use: a load/save failure only warns and the probe proceeds/returns normally either way. connection_check.rs's validate_connection() now builds the same CookieStore (from config.orchestration.cookie_path) the orchestrator uses and passes it through. Updated the 3 existing check_nebraska_reachable tests to pass a temp CookieStore, and added a new test proving the probe persists a cookie on one call and sends it on the next. This also resolves the "should the connectivity probe participate in the cookie contract" open question noted in wiki/projects/trident/trident-acl-agent.md - answer, confirmed by this real pipeline failure: yes, it must share the same store. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c26d0a1-b43f-4ccb-a684-0d92c41f1321
Two new envy-driven OrchestrationConfig fields, alongside the existing cookie_path: - TRIDENT_ACL_AGENT_ORCHESTRATION_COOKIE_ENABLED (bool, default true): blanket kill switch. When false, no Cookie: header is ever sent and no Set-Cookie is ever adopted - the persisted file (if one exists from before the toggle was flipped) is never read or written either, so toggling it back on later just picks the stale file back up as-is. - TRIDENT_ACL_AGENT_ORCHESTRATION_COOKIE_LIFETIME (enum, default "forever"): "forever" reuses a persisted cookie across every update until Nebraska replaces it (the originally-shipped behavior); "per-update" forces a fresh cookie at the start of every update by skipping the load on handle_stage's initial check_for_update call - definitionally the first Nebraska call of any update, since stage always precedes finalize/commit. Every other call site (report_nebraska_event's progress/terminal reports, the --validate-connection nebraska probe) is always a continuation and is unaffected by this choice. Implementation: - core/config.rs: new CookieLifetime enum (FromStr-parsed, mirroring Mode), cookie_enabled/cookie_lifetime fields on OrchestrationConfig, new empty_bool_as_none/empty_cookie_lifetime_as_none deserializers, defaults/override/malformed-value tests. - annotations/orchestrator.rs: load_nebraska_cookie gains a force_fresh: bool parameter, short-circuiting (without touching disk) when cookies are disabled or force_fresh is set; save_nebraska_cookie short-circuits when disabled. handle_stage's check_for_update is the only call site that computes force_fresh = cookie_lifetime == PerUpdate; report_nebraska_event always passes force_fresh: false. - lib.rs: check_nebraska_reachable gains a cookie_enabled: bool parameter (no lifetime concept - a diagnostic probe isn't part of any update); new test proving a disabled probe never touches an existing persisted cookie. connection_check.rs passes config.orchestration.cookie_enabled through. All 218 trident-acl-agent lib tests pass; clippy and fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c26d0a1-b43f-4ccb-a684-0d92c41f1321
Clear persisted cookies at per-update boundaries and disable cookie adoption and transmission throughout client retries and probes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6400e928-51a9-4a92-aead-de8b03854676
Clarify that restarting an unfinished stage resets affinity even with unchanged request IDs. Cover reset and continuation behavior across an orchestrator restart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6400e928-51a9-4a92-aead-de8b03854676
be45925 to
7035f79
Compare
|
Copilot resolve the merge conflicts in this pull request |
…-affinity-cookie # Conflicts: # crates/trident-acl-agent/src/core/config.rs Co-authored-by: bfjelds <9874445+bfjelds@users.noreply.github.com>
Merged latest |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0eeaf50a-88e0-4d3b-af9d-9a3bb53fd7c6
Keep configuration errors descriptive and reject noncanonical policies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0eeaf50a-88e0-4d3b-af9d-9a3bb53fd7c6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0eeaf50a-88e0-4d3b-af9d-9a3bb53fd7c6
Document cookie enable/path settings and default affinity behavior without exposing lifetime configuration. Correct staging, diagnostics, timeout, and deployment details against the implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0eeaf50a-88e0-4d3b-af9d-9a3bb53fd7c6
| /// no relationship to any update operation, unlike everything else in | ||
| /// `state.json`. | ||
| pub const NEBRASKA_COOKIE_FILE_NAME: &str = "nebraska-cookie"; | ||
| pub const DEFAULT_NEBRASKA_COOKIE_PATH: &str = |
There was a problem hiding this comment.
follow up: i feel tempted to not make this configurable
There was a problem hiding this comment.
fair. i think at some point, anything i could imagine as quasi-unknown became a configurable things. :)
There was a problem hiding this comment.
will create a branch and investigate
| machine_id: MachineId, | ||
| client_version: Option<String>, | ||
| cookie_store: Option<CookieStore>, | ||
| cookie_lifetime: CookieLifetime, |
There was a problem hiding this comment.
follow up: the lifetime (if we keep it) should be a property of the store, not the client
There was a problem hiding this comment.
seems odd to me to have the store understand whether this is the first request or not (per-update vs something else)
There was a problem hiding this comment.
will create a branch and investigate
Paco Huelsz (frhuelsz)
left a comment
There was a problem hiding this comment.
LGTM. Agent review found no blocking issues with the core cookie lifecycle. These concerns are scoped as follow-up hardening:
| Concern | Why it matters | Proposed follow-up |
|---|---|---|
| Cookie is not scoped to an origin | The persisted file contains only name=value. A diagnostic pointed at another endpoint, or a controller bug changing request.server within an update, could send the cookie to another host. The default per-update policy limits the practical window by clearing it between staging attempts. |
Persist the issuing scheme/host/port with the cookie and reuse it only for a matching origin. |
| Failed reset can leave a stale cookie | If clearing the per-update cookie fails but a later read succeeds, a progress request could reload the previous update's cookie. This requires an unusual transient filesystem failure and should not block the base functionality. | If reset fails, disable cookie loading for that staging attempt instead of relying on the existing file. |
| Cookie expiry/deletion is ignored | Cookie attributes such as expiry and Max-Age=0 are discarded, and later Set-Cookie values are intentionally ignored in per-update mode. This is correct under the confirmed one-stable-cookie contract, but would need revisiting if the load balancer begins refreshing or deleting cookies. |
Document the load-balancer contract and add refresh/deletion handling only if that behavior changes. |
| Persistence directory is application-created | The cookie file is safely written with mode 0600, but directory creation and ownership are still handled by the application. |
Consider adding StateDirectory=trident-acl-agent and StateDirectoryMode=0700 to the systemd unit. |
The opt-in default, per-update reset, cross-process/reboot persistence, disabled behavior, atomic writes, and E2E coverage all look sound. I recommend merging the base functionality and tracking the items above separately.

Description
Persist Nebraska affinity cookies across requests, process restarts, and reboots. Cookies remain opt-in.
per-update(default and only publicly supported option): start each initial update check without a cookie. Persist only that response's cookie and reuse it through progress, finalize, and post-reboot completion. Ignore later response cookies; reset at the next initial update check.per-message(not publicly supported option): persist each eligible response cookie for the next request. Responses without a cookie retain the stored value.Configuration
TRIDENT_ACL_AGENT_ORCHESTRATION_COOKIE_ENABLEDfalseTRIDENT_ACL_AGENT_ORCHESTRATION_COOKIE_PATH/var/lib/trident-acl-agent/nebraska-cookieValidation
7fd70acb