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

@@ -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()
@@ -507,15 +507,9 @@ 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{
+	res, err := cl.PutObject(ctx, bucket.GetBucketName(), key, file, info.Size(), minio.PutObjectOptions{})
-		UserMetadata: map[string]string{
-			"x-amz-checksum-algorithm": "SHA256",
-			"x-amz-checksum-sha256":    checksum,
-		},
-		DisableMultipart: true,
-	})
 	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
@@ -569,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())
@@ -581,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),
@@ -594,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)
 	}
@@ -611,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++
 	}
@@ -817,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

@@ -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(),
+		))
+	})
+}