fix: s3proxy sends bodyless CreateBucket when the configuration is empty

Since 9bde1ddb (tagging support for CreateBucket) the api layer always populates CreateBucketConfiguration, so the aws-sdk serializes an empty <CreateBucketConfiguration/> element on every CreateBucket the s3proxy backend issues. Strict backends reject that request — Ceph RGW answers 400 InvalidArgument — which breaks bucket creation through the gateway entirely for those backends. MinIO and SeaweedFS tolerate the empty element, which is why this went unnoticed. Drop the configuration before calling the backend when it carries no content; a bodyless CreateBucket is accepted by all tested backends for this no-location case. Configurations that carry tags, a location constraint, or location/bucket info are still forwarded unchanged. Adds wire-level unit tests via an injected capturing HTTP client: CreateBucket without tags must send no body (fails before this fix), and configurations carrying tags, a location constraint, location info or bucket info must be forwarded. Verified end-to-end against Ceph RGW (Quincy 17.2.8 and Squid 19.2.0): CreateBucket through the patched gateway succeeds (200) where it previously failed with 400 InvalidArgument, and the created bucket is usable and deletable.
2026-09-22 07:54:14 +00:00 · 2026-09-03 23:38:08 +02:00
parent 4a22b6d9d7
commit e05d0df3ed
2 changed files with 180 additions and 0 deletions
@@ -204,6 +204,15 @@ func (s *S3Proxy) CreateBucket(ctx context.Context, input *s3.CreateBucketInput,
 		}
 	}

