[#589] Add LimitExceeded error #593

Open
r.loginov wants to merge 1 commit from r.loginov/frostfs-s3-gw:feature/589-return-quota-limit-reached_status into master
4 changed files with 80 additions and 17 deletions

View file

@ -289,6 +289,9 @@ const (
//CORS configuration errors.
ErrCORSUnsupportedMethod
ErrCORSWildcardExposeHeaders
// Limits errors.
ErrLimitExceeded
)
// error code to Error structure, these fields carry respective
@ -1763,6 +1766,14 @@ var errorCodes = errorCodeMap{
Description: "Content-Range header is mandatory for this type of request",
HTTPStatusCode: http.StatusBadRequest,
},
// The Conflict status is used because this error was made based on the LimitExceeded error
// from aws iam error https://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateUser.html#API_CreateUser_Errors.
ErrLimitExceeded: {
ErrCode: ErrLimitExceeded,
Code: "LimitExceeded",
r.loginov marked this conversation as resolved Outdated

It seems to me that the StatusConflict is not very suitable for quota limit errors. Maybe we should use another response code, such as TooManyRequests or Forbidden?

It seems to me that the StatusConflict is not very suitable for quota limit errors. Maybe we should use another response code, such as TooManyRequests or Forbidden?

That's a good point. I agree that at first glance, the Conflict status is not quite appropriate
I used it for two reasons:

  1. The same status is used in the aws error on the basis of which this error was made.
  2. Based on RFC 9110, it seems that this status is appropriate. If you look at the phrase

This code is used in situations where the user might be able to resolve the conflict and resubmit the request. The server SHOULD generate content that includes enough information for a user to recognize the source of the conflict.

This is exactly the situation we are in. Upon receiving a message that the quota has been exceeded, the user can remove this quota himself by deleting related resources. And resend the request, which will work successfully.

Using the Too Many Requests status doesn't seem quite right, because based on RFC 6585, this status is used in communications when a user has sent too many requests in a given period of time. And in our situation, we are talking specifically about the resource quota (the amount of space occupied, the number of containers, the number of objects).

Using the Forbidden status can be considered for use in this situation. However, I am a little confused that if you follow RFC 9110, then this status is used in situations where a user with certain credentials does not have access to this resource. On the one hand, this is true, but only create/put operations are not available to the user. The user may well request and delete the resource. Moreover, it turns out that the user can delete the data in the resource himself, thereby removing his quota. It doesn't seem to be the behavior that the current status implies.

However, I don't have a clear opinion on whether to use Conflict or Forbidden. Let's also wait for the opinions of other reviewers.

That's a good point. I agree that at first glance, the `Conflict` status is not quite appropriate I used it for two reasons: 1. The same status is used in the [aws error](https://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateUser.html#API_CreateUser_Errors) on the basis of which this error was made. 2. Based on [RFC 9110](https://datatracker.ietf.org/doc/html/rfc9110#name-409-conflict), it seems that this status is appropriate. If you look at the phrase > This code is used in situations where the user might be able to resolve the conflict and resubmit the request. The server SHOULD generate content that includes enough information for a user to recognize the source of the conflict. This is exactly the situation we are in. Upon receiving a message that the quota has been exceeded, the user can remove this quota himself by deleting related resources. And resend the request, which will work successfully. Using the Too Many Requests status doesn't seem quite right, because based on [RFC 6585](https://datatracker.ietf.org/doc/html/rfc6585#section-4), this status is used in communications when a user has sent too many requests in a given period of time. And in our situation, we are talking specifically about the resource quota (the amount of space occupied, the number of containers, the number of objects). Using the Forbidden status can be considered for use in this situation. However, I am a little confused that if you follow [RFC 9110](https://datatracker.ietf.org/doc/html/rfc9110#name-403-forbidden), then this status is used in situations where a user with certain credentials does not have access to this resource. On the one hand, this is true, but only create/put operations are not available to the user. The user may well request and delete the resource. Moreover, it turns out that the user can delete the data in the resource himself, thereby removing his quota. It doesn't seem to be the behavior that the current status implies. However, I don't have a clear opinion on whether to use Conflict or Forbidden. Let's also wait for the opinions of other reviewers.

We have agreed that we will use the 409 Conflict

We have agreed that we will use the 409 Conflict
Description: "You have reached the quota limit.",
HTTPStatusCode: http.StatusConflict,
},
// Add your error structure here.
}
@ -1826,6 +1837,10 @@ func TransformToS3Error(err error) error {
return GetAPIError(ErrBucketAlreadyExists)
}
if errors.Is(err, frostfs.ErrQuotaLimitReached) {
return GetAPIError(ErrLimitExceeded)
}
return GetAPIError(ErrInternalError)
}

View file

