amadan
objstore: fix AWS provisioner review findings (DeleteObjects errors, checksum options)
Important 1: DeleteObjects is a 200-with-body API — per-key failures (AccessDenied, object-lock) land in out.Errors, not err. Unchecked, EmptyBucket returned nil with objects still in the bucket, and Teardown later died at DeleteBucket's BucketNotEmpty with no key and no reason, retrying forever. deleteObjectBatches now surfaces out.Errors as an error naming the count and first Key/Code/Message. Important 2: the hand-rolled endpoint-override options carried only BaseEndpoint + UsePathStyle, dropping the RequestChecksumCalculation/ ResponseChecksumValidation=WhenRequired pair that s3compat.Options() sets alongside them under a non-AWS endpoint — the pair exists because default-on flexible checksums wrap bodies in aws-chunked framing some S3-compatibles reject, and DeleteObjects (EmptyBucket's own batch delete) is exactly a checksummed request. Added both lines; comment now says explicitly the four lines mirror s3compat.Options and must move in lockstep. New tests: TestEmptyBucketSurfacesPerKeyDeleteErrors (stubbed DeleteObjects.Errors surfaces as an error); TestNewAWSProvisionerEndpointOptions and TestNewAWSProvisionerNoEndpointOptions (white-box, via the real *s3.Client's exported Options(), asserting all four settings land under an endpoint and stay untouched without one).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+117
−6
internal/objstore/aws.go+27 −6internal/objstore/aws_test.go+90 −0
diff --git a/internal/objstore/aws.go b/internal/objstore/aws.go| index df6e75cbe..02cec85f7 100644 |
| --- a/internal/objstore/aws.go |
| +++ b/internal/objstore/aws.go |
| @@ -56,11 +56,19 @@ type awsProvisioner struct { |
| var _ Provisioner = (*awsProvisioner)(nil) |
| // NewAWSProvisioner builds a Provisioner against AWS S3 and IAM in region. |
| -// The S3 client applies s3compat-style path-style addressing plus a base |
| -// endpoint override when endpoint is non-empty (mirroring |
| -// internal/s3compat.Options, hand-rolled here rather than reused so this |
| -// package stays independent of CARLOS_S3_ENDPOINT — the deployment's |
| -// Endpoint is threaded in explicitly instead). |
| +// The S3 client applies s3compat-style options when endpoint is non-empty |
| +// (hand-rolled here rather than calling internal/s3compat.Options directly, |
| +// so this package stays independent of CARLOS_S3_ENDPOINT — the |
| +// deployment's Endpoint is threaded in explicitly instead). These four lines |
| +// mirror s3compat.Options' universal (non-AWS) block line for line and MUST |
| +// move in lockstep with it: BaseEndpoint + UsePathStyle are the addressing |
| +// fix, and the checksum pair exists because default-on flexible checksums |
| +// wrap bodies in aws-chunked framing some S3-compatibles reject — DeleteObjects |
| +// (EmptyBucket's batch delete) is exactly a checksummed request, so dropping |
| +// either checksum line silently breaks teardown against those stores. Not |
| +// applied: s3compat's per-provider Quirks table (e.g. the Hetzner If-Match |
| +// unquoting) — none of it is conditional writes, which this provisioner |
| +// never issues. |
| // |
| // The IAM client never takes an endpoint override: IAM has no S3-compatible |
| // twin, so a non-AWS backend (endpoint set, "aws" backend) still means |
| @@ -74,6 +82,8 @@ func NewAWSProvisioner(ctx context.Context, region, endpoint string) (Provisione |
| if endpoint != "" { |
| o.BaseEndpoint = aws.String(endpoint) |
| o.UsePathStyle = true |
| + o.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired |
| + o.ResponseChecksumValidation = aws.ResponseChecksumValidationWhenRequired |
| } |
| }) |
| iamClient := iam.NewFromConfig(cfg) |
| @@ -206,7 +216,7 @@ func (p *awsProvisioner) deleteObjectBatches(ctx context.Context, bucket string, |
| for i, obj := range objs[start:end] { |
| ids[i] = s3types.ObjectIdentifier{Key: obj.Key} |
| } |
| - _, err := p.s3.DeleteObjects(ctx, &awss3.DeleteObjectsInput{ |
| + out, err := p.s3.DeleteObjects(ctx, &awss3.DeleteObjectsInput{ |
| Bucket: aws.String(bucket), |
| Delete: &s3types.Delete{Objects: ids}, |
| }) |
| @@ -216,6 +226,17 @@ func (p *awsProvisioner) deleteObjectBatches(ctx context.Context, bucket string, |
| } |
| return fmt.Errorf("objstore: delete objects in %q: %w", bucket, err) |
| } |
| + // DeleteObjects is a 200-with-body API: a per-key failure (AccessDenied, |
| + // object-lock, ...) never surfaces as err — it lands in out.Errors while |
| + // the call itself reports success. Left unchecked, EmptyBucket returns |
| + // nil with objects still in the bucket, and Teardown then dies later at |
| + // DeleteBucket's BucketNotEmpty with no key and no reason, retrying |
| + // forever without ever naming the object that's actually stuck. |
| + if len(out.Errors) > 0 { |
| + first := out.Errors[0] |
| + return fmt.Errorf("objstore: delete objects in %q: %d failed, first %s: %s (%s)", |
| + bucket, len(out.Errors), aws.ToString(first.Key), aws.ToString(first.Code), aws.ToString(first.Message)) |
| + } |
| } |
| return nil |
| } |
diff --git a/internal/objstore/aws_test.go b/internal/objstore/aws_test.go| index da7a89d06..20269f4d2 100644 |
| --- a/internal/objstore/aws_test.go |
| +++ b/internal/objstore/aws_test.go |
| @@ -26,6 +26,61 @@ func TestNewAWSProvisioner(t *testing.T) { |
| } |
| } |
| +// TestNewAWSProvisionerEndpointOptions checks the four s3compat-mirrored |
| +// options land on the real client when endpoint is set: BaseEndpoint + |
| +// UsePathStyle (addressing) and both checksum-WhenRequired settings (the |
| +// pair that keeps default-on flexible checksums from wrapping bodies in |
| +// aws-chunked framing some S3-compatibles reject — DeleteObjects, EmptyBucket's |
| +// batch delete, is exactly a checksummed request). White-box: reaches into |
| +// the unexported s3 field because this is the only seam that observes what |
| +// awss3.NewFromConfig actually built, short of a live request. |
| +func TestNewAWSProvisionerEndpointOptions(t *testing.T) { |
| + prov, err := NewAWSProvisioner(context.Background(), "eu-west-1", "http://127.0.0.1:9000") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + p, ok := prov.(*awsProvisioner) |
| + if !ok { |
| + t.Fatalf("NewAWSProvisioner returned %T, want *awsProvisioner", prov) |
| + } |
| + client, ok := p.s3.(*awss3.Client) |
| + if !ok { |
| + t.Fatalf("p.s3 is %T, want *s3.Client", p.s3) |
| + } |
| + opts := client.Options() |
| + if opts.BaseEndpoint == nil || *opts.BaseEndpoint != "http://127.0.0.1:9000" { |
| + t.Fatalf("BaseEndpoint = %v, want http://127.0.0.1:9000", opts.BaseEndpoint) |
| + } |
| + if !opts.UsePathStyle { |
| + t.Fatal("UsePathStyle = false, want true") |
| + } |
| + if opts.RequestChecksumCalculation != aws.RequestChecksumCalculationWhenRequired { |
| + t.Fatalf("RequestChecksumCalculation = %v, want WhenRequired", opts.RequestChecksumCalculation) |
| + } |
| + if opts.ResponseChecksumValidation != aws.ResponseChecksumValidationWhenRequired { |
| + t.Fatalf("ResponseChecksumValidation = %v, want WhenRequired", opts.ResponseChecksumValidation) |
| + } |
| +} |
| + |
| +// TestNewAWSProvisionerNoEndpointOptions checks the stock-AWS path (empty |
| +// endpoint) leaves those four options untouched, matching s3compat.Options' |
| +// own no-op guarantee for a client that talks to real S3. |
| +func TestNewAWSProvisionerNoEndpointOptions(t *testing.T) { |
| + prov, err := NewAWSProvisioner(context.Background(), "eu-west-1", "") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + p := prov.(*awsProvisioner) |
| + client := p.s3.(*awss3.Client) |
| + opts := client.Options() |
| + if opts.BaseEndpoint != nil { |
| + t.Fatalf("BaseEndpoint = %v, want nil", *opts.BaseEndpoint) |
| + } |
| + if opts.UsePathStyle { |
| + t.Fatal("UsePathStyle = true, want false") |
| + } |
| +} |
| + |
| // ---- stub clients ------------------------------------------------------- |
| // |
| // Each stub implements the narrow s3API/iamAPI seam with per-method hooks. |
| @@ -320,6 +375,41 @@ func TestEmptyBucketBatchesAndPaginates(t *testing.T) { |
| } |
| } |
| +// DeleteObjects is a 200-with-body API: a per-key failure lands in |
| +// out.Errors while the call itself reports no err. EmptyBucket must surface |
| +// that as an error naming the count and the first failing key, rather than |
| +// silently returning nil with objects still in the bucket — a caller that |
| +// swallowed this would hang forever retrying DeleteBucket's later |
| +// BucketNotEmpty with no clue which object is stuck. |
| +func TestEmptyBucketSurfacesPerKeyDeleteErrors(t *testing.T) { |
| + p := &awsProvisioner{s3: &stubS3{t: t, |
| + listObjectsV2: func(*awss3.ListObjectsV2Input) (*awss3.ListObjectsV2Output, error) { |
| + return &awss3.ListObjectsV2Output{ |
| + Contents: []s3types.Object{ |
| + {Key: aws.String("locked-obj")}, |
| + {Key: aws.String("other-obj")}, |
| + }, |
| + IsTruncated: aws.Bool(false), |
| + }, nil |
| + }, |
| + deleteObjects: func(*awss3.DeleteObjectsInput) (*awss3.DeleteObjectsOutput, error) { |
| + return &awss3.DeleteObjectsOutput{ |
| + Errors: []s3types.Error{ |
| + {Key: aws.String("locked-obj"), Code: aws.String("AccessDenied"), Message: aws.String("object is under legal hold")}, |
| + }, |
| + }, nil |
| + }, |
| + }} |
| + err := p.EmptyBucket(context.Background(), "b1") |
| + if err == nil { |
| + t.Fatal("expected an error from a DeleteObjects batch with populated Errors, got nil") |
| + } |
| + const want = `objstore: delete objects in "b1": 1 failed, first locked-obj: AccessDenied (object is under legal hold)` |
| + if err.Error() != want { |
| + t.Fatalf("error = %q, want %q", err.Error(), want) |
| + } |
| +} |
| + |
| // DeleteBucket on an absent bucket must be nil. |
| func TestDeleteBucketAbsentIsNil(t *testing.T) { |
| p := &awsProvisioner{s3: &stubS3{t: t, |