Skip to content

Commit 65f3aad

Browse files
committed
HYPERFLEET-971 - feat: reject cluster patch on soft-deleted cluster
1 parent 944134f commit 65f3aad

5 files changed

Lines changed: 217 additions & 10 deletions

File tree

openapi/openapi.yaml

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
openapi: 3.0.0
22
info:
33
title: HyperFleet API
4-
version: 1.0.8
4+
version: 1.0.9
55
contact:
66
name: HyperFleet Team
77
license:
@@ -125,6 +125,8 @@ paths:
125125
description: The server could not understand the request due to invalid syntax.
126126
'404':
127127
description: The server cannot find the requested resource.
128+
'409':
129+
description: The request conflicts with the current state of the server.
128130
default:
129131
description: An unexpected error response.
130132
content:
@@ -173,8 +175,8 @@ paths:
173175
conditions:
174176
- type: Ready
175177
status: 'True'
176-
reason: All adapters reported Ready True for the current generation
177-
message: All adapters reported Ready True for the current generation
178+
reason: All adapters reported Available=True for the current generation
179+
message: All adapters reported Available=True for the current generation
178180
observed_generation: 2
179181
created_time: '2021-01-01T10:00:00Z'
180182
last_updated_time: '2021-01-01T10:00:00Z'
@@ -283,6 +285,8 @@ paths:
283285
$ref: '#/components/schemas/NodePoolCreateResponse'
284286
'400':
285287
description: The server could not understand the request due to invalid syntax.
288+
'409':
289+
description: The request conflicts with the current state of the server.
286290
default:
287291
description: An unexpected error response.
288292
content:
@@ -376,8 +380,8 @@ paths:
376380
conditions:
377381
- type: Ready
378382
status: 'True'
379-
reason: All adapters reported Ready True for the current generation
380-
message: All adapters reported Ready True for the current generation
383+
reason: All adapters reported Available=True for the current generation
384+
message: All adapters reported Available=True for the current generation
381385
observed_generation: 2
382386
created_time: '2021-01-01T10:00:00Z'
383387
last_updated_time: '2021-01-01T10:00:00Z'
@@ -460,6 +464,8 @@ paths:
460464
description: The server could not understand the request due to invalid syntax.
461465
'404':
462466
description: The server cannot find the requested resource.
467+
'409':
468+
description: The request conflicts with the current state of the server.
463469
default:
464470
description: An unexpected error response.
465471
content:
@@ -1055,8 +1061,8 @@ components:
10551061
conditions:
10561062
- type: Ready
10571063
status: 'True'
1058-
reason: All adapters reported Ready True for the current generation
1059-
message: All adapters reported Ready True for the current generation
1064+
reason: All adapters reported Available=True for the current generation
1065+
message: All adapters reported Available=True for the current generation
10601066
observed_generation: 1
10611067
created_time: '2021-01-01T10:00:00Z'
10621068
last_updated_time: '2021-01-01T10:00:00Z'
@@ -1354,8 +1360,8 @@ components:
13541360
conditions:
13551361
- type: Ready
13561362
status: 'True'
1357-
reason: All adapters reported Ready True for the current generation
1358-
message: All adapters reported Ready True for the current generation
1363+
reason: All adapters reported Available=True for the current generation
1364+
message: All adapters reported Available=True for the current generation
13591365
observed_generation: 1
13601366
created_time: '2021-01-01T10:00:00Z'
13611367
last_updated_time: '2021-01-01T10:00:00Z'

pkg/errors/errors.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ var errorDefinitions = map[string]errorDefinition{
169169
},
170170
CodeConflictState: {
171171
ErrorTypeConflict, "State Conflict",
172-
"Operation not allowed in current resource state", http.StatusConflict,
172+
"Operation not allowed in current state", http.StatusConflict,
173173
},
174174

175175
// Rate Limit errors (LMT) - 429

pkg/handlers/cluster.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,10 @@ func (h ClusterHandler) Patch(w http.ResponseWriter, r *http.Request) {
7676
return nil, err
7777
}
7878

79+
if found.DeletedTime != nil {
80+
return nil, errors.ConflictState("Cluster '%s' is marked for deletion", id)
81+
}
82+
7983
if patch.Spec != nil {
8084
specJSON, err := json.Marshal(*patch.Spec)
8185
if err != nil {

pkg/handlers/cluster_nodepools_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -639,6 +639,62 @@ func TestClusterNodePoolsHandler_Patch(t *testing.T) {
639639
expectedStatusCode: http.StatusNotFound,
640640
expectedError: true,
641641
},
642+
{
643+
name: "Error 404 - NodePool not found",
644+
clusterID: clusterID,
645+
nodePoolID: "non-existent-np",
646+
setupMocks: func(ctrl *gomock.Controller) (
647+
*services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService,
648+
) {
649+
mockClusterSvc := services.NewMockClusterService(ctrl)
650+
mockNodePoolSvc := services.NewMockNodePoolService(ctrl)
651+
mockGenericSvc := services.NewMockGenericService(ctrl)
652+
653+
mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{
654+
Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now},
655+
Name: "test-cluster",
656+
}, nil)
657+
658+
mockNodePoolSvc.EXPECT().Get(gomock.Any(), "non-existent-np").Return(nil, errors.NotFound("NodePool not found"))
659+
660+
return mockClusterSvc, mockNodePoolSvc, mockGenericSvc
661+
},
662+
expectedStatusCode: http.StatusNotFound,
663+
expectedError: true,
664+
},
665+
{
666+
name: "Error 404 - NodePool belongs to different cluster",
667+
clusterID: clusterID,
668+
nodePoolID: nodePoolID,
669+
setupMocks: func(ctrl *gomock.Controller) (
670+
*services.MockClusterService, *services.MockNodePoolService, *services.MockGenericService,
671+
) {
672+
mockClusterSvc := services.NewMockClusterService(ctrl)
673+
mockNodePoolSvc := services.NewMockNodePoolService(ctrl)
674+
mockGenericSvc := services.NewMockGenericService(ctrl)
675+
676+
mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{
677+
Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now},
678+
Name: "test-cluster",
679+
}, nil)
680+
681+
mockNodePoolSvc.EXPECT().Get(gomock.Any(), nodePoolID).Return(&api.NodePool{
682+
Meta: api.Meta{ID: nodePoolID, CreatedTime: now, UpdatedTime: now},
683+
Kind: "NodePool",
684+
Name: "test-nodepool",
685+
OwnerID: "different-cluster-id",
686+
Spec: []byte("{}"),
687+
Labels: []byte("{}"),
688+
StatusConditions: []byte("[]"),
689+
CreatedBy: "user@example.com",
690+
UpdatedBy: "user@example.com",
691+
}, nil)
692+
693+
return mockClusterSvc, mockNodePoolSvc, mockGenericSvc
694+
},
695+
expectedStatusCode: http.StatusNotFound,
696+
expectedError: true,
697+
},
642698
}
643699

