HYPERFLEET-971 - feat: reject nodepool create/patch on soft-deleted cluster #113
Conversation
WalkthroughAdds a new RFC 9457 conflict error code Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as ClusterHandler / NodePoolsHandler
participant Service as ClusterService / NodePoolService
participant Errors as errors.Registry
Client->>Handler: HTTP PATCH/POST (cluster/nodepool)
Handler->>Service: Get(clusterID)
Service-->>Handler: cluster (maybe nil)
alt cluster missing
Handler-->>Client: 404 Not Found
else cluster present
alt cluster.DeletedTime != nil
Handler->>Errors: ConflictState("Cluster '%s' is marked for deletion", id)
Errors-->>Handler: ProblemDetails (409, HYPERFLEET-CNF-003)
Handler-->>Client: 409 Conflict (problem-details)
else cluster active
opt nodepool flow
Handler->>Service: Get(nodepoolID)
Service-->>Handler: nodepool (maybe nil)
alt nodepool missing
Handler-->>Client: 404 Not Found
else nodepool present
alt nodepool.DeletedTime != nil
Handler->>Errors: ConflictState("Nodepool '%s' is marked for deletion", id)
Errors-->>Handler: ProblemDetails (409, HYPERFLEET-CNF-003)
Handler-->>Client: 409 Conflict
else nodepool active
Handler->>Service: Replace/Create(...)
Service-->>Handler: updated resource
Handler-->>Client: 200/201 with body
end
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
accidentally clicked generate unit tests under the coderabbit comment, deleted his comments with an unsuccessful attempt to do that |
|
Nice work! The soft-delete guards on the cluster-scoped endpoints look solid. One thing I noticed: the standalone Worth adding the same guard there for consistency — or at least tracking it as a follow-up. |
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/handlers/cluster_nodepools.go`:
- Around line 193-200: The soft-delete check on cluster.DeletedTime in the
handler (after clusterService.Get) is racy; move this validation into the
service/DAO layer and enforce it atomically with the write. Update the nodepool
creation/replacement paths (the service methods that call Create()/Replace() on
the DAO) to either re-check DeletedTime inside the same transaction or add a
precondition to the DB write (e.g., include WHERE cluster.deleted_time IS NULL
in the INSERT/UPDATE SQL) so Create()/Replace() fail if the cluster is marked
deleted; ensure the service/DAO returns a clear ConflictState error when that
condition is hit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9ca03664-3434-4b51-9885-8613945b3352
📒 Files selected for processing (5)
pkg/errors/errors.gopkg/handlers/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_nodepools_test.gopkg/handlers/cluster_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/errors/errors.go
- pkg/handlers/cluster.go
|
/retest |
|
Nice work on the soft-delete guards! One thing to consider as a follow-up: The status endpoints ( |
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 `@openapi/openapi.yaml`:
- Around line 128-129: Add a 409 response body schema for the three explicit
conflict responses so the OpenAPI clients know the shape: for each endpoint
PATCH /api/hyperfleet/v1/clusters/{cluster_id}, POST
/api/hyperfleet/v1/clusters/{cluster_id}/nodepools, and PATCH
/api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} replace the
bare 409 description with a content section declaring "application/problem+json"
and reference the Problem Details schema (or inline the RFC7807-like fields:
type, title, status, detail, instance) so generated clients can validate the
conflict response. Ensure the media type key is "application/problem+json" and
the schema is either $ref to the shared ProblemDetails component or an
equivalent object schema.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e6a7ef09-e98d-4fdb-badd-6066a0ac1b56
📒 Files selected for processing (5)
openapi/openapi.yamlpkg/errors/errors.gopkg/handlers/cluster.gopkg/handlers/cluster_nodepools_test.gopkg/handlers/cluster_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/errors/errors.go
- pkg/handlers/cluster_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/handlers/cluster.go
- pkg/handlers/cluster_nodepools_test.go
| nodePoolID := testNodePoolID | ||
|
|
||
| tests := []struct { | ||
| setupMocks func(ctrl *gomock.Controller) ( //nolint:lll |
There was a problem hiding this comment.
Just minor thing not related to your changes. I notice we have these //nolint:lll on multiple lines in this file. Since code it well formatted, this doesn't have to be there. If you feel up to it, you can remove them
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene 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 |
openshift-merge-bot
Bot
merged commit 20f9bf8
into
openshift-hyperfleet:main
3 participants
Prevent nodepool creation and updates on clusters marked for deletion by returning 409 Conflict, avoiding orphaned resources after cluster cleanup.
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation