HYPERFLEET-1155 - feat: add force-delete endpoint for generic resources#266
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds force-delete handling for resources, with handler routing now branching on Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceHandler
participant sqlResourceService
participant resourceDao
participant registry
Client->>ResourceHandler: POST /.../force-delete {reason}
ResourceHandler->>ResourceHandler: validate reason
ResourceHandler->>ResourceHandler: verifyOwnership if parent_id present
ResourceHandler->>sqlResourceService: ForceDelete(kind, id, reason)
sqlResourceService->>resourceDao: GetForUpdate(kind, id)
sqlResourceService->>registry: ChildrenOf(kind)
sqlResourceService->>resourceDao: FindByKindAndOwnerForUpdate(...)
sqlResourceService->>resourceDao: Delete(resource)
sqlResourceService-->>ResourceHandler: success or ServiceError
ResourceHandler-->>Client: 204 or mapped error
Related PRs: None provided. Suggested labels: enhancement, security Suggested reviewers: None provided. CWE-284: ownership checks in CWE-863: destructive hard-delete flow depends on upstream authorization; confirm policy enforcement on the new routes. CWE-362: recursive locking across parent/child force-delete paths should be checked for deadlock and race conditions. 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 1019 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: plugins/entities | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@pkg/services/resource_test.go`:
- Around line 1318-1328: The tripwire test is only validating local fixtures
because setupTestDescriptors() rebuilds the registry, so it never checks real
plugin descriptors. Move the RequiredAdapters assertion out of
TestAllGenericDescriptors_HaveNoRequiredAdapters and into a test that loads the
actual generic-resource plugins, then iterate registry.All() to verify the
registered descriptors from the real registry. Keep the existing Expect check
and failure message, but ensure it runs against the production plugin
registration path rather than the test fixture registry.
In `@test/integration/resource_force_delete_test.go`:
- Around line 110-139: The NestedVersionForceDelete subtest in the resource
force-delete integration test only verifies that the Version identified by
versionID is removed, but it does not confirm that the parent channel remains
intact. Update this test around the existing ForceDelete and checkResourceCount
assertions to also assert that channel.ID still exists after the nested version
delete, using the existing helpers in setupResourceTest, createChannel, and
checkResourceCount so the parent scope is explicitly validated.
- Around line 22-141: The current `TestResourceForceDelete` suite only calls
`svc.ForceDelete`, so it misses regressions in
`pkg/handlers/resource_handler.go` and the route/plugin wiring for `POST
/{plural}/{id}/force-delete` and the nested owner-scoped force-delete path.
Update these subtests to use the integration HTTP setup from
`test.RegisterIntegration(t)` (helper, client) with Resty and Gomega, send real
requests through the handler routes, and assert on response status/body plus
`reason` validation instead of only checking `ServiceError.HTTPCode`. Keep the
DB cleanup assertions, but locate the tests by `TestResourceForceDelete`,
`setupResourceTest`, and `ForceDelete` when refactoring.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cd5c2875-03f9-424c-a4fe-6dd27eb0ccdd
📒 Files selected for processing (8)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/services/resource.gopkg/services/resource_test.goplugins/channels/plugin.goplugins/versions/plugin.goplugins/wifconfigs/plugin.gotest/integration/resource_force_delete_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
8779683 to
c0d96a5
Compare
|
JIRA description says "soft-deleting" but per ADR-0013 the intent is hard-delete. Worth updating the ticket wording to avoid future confusion. |
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
c0d96a5 to
1c2f75c
Compare
| if _, err := h.service.GetByOwner(r.Context(), h.descriptor.Kind, id, parentID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := h.service.ForceDelete(r.Context(), h.descriptor.Kind, id, req.Reason); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nit: Wdyt about creating a ForceDeleteByOwner(ctx, kind, id, parentID, reason) for this one?
The two-call approach works correctly (no TOCTOU — ForceDelete re-locks via GetForUpdate), but breaks the handler's "validate request, delegate once" pattern.
There was a problem hiding this comment.
I'd stick with the current implementation. The handler just calls the service layer, no additional logic 🙂
A dedicated service method would also create drift if the delete flow changes later...
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
1c2f75c to
8da9e50
Compare
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
8da9e50 to
9a28571
Compare
|
/test unit |
|
/test presubmits-integration |
| @@ -53,6 +54,7 @@ func RegisterEntityRoutes(apiV1Router *mux.Router, resourceService services.Reso | |||
| pr.HandleFunc("/{id}", h.GetByOwner).Methods(http.MethodGet) | |||
| pr.HandleFunc("/{id}", h.PatchByOwner).Methods(http.MethodPatch) | |||
| pr.HandleFunc("/{id}", h.DeleteByOwner).Methods(http.MethodDelete) | |||
| pr.HandleFunc("/{id}/force-delete", h.ForceDeleteByOwner).Methods(http.MethodPost) | |||
There was a problem hiding this comment.
These ByOwner methods start looking a bit suspicious to me.
Is the only difference that they query adding AND owner_id=?
Is that protection really needed on read?
My rationale:
- If we make sure that the write path can not store a child item without its proper parent...
- Do we need to check for every child read, that it belongs to a parent?
Add POST /{plural}/{id}/force-delete to ResourceHandler for all generic
entity types (channels, versions, wifconfigs). The endpoint bypasses
OnParentDelete policies and recursively hard-deletes the entire ownership
tree for resources in Finalizing state (deleted_time IS NOT NULL).
Key design decisions per ADR-0013 (DB-only force-delete):
- Requires Finalizing state and a reason field in the request body
- Bottom-up hard-delete ordering per ADR-0012
- Fail-loud guard if RequiredAdapters non-empty (tripwire for HYPERFLEET-1154)
- MarkForRollback on error prevents partial tree deletion from committing
- Audit log with structured fields before each resource deletion
Includes unit tests (handler + service + tripwire) and integration tests
confirming force-delete succeeds where normal DELETE returns 409 due to
Restrict policy.
Also collapses the flat/owner-nested method pairs on ResourceHandler
(Create/CreateWithOwner, Get/GetByOwner, List/ListByOwner, Patch/PatchByOwner,
Delete/DeleteByOwner, ForceDelete/ForceDeleteByOwner) into six methods that
branch on parent_id presence in mux.Vars(r). The split pairs were identical
logic with a parent-check bolted on the front — exactly the shape where a
fix lands in one and is forgotten in the other. Route registration in
plugins/entities/plugin.go collapses the same way: flat and nested routes
now share one prefix-builder and one set of HandleFunc calls instead of two
duplicated blocks.
Patch/Delete/ForceDelete share a verifyOwnership helper (parent-check only,
no parent data needed). Create/Get/List keep the branch inline since they
use the parent's ID/Kind/Href or a different *ByOwner service call — the two
shapes aren't unifiable under one helper without leaking either interface.
The owner_id predicate in GetByOwner/PatchByOwner-style checks stays: the
write path guarantees a child has a valid parent (referential integrity),
but not that the URL's parent matches the URL's child (request scoping).
Without the check, GET /parents/B/children/X returns X even if X actually
belongs to parent A — wrong-404 today, an IDOR the moment any authorization
is scoped by parent. Service-layer Get/GetByOwner and List/ListByOwner stay
separate rather than merging into an ownerID-optional signature — an empty
string standing in for "unscoped" is a stringly-typed sentinel on a
security-relevant filter, indistinguishable from a caller that forgot to
populate it.
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
9a28571 to
7fe740f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/handlers/resource_handler.go (1)
49-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConvert the documented silent-misroute risk into a fail-loud guard.
The comment at Lines 16-23 concedes that if the registration invariant is ever bypassed (a nested
ParentKind != ""descriptor wired to a flat route),Createsilently skips owner references instead of erroring. That failure mode is a broken owner-scoping / authorization gap (CWE-863): a child persisted with noOwnerIDescapes every subsequentGetByOwner/verifyOwnershipboundary. Relying only on cross-file wiring inplugins/entities/plugin.gois fragile — a single asserted check makes the mismatch loud.Proposed guard (illustrative)
// ownerScope resolves parent_id and enforces that presence matches the descriptor. func (h *ResourceHandler) ownerScope(r *http.Request) (parentID string, nested bool, err *errors.ServiceError) { parentID, present := mux.Vars(r)["parent_id"] if present != (h.descriptor.ParentKind != "") { return "", false, errors.GeneralError( "route/descriptor mismatch for kind %q: parent_id present=%v, ParentKind=%q", h.descriptor.Kind, present, h.descriptor.ParentKind) } return parentID, present, nil }🤖 Prompt for 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. In `@pkg/handlers/resource_handler.go` around lines 49 - 66, Add a fail-loud guard in ResourceHandler.Create so route ownership must match the descriptor’s nesting rules: if mux.Vars(r) contains parent_id, h.descriptor.ParentKind must be set, and if it does not, h.descriptor.ParentKind must be empty. Factor this check into a small helper near Create (for example, an ownerScope-style helper) and return a GeneralError on mismatch before calling presenters.ConvertResource / ConvertResourceWithOwner, so a miswired nested descriptor on a flat route cannot silently skip OwnerID assignment.
🤖 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.
Nitpick comments:
In `@pkg/handlers/resource_handler.go`:
- Around line 49-66: Add a fail-loud guard in ResourceHandler.Create so route
ownership must match the descriptor’s nesting rules: if mux.Vars(r) contains
parent_id, h.descriptor.ParentKind must be set, and if it does not,
h.descriptor.ParentKind must be empty. Factor this check into a small helper
near Create (for example, an ownerScope-style helper) and return a GeneralError
on mismatch before calling presenters.ConvertResource /
ConvertResourceWithOwner, so a miswired nested descriptor on a flat route cannot
silently skip OwnerID assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 85c51194-7193-4532-a32c-333389b2c4cb
📒 Files selected for processing (7)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/services/resource.gopkg/services/resource_test.goplugins/entities/plugin.gotest/integration/resource_force_delete_test.gotest/integration/resource_helpers.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/integration/resource_helpers.go
- test/integration/resource_force_delete_test.go
- pkg/services/resource.go
- pkg/services/resource_test.go
4 participants
Summary
POST /{plural}/{id}/force-deletetoResourceHandlerfor all generic entity types (channels, versions, wifconfigs)Design
Per ADR-0013 (DB-only force-delete) and ADR-0012 (bottom-up deletion ordering):
deleted_time IS NOT NULL(409 otherwise)validateNotEmpty+validateMaxLengthforceDeleteResourceTreewalks all children regardless of OnParentDelete policy, hard-deletes bottom-upMarkForRollbackon any error prevents partial commitsTestAllGenericDescriptors_HaveNoRequiredAdaptersbreaks CI when assumption changesTest plan
TestAllGenericDescriptors_HaveNoRequiredAdaptersmake verify-allpasses (1322 tests)make test-integrationpasses (force-delete scenarios)