OSAC-1412: add DB triggers for catalog item referential integrity#686
Open
ygalblum wants to merge 1 commit into
Open
OSAC-1412: add DB triggers for catalog item referential integrity#686ygalblum wants to merge 1 commit into
ygalblum wants to merge 1 commit into
Conversation
|
@ygalblum: This pull request references OSAC-1412 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (3)
WalkthroughAdds migration 54: DB triggers and indexes enforce catalog-item references and block soft-deletes while active resources reference an item. Removes DAO-backed reference-checker wiring from private servers; tests switch from gomock to persisting real Cluster/ComputeInstance records exercising DB triggers. Risk: High — incorrect trigger logic may block valid deletes or reject valid writes. ChangesReferential Integrity Validation Refactoring
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resource as Cluster/ComputeInstance(table)
participant CatalogItem as CatalogItem(table)
Client->>Resource: INSERT/UPDATE spec.catalog_item
Resource->>CatalogItem: call check_*_catalog_item_ref() (FOR SHARE)
alt referenced exists & active
CatalogItem-->>Resource: allow
else missing or soft-deleted
CatalogItem-->>Resource: raise Z0002 (abort)
end
Note over CatalogItem: On catalog-item soft-delete
Client->>CatalogItem: UPDATE deletion_timestamp != 'epoch'
CatalogItem->>Resource: call check_*_catalog_item_not_in_use()
alt active referencing rows exist
CatalogItem-->>Client: raise Z0003 (abort)
else no active references
CatalogItem-->>Client: allow soft-delete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✅ Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/database/migrations/54_add_catalog_item_ref_triggers_test.go`:
- Around line 190-208: The test currently only asserts the BEFORE INSERT trigger
'check_cluster_catalog_item_ref' on table 'clusters'—add assertions that the
same trigger exists for UPDATE (event_manipulation = 'UPDATE') and repeat
analogous checks for the compute instance trigger(s) referenced elsewhere; then
add behavior tests that exercise the UPDATE path: insert a valid cluster/compute
row, soft-delete or remove its referenced catalog_item, perform an UPDATE that
would keep the same catalog_item_id (or otherwise touch the row) and assert the
update fails or the constraint fires as expected (use the same test helpers and
DB connection used in the file to perform the UPDATE and validate
error/rollback). Ensure you modify the test cases that currently cover only
INSERT (the blocks around the 'check_cluster_catalog_item_ref' and the
compute-instance equivalent) to include both the registration assertion for
UPDATE and at least one failing UPDATE behavior case per resource.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 29954fae-77fd-4af7-a2a1-8d614381d886
📒 Files selected for processing (11)
internal/database/migrations/54_add_catalog_item_ref_triggers.up.sqlinternal/database/migrations/54_add_catalog_item_ref_triggers_test.gointernal/servers/catalog_item_reference_checker_mock.gointernal/servers/cluster_catalog_items_server.gointernal/servers/cluster_catalog_items_server_test.gointernal/servers/compute_instance_catalog_items_server.gointernal/servers/compute_instance_catalog_items_server_test.gointernal/servers/private_cluster_catalog_items_server.gointernal/servers/private_cluster_catalog_items_server_test.gointernal/servers/private_compute_instance_catalog_items_server.gointernal/servers/private_compute_instance_catalog_items_server_test.go
💤 Files with no reviewable changes (3)
- internal/servers/catalog_item_reference_checker_mock.go
- internal/servers/compute_instance_catalog_items_server.go
- internal/servers/cluster_catalog_items_server.go
ygalblum
force-pushed
the
fix/catalog-item-fix-toctou-race
branch
from
af3d8ca to
b3f5626
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/database/migrations/54_add_catalog_item_ref_triggers.up.sql`:
- Around line 38-42: The trigger SQL is matching catalog items only by id/name,
allowing cross-tenant matches; update the WHERE clauses in the SELECTs (the ones
using data->'spec'->>'catalog_item' = old.id or old.name) to also constrain by
tenant scope (e.g., compare the cluster's tenant column or the tenant field
inside data->spec to old.tenant_id or old.tenant) so only rows from the same
tenant are considered; apply this tenant-scoping change to every occurrence (the
blocks around the current select into child_id and the similar selects at the
other mentioned locations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 001a58e2-51d8-4dc3-b4be-dfcc791c26c0
📒 Files selected for processing (11)
internal/database/migrations/54_add_catalog_item_ref_triggers.up.sqlinternal/database/migrations/54_add_catalog_item_ref_triggers_test.gointernal/servers/catalog_item_reference_checker_mock.gointernal/servers/cluster_catalog_items_server.gointernal/servers/cluster_catalog_items_server_test.gointernal/servers/compute_instance_catalog_items_server.gointernal/servers/compute_instance_catalog_items_server_test.gointernal/servers/private_cluster_catalog_items_server.gointernal/servers/private_cluster_catalog_items_server_test.gointernal/servers/private_compute_instance_catalog_items_server.gointernal/servers/private_compute_instance_catalog_items_server_test.go
💤 Files with no reviewable changes (3)
- internal/servers/cluster_catalog_items_server.go
- internal/servers/catalog_item_reference_checker_mock.go
- internal/servers/compute_instance_catalog_items_server.go
ygalblum
force-pushed
the
fix/catalog-item-fix-toctou-race
branch
from
b3f5626 to
5120d60
Compare
Replace the application-level delete-time reference check with bidirectional PostgreSQL triggers for catalog item referential integrity, while preserving the shared reference checker for unpublished-item visibility in public servers. The previous query-then-delete pattern in the private catalog item servers had a TOCTOU race condition: a new referencing row could be inserted between the check and the soft-delete. Database triggers with FOR SHARE locking eliminate this race window entirely. - Added migration 54 with bidirectional triggers enforcing referential integrity between catalog items and their consumers (clusters, compute instances) - Parent-side triggers prevent soft-deleting a catalog item that is still referenced by active children (raises Z0003 / ErrInUse) - Child-side triggers prevent creating or updating a resource with a reference to a non-existent or soft-deleted catalog item (raises Z0002 / ErrReference) - Triggers resolve catalog item references by both id and name, matching the server-side lookup behavior that allows users to specify either - Added btree indexes on the catalog_item JSONB field for both child tables - Removed the application-level reference checker from private catalog item servers — delete integrity is now enforced by DB triggers - Kept the shared catalogItemReferenceChecker interface in public servers for unpublished catalog item visibility checks - Deleted the mock reference checker (tests now use real DAO queries) Migration tests covering trigger and index existence, parent-side enforcement (Z0003), and child-side enforcement (Z0002) for both catalog item types. Server tests updated to use real DAO queries for unpublished-item visibility and to assert trigger-based FailedPrecondition errors on delete. Assisted-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
ygalblum
force-pushed
the
fix/catalog-item-fix-toctou-race
branch
from
5120d60 to
4d4c5a3
Compare
Contributor
Author
|
/retest |
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
2 participants
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.
Replace the application-level delete-time reference check with bidirectional PostgreSQL triggers for catalog item referential integrity, while preserving the shared reference checker for unpublished-item visibility in public servers.
The previous query-then-delete pattern in the private catalog item servers had a TOCTOU race condition: a new referencing row could be inserted between the check and the soft-delete. Database triggers with FOR SHARE locking eliminate this race window entirely.
Migration tests covering trigger and index existence, parent-side enforcement (Z0003), and child-side enforcement (Z0002) for both catalog item types. Server tests updated to use real DAO queries for unpublished-item visibility and to assert trigger-based FailedPrecondition errors on delete.
Assisted-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests