Skip to content

feat(reconcile): add Permission and Role kinds, bootstrap only creates missing roles#1737

Open
rohilsurana wants to merge 18 commits into
feat/reconcile-exportfrom
feat/reconcile-roles-permissions
Open

feat(reconcile): add Permission and Role kinds, bootstrap only creates missing roles#1737
rohilsurana wants to merge 18 commits into
feat/reconcile-exportfrom
feat/reconcile-roles-permissions

Conversation

@rohilsurana

@rohilsurana rohilsurana commented Jul 3, 2026

Copy link
Copy Markdown
Member

Stacked on #1731; will rebase onto main once that squash-merges.

What

Two new desired-state kinds for frontier reconcile and frontier export, plus a bootstrap behavior change that makes role management through the API stick.

Permission kind

A permission is identity only (namespace + name). It is added or deleted, never updated.

kind: Permission
spec:
  - namespace: compute/order
    name: get
  - namespace: compute/order
    name: legacy
    delete: true
  • Base-schema permissions (app namespaces) are server-managed: the diff and export ignore them, and spec entries targeting them are rejected.
  • A custom permission on the server that the file does not mention fails the plan. Nothing is deleted by omission; deletion needs the explicit flag.
  • Validation mirrors the server: alphanumeric name, two-part service/resource namespace.

Role kind

Platform-level roles only. The role name is the identity and never changes. Title, permissions, and scopes are the managed fields. For custom roles, a field that is present in an entry is managed and a field that is omitted keeps its server value. Predefined roles converge to their shipped definitions instead: an entry overrides the fields it lists, an omitted field takes the definition's value, and a predefined role with no entry resets to the definition.

kind: Role
spec:
  - name: compute_order_manager
    title: Order Manager
    permissions: [compute_order_get, compute_order_update]
  - name: old_role
    delete: true
  - name: app_project_viewer       # predefined: override title and/or permissions
    permissions:                   # extends the predefined set with custom permissions
      - app_project_get
      - app_project_resourcelist
      - resource_aoi_get
  - name: app_organization_owner   # predefined: title-only override also works
    title: Workspace Owner
  • Custom roles must appear in the file — kept, or marked delete: true — so the plan fails loudly on unaccounted roles instead of removing anything by omission.
  • Predefined roles reset to their shipped definitions when not listed, so drift shows up in the plan (marked "not in file, reset to default") instead of being skipped. Removing a predefined role's entry from the file is therefore a visible action: the role goes back to its defaults on the next run.
  • Fields that are empty on the server (legacy rows without scopes, for example) converge only when an entry lists them explicitly — a file cannot represent an empty value, so export and diff both treat unlisted-empty as settled, keeping the round trip clean.
  • Deleting a predefined role is rejected (bootstrap would recreate it at boot), and creating missing ones stays boot's job.
  • Permission references accept any server-known form (slug, service/resource:verb, service.resource.verb); slugs are canonical.
  • Deleting a role that still has policy bindings fails with the server's foreign-key error, so a bound role cannot be dropped by accident.
  • Roles the reconciler creates or updates carry managed_by: frontier-reconcile in metadata for provenance.

Bootstrap change: existing roles are left alone

MigrateRoles used to write a role's definition back whenever its permission set drifted. That meant any change made through the API — including this reconciler's — was reverted on the next boot. Now boot creates a role only when it is missing and never updates an existing one. Operators own existing roles; the reconcile flow is the way to change them.

The definition-update job boot loses moves to reconcile: when a new frontier version changes a predefined role's definition, the first reconcile run with the new image plans that change for every role the file does not override, and applying it converges the deployment. The same update that used to happen invisibly at every restart now happens once, reviewed. This also means the reconcile CLI image should match the server's release — it carries the definitions the reset converges to.

Why no role renames

An earlier draft allowed renaming predefined roles. Verifying it showed the server resolves predefined roles by name at runtime in 13+ places (org owner constraint checks, group creation, invitation acceptance, service-user creation, admin listings, notifications). Renaming a predefined role would break all of them, so the name is fixed as the identity and only the display title is renameable.

