docs: RFC 0001 for declarative management of platform resources#1751
docs: RFC 0001 for declarative management of platform resources#1751rohilsurana wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe RFC documents a declarative YAML workflow for reconciling and exporting Frontier resources, defines shared kind behavior and rules for ChangesDeclarative reconciliation design
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8fb691c-6b4f-4ac9-9f5e-4c8db0d04f33
📒 Files selected for processing (1)
docs/rfcs/0001-declarative-reconcile.md
| - `frontier reconcile -f <file> [--dry-run] --host <host> -H "Authorization:Basic <token>"` | ||
| parses and validates the whole file first, then handles each document in file order. It | ||
| prints one report per kind: `no changes`, a plan (dry run), or what was applied. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not claim whole-file validation before apply without a preflight phase.
The RFC promises that the entire file is parsed and validated before any change applies. However, reconcile.Run dispatches documents sequentially and each reconciler applies immediately; a later malformed, unknown, or invalid document can therefore run after earlier documents have already changed the server. Either add a validation-only phase or document partial application as supported behavior.
Also applies to: 136-139
| - `frontier reconcile -f <file> [--dry-run] --host <host> -H "Authorization:Basic <token>"` | ||
| parses and validates the whole file first, then handles each document in file order. It | ||
| prints one report per kind: `no changes`, a plan (dry run), or what was applied. | ||
| - `frontier export <kind>` prints the live state of one kind as a desired-state document on | ||
| stdout. The kind argument is case-insensitive and accepts a plural. | ||
|
|
||
| Both commands talk to the admin API. They log in with any superuser credential; the | ||
| bootstrap service account exists so automation always has one. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid exposing bearer credentials in process arguments.
The documented examples pass the authorization token through -H; command-line arguments can be visible through process listings and shell history. Provide an environment-variable, file, stdin, or client-configuration mechanism before treating this as the standard automation workflow.
Also applies to: 306-307
| parses and validates the whole file first, then handles each document in file order. It | ||
| prints one report per kind: `no changes`, a plan (dry run), or what was applied. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Implement dependency ordering or require validated file ordering.
The RFC says documents are processed in file order but also guarantees that permissions apply before roles. The supplied reconcile.Run implementation has no dependency sorting, so a Role document appearing before its Permission can fail. Topologically order registered kinds, or explicitly require and validate dependency order.
Also applies to: 140-145
| Each kind implements a small interface in `internal/reconcile`: | ||
|
|
||
| - `Reconcile(spec, dryRun)`: read the current state through the API, diff it against the | ||
| spec, report the plan, and apply it unless this is a dry run. | ||
| - `Export()` (optional): return the current state as a spec value. | ||
|
|
||
| New kinds register in one map in the CLI. The behavior rules every kind must follow are |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the export contract consistent.
Lines 103-104 describe Export() as optional, while rule 9 requires every kind to implement export. The actual Reconciler interface also contains only Kind() and Reconcile(). Define export as a mandatory registered capability, or specify the exact behavior for kinds without export.
Also applies to: 143-145
| 7. **Reads and writes use the public API**, not the database. The reconciler is an API | ||
| client like any other: every change is validated, audited, and visible to all pods at | ||
| once. It also means the CLI works against any reachable Frontier, old or new. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Qualify the “old or new” server compatibility claim.
The current CLI registration supports only PlatformUser, while Permission and Role are proposed additions. A server that predates those APIs cannot support every documented kind. State the minimum compatible server version or define capability negotiation.
Coverage Report for CI Build 29221775459Coverage remained the same at 44.876%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Verified the narrowing warning against the schema and against a live server (built from #1737, seeded an org, narrowed app_organization_owner via reconcile). One sentence in it is factually wrong; details inline. More comments on the other sections will follow.
| directly, outside roles. For example, organization `get` is also granted to the org's | ||
| `member` and `owner` relations and to platform superusers. Removing `get` from a role does | ||
| not block someone who also holds one of those direct grants. Role-only permissions (like | ||
| `rolemanage`, `policymanage`, and all custom permissions) always flip cleanly. Before |
There was a problem hiding this comment.
This sentence is wrong for rolemanage and policymanage. Both have direct grants in base_schema.zed, exactly like the get example above:
permission rolemanage = platform->superuser + granted->app_organization_administer + granted->app_organization_rolemanage + owner
permission policymanage = platform->superuser + granted->app_organization_administer + granted->app_organization_policymanage + owner
An org owner keeps both no matter what the role says. In fact almost every predefined org permission carries + owner. Only custom permissions flip cleanly, because they are granted purely through granted-> role paths.
I tested this on a live server built from #1737. Two users held app_organization_owner: one with the owner relation (the normal case — every membership path pairs the role with the relation via orgRoleToRelation), one with only a role policy. After narrowing the role's permissions to [get, update, policymanage] via reconcile:
| Check | owner relation + role | role only |
|---|---|---|
rolemanage |
still allowed | denied |
delete |
still allowed | denied |
So the role mechanism works instantly for role-path access, but the direct owner grant bypasses it for every real owner.
Suggested fix for the paragraph: name only custom permissions as the clean case, and note that narrowing a predefined role today does not restrict org owners at all, because ownership is granted through the owner relation directly in the schema. Worth adding the forward-looking point too: once ownership moves fully to roles and the relation-based grants (+ owner, + member) come out of the base schema, reconcile will be able to properly control access for predefined roles as well. That migration is out of scope here, but naming it explains why the limitation exists and where it goes.
2 participants
Proposes a declarative way to manage Frontier's platform configuration: desired-state YAML files, the
frontier reconcileandfrontier exportcommands, and per-kind behavior rules.What it covers
PlatformUser(shipped in v0.108.0), andPermissionandRole(proposed here).Status
PlatformUsershipped and set the pattern. #1731 (export command) and #1737 (Permission and Role kinds) are draft implementations of this proposal; they change with this RFC's review and merge after it.