diff --git a/backend/azure/azure.go b/backend/azure/azure.go index f8cd9410..465398bb 100644 --- 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 index 41f9b8bd..7b463672 100644 --- 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 index 41064ca7..fbe3f429 100644 --- 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 index 14a68785..fea085d3 100644 --- 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 index 2b1bbac6..febeec6e 100644 --- a/s3api/controllers/backend_moq_test.go +++ b/s3api/controllers/backend_moq_test.go @@ -41,7 +41,7 @@ var _ backend.Backend = &BackendMock{} // CreateMultipartUploadFunc: func(contextMoqParam context.Context, createMultipartUploadInput *s3.CreateMultipartUploadInput) (s3response.InitiateMultipartUploadResult, error) { // panic("mock out the CreateMultipartUpload method") // }, -// DeleteBucketFunc: func(contextMoqParam context.Context, deleteBucketInput *s3.DeleteBucketInput) error { +// DeleteBucketFunc: func(contextMoqParam context.Context, bucket string) error { // panic("mock out the DeleteBucket method") // }, // DeleteBucketOwnershipControlsFunc: func(contextMoqParam context.Context, bucket string) error { @@ -202,7 +202,7 @@ type BackendMock struct { CreateMultipartUploadFunc func(contextMoqParam context.Context, createMultipartUploadInput *s3.CreateMultipartUploadInput) (s3response.InitiateMultipartUploadResult, error) // DeleteBucketFunc mocks the DeleteBucket method. - DeleteBucketFunc func(contextMoqParam context.Context, deleteBucketInput *s3.DeleteBucketInput) error + DeleteBucketFunc func(contextMoqParam context.Context, bucket string) error // DeleteBucketOwnershipControlsFunc mocks the DeleteBucketOwnershipControls method. DeleteBucketOwnershipControlsFunc func(contextMoqParam context.Context, bucket string) error @@ -388,8 +388,8 @@ type BackendMock struct { DeleteBucket []struct { // ContextMoqParam is the contextMoqParam argument value. ContextMoqParam context.Context - // DeleteBucketInput is the deleteBucketInput argument value. - DeleteBucketInput *s3.DeleteBucketInput + // Bucket is the bucket argument value. + Bucket string } // DeleteBucketOwnershipControls holds details about calls to the DeleteBucketOwnershipControls method. DeleteBucketOwnershipControls []struct { @@ -1012,21 +1012,21 @@ func (mock *BackendMock) CreateMultipartUploadCalls() []struct { } // DeleteBucket calls DeleteBucketFunc. -func (mock *BackendMock) DeleteBucket(contextMoqParam context.Context, deleteBucketInput *s3.DeleteBucketInput) error { +func (mock *BackendMock) DeleteBucket(contextMoqParam context.Context, bucket string) error { if mock.DeleteBucketFunc == nil { panic("BackendMock.DeleteBucketFunc: method is nil but Backend.DeleteBucket was just called") } callInfo := struct { - ContextMoqParam context.Context - DeleteBucketInput *s3.DeleteBucketInput + ContextMoqParam context.Context + Bucket string }{ - ContextMoqParam: contextMoqParam, - DeleteBucketInput: deleteBucketInput, + ContextMoqParam: contextMoqParam, + Bucket: bucket, } mock.lockDeleteBucket.Lock() mock.calls.DeleteBucket = append(mock.calls.DeleteBucket, callInfo) mock.lockDeleteBucket.Unlock() - return mock.DeleteBucketFunc(contextMoqParam, deleteBucketInput) + return mock.DeleteBucketFunc(contextMoqParam, bucket) } // DeleteBucketCalls gets all the calls that were made to DeleteBucket. @@ -1034,12 +1034,12 @@ func (mock *BackendMock) DeleteBucket(contextMoqParam context.Context, deleteBuc // // len(mockedBackend.DeleteBucketCalls()) func (mock *BackendMock) DeleteBucketCalls() []struct { - ContextMoqParam context.Context - DeleteBucketInput *s3.DeleteBucketInput + ContextMoqParam context.Context + Bucket string } { var calls []struct { - ContextMoqParam context.Context - DeleteBucketInput *s3.DeleteBucketInput + ContextMoqParam context.Context + Bucket string } mock.lockDeleteBucket.RLock() calls = mock.calls.DeleteBucket diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index b2de22d3..097cc025 100644 --- 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 index 9869a24f..2964d76e 100644 --- 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 index e5499ed0..8153a1af 100644 --- 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 index 4dd8b448..4aa6beba 100644 --- 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 index 8a313c17..ee6e4a8b 100644 --- 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)) }