Merge pull request #2465 from versity/sis/delete-marker-last-modified

fix: report the delete time as the last modified time of delete markers in posix

backend/posix/posix.go

@@ -1690,7 +1690,7 @@ func (p *Posix) checkCopySourceDeleteMarker(bucket, object, versionId string) er
 func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc {
 	return func(path, versionIdMarker string, pastVersionIdMarker *bool, availableObjCount int, d fs.DirEntry) (*backend.ObjVersionFuncResult, error) {
 		var objects []s3response.ObjectVersion
-		var delMarkers []types.DeleteMarkerEntry
+		var delMarkers []s3response.DeleteMarkerEntry
 		// if the number of available objects is 0, return truncated response
 		if availableObjCount <= 0 {
 			return &backend.ObjVersionFuncResult{
@@ -1752,7 +1752,7 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc {
 			}
 
 			if isDel {
-				delMarkers = append(delMarkers, types.DeleteMarkerEntry{
+				delMarkers = append(delMarkers, s3response.DeleteMarkerEntry{
 					IsLatest:     getBoolPtr(true),
 					VersionId:    &versionId,
 					LastModified: backend.GetTimePtr(fi.ModTime()),
@@ -1819,7 +1819,7 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc {
 		// First find the null versionId object(if exists)
 		// before starting the object versions listing
 		var nullVersionIdObj *s3response.ObjectVersion
-		var nullObjDelMarker *types.DeleteMarkerEntry
+		var nullObjDelMarker *s3response.DeleteMarkerEntry
 		var nullPos nullVersionPos
 		nf, err := os.Stat(filepath.Join(versionPath, nullVersionId))
 		if err != nil && !errors.Is(err, fs.ErrNotExist) {
@@ -1838,7 +1838,7 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc {
 
 			// Check to see if the null versionId object is delete marker or not
 			if isDel {
-				nullObjDelMarker = &types.DeleteMarkerEntry{
+				nullObjDelMarker = &s3response.DeleteMarkerEntry{
 					VersionId:    backend.GetPtrFromString("null"),
 					LastModified: backend.GetTimePtr(nf.ModTime()),
 					Key:          &key,
@@ -1961,7 +1961,7 @@ func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc {
 			}
 
 			if isDel {
-				delMarkers = append(delMarkers, types.DeleteMarkerEntry{
+				delMarkers = append(delMarkers, s3response.DeleteMarkerEntry{
 					VersionId:    &versionId,
 					LastModified: backend.GetTimePtr(f.ModTime()),
 					Key:          &key,
@@ -5200,6 +5200,12 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 				}
 			}
 
+			// The delete marker is last modified when it's created, not
+			// when the version it hides was. Setting the time needs the
+			// entry to be owned by the gateway, otherwise it's left as is.
+			now := time.Now()
+			_ = os.Chtimes(objpath, now, now)
+
 			return &s3.DeleteObjectOutput{
 				DeleteMarker: getBoolPtr(true),
 				VersionId:    &versionId,

backend/s3proxy/s3.go

@@ -345,7 +345,7 @@ func (s *S3Proxy) ListObjectVersions(ctx context.Context, input *s3.ListObjectVe
 
 	return s3response.ListVersionsResult{
 		CommonPrefixes:      out.CommonPrefixes,
-		DeleteMarkers:       out.DeleteMarkers,
+		DeleteMarkers:       convertDeleteMarkers(out.DeleteMarkers),
 		Delimiter:           out.Delimiter,
 		EncodingType:        out.EncodingType,
 		IsTruncated:         out.IsTruncated,
@@ -1931,3 +1931,18 @@ func convertObjectVersions(versions []types.ObjectVersion) []s3response.ObjectVe
 
 	return result
 }
+
+func convertDeleteMarkers(markers []types.DeleteMarkerEntry) []s3response.DeleteMarkerEntry {
+	result := make([]s3response.DeleteMarkerEntry, 0, len(markers))
+	for _, m := range markers {
+		result = append(result, s3response.DeleteMarkerEntry{
+			IsLatest:     m.IsLatest,
+			Key:          m.Key,
+			LastModified: m.LastModified,
+			Owner:        m.Owner,
+			VersionId:    m.VersionId,
+		})
+	}
+
+	return result
+}

backend/walk.go

@@ -130,7 +130,7 @@ func Walk(ctx context.Context, fileSystem fs.FS, prefix, delimiter, marker strin
 type WalkVersioningResults struct {
 	CommonPrefixes      []types.CommonPrefix
 	ObjectVersions      []s3response.ObjectVersion
-	DelMarkers          []types.DeleteMarkerEntry
+	DelMarkers          []s3response.DeleteMarkerEntry
 	Truncated           bool
 	NextMarker          string
 	NextVersionIdMarker string
@@ -138,7 +138,7 @@ type WalkVersioningResults struct {
 
 type ObjVersionFuncResult struct {
 	ObjectVersions      []s3response.ObjectVersion
-	DelMarkers          []types.DeleteMarkerEntry
+	DelMarkers          []s3response.DeleteMarkerEntry
 	NextVersionIdMarker string
 	Truncated           bool
 }
@@ -150,7 +150,7 @@ type GetVersionsFunc func(path, versionIdMarker string, pastVersionIdMarker *boo
 func WalkVersions(ctx context.Context, fileSystem fs.FS, prefix, delimiter, keyMarker, versionIdMarker string, max int, getObj GetVersionsFunc, skipdirs []string) (WalkVersioningResults, error) {
 	cpmap := cpMap{}
 	var objects []s3response.ObjectVersion
-	var delMarkers []types.DeleteMarkerEntry
+	var delMarkers []s3response.DeleteMarkerEntry
 
 	var pastMarker bool
 	if keyMarker == "" {

s3response/s3response.go

@@ -511,7 +511,7 @@ type InitiateMultipartUploadResult struct {
 type ListVersionsResult struct {
 	XMLName             xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ ListVersionsResult" json:"-"`
 	CommonPrefixes      []types.CommonPrefix
-	DeleteMarkers       []types.DeleteMarkerEntry `xml:"DeleteMarker"`
+	DeleteMarkers       []DeleteMarkerEntry `xml:"DeleteMarker"`
 	Delimiter           *string
 	EncodingType        types.EncodingType
 	IsTruncated         *bool
@@ -555,6 +555,30 @@ func (o ObjectVersion) MarshalXML(e *xml.Encoder, start xml.StartElement) error
 	return e.EncodeElement(aux, start)
 }
 
+type DeleteMarkerEntry struct {
+	IsLatest     *bool
+	Key          *string
+	LastModified *time.Time
+	Owner        *types.Owner
+	VersionId    *string
+}
+
+func (d DeleteMarkerEntry) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
+	type Alias DeleteMarkerEntry
+	aux := &struct {
+		LastModified string `xml:"LastModified"`
+		*Alias
+	}{
+		Alias: (*Alias)(&d),
+	}
+
+	if d.LastModified != nil {
+		aux.LastModified = d.LastModified.UTC().Format(time.RFC3339)
+	}
+
+	return e.EncodeElement(aux, start)
+}
+
 type GetBucketVersioningOutput struct {
 	XMLName   xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ VersioningConfiguration" json:"-"`
 	MFADelete *types.MFADeleteStatus

tests/integration/group-tests.go

@@ -2058,6 +2058,7 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_DeleteObject_latest_version_null_version_order)
 	ts.Run(Versioning_DeleteObject_promoted_version_attributes)
 	ts.Run(Versioning_DeleteObject_promoted_version_last_modified)
+	ts.Run(Versioning_DeleteObject_delete_marker_last_modified)
 	ts.Run(Versioning_DeleteObject_non_existing_object)
 	ts.Run(Versioning_DeleteObject_implicit_dir)
 	ts.Run(Versioning_DeleteObject_trailing_slash_counterpart)
@@ -3661,6 +3662,7 @@ func GetIntTests() IntTests {
 		"Versioning_DeleteObject_latest_version_null_version_order":                        Versioning_DeleteObject_latest_version_null_version_order,
 		"Versioning_DeleteObject_promoted_version_attributes":                              Versioning_DeleteObject_promoted_version_attributes,
 		"Versioning_DeleteObject_promoted_version_last_modified":                           Versioning_DeleteObject_promoted_version_last_modified,
+		"Versioning_DeleteObject_delete_marker_last_modified":                              Versioning_DeleteObject_delete_marker_last_modified,
 		"Versioning_DeleteObject_dir_object_latest_version":                                Versioning_DeleteObject_dir_object_latest_version,
 		"Versioning_DeleteObject_non_existing_object":                                      Versioning_DeleteObject_non_existing_object,
 		"Versioning_DeleteObject_implicit_dir":                                             Versioning_DeleteObject_implicit_dir,

tests/integration/versioning.go

@@ -17,6 +17,7 @@ package integration
 import (
 	"context"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -25,6 +26,7 @@ import (
 	"sync"
 	"time"
 
+	awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http"
 	"github.com/aws/aws-sdk-go-v2/service/s3"
 	"github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/versity/versitygw/s3err"
@@ -2961,6 +2963,207 @@ func Versioning_DeleteObject_promoted_version_last_modified(s *S3Conf) error {
 	}, withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+// Versioning_DeleteObject_delete_marker_last_modified creates delete markers
+// over an object version, over another delete marker and, while versioning
+// is suspended, over the null version. A delete marker is last modified when
+// it's created, not when the version it hides was.
+func Versioning_DeleteObject_delete_marker_last_modified(s *S3Conf) error {
+	testName := "Versioning_DeleteObject_delete_marker_last_modified"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		cases := []struct {
+			obj string
+			// the object is put and deleted while versioning is suspended
+			null bool
+			// the last modified time of the object version
+			lastModified *time.Time
+			// the delete markers, from the oldest to the newest
+			delMarkers []string
+		}{
+			{obj: "my-obj"},
+			{obj: "my-dir/"},
+			{obj: "my-null-obj", null: true},
+			{obj: "my-null-dir/", null: true},
+		}
+
+		// delMarkerLastModified returns the last modified time of the delete
+		// marker of obj, which the listing and the Last-Modified header of a
+		// HeadObject request for it report alike
+		delMarkerLastModified := func(obj, versionId string) (time.Time, error) {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			res, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{
+				Bucket: &bucket,
+				Prefix: &obj,
+			})
+			cancel()
+			if err != nil {
+				return time.Time{}, err
+			}
+
+			var lastModified *time.Time
+			for _, dm := range res.DeleteMarkers {
+				if getString(dm.Key) == obj && getString(dm.VersionId) == versionId {
+					lastModified = dm.LastModified
+				}
+			}
+			if lastModified == nil {
+				return time.Time{}, fmt.Errorf("%v: expected the delete marker %v to be listed with LastModified, instead got %v",
+					obj, versionId, res.DeleteMarkers)
+			}
+
+			ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+			_, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{
+				Bucket:    &bucket,
+				Key:       &obj,
+				VersionId: &versionId,
+			})
+			cancel()
+			if err := checkSdkApiErr(err, "MethodNotAllowed"); err != nil {
+				return time.Time{}, err
+			}
+			var respErr *awshttp.ResponseError
+			if !errors.As(err, &respErr) {
+				return time.Time{}, fmt.Errorf("expected an HTTP response error, instead got %w", err)
+			}
+			header, err := http.ParseTime(respErr.Response.Header.Get("Last-Modified"))
+			if err != nil {
+				return time.Time{}, fmt.Errorf("%v: parse the delete marker %v Last-Modified header: %w",
+					obj, versionId, err)
+			}
+			if !header.Equal(*lastModified) {
+				return time.Time{}, fmt.Errorf("%v: expected the delete marker %v Last-Modified header to be %v, instead got %v",
+					obj, versionId, *lastModified, header)
+			}
+
+			return *lastModified, nil
+		}
+
+		// deleteObjects creates a delete marker for each of the cases the
+		// versioning status applies to
+		deleteObjects := func(null bool) error {
+			for i, c := range cases {
+				if c.null != null {
+					continue
+				}
+				ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+				out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+					Bucket: &bucket,
+					Key:    &c.obj,
+				})
+				cancel()
+				if err != nil {
+					return err
+				}
+				cases[i].delMarkers = append(cases[i].delMarkers, getString(out.VersionId))
+			}
+			return nil
+		}
+
+		// the null versions are put and deleted while versioning is suspended
+		for _, status := range []types.BucketVersioningStatus{
+			types.BucketVersioningStatusSuspended,
+			types.BucketVersioningStatusEnabled,
+		} {
+			err := putBucketVersioningStatus(s3client, bucket, status)
+			if err != nil {
+				return err
+			}
+			for i, c := range cases {
+				if c.null != (status == types.BucketVersioningStatusSuspended) {
+					continue
+				}
+				_, err := putObjectWithData(objDataLen(c.obj, 100), &s3.PutObjectInput{
+					Bucket: &bucket,
+					Key:    &c.obj,
+				}, s3client)
+				if err != nil {
+					return err
+				}
+
+				ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+				res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{
+					Bucket: &bucket,
+					Key:    &c.obj,
+				})
+				cancel()
+				if err != nil {
+					return err
+				}
+				if res.LastModified == nil {
+					return fmt.Errorf("%v: expected non nil LastModified", c.obj)
+				}
+				cases[i].lastModified = res.LastModified
+			}
+		}
+
+		// every delete marker is created in a later second than the
+		// version or the delete marker it hides
+		time.Sleep(time.Second)
+		if err := deleteObjects(false); err != nil {
+			return err
+		}
+		err := putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusSuspended)
+		if err != nil {
+			return err
+		}
+		if err := deleteObjects(true); err != nil {
+			return err
+		}
+		err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled)
+		if err != nil {
+			return err
+		}
+		time.Sleep(time.Second)
+		if err := deleteObjects(false); err != nil {
+			return err
+		}
+
+		for _, c := range cases {
+			prev := *c.lastModified
+			var lastModified []time.Time
+			for _, versionId := range c.delMarkers {
+				dmLastModified, err := delMarkerLastModified(c.obj, versionId)
+				if err != nil {
+					return err
+				}
+				if !dmLastModified.After(prev) {
+					return fmt.Errorf("%v: expected the delete marker %v LastModified %v to be after %v",
+						c.obj, versionId, dmLastModified, prev)
+				}
+				prev = dmLastModified
+				lastModified = append(lastModified, dmLastModified)
+			}
+
+			if c.null {
+				continue
+			}
+
+			// deleting the newest delete marker makes the one it hides
+			// the latest, with its own last modified time
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			_, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+				Bucket:    &bucket,
+				Key:       &c.obj,
+				VersionId: &c.delMarkers[1],
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+
+			dmLastModified, err := delMarkerLastModified(c.obj, c.delMarkers[0])
+			if err != nil {
+				return err
+			}
+			if !dmLastModified.Equal(lastModified[0]) {
+				return fmt.Errorf("%v: expected the promoted delete marker LastModified to be %v, instead got %v",
+					c.obj, lastModified[0], dmLastModified)
+			}
+		}
+
+		return nil
+	}, withVersioning(types.BucketVersioningStatusEnabled))
+}
+
 func Versioning_DeleteObject_non_existing_object(s *S3Conf) error {
 	testName := "Versioning_DeleteObject_non_existing_object"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {