[#226] Fix status code in GET/HEAD delete marker #242
4 changed files with 47 additions and 1 deletions
|
@ -17,6 +17,7 @@ This document outlines major changes between releases.
|
|||
- Use correct keys in `list-multipart-uploads` response (#185)
|
||||
- Fix parsing `key-marker` for object list versions (#243)
|
||||
- Fix marshaling errors in `DeleteObjects` method (#222)
|
||||
- Fix status code in GET/HEAD delete marker (#226)
|
||||
|
||||
### Added
|
||||
- Add `trace_id` value into log record when tracing is enabled (#142)
|
||||
|
|
|
@ -363,6 +363,27 @@ func TestDeleteMarkers(t *testing.T) {
|
|||
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) {
|
||||
tc := prepareHandlerContext(t)
|
||||
|
||||
|
|
|
@ -3,6 +3,7 @@ package handler
|
|||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"strconv"
|
||||
"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) {
|
||||
err = handleDeleteMarker(w, err)
|
||||
code := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err))
|
||||
fields := []zap.Field{
|
||||
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)
|
||||
}
|
||||
|
||||
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
|
||||
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) {
|
||||
middleware.WriteErrorResponseNoHeader(w, reqInfo, transformToS3Error(err))
|
||||
fields := []zap.Field{
|
||||
|
|
|
@ -80,8 +80,16 @@ type (
|
|||
Marker string
|
||||
ContinuationToken string
|
||||
}
|
||||
|
||||
DeleteMarkerError struct {
|
||||
ErrorCode apiErrors.ErrorCode
|
||||
}
|
||||
)
|
||||
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
What you think about having What you think about having `%v: object is delete marker` formatting with inclusion of api error code. Does it make sense here or not?
dkirillov
commented
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 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 (
|
||||
continuationToken = "<continuation-token>"
|
||||
)
|
||||
|
@ -389,7 +397,7 @@ func (n *layer) headLastVersionIfNotDeleted(ctx context.Context, bkt *data.Bucke
|
|||
}
|
||||
|
||||
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)
|
||||
|
@ -445,6 +453,10 @@ func (n *layer) headVersion(ctx context.Context, bkt *data.BucketInfo, p *HeadOb
|
|||
return extObjInfo, nil
|
||||
}
|
||||
|
||||
if foundVersion.IsDeleteMarker() {
|
||||
return nil, DeleteMarkerError{ErrorCode: apiErrors.ErrMethodNotAllowed}
|
||||
}
|
||||
|
||||
meta, err := n.objectHead(ctx, bkt, foundVersion.OID)
|
||||
if err != nil {
|
||||
if client.IsErrObjectNotFound(err) {
|
||||
|
|
Loading…
Reference in a new issue
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