Skip to content

Commit 716b1da

Browse files
sunnylovestiramisusunnylovestiramisu
committed
Separate user errors from internal errors
1 parent 41af246 commit 716b1da

File tree

3 files changed

+103
-28
lines changed

3 files changed

+103
-28
lines changed

‎pkg/gce-pd-csi-driver/controller.go

+28-28
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
207207

208208
volumeID, err := common.KeyToVolumeID(volKey, gceCS.CloudProvider.GetDefaultProject())
209209
if err != nil {
210-
return nil, status.Errorf(codes.Internal, "Failed to convert volume key to volume ID: %v", err)
210+
return nil, LoggedError("Failed to convert volume key to volume ID: ", err)
211211
}
212212
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
213213
return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, volumeID)
@@ -218,7 +218,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
218218
existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, gceCS.CloudProvider.GetDefaultProject(), volKey, gceAPIVersion)
219219
if err != nil {
220220
if !gce.IsGCEError(err, "notFound") {
221-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume unknown get disk error when validating: %v", err))
221+
return nil, LoggedError("CreateVolume unknown get disk error when validating: ", err)
222222
}
223223
}
224224
if err == nil {
@@ -233,7 +233,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
233233

234234
ready, err := isDiskReady(existingDisk)
235235
if err != nil {
236-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume disk %v had error checking ready status: %v", volKey, err))
236+
return nil, LoggedError("CreateVolume disk "+volKey.String()+" had error checking ready status: ", err)
237237
}
238238
if !ready {
239239
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume existing disk %v is not ready", volKey))
@@ -254,7 +254,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
254254
// Verify that snapshot exists
255255
sl, err := gceCS.getSnapshotByID(ctx, snapshotID)
256256
if err != nil {
257-
return nil, status.Errorf(codes.Internal, "CreateVolume failed to get snapshot %s: %v", snapshotID, err)
257+
return nil, LoggedError("CreateVolume failed to get snapshot "+snapshotID+": ", err)
258258
} else if len(sl.Entries) == 0 {
259259
return nil, status.Errorf(codes.NotFound, "CreateVolume source snapshot %s does not exist", snapshotID)
260260
}
@@ -274,7 +274,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
274274
if gce.IsGCEError(err, "notFound") {
275275
return nil, status.Errorf(codes.NotFound, "CreateVolume source volume %s does not exist", volumeContentSourceVolumeID)
276276
} else {
277-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume unknown get disk error when validating: %v", err))
277+
return nil, LoggedError("CreateVolume unknown get disk error when validating: ", err)
278278
}
279279
}
280280

@@ -312,7 +312,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
312312
// Verify the source disk is ready.
313313
ready, err := isDiskReady(diskFromSourceVolume)
314314
if err != nil {
315-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume disk from source volume %v had error checking ready status: %v", sourceVolKey, err))
315+
return nil, LoggedError("CreateVolume disk from source volume "+sourceVolKey.String()+" had error checking ready status: ", err)
316316
}
317317
if !ready {
318318
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume disk from source volume %v is not ready", sourceVolKey))
@@ -333,15 +333,15 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
333333
}
334334
disk, err = createSingleZoneDisk(ctx, gceCS.CloudProvider, name, zones, params, capacityRange, capBytes, snapshotID, volumeContentSourceVolumeID, multiWriter)
335335
if err != nil {
336-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume failed to create single zonal disk %#v: %v", name, err))
336+
return nil, LoggedError("CreateVolume failed to create single zonal disk "+name+": ", err)
337337
}
338338
case replicationTypeRegionalPD:
339339
if len(zones) != 2 {
340340
return nil, status.Errorf(codes.Internal, fmt.Sprintf("CreateVolume failed to get a 2 zones for creating regional disk, instead got: %v", zones))
341341
}
342342
disk, err = createRegionalDisk(ctx, gceCS.CloudProvider, name, zones, params, capacityRange, capBytes, snapshotID, volumeContentSourceVolumeID, multiWriter)
343343
if err != nil {
344-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume failed to create regional disk %#v: %v", name, err))
344+
return nil, LoggedError("CreateVolume failed to create regional disk "+name+": ", err)
345345
}
346346
default:
347347
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("CreateVolume replication type '%s' is not supported", params.ReplicationType))
@@ -381,7 +381,7 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
381381
klog.Warningf("DeleteVolume treating volume as deleted because cannot find volume %v: %w", volumeID, err)
382382
return &csi.DeleteVolumeResponse{}, nil
383383
}
384-
return nil, status.Errorf(codes.Internal, "DeleteVolume error repairing underspecified volume key: %v", err)
384+
return nil, LoggedError("DeleteVolume error repairing underspecified volume key: ", err)
385385
}
386386

