diff --git a/spinifex/handlers/ec2/volume/delete_prefix_test.go b/spinifex/handlers/ec2/volume/delete_prefix_test.go new file mode 100644 index 000000000..c2a213b56 --- /dev/null +++ b/spinifex/handlers/ec2/volume/delete_prefix_test.go @@ -0,0 +1,96 @@ +package handlers_ec2_volume + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/mulgadc/spinifex/spinifex/config" + "github.com/mulgadc/spinifex/spinifex/objectstore" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newPrefixTestConfig() *config.Config { + return &config.Config{ + AZ: "ap-southeast-2a", + Predastore: config.PredastoreConfig{ + Bucket: "test-bucket", + Region: "ap-southeast-2", + Host: fakeS3Host, + AccessKey: "testkey", + SecretKey: "testsecret", + }, + WalDir: "/tmp/test-wal", + } +} + +// racingDeleteStore answers every delete the way a backend does once a +// concurrent sweep has already removed the key. +type racingDeleteStore struct { + objectstore.ObjectStore + + deletes int + err error +} + +func (r *racingDeleteStore) DeleteObject(_ context.Context, input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error) { + r.deletes++ + return nil, r.err +} + +func newPrefixTestStore(t *testing.T, keys ...string) *objectstore.MemoryObjectStore { + t.Helper() + store := objectstore.NewMemoryObjectStore() + for _, key := range keys { + _, err := store.PutObject(context.Background(), &s3.PutObjectInput{ + Bucket: aws.String("test-bucket"), + Key: aws.String(key), + Body: strings.NewReader("chunk"), + }) + require.NoError(t, err) + } + return store +} + +func TestDeleteS3PrefixTreatsMissingObjectAsDeleted(t *testing.T) { + backing := newPrefixTestStore(t, "vol-abc/chunks/chunk.00000028.bin", "vol-abc/config.json") + store := &racingDeleteStore{ + ObjectStore: backing, + err: &objectstore.NoSuchKeyError{Key: "vol-abc/chunks/chunk.00000028.bin"}, + } + svc := NewVolumeServiceImplWithStore(newPrefixTestConfig(), store, nil) + + err := svc.deleteS3Prefix(context.Background(), "vol-abc/") + + require.NoError(t, err, "an object another sweep already removed must not fail the delete") + assert.Equal(t, 2, store.deletes, "the sweep must continue past a missing object") +} + +func TestDeleteS3PrefixPropagatesBackendFailure(t *testing.T) { + backing := newPrefixTestStore(t, "vol-abc/chunks/chunk.00000028.bin") + store := &racingDeleteStore{ObjectStore: backing, err: errors.New("connection refused")} + svc := NewVolumeServiceImplWithStore(newPrefixTestConfig(), store, nil) + + err := svc.deleteS3Prefix(context.Background(), "vol-abc/") + + require.Error(t, err) + assert.Contains(t, err.Error(), "connection refused") +} + +func TestDeleteS3PrefixRemovesEveryObject(t *testing.T) { + store := newPrefixTestStore(t, "vol-abc/chunks/chunk.00000000.bin", "vol-abc/config.json") + svc := NewVolumeServiceImplWithStore(newPrefixTestConfig(), store, nil) + + require.NoError(t, svc.deleteS3Prefix(context.Background(), "vol-abc/")) + + out, err := store.ListObjectsV2(context.Background(), &s3.ListObjectsV2Input{ + Bucket: aws.String("test-bucket"), + Prefix: aws.String("vol-abc/"), + }) + require.NoError(t, err) + assert.Empty(t, out.Contents) +} diff --git a/spinifex/handlers/ec2/volume/service_impl.go b/spinifex/handlers/ec2/volume/service_impl.go index dcbff1b91..8d3feede8 100644 --- a/spinifex/handlers/ec2/volume/service_impl.go +++ b/spinifex/handlers/ec2/volume/service_impl.go @@ -1609,7 +1609,9 @@ func (s *VolumeServiceImpl) deleteS3Prefix(ctx context.Context, prefix string) e Bucket: aws.String(bucket), Key: obj.Key, }) - if err != nil { + // A concurrent sweep may have deleted the object between the list + // and here. It is gone either way, which is what this asked for. + if err != nil && !objectstore.IsNoSuchKeyError(err) { return fmt.Errorf("failed to delete object %s: %w", *obj.Key, err) } } diff --git a/spinifex/objectstore/deleteobject_test.go b/spinifex/objectstore/deleteobject_test.go new file mode 100644 index 000000000..d687ee7b4 --- /dev/null +++ b/spinifex/objectstore/deleteobject_test.go @@ -0,0 +1,85 @@ +package objectstore + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const noSuchKeyBody = ` +NoSuchKeyThe specified key does not exist` + +// deleteResponder answers every DELETE with a fixed status and body, standing in +// for a backend that has already dropped the key. +type deleteResponder struct { + code int + body string +} + +func (d *deleteResponder) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + w.WriteHeader(http.StatusBadRequest) + return + } + w.WriteHeader(d.code) + if d.body != "" { + _, _ = w.Write([]byte(d.body)) + } +} + +func TestS3ObjectStoreDeleteObjectMissingKey(t *testing.T) { + tests := []struct { + name string + code int + body string + }{ + {name: "NoSuchKey body", code: http.StatusNotFound, body: noSuchKeyBody}, + {name: "bare 404", code: http.StatusNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv := httptest.NewServer(&deleteResponder{code: tt.code, body: tt.body}) + defer srv.Close() + + _, err := newS3Store(t, srv.URL).DeleteObject(context.Background(), &s3.DeleteObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("vol-abc/chunks/chunk.00000028.bin"), + }) + + require.Error(t, err) + assert.True(t, IsNoSuchKeyError(err), "want NoSuchKeyError, got %v", err) + }) + } +} + +func TestS3ObjectStoreDeleteObjectPropagatesOtherErrors(t *testing.T) { + srv := httptest.NewServer(&deleteResponder{code: http.StatusInternalServerError}) + defer srv.Close() + + _, err := newS3Store(t, srv.URL).DeleteObject(context.Background(), &s3.DeleteObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("vol-abc/chunks/chunk.00000028.bin"), + }) + + require.Error(t, err) + assert.False(t, IsNoSuchKeyError(err), "a backend failure must not read as a missing key") +} + +func TestS3ObjectStoreDeleteObjectSucceeds(t *testing.T) { + srv := httptest.NewServer(&deleteResponder{code: http.StatusNoContent}) + defer srv.Close() + + _, err := newS3Store(t, srv.URL).DeleteObject(context.Background(), &s3.DeleteObjectInput{ + Bucket: aws.String("bucket"), + Key: aws.String("vol-abc/chunks/chunk.00000028.bin"), + }) + + require.NoError(t, err) +} diff --git a/spinifex/objectstore/objectstore.go b/spinifex/objectstore/objectstore.go index 47b64ba20..6f92285e3 100644 --- a/spinifex/objectstore/objectstore.go +++ b/spinifex/objectstore/objectstore.go @@ -119,8 +119,20 @@ func (s *S3ObjectStore) PutObject(ctx context.Context, input *s3.PutObjectInput) return s.client.PutObjectWithContext(ctx, input) } +// DeleteObject reports a missing key as NoSuchKeyError so callers can tell an +// already-deleted object from a backend failure. S3 itself answers 204 here, but +// Predastore answers 404, and a delete sweep must treat both as done. func (s *S3ObjectStore) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error) { - return s.client.DeleteObjectWithContext(ctx, input) + out, err := s.client.DeleteObjectWithContext(ctx, input) + if err != nil { + var aerr awserr.Error + if errors.As(err, &aerr) && + (aerr.Code() == s3.ErrCodeNoSuchKey || aerr.Code() == "NotFound") { + return nil, &NoSuchKeyError{Key: aws.StringValue(input.Key)} + } + return nil, err + } + return out, nil } func (s *S3ObjectStore) ListObjectsV2(ctx context.Context, input *s3.ListObjectsV2Input) (*s3.ListObjectsV2Output, error) {