Skip to content

Commit 894aac3

Browse files
kuudorikuudori
committed
HYPERFLEET-971 - feat: reject cluster patch on soft-deleted cluster
1 parent 944134f commit 894aac3

4 files changed

Lines changed: 202 additions & 1 deletion

File tree

‎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)