From ff41bcf36913771ab67b66e059734344f87ecbe1 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:29:35 +1000 Subject: [PATCH] fix: read only the named documents on explicit-ID describes (#1069) --- spinifex/ebsmetadata/store.go | 45 ++- spinifex/ebsmetadata/store_test.go | 41 +- .../handlers/ec2/image/describe_byid_test.go | 348 +++++++++++++++++ spinifex/handlers/ec2/image/service_impl.go | 280 ++++++++------ .../ec2/snapshot/describe_byid_test.go | 349 ++++++++++++++++++ .../handlers/ec2/snapshot/service_impl.go | 134 +++++-- spinifex/utils/utils.go | 16 + spinifex/utils/utils_test.go | 24 ++ 8 files changed, 1089 insertions(+), 148 deletions(-) create mode 100644 spinifex/handlers/ec2/image/describe_byid_test.go create mode 100644 spinifex/handlers/ec2/snapshot/describe_byid_test.go diff --git a/spinifex/ebsmetadata/store.go b/spinifex/ebsmetadata/store.go index ef65f3441..af360a2e0 100644 --- a/spinifex/ebsmetadata/store.go +++ b/spinifex/ebsmetadata/store.go @@ -229,11 +229,11 @@ const ( amiPrefix = "spinifex/ebsmetadata/v2/amis/" ) -// listFetchConcurrency bounds the object fetches a single listing may have in -// flight. A listing costs one round trip per document, so fetching them one at -// a time makes the call scale with the number of documents in the bucket rather -// than with the answer's size. -const listFetchConcurrency = 16 +// FetchConcurrency bounds the object fetches one metadata read may have in +// flight, whether it is a listing or an explicit-ID fan-out. A fetch costs one +// round trip per document, so fetching them one at a time makes the call scale +// with the number of documents rather than with the answer's size. +const FetchConcurrency = 16 // listFetchTimeout bounds how long one document may hold up a listing. A // document whose shards no longer reassemble does not fail quickly — the read @@ -316,7 +316,7 @@ func listDocuments[T any]( } results := make([]fetched, len(keys)) - slots := make(chan struct{}, listFetchConcurrency) + slots := make(chan struct{}, FetchConcurrency) var wg sync.WaitGroup for i, key := range keys { @@ -359,6 +359,39 @@ func listDocuments[T any]( return documents, nil } +// FetchResult is one document from an explicit-ID fan-out. Err is that ID's +// own store error, so a failed read stays attributable to the ID that caused it. +type FetchResult[T any] struct { + Document T + Err error +} + +// FetchByIDs reads one document per ID through get, bounded to the same +// concurrency a listing uses and positionally aligned with ids so ordering and +// per-ID errors do not depend on goroutine scheduling. +// +// No per-document timeout, unlike a listing: every named document is part of +// the answer, so the caller's own deadline is the bound. +func FetchByIDs[T any](ctx context.Context, ids []string, get func(context.Context, string) (T, error)) []FetchResult[T] { + results := make([]FetchResult[T], len(ids)) + slots := make(chan struct{}, FetchConcurrency) + var wg sync.WaitGroup + + for i, id := range ids { + wg.Add(1) + slots <- struct{}{} + go func(idx int, id string) { + defer wg.Done() + defer func() { <-slots }() + + results[idx].Document, results[idx].Err = get(ctx, id) + }(i, id) + } + wg.Wait() + + return results +} + func (s *Store) PutAMI(ctx context.Context, ami AMI) error { key, err := AMIKey(ami.ImageID) if err != nil { diff --git a/spinifex/ebsmetadata/store_test.go b/spinifex/ebsmetadata/store_test.go index 6d7859eb9..338e918b9 100644 --- a/spinifex/ebsmetadata/store_test.go +++ b/spinifex/ebsmetadata/store_test.go @@ -384,6 +384,45 @@ func TestListAMIs_SkipsCorruptButStrictDoesNot(t *testing.T) { require.ErrorIs(t, err, ErrCorruptDocument) } +// TestFetchByIDs_BoundedAndPositionallyAligned covers what an explicit-ID +// describe needs from the fan-out: reads overlap, the pool bound holds however +// many IDs a caller names, and each result stays with the ID that produced it. +func TestFetchByIDs_BoundedAndPositionallyAligned(t *testing.T) { + objects := objectstore.NewMemoryObjectStore() + store := NewStore(objects, "control-plane") + ctx := context.Background() + + const documents = FetchConcurrency * 4 + ids := make([]string, 0, documents) + for i := range documents { + id := fmt.Sprintf("ami-%02d", i) + ids = append(ids, id) + require.NoError(t, store.PutAMI(ctx, AMI{ImageID: id, Name: id})) + } + // One ID names no document, to pin that its error lands on its own result + // rather than failing the whole fan-out. + ids = append(ids, "ami-absent") + + slow := &slowGetStore{ObjectStore: objects, delay: 20 * time.Millisecond} + measured := NewStore(slow, "control-plane") + + start := time.Now() + results := FetchByIDs(ctx, ids, measured.GetAMI) + elapsed := time.Since(start) + + require.Len(t, results, len(ids)) + assert.Less(t, elapsed, time.Duration(len(ids))*20*time.Millisecond/2, + "named documents must be read concurrently, not one after the other") + assert.LessOrEqual(t, slow.peak(), int64(FetchConcurrency), + "a caller naming hundreds of IDs must not open hundreds of reads") + for i, id := range ids[:documents] { + require.NoError(t, results[i].Err) + assert.Equal(t, id, results[i].Document.ImageID) + } + assert.True(t, objectstore.IsNoSuchKeyError(results[documents].Err), + "the absent ID's own result carries its error") +} + // TestListVolumes_FetchesDocumentsConcurrently is the reason listDocuments has // a worker pool at all. A listing costs one object fetch per document, and // fetching them one at a time makes DescribeVolumes scale with the number of @@ -415,7 +454,7 @@ func TestListVolumes_FetchesDocumentsConcurrently(t *testing.T) { assert.Less(t, elapsed, serial/2, "fetching %d documents took %s; serially it would be %s", documents, elapsed, serial) assert.Equal(t, int64(documents), slow.gets.Load(), "every document must still be fetched") - assert.LessOrEqual(t, slow.peak(), int64(listFetchConcurrency), + assert.LessOrEqual(t, slow.peak(), int64(FetchConcurrency), "the pool bound must hold: an unbounded fan-out over a large bucket is its own problem") } diff --git a/spinifex/handlers/ec2/image/describe_byid_test.go b/spinifex/handlers/ec2/image/describe_byid_test.go new file mode 100644 index 000000000..579400892 --- /dev/null +++ b/spinifex/handlers/ec2/image/describe_byid_test.go @@ -0,0 +1,348 @@ +package handlers_ec2_image_test + +import ( + "context" + "errors" + "strconv" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/ebsmetadata" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/mulgadc/spinifex/spinifex/objectstore" + "github.com/mulgadc/spinifex/spinifex/testutil/recordingstore" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + byIDBucket = "test-bucket" + byIDAccount = "000000000001" + byIDOther = "000000000002" + byIDImage = "ami-target" +) + +func putByIDAMI(t *testing.T, store objectstore.ObjectStore, ami ebsmetadata.AMI) { + t.Helper() + require.NoError(t, ebsmetadata.NewStore(store, byIDBucket).PutAMI(context.Background(), ami)) +} + +// byIDAMIDoc is a complete, readable AMI document owned by ownerAlias, which is +// an account ID for a private image and a name like "amazon" for a system one. +func byIDAMIDoc(imageID, ownerAlias string) ebsmetadata.AMI { + return ebsmetadata.AMI{ + ImageID: imageID, Name: "debian-13", Description: "Debian 13", + Architecture: "x86_64", PlatformDetails: "Linux/UNIX", CreationDate: time.Now().UTC(), + RootDeviceType: "ebs", Virtualization: "hvm", ImageOwnerAlias: ownerAlias, + VolumeSizeGiB: 8, State: "available", Tags: map[string]string{"Name": "base"}, + } +} + +// seedByIDCatalog grows the AMI prefix with another account's images. The prefix +// is unpartitioned, so before the fast path these were fetched and decoded on +// every by-ID describe only to be discarded. +func seedByIDCatalog(t *testing.T, store objectstore.ObjectStore, n int) { + t.Helper() + for i := range n { + putByIDAMI(t, store, byIDAMIDoc("ami-bulk"+strconv.Itoa(i), byIDOther)) + } +} + +func amiKey(t *testing.T, imageID string) string { + t.Helper() + key, err := ebsmetadata.AMIKey(imageID) + require.NoError(t, err) + return key +} + +// A named image costs one document read and no listing, however many images the +// cluster holds. The whole cluster's count was the old cost, so the operation +// count is the assertion, not the answer. +func TestDescribeImagesByID_ReadsOnlyTheNamedDocument(t *testing.T) { + for _, catalog := range []int{1, 500} { + t.Run("catalog"+strconv.Itoa(catalog), func(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount)) + seedByIDCatalog(t, store, catalog) + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + }, byIDAccount) + + require.NoError(t, err) + require.Len(t, out.Images, 1) + assert.Equal(t, byIDImage, aws.StringValue(out.Images[0].ImageId)) + assert.Empty(t, store.ListPrefixes(), "a named image must not list the AMI prefix") + assert.Equal(t, []string{amiKey(t, byIDImage)}, store.Gets(), + "a named image must read its own document and nothing else") + }) + } +} + +// Naming the same image twice is one read and one result: the enumerating path +// matched each document once however often the request named it. +func TestDescribeImagesByID_MultipleAndDuplicateIDs(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc("ami-a", byIDAccount)) + putByIDAMI(t, store, byIDAMIDoc("ami-b", byIDAccount)) + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{"ami-a", "ami-b", "ami-a"}), + }, byIDAccount) + + require.NoError(t, err) + require.Len(t, out.Images, 2) + assert.Equal(t, "ami-a", aws.StringValue(out.Images[0].ImageId)) + assert.Equal(t, "ami-b", aws.StringValue(out.Images[1].ImageId)) + assert.Len(t, store.Gets(), 2, "a duplicated ID must not be read twice") + assert.Empty(t, store.ListPrefixes()) +} + +// The AMI key carries no tenant scoping, so the fast path reads a document it +// must not return. Reporting anything other than not-found here discloses +// another account's private image to a caller that named its ID. +func TestDescribeImagesByID_HiddenAndMissingAreIndistinguishable(t *testing.T) { + tests := []struct { + name string + seedOwner string + seedImage bool + requestID string + wantHidden string + }{ + {name: "another account's private image", seedOwner: byIDOther, seedImage: true, requestID: byIDImage}, + {name: "an empty owner alias", seedOwner: "", seedImage: true, requestID: byIDImage}, + {name: "an image that does not exist", seedImage: false, requestID: "ami-never-registered"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + if tt.seedImage { + putByIDAMI(t, store, byIDAMIDoc(byIDImage, tt.seedOwner)) + } + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{tt.requestID}), + }, byIDAccount) + + require.Error(t, err) + assert.Nil(t, out) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error(), + "an image the caller cannot read must answer exactly as a nonexistent one") + }) + } +} + +// A system AMI's alias is not an account ID, so it resolves to the global +// account before any owner comparison and stays visible to every tenant. +func TestDescribeImagesByID_SystemAMIIsVisibleToEveryAccount(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc(byIDImage, "amazon")) + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + Owners: aws.StringSlice([]string{"amazon"}), + }, byIDAccount) + + require.NoError(t, err) + require.Len(t, out.Images, 1) + assert.Equal(t, utils.GlobalAccountID, aws.StringValue(out.Images[0].OwnerId)) + assert.Equal(t, "amazon", aws.StringValue(out.Images[0].ImageOwnerAlias)) +} + +// The Owners parameter is matched against the resolved owner, and a named image +// that no owner selects is not-found rather than an empty success. +func TestDescribeImagesByID_OwnerFilters(t *testing.T) { + tests := []struct { + name string + ownerAlias string + owners []string + wantErr string + }{ + {name: "self against an own image", ownerAlias: byIDAccount, owners: []string{"self"}}, + {name: "own numeric account", ownerAlias: byIDAccount, owners: []string{byIDAccount}}, + {name: "system aliases against a system image", ownerAlias: "amazon", + owners: []string{"system", "aws-marketplace"}}, + {name: "another account against an own image", ownerAlias: byIDAccount, owners: []string{byIDOther}, + wantErr: awserrors.ErrorInvalidAMIIDNotFound}, + {name: "self against a system image", ownerAlias: "amazon", owners: []string{"self"}, + wantErr: awserrors.ErrorInvalidAMIIDNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc(byIDImage, tt.ownerAlias)) + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + Owners: aws.StringSlice(tt.owners), + }, byIDAccount) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + return + } + require.NoError(t, err) + assert.Len(t, out.Images, 1) + }) + } +} + +// A filter applies to a directly-fetched document exactly as it did to a listed +// one, tags included, and a named image a filter excludes is not-found. +func TestDescribeImagesByID_AttributeFilters(t *testing.T) { + tests := []struct { + name string + filterName string + filterValue string + wantErr string + }{ + {name: "matching name", filterName: "name", filterValue: "debian-13"}, + {name: "matching state", filterName: "state", filterValue: "available"}, + {name: "matching tag", filterName: "tag:Name", filterValue: "base"}, + {name: "non-matching name", filterName: "name", filterValue: "ubuntu-24", + wantErr: awserrors.ErrorInvalidAMIIDNotFound}, + {name: "non-matching architecture", filterName: "architecture", filterValue: "arm64", + wantErr: awserrors.ErrorInvalidAMIIDNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount)) + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + Filters: []*ec2.Filter{{ + Name: aws.String(tt.filterName), + Values: aws.StringSlice([]string{tt.filterValue}), + }}, + }, byIDAccount) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + return + } + require.NoError(t, err) + assert.Len(t, out.Images, 1) + }) + } +} + +// The enumerating path skips a document it cannot decode so one bad AMI cannot +// hide every other image. A named image is never one of those others, so its +// unreadable document fails the call instead of reporting it as absent. +func TestDescribeImagesByID_CorruptDocumentFailsTheCall(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + _, err := store.PutObject(t.Context(), &s3.PutObjectInput{ + Bucket: aws.String(byIDBucket), + Key: aws.String(amiKey(t, byIDImage)), + Body: strings.NewReader("not-json"), + }) + require.NoError(t, err) + + _, err = svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + }, byIDAccount) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error(), + "a named image that cannot be decoded must not report as not-found") +} + +// getFailingStore fails the read of one key with an error that is not a missing +// key, standing in for a store that is reachable but unwell. +type getFailingStore struct { + *objectstore.MemoryObjectStore + + failKey string +} + +func (s *getFailingStore) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { + if aws.StringValue(input.Key) == s.failKey { + return nil, errors.New("connection reset by peer") + } + return s.MemoryObjectStore.GetObject(ctx, input) +} + +// A store failure is not absence: reporting not-found here would tell a caller +// its image is deregistered on the strength of a broken connection. +func TestDescribeImagesByID_StoreErrorIsInternal(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + store := &getFailingStore{MemoryObjectStore: memory, failKey: amiKey(t, byIDImage)} + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, memory, byIDAMIDoc(byIDImage, byIDAccount)) + + _, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + }, byIDAccount) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// A store failure anywhere in the fan-out outranks a missing ID, whatever order +// the request named them in. Answering not-found here would tell a caller its +// image was deregistered on the strength of a broken connection. +func TestDescribeImagesByID_StoreErrorOutranksAMissingID(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + store := &getFailingStore{MemoryObjectStore: memory, failKey: amiKey(t, byIDImage)} + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, memory, byIDAMIDoc(byIDImage, byIDAccount)) + + _, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{"ami-never-registered", byIDImage}), + }, byIDAccount) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// A caller that has already given up gets an error, never an empty result that +// reads as "this image does not exist". +func TestDescribeImagesByID_CancelledContextFails(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount)) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + out, err := svc.DescribeImages(ctx, &ec2.DescribeImagesInput{ + ImageIds: aws.StringSlice([]string{byIDImage}), + }, byIDAccount) + + require.Error(t, err) + assert.Nil(t, out) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// A request whose only IDs are nil names nothing, which is an empty result +// rather than a missing image, and reads nothing to find that out. +func TestDescribeImagesByID_OnlyNilIDs(t *testing.T) { + store := recordingstore.New() + svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket) + putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount)) + + out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ImageIds: []*string{nil}}, byIDAccount) + + require.NoError(t, err) + assert.Empty(t, out.Images) + assert.Empty(t, store.Gets()) + assert.Empty(t, store.ListPrefixes()) +} diff --git a/spinifex/handlers/ec2/image/service_impl.go b/spinifex/handlers/ec2/image/service_impl.go index f9b326e1a..17e3e7342 100644 --- a/spinifex/handlers/ec2/image/service_impl.go +++ b/spinifex/handlers/ec2/image/service_impl.go @@ -138,129 +138,29 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri return s.describeImages(ctx, input, accountID, parsedFilters) } -// describeImages enumerates AMIs via Store.ListAMIs, then filters and renders -// each document into the AWS SDK image shape. +// describeImages renders the AWS SDK image shape for every AMI the caller may +// see. Named IDs read their own documents; an unnamed request enumerates the +// whole catalog via Store.ListAMIs, which is proportional to its own answer. func (s *ImageServiceImpl) describeImages(ctx context.Context, input *ec2.DescribeImagesInput, accountID string, parsedFilters map[string][]string) (*ec2.DescribeImagesOutput, error) { + encryptedAtRest := s.clusterEncryptionEnabled() + + if len(input.ImageIds) > 0 { + return s.describeImagesByIDs(ctx, input, accountID, parsedFilters, encryptedAtRest) + } + amis, err := s.metadata.ListAMIs(ctx) if err != nil { slog.ErrorContext(ctx, "DescribeImages: failed to list AMIs", "err", err) return nil, errors.New(awserrors.ErrorServerInternal) } - encryptedAtRest := s.clusterEncryptionEnabled() var images []*ec2.Image - for _, amiMeta := range amis { if amiMeta.ImageID == "" { continue } - - if len(input.ImageIds) > 0 { - found := false - for _, filterID := range input.ImageIds { - if filterID != nil && *filterID == amiMeta.ImageID { - found = true - break - } - } - if !found { - continue - } - } - - amiOwner := amiMeta.ImageOwnerAlias - isSystemAMI := amiOwner != "" && !utils.IsAccountID(amiOwner) - - // Resolved up front so the owner filter below compares against the same - // value the OwnerId field reports, rather than the raw alias string a - // system AMI carries instead of a numeric account ID. - ownerID := amiOwner - if isSystemAMI { - ownerID = utils.GlobalAccountID - } - - if !callerCanReadAMI(amiMeta, accountID) { - continue - } - - if len(input.Owners) > 0 { - found := false - for _, owner := range input.Owners { - if owner == nil { - continue - } - switch *owner { - case "self": - if ownerID == accountID { - found = true - } - case "amazon", "system", "aws-marketplace": - if isSystemAMI { - found = true - } - default: - if ownerID == *owner { - found = true - } - } - if found { - break - } - } - if !found { - continue - } - } - - image := &ec2.Image{ - ImageId: aws.String(amiMeta.ImageID), - Name: aws.String(amiMeta.Name), - Description: aws.String(amiMeta.Description), - Architecture: aws.String(amiMeta.Architecture), - PlatformDetails: aws.String(amiMeta.PlatformDetails), - Platform: utils.PlatformFromDetails(amiMeta.PlatformDetails), - CreationDate: aws.String(amiMeta.CreationDate.UTC().Format("2006-01-02T15:04:05.000Z")), - RootDeviceType: aws.String(amiMeta.RootDeviceType), - VirtualizationType: aws.String(amiMeta.Virtualization), - ImageOwnerAlias: aws.String(amiMeta.ImageOwnerAlias), - OwnerId: aws.String(ownerID), - Public: aws.Bool(false), - State: aws.String(amiImageState(amiMeta.State)), - ImageType: aws.String("machine"), - Hypervisor: aws.String("xen"), - } - - // RegisterImage takes BootMode as an optional input, so an AMI can - // record none. The enum has no empty member: omit it, don't emit "". - if amiMeta.BootMode != "" { - image.BootMode = aws.String(amiMeta.BootMode) - } - - if bdms := synthesizeRootBlockDeviceMapping(amiMeta, encryptedAtRest); bdms != nil { - image.RootDeviceName = aws.String("/dev/sda1") - image.BlockDeviceMappings = bdms - } - - image.Tags = utils.MapToEC2Tags(amiMeta.Tags) - - if len(parsedFilters) > 0 && !imageMatchesFilters(image, parsedFilters, amiMeta.Tags) { - continue - } - - images = append(images, image) - } - - if len(input.ImageIds) > 0 { - foundIDs := make(map[string]bool, len(images)) - for _, img := range images { - if img.ImageId != nil { - foundIDs[*img.ImageId] = true - } - } - for _, reqID := range input.ImageIds { - if reqID != nil && !foundIDs[*reqID] { - return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) - } + if image := renderVisibleImage(amiMeta, input, accountID, parsedFilters, encryptedAtRest); image != nil { + images = append(images, image) } } @@ -268,6 +168,164 @@ func (s *ImageServiceImpl) describeImages(ctx context.Context, input *ec2.Descri return &ec2.DescribeImagesOutput{Images: images}, nil } +// describeImagesByIDs reads each named AMI's document directly. Enumerating +// instead would scale with the unpartitioned AMI prefix, so with every other +// tenant's image count. +// +// The key carries no tenant scoping, so every visibility rule is applied after +// the fetch, in the order the enumerating path applies them. +func (s *ImageServiceImpl) describeImagesByIDs(ctx context.Context, input *ec2.DescribeImagesInput, accountID string, + parsedFilters map[string][]string, encryptedAtRest bool) (*ec2.DescribeImagesOutput, error) { + // Only nil entries names nothing, which the enumerating path answers with an + // empty result rather than a missing ID. + imageIDs := utils.DistinctIDs(input.ImageIds) + if len(imageIDs) == 0 { + return &ec2.DescribeImagesOutput{}, nil + } + + for _, imageID := range imageIDs { + // An ID no key can be built for names no document, which is also what + // the enumerating path reports for it. + if _, err := ebsmetadata.AMIKey(imageID); err != nil { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + } + + results := ebsmetadata.FetchByIDs(ctx, imageIDs, s.metadata.GetAMI) + + // A read that failed once the deadline passed is not evidence the AMI is + // absent, which is the only other thing this path reports. + if err := ctx.Err(); err != nil { + slog.WarnContext(ctx, "DescribeImages gave up reading AMI documents", "requested", len(imageIDs), "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + // A failed read outranks a missing one however the request ordered them, so + // a broken store is never reported as a deregistered image, and every + // failure is logged rather than only the one that happens to come first. + var readErr error + for i, result := range results { + if result.Err != nil && !objectstore.IsNoSuchKeyError(result.Err) { + slog.ErrorContext(ctx, "DescribeImages: failed to read AMI document", "imageId", imageIDs[i], "err", result.Err) + readErr = result.Err + } + } + if readErr != nil { + return nil, errors.New(awserrors.ErrorServerInternal) + } + + images := make([]*ec2.Image, 0, len(imageIDs)) + for i, result := range results { + // Every other store error was classified above, so this is a missing key. + if result.Err != nil { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + + // The document is keyed by its own ID, and the enumerating path matches + // on this field. The comparison stays as an assertion. + if result.Document.ImageID != imageIDs[i] { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + + // A named ID no rule lets through is not-found, never an empty success. + image := renderVisibleImage(result.Document, input, accountID, parsedFilters, encryptedAtRest) + if image == nil { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + images = append(images, image) + } + + slog.InfoContext(ctx, "DescribeImages completed", "count", len(images)) + return &ec2.DescribeImagesOutput{Images: images}, nil +} + +// renderVisibleImage applies the DescribeImages visibility rules to one AMI +// document and renders the SDK shape, or reports nil for a document the caller +// must not see, an owner that does not match, or a filter that excludes it. +func renderVisibleImage(amiMeta ebsmetadata.AMI, input *ec2.DescribeImagesInput, accountID string, + parsedFilters map[string][]string, encryptedAtRest bool) *ec2.Image { + amiOwner := amiMeta.ImageOwnerAlias + isSystemAMI := amiOwner != "" && !utils.IsAccountID(amiOwner) + + // Resolved up front so the owner filter below compares against the same + // value the OwnerId field reports, rather than the raw alias string a + // system AMI carries instead of a numeric account ID. + ownerID := amiOwner + if isSystemAMI { + ownerID = utils.GlobalAccountID + } + + if !callerCanReadAMI(amiMeta, accountID) { + return nil + } + + if len(input.Owners) > 0 { + found := false + for _, owner := range input.Owners { + if owner == nil { + continue + } + switch *owner { + case "self": + if ownerID == accountID { + found = true + } + case "amazon", "system", "aws-marketplace": + if isSystemAMI { + found = true + } + default: + if ownerID == *owner { + found = true + } + } + if found { + break + } + } + if !found { + return nil + } + } + + image := &ec2.Image{ + ImageId: aws.String(amiMeta.ImageID), + Name: aws.String(amiMeta.Name), + Description: aws.String(amiMeta.Description), + Architecture: aws.String(amiMeta.Architecture), + PlatformDetails: aws.String(amiMeta.PlatformDetails), + Platform: utils.PlatformFromDetails(amiMeta.PlatformDetails), + CreationDate: aws.String(amiMeta.CreationDate.UTC().Format("2006-01-02T15:04:05.000Z")), + RootDeviceType: aws.String(amiMeta.RootDeviceType), + VirtualizationType: aws.String(amiMeta.Virtualization), + ImageOwnerAlias: aws.String(amiMeta.ImageOwnerAlias), + OwnerId: aws.String(ownerID), + Public: aws.Bool(false), + State: aws.String(amiImageState(amiMeta.State)), + ImageType: aws.String("machine"), + Hypervisor: aws.String("xen"), + } + + // RegisterImage takes BootMode as an optional input, so an AMI can + // record none. The enum has no empty member: omit it, don't emit "". + if amiMeta.BootMode != "" { + image.BootMode = aws.String(amiMeta.BootMode) + } + + if bdms := synthesizeRootBlockDeviceMapping(amiMeta, encryptedAtRest); bdms != nil { + image.RootDeviceName = aws.String("/dev/sda1") + image.BlockDeviceMappings = bdms + } + + image.Tags = utils.MapToEC2Tags(amiMeta.Tags) + + if len(parsedFilters) > 0 && !imageMatchesFilters(image, parsedFilters, amiMeta.Tags) { + return nil + } + + return image +} + // amiImageState maps AMIMetadata.State to the ec2.Image state string. An // empty State means the AMI was registered before the field existed, and // MUST report as "available" — those images are already complete and diff --git a/spinifex/handlers/ec2/snapshot/describe_byid_test.go b/spinifex/handlers/ec2/snapshot/describe_byid_test.go new file mode 100644 index 000000000..99192d349 --- /dev/null +++ b/spinifex/handlers/ec2/snapshot/describe_byid_test.go @@ -0,0 +1,349 @@ +package handlers_ec2_snapshot_test + +import ( + "context" + "errors" + "strconv" + "strings" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/config" + "github.com/mulgadc/spinifex/spinifex/ebsmetadata" + handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot" + "github.com/mulgadc/spinifex/spinifex/objectstore" + "github.com/mulgadc/spinifex/spinifex/testutil/recordingstore" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + byIDBucket = "test-bucket" + byIDAccount = "111122223333" + byIDOther = "444455556666" + byIDSnapshot = "snap-target" + byIDSnapshots = "spinifex/ebsmetadata/v2/snapshots/" +) + +// newByIDService builds a snapshot service over store, with no EBS provider: +// every test here answers out of the metadata documents alone. +func newByIDService(store objectstore.ObjectStore) *handlers_ec2_snapshot.SnapshotServiceImpl { + cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: byIDBucket}} + return handlers_ec2_snapshot.NewSnapshotServiceImplWithStore(cfg, store, nil) +} + +func putByIDSnapshot(t *testing.T, store objectstore.ObjectStore, snapshot ebsmetadata.Snapshot) { + t.Helper() + require.NoError(t, ebsmetadata.NewStore(store, byIDBucket).PutSnapshot(context.Background(), snapshot)) +} + +// byIDSnapshotDoc is a complete, readable snapshot document owned by ownerID. +func byIDSnapshotDoc(snapshotID, ownerID string) ebsmetadata.Snapshot { + return ebsmetadata.Snapshot{ + SnapshotID: snapshotID, VolumeID: "vol-source", VolumeSize: 8, State: "completed", + Progress: "100%", OwnerID: ownerID, StartTime: time.Now().UTC(), + Tags: map[string]string{"Name": "nightly"}, + } +} + +// seedByIDCatalog grows one account's catalog with documents no request names, +// so a by-ID describe's cost can be measured against a catalog it must ignore. +func seedByIDCatalog(t *testing.T, store objectstore.ObjectStore, ownerID string, n int) { + t.Helper() + for i := range n { + putByIDSnapshot(t, store, byIDSnapshotDoc("snap-bulk"+strconv.Itoa(i), ownerID)) + } +} + +func snapshotKey(t *testing.T, accountID, snapshotID string) string { + t.Helper() + key, err := ebsmetadata.SnapshotKey(accountID, snapshotID) + require.NoError(t, err) + return key +} + +// A named snapshot costs one document read and no listing, whatever the account +// owns. The cost of the old path was the whole catalog, so the count is the +// assertion, not the answer. +func TestDescribeSnapshotsByID_ReadsOnlyTheNamedDocument(t *testing.T) { + for _, catalog := range []int{1, 500} { + t.Run("catalog"+strconv.Itoa(catalog), func(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + seedByIDCatalog(t, store, byIDAccount, catalog) + + out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{byIDSnapshot}), + }, byIDAccount) + + require.NoError(t, err) + require.Len(t, out.Snapshots, 1) + assert.Equal(t, byIDSnapshot, aws.StringValue(out.Snapshots[0].SnapshotId)) + assert.Empty(t, store.ListPrefixes(), "a named snapshot must not list a prefix") + assert.Equal(t, []string{snapshotKey(t, byIDAccount, byIDSnapshot)}, store.Gets(), + "a named snapshot must read its own document and nothing else") + }) + } +} + +// Naming the same snapshot twice is one read and one result: the listing path +// matched each document once however often the request named it. +func TestDescribeSnapshotsByID_MultipleAndDuplicateIDs(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc("snap-a", byIDAccount)) + putByIDSnapshot(t, store, byIDSnapshotDoc("snap-b", byIDAccount)) + + out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{"snap-a", "snap-b", "snap-a"}), + }, byIDAccount) + + require.NoError(t, err) + require.Len(t, out.Snapshots, 2) + assert.Equal(t, "snap-a", aws.StringValue(out.Snapshots[0].SnapshotId)) + assert.Equal(t, "snap-b", aws.StringValue(out.Snapshots[1].SnapshotId)) + assert.Len(t, store.Gets(), 2, "a duplicated ID must not be read twice") + assert.Empty(t, store.ListPrefixes()) +} + +// Another account's snapshot and a snapshot that was never created are the same +// answer, and neither reads outside the caller's prefix. +func TestDescribeSnapshotsByID_CrossTenantAndMissingAreIndistinguishable(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc("snap-theirs", byIDOther)) + + crossTenant, crossErr := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{"snap-theirs"}), + }, byIDAccount) + missing, missingErr := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{"snap-never-created"}), + }, byIDAccount) + + require.Error(t, crossErr) + require.Error(t, missingErr) + assert.Nil(t, crossTenant) + assert.Nil(t, missing) + assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, crossErr.Error()) + assert.Equal(t, missingErr.Error(), crossErr.Error(), + "another account's snapshot must be indistinguishable from one that does not exist") + for _, key := range store.Gets() { + assert.True(t, strings.HasPrefix(key, byIDSnapshots+byIDAccount+"/"), + "a by-ID read must stay inside the caller's prefix, read %q", key) + } +} + +// OwnerIds selecting only another account names snapshots the caller's prefix +// cannot hold, so the named ID is absent without any read at all. +func TestDescribeSnapshotsByID_OwnerIDsFilter(t *testing.T) { + tests := []struct { + name string + ownerIDs []string + wantErr string + wantGets int + }{ + {name: "self", ownerIDs: []string{"self"}, wantGets: 1}, + {name: "own numeric account", ownerIDs: []string{byIDAccount}, wantGets: 1}, + {name: "another account", ownerIDs: []string{byIDOther}, + wantErr: awserrors.ErrorInvalidSnapshotNotFound, wantGets: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + + out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{byIDSnapshot}), + OwnerIds: aws.StringSlice(tt.ownerIDs), + }, byIDAccount) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + } else { + require.NoError(t, err) + assert.Len(t, out.Snapshots, 1) + } + assert.Len(t, store.Gets(), tt.wantGets) + }) + } +} + +// A filter applies to a directly-fetched document exactly as it did to a listed +// one, and a named snapshot a filter excludes is not-found rather than an empty +// success. +func TestDescribeSnapshotsByID_AttributeFilters(t *testing.T) { + tests := []struct { + name string + filterName string + filterValue string + wantErr string + }{ + {name: "matching status", filterName: "status", filterValue: "completed"}, + {name: "matching tag", filterName: "tag:Name", filterValue: "nightly"}, + {name: "non-matching status", filterName: "status", filterValue: "pending", + wantErr: awserrors.ErrorInvalidSnapshotNotFound}, + {name: "non-matching volume", filterName: "volume-id", filterValue: "vol-other", + wantErr: awserrors.ErrorInvalidSnapshotNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + + out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{byIDSnapshot}), + Filters: []*ec2.Filter{{ + Name: aws.String(tt.filterName), + Values: aws.StringSlice([]string{tt.filterValue}), + }}, + }, byIDAccount) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + return + } + require.NoError(t, err) + assert.Len(t, out.Snapshots, 1) + }) + } +} + +// The tolerant variant's skip exists to stop one bad document hiding unrelated +// snapshots. A specifically-named snapshot is never unrelated, so an unreadable +// document for it fails both variants rather than reporting it as absent. +func TestDescribeSnapshotsByID_CorruptDocumentFailsBothVariants(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + _, err := store.PutObject(t.Context(), &s3.PutObjectInput{ + Bucket: aws.String(byIDBucket), + Key: aws.String(snapshotKey(t, byIDAccount, byIDSnapshot)), + Body: strings.NewReader("not-json"), + }) + require.NoError(t, err) + input := &ec2.DescribeSnapshotsInput{SnapshotIds: aws.StringSlice([]string{byIDSnapshot})} + + _, tolerantErr := svc.DescribeSnapshots(t.Context(), input, byIDAccount) + _, strictErr := svc.DescribeSnapshotsStrict(t.Context(), input, byIDAccount) + + require.Error(t, tolerantErr) + require.Error(t, strictErr) + assert.Equal(t, awserrors.ErrorServerInternal, tolerantErr.Error(), + "a named snapshot that cannot be decoded must not report as not-found") + assert.ErrorIs(t, strictErr, ebsmetadata.ErrCorruptDocument, + "the strict variant must surface the cause on the fast path as it does on the listing path") +} + +// A store failure anywhere in the fan-out outranks a missing ID, whatever order +// the request named them in. Answering not-found here would tell a caller its +// snapshot was deleted on the strength of a broken connection. +func TestDescribeSnapshotsByID_StoreErrorOutranksAMissingID(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + store := &getFailingStore{MemoryObjectStore: memory, failKey: snapshotKey(t, byIDAccount, byIDSnapshot)} + svc := newByIDService(store) + putByIDSnapshot(t, memory, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + + input := &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{"snap-never-created", byIDSnapshot}), + } + _, tolerantErr := svc.DescribeSnapshots(t.Context(), input, byIDAccount) + _, strictErr := svc.DescribeSnapshotsStrict(t.Context(), input, byIDAccount) + + require.Error(t, tolerantErr) + require.Error(t, strictErr) + assert.Equal(t, awserrors.ErrorServerInternal, tolerantErr.Error()) + assert.Contains(t, strictErr.Error(), "connection reset by peer") +} + +// The strict variant reports an untenanted caller as the listing path does, +// rather than flattening the reason the read could not be attempted. +func TestDescribeSnapshotsByID_UntenantedCallerIsInternalInBothVariants(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + input := &ec2.DescribeSnapshotsInput{SnapshotIds: aws.StringSlice([]string{byIDSnapshot})} + + _, tolerantErr := svc.DescribeSnapshots(t.Context(), input, "") + _, strictErr := svc.DescribeSnapshotsStrict(t.Context(), input, "") + + require.Error(t, tolerantErr) + require.Error(t, strictErr) + assert.Equal(t, awserrors.ErrorServerInternal, tolerantErr.Error()) + assert.Contains(t, strictErr.Error(), "invalid EBS metadata account ID") + assert.Empty(t, store.Gets(), "an untenanted caller has no prefix to read under") +} + +// getFailingStore fails the read of one key with an error that is not a missing +// key, standing in for a store that is reachable but unwell. +type getFailingStore struct { + *objectstore.MemoryObjectStore + + failKey string +} + +func (s *getFailingStore) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { + if aws.StringValue(input.Key) == s.failKey { + return nil, errors.New("connection reset by peer") + } + return s.MemoryObjectStore.GetObject(ctx, input) +} + +// A store failure is not absence: reporting not-found here would tell a caller +// its snapshot is gone on the strength of a broken connection. +func TestDescribeSnapshotsByID_StoreErrorIsInternal(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + store := &getFailingStore{MemoryObjectStore: memory, failKey: snapshotKey(t, byIDAccount, byIDSnapshot)} + svc := newByIDService(store) + putByIDSnapshot(t, memory, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + + _, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{byIDSnapshot}), + }, byIDAccount) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// A caller that has already given up gets an error, never an empty result that +// reads as "this snapshot does not exist". +func TestDescribeSnapshotsByID_CancelledContextFails(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + out, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{ + SnapshotIds: aws.StringSlice([]string{byIDSnapshot}), + }, byIDAccount) + + require.Error(t, err) + assert.Nil(t, out) + assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) +} + +// A request whose only IDs are nil names nothing, which is an empty result +// rather than a missing snapshot, and reads nothing to find that out. +func TestDescribeSnapshotsByID_OnlyNilIDs(t *testing.T) { + store := recordingstore.New() + svc := newByIDService(store) + putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount)) + + out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: []*string{nil}, + }, byIDAccount) + + require.NoError(t, err) + assert.Empty(t, out.Snapshots) + assert.Empty(t, store.Gets()) + assert.Empty(t, store.ListPrefixes()) +} diff --git a/spinifex/handlers/ec2/snapshot/service_impl.go b/spinifex/handlers/ec2/snapshot/service_impl.go index c3bd9cfbf..c5ab5134b 100644 --- a/spinifex/handlers/ec2/snapshot/service_impl.go +++ b/spinifex/handlers/ec2/snapshot/service_impl.go @@ -451,27 +451,19 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2. slog.InfoContext(ctx, "DescribeSnapshots request", "snapshotIds", input.SnapshotIds, "accountID", accountID) - snapshotIDFilter := make(map[string]bool) - for _, id := range input.SnapshotIds { - if id != nil { - snapshotIDFilter[*id] = true - } - } - parsedFilters, err := filterutil.ParseFilters(input.Filters, describeSnapshotsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeSnapshots: invalid filter", "err", err) return nil, err } + if len(input.SnapshotIds) > 0 { + return s.describeSnapshotsByIDs(ctx, input, accountID, parsedFilters, strict) + } + // The caller's prefix is the isolation and the whole answer: no other - // account's document is read, so none has to be filtered out afterwards. - // Named ids are matched against this listing rather than fetched directly — - // the listing already costs what the account owns. - // - // OwnerIds names either the caller, which the prefix already is, or another - // account, whose snapshots cannot be under it — so that case is empty without - // a read, and the tail below still reports a named id as not found. + // account's document is read, and OwnerIds either names the caller, whose + // prefix this is, or an account whose snapshots cannot be under it. var configs []ebsmetadata.Snapshot if ownerIDsIncludeCaller(input.OwnerIds, accountID) { if strict { @@ -499,10 +491,6 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2. var snapshots []*ec2.Snapshot for _, cfg := range configs { - if len(snapshotIDFilter) > 0 && !snapshotIDFilter[cfg.SnapshotID] { - continue - } - if len(parsedFilters) > 0 && !snapshotMatchesFilters(cfg, parsedFilters) { continue } @@ -510,22 +498,108 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2. snapshots = append(snapshots, snapshotConfigToEC2(cfg)) } - // Naming a specific, nonexistent snapshot ID is an error, unlike an - // unfiltered list or a --filters query that simply matches nothing. - if len(snapshotIDFilter) > 0 { - found := make(map[string]bool, len(snapshots)) - for _, snap := range snapshots { - if snap.SnapshotId != nil { - found[*snap.SnapshotId] = true - } + slog.InfoContext(ctx, "DescribeSnapshots completed", "count", len(snapshots)) + + return &ec2.DescribeSnapshotsOutput{ + Snapshots: snapshots, + }, nil +} + +// describeSnapshotsByIDs reads each named snapshot's document directly, rather +// than pay a fetch per snapshot the account owns however few it names. +// +// The key is scoped to the caller's account, so a cross-tenant ID reads a key +// that does not exist and reports not-found — the same answer the listing path +// gives, and for the same reason. +// +// strict keeps the control-plane contract the listing path has: the underlying +// metadata failure is returned rather than flattened to an opaque code. +func (s *SnapshotServiceImpl) describeSnapshotsByIDs(ctx context.Context, input *ec2.DescribeSnapshotsInput, + accountID string, parsedFilters map[string][]string, strict bool) (*ec2.DescribeSnapshotsOutput, error) { + // Only nil entries names nothing, which the listing path answers with an + // empty result rather than a missing ID. + snapshotIDs := utils.DistinctIDs(input.SnapshotIds) + if len(snapshotIDs) == 0 { + return &ec2.DescribeSnapshotsOutput{}, nil + } + + // OwnerIds naming only another account selects snapshots the caller's + // prefix cannot contain, so a named ID is absent without any read. + if !ownerIDsIncludeCaller(input.OwnerIds, accountID) { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) + } + + // An untenanted caller has no prefix to read under; refuse here rather than + // report a snapshot that may well exist as absent. The key builder owns the + // wording, so the strict variant reports what the listing path reports. + if !utils.IsAccountID(accountID) { + _, keyErr := ebsmetadata.SnapshotKey(accountID, snapshotIDs[0]) + slog.ErrorContext(ctx, "DescribeSnapshots refused a caller with no account", "accountID", accountID) + if strict { + return nil, fmt.Errorf("describe snapshots for %s: %w", accountID, keyErr) } - for id := range snapshotIDFilter { - if !found[id] { - return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) - } + return nil, errors.New(awserrors.ErrorServerInternal) + } + + for _, snapshotID := range snapshotIDs { + // An ID no key can be built for names no document, which is also what + // the listing path reports for it. + if _, err := ebsmetadata.SnapshotKey(accountID, snapshotID); err != nil { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) } } + results := ebsmetadata.FetchByIDs(ctx, snapshotIDs, func(ctx context.Context, id string) (ebsmetadata.Snapshot, error) { + return s.metadata.GetSnapshot(ctx, accountID, id) + }) + + // A read that failed once the deadline passed is not evidence the snapshot + // is absent, which is the only other thing this path reports. + if err := ctx.Err(); err != nil { + slog.WarnContext(ctx, "DescribeSnapshots gave up reading snapshot documents", + "requested", len(snapshotIDs), "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + // A named ID is never the unrelated document the tolerant listing's skip + // exists for, so an unreadable one fails both variants. A failed read also + // outranks a missing one however the request ordered them. + var readErr error + for i, result := range results { + if result.Err != nil && !objectstore.IsNoSuchKeyError(result.Err) { + slog.ErrorContext(ctx, "DescribeSnapshots failed to read a named snapshot document", + "snapshotId", snapshotIDs[i], "accountID", accountID, "err", result.Err) + readErr = result.Err + } + } + if readErr != nil { + if strict { + return nil, fmt.Errorf("describe snapshots for %s: %w", accountID, readErr) + } + return nil, errors.New(awserrors.ErrorServerInternal) + } + + snapshots := make([]*ec2.Snapshot, 0, len(snapshotIDs)) + for i, result := range results { + // Every other store error was classified above, so this is a missing key. + if result.Err != nil { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) + } + + // The read was keyed by the ID inside the caller's prefix, so the + // document is theirs and is the one named. Both stay as assertions. + if result.Document.SnapshotID != snapshotIDs[i] || result.Document.OwnerID != accountID { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) + } + + // A named snapshot a filter excludes is not-found, as on the listing path. + if len(parsedFilters) > 0 && !snapshotMatchesFilters(result.Document, parsedFilters) { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) + } + + snapshots = append(snapshots, snapshotConfigToEC2(result.Document)) + } + slog.InfoContext(ctx, "DescribeSnapshots completed", "count", len(snapshots)) return &ec2.DescribeSnapshotsOutput{ diff --git a/spinifex/utils/utils.go b/spinifex/utils/utils.go index a01f64a9d..ccd2362ef 100644 --- a/spinifex/utils/utils.go +++ b/spinifex/utils/utils.go @@ -40,6 +40,22 @@ func GenerateResourceID(prefix string) string { return prefix + "-" + hex.EncodeToString(b)[:17] } +// DistinctIDs returns the distinct non-nil values of a request's resource-ID +// slice, in request order. A describe that reads one document per named ID must +// not read or report the same resource twice because the request listed it twice. +func DistinctIDs(ids []*string) []string { + distinct := make([]string, 0, len(ids)) + seen := make(map[string]bool, len(ids)) + for _, id := range ids { + if id == nil || seen[*id] { + continue + } + seen[*id] = true + distinct = append(distinct, *id) + } + return distinct +} + func MarshalToXML(payload any) ([]byte, error) { var buf bytes.Buffer enc := xml.NewEncoder(&buf) diff --git a/spinifex/utils/utils_test.go b/spinifex/utils/utils_test.go index 9ffe786cf..330fb9086 100644 --- a/spinifex/utils/utils_test.go +++ b/spinifex/utils/utils_test.go @@ -26,6 +26,30 @@ import ( "github.com/stretchr/testify/require" ) +// A describe that reads one document per named ID pays for every entry it is +// given, so a repeated or nil ID has to be dropped before the fan-out and the +// surviving order has to be the request's. +func TestDistinctIDs(t *testing.T) { + first, second := "vol-1", "vol-2" + tests := []struct { + name string + ids []*string + want []string + }{ + {name: "nil slice", ids: nil, want: []string{}}, + {name: "only nil entries", ids: []*string{nil, nil}, want: []string{}}, + {name: "request order", ids: []*string{&second, &first}, want: []string{"vol-2", "vol-1"}}, + {name: "duplicates", ids: []*string{&first, &second, &first}, want: []string{"vol-1", "vol-2"}}, + {name: "nil among values", ids: []*string{&first, nil, &second}, want: []string{"vol-1", "vol-2"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, DistinctIDs(tt.ids)) + }) + } +} + func TestGenerateResourceID(t *testing.T) { tests := []struct { prefix string