@ -240,6 +240,9 @@ var (
// ErrGlobalDomainIsAlreadyTaken is returned from FrostFS in case of global domain is already taken.
ErrGlobalDomainIsAlreadyTaken = errors.New("global domain is already taken")
// ErrQuotaLimitReached is returned from FrostFS in case of quota exceeded.
ErrQuotaLimitReached = errors.New("quota limit reached")
)
// FrostFS represents virtual connection to FrostFS network.

View file

@ -477,6 +477,9 @@ func handleObjectError(msg string, err error) error {
}
if reason, ok := frosterr.IsErrObjectAccessDenied(err); ok {
if strings.Contains(reason, "limit reached") {
r.loginov marked this conversation as resolved Outdated

Can we ensure if reason always stars with capital letter?
Probably we can check this in lowercased reason or find just limit reached

Can we ensure if reason always stars with capital letter? Probably we can check this in lowercased reason or find just `limit reached`
return fmt.Errorf("%s: %w: %s", msg, frostfs.ErrQuotaLimitReached, reason)
}
return fmt.Errorf("%s: %w: %s", msg, frostfs.ErrAccessDenied, reason)
}

View file

@ -8,31 +8,49 @@ import (
"time"
"git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/frostfs"
frosterr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
"github.com/stretchr/testify/require"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
func TestErrorChecking(t *testing.T) {
reason := "some reason"
err := new(apistatus.ObjectAccessDenied)
err.WriteReason(reason)
func TestHandleObjectError(t *testing.T) {
msg := "some msg"
var wrappedError error
t.Run("nil error", func(t *testing.T) {
err := handleObjectError(msg, nil)
require.Nil(t, err)
})
if fetchedReason, ok := frosterr.IsErrObjectAccessDenied(err); ok {
wrappedError = fmt.Errorf("%w: %s", frostfs.ErrAccessDenied, fetchedReason)
}
t.Run("simple access denied", func(t *testing.T) {
reason := "some reason"
inputErr := new(apistatus.ObjectAccessDenied)
inputErr.WriteReason(reason)
require.ErrorIs(t, wrappedError, frostfs.ErrAccessDenied)
require.Contains(t, wrappedError.Error(), reason)
}
err := handleObjectError(msg, inputErr)
require.ErrorIs(t, err, frostfs.ErrAccessDenied)
require.Contains(t, err.Error(), reason)
require.Contains(t, err.Error(), msg)
})
t.Run("access denied - quota reached", func(t *testing.T) {
reason := "Quota limit reached"
inputErr := new(apistatus.ObjectAccessDenied)
inputErr.WriteReason(reason)
err := handleObjectError(msg, inputErr)
require.ErrorIs(t, err, frostfs.ErrQuotaLimitReached)
require.Contains(t, err.Error(), reason)
require.Contains(t, err.Error(), msg)
})
func TestErrorTimeoutChecking(t *testing.T) {
t.Run("simple timeout", func(t *testing.T) {
require.True(t, frosterr.IsTimeoutError(errors.New("timeout")))
inputErr := errors.New("timeout")
err := handleObjectError(msg, inputErr)
require.ErrorIs(t, err, frostfs.ErrGatewayTimeout)
require.Contains(t, err.Error(), inputErr.Error())
require.Contains(t, err.Error(), msg)
})
t.Run("deadline exceeded", func(t *testing.T) {
@ -40,11 +58,35 @@ func TestErrorTimeoutChecking(t *testing.T) {
defer cancel()
<-ctx.Done()
require.True(t, frosterr.IsTimeoutError(ctx.Err()))
err := handleObjectError(msg, ctx.Err())
require.ErrorIs(t, err, frostfs.ErrGatewayTimeout)
require.Contains(t, err.Error(), ctx.Err().Error())
require.Contains(t, err.Error(), msg)
})
t.Run("grpc deadline exceeded", func(t *testing.T) {
err := fmt.Errorf("wrap grpc error: %w", status.Error(codes.DeadlineExceeded, "error"))
require.True(t, frosterr.IsTimeoutError(err))
inputErr := fmt.Errorf("wrap grpc error: %w", status.Error(codes.DeadlineExceeded, "error"))
err := handleObjectError(msg, inputErr)
require.ErrorIs(t, err, frostfs.ErrGatewayTimeout)
require.Contains(t, err.Error(), err.Error())
require.Contains(t, err.Error(), msg)
})
t.Run("global domain already", func(t *testing.T) {
inputErr := errors.New("global domain is already taken")
err := handleObjectError(msg, inputErr)
require.ErrorIs(t, err, frostfs.ErrGlobalDomainIsAlreadyTaken)
require.Contains(t, err.Error(), err.Error())
require.Contains(t, err.Error(), msg)
})
t.Run("unknown error", func(t *testing.T) {
inputErr := errors.New("unknown error")
err := handleObjectError(msg, inputErr)
require.ErrorIs(t, err, inputErr)
require.Contains(t, err.Error(), msg)
})
}