Merge pull request #849 from versity/fix/get-bucket-versioning-empty-…

…response fix: Changed the GetBucketVersioning action return type, to return em…
versity · Sep 28, 2024 · 5c40de2 · 5c40de2
2 parents 92af769 + 7b5765b
commit 5c40de2
Show file tree

Hide file tree

Showing 8 changed files with 62 additions and 21 deletions.
diff --git a/backend/backend.go b/backend/backend.go
@@ -39,7 +39,7 @@ type Backend interface {
 	PutBucketAcl(_ context.Context, bucket string, data []byte) error
 	DeleteBucket(context.Context, *s3.DeleteBucketInput) error
 	PutBucketVersioning(_ context.Context, bucket string, status types.BucketVersioningStatus) error
-	GetBucketVersioning(_ context.Context, bucket string) (*s3.GetBucketVersioningOutput, error)
+	GetBucketVersioning(_ context.Context, bucket string) (s3response.GetBucketVersioningOutput, error)
 	PutBucketPolicy(_ context.Context, bucket string, policy []byte) error
 	GetBucketPolicy(_ context.Context, bucket string) ([]byte, error)
 	DeleteBucketPolicy(_ context.Context, bucket string) error
@@ -129,8 +129,8 @@ func (BackendUnsupported) DeleteBucket(context.Context, *s3.DeleteBucketInput) e
 func (BackendUnsupported) PutBucketVersioning(_ context.Context, bucket string, status types.BucketVersioningStatus) error {
 	return s3err.GetAPIError(s3err.ErrNotImplemented)
 }
-func (BackendUnsupported) GetBucketVersioning(_ context.Context, bucket string) (*s3.GetBucketVersioningOutput, error) {
-	return nil, s3err.GetAPIError(s3err.ErrNotImplemented)
+func (BackendUnsupported) GetBucketVersioning(_ context.Context, bucket string) (s3response.GetBucketVersioningOutput, error) {
+	return s3response.GetBucketVersioningOutput{}, s3err.GetAPIError(s3err.ErrNotImplemented)
 }
 func (BackendUnsupported) PutBucketPolicy(_ context.Context, bucket string, policy []byte) error {
 	return s3err.GetAPIError(s3err.ErrNotImplemented)

diff --git a/backend/posix/posix.go b/backend/posix/posix.go
@@ -468,38 +468,41 @@ func (p *Posix) PutBucketVersioning(_ context.Context, bucket string, status typ
 	return nil
 }
 
-func (p *Posix) GetBucketVersioning(_ context.Context, bucket string) (*s3.GetBucketVersioningOutput, error) {
+func (p *Posix) GetBucketVersioning(_ context.Context, bucket string) (s3response.GetBucketVersioningOutput, error) {
 	if !p.versioningEnabled() {
 		// AWS returns empty response, if versioning is not set
 		//TODO: Maybe we need to return our custom error here?
-		return &s3.GetBucketVersioningOutput{}, nil
+		return s3response.GetBucketVersioningOutput{}, nil
 	}
 
 	_, err := os.Stat(bucket)
 	if errors.Is(err, fs.ErrNotExist) {
-		return nil, s3err.GetAPIError(s3err.ErrNoSuchBucket)
+		return s3response.GetBucketVersioningOutput{}, s3err.GetAPIError(s3err.ErrNoSuchBucket)
 	}
 	if err != nil {
-		return nil, fmt.Errorf("stat bucket: %w", err)
+		return s3response.GetBucketVersioningOutput{}, fmt.Errorf("stat bucket: %w", err)
 	}
 
 	vData, err := p.meta.RetrieveAttribute(bucket, "", versioningKey)
 	if errors.Is(err, meta.ErrNoSuchKey) {
-		return &s3.GetBucketVersioningOutput{}, nil
+		return s3response.GetBucketVersioningOutput{}, nil
+	} else if err != nil {
+		return s3response.GetBucketVersioningOutput{}, fmt.Errorf("get bucket versioning config: %w", err)
 	}
 
+	enabled, suspended := types.BucketVersioningStatusEnabled, types.BucketVersioningStatusSuspended
 	switch vData[0] {
 	case 1:
-		return &s3.GetBucketVersioningOutput{
-			Status: types.BucketVersioningStatusEnabled,
+		return s3response.GetBucketVersioningOutput{
+			Status: &enabled,
 		}, nil
 	case 0:
-		return &s3.GetBucketVersioningOutput{
-			Status: types.BucketVersioningStatusSuspended,
+		return s3response.GetBucketVersioningOutput{
+			Status: &suspended,
 		}, nil
 	}
 
-	return &s3.GetBucketVersioningOutput{}, nil
+	return s3response.GetBucketVersioningOutput{}, nil
 }
 
 // Returns the specified bucket versioning status
@@ -509,7 +512,11 @@ func (p *Posix) isBucketVersioningEnabled(ctx context.Context, bucket string) (b
 		return false, err
 	}
 
-	return res.Status == types.BucketVersioningStatusEnabled, nil
+	if res.Status != nil {
+		return *res.Status == types.BucketVersioningStatusEnabled, nil
+	}
+
+	return false, nil
 }
 
 // Generates the object version path in the versioning directory

diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go
@@ -172,12 +172,15 @@ func (s *S3Proxy) PutBucketVersioning(ctx context.Context, bucket string, status
 	return handleError(err)
 }
 
-func (s *S3Proxy) GetBucketVersioning(ctx context.Context, bucket string) (*s3.GetBucketVersioningOutput, error) {
+func (s *S3Proxy) GetBucketVersioning(ctx context.Context, bucket string) (s3response.GetBucketVersioningOutput, error) {
 	out, err := s.client.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{
 		Bucket: &bucket,
 	})
 
-	return out, handleError(err)
+	return s3response.GetBucketVersioningOutput{
+		Status:    &out.Status,
+		MFADelete: &out.MFADelete,
+	}, handleError(err)
 }
 
 func (s *S3Proxy) ListObjectVersions(ctx context.Context, input *s3.ListObjectVersionsInput) (s3response.ListVersionsResult, error) {

diff --git a/s3api/controllers/backend_moq_test.go b/s3api/controllers/backend_moq_test.go
diff --git a/s3api/controllers/base_test.go b/s3api/controllers/base_test.go
@@ -382,8 +382,8 @@ func TestS3ApiController_ListActions(t *testing.T) {
 			GetBucketTaggingFunc: func(contextMoqParam context.Context, bucket string) (map[string]string, error) {
 				return map[string]string{}, nil
 			},
-			GetBucketVersioningFunc: func(contextMoqParam context.Context, bucket string) (*s3.GetBucketVersioningOutput, error) {
-				return &s3.GetBucketVersioningOutput{}, nil
+			GetBucketVersioningFunc: func(contextMoqParam context.Context, bucket string) (s3response.GetBucketVersioningOutput, error) {
+				return s3response.GetBucketVersioningOutput{}, nil
 			},
 			ListObjectVersionsFunc: func(contextMoqParam context.Context, listObjectVersionsInput *s3.ListObjectVersionsInput) (s3response.ListVersionsResult, error) {
 				return s3response.ListVersionsResult{}, nil

diff --git a/s3response/s3response.go b/s3response/s3response.go
@@ -384,3 +384,9 @@ type ListVersionsResult struct {
 	VersionIdMarker     *string
 	Versions            []types.ObjectVersion `xml:"Version"`
 }
+
+type GetBucketVersioningOutput struct {
+	XMLName   xml.Name `xml:"http://s3.amazonaws.com/doc/2006-03-01/ VersioningConfiguration" json:"-"`
+	MFADelete *types.MFADeleteStatus
+	Status    *types.BucketVersioningStatus
+}
diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go
@@ -521,6 +521,7 @@ func TestVersioning(s *S3Conf) {
 	PutBucketVersioning_success(s)
 	// GetBucketVersioning action
 	GetBucketVersioning_non_existing_bucket(s)
+	GetBucketVersioning_empty_response(s)
 	GetBucketVersioning_success(s)
 	Versioning_PutObject_success(s)
 	// CopyObject action
@@ -873,6 +874,7 @@ func GetIntTests() IntTests {
 		"PutBucketVersioning_invalid_status":                                  PutBucketVersioning_invalid_status,
 		"PutBucketVersioning_success":                                         PutBucketVersioning_success,
 		"GetBucketVersioning_non_existing_bucket":                             GetBucketVersioning_non_existing_bucket,
+		"GetBucketVersioning_empty_response":                                  GetBucketVersioning_empty_response,
 		"GetBucketVersioning_success":                                         GetBucketVersioning_success,
 		"Versioning_PutObject_success":                                        Versioning_PutObject_success,
 		"Versioning_CopyObject_success":                                       Versioning_CopyObject_success,

diff --git a/tests/integration/tests.go b/tests/integration/tests.go
@@ -10447,6 +10447,29 @@ func GetBucketVersioning_non_existing_bucket(s *S3Conf) error {
 	})
 }
 
+func GetBucketVersioning_empty_response(s *S3Conf) error {
+	testName := "GetBucketVersioning_empty_response"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		res, err := s3client.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if res.Status != "" {
+			return fmt.Errorf("expected empty versioning status, instead got %v", res.Status)
+		}
+		if res.MFADelete != "" {
+			return fmt.Errorf("expected empty mfa delete status, instead got %v", res.MFADelete)
+		}
+
+		return nil
+	})
+}
+
 func GetBucketVersioning_success(s *S3Conf) error {
 	testName := "GetBucketVersioning_success"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {