Skip to content

HYPERFLEET-1155 - feat: add force-delete endpoint for generic resources #266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kuudori wants to merge 1 commit into
base: main
Choose a base branch
from
Open

HYPERFLEET-1155 - feat: add force-delete endpoint for generic resources #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions pkg/handlers/resource_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,3 +275,47 @@ func (h *ResourceHandler) DeleteByOwner(w http.ResponseWriter, r *http.Request)
}
handleSoftDelete(w, r, cfg)
}

func (h *ResourceHandler) ForceDelete(w http.ResponseWriter, r *http.Request) {
var req openapi.ForceDeleteRequest
cfg := &handlerConfig{
MarshalInto: &req,
Validate: []validate{
validateNotEmpty(&req, "Reason", "reason"),
validateMaxLength(&req, "Reason", "reason", maxReasonLength),
},
Action: func() (interface{}, *errors.ServiceError) {
id := mux.Vars(r)["id"]
if err := h.service.ForceDelete(r.Context(), h.descriptor.Kind, id, req.Reason); err != nil {
return nil, err
}
return nil, nil
},
}
handleForceDelete(w, r, cfg)
}

func (h *ResourceHandler) ForceDeleteByOwner(w http.ResponseWriter, r *http.Request) {
var req openapi.ForceDeleteRequest
cfg := &handlerConfig{
MarshalInto: &req,
Validate: []validate{
validateNotEmpty(&req, "Reason", "reason"),
validateMaxLength(&req, "Reason", "reason", maxReasonLength),
},
Action: func() (interface{}, *errors.ServiceError) {
vars := mux.Vars(r)
parentID, id := vars["parent_id"], vars["id"]

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
}
Comment on lines +310 to +316

@tirthct tirthct Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kuudori kuudori Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

return nil, nil
},
}
handleForceDelete(w, r, cfg)
}
172 changes: 172 additions & 0 deletions pkg/handlers/resource_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,3 +650,175 @@ func TestResourceHandler_DeleteByOwner(t *testing.T) {
})
}
}

func TestResourceHandler_ForceDelete(t *testing.T) {
RegisterTestingT(t)

resourceID := "ch-123"

tests := []struct {
setupMock func(mock *services.MockResourceService)
name string
body string
expectedStatusCode int
}{
{
name: "Success 204 - resource force-deleted",
body: `{"reason": "Stuck in finalizing for 2 hours"}`,
setupMock: func(mock *services.MockResourceService) {
mock.EXPECT().
ForceDelete(gomock.Any(), "Channel", resourceID, "Stuck in finalizing for 2 hours").
Return(nil)
},
expectedStatusCode: http.StatusNoContent,
},
{
name: "Error 400 - malformed JSON",
body: `not json`,
setupMock: func(mock *services.MockResourceService) {
},
expectedStatusCode: http.StatusBadRequest,
},
{
name: "Error 400 - empty reason",
body: `{"reason": ""}`,
setupMock: func(mock *services.MockResourceService) {
},
expectedStatusCode: http.StatusBadRequest,
},
{
name: "Error 400 - reason exceeds max length",
body: `{"reason": "` + strings.Repeat("x", maxReasonLength+1) + `"}`,
setupMock: func(mock *services.MockResourceService) {
},
expectedStatusCode: http.StatusBadRequest,
},
{
name: "Error 404 - resource not found",
body: `{"reason": "some reason"}`,
setupMock: func(mock *services.MockResourceService) {
mock.EXPECT().
ForceDelete(gomock.Any(), "Channel", resourceID, "some reason").
Return(errors.NotFound("Channel with id='%s' not found", resourceID))
},
expectedStatusCode: http.StatusNotFound,
},
{
name: "Error 409 - resource not in Finalizing state",
body: `{"reason": "some reason"}`,
setupMock: func(mock *services.MockResourceService) {
mock.EXPECT().
ForceDelete(gomock.Any(), "Channel", resourceID, "some reason").
Return(errors.ConflictState("Channel '%s' is not in Finalizing state", resourceID))
},
expectedStatusCode: http.StatusConflict,
},
{
name: "Error 500 - service internal error",
body: `{"reason": "some reason"}`,
setupMock: func(mock *services.MockResourceService) {
mock.EXPECT().
ForceDelete(gomock.Any(), "Channel", resourceID, "some reason").
Return(errors.GeneralError("database connection lost"))
},
expectedStatusCode: http.StatusInternalServerError,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
RegisterTestingT(t)

ctrl := gomock.NewController(t)
defer ctrl.Finish()

handler, mockSvc := newTestResourceHandler(ctrl)
tt.setupMock(mockSvc)

reqURL := "/api/hyperfleet/v1/channels/" + resourceID + "/force-delete"
req := httptest.NewRequest(http.MethodPost, reqURL, strings.NewReader(tt.body))
req.Header.Set("Content-Type", "application/json")
req = mux.SetURLVars(req, map[string]string{"id": resourceID})

rr := httptest.NewRecorder()
handler.ForceDelete(rr, req)

Expect(rr.Code).To(Equal(tt.expectedStatusCode))

if tt.expectedStatusCode == http.StatusNoContent {
Expect(rr.Body.Len()).To(Equal(0))
}
})
}
}