+	// drop an empty CreateBucketConfiguration: strict backends (Ceph RGW) reject the empty element
+	if input.CreateBucketConfiguration != nil &&
+		len(input.CreateBucketConfiguration.Tags) == 0 &&
+		input.CreateBucketConfiguration.LocationConstraint == "" &&
+		input.CreateBucketConfiguration.Location == nil &&
+		input.CreateBucketConfiguration.Bucket == nil {
+		input.CreateBucketConfiguration = nil
+	}
+
 	_, err := s.client.CreateBucket(ctx, input)
 	if err != nil {
 		return handleError(err)
@@ -0,0 +1,171 @@
+// Copyright 2023 Versity Software
+// This file is licensed under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package s3proxy
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/credentials"
+	"github.com/aws/aws-sdk-go-v2/service/s3"
+	"github.com/aws/aws-sdk-go-v2/service/s3/types"
+)
+
+// captureHTTPClient records the request the aws sdk sends and answers 200,
+// so tests can assert on the wire format without a real backend.
+type captureHTTPClient struct {
+	body          []byte
+	contentLength int64
+}
+
+func (c *captureHTTPClient) Do(req *http.Request) (*http.Response, error) {
+	c.contentLength = req.ContentLength
+	if req.Body != nil {
+		b, err := io.ReadAll(req.Body)
+		if err != nil {
+			return nil, err
+		}
+		c.body = b
+	} else {
+		c.body = nil
+	}
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Header:     http.Header{},
+		Body:       http.NoBody,
+		Request:    req,
+	}, nil
+}
+
+func newCaptureProxy(t *testing.T) (*S3Proxy, *captureHTTPClient) {
+	t.Helper()
+	capture := &captureHTTPClient{}
+	client := s3.New(s3.Options{
+		Region:       "us-east-1",
+		BaseEndpoint: aws.String("http://backend.test"),
+		UsePathStyle: true,
+		Credentials:  credentials.NewStaticCredentialsProvider("access", "secret", ""),
+		HTTPClient:   capture,
+	})
+	p, err := NewWithClient(context.Background(), client, "")
+	if err != nil {
+		t.Fatalf("NewWithClient: %v", err)
+	}
+	return p, capture
+}
+
+// The api layer always populates CreateBucketConfiguration (it carries the
+// bucket tags), so the backend must drop it when empty: strict backends such
+// as Ceph RGW reject an empty <CreateBucketConfiguration/> element with
+// 400 InvalidArgument, while a bodyless CreateBucket is accepted by all
+// tested backends for this no-location case.
+func TestCreateBucketOmitsEmptyConfiguration(t *testing.T) {
+	p, capture := newCaptureProxy(t)
+
+	err := p.CreateBucket(context.Background(), &s3.CreateBucketInput{
+		Bucket:                    aws.String("test-bucket"),
+		CreateBucketConfiguration: &types.CreateBucketConfiguration{},
+	}, []byte{})
+	if err != nil {
+		t.Fatalf("CreateBucket: %v", err)
+	}
+	if len(capture.body) != 0 {
+		t.Errorf("CreateBucket without tags must send no body, got %q", capture.body)
+	}
+	if capture.contentLength != 0 {
+		t.Errorf("CreateBucket without tags must send Content-Length 0, got %d", capture.contentLength)
+	}
+}
+
+func TestCreateBucketKeepsTaggedConfiguration(t *testing.T) {
+	p, capture := newCaptureProxy(t)
+
+	err := p.CreateBucket(context.Background(), &s3.CreateBucketInput{
+		Bucket: aws.String("test-bucket"),
+		CreateBucketConfiguration: &types.CreateBucketConfiguration{
+			Tags: []types.Tag{
+				{Key: aws.String("env"), Value: aws.String("test")},
+			},
+		},
+	}, []byte{})
+	if err != nil {
+		t.Fatalf("CreateBucket: %v", err)
+	}
+	body := string(capture.body)
+	if !strings.Contains(body, "<Key>env</Key>") || !strings.Contains(body, "<Value>test</Value>") {
+		t.Errorf("CreateBucket with tags must forward the configuration, got %q", body)
+	}
+}
+
+func TestCreateBucketKeepsLocationConstraint(t *testing.T) {
+	p, capture := newCaptureProxy(t)
+
+	err := p.CreateBucket(context.Background(), &s3.CreateBucketInput{
+		Bucket: aws.String("test-bucket"),
+		CreateBucketConfiguration: &types.CreateBucketConfiguration{
+			LocationConstraint: types.BucketLocationConstraint("eu-west-1"),
+		},
+	}, []byte{})
+	if err != nil {
+		t.Fatalf("CreateBucket: %v", err)
+	}
+	if !strings.Contains(string(capture.body), "<LocationConstraint>eu-west-1</LocationConstraint>") {
+		t.Errorf("CreateBucket with a location constraint must forward the configuration, got %q", capture.body)
+	}
+}
+
+func TestCreateBucketKeepsLocationInfo(t *testing.T) {
+	p, capture := newCaptureProxy(t)
+
+	err := p.CreateBucket(context.Background(), &s3.CreateBucketInput{
+		Bucket: aws.String("test-bucket"),
+		CreateBucketConfiguration: &types.CreateBucketConfiguration{
+			Location: &types.LocationInfo{
+				Name: aws.String("usw2-az1"),
+				Type: types.LocationTypeAvailabilityZone,
+			},
+		},
+	}, []byte{})
+	if err != nil {
+		t.Fatalf("CreateBucket: %v", err)
+	}
+	if !strings.Contains(string(capture.body), "usw2-az1") {
+		t.Errorf("CreateBucket with location info must forward the configuration, got %q", capture.body)
+	}
+}
+
+func TestCreateBucketKeepsBucketInfo(t *testing.T) {
+	p, capture := newCaptureProxy(t)
+
+	err := p.CreateBucket(context.Background(), &s3.CreateBucketInput{
+		Bucket: aws.String("test-bucket"),
+		CreateBucketConfiguration: &types.CreateBucketConfiguration{
+			Bucket: &types.BucketInfo{
+				DataRedundancy: types.DataRedundancySingleAvailabilityZone,
+				Type:           types.BucketTypeDirectory,
+			},
+		},
+	}, []byte{})
+	if err != nil {
+		t.Fatalf("CreateBucket: %v", err)
+	}
+	if !strings.Contains(string(capture.body), "Directory") {
+		t.Errorf("CreateBucket with bucket info must forward the configuration, got %q", capture.body)
+	}
+}