From ce04b0836a0609ba6278558d893f8ee1a456682c Mon Sep 17 00:00:00 2001 From: Roman Loginov Date: Wed, 25 Dec 2024 15:49:18 +0300 Subject: [PATCH] [#187] Fix handling ErrAccessDenied and ErrGatewayTimeout If an Access Denied error occurs, we should return 403 Forbidden instead of 400 Bad Request. Gateway Timeout error we also need to process and return the 504 Gateway Timeout code. Signed-off-by: Roman Loginov --- cmd/http-gw/app.go | 21 ++++--- internal/handler/download.go | 5 +- internal/handler/handler.go | 11 ++-- internal/handler/reader.go | 3 +- internal/handler/upload.go | 16 +++--- internal/handler/utils.go | 45 +++++++++++---- internal/service/frostfs/frostfs_test.go | 72 ++++++++++++++++++++++++ response/utils.go | 41 -------------- 8 files changed, 132 insertions(+), 82 deletions(-) create mode 100644 internal/service/frostfs/frostfs_test.go delete mode 100644 response/utils.go diff --git a/cmd/http-gw/app.go b/cmd/http-gw/app.go index 23a752a..9d90ccd 100644 --- a/cmd/http-gw/app.go +++ b/cmd/http-gw/app.go @@ -25,7 +25,6 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/templates" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/metrics" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/resolver" - "git.frostfs.info/TrueCloudLab/frostfs-http-gw/response" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/tokens" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/tree" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/utils" @@ -634,28 +633,28 @@ func (a *app) stopServices() { } } -func (a *app) configureRouter(handler *handler.Handler) { +func (a *app) configureRouter(h *handler.Handler) { r := router.New() r.RedirectTrailingSlash = true r.NotFound = func(r *fasthttp.RequestCtx) { - response.Error(r, "Not found", fasthttp.StatusNotFound) + handler.ResponseError(r, "Not found", fasthttp.StatusNotFound) } r.MethodNotAllowed = func(r *fasthttp.RequestCtx) { - response.Error(r, "Method Not Allowed", fasthttp.StatusMethodNotAllowed) + handler.ResponseError(r, "Method Not Allowed", fasthttp.StatusMethodNotAllowed) } - r.POST("/upload/{cid}", a.addMiddlewares(handler.Upload)) + r.POST("/upload/{cid}", a.addMiddlewares(h.Upload)) r.OPTIONS("/upload/{cid}", a.addPreflight()) a.log.Info(logs.AddedPathUploadCid) - r.GET("/get/{cid}/{oid:*}", a.addMiddlewares(handler.DownloadByAddressOrBucketName)) - r.HEAD("/get/{cid}/{oid:*}", a.addMiddlewares(handler.HeadByAddressOrBucketName)) + r.GET("/get/{cid}/{oid:*}", a.addMiddlewares(h.DownloadByAddressOrBucketName)) + r.HEAD("/get/{cid}/{oid:*}", a.addMiddlewares(h.HeadByAddressOrBucketName)) r.OPTIONS("/get/{cid}/{oid:*}", a.addPreflight()) a.log.Info(logs.AddedPathGetCidOid) - r.GET("/get_by_attribute/{cid}/{attr_key}/{attr_val:*}", a.addMiddlewares(handler.DownloadByAttribute)) - r.HEAD("/get_by_attribute/{cid}/{attr_key}/{attr_val:*}", a.addMiddlewares(handler.HeadByAttribute)) + r.GET("/get_by_attribute/{cid}/{attr_key}/{attr_val:*}", a.addMiddlewares(h.DownloadByAttribute)) + r.HEAD("/get_by_attribute/{cid}/{attr_key}/{attr_val:*}", a.addMiddlewares(h.HeadByAttribute)) r.OPTIONS("/get_by_attribute/{cid}/{attr_key}/{attr_val:*}", a.addPreflight()) a.log.Info(logs.AddedPathGetByAttributeCidAttrKeyAttrVal) - r.GET("/zip/{cid}/{prefix:*}", a.addMiddlewares(handler.DownloadZipped)) + r.GET("/zip/{cid}/{prefix:*}", a.addMiddlewares(h.DownloadZipped)) r.OPTIONS("/zip/{cid}/{prefix:*}", a.addPreflight()) a.log.Info(logs.AddedPathZipCidPrefix) @@ -798,7 +797,7 @@ func (a *app) tokenizer(h fasthttp.RequestHandler) fasthttp.RequestHandler { log := utils.GetReqLogOrDefault(reqCtx, a.log) log.Error(logs.CouldNotFetchAndStoreBearerToken, zap.Error(err)) - response.Error(req, "could not fetch and store bearer token: "+err.Error(), fasthttp.StatusBadRequest) + handler.ResponseError(req, "could not fetch and store bearer token: "+err.Error(), fasthttp.StatusBadRequest) return } utils.SetContextToRequest(appCtx, req) diff --git a/internal/handler/download.go b/internal/handler/download.go index de27fa3..8766f0c 100644 --- a/internal/handler/download.go +++ b/internal/handler/download.go @@ -13,7 +13,6 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/layer" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/logs" - "git.frostfs.info/TrueCloudLab/frostfs-http-gw/response" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/utils" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer" cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" @@ -120,7 +119,7 @@ func (h *Handler) DownloadZipped(c *fasthttp.RequestCtx) { prefix, err := url.QueryUnescape(prefix) if err != nil { log.Error(logs.FailedToUnescapeQuery, zap.String("cid", scid), zap.String("prefix", prefix), zap.Error(err)) - response.Error(c, "could not unescape prefix: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, "could not unescape prefix: "+err.Error(), fasthttp.StatusBadRequest) return } @@ -135,7 +134,7 @@ func (h *Handler) DownloadZipped(c *fasthttp.RequestCtx) { resSearch, err := h.search(ctx, bktInfo.CID, object.AttributeFilePath, prefix, object.MatchCommonPrefix) if err != nil { log.Error(logs.CouldNotSearchForObjects, zap.Error(err)) - response.Error(c, "could not search for objects: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, "could not search for objects: "+err.Error(), fasthttp.StatusBadRequest) return } diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 3805c2d..470f494 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -13,7 +13,6 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/handler/middleware" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/layer" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/logs" - "git.frostfs.info/TrueCloudLab/frostfs-http-gw/response" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/utils" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer" apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status" @@ -210,7 +209,7 @@ func (h *Handler) byS3Path(ctx context.Context, req request, cnrID cid.ID, path } if foundOID.IsDeleteMarker { log.Error(logs.ObjectWasDeleted) - response.Error(c, "object deleted", fasthttp.StatusNotFound) + ResponseError(c, "object deleted", fasthttp.StatusNotFound) return } @@ -230,14 +229,14 @@ func (h *Handler) byAttribute(c *fasthttp.RequestCtx, handler func(context.Conte key, err := url.QueryUnescape(key) if err != nil { log.Error(logs.FailedToUnescapeQuery, zap.String("cid", cidParam), zap.String("attr_key", key), zap.Error(err)) - response.Error(c, "could not unescape attr_key: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, "could not unescape attr_key: "+err.Error(), fasthttp.StatusBadRequest) return } val, err = url.QueryUnescape(val) if err != nil { log.Error(logs.FailedToUnescapeQuery, zap.String("cid", cidParam), zap.String("attr_val", val), zap.Error(err)) - response.Error(c, "could not unescape attr_val: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, "could not unescape attr_val: "+err.Error(), fasthttp.StatusBadRequest) return } @@ -252,11 +251,11 @@ func (h *Handler) byAttribute(c *fasthttp.RequestCtx, handler func(context.Conte objID, err := h.findObjectByAttribute(ctx, log, bktInfo.CID, key, val) if err != nil { if errors.Is(err, io.EOF) { - response.Error(c, err.Error(), fasthttp.StatusNotFound) + ResponseError(c, err.Error(), fasthttp.StatusNotFound) return } - response.Error(c, err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, err.Error(), fasthttp.StatusBadRequest) return } diff --git a/internal/handler/reader.go b/internal/handler/reader.go index 50121c9..833e3d6 100644 --- a/internal/handler/reader.go +++ b/internal/handler/reader.go @@ -10,7 +10,6 @@ import ( "time" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/logs" - "git.frostfs.info/TrueCloudLab/frostfs-http-gw/response" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/utils" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object" oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" @@ -131,7 +130,7 @@ func (h *Handler) receiveFile(ctx context.Context, req request, objAddress oid.A }) if err != nil && err != io.EOF { req.log.Error(logs.CouldNotDetectContentTypeFromPayload, zap.Error(err)) - response.Error(req.RequestCtx, "could not detect Content-Type from payload: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(req.RequestCtx, "could not detect Content-Type from payload: "+err.Error(), fasthttp.StatusBadRequest) return } diff --git a/internal/handler/upload.go b/internal/handler/upload.go index 867025d..0d0053e 100644 --- a/internal/handler/upload.go +++ b/internal/handler/upload.go @@ -9,7 +9,6 @@ import ( "time" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/logs" - "git.frostfs.info/TrueCloudLab/frostfs-http-gw/response" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/tokens" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/utils" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer" @@ -81,14 +80,14 @@ func (h *Handler) Upload(c *fasthttp.RequestCtx) { boundary := string(c.Request.Header.MultipartFormBoundary()) if file, err = fetchMultipartFile(log, bodyStream, boundary); err != nil { log.Error(logs.CouldNotReceiveMultipartForm, zap.Error(err)) - response.Error(c, "could not receive multipart/form: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, "could not receive multipart/form: "+err.Error(), fasthttp.StatusBadRequest) return } filtered, err := filterHeaders(log, &c.Request.Header) if err != nil { log.Error(logs.CouldNotProcessHeaders, zap.Error(err)) - response.Error(c, err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, err.Error(), fasthttp.StatusBadRequest) return } @@ -103,7 +102,7 @@ func (h *Handler) Upload(c *fasthttp.RequestCtx) { if err = utils.PrepareExpirationHeader(c, h.frostfs, filtered, now); err != nil { log.Error(logs.CouldNotPrepareExpirationHeader, zap.Error(err)) - response.Error(c, "could not prepare expiration header: "+err.Error(), fasthttp.StatusBadRequest) + ResponseError(c, "could not prepare expiration header: "+err.Error(), fasthttp.StatusBadRequest) return } @@ -157,7 +156,7 @@ func (h *Handler) Upload(c *fasthttp.RequestCtx) { // Try to return the response, otherwise, if something went wrong, throw an error. if err = newPutResponse(addr).encode(c); err != nil { log.Error(logs.CouldNotEncodeResponse, zap.Error(err)) - response.Error(c, "could not encode response", fasthttp.StatusBadRequest) + ResponseError(c, "could not encode response", fasthttp.StatusBadRequest) return } @@ -179,11 +178,10 @@ func (h *Handler) Upload(c *fasthttp.RequestCtx) { } func (h *Handler) handlePutFrostFSErr(r *fasthttp.RequestCtx, err error, log *zap.Logger) { - statusCode, msg, additionalFields := response.FormErrorResponse("could not store file in frostfs", err) - logFields := append([]zap.Field{zap.Error(err)}, additionalFields...) + statusCode, msg := formErrorResponse("could not store file in frostfs", err) - log.Error(logs.CouldNotStoreFileInFrostfs, logFields...) - response.Error(r, msg, statusCode) + log.Error(logs.CouldNotStoreFileInFrostfs, zap.Error(err)) + ResponseError(r, msg, statusCode) } func (h *Handler) fetchBearerToken(ctx context.Context) *bearer.Token { diff --git a/internal/handler/utils.go b/internal/handler/utils.go index d09ed23..8ebcecd 100644 --- a/internal/handler/utils.go +++ b/internal/handler/utils.go @@ -2,11 +2,12 @@ package handler import ( "context" + "errors" + "fmt" "strings" "time" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/logs" - "git.frostfs.info/TrueCloudLab/frostfs-http-gw/response" "git.frostfs.info/TrueCloudLab/frostfs-http-gw/tokens" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/bearer" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client" @@ -27,11 +28,10 @@ func (r *request) handleFrostFSErr(err error, start time.Time) { zap.Stringer("elapsed", time.Since(start)), zap.Error(err), } - statusCode, msg, additionalFields := response.FormErrorResponse("could not receive object", err) - logFields = append(logFields, additionalFields...) + statusCode, msg := formErrorResponse("could not receive object", err) r.log.Error(logs.CouldNotReceiveObject, logFields...) - response.Error(r.RequestCtx, msg, statusCode) + ResponseError(r.RequestCtx, msg, statusCode) } func bearerToken(ctx context.Context) *bearer.Token { @@ -76,13 +76,10 @@ func isValidValue(s string) bool { } func logAndSendBucketError(c *fasthttp.RequestCtx, log *zap.Logger, err error) { - log.Error(logs.CouldntGetBucket, zap.Error(err)) + statusCode, msg := formErrorResponse("could not get bucket", err) - if client.IsErrContainerNotFound(err) { - response.Error(c, "Not Found", fasthttp.StatusNotFound) - return - } - response.Error(c, "could not get bucket: "+err.Error(), fasthttp.StatusBadRequest) + log.Error(logs.CouldntGetBucket, zap.Error(err)) + ResponseError(c, msg, statusCode) } func newAddress(cnr cid.ID, obj oid.ID) oid.Address { @@ -91,3 +88,31 @@ func newAddress(cnr cid.ID, obj oid.ID) oid.Address { addr.SetObject(obj) return addr } + +func ResponseError(r *fasthttp.RequestCtx, msg string, code int) { + r.Error(msg+"\n", code) +} + +func formErrorResponse(message string, err error) (int, string) { + var ( + errMsg string + statusCode int + ) + + switch { + case errors.Is(err, ErrAccessDenied): + statusCode = fasthttp.StatusForbidden + errMsg = err.Error() + case errors.Is(err, ErrGatewayTimeout): + statusCode = fasthttp.StatusBadGateway + errMsg = err.Error() + case client.IsErrObjectNotFound(err) || client.IsErrContainerNotFound(err): + statusCode = fasthttp.StatusNotFound + errMsg = "Not Found" + default: + statusCode = fasthttp.StatusBadRequest + errMsg = err.Error() + } + + return statusCode, fmt.Sprintf("%s: %s", message, errMsg) +} diff --git a/internal/service/frostfs/frostfs_test.go b/internal/service/frostfs/frostfs_test.go new file mode 100644 index 0000000..78c8f38 --- /dev/null +++ b/internal/service/frostfs/frostfs_test.go @@ -0,0 +1,72 @@ +package frostfs + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "git.frostfs.info/TrueCloudLab/frostfs-http-gw/internal/handler" + 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 TestHandleObjectError(t *testing.T) { + msg := "some msg" + + t.Run("nil error", func(t *testing.T) { + err := handleObjectError(msg, nil) + require.Nil(t, err) + }) + + t.Run("simple access denied", func(t *testing.T) { + reason := "some reason" + inputErr := new(apistatus.ObjectAccessDenied) + inputErr.WriteReason(reason) + + err := handleObjectError(msg, inputErr) + require.ErrorIs(t, err, handler.ErrAccessDenied) + require.Contains(t, err.Error(), reason) + require.Contains(t, err.Error(), msg) + }) + + t.Run("simple timeout", func(t *testing.T) { + inputErr := errors.New("timeout") + + err := handleObjectError(msg, inputErr) + require.ErrorIs(t, err, handler.ErrGatewayTimeout) + require.Contains(t, err.Error(), inputErr.Error()) + require.Contains(t, err.Error(), msg) + }) + + t.Run("deadline exceeded", func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + <-ctx.Done() + + err := handleObjectError(msg, ctx.Err()) + require.ErrorIs(t, err, handler.ErrGatewayTimeout) + require.Contains(t, err.Error(), ctx.Err().Error()) + require.Contains(t, err.Error(), msg) + }) + + t.Run("grpc deadline exceeded", func(t *testing.T) { + inputErr := fmt.Errorf("wrap grpc error: %w", status.Error(codes.DeadlineExceeded, "error")) + + err := handleObjectError(msg, inputErr) + require.ErrorIs(t, err, handler.ErrGatewayTimeout) + 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) + }) +} diff --git a/response/utils.go b/response/utils.go deleted file mode 100644 index f233943..0000000 --- a/response/utils.go +++ /dev/null @@ -1,41 +0,0 @@ -package response - -import ( - "errors" - "fmt" - - "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client" - sdkstatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status" - "github.com/valyala/fasthttp" - "go.uber.org/zap" -) - -func Error(r *fasthttp.RequestCtx, msg string, code int) { - r.Error(msg+"\n", code) -} - -func FormErrorResponse(message string, err error) (int, string, []zap.Field) { - var ( - msg string - statusCode int - logFields []zap.Field - ) - - st := new(sdkstatus.ObjectAccessDenied) - - switch { - case errors.As(err, &st): - statusCode = fasthttp.StatusForbidden - reason := st.Reason() - msg = fmt.Sprintf("%s: %v: %s", message, err, reason) - logFields = append(logFields, zap.String("error_detail", reason)) - case client.IsErrObjectNotFound(err) || client.IsErrContainerNotFound(err): - statusCode = fasthttp.StatusNotFound - msg = "Not Found" - default: - statusCode = fasthttp.StatusBadRequest - msg = fmt.Sprintf("%s: %v", message, err) - } - - return statusCode, msg, logFields -}