func TestResourceHandler_ForceDeleteByOwner(t *testing.T) {
RegisterTestingT(t)

parentID := "ch-1"
versionID := "v-1"

tests := []struct {
setupMock func(mock *services.MockResourceService)
name string
body string
expectedStatusCode int
}{
{
name: "Success 204 - nested resource force-deleted",
body: `{"reason": "Stuck in finalizing"}`,
setupMock: func(mock *services.MockResourceService) {
mock.EXPECT().
GetByOwner(gomock.Any(), "Version", versionID, parentID).
Return(&api.Resource{Meta: api.Meta{ID: versionID}, Kind: "Version"}, nil)
mock.EXPECT().
ForceDelete(gomock.Any(), "Version", versionID, "Stuck in finalizing").
Return(nil)
},
expectedStatusCode: http.StatusNoContent,
},
{
name: "Error 404 - ownership mismatch",
body: `{"reason": "some reason"}`,
setupMock: func(mock *services.MockResourceService) {
mock.EXPECT().
GetByOwner(gomock.Any(), "Version", versionID, parentID).
Return(nil, errors.NotFound("Version with id='%s' not found for owner '%s'", versionID, parentID))
},
expectedStatusCode: http.StatusNotFound,
},
{
name: "Error 400 - empty reason",
body: `{"reason": ""}`,
setupMock: func(mock *services.MockResourceService) {
},
expectedStatusCode: http.StatusBadRequest,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
RegisterTestingT(t)

ctrl := gomock.NewController(t)
defer ctrl.Finish()

handler, mockSvc := newTestVersionHandler(ctrl)
tt.setupMock(mockSvc)

reqURL := "/api/hyperfleet/v1/channels/" + parentID + "/versions/" + versionID + "/force-delete"
req := httptest.NewRequest(http.MethodPost, reqURL, strings.NewReader(tt.body))
req.Header.Set("Content-Type", "application/json")
req = mux.SetURLVars(req, map[string]string{"parent_id": parentID, "id": versionID})

rr := httptest.NewRecorder()
handler.ForceDeleteByOwner(rr, req)

Expect(rr.Code).To(Equal(tt.expectedStatusCode))

if tt.expectedStatusCode == http.StatusNoContent {
Expect(rr.Body.Len()).To(Equal(0))
}
})
}
}
69 changes: 69 additions & 0 deletions pkg/services/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao"
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/db"
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors"
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger"
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry"
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/util"
)
Expand All @@ -24,6 +25,7 @@ type ResourceService interface {
List(ctx context.Context, kind string, args *ListArguments) (api.ResourceList, *api.PagingMeta, *errors.ServiceError)
GetByOwner(ctx context.Context, kind, id, ownerID string) (*api.Resource, *errors.ServiceError)
ListByOwner(ctx context.Context, kind, ownerID string, args *ListArguments) (api.ResourceList, *api.PagingMeta, *errors.ServiceError) // nolint:lll
ForceDelete(ctx context.Context, kind, id, reason string) *errors.ServiceError
}

func NewResourceService(resourceDao dao.ResourceDao, generic GenericService) ResourceService {
Expand Down Expand Up @@ -355,3 +357,70 @@ func applyResourcePatch(resource *api.Resource, patch *api.ResourcePatch) error
// via dao.ReplaceReferences per generic-resource-registry-design.md §9.2
return nil
}

func (s *sqlResourceService) ForceDelete(ctx context.Context, kind, id, reason string) *errors.ServiceError {
if svcErr := validateKind(kind); svcErr != nil {
return svcErr
}

resource, err := s.resourceDao.GetForUpdate(ctx, kind, id)
if err != nil {
return handleGetError(kind, "id", id, err)
}

if resource.DeletedTime == nil {
return errors.ConflictState("%s '%s' is not in Finalizing state", kind, id)
}

caller := actorFromContext(ctx)
if svcErr := s.forceDeleteResourceTree(ctx, resource, caller, reason); svcErr != nil {
db.MarkForRollback(ctx, svcErr)
return svcErr
}
return nil
}

func (s *sqlResourceService) forceDeleteResourceTree(
ctx context.Context, resource *api.Resource, caller, reason string,
) *errors.ServiceError {
desc := registry.MustGet(resource.Kind)
if len(desc.RequiredAdapters) > 0 {
return errors.GeneralError(
"force-delete not implemented for resources with required adapters (kind=%s)"+
" — adapter_status cleanup needed, see HYPERFLEET-1154",
resource.Kind,
)
}
Comment thread
kuudori marked this conversation as resolved.

children := registry.ChildrenOf(resource.Kind)

childIDs := make([]string, 0)
for _, child := range children {
items, err := s.resourceDao.FindByKindAndOwnerForUpdate(ctx, child.Kind, resource.ID)
if err != nil {
logger.With(ctx, "resource_id", resource.ID, "child_kind", child.Kind).
WithError(err).Error("Failed to find children for force-delete")
return errors.GeneralError("Unable to find %s children for force-delete", child.Kind)
}
Comment thread
kuudori marked this conversation as resolved.
for _, item := range items {
childIDs = append(childIDs, item.ID)
if svcErr := s.forceDeleteResourceTree(ctx, item, caller, reason); svcErr != nil {
return svcErr
}
}
}

logger.With(ctx,
"resource_kind", resource.Kind,
"resource_id", resource.ID,
"caller", caller,
"reason", reason,
"child_resource_ids", childIDs,
).Info("Force-deleting resource")

if err := s.resourceDao.Delete(ctx, resource.Kind, resource.ID); err != nil {
return handleDeleteError(resource.Kind, err)
}

return nil
}
Loading