fix: Prevents bucket deletion when it contains object versions by ret…

…urning ErrVersionedBucketNotEmpty error. Enabled object deletion with versionId and delete markers creation with DeleteObject when the versioning status is Suspended
versity · Oct 18, 2024 · c803af4 · c803af4
1 parent a78c826
commit c803af4
Show file tree

Hide file tree

Showing 10 changed files with 108 additions and 55 deletions.
diff --git a/backend/azure/azure.go b/backend/azure/azure.go
@@ -250,8 +250,8 @@ func (az *Azure) HeadBucket(ctx context.Context, input *s3.HeadBucketInput) (*s3
 	return &s3.HeadBucketOutput{}, nil
 }
 
-func (az *Azure) DeleteBucket(ctx context.Context, input *s3.DeleteBucketInput) error {
-	pager := az.client.NewListBlobsFlatPager(*input.Bucket, nil)
+func (az *Azure) DeleteBucket(ctx context.Context, bucket string) error {
+	pager := az.client.NewListBlobsFlatPager(bucket, nil)
 
 	pg, err := pager.NextPage(ctx)
 	if err != nil {
@@ -261,7 +261,7 @@ func (az *Azure) DeleteBucket(ctx context.Context, input *s3.DeleteBucketInput)
 	if len(pg.Segment.BlobItems) > 0 {
 		return s3err.GetAPIError(s3err.ErrBucketNotEmpty)
 	}
-	_, err = az.client.DeleteContainer(ctx, *input.Bucket, nil)
+	_, err = az.client.DeleteContainer(ctx, bucket, nil)
 	return azureErrToS3Err(err)
 }
 

diff --git a/backend/backend.go b/backend/backend.go
@@ -37,7 +37,7 @@ type Backend interface {
 	GetBucketAcl(context.Context, *s3.GetBucketAclInput) ([]byte, error)
 	CreateBucket(_ context.Context, _ *s3.CreateBucketInput, defaultACL []byte) error
 	PutBucketAcl(_ context.Context, bucket string, data []byte) error
-	DeleteBucket(context.Context, *s3.DeleteBucketInput) error
+	DeleteBucket(_ context.Context, bucket string) error
 	PutBucketVersioning(_ context.Context, bucket string, status types.BucketVersioningStatus) error
 	GetBucketVersioning(_ context.Context, bucket string) (s3response.GetBucketVersioningOutput, error)
 	PutBucketPolicy(_ context.Context, bucket string, policy []byte) error
@@ -123,7 +123,7 @@ func (BackendUnsupported) CreateBucket(context.Context, *s3.CreateBucketInput, [
 func (BackendUnsupported) PutBucketAcl(_ context.Context, bucket string, data []byte) error {
 	return s3err.GetAPIError(s3err.ErrNotImplemented)
 }
-func (BackendUnsupported) DeleteBucket(context.Context, *s3.DeleteBucketInput) error {
+func (BackendUnsupported) DeleteBucket(_ context.Context, bucket string) error {
 	return s3err.GetAPIError(s3err.ErrNotImplemented)
 }
 func (BackendUnsupported) PutBucketVersioning(_ context.Context, bucket string, status types.BucketVersioningStatus) error {

diff --git a/backend/posix/posix.go b/backend/posix/posix.go
@@ -376,39 +376,59 @@ func (p *Posix) CreateBucket(ctx context.Context, input *s3.CreateBucketInput, a
 	return nil
 }
 
-func (p *Posix) DeleteBucket(_ context.Context, input *s3.DeleteBucketInput) error {
-	if input.Bucket == nil {
-		return s3err.GetAPIError(s3err.ErrInvalidBucketName)
+func (p *Posix) isBucketEmpty(bucket string) error {
+	if p.versioningEnabled() {
+		ents, err := os.ReadDir(filepath.Join(p.versioningDir, bucket))
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return fmt.Errorf("readdir bucket: %w", err)
+		}
+		if err == nil {
+			if len(ents) == 1 && ents[0].Name() != metaTmpDir {
+				return s3err.GetAPIError(s3err.ErrVersionedBucketNotEmpty)
+			} else if len(ents) > 1 {
+				return s3err.GetAPIError(s3err.ErrVersionedBucketNotEmpty)
+			}
+		}
 	}
 
-	names, err := os.ReadDir(*input.Bucket)
+	ents, err := os.ReadDir(bucket)
+	if err != nil && !errors.Is(err, fs.ErrNotExist) {
+		return fmt.Errorf("readdir bucket: %w", err)
+	}
 	if errors.Is(err, fs.ErrNotExist) {
 		return s3err.GetAPIError(s3err.ErrNoSuchBucket)
 	}
-	if err != nil {
-		return fmt.Errorf("readdir bucket: %w", err)
+	if len(ents) == 1 && ents[0].Name() != metaTmpDir {
+		return s3err.GetAPIError(s3err.ErrBucketNotEmpty)
+	} else if len(ents) > 1 {
+		return s3err.GetAPIError(s3err.ErrBucketNotEmpty)
 	}
 
-	if len(names) == 1 && names[0].Name() == metaTmpDir {
-		// if .sgwtmp is only item in directory
-		// then clean this up before trying to remove the bucket
-		err = os.RemoveAll(filepath.Join(*input.Bucket, metaTmpDir))
-		if err != nil && !errors.Is(err, fs.ErrNotExist) {
-			return fmt.Errorf("remove temp dir: %w", err)
-		}
-	}
+	return nil
+}
 
-	err = os.Remove(*input.Bucket)
-	if err != nil && err.(*os.PathError).Err == syscall.ENOTEMPTY {
-		return s3err.GetAPIError(s3err.ErrBucketNotEmpty)
+func (p *Posix) DeleteBucket(_ context.Context, bucket string) error {
+	if bucket == "" {
+		return s3err.GetAPIError(s3err.ErrInvalidBucketName)
 	}
+
+	// Check if the bucket is empty
+	err := p.isBucketEmpty(bucket)
 	if err != nil {
-		return fmt.Errorf("remove bucket: %w", err)
+		return err
 	}
 
-	err = p.meta.DeleteAttributes(*input.Bucket, "")
+	// Remove the bucket
+	err = os.RemoveAll(bucket)
 	if err != nil {
-		return fmt.Errorf("remove bucket attributes: %w", err)
+		return fmt.Errorf("remove bucket: %w", err)
+	}
+	// Remove the bucket from versioning directory
+	if p.versioningEnabled() {
+		err = os.RemoveAll(filepath.Join(p.versioningDir, bucket))
+		if err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return fmt.Errorf("remove bucket version: %w", err)
+		}
 	}
 
 	return nil
@@ -2393,10 +2413,9 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 	if err != nil {
 		return nil, err
 	}
-	vEnabled := p.isBucketVersioningEnabled(vStatus)
 
 	// Directory objects can't have versions
-	if !isDir && p.versioningEnabled() && vEnabled {
+	if !isDir && p.versioningEnabled() && vStatus != "" {
 		if getString(input.VersionId) == "" {
 			// if the versionId is not specified, make the current version a delete marker
 			fi, err := os.Stat(objpath)
@@ -2533,6 +2552,8 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 					return nil, fmt.Errorf("remove obj version %w", err)
 				}
 
+				p.removeParents(filepath.Join(p.versioningDir, bucket), filepath.Join(genObjVersionKey(object), *input.VersionId))
+
 				return &s3.DeleteObjectOutput{
 					DeleteMarker: &isDelMarker,
 					VersionId:    input.VersionId,
@@ -2552,6 +2573,8 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 				return nil, fmt.Errorf("delete object: %w", err)
 			}
 
+			p.removeParents(filepath.Join(p.versioningDir, bucket), filepath.Join(genObjVersionKey(object), *input.VersionId))
+
 			return &s3.DeleteObjectOutput{
 				DeleteMarker: &isDelMarker,
 				VersionId:    input.VersionId,
@@ -2597,15 +2620,12 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 		return nil, fmt.Errorf("delete object attributes: %w", err)
 	}
 
-	err = p.removeParents(bucket, object)
-	if err != nil {
-		return nil, err
-	}
+	p.removeParents(bucket, object)
 
 	return &s3.DeleteObjectOutput{}, nil
 }
 
-func (p *Posix) removeParents(bucket, object string) error {
+func (p *Posix) removeParents(bucket, object string) {
 	// this will remove all parent directories that were not
 	// specifically uploaded with a put object. we detect
 	// this with a special attribute to indicate these. stop
@@ -2614,7 +2634,6 @@ func (p *Posix) removeParents(bucket, object string) error {
 	objPath := object
 	for {
 		parent := filepath.Dir(objPath)
-
 		if parent == string(filepath.Separator) || parent == "." {
 			// stop removing parents if we hit the bucket directory.
 			break
@@ -2635,7 +2654,6 @@ func (p *Posix) removeParents(bucket, object string) error {
 
 		objPath = parent
 	}
-	return nil
 }
 
 func (p *Posix) DeleteObjects(ctx context.Context, input *s3.DeleteObjectsInput) (s3response.DeleteResult, error) {

diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go
@@ -125,8 +125,10 @@ func (s *S3Proxy) CreateBucket(ctx context.Context, input *s3.CreateBucketInput,
 	return handleError(err)
 }
 
-func (s *S3Proxy) DeleteBucket(ctx context.Context, input *s3.DeleteBucketInput) error {
-	_, err := s.client.DeleteBucket(ctx, input)
+func (s *S3Proxy) DeleteBucket(ctx context.Context, bucket string) error {
+	_, err := s.client.DeleteBucket(ctx, &s3.DeleteBucketInput{
+		Bucket: &bucket,
+	})
 	return handleError(err)
 }
 

diff --git a/s3api/controllers/backend_moq_test.go b/s3api/controllers/backend_moq_test.go
diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go
@@ -2468,10 +2468,7 @@ func (c S3ApiController) DeleteBucket(ctx *fiber.Ctx) error {
 			})
 	}
 
-	err = c.be.DeleteBucket(ctx.Context(),
-		&s3.DeleteBucketInput{
-			Bucket: &bucket,
-		})
+	err = c.be.DeleteBucket(ctx.Context(), bucket)
 	return SendResponse(ctx, err,
 		&MetaOpts{
 			Logger:      c.logger,

diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go
@@ -1217,7 +1217,7 @@ func TestS3ApiController_DeleteBucket(t *testing.T) {
 	app := fiber.New()
 	s3ApiController := S3ApiController{
 		be: &BackendMock{
-			DeleteBucketFunc: func(context.Context, *s3.DeleteBucketInput) error {
+			DeleteBucketFunc: func(_ context.Context, bucket string) error {
 				return nil
 			},
 			DeleteBucketTaggingFunc: func(contextMoqParam context.Context, bucket string) error {

diff --git a/s3err/s3err.go b/s3err/s3err.go
@@ -57,6 +57,7 @@ const (
 	ErrAccessDenied
 	ErrMethodNotAllowed
 	ErrBucketNotEmpty
+	ErrVersionedBucketNotEmpty
 	ErrBucketAlreadyExists
 	ErrBucketAlreadyOwnedByYou
 	ErrNoSuchBucket
@@ -160,6 +161,11 @@ var errorCodeResponse = map[ErrorCode]APIError{
 		Description:    "The bucket you tried to delete is not empty.",
 		HTTPStatusCode: http.StatusConflict,
 	},
+	ErrVersionedBucketNotEmpty: {
+		Code:           "BucketNotEmpty",
+		Description:    "The bucket you tried to delete is not empty. You must delete all versions in the bucket.",
+		HTTPStatusCode: http.StatusConflict,
+	},
 	ErrBucketAlreadyExists: {
 		Code:           "BucketAlreadyExists",
 		Description:    "The requested bucket name is not available. The bucket name can not be an existing collection, and the bucket namespace is shared by all users of the system. Please select a different name and try again.",

diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go
@@ -545,6 +545,8 @@ func TestVersioning(s *S3Conf) {
 	GetBucketVersioning_non_existing_bucket(s)
 	GetBucketVersioning_empty_response(s)
 	GetBucketVersioning_success(s)
+	// DeleteBucket action
+	Versioning_DeleteBucket_not_empty(s)
 	// PutObject action
 	Versioning_PutObject_suspended_null_versionId_obj(s)
 	Versioning_PutObject_null_versionId_obj(s)
@@ -936,6 +938,7 @@ func GetIntTests() IntTests {
 		"GetBucketVersioning_non_existing_bucket":                             GetBucketVersioning_non_existing_bucket,
 		"GetBucketVersioning_empty_response":                                  GetBucketVersioning_empty_response,
 		"GetBucketVersioning_success":                                         GetBucketVersioning_success,
+		"Versioning_DeleteBucket_not_empty":                                   Versioning_DeleteBucket_not_empty,
 		"Versioning_PutObject_suspended_null_versionId_obj":                   Versioning_PutObject_suspended_null_versionId_obj,
 		"Versioning_PutObject_null_versionId_obj":                             Versioning_PutObject_null_versionId_obj,
 		"Versioning_PutObject_overwrite_null_versionId_obj":                   Versioning_PutObject_overwrite_null_versionId_obj,

diff --git a/tests/integration/tests.go b/tests/integration/tests.go
@@ -10769,6 +10769,28 @@ func GetBucketVersioning_success(s *S3Conf) error {
 	}, withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+func Versioning_DeleteBucket_not_empty(s *S3Conf) error {
+	testName := "Versioning_DeleteBucket_not_empty"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-obj"
+		_, err := createObjVersions(s3client, bucket, obj, 2)
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.DeleteBucket(ctx, &s3.DeleteBucketInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrVersionedBucketNotEmpty)); err != nil {
+			return err
+		}
+
+		return nil
+	}, withVersioning(types.BucketVersioningStatusEnabled))
+}
+
 func Versioning_PutObject_suspended_null_versionId_obj(s *S3Conf) error {
 	testName := "Versioning_PutObject_suspended_null_versionId_obj"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -11144,6 +11166,11 @@ func Versioning_CopyObject_special_chars(s *S3Conf) error {
 			return fmt.Errorf("expected the copied object versionId to be %v, instead got %v", *res.VersionId, *out.VersionId)
 		}
 
+		err = teardown(s, dstBucket)
+		if err != nil {
+			return err
+		}
+
 		return nil
 	}, withVersioning(types.BucketVersioningStatusEnabled))
 }