From e1d72907e464ba8cec0af0d3be3cf1fafbd7d34b Mon Sep 17 00:00:00 2001 From: niksis02 Date: Mon, 21 Sep 2026 20:34:47 +0400 Subject: [PATCH] fix: handle the null version id correctly in posix versioning Fixes #2165 `GetObject` marked an object carrying no `versionIdKey` attribute as the null version and then overwrote that with the empty attribute value, and `HeadObject` never marked it at all, so neither reported a version id for an object put into a versioning-suspended bucket. Both now resolve it through a shared `liveObjVersionId`, which reports `null` only when the bucket has versioning configured so that a bucket that was never versioned keeps reporting no version id at all. `HeadObject` also resolved an explicit `?versionId=null` into the versioning directory twice, once for the missing attribute and once for the mismatch that followed from it, and answered `404` for a version that `GetObject` served. It now treats the missing attribute as the null version the way `GetObject` does. `latestObjVersion` took the last version directory entry as the newest, but `os.ReadDir` sorts by name and `null` sorts after every `ulid`. Deleting the current version of an object whose history also held a null version therefore restored `null` rather than the version created immediately before the deleted one. The `ulid` entries keep their name order, which is creation order, and the null version is placed by its modification time, the same rule `fileToObjVersions` already applies. --- backend/posix/posix.go | 99 +++++++--- tests/integration/group-tests.go | 10 + tests/integration/versioning.go | 329 +++++++++++++++++++++++++++++++ 3 files changed, 414 insertions(+), 24 deletions(-) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 165a67f9..f1b689fd 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -1226,6 +1226,30 @@ func (p *Posix) isBucketVersioningSuspended(s types.BucketVersioningStatus) bool return s == types.BucketVersioningStatusSuspended } +// liveObjVersionId returns the version id to report for the current version +// of bucket/object. An object with no versionId attribute is the null +// version, but only a bucket with versioning configured has versions at all: +// in a bucket that was never versioned the object has no version id. +func (p *Posix) liveObjVersionId(ctx context.Context, bucket, object string) (string, error) { + vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) + if err == nil { + return string(vId), nil + } + if !errors.Is(err, meta.ErrNoSuchKey) { + return "", fmt.Errorf("get obj versionId: %w", err) + } + + status, err := p.getBucketVersioningStatus(ctx, bucket) + if err != nil { + return "", fmt.Errorf("get bucket versioning status: %w", err) + } + if status == "" { + return "", nil + } + + return nullVersionId, nil +} + // Generates the object version path in the versioning directory func (p *Posix) genObjVersionPath(bucket, key string) string { return filepath.Join(p.versioningDir, bucket, genObjVersionKey(key)) @@ -5155,9 +5179,9 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( }, nil } - srcObjVersion, err := latestObjVersion(ents).Info() + srcObjVersion, err := latestObjVersion(ents) if err != nil { - return nil, fmt.Errorf("get file info: %w", err) + return nil, fmt.Errorf("get latest obj version: %w", err) } srcVersionId := srcObjVersion.Name() sf, err := os.Open(filepath.Join(versionPath, srcVersionId)) @@ -5326,11 +5350,45 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return &s3.DeleteObjectOutput{}, nil } -// latestObjVersion returns the entry of the version, among the version -// directory entries, that becomes the latest one when the latest version -// of the object is deleted -func latestObjVersion(ents []fs.DirEntry) fs.DirEntry { - return ents[len(ents)-1] +// latestObjVersion returns the version, among the version directory entries, +// that becomes the latest one when the latest version of the object is +// deleted. The entries are named after the version id and os.ReadDir sorts +// them by name, which puts the ulid version ids in creation order. The null +// version id doesn't sort with them, so it's placed by its modification time, +// which is the time the version was created. +func latestObjVersion(ents []fs.DirEntry) (fs.FileInfo, error) { + var latest, nullEnt fs.DirEntry + for _, ent := range ents { + if ent.Name() == nullVersionId { + nullEnt = ent + continue + } + latest = ent + } + + switch { + case latest == nil && nullEnt == nil: + return nil, fs.ErrNotExist + case latest == nil: + return nullEnt.Info() + case nullEnt == nil: + return latest.Info() + } + + latestInfo, err := latest.Info() + if err != nil { + return nil, err + } + nullInfo, err := nullEnt.Info() + if err != nil { + return nil, err + } + + if nullInfo.ModTime().After(latestInfo.ModTime()) { + return nullInfo, nil + } + + return latestInfo, nil } // deleteDirObjectLatestVersion removes the latest version of the directory @@ -5371,12 +5429,11 @@ func (p *Posix) deleteDirObjectLatestVersion(bucket, key string) error { return nil } - srcVersion := latestObjVersion(ents) - srcVersionId := srcVersion.Name() - srcInfo, err := srcVersion.Info() + srcInfo, err := latestObjVersion(ents) if err != nil { - return fmt.Errorf("get file info: %w", err) + return fmt.Errorf("get latest obj version: %w", err) } + srcVersionId := srcInfo.Name() // replace the attributes in place, so that the directory keeps its etag for _, attr := range dirObjectAttrs { @@ -5623,14 +5680,10 @@ func (p *Posix) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.Ge // If versioning is configured get the object versionId if p.versioningEnabled() && versionId == "" { - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, meta.ErrNoSuchKey) { - versionId = nullVersionId - } else if err != nil { + versionId, err = p.liveObjVersionId(ctx, bucket, object) + if err != nil { return nil, err } - - versionId = string(vId) } if fid.IsDir() { @@ -5879,8 +5932,8 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3. return nil, fmt.Errorf("get obj versionId: %w", err) } if errors.Is(err, meta.ErrNoSuchKey) { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) + // an object without a versionId attribute is the null version + vId = []byte(nullVersionId) } if string(vId) != versionId { @@ -5944,12 +5997,10 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3. } if p.versioningEnabled() && versionId == "" { - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return nil, fmt.Errorf("get object versionId: %v", err) + versionId, err = p.liveObjVersionId(ctx, bucket, object) + if err != nil { + return nil, err } - - versionId = string(vId) } objMeta := p.loadObjectMetaProperties(nil, bucket, object, &fi) diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 6f5d8c79..f7311936 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1972,6 +1972,8 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_HeadObject_success) ts.Run(Versioning_HeadObject_dir_object_versions) ts.Run(Versioning_HeadObject_without_versionId) + ts.Run(Versioning_HeadObject_null_version_without_versionId) + ts.Run(Versioning_HeadObject_null_versionId_obj) ts.Run(Versioning_HeadObject_delete_marker) // GetObject action ts.Run(Versioning_GetObject_invalid_versionId) @@ -1980,6 +1982,8 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_GetObject_delete_marker_without_versionId) ts.Run(Versioning_GetObject_delete_marker) ts.Run(Versioning_GetObject_null_versionId_obj) + ts.Run(Versioning_GetObject_null_version_without_versionId) + ts.Run(Versioning_unversioned_bucket_omits_versionId) // object tagging actions ts.Run(Versioning_PutObjectTagging_invalid_versionId) ts.Run(Versioning_PutObjectTagging_non_existing_object_version) @@ -1998,6 +2002,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_DeleteObject_invalid_versionId) ts.Run(Versioning_DeleteObject_delete_object_version) ts.Run(Versioning_DeleteObject_dir_object_latest_version) + ts.Run(Versioning_DeleteObject_latest_version_with_null_version) ts.Run(Versioning_DeleteObject_non_existing_object) ts.Run(Versioning_DeleteObject_implicit_dir) ts.Run(Versioning_DeleteObject_trailing_slash_counterpart) @@ -3509,12 +3514,16 @@ func GetIntTests() IntTests { "Versioning_HeadObject_success": Versioning_HeadObject_success, "Versioning_HeadObject_dir_object_versions": Versioning_HeadObject_dir_object_versions, "Versioning_HeadObject_without_versionId": Versioning_HeadObject_without_versionId, + "Versioning_HeadObject_null_versionId_obj": Versioning_HeadObject_null_versionId_obj, + "Versioning_HeadObject_null_version_without_versionId": Versioning_HeadObject_null_version_without_versionId, "Versioning_HeadObject_delete_marker": Versioning_HeadObject_delete_marker, "Versioning_GetObject_invalid_versionId": Versioning_GetObject_invalid_versionId, "Versioning_GetObject_non_existing_object_version": Versioning_GetObject_non_existing_object_version, "Versioning_GetObject_success": Versioning_GetObject_success, "Versioning_GetObject_delete_marker_without_versionId": Versioning_GetObject_delete_marker_without_versionId, "Versioning_GetObject_delete_marker": Versioning_GetObject_delete_marker, + "Versioning_unversioned_bucket_omits_versionId": Versioning_unversioned_bucket_omits_versionId, + "Versioning_GetObject_null_version_without_versionId": Versioning_GetObject_null_version_without_versionId, "Versioning_GetObject_null_versionId_obj": Versioning_GetObject_null_versionId_obj, "Versioning_PutObjectTagging_invalid_versionId": Versioning_PutObjectTagging_invalid_versionId, "Versioning_PutObjectTagging_non_existing_object_version": Versioning_PutObjectTagging_non_existing_object_version, @@ -3530,6 +3539,7 @@ func GetIntTests() IntTests { "Versioning_GetObjectAttributes_delete_marker": Versioning_GetObjectAttributes_delete_marker, "Versioning_DeleteObject_invalid_versionId": Versioning_DeleteObject_invalid_versionId, "Versioning_DeleteObject_delete_object_version": Versioning_DeleteObject_delete_object_version, + "Versioning_DeleteObject_latest_version_with_null_version": Versioning_DeleteObject_latest_version_with_null_version, "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, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 8e71ecd5..3e32d2a7 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -1269,6 +1269,105 @@ func Versioning_HeadObject_without_versionId(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +// Versioning_HeadObject_null_version_without_versionId heads an object put +// into a versioning-suspended bucket, without naming a version id. +func Versioning_HeadObject_null_versionId_obj(s *S3Conf) error { + testName := "Versioning_HeadObject_null_versionId_obj" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + keys, dataLen := []string{"my-obj", "my-dir/"}, int64(321) + // the objects are put before versioning is enabled + etags := make(map[string]string, len(keys)) + err := forEachKey(keys, func(obj string) error { + out, err := putObjectWithData(objDataLen(obj, dataLen), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + etags[obj] = getString(out.res.ETag) + return nil + }) + if err != nil { + return err + } + + err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled) + if err != nil { + return err + } + + return forEachKey(keys, func(obj string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &nullVersionId, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != nullVersionId { + return fmt.Errorf("expected the versionId to be %v, instead got %v", + nullVersionId, getString(res.VersionId)) + } + if res.ContentLength == nil || *res.ContentLength != objDataLen(obj, dataLen) { + return fmt.Errorf("expected the Content-Length to be %v, instead got %v", + objDataLen(obj, dataLen), res.ContentLength) + } + if getString(res.ETag) != etags[obj] { + return fmt.Errorf("expected the ETag to be %v, instead got %v", + etags[obj], getString(res.ETag)) + } + + return nil + }) + }) +} + +func Versioning_HeadObject_null_version_without_versionId(s *S3Conf) error { + testName := "Versioning_HeadObject_null_version_without_versionId" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + dataLen := objDataLen(obj, 765) + out, err := putObjectWithData(dataLen, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != nullVersionId { + return fmt.Errorf("expected the versionId to be %v, instead got %v", + nullVersionId, getString(res.VersionId)) + } + if res.ContentLength == nil || *res.ContentLength != dataLen { + return fmt.Errorf("expected the Content-Length to be %v, instead got %v", + dataLen, res.ContentLength) + } + if getString(res.ETag) != getString(out.res.ETag) { + return fmt.Errorf("expected the ETag to be %v, instead got %v", + getString(out.res.ETag), getString(res.ETag)) + } + + return nil + }) + }, withVersioning(types.BucketVersioningStatusSuspended)) +} + func Versioning_HeadObject_delete_marker(s *S3Conf) error { testName := "Versioning_HeadObject_delete_marker" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -1611,6 +1710,120 @@ func Versioning_GetObject_null_versionId_obj(s *S3Conf) error { }) } +// Versioning_GetObject_null_version_without_versionId reads an object put +// into a versioning-suspended bucket, without naming a version id. The object +// is the null version and the response has to report it as null rather than +// leave the version id out. +func Versioning_GetObject_null_version_without_versionId(s *S3Conf) error { + testName := "Versioning_GetObject_null_version_without_versionId" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + dataLen := objDataLen(obj, 543) + out, err := putObjectWithData(dataLen, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + // a put into a versioning-suspended bucket creates the null + // version and reports no version id + if out.res.VersionId != nil { + return fmt.Errorf("expected PutObject response to omit versionId, instead got %v", + getString(out.res.VersionId)) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != nullVersionId { + return fmt.Errorf("expected the versionId to be %v, instead got %v", + nullVersionId, getString(res.VersionId)) + } + if res.ContentLength == nil || *res.ContentLength != dataLen { + return fmt.Errorf("expected the Content-Length to be %v, instead got %v", + dataLen, res.ContentLength) + } + if getString(res.ETag) != getString(out.res.ETag) { + return fmt.Errorf("expected the ETag to be %v, instead got %v", + getString(out.res.ETag), getString(res.ETag)) + } + + return nil + }) + }, withVersioning(types.BucketVersioningStatusSuspended)) +} + +// Versioning_unversioned_bucket_omits_versionId reads an object in a bucket +// that never had versioning configured. Such a bucket has no versions at all, +// so neither GetObject nor HeadObject reports a version id, not even the null +// one, even though the gateway itself runs with versioning enabled. +func Versioning_unversioned_bucket_omits_versionId(s *S3Conf) error { + testName := "Versioning_unversioned_bucket_omits_versionId" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + vRes, err := s3client.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + // guard the premise of the test + if vRes.Status != "" { + return fmt.Errorf("expected the bucket versioning to be unconfigured, instead got %v", + vRes.Status) + } + + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + _, err := putObjectWithData(objDataLen(obj, 432), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + }, s3client) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + gRes, err := s3client.GetObject(ctx, &s3.GetObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + if gRes.VersionId != nil { + return fmt.Errorf("expected GetObject to omit the versionId, instead got %v", + *gRes.VersionId) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + hRes, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + if hRes.VersionId != nil { + return fmt.Errorf("expected HeadObject to omit the versionId, instead got %v", + *hRes.VersionId) + } + + return nil + }) + }) +} + func Versioning_GetObjectAttributes_invalid_versionId(s *S3Conf) error { testName := "Versioning_GetObjectAttributes_invalid_versionId" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -1961,6 +2174,122 @@ func Versioning_DeleteObject_dir_object_latest_version(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +// Versioning_DeleteObject_latest_version_with_null_version deletes the current +// version of an object whose history also holds a null version, created while +// versioning was suspended. The version that becomes current has to be the one +// created right before the deleted version, not the older null version. +func Versioning_DeleteObject_latest_version_with_null_version(s *S3Conf) error { + testName := "Versioning_DeleteObject_latest_version_with_null_version" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + // the versions are told apart by a metadata entry, as a + // directory object carries no data + put := func(marker string) (*s3.PutObjectOutput, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + defer cancel() + return s3client.PutObject(ctx, &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + Metadata: map[string]string{"marker": marker}, + }) + } + + if _, err := put("v1"); err != nil { + return err + } + if _, err := put("v2"); err != nil { + return err + } + + err := putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusSuspended) + if err != nil { + return err + } + + // the null version sits in the middle of the version history + if _, err := put("null"); err != nil { + return err + } + + err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled) + if err != nil { + return err + } + + third, err := put("v3") + if err != nil { + return err + } + latest, err := put("v4") + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: latest.VersionId, + }) + cancel() + if err != nil { + return err + } + if getString(out.VersionId) != getString(latest.VersionId) { + return fmt.Errorf("expected the deleted versionId to be %v, instead got %v", + getString(latest.VersionId), getString(out.VersionId)) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + + expectedMeta := map[string]string{"marker": "v3"} + if !areMapsSame(res.Metadata, expectedMeta) { + return fmt.Errorf("expected the object metadata to be %v, instead got %v", + expectedMeta, res.Metadata) + } + if getString(res.VersionId) != getString(third.VersionId) { + return fmt.Errorf("expected the current versionId to be %v, instead got %v", + getString(third.VersionId), getString(res.VersionId)) + } + + // the null version has to be left untouched by the delete + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + versions, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + Prefix: &obj, + }) + cancel() + if err != nil { + return err + } + + nullFound := false + for _, v := range versions.Versions { + if getString(v.VersionId) == nullVersionId { + nullFound = true + if v.IsLatest != nil && *v.IsLatest { + return fmt.Errorf("expected the null version not to be the latest") + } + } + } + if !nullFound { + return fmt.Errorf("expected the null version to be kept in %v", + versions.Versions) + } + + 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 {