387387
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
@@ -391,7 +391,7 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
391391

392392
err = gceCS.CloudProvider.DeleteDisk(ctx, project, volKey)
393393
if err != nil {
394-
return nil, status.Error(codes.Internal, fmt.Sprintf("unknown Delete disk error: %v", err))
394+
return nil, LoggedError("unknown Delete disk error: ", err)
395395
}
396396

397397
klog.V(4).Infof("DeleteVolume succeeded for disk %v", volKey)
@@ -470,7 +470,7 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
470470
if gce.IsGCENotFoundError(err) {
471471
return nil, status.Errorf(codes.NotFound, "ControllerPublishVolume could not find volume with ID %v: %v", volumeID, err)
472472
}
473-
return nil, status.Errorf(codes.Internal, "ControllerPublishVolume error repairing underspecified volume key: %v", err)
473+
return nil, LoggedError("ControllerPublishVolume error repairing underspecified volume key: ", err)
474474
}
475475

476476
// Acquires the lock for the volume on that node only, because we need to support the ability
@@ -592,7 +592,7 @@ func (gceCS *GCEControllerServer) executeControllerUnpublishVolume(ctx context.C
592592
if gce.IsGCENotFoundError(err) {
593593
return nil, status.Errorf(codes.NotFound, "ControllerUnpublishVolume could not find volume with ID %v: %v", volumeID, err)
594594
}
595-
return nil, status.Errorf(codes.Internal, "ControllerUnpublishVolume error repairing underspecified volume key: %v", err)
595+
return nil, LoggedError("ControllerUnpublishVolume error repairing underspecified volume key: ", err)
596596
}
597597

598598
// Acquires the lock for the volume on that node only, because we need to support the ability
@@ -632,7 +632,7 @@ func (gceCS *GCEControllerServer) executeControllerUnpublishVolume(ctx context.C
632632

633633
err = gceCS.CloudProvider.DetachDisk(ctx, project, deviceName, instanceZone, instanceName)
634634
if err != nil {
635-
return nil, status.Error(codes.Internal, fmt.Sprintf("unknown detach error: %v", err))
635+
return nil, LoggedError("unknown detach error: ", err)
636636
}
637637

638638
klog.V(4).Infof("ControllerUnpublishVolume succeeded for disk %v from node %v", volKey, nodeID)
@@ -657,7 +657,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
657657
if gce.IsGCENotFoundError(err) {
658658
return nil, status.Errorf(codes.NotFound, "ValidateVolumeCapabilities could not find volume with ID %v: %v", volumeID, err)
659659
}
660-
return nil, status.Errorf(codes.Internal, "ValidateVolumeCapabilities error repairing underspecified volume key: %v", err)
660+
return nil, LoggedError("ValidateVolumeCapabilities error repairing underspecified volume key: ", err)
661661
}
662662

663663
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
@@ -670,7 +670,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
670670
if gce.IsGCENotFoundError(err) {
671671
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find disk %v: %v", volKey.Name, err))
672672
}
673-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown get disk error: %v", err))
673+
return nil, LoggedError("Unknown get disk error: ", err)
674674
}
675675

676676
// Check Volume Context is Empty
@@ -733,7 +733,7 @@ func (gceCS *GCEControllerServer) ListVolumes(ctx context.Context, req *csi.List
733733
if gce.IsGCEInvalidError(err) {
734734
return nil, status.Error(codes.Aborted, fmt.Sprintf("ListVolumes error with invalid request: %v", err))
735735
}
736-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown list disk error: %v", err))
736+
return nil, LoggedError("Unknown list disk error: ", err)
737737
}
738738
gceCS.disks = diskList
739739
gceCS.seen = map[string]int{}
@@ -816,7 +816,7 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C
816816
if gce.IsGCENotFoundError(err) {
817817
return nil, status.Error(codes.NotFound, fmt.Sprintf("CreateSnapshot could not find disk %v: %v", volKey.String(), err))
818818
}
819-
return nil, status.Error(codes.Internal, fmt.Sprintf("CreateSnapshot unknown get disk error: %v", err))
819+
return nil, LoggedError("CreateSnapshot unknown get disk error: ", err)
820820
}
821821

822822
snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(req.GetParameters(), gceCS.Driver.name)
@@ -863,7 +863,7 @@ func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project
863863
if gce.IsGCEError(err, "notFound") {
864864
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volKey.String(), err))
865865
}
866-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown create snapshot error: %v", err))
866+
return nil, LoggedError("Unknown create snapshot error: ", err)
867867
}
868868
}
869869

