fix(rbac): make conditional policy reconcile non-destructive and support sibling merges#9731
Merged
Merged
Conversation
…ort sibling merges Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>
PatAKnight
requested review from
a team,
AndrienkoAleksandr,
HusneShabbir,
christoph-jerolimov,
debsmita1,
divyanshiGupta,
djanickova,
dzemanov,
jrichter1,
karthikjeeyar and
rohitkrai03
as code owners
Contributor
Changed Packages
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts conditional policy reconciliation in the RBAC backend to be non-destructive during reload/sync, preventing stored conditions from being wiped when staging/validation or persistence fails mid-reconcile. It also adds support for “sibling” conditional policy merges (multiple stored rows for the same role/resource) by excluding pending deletes from conflict checks during create/update.
Changes:
- Switch conditional policy reconcile flows (provider sync + YAML watcher) from “delete then add” to a planned diff-based reconcile (updates/creates before deletes) with an explicit abort audit/log path on failure.
- Add a reconcile planner (
planConditionalReconcile) and supporting helpers (diffConditionalPolicies,pendingDeleteIdsFromPlan,abortConditionalPolicyReconcile), plus tests. - Extend conditional storage conflict checking to support excluding “pending delete” row IDs so sibling merges don’t conflict.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.ts | Provider conditional reconcile now diffs/plans updates+creates+deletes and emits a batch abort audit event on failure. |
| workspaces/rbac/plugins/rbac-backend/src/providers/connect-providers.test.ts | Updates expectations for update-based reconcile and adds regression coverage for non-destructive abort + sibling merge. |
| workspaces/rbac/plugins/rbac-backend/src/helper.ts | Introduces diff/plan helpers and a standardized abort audit/log helper for failed conditional reconciles. |
| workspaces/rbac/plugins/rbac-backend/src/helper.test.ts | Adds unit tests for planning behavior, action overlap, and toError. |
| workspaces/rbac/plugins/rbac-backend/src/file-permissions/yaml-conditional-file-watcher.ts | YAML reload path now stages additions, plans reconcile, applies update/create/delete ordering, and emits abort audit/log on failure. |
| workspaces/rbac/plugins/rbac-backend/src/file-permissions/yaml-conditional-file-watcher.test.ts | Updates audit expectations and adds regression coverage for metadata-unavailable abort and sibling merges. |
| workspaces/rbac/plugins/rbac-backend/src/database/conditional-storage.ts | Adds idsToExclude support to conflict checking for sibling merges during update/create. |
| workspaces/rbac/plugins/rbac-backend/src/database/conditional-storage.test.ts | Adds coverage that updates succeed when excluding a pending sibling delete. |
| workspaces/rbac/plugins/rbac-backend/src/auditor/auditor.ts | Adds RECONCILE_ABORT action type for audit meta. |
| workspaces/rbac/plugins/rbac-backend/docs/audit-log.md | Documents reconcile abort semantics and the new audit meta fields. |
| workspaces/rbac/.changeset/fuzzy-pugs-search.md | Patch changeset describing the non-destructive reconcile and sibling merge behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Assisted-by: Cursor AI Signed-off-by: Patrick Knight <pknight@redhat.com>
AndrienkoAleksandr
approved these changes
Jul 6, 2026
AndrienkoAleksandr
left a comment
AndrienkoAleksandr
left a comment
Contributor
There was a problem hiding this comment.
Great work, @PatAKnight! Tested, it works fine.
This was referenced Jul 6, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey, I just made a Pull Request!
Fixes a regression where conditional policy reload could wipe stored conditions when Catalog metadata was unavailable or when persistence failed partway through a reconcile.
Previously, the YAML file watcher and provider sync paths would remove conditions up front and then re-add them. If staging (metadata lookup) or a later create/update failed, the deletes had already landed and operators could end up with fewer conditions than before the reload.
This change makes reconcile non-destructive: we diff desired vs stored state, validate only pending additions, then apply updates, creates, and deletes in that order. If anything fails, we abort and leave the database as-is. Related conditions for the same role/resource (sibling merges) are handled by excluding pending deletes from conflict checks during create/update.
✔️ Checklist
Signed-off-byline in the message. (more info)