From a606e57bbd49382891f655efbb02b75ec3569681 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Fri, 3 Oct 2025 00:16:30 +0400 Subject: [PATCH] fix: correct a few object lock behaviors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #1565 Fixes #1561 Fixes #1300 This PR focuses on three main changes: 1. **Prioritizing object-level lock configuration over bucket-level default retention** When an object is uploaded with a specific retention configuration, it takes precedence over the bucket’s default retention set via `PutObjectLockConfiguration`. If the object’s retention expires, the object must become available for write operations, even if the bucket-level default retention is still active. 2. **Preventing object lock configuration from being disabled once enabled** To align with AWS S3 behavior, once object lock is enabled for a bucket, it can no longer be disabled. Previously, sending an empty `Enabled` field in the payload would disable object lock. Now, this behavior is removed—an empty `Enabled` field will result in a `MalformedXML` error. This creates a challenge for integration tests that need to clean up locked objects in order to delete the bucket. To handle this, a method has been implemented that: * Removes any legal hold if present. * Applies a temporary retention with a "retain until" date set 3 seconds ahead. * Waits for 3 seconds before deleting the object and bucket. 3. **Allowing object lock to be enabled on existing buckets via `PutObjectLockConfiguration`** Object lock can now be enabled on an existing bucket if it wasn’t enabled at creation time. * If versioning is enabled at the gateway level, the behavior matches AWS S3: object lock can only be enabled when bucket versioning status is `Enabled`. * If versioning is not enabled at the gateway level, object lock can always be enabled on existing buckets via `PutObjectLockConfiguration`. * In Azure (which does not support bucket versioning), enabling object lock is always allowed. This change also fixes the error message returned in this scenario for better clarity. --- auth/object_lock.go | 52 +++-- backend/azure/azure.go | 18 -- backend/posix/posix.go | 30 +-- s3err/s3err.go | 2 +- tests/integration/group-tests.go | 23 +- tests/integration/tests.go | 366 +++++++------------------------ tests/integration/utils.go | 144 ++++++++++-- 7 files changed, 264 insertions(+), 371 deletions(-) diff --git a/auth/object_lock.go b/auth/object_lock.go index 8ab6942..a46aed0 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -41,7 +41,7 @@ func ParseBucketLockConfigurationInput(input []byte) ([]byte, error) { return nil, s3err.GetAPIError(s3err.ErrMalformedXML) } - if lockConfig.ObjectLockEnabled != "" && lockConfig.ObjectLockEnabled != types.ObjectLockEnabledEnabled { + if lockConfig.ObjectLockEnabled != types.ObjectLockEnabledEnabled { return nil, s3err.GetAPIError(s3err.ErrMalformedXML) } @@ -272,31 +272,35 @@ func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects [ } if retention.Mode != "" && retention.RetainUntilDate != nil { - if retention.RetainUntilDate.After(time.Now()) { - switch retention.Mode { - case types.ObjectLockRetentionModeGovernance: - if !bypass { - return s3err.GetAPIError(s3err.ErrObjectLocked) - } else { - policy, err := be.GetBucketPolicy(ctx, bucket) - if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchBucketPolicy)) { - return s3err.GetAPIError(s3err.ErrObjectLocked) - } - if err != nil { - return err - } - if isBucketPublic { - err = VerifyPublicBucketPolicy(policy, bucket, key, BypassGovernanceRetentionAction) - } else { - err = VerifyBucketPolicy(policy, userAccess, bucket, key, BypassGovernanceRetentionAction) - } - if err != nil { - return s3err.GetAPIError(s3err.ErrObjectLocked) - } - } - case types.ObjectLockRetentionModeCompliance: + if retention.RetainUntilDate.Before(time.Now()) { + // if the object retention is expired, the object + // is allowed for write operations(delete, modify) + return nil + } + + switch retention.Mode { + case types.ObjectLockRetentionModeGovernance: + if !bypass { return s3err.GetAPIError(s3err.ErrObjectLocked) + } else { + policy, err := be.GetBucketPolicy(ctx, bucket) + if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchBucketPolicy)) { + return s3err.GetAPIError(s3err.ErrObjectLocked) + } + if err != nil { + return err + } + if isBucketPublic { + err = VerifyPublicBucketPolicy(policy, bucket, key, BypassGovernanceRetentionAction) + } else { + err = VerifyBucketPolicy(policy, userAccess, bucket, key, BypassGovernanceRetentionAction) + } + if err != nil { + return s3err.GetAPIError(s3err.ErrObjectLocked) + } } + case types.ObjectLockRetentionModeCompliance: + return s3err.GetAPIError(s3err.ErrObjectLocked) } } } diff --git a/backend/azure/azure.go b/backend/azure/azure.go index 5b1514b..bd205c5 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -1644,24 +1644,6 @@ func (az *Azure) DeleteBucketCors(ctx context.Context, bucket string) error { } func (az *Azure) PutObjectLockConfiguration(ctx context.Context, bucket string, config []byte) error { - cfg, err := az.getContainerMetaData(ctx, bucket, string(keyBucketLock)) - if err != nil { - return err - } - - if len(cfg) == 0 { - return s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed) - } - - var bucketLockCfg auth.BucketLockConfig - if err := json.Unmarshal(cfg, &bucketLockCfg); err != nil { - return fmt.Errorf("unmarshal object lock config: %w", err) - } - - if !bucketLockCfg.Enabled { - return s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed) - } - return az.setContainerMetaData(ctx, bucket, string(keyBucketLock), config) } diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 7e22828..18e435e 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -624,7 +624,7 @@ func (p *Posix) getBucketVersioningStatus(ctx context.Context, bucket string) (t if errors.Is(err, s3err.GetAPIError(s3err.ErrVersioningNotConfigured)) { return "", nil } - if err != nil && !errors.Is(err, s3err.GetAPIError(s3err.ErrVersioningNotConfigured)) { + if err != nil { return "", err } @@ -4805,21 +4805,21 @@ func (p *Posix) PutObjectLockConfiguration(ctx context.Context, bucket string, c return fmt.Errorf("stat bucket: %w", err) } - cfg, err := p.meta.RetrieveAttribute(nil, bucket, "", bucketLockKey) - if errors.Is(err, meta.ErrNoSuchKey) { - return s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed) - } - if err != nil { - return fmt.Errorf("get object lock config: %w", err) - } + if p.versioningEnabled() { + // if versioning is enabled on gateway level and bucket versioning + // status is not `Enabled`, object lock can't be enabled. + // if object lock has been enabled on bucket creation + // it means the versioning has been enabled alongside with object lock + // and it can't be suspended ever again + status, err := p.getBucketVersioningStatus(ctx, bucket) + if err != nil { + return err + } - var bucketLockCfg auth.BucketLockConfig - if err := json.Unmarshal(cfg, &bucketLockCfg); err != nil { - return fmt.Errorf("unmarshal object lock config: %w", err) - } - - if !bucketLockCfg.Enabled { - return s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed) + if status != types.BucketVersioningStatusEnabled { + // if versioning is enabled on gateway level + return s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed) + } } err = p.meta.StoreAttribute(nil, bucket, "", bucketLockKey, config) diff --git a/s3err/s3err.go b/s3err/s3err.go index 84b651f..dc71f40 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -549,7 +549,7 @@ var errorCodeResponse = map[ErrorCode]APIError{ }, ErrObjectLockConfigurationNotAllowed: { Code: "InvalidBucketState", - Description: "Object Lock configuration cannot be enabled on existing buckets.", + Description: "Versioning must be 'Enabled' on the bucket to apply a Object Lock configuration", HTTPStatusCode: http.StatusConflict, }, ErrObjectLocked: { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 0822cbc..49b3d8b 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -597,7 +597,9 @@ func TestCORSMiddleware(s *S3Conf) { func TestPutObjectLockConfiguration(s *S3Conf) { PutObjectLockConfiguration_non_existing_bucket(s) PutObjectLockConfiguration_empty_config(s) - PutObjectLockConfiguration_not_enabled_on_bucket_creation(s) + if !s.versioningEnabled { + PutObjectLockConfiguration_not_enabled_on_bucket_creation(s) + } PutObjectLockConfiguration_invalid_status(s) PutObjectLockConfiguration_invalid_mode(s) PutObjectLockConfiguration_both_years_and_days(s) @@ -615,7 +617,6 @@ func TestPutObjectRetention(s *S3Conf) { PutObjectRetention_non_existing_bucket(s) PutObjectRetention_non_existing_object(s) PutObjectRetention_unset_bucket_object_lock_config(s) - PutObjectRetention_disabled_bucket_object_lock_config(s) PutObjectRetention_expired_retain_until_date(s) PutObjectRetention_invalid_mode(s) PutObjectRetention_overwrite_compliance_mode(s) @@ -640,7 +641,6 @@ func TestPutObjectLegalHold(s *S3Conf) { PutObjectLegalHold_invalid_body(s) PutObjectLegalHold_invalid_status(s) PutObjectLegalHold_unset_bucket_object_lock_config(s) - PutObjectLegalHold_disabled_bucket_object_lock_config(s) PutObjectLegalHold_success(s) } @@ -773,7 +773,9 @@ func TestFullFlow(s *S3Conf) { TestGetObjectRetention(s) TestPutObjectLegalHold(s) TestGetObjectLegalHold(s) - TestWORMProtection(s) + if !s.versioningEnabled { + TestWORMProtection(s) + } TestAccessControl(s) TestRouter(s) // FIXME: The tests should pass for azure as well @@ -926,7 +928,14 @@ func TestAccessControl(s *S3Conf) { func TestPublicBuckets(s *S3Conf) { PublicBucket_default_private_bucket(s) PublicBucket_public_bucket_policy(s) - PublicBucket_public_object_policy(s) + if !s.versioningEnabled { + // This test targets gateway actions when bucket grants + // public access to object operations: no specific + // bucket versioning operations. As object version cleanup + // is hard to perform, run the test only on the versioning-disabled + // gateway instance + PublicBucket_public_object_policy(s) + } PublicBucket_public_acl(s) PublicBucket_signed_streaming_payload(s) PublicBucket_incorrect_sha256_hash(s) @@ -993,6 +1002,7 @@ func TestVersioning(s *S3Conf) { Versioning_UploadPartCopy_non_existing_versionId(s) Versioning_UploadPartCopy_from_an_object_version(s) // Object lock configuration + Versioning_object_lock_not_enabled_on_bucket_creation(s) Versioning_Enable_object_lock(s) Versioning_status_switch_to_suspended_with_object_lock(s) // Object-Lock Retention @@ -1438,7 +1448,6 @@ func GetIntTests() IntTests { "PutObjectRetention_non_existing_bucket": PutObjectRetention_non_existing_bucket, "PutObjectRetention_non_existing_object": PutObjectRetention_non_existing_object, "PutObjectRetention_unset_bucket_object_lock_config": PutObjectRetention_unset_bucket_object_lock_config, - "PutObjectRetention_disabled_bucket_object_lock_config": PutObjectRetention_disabled_bucket_object_lock_config, "PutObjectRetention_expired_retain_until_date": PutObjectRetention_expired_retain_until_date, "PutObjectRetention_invalid_mode": PutObjectRetention_invalid_mode, "PutObjectRetention_overwrite_compliance_mode": PutObjectRetention_overwrite_compliance_mode, @@ -1457,7 +1466,6 @@ func GetIntTests() IntTests { "PutObjectLegalHold_invalid_body": PutObjectLegalHold_invalid_body, "PutObjectLegalHold_invalid_status": PutObjectLegalHold_invalid_status, "PutObjectLegalHold_unset_bucket_object_lock_config": PutObjectLegalHold_unset_bucket_object_lock_config, - "PutObjectLegalHold_disabled_bucket_object_lock_config": PutObjectLegalHold_disabled_bucket_object_lock_config, "PutObjectLegalHold_success": PutObjectLegalHold_success, "GetObjectLegalHold_non_existing_bucket": GetObjectLegalHold_non_existing_bucket, "GetObjectLegalHold_non_existing_object": GetObjectLegalHold_non_existing_object, @@ -1590,6 +1598,7 @@ func GetIntTests() IntTests { "Versioning_Multipart_Upload_overwrite_an_object": Versioning_Multipart_Upload_overwrite_an_object, "Versioning_UploadPartCopy_non_existing_versionId": Versioning_UploadPartCopy_non_existing_versionId, "Versioning_UploadPartCopy_from_an_object_version": Versioning_UploadPartCopy_from_an_object_version, + "Versioning_object_lock_not_enabled_on_bucket_creation": Versioning_object_lock_not_enabled_on_bucket_creation, "Versioning_Enable_object_lock": Versioning_Enable_object_lock, "Versioning_status_switch_to_suspended_with_object_lock": Versioning_status_switch_to_suspended_with_object_lock, "Versioning_PutObjectRetention_invalid_versionId": Versioning_PutObjectRetention_invalid_versionId, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index f718a92..f7a66c0 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -3046,14 +3046,10 @@ func PutObject_with_object_lock(s *S3Conf) error { return fmt.Errorf("expected object lock mode to be %v, instead got %v", types.ObjectLockModeCompliance, out.ObjectLockMode) } if out.ObjectLockLegalHoldStatus != types.ObjectLockLegalHoldStatusOn { - return fmt.Errorf("expected object lock legal hold status to be %v, instead got %v", types.ObjectLockLegalHoldStatusOn, out.ObjectLockLegalHoldStatus) + return fmt.Errorf("expected object lock mode to be %v, instead got %v", types.ObjectLockLegalHoldStatusOn, out.ObjectLockLegalHoldStatus) } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj, removeLegalHold: true, isCompliance: true}}) }, withLock()) } @@ -8121,12 +8117,7 @@ func CopyObject_with_legal_hold(s *S3Conf) error { types.ObjectLockLegalHoldStatusOn, res.LegalHold.Status) } - err = changeBucketObjectLockStatus(s3client, bucket, false) - if err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: dstObj, removeOnlyLeglHold: true}}) }, withLock()) } @@ -8176,12 +8167,7 @@ func CopyObject_with_retention_lock(s *S3Conf) error { retDate.Format(time.RFC1123), res.Retention.RetainUntilDate.Format(time.RFC1123)) } - err = changeBucketObjectLockStatus(s3client, bucket, false) - if err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: dstObj}}) }, withLock()) } @@ -9177,11 +9163,7 @@ func CreateMultipartUpload_with_object_lock(s *S3Conf) error { types.ObjectLockModeGovernance, resp.ObjectLockMode) } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj, removeLegalHold: true}}) }, withLock()) } @@ -15962,10 +15944,11 @@ func PutObjectLockConfiguration_not_enabled_on_bucket_creation(s *S3Conf) error }, }) cancel() - if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed)); err != nil { - return err - } - return nil + // this test cases address the successful object lock status upload + // on versioning-disabled gateway mode, where versioning is not supported + // and object lock may be enabled without bucket versioning status check + // Note: this is not S3 compatible feature. + return err }) } @@ -16219,10 +16202,6 @@ func PutObjectRetention_non_existing_bucket(s *S3Conf) error { func PutObjectRetention_non_existing_object(s *S3Conf) error { testName := "PutObjectRetention_non_existing_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } - date := time.Now().Add(time.Hour * 3) ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ @@ -16271,52 +16250,9 @@ func PutObjectRetention_unset_bucket_object_lock_config(s *S3Conf) error { }) } -func PutObjectRetention_disabled_bucket_object_lock_config(s *S3Conf) error { - testName := "PutObjectRetention_disabled_bucket_object_lock_config" - return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) - _, err := s3client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{ - Bucket: &bucket, - ObjectLockConfiguration: &types.ObjectLockConfiguration{}, - }) - cancel() - if err != nil { - return err - } - - date := time.Now().Add(time.Hour * 3) - key := "my-obj" - - _, err = putObjects(s3client, []string{key}, bucket) - if err != nil { - return err - } - - ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) - _, err = s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ - Bucket: &bucket, - Key: &key, - Retention: &types.ObjectLockRetention{ - Mode: types.ObjectLockRetentionModeCompliance, - RetainUntilDate: &date, - }, - }) - cancel() - if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidBucketObjectLockConfiguration)); err != nil { - return err - } - - return nil - }, withLock()) -} - func PutObjectRetention_expired_retain_until_date(s *S3Conf) error { testName := "PutObjectRetention_expired_retain_until_date" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } - date := time.Now().Add(-time.Hour * 3) ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) @@ -16397,11 +16333,7 @@ func PutObjectRetention_overwrite_compliance_mode(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj, isCompliance: true}}) }, withLock()) } @@ -16445,11 +16377,7 @@ func PutObjectRetention_overwrite_compliance_with_compliance(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj, isCompliance: true}}) }, withLock()) } @@ -16493,11 +16421,7 @@ func PutObjectRetention_overwrite_governance_with_governance(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj}}) }, withLock()) } @@ -16539,11 +16463,7 @@ func PutObjectRetention_overwrite_governance_without_bypass_specified(s *S3Conf) return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj}}) }, withLock()) } @@ -16599,21 +16519,13 @@ func PutObjectRetention_overwrite_governance_with_permission(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: obj, isCompliance: true}}) }, withLock()) } func PutObjectRetention_success(s *S3Conf) error { testName := "PutObjectRetention_success" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } - date := time.Now().Add(time.Hour * 3) key := "my-obj" @@ -16636,11 +16548,7 @@ func PutObjectRetention_success(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, isCompliance: true}}) }, withLock()) } @@ -16727,9 +16635,6 @@ func GetObjectRetention_unset_config(s *S3Conf) error { func GetObjectRetention_success(s *S3Conf) error { testName := "GetObjectRetention_success" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } key := "my-obj" _, err := putObjects(s3client, []string{key}, bucket) if err != nil { @@ -16777,11 +16682,7 @@ func GetObjectRetention_success(s *S3Conf) error { // return fmt.Errorf("expected retain until date to be %v, instead got %v", retention.RetainUntilDate.Format(iso8601Format), ret.RetainUntilDate.Format(iso8601Format)) // } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, isCompliance: true}}) }, withLock()) } @@ -16805,10 +16706,6 @@ func PutObjectLegalHold_non_existing_bucket(s *S3Conf) error { func PutObjectLegalHold_non_existing_object(s *S3Conf) error { testName := "PutObjectLegalHold_non_existing_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } - ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ Bucket: &bucket, @@ -16890,50 +16787,9 @@ func PutObjectLegalHold_unset_bucket_object_lock_config(s *S3Conf) error { }) } -func PutObjectLegalHold_disabled_bucket_object_lock_config(s *S3Conf) error { - testName := "PutObjectLegalHold_disabled_bucket_object_lock_config" - return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) - _, err := s3client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{ - Bucket: &bucket, - ObjectLockConfiguration: &types.ObjectLockConfiguration{}, - }) - cancel() - if err != nil { - return err - } - - key := "my-obj" - - _, err = putObjects(s3client, []string{key}, bucket) - if err != nil { - return err - } - - ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) - _, err = s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ - Bucket: &bucket, - Key: &key, - LegalHold: &types.ObjectLockLegalHold{ - Status: types.ObjectLockLegalHoldStatusOn, - }, - }) - cancel() - if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidBucketObjectLockConfiguration)); err != nil { - return err - } - - return nil - }, withLock()) -} - func PutObjectLegalHold_success(s *S3Conf) error { testName := "PutObjectLegalHold_success" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } - key := "my-obj" _, err := putObjects(s3client, []string{key}, bucket) @@ -16954,11 +16810,7 @@ func PutObjectLegalHold_success(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, removeOnlyLeglHold: true}}) }, withLock()) } @@ -17045,9 +16897,6 @@ func GetObjectLegalHold_unset_config(s *S3Conf) error { func GetObjectLegalHold_success(s *S3Conf) error { testName := "GetObjectLegalHold_success" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } key := "my-obj" _, err := putObjects(s3client, []string{key}, bucket) if err != nil { @@ -17082,11 +16931,7 @@ func GetObjectLegalHold_success(s *S3Conf) error { resp.LegalHold.Status) } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: key, removeOnlyLeglHold: true}}) }, withLock()) } @@ -17801,11 +17646,7 @@ func WORMProtection_bucket_object_lock_configuration_compliance_mode(s *S3Conf) if err := checkWORMProtection(s3client, bucket, object); err != nil { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: object, isCompliance: true}}) }, withLock()) } @@ -17840,11 +17681,7 @@ func WORMProtection_bucket_object_lock_configuration_governance_mode(s *S3Conf) if err := checkWORMProtection(s3client, bucket, object); err != nil { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: object}}) }, withLock()) } @@ -17896,15 +17733,7 @@ func WORMProtection_bucket_object_lock_governance_bypass_delete(s *S3Conf) error BypassGovernanceRetention: &bypass, }) cancel() - if err != nil { - return err - } - - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return err }, withLock()) } @@ -17968,15 +17797,7 @@ func WORMProtection_bucket_object_lock_governance_bypass_delete_multiple(s *S3Co }, }) cancel() - if err != nil { - return err - } - - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return err }, withLock()) } @@ -18008,11 +17829,8 @@ func WORMProtection_object_lock_retention_compliance_locked(s *S3Conf) error { if err := checkWORMProtection(s3client, bucket, object); err != nil { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: object, isCompliance: true}}) }, withLock()) } @@ -18044,11 +17862,7 @@ func WORMProtection_object_lock_retention_governance_locked(s *S3Conf) error { if err := checkWORMProtection(s3client, bucket, object); err != nil { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: object}}) }, withLock()) } @@ -18095,15 +17909,7 @@ func WORMProtection_object_lock_retention_governance_bypass_overwrite(s *S3Conf) Key: &object, }) cancel() - if err != nil { - return err - } - - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return err }, withLock()) } @@ -18152,15 +17958,7 @@ func WORMProtection_object_lock_retention_governance_bypass_delete(s *S3Conf) er BypassGovernanceRetention: &bypass, }) cancel() - if err != nil { - return err - } - - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return err }, withLock()) } @@ -18224,25 +18022,13 @@ func WORMProtection_object_lock_retention_governance_bypass_delete_mul(s *S3Conf }, }) cancel() - if err != nil { - return err - } - - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return err }, withLock()) } func WORMProtection_object_lock_legal_hold_locked(s *S3Conf) error { testName := "WORMProtection_object_lock_legal_hold_locked" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { - if err := changeBucketObjectLockStatus(s3client, bucket, true); err != nil { - return err - } - object := "my-obj" _, err := putObjects(s3client, []string{object}, bucket) @@ -18268,11 +18054,7 @@ func WORMProtection_object_lock_legal_hold_locked(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: object, removeOnlyLeglHold: true}}) }, withLock()) } @@ -18324,15 +18106,7 @@ func WORMProtection_root_bypass_governance_retention_delete_object(s *S3Conf) er BypassGovernanceRetention: &bypass, }) cancel() - if err != nil { - return err - } - - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return err }, withLock()) } @@ -20524,12 +20298,6 @@ func PublicBucket_public_object_policy(s *S3Conf) error { } } - // disable object lock on the bucket - err = changeBucketObjectLockStatus(rootClient, bucket, false) - if err != nil { - return err - } - return nil }, withAnonymousClient(), withLock()) } @@ -23886,6 +23654,27 @@ func Versioning_Enable_object_lock(s *S3Conf) error { }, withLock()) } +func Versioning_object_lock_not_enabled_on_bucket_creation(s *S3Conf) error { + testName := "Versioning_not_enabled_on_bucket_creation" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{ + Bucket: &bucket, + ObjectLockConfiguration: &types.ObjectLockConfiguration{ + ObjectLockEnabled: types.ObjectLockEnabledEnabled, + Rule: &types.ObjectLockRule{ + DefaultRetention: &types.DefaultRetention{ + Mode: types.ObjectLockRetentionModeCompliance, + Days: getPtr(int32(10)), + }, + }, + }, + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotAllowed)) + }) +} + func Versioning_status_switch_to_suspended_with_object_lock(s *S3Conf) error { testName := "Versioning_status_switch_to_suspended_with_object_lock" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -23993,11 +23782,7 @@ func Versioning_Put_GetObjectRetention_success(s *S3Conf) error { types.ObjectLockRetentionModeGovernance, res.Retention.Mode) } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{{key: getString(objVersion.Key), versionId: getString(objVersion.VersionId)}}) }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } @@ -24092,11 +23877,13 @@ func Versioning_Put_GetObjectLegalHold_success(s *S3Conf) error { types.ObjectLockLegalHoldStatusOn, res.LegalHold.Status) } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: getString(objVersion.Key), + versionId: getString(objVersion.VersionId), + removeOnlyLeglHold: true, + }, + }) }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } @@ -24135,11 +23922,13 @@ func Versioning_WORM_obj_version_locked_with_legal_hold(s *S3Conf) error { return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(version.VersionId), + removeOnlyLeglHold: true, + }, + }) }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } @@ -24180,11 +23969,12 @@ func Versioning_WORM_obj_version_locked_with_governance_retention(s *S3Conf) err return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(version.VersionId), + }, + }) }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } @@ -24225,11 +24015,13 @@ func Versioning_WORM_obj_version_locked_with_compliance_retention(s *S3Conf) err return err } - if err := changeBucketObjectLockStatus(s3client, bucket, false); err != nil { - return err - } - - return nil + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: getString(version.VersionId), + isCompliance: true, + }, + }) }, withLock(), withVersioning(types.BucketVersioningStatusEnabled)) } diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 21d6ede..49ead60 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -48,6 +48,8 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/aws/smithy-go" "github.com/versity/versitygw/s3err" + "golang.org/x/sync/errgroup" + "golang.org/x/sync/semaphore" ) var ( @@ -1100,25 +1102,6 @@ func getMalformedPolicyError(msg string) s3err.APIError { } } -// if true enables, otherwise disables -func changeBucketObjectLockStatus(client *s3.Client, bucket string, status bool) error { - cfg := types.ObjectLockConfiguration{} - if status { - cfg.ObjectLockEnabled = types.ObjectLockEnabledEnabled - } - ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) - _, err := client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{ - Bucket: &bucket, - ObjectLockConfiguration: &cfg, - }) - cancel() - if err != nil { - return err - } - - return nil -} - func putBucketVersioningStatus(client *s3.Client, bucket string, status types.BucketVersioningStatus) error { ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) _, err := client.PutBucketVersioning(ctx, &s3.PutBucketVersioningInput{ @@ -1835,3 +1818,126 @@ func sprintVersions(objects []types.ObjectVersion) string { return strings.Join(names, ",") } + +// objToDelete represents the metadata of an object that needs to be deleted. +// It holds details like the key, version, and legal/compliance lock flags. +type objToDelete struct { + key string // Object key (name) in the bucket + versionId string // Specific object version ID + removeLegalHold bool // Whether to remove legal hold before deletion + removeOnlyLeglHold bool // Whether to only remove legal hold, without deletion + isCompliance bool // Whether the object is under Compliance mode retention +} + +// Worker and retry configuration for deleting locked objects +const ( + maxDelObjWorkers int64 = 20 // Maximum number of concurrent delete workers + maxRetryAttempts int = 3 // Maximum retries for object deletion + lockWaitTime time.Duration = time.Second * 3 // Wait time for lock expiration before retrying delete +) + +// cleanupLockedObjects removes objects from a bucket that may be protected by +// Object Lock (legal hold or retention). +// It handles both Governance and Compliance retention modes and retries deletions +// when necessary. +func cleanupLockedObjects(client *s3.Client, bucket string, objs []objToDelete) error { + eg, ctx := errgroup.WithContext(context.Background()) + + // Semaphore to limit the number of concurrent workers + sem := semaphore.NewWeighted(maxDelObjWorkers) + + for _, obj := range objs { + obj := obj // capture loop variable + + // Acquire worker slot before processing an object + if err := sem.Acquire(ctx, 1); err != nil { + return fmt.Errorf("failed to acquire worker space: %w", err) + } + + eg.Go(func() error { + // Remove legal hold if required + if obj.removeLegalHold || obj.removeOnlyLeglHold { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj.key, + VersionId: getPtr(obj.versionId), + LegalHold: &types.ObjectLockLegalHold{ + Status: types.ObjectLockLegalHoldStatusOff, // Disable legal hold + }, + }) + cancel() + // If object was already deleted, ignore the error + if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) { + return nil + } + if err != nil { + return err + } + + // If only the legal hold needs to be removed, stop here + if obj.removeOnlyLeglHold { + return nil + } + } + + // Apply temporary retention policy to allow deletion + // RetainUntilDate is set a few seconds in the future to handle network delays + retDate := time.Now().Add(lockWaitTime) + mode := types.ObjectLockRetentionModeGovernance + if obj.isCompliance { + mode = types.ObjectLockRetentionModeCompliance + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ + Bucket: &bucket, + Key: &obj.key, + VersionId: getPtr(obj.versionId), + Retention: &types.ObjectLockRetention{ + Mode: mode, + RetainUntilDate: &retDate, + }, + }) + cancel() + + // If object was already deleted, ignore the error + if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) { + return nil + } + if err != nil { + return err + } + + // Wait until retention lock expires before attempting delete + time.Sleep(lockWaitTime) + + // Attempt deletion with retries + attempts := 0 + for attempts != maxRetryAttempts { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &obj.key, + VersionId: getPtr(obj.versionId), + }) + cancel() + if err != nil { + // Retry after a short delay if delete fails + time.Sleep(time.Second) + attempts++ + continue + } + + // Success, no more retries needed + return nil + } + + // Return last error if all retries failed + return err + }) + } + + // Wait for all goroutines to finish, return any error encountered + return eg.Wait() +}