@@ -902,15 +902,15 @@ func (gceCS *GCEControllerServer) createImage(ctx context.Context, project strin
902902
image, err = gceCS.CloudProvider.GetImage(ctx, project, imageName)
903903
if err != nil {
904904
if !gce.IsGCEError(err, "notFound") {
905-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown get image error: %v", err))
905+
return nil, LoggedError("Unknown get image error: ", err)
906906
}
907907
// create a new image
908908
image, err = gceCS.CloudProvider.CreateImage(ctx, project, volKey, imageName, snapshotParams)
909909
if err != nil {
910910
if gce.IsGCEError(err, "notFound") {
911911
return nil, status.Error(codes.NotFound, fmt.Sprintf("Could not find volume with ID %v: %v", volKey.String(), err))
912912
}
913-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown create image error: %v", err))
913+
return nil, LoggedError("Unknown create image error: ", err)
914914
}
915915
}
916916

@@ -1040,12 +1040,12 @@ func (gceCS *GCEControllerServer) DeleteSnapshot(ctx context.Context, req *csi.D
10401040
case common.DiskSnapshotType:
10411041
err = gceCS.CloudProvider.DeleteSnapshot(ctx, project, key)
10421042
if err != nil {
1043-
return nil, status.Error(codes.Internal, fmt.Sprintf("unknown Delete snapshot error: %v", err))
1043+
return nil, LoggedError("unknown Delete snapshot error: ", err)
10441044
}
10451045
case common.DiskImageType:
10461046
err = gceCS.CloudProvider.DeleteImage(ctx, project, key)
10471047
if err != nil {
1048-
return nil, status.Error(codes.Internal, fmt.Sprintf("unknown Delete image error: %v", err))
1048+
return nil, LoggedError("unknown Delete image error: ", err)
10491049
}
10501050
default:
10511051
return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("unknown snapshot type %s", snapshotType))
@@ -1077,7 +1077,7 @@ func (gceCS *GCEControllerServer) ListSnapshots(ctx context.Context, req *csi.Li
10771077
if gce.IsGCEInvalidError(err) {
10781078
return nil, status.Error(codes.Aborted, fmt.Sprintf("ListSnapshots error with invalid request: %v", err))
10791079
}
1080-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown list snapshots error: %v", err))
1080+
return nil, LoggedError("Unknown list snapshots error: ", err)
10811081
}
10821082
gceCS.snapshots = snapshotList
10831083
gceCS.snapshotTokens = map[string]int{}
@@ -1126,12 +1126,12 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
11261126
if gce.IsGCENotFoundError(err) {
11271127
return nil, status.Errorf(codes.NotFound, "ControllerExpandVolume could not find volume with ID %v: %v", volumeID, err)
11281128
}
1129-
return nil, status.Errorf(codes.Internal, "ControllerExpandVolume error repairing underspecified volume key: %v", err)
1129+
return nil, LoggedError("ControllerExpandVolume error repairing underspecified volume key: ", err)
11301130
}
11311131

11321132
resizedGb, err := gceCS.CloudProvider.ResizeDisk(ctx, project, volKey, reqBytes)
11331133
if err != nil {
1134-
return nil, status.Error(codes.Internal, fmt.Sprintf("ControllerExpandVolume failed to resize disk: %v", err))
1134+
return nil, LoggedError("ControllerExpandVolume failed to resize disk: ", err)
11351135
}
11361136

11371137
klog.V(4).Infof("ControllerExpandVolume succeeded for disk %v to size %v", volKey, resizedGb)
@@ -1154,15 +1154,15 @@ func (gceCS *GCEControllerServer) getSnapshots(ctx context.Context, req *csi.Lis
11541154
if gce.IsGCEError(err, "invalid") {
11551155
return nil, status.Error(codes.Aborted, fmt.Sprintf("Invalid error: %v", err))
11561156
}
1157-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown list snapshot error: %v", err))
1157+
return nil, LoggedError("Unknown list snapshot error: ", err)
11581158
}
11591159

11601160
images, _, err = gceCS.CloudProvider.ListImages(ctx, filter)
11611161
if err != nil {
11621162
if gce.IsGCEError(err, "invalid") {
11631163
return nil, status.Error(codes.Aborted, fmt.Sprintf("Invalid error: %v", err))
11641164
}
1165-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown list image error: %v", err))
1165+
return nil, LoggedError("Unknown list image error: ", err)
11661166
}
11671167