644700
for _, tt := range tests {

pkg/handlers/cluster_test.go

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package handlers
2+
3+
import (
4+
"encoding/json"
5+
"net/http"
6+
"net/http/httptest"
7+
"strings"
8+
"testing"
9+
"time"
10+
11+
"github.com/gorilla/mux"
12+
. "github.com/onsi/gomega"
13+
"go.uber.org/mock/gomock"
14+
15+
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/api"
16+
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi"
17+
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors"
18+
"github.com/openshift-hyperfleet/hyperfleet-api/pkg/services"
19+
)
20+
21+
func TestClusterHandler_Patch(t *testing.T) {
22+
RegisterTestingT(t)
23+
24+
now := time.Now()
25+
clusterID := testClusterID
26+
validBody := `{"spec":{"region":"us-east1"}}`
27+
28+
tests := []struct {
29+
setupMocks func(ctrl *gomock.Controller) (*services.MockClusterService, *services.MockGenericService)
30+
name string
31+
clusterID string
32+
expectedStatusCode int
33+
expectedError bool
34+
}{
35+
{
36+
name: "Success - Patch active cluster",
37+
clusterID: clusterID,
38+
setupMocks: func(ctrl *gomock.Controller) (*services.MockClusterService, *services.MockGenericService) {
39+
mockClusterSvc := services.NewMockClusterService(ctrl)
40+
mockGenericSvc := services.NewMockGenericService(ctrl)
41+
42+
mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{
43+
Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now},
44+
Name: "test-cluster",
45+
Spec: []byte("{}"),
46+
Labels: []byte("{}"),
47+
StatusConditions: []byte("[]"),
48+
CreatedBy: testSystemUser,
49+
UpdatedBy: testSystemUser,
50+
}, nil)
51+
52+
mockClusterSvc.EXPECT().Replace(gomock.Any(), gomock.Any()).Return(&api.Cluster{
53+
Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now},
54+
Name: "test-cluster",
55+
Spec: []byte(`{"region":"us-east1"}`),
56+
Labels: []byte("{}"),
57+
StatusConditions: []byte("[]"),
58+
CreatedBy: testSystemUser,
59+
UpdatedBy: testSystemUser,
60+
}, nil)
61+
62+
return mockClusterSvc, mockGenericSvc
63+
},
64+
expectedStatusCode: http.StatusOK,
65+
expectedError: false,
66+
},
67+
{
68+
name: "Error 409 - Cluster is soft-deleted",
69+
clusterID: clusterID,
70+
setupMocks: func(ctrl *gomock.Controller) (*services.MockClusterService, *services.MockGenericService) {
71+
mockClusterSvc := services.NewMockClusterService(ctrl)
72+
mockGenericSvc := services.NewMockGenericService(ctrl)
73+
74+
deletedTime := now
75+
mockClusterSvc.EXPECT().Get(gomock.Any(), clusterID).Return(&api.Cluster{
76+
Meta: api.Meta{ID: clusterID, CreatedTime: now, UpdatedTime: now},
77+
Name: "test-cluster",
78+
DeletedTime: &deletedTime,
79+
}, nil)
80+
81+
return mockClusterSvc, mockGenericSvc
82+
},
83+
expectedStatusCode: http.StatusConflict,
84+
expectedError: true,
85+
},
86+
{
87+
name: "Error 404 - Cluster not found",
88+
clusterID: "non-existent",
89+
setupMocks: func(ctrl *gomock.Controller) (*services.MockClusterService, *services.MockGenericService) {
90+
mockClusterSvc := services.NewMockClusterService(ctrl)
91+
mockGenericSvc := services.NewMockGenericService(ctrl)
92+
93+
mockClusterSvc.EXPECT().Get(gomock.Any(), "non-existent").Return(nil, errors.NotFound("Cluster not found"))
94+
95+
return mockClusterSvc, mockGenericSvc
96+
},
97+
expectedStatusCode: http.StatusNotFound,
98+
expectedError: true,
99+
},
100+
}
101+
102+
for _, tt := range tests {
103+
t.Run(tt.name, func(t *testing.T) {
104+
RegisterTestingT(t)
105+
106+
ctrl := gomock.NewController(t)
107+
defer ctrl.Finish()
108+
109+
mockClusterSvc, mockGenericSvc := tt.setupMocks(ctrl)
110+
handler := NewClusterHandler(mockClusterSvc, mockGenericSvc)
111+
112+
reqURL := "/api/hyperfleet/v1/clusters/" + tt.clusterID
113+
req := httptest.NewRequest(http.MethodPatch, reqURL, strings.NewReader(validBody))
114+
req.Header.Set("Content-Type", "application/json")
115+
req = mux.SetURLVars(req, map[string]string{
116+
"id": tt.clusterID,
117+
})
118+
119+
rr := httptest.NewRecorder()
120+
handler.Patch(rr, req)
121+
122+
Expect(rr.Code).To(Equal(tt.expectedStatusCode))
123+
124+
if !tt.expectedError {
125+
var response openapi.Cluster
126+
err := json.Unmarshal(rr.Body.Bytes(), &response)
127+
Expect(err).NotTo(HaveOccurred())
128+
Expect(*response.Id).To(Equal(clusterID))
129+
}
130+
131+
if tt.expectedStatusCode == http.StatusConflict {
132+
var errResp openapi.Error
133+
err := json.Unmarshal(rr.Body.Bytes(), &errResp)
134+
Expect(err).NotTo(HaveOccurred())
135+
Expect(errResp.Status).To(Equal(http.StatusConflict))
136+
Expect(*errResp.Detail).To(ContainSubstring("marked for deletion"))
137+
Expect(*errResp.Code).To(Equal("HYPERFLEET-CNF-003"))
138+
}
139+
})
140+
}
141+
}

0 commit comments

Comments
 (0)