cli: add access-pass plan and apply - #4281
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The apply path has confirmed issues in dry-run behavior and JSON reporting semantics that can mislead automation and operators.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds declarative reconciliation for access passes in the DoubleZero CLI via new doublezero access-pass plan and doublezero access-pass apply subcommands, driven by a YAML “desired state” document. It also includes operator docs, an RFC, and changelog entry to document the workflow and safety properties.
Changes:
- Add YAML schema parsing and normalization for declarative access-pass intent (
desired.rs). - Implement a diff engine plus renderer for planned multicast allowlist and IBRL tenant changes (
plan.rs). - Implement an apply path that prompts (or auto-approves), sends per-item transactions, and can emit a single JSON object for automation (
apply.rs), plus CLI wiring and documentation.
File summaries
| File | Description |
|---|---|
| smartcontract/cli/src/cli/command.rs | Dispatch new access-pass plan and access-pass apply commands (apply uses stdin). |
| smartcontract/cli/src/cli/accesspass.rs | Register new Clap subcommands for access-pass planning and applying. |
| smartcontract/cli/src/accesspass/plan.rs | Implement plan building, feed coverage logic, blocked conditions, and human/JSON rendering. |
| smartcontract/cli/src/accesspass/apply.rs | Implement apply execution, confirmation, per-change transaction sends, and JSON output contract. |
| smartcontract/cli/src/accesspass/desired.rs | Define and parse the YAML schema, resolve payers, and normalize group lists. |
| smartcontract/cli/src/accesspass/mod.rs | Export new access-pass modules. |
| rfcs/rfc28-declarative-access-passes.md | Add design RFC describing schema, semantics, and limitations. |
| docs/access-pass-plan-apply.md | Add operator documentation for plan/apply usage and automation. |
| CHANGELOG.md | Document the new CLI functionality and its key behaviors. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| out: &mut W, | ||
| input: &mut R, | ||
| ) -> eyre::Result<()> { | ||
| client.check_requirements(CHECK_ID_JSON | CHECK_BALANCE)?; |
| } | ||
|
|
||
| if failed > 0 { | ||
| eyre::bail!("{failed} of {} allowlist changes failed", results.len()); |
| let applied = results.iter().filter(|r| r.state == "applied").count() | ||
| + ibrl_results.iter().filter(|r| r.state == "applied").count(); | ||
| let total = results.len() + ibrl_results.len(); | ||
| let json = serde_json::to_string_pretty(&ApplyJson { | ||
| changed: applied > 0, | ||
| counts: Counts { |
| let user_payer = if raw_payer.eq_ignore_ascii_case("me") { | ||
| payer | ||
| } else { | ||
| Pubkey::from_str(raw_payer).map_err(|_| { | ||
| eyre::eyre!( | ||
| "access_passes[{index}] ({}) has an invalid user_payer: {raw_payer}", | ||
| entry.client_ip | ||
| ) | ||
| })? | ||
| }; |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8b9f504. Configure here.
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Plan exits zero when blocked
Medium Severity
Issue
access-pass plan always returns success when the plan contains blocked items.
Context
An operator or CI job runs plan to gate apply. The document names a missing pass or a dual-revoke group. The command prints the blocked items and then exits 0.
Proposed Fix
Have plan return the same blocked-item error that apply already returns.
Additional Locations (1)
Triggered by project rule: Comment shape
Reviewed by Cursor Bugbot for commit 8b9f504. Configure here.
|
|
||
| if failed > 0 { | ||
| eyre::bail!("{failed} of {} allowlist changes failed", results.len()); | ||
| } |
There was a problem hiding this comment.
Apply failure count omits IBRL
Low Severity
Issue
The non-zero exit message counts only multicast results in the denominator, so an IBRL-only failure reports a total of zero.
Context
An operator runs apply and the tenant write fails while every allowlist write succeeds, or only IBRL was planned. The summary line already includes IBRL, but the bail text does not.
Proposed Fix
Use the combined multicast plus IBRL count in the failure message.
Reviewed by Cursor Bugbot for commit 8b9f504. Configure here.
Reconcile access passes against a YAML document describing the access a fleet should have. `plan` reports the difference between the document and the ledger and writes nothing; `apply` sends the same difference after a confirmation. The document covers both halves of a pass's access -- the multicast publish/subscribe allowlists and the IBRL (unicast) tenant -- keyed by (client_ip, user_payer), the pass's PDA seeds. Every field is declarative: a group the document does not name is revoked, and an entry with no ibrl has its tenant cleared. A declared subscribe that an EdgeSeat feed already grants is reported as satisfied and never written. Re-granting would spend a transaction, change nothing and make every run report as changed, so this is what keeps a converged document a no-op. Publisher is never feed-covered, so a publish gap on the same group is still reported. Two situations are refused rather than attempted, and both exit non-zero: an access pass that does not exist, since granting against an empty PDA silently mints a Prepaid pass with 1/1 seats and no epoch; and a group leaving both allowlists at once, since the detach verbs send the role being kept as desired state and revoking both entries first strands the roles on the User account. The IBRL write reads the pass first and re-sends the type, seat caps and allow_multiple_ip unchanged, because SetAccessPass overwrites those from its arguments, and it targets the stored pass so a grant on a shared 0.0.0.0 pass is not written to a different account. Declaring ibrl pins last_access_epoch to unlimited: the tenant and the epoch are one grant, and a finite value fails a later connect ibrl at an unpredictable date. apply --json writes one object with changed first, for configuration management, and requires --auto-approve since there is no terminal to confirm on. Design and open questions in RFC-28; operator documentation in docs/access-pass-plan-apply.md.
7e428f5 to
aab508d
Compare
Related: RFC-28
Summary of Changes
doublezero access-pass plananddoublezero access-pass apply, which reconcile access passes against a YAML document describing the access a fleet should have.planreports the difference between the document and the ledger and writes nothing;applysends the same difference after a confirmation. Granting six servers across four groups in both roles goes from 48 invocations to one reviewable file.(client_ip, user_payer)— the pass's PDA seeds.ibrlhas its tenant cleared. This is what makes the file a description of state rather than a list of additions, and it is why the revocations are shown before they happen.AddMulticastGroup*Allowlistagainst an empty PDA silently mints aPrepaidpass with 1/1 seats andlast_access_epoch: 0, so a typo'd address would produce a junk pass that looks real), and a group leaving both allowlists at once (the detach verbs send the role being kept as desired state, so revoking both entries first strands the roles on the User account with no legal write to remove them).apply --jsonwrites exactly one object to stdout withchangedfirst, so configuration management can drive it withchanged_when. It requires--auto-approveor--dry-run, since there is no terminal to answer the prompt on.serde_yamlis already in the crate, and all writes go through existing instructions.Two details worth a reviewer's attention:
allow_multiple_iplives at the PDA seeded with0.0.0.0and serves any client IP, andSetAccessPassseeds its PDA from theclient_ipargument — sending the declared address would write a different account than the plan described.planalso warns whenever a declared address resolves to a shared pass.SetAccessPassoverwritesaccesspass_type,last_access_epoch,allow_multiple_ipand both seat caps from its arguments, so those are read back and re-sent unchanged; the transaction moves the tenant, pins the epoch, and touches nothing else. Declaringibrlpins the epoch to unlimited, because the tenant and the epoch are one grant and a finite value fails a laterconnect ibrlat an unpredictable date.Diff Breakdown
Roughly half the diff is tests, and the tests live inline in the same three files as the core logic. Nothing is modified — every line is new.
Key files (click to expand)
smartcontract/cli/src/accesspass/plan.rs(+567 core / +515 tests) — the diff engine and its renderer. Builds the multicast grants/revokes and the IBRL change, applies the feed-coverage and dual-revoke rules, batches the group/tenant/feed reads, and renders the plan.smartcontract/cli/src/accesspass/apply.rs(+371 core / +443 tests) — the write path. Prompts, sends each change, reports per item and continues past failures; contains the read-modify-write forSetAccessPassand the JSON contract.smartcontract/cli/src/accesspass/desired.rs(+180 core / +167 tests) — the document schema, strict parsing, and payer resolution (kept separate from parsing so a document can be validated with no keypair and no network).rfcs/rfc28-declarative-access-passes.md(+223) — design, alternatives considered, and six open questions.docs/access-pass-plan-apply.md(+171) — operator documentation: schema, reading a plan, what it refuses, automation.smartcontract/cli/src/cli/{accesspass,command}.rs,accesspass/mod.rs(+16) — subcommand enum, dispatch and module registration.Testing Verification
applyreportschanged: falseand sends zero transactions.mgroup_sub_allowlistbut a covering feed reports the group as satisfiedvia feed <code>and issues no transaction, while still reporting the publisher gap on the same group.203.0.113.10that resolves to a pass stored at0.0.0.0sendsSetAccessPasswithclient_ip: 0.0.0.0, preserving the EdgeSeat type,allow_multiple_ip, and 3/5 seat caps, withlast_access_epochpinned tou64::MAX.subscibe:is rejected with the entry index, the valid alternatives and the line/column, before any RPC call — confirmed against the built binary, which exits 1 at parse time while a valid document proceeds to the network stage.ibrlskips the tenant scan when no pass carries a tenant, but still reads them when one does — a pass with a tenant has to be cleared, so the scan cannot be skipped merely because the document is silent.dev/local-devnet-multicast-setup.mdis the remaining verification.