Tests

  • Diff-level: adds/updates/deletes ordering, merge-over-current semantics for custom roles, predefined-role resets (unlisted drift, omitted-field convergence, empty-field carve-out), unaccounted-role and unaccounted-permission failures, predefined-role guards, permission-format normalization, duplicate and conflict detection.
  • Reconciler-level with fake APIs: apply paths incl. the metadata marker, dry-run, base-namespace invisibility, and an unlisted drifted predefined role reset through the API.
  • Round-trips: exporting each kind and reconciling the output plans no changes, including a retitled predefined role and a legacy row with empty fields.
  • Bootstrap: existing role (even drifted) is not touched; missing role is created; transient Get errors do not fall through to create.

@vercel

vercel Bot commented Jul 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Jul 15, 2026 11:21am

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 41a40c36-3b71-4c8a-807a-4ebe12a73cfc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Jul 3, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 29411290406

Coverage increased (+0.5%) to 45.466%

Details

  • Coverage increased (+0.5%) from the base build.
  • Patch coverage: 69 uncovered changes across 7 files (449 of 518 lines covered, 86.68%).
  • 285 coverage regressions across 9 files.

Uncovered Changes

File Changed Covered %
internal/reconcile/role_reconciler.go 132 109 82.58%
internal/reconcile/permission_reconciler.go 77 61 79.22%
cmd/reconcile.go 20 5 25.0%
internal/reconcile/role.go 204 195 95.59%
cmd/serve.go 4 0 0.0%
cmd/export.go 1 0 0.0%
internal/reconcile/permission.go 68 67 98.53%
Total (10 files) 518 449 86.68%

Coverage Regressions

285 previously-covered lines in 9 files lost coverage.

File Lines Losing Coverage Coverage
billing/invoice/service.go 71 5.54%
core/invitation/service.go 53 48.55%
internal/api/v1beta1connect/group.go 52 81.7%
cmd/serve.go 34 0.0%
internal/store/postgres/group_repository.go 32 76.03%
core/group/service.go 20 55.36%
cmd/group.go 10 43.44%
internal/store/postgres/org_users_repository.go 9 72.73%
internal/store/postgres/invitation_repository.go 4 80.3%

Coverage Stats

Coverage Status
Relevant Lines: 38189
Covered Lines: 17363
Line Coverage: 45.47%
Coverage Strength: 13.07 hits per line

💛 - Coveralls

@rohilsurana

Copy link
Copy Markdown
Member Author

