mirrors/versitygw
1
0
Fork 0
mirror of https://github.com/versity/versitygw.git synced 2026-07-20 06:52:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #901 from versity/fix/versioning-bucket-cleanup

Browse Source 
fix: Prevents bucket deletion when it contains object versions by ret…
This commit is contained in:
Ben McClelland
2024-10-18 15:28:22 -07:00
committed by GitHub
parent d7d6b60bb1 c803af4688
commit 32f2571056
10 changed files with 112 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/azure/azure.go
+3 -3
View File
@@ -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)
}
backend/backend.go
+2 -2
View File
@@ -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 {
backend/posix/posix.go
+51 -33
View File
@@ -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)
}
names, err := os.ReadDir(*input.Bucket)
if errors.Is(err, fs.ErrNotExist) {
return s3err.GetAPIError(s3err.ErrNoSuchBucket)
}
if err != nil {
return fmt.Errorf("readdir bucket: %w", err)
}
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))
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("remove temp dir: %w", err)
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)
}
}
}
err = os.Remove(*input.Bucket)
if err != nil && err.(*os.PathError).Err == syscall.ENOTEMPTY {
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 len(ents) == 1 && ents[0].Name() != metaTmpDir {
return s3err.GetAPIError(s3err.ErrBucketNotEmpty)
} else if len(ents) > 1 {
return s3err.GetAPIError(s3err.ErrBucketNotEmpty)
}
return nil
}
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 err
}
// Remove the bucket
err = os.RemoveAll(bucket)
if err != nil {
return fmt.Errorf("remove bucket: %w", err)
}
err = p.meta.DeleteAttributes(*input.Bucket, "")
if err != nil {
return fmt.Errorf("remove bucket attributes: %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) {
backend/s3proxy/s3.go
+4 -2
View File
@@ -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)
}
s3api/controllers/backend_moq_test.go
+14 -14
View File
@@ -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
s3api/controllers/base.go
+1 -4
View File
@@ -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,
s3api/controllers/base_test.go
+1 -1
View File
@@ -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 {
s3err/s3err.go
+6
View File
@@ -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.",
tests/integration/group-tests.go
+3
View File
@@ -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,
tests/integration/tests.go
+27
View File
@@ -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))
}