[#226] Fix status code in GET/HEAD delete marker #242

Merged
alexvanin merged 1 commit from dkirillov/frostfs-s3-gw:bugfix/226-get_head_delete_marker into master 2023-10-31 13:32:26 +00:00
4 changed files with 47 additions and 1 deletions

View file

@ -17,6 +17,7 @@ This document outlines major changes between releases.
- Use correct keys in `list-multipart-uploads` response (#185) - Use correct keys in `list-multipart-uploads` response (#185)
- Fix parsing `key-marker` for object list versions (#243) - Fix parsing `key-marker` for object list versions (#243)
- Fix marshaling errors in `DeleteObjects` method (#222) - Fix marshaling errors in `DeleteObjects` method (#222)
- Fix status code in GET/HEAD delete marker (#226)
### Added ### Added
- Add `trace_id` value into log record when tracing is enabled (#142) - Add `trace_id` value into log record when tracing is enabled (#142)

View file

@ -363,6 +363,27 @@ func TestDeleteMarkers(t *testing.T) {
require.Len(t, listOIDsFromMockedFrostFS(t, tc, bktName), 0, "shouldn't be any object in frostfs") require.Len(t, listOIDsFromMockedFrostFS(t, tc, bktName), 0, "shouldn't be any object in frostfs")
} }
func TestGetHeadDeleteMarker(t *testing.T) {
hc := prepareHandlerContext(t)
bktName, objName := "bucket-for-removal", "object-to-delete"
createTestBucket(hc, bktName)
putBucketVersioning(t, hc, bktName, true)
putObject(hc, bktName, objName)
deleteMarkerVersionID, _ := deleteObject(t, hc, bktName, objName, emptyVersion)
w := headObjectBase(hc, bktName, objName, deleteMarkerVersionID)
require.Equal(t, w.Code, http.StatusMethodNotAllowed)
require.Equal(t, w.Result().Header.Get(api.AmzDeleteMarker), "true")
w, r := prepareTestRequest(hc, bktName, objName, nil)
hc.Handler().GetObjectHandler(w, r)
assertStatus(hc.t, w, http.StatusNotFound)
require.Equal(t, w.Result().Header.Get(api.AmzDeleteMarker), "true")
}
func TestDeleteObjectFromListCache(t *testing.T) { func TestDeleteObjectFromListCache(t *testing.T) {
tc := prepareHandlerContext(t) tc := prepareHandlerContext(t)

View file

@ -3,6 +3,7 @@ package handler
import ( import (
"context" "context"
"errors" "errors"
"fmt"
"net/http" "net/http"
"strconv" "strconv"
"strings" "strings"
@ -28,6 +29,7 @@ func (h *handler) reqLogger(ctx context.Context) *zap.Logger {
} }
func (h *handler) logAndSendError(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) { func (h *handler) logAndSendError(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) {
err = handleDeleteMarker(w, err)
code := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err)) code := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err))
fields := []zap.Field{ fields := []zap.Field{
zap.Int("status", code), zap.Int("status", code),
@ -44,6 +46,16 @@ func (h *handler) logAndSendError(w http.ResponseWriter, logText string, reqInfo
h.log.Error(logs.RequestFailed, fields...) // consider using h.reqLogger (it requires accept context.Context or http.Request) h.log.Error(logs.RequestFailed, fields...) // consider using h.reqLogger (it requires accept context.Context or http.Request)
} }
func handleDeleteMarker(w http.ResponseWriter, err error) error {
var target layer.DeleteMarkerError
if !errors.As(err, &target) {
return err
}
w.Header().Set(api.AmzDeleteMarker, "true")
mbiryukova marked this conversation as resolved Outdated

Is it ok that AmzDeleteMarker header may appear in other responses (e.g. copy)?

Is it ok that `AmzDeleteMarker` header may appear in other responses (e.g. copy)?

I think yes. I suppose it doesn't break anything because the clients probably don't handle unexpected/unknown response headers

I think yes. I suppose it doesn't break anything because the clients probably don't handle unexpected/unknown response headers
return fmt.Errorf("%w: %s", s3errors.GetAPIError(target.ErrorCode), err)
}
func (h *handler) logAndSendErrorNoHeader(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) { func (h *handler) logAndSendErrorNoHeader(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) {
middleware.WriteErrorResponseNoHeader(w, reqInfo, transformToS3Error(err)) middleware.WriteErrorResponseNoHeader(w, reqInfo, transformToS3Error(err))
fields := []zap.Field{ fields := []zap.Field{

View file

@ -80,8 +80,16 @@ type (
Marker string Marker string
ContinuationToken string ContinuationToken string
} }
DeleteMarkerError struct {
ErrorCode apiErrors.ErrorCode
}
) )
alexvanin marked this conversation as resolved Outdated

What you think about having %v: object is delete marker formatting with inclusion of api error code. Does it make sense here or not?

What you think about having `%v: object is delete marker` formatting with inclusion of api error code. Does it make sense here or not?

I'm not sure that it makes a big sense. I don't know how having api error code can help us. In addition currently we log this error along with full s3 api error description (in logAndSendError function).

I'm not sure that it makes a big sense. I don't know how having api error code can help us. In addition currently we log this error along with full s3 api error description (in `logAndSendError` function).
func (e DeleteMarkerError) Error() string {
return "object is delete marker"
}
const ( const (
continuationToken = "<continuation-token>" continuationToken = "<continuation-token>"
) )
@ -389,7 +397,7 @@ func (n *layer) headLastVersionIfNotDeleted(ctx context.Context, bkt *data.Bucke
} }
if node.IsDeleteMarker() { if node.IsDeleteMarker() {
return nil, fmt.Errorf("%w: found version is delete marker", apiErrors.GetAPIError(apiErrors.ErrNoSuchKey)) return nil, DeleteMarkerError{ErrorCode: apiErrors.ErrNoSuchKey}
} }
meta, err := n.objectHead(ctx, bkt, node.OID) meta, err := n.objectHead(ctx, bkt, node.OID)
@ -445,6 +453,10 @@ func (n *layer) headVersion(ctx context.Context, bkt *data.BucketInfo, p *HeadOb
return extObjInfo, nil return extObjInfo, nil
} }
if foundVersion.IsDeleteMarker() {
return nil, DeleteMarkerError{ErrorCode: apiErrors.ErrMethodNotAllowed}
}
meta, err := n.objectHead(ctx, bkt, foundVersion.OID) meta, err := n.objectHead(ctx, bkt, foundVersion.OID)
if err != nil { if err != nil {
if client.IsErrObjectNotFound(err) { if client.IsErrObjectNotFound(err) {