Skip to content

fix(reconcile): identify permissions by key instead of namespace and name - #1892

Open
rohilsurana wants to merge 6 commits into
docs/reconcile-platformuser-disabled-out-of-scopefrom
fix/reconcile-permission-key-spec
Open

fix(reconcile): identify permissions by key instead of namespace and name#1892
rohilsurana wants to merge 6 commits into
docs/reconcile-platformuser-disabled-out-of-scopefrom
fix/reconcile-permission-key-spec

Conversation

@rohilsurana

@rohilsurana rohilsurana commented Aug 17, 2026

Copy link
Copy Markdown
Member

What

The Permission reconcile kind now uses one key field instead of separate namespace and name fields. The key is the service.resource.verb value that the API already returns and stores.

Before:

kind: Permission
spec:
  - namespace: compute/order
    name: get

After:

kind: Permission
spec:
  - key: compute.order.get

Why

The API's permission identity is the key. The reconciler used to read the deprecated namespace and name response fields and rebuild the key at apply time. Reading the key straight from the API is simpler, drops the deprecated-field use, and makes the file match what the server reports.

Changes

  • PermissionSpec is now { key, delete }. The diff, validate, and export paths all key off the permission key.
  • fetchCurrent reads GetKey() and parses it into namespace and verb only to skip base-schema permissions. A key that does not parse into service.resource.verb (a base or system permission, or an older server that does not set the key) is skipped, not fatal.
  • Validation still runs fully before any apply. It rejects an empty key, a bad shape, a base-schema key, and a key whose service or resource part is not lowercase alphanumeric. That last check keeps the key one-to-one with the slug the server stores.
  • Export emits key entries sorted by key, so a re-reconcile of an exported file plans nothing.
  • Docs updated to the key form.

Breaking change

This is a hard switch. A permission file that still uses namespace and name now fails the plan with an unknown-field error instead of silently reconciling nothing. Any pixxelhq/configs permission file must move to the key form in a separate configs change.

Stacking

This PR is the top of the reconcile-audit stack: #1888 -> #1889 -> #1890 -> #1891 -> this. It builds on #1889, which tightens the permission grammar. Review and merge in stack order.

@vercel

vercel Bot commented Aug 17, 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 Aug 18, 2026 6:17am

@coderabbitai

coderabbitai Bot commented Aug 17, 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 Plus

Run ID: 3b6d5b39-6b48-4a1c-b8c9-0404c53dd9bf

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 Aug 17, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 32106205339

Coverage decreased (-0.02%) to 48.764%

Details

  • Coverage decreased (-0.02%) from the base build.
  • Patch coverage: 1 uncovered change across 1 file (35 of 36 lines covered, 97.22%).
  • 4 coverage regressions across 2 files.

Uncovered Changes

File Changed Covered %
internal/reconcile/permission.go 24 23 95.83%
Total (2 files) 36 35 97.22%

Coverage Regressions

4 previously-covered lines in 2 files lost coverage.

File Lines Losing Coverage Coverage
internal/reconcile/permission_reconciler.go 3 80.0%
internal/reconcile/permission.go 1 95.52%

Coverage Stats

Coverage Status
Relevant Lines: 40081
Covered Lines: 19545
Line Coverage: 48.76%
Coverage Strength: 15.71 hits per line

💛 - Coveralls

@rohilsurana
rohilsurana force-pushed the fix/reconcile-permission-key-spec branch from e363944 to cf77ebc Compare August 17, 2026 07:07
@rohilsurana
rohilsurana changed the base branch from main to docs/reconcile-platformuser-disabled-out-of-scope August 17, 2026 07:07
@rohilsurana

Copy link
Copy Markdown
Member Author

Review: identify permissions by key (RFC 0001 Rule 1)

Verified on this branch: go build ./..., go test ./internal/reconcile/..., go vet, and golangci-lint run on the changed packages all pass.

The code achieves Rule 1 cleanly. PermissionSpec is now {Key, Delete}, currentPermission is {ID, Key}, the diff keys byKey, export emits Key sorted by key, create sends Key, and the old slug-collision guard is correctly removed. An old {namespace, name} file fails loudly through decodeSpec's KnownFields(true) (covered by the new test), and that strictness pre-dates the stack, so this stands on its own. Rule 5 holds for valid keys.

The rebase onto the stack resolved the earlier overlap with #1889: this branch now sits on top of #1889 and inherits its tightened grammar (the verb error even carries #1889's wording). The shared permission-validation items (the CreatePermission API not validating namespace grammar, and the combined slug-length bound) are tracked in #1889 so they are not duplicated here. Fixing them there fixes them for this PR too.

Verdict: changes needed. The code is approvable; the blocker is a doc file this PR claims to have migrated but did not.

