[#589] Add LimitExceeded error #593
4 changed files with 80 additions and 17 deletions
|
@ -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
|
||||
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)
|
||||
}
|
||||
|
||||
|
|
|
@ -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.
|
||||
|
|
|
@ -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
dkirillov
commented
Can we ensure if reason always stars with capital letter? 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)
|
||||
}
|
||||
|
||||
|
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue
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 appropriateI used it for two reasons:
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.
We have agreed that we will use the 409 Conflict