11681168
entries := []*csi.ListSnapshotsResponse_Entry{}
@@ -1203,7 +1203,7 @@ func (gceCS *GCEControllerServer) getSnapshotByID(ctx context.Context, snapshotI
12031203
// return empty list if no snapshot is found
12041204
return &csi.ListSnapshotsResponse{}, nil
12051205
}
1206-
return nil, status.Error(codes.Internal, fmt.Sprintf("Unknown list snapshot error: %v", err))
1206+
return nil, LoggedError("Unknown list snapshot error: ", err)
12071207
}
12081208
e, err := generateDiskSnapshotEntry(snapshot)
12091209
if err != nil {

‎pkg/gce-pd-csi-driver/utils.go

+37
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,15 @@ package gceGCEDriver
1919
import (
2020
"errors"
2121
"fmt"
22+
"net/http"
2223

2324
"context"
2425

2526
csi "github.com/container-storage-interface/spec/lib/go/csi"
27+
"google.golang.org/api/googleapi"
2628
"google.golang.org/grpc"
29+
"google.golang.org/grpc/codes"
30+
"google.golang.org/grpc/status"
2731
"k8s.io/klog/v2"
2832
)
2933

@@ -213,3 +217,36 @@ func containsZone(zones []string, zone string) bool {
213217

214218
return false
215219
}
220+
221+
// CodeForError returns a pointer to the grpc error code that maps to the http
222+
// error code for the passed in user googleapi error. Returns nil if the
223+
// given error is not a googleapi error caused by the user. The following
224+
// http error codes are considered user errors:
225+
// (1) http 400 Bad Request, returns grpc InvalidArgument,
226+
// (2) http 403 Forbidden, returns grpc PermissionDenied,
227+
// (3) http 404 Not Found, returns grpc NotFound
228+
// (4) http 429 Too Many Requests, returns grpc ResourceExhausted
229+
func CodeForError(err error) *codes.Code {
230+
internalErrorCode := codes.Internal
231+
// Upwrap the error
232+
var apiErr *googleapi.Error
233+
if !errors.As(err, &apiErr) {
234+
return &internalErrorCode
235+
}
236+
237+
userErrors := map[int]codes.Code{
238+
http.StatusForbidden: codes.PermissionDenied,
239+
http.StatusBadRequest: codes.InvalidArgument,
240+
http.StatusTooManyRequests: codes.ResourceExhausted,
241+
http.StatusNotFound: codes.NotFound,
242+
}
243+
if code, ok := userErrors[apiErr.Code]; ok {
244+
return &code
245+
}
246+
return &internalErrorCode
247+
}
248+
249+
func LoggedError(msg string, err error) error {
250+
klog.Errorf(msg+"%w", err)
251+
return status.Errorf(*CodeForError(err), msg+"%w", err)
252+
}

‎pkg/gce-pd-csi-driver/utils_test.go

+38
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,13 @@ limitations under the License.
1818
package gceGCEDriver
1919

2020
import (
21+
"errors"
22+
"net/http"
2123
"testing"
2224

2325
csi "github.com/container-storage-interface/spec/lib/go/csi"
26+
"google.golang.org/api/googleapi"
27+
"google.golang.org/grpc/codes"
2428
)
2529

2630
var (
@@ -291,3 +295,37 @@ func TestGetReadOnlyFromCapabilities(t *testing.T) {
291295
}
292296
}
293297
}
298+
299+
func TestCodeForError(t *testing.T) {
300+
internalErrorCode := codes.Internal
301+
userErrorCode := codes.InvalidArgument
302+
testCases := []struct {
303+
name string
304+
inputErr error
305+
expCode *codes.Code
306+
}{
307+
{
308+
name: "Not googleapi.Error",
309+
inputErr: errors.New("I am not a googleapi.Error"),
310+
expCode: &internalErrorCode,
311+
},
312+
{
313+
name: "User error",
314+
inputErr: &googleapi.Error{Code: http.StatusBadRequest, Message: "User error with bad request"},
315+
expCode: &userErrorCode,
316+
},
317+
{
318+
name: "googleapi.Error but not a user error",
319+
inputErr: &googleapi.Error{Code: http.StatusInternalServerError, Message: "Internal error"},
320+
expCode: &internalErrorCode,
321+
},
322+
}
323+
324+
for _, tc := range testCases {
325+
t.Logf("Running test: %v", tc.name)
326+
actualCode := *CodeForError(tc.inputErr)
327+
if *tc.expCode != actualCode {
328+
t.Fatalf("Expected error code '%v' but got '%v'", tc.expCode, actualCode)
329+
}
330+
}
331+
}

0 commit comments

Comments
 (0)