Skip to content

Commit a004e97

Browse files
Matthew BarnesSudoBrendan
Matthew Barnes
authored and
SudoBrendan
committed
frontend: Add CheckForConflict method
Returns a "409 Conflict" error response if the provisioning state of the resource or any parent resources (in the same namespace) should block a request from proceeding. Furthermore, if a DELETE request is accepted, mark any active operation on the resource as canceled. (Cluster Service will handle the actual cancellation of operations.)
1 parent 1464532 commit a004e97

File tree

5 files changed

+366
-5
lines changed

5 files changed

+366
-5
lines changed

‎frontend/pkg/frontend/frontend.go‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"strconv"
1717
"strings"
1818
"sync/atomic"
19+
"time"
1920

2021
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
2122
"golang.org/x/sync/errgroup"
@@ -408,6 +409,14 @@ func (f *Frontend) ArmResourceCreateOrUpdate(writer http.ResponseWriter, request
408409
doc = database.NewResourceDocument(resourceID)
409410
}
410411

412+
// CheckForProvisioningStateConflict does not log conflict errors
413+
// but does log unexpected errors like database failures.
414+
cloudError := f.CheckForProvisioningStateConflict(ctx, operationRequest, doc)
415+
if cloudError != nil {
416+
arm.WriteCloudError(writer, cloudError)
417+
return
418+
}
419+
411420
body, err := BodyFromContext(ctx)
412421
if err != nil {
413422
f.logger.Error(err.Error())
@@ -420,7 +429,7 @@ func (f *Frontend) ArmResourceCreateOrUpdate(writer http.ResponseWriter, request
420429
return
421430
}
422431

423-
cloudError := versionedRequestCluster.ValidateStatic(versionedCurrentCluster, updating, request.Method)
432+
cloudError = versionedRequestCluster.ValidateStatic(versionedCurrentCluster, updating, request.Method)
424433
if cloudError != nil {
425434
f.logger.Error(cloudError.Error())
426435
arm.WriteCloudError(writer, cloudError)
@@ -563,14 +572,44 @@ func (f *Frontend) ArmResourceDelete(writer http.ResponseWriter, request *http.R
563572
return
564573
}
565574

575+
operationRequest := database.OperationRequestDelete
576+
577+
// CheckForProvisioningStateConflict does not log conflict errors
578+
// but does log unexpected errors like database failures.
579+
cloudError := f.CheckForProvisioningStateConflict(ctx, operationRequest, doc)
580+
if cloudError != nil {
581+
arm.WriteCloudError(writer, cloudError)
582+
return
583+
}
584+
566585
err = f.clusterServiceClient.DeleteCSCluster(ctx, doc.InternalID)
567586
if err != nil {
568587
f.logger.Error(fmt.Sprintf("failed to delete cluster %s: %v", resourceID, err))
569588
arm.WriteInternalServerError(writer)
570589
return
571590
}
572591

573-
operationDoc, err := f.StartOperation(writer, request, doc, database.OperationRequestDelete)
592+
// Deletion is underway; mark any active operation as canceled.
593+
if doc.ActiveOperationID != "" {
594+
updated, err := f.dbClient.UpdateOperationDoc(ctx, doc.ActiveOperationID, func(updateDoc *database.OperationDocument) bool {
595+
if updateDoc.Status != arm.ProvisioningStateCanceled {
596+
updateDoc.LastTransitionTime = time.Now()
597+
updateDoc.Status = arm.ProvisioningStateCanceled
598+
return true
599+
}
600+
return false
601+
})
602+
if err != nil {
603+
f.logger.Error(err.Error())
604+
arm.WriteInternalServerError(writer)
605+
return
606+
}
607+
if updated {
608+
f.logger.Info(fmt.Sprintf("canceled operation '%s'", doc.ActiveOperationID))
609+
}
610+
}
611+
612+
operationDoc, err := f.StartOperation(writer, request, doc, operationRequest)
574613
if err != nil {
575614
f.logger.Error(fmt.Sprintf("failed to write operation document: %v", err))
576615
arm.WriteInternalServerError(writer)

‎frontend/pkg/frontend/helpers.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"errors"
99
"fmt"
1010
"net/http"
11+
"strings"
1112

1213
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
1314
ocmerrors "github.com/openshift-online/ocm-sdk-go/errors"
@@ -17,6 +18,57 @@ import (
1718
"github.com/Azure/ARO-HCP/internal/database"
1819
)
1920

21+
// CheckForProvisioningStateConflict returns a "409 Conflict" error response if the
22+
// provisioning state of the resource is non-terminal, or any of its parent resources
23+
// within the same provider namespace are in a "Deleting" state.
24+
func (f *Frontend) CheckForProvisioningStateConflict(ctx context.Context, operationRequest database.OperationRequest, doc *database.ResourceDocument) *arm.CloudError {
25+
switch operationRequest {
26+
case database.OperationRequestCreate:
27+
// Resource must already exist for there to be a conflict.
28+
case database.OperationRequestDelete:
29+
if doc.ProvisioningState == arm.ProvisioningStateDeleting {
30+
return arm.NewCloudError(
31+
http.StatusConflict,
32+
arm.CloudErrorCodeConflict,
33+
doc.Key.String(),
34+
"Resource is already deleting")
35+
}
36+
case database.OperationRequestUpdate:
37+
if !doc.ProvisioningState.IsTerminal() {
38+
return arm.NewCloudError(
39+
http.StatusConflict,
40+
arm.CloudErrorCodeConflict,
41+
doc.Key.String(),
42+
"Cannot update resource while resource is %s",
43+
strings.ToLower(string(doc.ProvisioningState)))
44+
}
45+
}
46+
47+
parent := doc.Key.GetParent()
48+
49+
// ResourceType casing is preserved for parents in the same namespace.
50+
for parent.ResourceType.Namespace == doc.Key.ResourceType.Namespace {
51+
parentDoc, err := f.dbClient.GetResourceDoc(ctx, parent)
52+
if err != nil {
53+
f.logger.Error(err.Error())
54+
return arm.NewInternalServerError()
55+
}
56+
57+
if parentDoc.ProvisioningState == arm.ProvisioningStateDeleting {
58+
return arm.NewCloudError(
59+
http.StatusConflict,
60+
arm.CloudErrorCodeConflict,
61+
doc.Key.String(),
62+
"Cannot %s resource while parent resource is deleting",
63+
strings.ToLower(string(operationRequest)))
64+
}
65+
66+
parent = parent.GetParent()
67+
}
68+
69+
return nil
70+
}
71+
2072
func (f *Frontend) MarshalResource(ctx context.Context, resourceID *arm.ResourceID, versionedInterface api.Version) ([]byte, *arm.CloudError) {
2173
var responseBody []byte
2274

‎frontend/pkg/frontend/helpers_test.go‎

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
package frontend
2+
3+
// Copyright (c) Microsoft Corporation.
4+
// Licensed under the Apache License 2.0.
5+
6+
import (
7+
"context"
8+
"fmt"
9+
"log/slog"
10+
"net/http"
11+
"testing"
12+
13+
"github.com/Azure/ARO-HCP/internal/api/arm"
14+
"github.com/Azure/ARO-HCP/internal/database"
15+
)
16+
17+
func TestCheckForProvisioningStateConflict(t *testing.T) {
18+
const clusterResourceID = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/testGroup/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/testCluster"
19+
const nodePoolResourceID = clusterResourceID + "/nodePools/testNodePool"
20+
21+
tests := []struct {
22+
name string
23+
resourceID string
24+
operationRequest database.OperationRequest
25+
directConflicts map[arm.ProvisioningState]bool
26+
parentConflicts map[arm.ProvisioningState]bool
27+
}{
28+
{
29+
name: "Create cluster",
30+
resourceID: clusterResourceID,
31+
operationRequest: database.OperationRequestCreate,
32+
directConflicts: map[arm.ProvisioningState]bool{
33+
arm.ProvisioningStateSucceeded: false,
34+
arm.ProvisioningStateFailed: false,
35+
arm.ProvisioningStateCanceled: false,
36+
arm.ProvisioningStateAccepted: false,
37+
arm.ProvisioningStateDeleting: false,
38+
arm.ProvisioningStateProvisioning: false,
39+
arm.ProvisioningStateUpdating: false,
40+
},
41+
},
42+
{
43+
name: "Delete cluster",
44+
resourceID: clusterResourceID,
45+
operationRequest: database.OperationRequestDelete,
46+
directConflicts: map[arm.ProvisioningState]bool{
47+
arm.ProvisioningStateSucceeded: false,
48+
arm.ProvisioningStateFailed: false,
49+
arm.ProvisioningStateCanceled: false,
50+
arm.ProvisioningStateAccepted: false,
51+
arm.ProvisioningStateDeleting: true,
52+
arm.ProvisioningStateProvisioning: false,
53+
arm.ProvisioningStateUpdating: false,
54+
},
55+
},
56+
{
57+
name: "Update cluster",
58+
resourceID: clusterResourceID,
59+
operationRequest: database.OperationRequestUpdate,
60+
directConflicts: map[arm.ProvisioningState]bool{
61+
arm.ProvisioningStateSucceeded: false,
62+
arm.ProvisioningStateFailed: false,
63+
arm.ProvisioningStateCanceled: false,
64+
arm.ProvisioningStateAccepted: true,
65+
arm.ProvisioningStateDeleting: true,
66+
arm.ProvisioningStateProvisioning: true,
67+
arm.ProvisioningStateUpdating: true,
68+
},
69+
},
70+
{
71+
name: "Create node pool",
72+
resourceID: nodePoolResourceID,
73+
operationRequest: database.OperationRequestCreate,
74+
directConflicts: map[arm.ProvisioningState]bool{
75+
arm.ProvisioningStateSucceeded: false,
76+
arm.ProvisioningStateFailed: false,
77+
arm.ProvisioningStateCanceled: false,
78+
arm.ProvisioningStateAccepted: false,
79+
arm.ProvisioningStateDeleting: false,
80+
arm.ProvisioningStateProvisioning: false,
81+
arm.ProvisioningStateUpdating: false,
82+
},
83+
parentConflicts: map[arm.ProvisioningState]bool{
84+
arm.ProvisioningStateSucceeded: false,
85+
arm.ProvisioningStateFailed: false,
86+
arm.ProvisioningStateCanceled: false,
87+
arm.ProvisioningStateAccepted: false,
88+
arm.ProvisioningStateDeleting: true,
89+
arm.ProvisioningStateProvisioning: false,
90+
arm.ProvisioningStateUpdating: false,
91+
},
92+
},
93+
{
94+
name: "Delete node pool",
95+
resourceID: nodePoolResourceID,
96+
operationRequest: database.OperationRequestDelete,
97+
directConflicts: map[arm.ProvisioningState]bool{
98+
arm.ProvisioningStateSucceeded: false,
99+
arm.ProvisioningStateFailed: false,
100+
arm.ProvisioningStateCanceled: false,
101+
arm.ProvisioningStateAccepted: false,
102+
arm.ProvisioningStateDeleting: true,
103+
arm.ProvisioningStateProvisioning: false,
104+
arm.ProvisioningStateUpdating: false,
105+
},
106+
parentConflicts: map[arm.ProvisioningState]bool{
107+
arm.ProvisioningStateSucceeded: false,
108+
arm.ProvisioningStateFailed: false,
109+
arm.ProvisioningStateCanceled: false,
110+
arm.ProvisioningStateAccepted: false,
111+
arm.ProvisioningStateDeleting: true,
112+
arm.ProvisioningStateProvisioning: false,
113+
arm.ProvisioningStateUpdating: false,
114+
},
115+
},
116+
{
117+
name: "Update node pool",
118+
resourceID: nodePoolResourceID,
119+
operationRequest: database.OperationRequestUpdate,
120+
directConflicts: map[arm.ProvisioningState]bool{
121+
arm.ProvisioningStateSucceeded: false,
122+
arm.ProvisioningStateFailed: false,
123+
arm.ProvisioningStateCanceled: false,
124+
arm.ProvisioningStateAccepted: true,
125+
arm.ProvisioningStateDeleting: true,
126+
arm.ProvisioningStateProvisioning: true,
127+
arm.ProvisioningStateUpdating: true,
128+
},
129+
parentConflicts: map[arm.ProvisioningState]bool{
130+
arm.ProvisioningStateSucceeded: false,
131+
arm.ProvisioningStateFailed: false,
132+
arm.ProvisioningStateCanceled: false,
133+
arm.ProvisioningStateAccepted: false,
134+
arm.ProvisioningStateDeleting: true,
135+
arm.ProvisioningStateProvisioning: false,
136+
arm.ProvisioningStateUpdating: false,
137+
},
138+
},
139+
}
140+
141+
for _, tt := range tests {
142+
var name string
143+
144+
resourceID, err := arm.ParseResourceID(tt.resourceID)
145+
if err != nil {
146+
t.Fatal(err)
147+
}
148+
149+
for directState, directConflict := range tt.directConflicts {
150+
name = fmt.Sprintf("%s (directState=%s)", tt.name, directState)
151+
t.Run(name, func(t *testing.T) {
152+
ctx := context.Background()
153+
154+
frontend := &Frontend{
155+
logger: slog.Default(),
156+
dbClient: database.NewCache(),
157+
}
158+
159+
doc := database.NewResourceDocument(resourceID)
160+
doc.ProvisioningState = directState
161+
162+
parentResourceID := resourceID.GetParent()
163+
if parentResourceID.ResourceType.Namespace == resourceID.ResourceType.Namespace {
164+
parentDoc := database.NewResourceDocument(parentResourceID)
165+
// Hold the provisioning state to something benign.
166+
parentDoc.ProvisioningState = arm.ProvisioningStateSucceeded
167+
_ = frontend.dbClient.CreateResourceDoc(ctx, parentDoc)
168+
}
169+
170+
cloudError := frontend.CheckForProvisioningStateConflict(ctx, tt.operationRequest, doc)
171+
172+
if cloudError == nil {
173+
if directConflict {
174+
t.Errorf("Expected %d %s but got no error", http.StatusConflict, http.StatusText(http.StatusConflict))
175+
}
176+
} else {
177+
if !directConflict || cloudError.StatusCode != http.StatusConflict {
178+
t.Errorf("Got unexpected error: %d %s", cloudError.StatusCode, http.StatusText(cloudError.StatusCode))
179+
}
180+
}
181+
})
182+
}
183+
184+
for parentState, parentConflict := range tt.parentConflicts {
185+
name = fmt.Sprintf("%s (parentState=%s)", tt.name, parentState)
186+
t.Run(name, func(t *testing.T) {
187+
ctx := context.Background()
188+
189+
frontend := &Frontend{
190+
logger: slog.Default(),
191+
dbClient: database.NewCache(),
192+
}
193+
194+
doc := database.NewResourceDocument(resourceID)
195+
// Hold the provisioning state to something benign.
196+
doc.ProvisioningState = arm.ProvisioningStateSucceeded
197+
198+
parentResourceID := resourceID.GetParent()
199+
if parentResourceID.ResourceType.Namespace == resourceID.ResourceType.Namespace {
200+
parentDoc := database.NewResourceDocument(parentResourceID)
201+
parentDoc.ProvisioningState = parentState
202+
_ = frontend.dbClient.CreateResourceDoc(ctx, parentDoc)
203+
} else {
204+
t.Fatalf("Parent resource type namespace (%s) differs from child namespace (%s)",
205+
parentResourceID.ResourceType.Namespace,
206+
resourceID.ResourceType.Namespace)
207+
}
208+
209+
cloudError := frontend.CheckForProvisioningStateConflict(ctx, tt.operationRequest, doc)
210+
211+
if cloudError == nil {
212+
if parentConflict {
213+
t.Errorf("Expected %d %s but got no error", http.StatusConflict, http.StatusText(http.StatusConflict))
214+
}
215+
} else {
216+
if !parentConflict || cloudError.StatusCode != http.StatusConflict {
217+
t.Errorf("Got unexpected error: %d %s", cloudError.StatusCode, http.StatusText(cloudError.StatusCode))
218+
}
219+
}
220+
})
221+
}
222+
}
223+
}

0 commit comments

Comments
 (0)