From 7d2654e707a53466c46ff147e3f7786ede64df4a Mon Sep 17 00:00:00 2001 From: Hugo <10965479+hugoghx@users.noreply.github.com> Date: Tue, 1 Oct 2024 15:04:51 +0100 Subject: [PATCH] feat(storage): Improve error messages Current `errorToStatus` is not displaying the underlying error if the status code is 400. Additionally, we are sending the error into this function twice: Once in the error message and again in the error itself. This commit changes the way `errorToStatus` works by sending a message indicating where the error occured and then the actual error as a different argument. Additionally, it changes `errorToStatus` to always print the input message and the error regardless of error type. --- plugin/service/storage/plugin.go | 55 +++---- plugin/service/storage/plugin_test.go | 224 ++++++++++++++++++++++++++ 2 files changed, 252 insertions(+), 27 deletions(-) diff --git a/plugin/service/storage/plugin.go b/plugin/service/storage/plugin.go index 358e321..1d1145d 100644 --- a/plugin/service/storage/plugin.go +++ b/plugin/service/storage/plugin.go @@ -359,13 +359,13 @@ func (sp *StoragePlugin) HeadObject(ctx context.Context, req *pb.HeadObjectReque Region: sa.Region, }) if err != nil { - return nil, errorToStatus(codes.Unknown, fmt.Sprintf("failed to create minio sdk client: %v", err), err, req).Err() + return nil, errorToStatus(codes.Unknown, "failed to create minio sdk client", err, req).Err() } objKey := path.Join(bucket.GetBucketPrefix(), req.GetKey()) oi, err := cl.StatObject(ctx, bucket.GetBucketName(), objKey, minio.StatObjectOptions{}) if err != nil { - return nil, errorToStatus(codes.Unknown, fmt.Sprintf("failed to stat object: %v", err), err, req).Err() + return nil, errorToStatus(codes.Unknown, "failed to stat object", err, req).Err() } return &pb.HeadObjectResponse{ @@ -404,13 +404,13 @@ func (sp *StoragePlugin) GetObject(req *pb.GetObjectRequest, objServer pb.Storag Region: sa.Region, }) if err != nil { - return errorToStatus(codes.Unknown, fmt.Sprintf("failed to create minio sdk client: %v", err), err, req).Err() + return errorToStatus(codes.Unknown, "failed to create minio sdk client", err, req).Err() } objKey := path.Join(bucket.GetBucketPrefix(), req.GetKey()) obj, err := cl.GetObject(objServer.Context(), bucket.GetBucketName(), objKey, minio.GetObjectOptions{}) if err != nil { - return errorToStatus(codes.Unknown, fmt.Sprintf("failed to get object: %v", err), err, req).Err() + return errorToStatus(codes.Unknown, "failed to get object", err, req).Err() } defer obj.Close() @@ -493,7 +493,7 @@ func (sp *StoragePlugin) PutObject(ctx context.Context, req *pb.PutObjectRequest Region: sa.Region, }) if err != nil { - return nil, errorToStatus(codes.Internal, fmt.Sprintf("failed to create minio sdk client: %v", err), err, req).Err() + return nil, errorToStatus(codes.Internal, "failed to create minio sdk client", err, req).Err() } hash := sha256.New() @@ -509,7 +509,7 @@ func (sp *StoragePlugin) PutObject(ctx context.Context, req *pb.PutObjectRequest key := path.Join(bucket.GetBucketPrefix(), req.GetKey()) res, err := cl.PutObject(ctx, bucket.GetBucketName(), key, file, info.Size(), minio.PutObjectOptions{}) if err != nil { - return nil, errorToStatus(codes.Internal, fmt.Sprintf("failed to put object into minio: %v", err), err, req).Err() + return nil, errorToStatus(codes.Internal, "failed to put object into minio", err, req).Err() } resChecksum := res.ChecksumSHA256 @@ -563,7 +563,7 @@ func (sp *StoragePlugin) DeleteObjects(ctx context.Context, req *pb.DeleteObject Region: sa.Region, }) if err != nil { - return nil, errorToStatus(codes.Unknown, fmt.Sprintf("failed to create minio sdk client: %v", err), err, req).Err() + return nil, errorToStatus(codes.Unknown, "failed to create minio sdk client", err, req).Err() } prefix := path.Join(bucket.GetBucketPrefix(), req.GetKeyPrefix()) @@ -575,7 +575,7 @@ func (sp *StoragePlugin) DeleteObjects(ctx context.Context, req *pb.DeleteObject if !req.Recursive { if err := cl.RemoveObject(ctx, bucket.GetBucketName(), prefix, minio.RemoveObjectOptions{}); err != nil { - return nil, errorToStatus(codes.Unknown, fmt.Sprintf("error deleting minio object: %v", err), err, req).Err() + return nil, errorToStatus(codes.Unknown, "error deleting minio object", err, req).Err() } return &pb.DeleteObjectsResponse{ ObjectsDeleted: uint32(1), @@ -588,7 +588,7 @@ func (sp *StoragePlugin) DeleteObjects(ctx context.Context, req *pb.DeleteObject Recursive: true, }) { if obj.Err != nil { - return nil, errorToStatus(codes.Unknown, fmt.Sprintf("error iterating minio bucket contents: %v", obj.Err), obj.Err, req).Err() + return nil, errorToStatus(codes.Unknown, "error iterating minio bucket contents", obj.Err, req).Err() } objects = append(objects, obj) } @@ -605,7 +605,7 @@ func (sp *StoragePlugin) DeleteObjects(ctx context.Context, req *pb.DeleteObject removed := 0 for res := range cl.RemoveObjectsWithResult(ctx, bucket.GetBucketName(), emitter, minio.RemoveObjectsOptions{}) { if res.Err != nil { - return nil, errorToStatus(codes.Unknown, fmt.Sprintf("error deleting minio object(s): %v", res.Err), res.Err, req).Err() + return nil, errorToStatus(codes.Unknown, "error deleting minio object(s)", res.Err, req).Err() } removed++ } @@ -811,49 +811,50 @@ func errorToPermission(err error) *pb.Permission { // If the permission error is not recognized, a gRPC status will be returned // with an code and error message passed in. func errorToStatus(code codes.Code, msg string, err error, req any) *status.Status { + if err == nil { + // If we see this status, it means this function was called with a nil + // err. This is odd and should probably be investigated. + return status.New(code, fmt.Sprintf("%s: no error from storage service provider", msg)) + } + errResponse := minio.ToErrorResponse(err) switch errResponse.Code { // https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList case "RequestTimeout": - return status.New(codes.DeadlineExceeded, "timeout exceeded when communicating with storage service provider") + return status.New(codes.DeadlineExceeded, fmt.Sprintf("%s: timeout exceeded when communicating with storage service provider", msg)) case "SlowDown": - return status.New(codes.Unavailable, "received a throttling response from storage service provider") + return status.New(codes.Unavailable, fmt.Sprintf("%s: received a throttling response from storage service provider", msg)) case "BadDigest": - return status.New(codes.Aborted, "storage checksum mismatch") + return status.New(codes.Aborted, fmt.Sprintf("%s: storage checksum mismatch", msg)) case "NoSuchBucket": - return status.New(codes.NotFound, "specified storage bucket does not exist") + return status.New(codes.NotFound, fmt.Sprintf("%s: specified storage bucket does not exist", msg)) case "NoSuchKey": - return status.New(codes.NotFound, "specified object does not exist") + return status.New(codes.NotFound, fmt.Sprintf("%s: specified object does not exist", msg)) } if errResponse.StatusCode == 400 { // catch-all for various different bad-request related errors - return status.New(codes.InvalidArgument, "received a bad request response from storage service provider") + return status.New(codes.InvalidArgument, fmt.Sprintf("%s: received a bad request response from storage service provider: %v", msg, err)) } // for all other errors, we can parse as permission state errors - - st := status.New(code, msg) - if err == nil { - return st - } permission := errorToPermission(err) state := &pb.StorageBucketCredentialState{State: &pb.Permissions{}} // we can use the request type to determine what kind of response we should return switch req.(type) { - case pb.HeadObjectRequest: + case pb.HeadObjectRequest, *pb.HeadObjectRequest: state.State.Read = permission - case pb.GetObjectRequest: + case pb.GetObjectRequest, *pb.GetObjectRequest: state.State.Read = permission - case pb.PutObjectRequest: + case pb.PutObjectRequest, *pb.PutObjectRequest: state.State.Write = permission - case pb.DeleteObjectsRequest: + case pb.DeleteObjectsRequest, *pb.DeleteObjectsRequest: state.State.Delete = permission } - st, err = st.WithDetails(state) + st, err := status.New(code, fmt.Sprintf("%s: %s", msg, err)).WithDetails(state) if err != nil { - st = status.New(code, msg) + st = status.New(code, fmt.Sprintf("%s: %s", msg, err)) } return st } diff --git a/plugin/service/storage/plugin_test.go b/plugin/service/storage/plugin_test.go index efbf697..61e2958 100644 --- a/plugin/service/storage/plugin_test.go +++ b/plugin/service/storage/plugin_test.go @@ -3314,3 +3314,227 @@ func TestEnsureServiceAccount(t *testing.T) { }) } } + +func TestErrorToStatus(t *testing.T) { + t.Run("nilErr", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", nil, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: no error from storage service provider") + require.Equal(t, codes.Internal, st.Code()) + }) + + t.Run("requestTimeout", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", minio.ErrorResponse{Code: "RequestTimeout"}, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: timeout exceeded when communicating with storage service provider") + require.Equal(t, codes.DeadlineExceeded, st.Code()) + }) + + t.Run("slowDown", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", minio.ErrorResponse{Code: "SlowDown"}, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: received a throttling response from storage service provider") + require.Equal(t, codes.Unavailable, st.Code()) + }) + + t.Run("badDigest", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", minio.ErrorResponse{Code: "BadDigest"}, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: storage checksum mismatch") + require.Equal(t, codes.Aborted, st.Code()) + }) + + t.Run("noSuchBucket", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", minio.ErrorResponse{Code: "NoSuchBucket"}, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: specified storage bucket does not exist") + require.Equal(t, codes.NotFound, st.Code()) + }) + + t.Run("noSuchKey", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", minio.ErrorResponse{Code: "NoSuchKey"}, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: specified object does not exist") + require.Equal(t, codes.NotFound, st.Code()) + }) + + t.Run("statusCode400", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", minio.ErrorResponse{StatusCode: 400, Message: "oops there was an error doing the thing"}, nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: received a bad request response from storage service provider: oops there was an error doing the thing") + require.Equal(t, codes.InvalidArgument, st.Code()) + }) + + t.Run("permissionsErr", func(t *testing.T) { + tests := []struct { + name string + code codes.Code + errMsg string + err error + reqType any + expStatusContains string + statusDetailsCheckFn func(t *testing.T, p *pb.Permissions) + }{ + { + name: "headObject", + code: codes.Internal, + errMsg: "head object failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: pb.HeadObjectRequest{}, + expStatusContains: "head object failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetRead(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "headObjectReqPtr", + code: codes.Internal, + errMsg: "head object failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: &pb.HeadObjectRequest{}, + expStatusContains: "head object failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetRead(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "getObject", + code: codes.Internal, + errMsg: "get object failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: pb.GetObjectRequest{}, + expStatusContains: "get object failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetRead(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "getObjectReqPtr", + code: codes.Internal, + errMsg: "get object failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: &pb.GetObjectRequest{}, + expStatusContains: "get object failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetRead(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "putObject", + code: codes.Internal, + errMsg: "put object failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: pb.PutObjectRequest{}, + expStatusContains: "put object failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetWrite(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "putObjectReqPtr", + code: codes.Internal, + errMsg: "put object failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: &pb.PutObjectRequest{}, + expStatusContains: "put object failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetWrite(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "deleteObjects", + code: codes.Internal, + errMsg: "delete objects failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: pb.DeleteObjectsRequest{}, + expStatusContains: "delete objects failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetDelete(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + { + name: "deleteObjectsReqPtr", + code: codes.Internal, + errMsg: "delete objects failed", + err: minio.ErrorResponse{StatusCode: 401, Code: "AccessDenied", Message: "object access denied"}, + reqType: &pb.DeleteObjectsRequest{}, + expStatusContains: "delete objects failed: object access denied", + statusDetailsCheckFn: func(t *testing.T, in *pb.Permissions) { + require.Empty(t, cmp.Diff( + in.GetDelete(), + &pb.Permission{State: pb.StateType_STATE_TYPE_ERROR, ErrorDetails: "object access denied"}, + protocmp.Transform(), + protocmp.IgnoreFields(in.GetRead(), "checked_at"), + )) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + st := errorToStatus(tt.code, tt.errMsg, tt.err, tt.reqType) + require.NotNil(t, st) + + require.Contains(t, st.String(), tt.expStatusContains) + require.Equal(t, codes.Internal, tt.code) + + require.Len(t, st.Details(), 1) + require.IsType(t, st.Details()[0], &pb.StorageBucketCredentialState{}) + + state := st.Details()[0].(*pb.StorageBucketCredentialState) + require.NotNil(t, state.GetState()) + if tt.statusDetailsCheckFn != nil { + tt.statusDetailsCheckFn(t, state.GetState()) + } + }) + } + }) + + t.Run("unknownErr", func(t *testing.T) { + st := errorToStatus(codes.Internal, "failed to do the thing", fmt.Errorf("oops the thing failed but it's just a normal go error"), nil) + require.NotNil(t, st) + require.Contains(t, st.String(), "failed to do the thing: oops the thing failed but it's just a normal go error") + require.Equal(t, codes.Internal, st.Code()) + require.Len(t, st.Details(), 1) + require.Empty(t, cmp.Diff( + &pb.StorageBucketCredentialState{State: &pb.Permissions{}}, + st.Details()[0], + protocmp.Transform(), + )) + }) +}