From the RFC review (#1751), verified on a live sandbox built from this branch: frontier export role omits predefined roles even after they have been modified. I narrowed app_organization_owner to [get, update, policymanage] via reconcile, then ran export — the output contained only the custom role. A file seeded from that export silently drops the override, and the RFC's "seed files from exports" guidance breaks for exactly the roles this PR makes overridable. Suggest: Role export should include predefined roles whose fields differ from the boot defaults in schema.PredefinedRoles, and keep omitting untouched ones. That also keeps the round-trip rule intact: reconciling the export output still plans no changes.

@rohilsurana

Copy link
Copy Markdown
Member Author

Checked this against a fresh sandbox built from the current branch, and the exact scenario works: narrowing app_organization_owner to [get, update, policymanage] via reconcile, then exporting, produces an entry for the owner with its changed permissions (title omitted since it still matches the default), and reconciling that export plans no changes. There is also a unit test now with exactly this scenario. The earlier repro most likely ran a stale binary.

The review still caught real gaps right next to it, fixed in 2cb94ca:

  • Scope overrides on predefined roles were never compared, so a scopes-only change was silently dropped from exports. Export now compares title, permissions, and scopes.
  • Managed-empty lists could not survive a round trip: YAML omits empty lists, so a predefined role emptied to zero permissions (or scopes) would be exported as if untouched. Rather than represent the unrepresentable, validation now rejects emptying a predefined role's permissions or scopes, and a custom role must list at least one permission.
  • The permission comparison now normalizes both sides, so duplicate slugs from the server cannot cause spurious or missed diffs.

Kept the differs-only export rather than always exporting predefined roles — the fixes are small, the files stay minimal, and the round-trip rule holds.

@rohilsurana

Copy link
Copy Markdown
Member Author

Verified the reconciler against the database identity model:

  • Roles: the database enforces UNIQUE(org_id, name), and scopes is a plain indexed column with no uniqueness. The reconciler matches: it lists platform-org roles only (so name alone is a complete identity inside its scope), treats scopes as a managed field never used for matching, upserts hit the same (org_id, name) conflict target, and the reserved org field lines up with the composite key for later.
  • Permissions: the database enforces both UNIQUE(namespace_name, name) and UNIQUE(slug). The diff is keyed by slug, which the slug uniqueness makes collision-free for live rows.

One edge fixed in d555799: slugs flatten / to _, and namespace parts may contain underscores, so two different file entries (for example resource/order_item:get and resource_order/item:get) produce the same slug. The diff used to silently treat the second as a duplicate; it now fails the plan naming both entries and the colliding slug.

# Conflicts:
#	docs/content/docs/reference/cli.mdx
@rohilsurana

Copy link
Copy Markdown
Member Author

Review notes (self-review after an independent pass)

Needs a decision — role metadata is wiped on update

roleBody always sends metadata: {managed_by: frontier-reconcile} and nothing else. The
server's UpdateRole builds metadata only from the request body, and the store replaces
the whole metadata column. Bootstrap seeds every predefined role with
{description: ...}. So the first reconcile update of a role — including an automatic
reset of a drifted predefined role — drops its description for good. Boot does not put
it back (skip-if-exists).

Nothing in non-test code reads role description today, and the default role metaschema
accepts the body, so nothing breaks right now. But it is silent data loss, and a stricter
role metaschema that requires description would make every reconcile role update fail.

Three ways to fix it:

  • A — merge in the CLI (recommended). The reconciler reads the role's current metadata
    and writes managed_by on top of it. Stays inside this PR, and no other API client is
    affected. Needs metadata carried through ListRoles into the diff. It is still a full
    replace at the API, so a metadata change made between our read and write is lost — but
    that is already how the flow behaves (last write wins).
  • B — merge on the server. UpdateRole merges the incoming metadata over what the role
    already has. Fixes it for every client, but it changes the public API: a caller can no
    longer drop a key by leaving it out. Bigger change, needs maintainer agreement.
  • C — drop the marker. Stop stamping managed_by. No data loss and no complexity, but
    we lose the way to tell reconcile-managed roles apart, which is the reason the marker
    exists.

Leaning towards A. Open to input before I implement.

Fixed

  • A platform role could be created with no permissions. UpdateRole already rejected
    an empty permission list, but CreateRole did not, so a zero-permission role could
    exist and then fail to reconcile back from its own export. CreateRole now rejects it
    too, with a test. (bff42ac)
  • The reconcile guide did not cover the new kinds. docs/content/docs/reconcile.mdx
    now documents the Permission and Role kinds, the reset-to-default behavior for
    predefined roles, and the permission-before-role ordering rule. (2ec0b55)

Nits (not blocking)

  • The server permission list is compared as a set but is not de-duplicated first
    (role.go, sortedCopy(cur.Permissions)). A role row with a repeated slug would plan a
    spurious update every run. DB uniqueness makes this unlikely; worth de-duplicating both
    sides anyway.
  • On a partial apply, the printed report lists the full plan while the applied count is
    correct, so the list can show more lines than were applied. Cosmetic.

@rohilsurana

Copy link
Copy Markdown
Member Author

Update on the metadata-wipe item above: description is now a managed spec field
(24fc1165), read from and written to role metadata, and treated the same as title for
custom and predefined roles. That closes the real harm. Reconcile-managed platform roles
only ever carry managed_by and description in metadata, and both now round-trip, so a
reconcile update no longer drops anything an operator can see.

The only residual is a metadata key someone sets directly on a platform role outside this
flow (not managed_by or description); a later reconcile update would drop it, which is
consistent with reconcile owning the role's declared fields. If we ever want to keep such
keys, option B (merge on the server) is the way. Leaving that out for now.

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