[#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)
|
- 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)
|
||||||
|
|
|
@ -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)
|
||||||
|
|
||||||
|
|
|
@ -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
|
|||||||
|
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{
|
||||||
|
|
|
@ -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
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 (
|
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) {
|
||||||
|
|
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