Findings:

  1. Blocking (docs). docs/content/docs/authz/custom-resources.mdx still teaches the old namespace: plus name: Permission format in two reconcile examples (around lines 37-48 and 274-293). The PR description says the docs were moved to the key form, but only reconcile.mdx was. A reader who copies the user/project + createcomputemachine example and runs reconcile now gets an unknown-field error. Convert those entries to the key form, for example - key: compute.machine.get and - key: user.project.createcomputemachine, and update the surrounding prose ("a namespace and an action name") to describe the key.
  2. Minor. fetchCurrent hard-errors the whole list, and therefore export too, if any single GetKey() does not split into three dot-parts, and this runs before the base-namespace skip. The old path read namespace and name and degraded gracefully. Consider tolerating an unparseable key (skip it, or only fail for a non-base one). This is exactly where an older or different server could bite, which ties into the fix(permission): make the response key lossless and migrate its readers #1887 overlap on fetchCurrent: whichever of fix(permission): make the response key lossless and migrate its readers #1887 and this PR lands second should confirm the server's Key is always populated and canonical so this assumption stays safe.

@rohilsurana
rohilsurana force-pushed the docs/reconcile-platformuser-disabled-out-of-scope branch from e70b648 to 345417c Compare August 17, 2026 07:41
@rohilsurana
rohilsurana force-pushed the fix/reconcile-permission-key-spec branch from cf77ebc to 6bdbd09 Compare August 17, 2026 07:41
@rohilsurana

Copy link
Copy Markdown
Member Author

Follow-up on the findings.

  • Docs: done. Migrated both Permission examples in authz/custom-resources.mdx and the surrounding prose to the key form. There are no namespace:/name: Permission examples left in the docs.
  • Unparseable key: done. fetchCurrent now skips a permission whose key does not split into service.resource.verb, rather than failing the whole list. That covers a base or system permission and an older server that does not set the key. Export only emits keys it kept, so a skipped permission never shows up as missing from the file. Added a test.

Also, the stack was rebased so this PR now sits on top of #1889 and inherits its tightened grammar and the new slug-length bound, so those apply to the key form too.

@rohilsurana

Copy link
Copy Markdown
Member Author

Re-review (head 6bdbd09)

Build, tests, and lint are all green. Both code findings are resolved.

  • Docs examples: the two kind: Permission examples are correctly migrated to the key form, every conversion is right (a/b + c becomes a.b.c), and no namespace:/name: entry remains in any Permission spec block. The remaining name: lines are Role identities, which are legitimate.
  • fetchCurrent skip: sound. It skips only unparseable or base keys, and a valid custom key always has three parts, so no manageable permission is silently dropped. Rules 4 and 5 still hold: skipped permissions never enter current, so they are never deleted by omission, and export skips them too, so the round trip plans zero ops.

One doc item remains, so this is not fully closed. The bullet list "A few rules the reconcile flow enforces for permissions" at docs/content/docs/authz/custom-resources.mdx:60-70 still teaches the old field model, directly under the now-fixed key examples:

  • ":61" "A permission is identity only (namespace plus name)."
  • ":66-68" "A namespace must be in service/resource form: two non-empty parts, each lowercase alphanumeric... The action name must be alphanumeric too."

A reader who builds a file from that prose, rather than copying the example above it, would still write namespace/name and hit an unknown-field error. Update that list to the key form to match the examples and the already-updated reconcile.mdx. The slug-mechanics line at :100-102 ("namespace compute/machine with action get becomes compute_machine_get") is describing SpiceDB slug generation, so it is acceptable, though it could mention the key for consistency.

Verdict: approve with nits, once the bullet list is updated.

Comment thread docs/content/docs/authz/custom-resources.mdx
Comment thread internal/reconcile/permission_reconciler.go
@rohilsurana

Copy link
Copy Markdown
Member Author

Doc consistency (not inline, since the file is not in this diff): this PR updates reconcile.mdx and authz/custom-resources.mdx to the key form, but docs/rfcs/0001-declarative-reconcile.md still describes the Permission entry as {namespace, name} (line 54 in the "At a glance" table, and line 210 "An entry is {namespace, name}, for example compute/order + get"). Since that RFC is the spec this stack conforms to, it should move to the key form too. Otherwise a reader following the RFC writes namespace:/name: and the file fails with an unknown-field error.

@rohilsurana
rohilsurana force-pushed the docs/reconcile-platformuser-disabled-out-of-scope branch from 4648a87 to ea6d8fd Compare August 18, 2026 06:16
@rohilsurana
rohilsurana force-pushed the fix/reconcile-permission-key-spec branch from 04c4f1a to 8c2fbdc Compare August 18, 2026 06:16
@rohilsurana

Copy link
Copy Markdown
Member Author

Fixed. docs/rfcs/0001-declarative-reconcile.md now uses the key form: the "At a glance" table lists the Permission identity as key, and the entry line reads {key} in service.resource.verb form, for example compute.order.get. No {namespace, name} Permission references are left in the RFC. Pushed in 8c2fbdc.

@rohilsurana
rohilsurana marked this pull request as ready for review August 18, 2026 06:55
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