CyberCoveLLC/threshold-plugin-minio
1
0 Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

feat(storage): Improve error messages

Browse Source 
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.
This commit is contained in:
Hugo committed 2024-10-01 15:13:57 +01:00
1 parent 759ac083ac
commit 7d2654e707
2 files changed
+252 -27
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

plugin/service/storage/plugin.go
+28 -27
View File
@@ -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
}
plugin/service/storage/plugin_test.go
+224
View File
@@ -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(),
))
})
}