feat: partition EBS metadata keys by owning account (#989)

This commit is contained in:
Julian Sommer
2026-09-03 15:38:23 +10:00
committed by GitHub
parent de44fa3264
commit d8d3e2268d
56 changed files with 2314 additions and 1035 deletions
+2 -3
View File
@@ -35,7 +35,6 @@ import (
"github.com/mulgadc/spinifex/spinifex/ebsprovider"
"github.com/mulgadc/spinifex/spinifex/formation"
"github.com/mulgadc/spinifex/spinifex/gpu"
handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc"
handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam"
"github.com/mulgadc/spinifex/spinifex/hostdns"
@@ -733,7 +732,7 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
// DescribeSnapshots call cannot resolve it. The volume ID matches ami.ImageID:
// admin.ImportImage creates the volume under the AMI's own ID.
func registerImportedAMISnapshot(store objectstore.ObjectStore, bucket string, ami ebsmetadata.AMI, az string, encrypted bool) error {
cfg := &handlers_ec2_snapshot.SnapshotConfig{
cfg := ebsmetadata.Snapshot{
SnapshotID: ami.SnapshotID,
VolumeID: ami.ImageID,
VolumeSize: safecast.Uint64ToInt64(ami.VolumeSizeGiB),
@@ -745,7 +744,7 @@ func registerImportedAMISnapshot(store objectstore.ObjectStore, bucket string, a
OwnerID: utils.GlobalAccountID,
AvailabilityZone: az,
}
if err := handlers_ec2_snapshot.WriteSnapshotConfig(store, bucket, ami.SnapshotID, cfg); err != nil {
if err := ebsmetadata.NewStore(store, bucket).PutSnapshot(context.Background(), cfg); err != nil {
return fmt.Errorf("register snapshot metadata: %w", err)
}
return nil
+9 -8
View File
@@ -20,10 +20,10 @@ import (
"github.com/mulgadc/bluebottle/pkg/safecast"
"github.com/mulgadc/spinifex/spinifex/admin"
"github.com/mulgadc/spinifex/spinifex/config"
"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
"github.com/mulgadc/spinifex/spinifex/ebsprovider"
gateway_bedrock "github.com/mulgadc/spinifex/spinifex/gateway/bedrock"
"github.com/mulgadc/spinifex/spinifex/gateway/bedrock/hfhub"
handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
"github.com/mulgadc/spinifex/spinifex/objectstore"
"github.com/mulgadc/spinifex/spinifex/utils"
"github.com/nats-io/nats.go"
@@ -633,7 +633,7 @@ func buildWeightsImage(srcDir, imagePath string, contentBytes int64) error {
// metadata.json sitting alongside them, so without this the endpoint launcher
// cannot see the snapshot at all and fails with InvalidSnapshot.NotFound.
func registerWeightsSnapshot(store objectstore.ObjectStore, bucket, snapshotID, volumeID string, volumeSize uint64, az string, encrypted bool) error {
cfg := &handlers_ec2_snapshot.SnapshotConfig{
cfg := ebsmetadata.Snapshot{
SnapshotID: snapshotID,
VolumeID: volumeID,
// GiB, not bytes: CreateVolume compares this against a requested Size
@@ -647,7 +647,7 @@ func registerWeightsSnapshot(store objectstore.ObjectStore, bucket, snapshotID,
OwnerID: utils.GlobalAccountID,
AvailabilityZone: az,
}
if err := handlers_ec2_snapshot.WriteSnapshotConfig(store, bucket, snapshotID, cfg); err != nil {
if err := ebsmetadata.NewStore(store, bucket).PutSnapshot(context.Background(), cfg); err != nil {
return fmt.Errorf("register snapshot metadata: %w", err)
}
return nil
@@ -805,13 +805,14 @@ func materializeWeightsVolume(ctx context.Context, provider ebsprovider.EBSProvi
}
// newWeightsSnapshotChecker is the real weightsSnapshotChecker: it reads the
// EC2 control plane's metadata.json for snapshotID from the node's
// predastore bucket -- the same record registerWeightsSnapshot writes at
// stage time -- and treats a missing object as gone. Any other read failure
// is a real error, not a signal to guess the snapshot's existence.
// EC2 control plane's snapshot document for snapshotID under the global
// account -- the same record registerWeightsSnapshot writes at stage time --
// and treats a missing object as gone. Any other read failure is a real
// error, not a signal to guess the snapshot's existence.
func newWeightsSnapshotChecker(store objectstore.ObjectStore, bucket string) weightsSnapshotChecker {
metadata := ebsmetadata.NewStore(store, bucket)
return func(ctx context.Context, snapshotID string) (bool, error) {
if _, err := handlers_ec2_snapshot.ReadSnapshotConfig(ctx, store, bucket, snapshotID); err != nil {
if _, err := metadata.GetSnapshot(ctx, utils.GlobalAccountID, snapshotID); err != nil {
if objectstore.IsNoSuchKeyError(err) {
return false, nil
}
+4 -3
View File
@@ -15,10 +15,11 @@ import (
"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/ebsmetadata"
gateway_bedrock "github.com/mulgadc/spinifex/spinifex/gateway/bedrock"
handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
"github.com/mulgadc/spinifex/spinifex/objectstore"
"github.com/mulgadc/spinifex/spinifex/testutil"
"github.com/mulgadc/spinifex/spinifex/utils"
"github.com/nats-io/nats.go"
"github.com/spf13/cobra"
"github.com/stretchr/testify/assert"
@@ -1094,7 +1095,7 @@ func TestRegisterWeightsSnapshot_WritesEC2ReadableMetadata(t *testing.T) {
require.NoError(t, registerWeightsSnapshot(store, bucket,
"snap-vol-abc", "vol-abc", 12*bytesPerGiB, "ap-southeast-2a", true))
cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, bucket, "snap-vol-abc")
cfg, err := ebsmetadata.NewStore(store, bucket).GetSnapshot(context.Background(), utils.GlobalAccountID, "snap-vol-abc")
require.NoError(t, err)
assert.Equal(t, "snap-vol-abc", cfg.SnapshotID)
assert.Equal(t, "vol-abc", cfg.VolumeID)
@@ -1111,7 +1112,7 @@ func TestRegisterWeightsSnapshot_UnencryptedVolume(t *testing.T) {
require.NoError(t, registerWeightsSnapshot(store, "predastore",
"snap-vol-plain", "vol-plain", bytesPerGiB, "ap-southeast-2a", false))
cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, "predastore", "snap-vol-plain")
cfg, err := ebsmetadata.NewStore(store, "predastore").GetSnapshot(context.Background(), utils.GlobalAccountID, "snap-vol-plain")
require.NoError(t, err)
assert.False(t, cfg.Encrypted)
}
@@ -5,7 +5,6 @@ import (
"testing"
"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/utils"
"github.com/stretchr/testify/assert"
@@ -29,7 +28,7 @@ func TestRegisterImportedAMISnapshot_WritesEC2ReadableMetadata(t *testing.T) {
require.NoError(t, registerImportedAMISnapshot(store, bucket, ami, "ap-southeast-2a", true))
cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, bucket, "snap-ami-import01")
cfg, err := ebsmetadata.NewStore(store, bucket).GetSnapshot(context.Background(), utils.GlobalAccountID, "snap-ami-import01")
require.NoError(t, err)
assert.Equal(t, "snap-ami-import01", cfg.SnapshotID)
assert.Equal(t, "ami-import01", cfg.VolumeID, "the import path creates the volume under the AMI's own ID")
@@ -50,7 +49,7 @@ func TestRegisterImportedAMISnapshot_UnencryptedVolume(t *testing.T) {
require.NoError(t, registerImportedAMISnapshot(store, "predastore", ami, "ap-southeast-2a", false))
cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, "predastore", "snap-ami-plain01")
cfg, err := ebsmetadata.NewStore(store, "predastore").GetSnapshot(context.Background(), utils.GlobalAccountID, "snap-ami-plain01")
require.NoError(t, err)
assert.False(t, cfg.Encrypted)
}
+59 -5
View File
@@ -32,13 +32,15 @@ type PromoteImageResult struct {
}
// PromoteSystemImage promotes an account-owned AMI to a system image by
// rewriting its ImageOwnerAlias to SystemOwnerAlias. After the call the AMI
// is immediately visible to all accounts via DescribeImages.
// rewriting its ImageOwnerAlias to SystemOwnerAlias and re-keying its snapshot
// document under the global account. After the call the AMI is immediately
// visible to all accounts via DescribeImages.
//
// Guards:
// - ImageID must have "ami-" prefix
// - config.json must exist and parse cleanly
// - AMI must currently be account-owned; already-system AMIs are rejected
// - the snapshot document must be readable, so it can be re-keyed
func PromoteSystemImage(store objectstore.ObjectStore, bucket string, opts PromoteImageOpts) (*PromoteImageResult, error) {
if !strings.HasPrefix(opts.ImageID, "ami-") {
return nil, errors.New(awserrors.ErrorInvalidAMIIDMalformed)
@@ -62,17 +64,69 @@ func PromoteSystemImage(store objectstore.ObjectStore, bucket string, opts Promo
}
prev := meta.ImageOwnerAlias
meta.ImageOwnerAlias = SystemOwnerAlias
metaStore := ebsmetadata.NewStore(store, bucket)
if err := ebsmetadata.NewStore(store, bucket).PutAMI(context.Background(), meta); err != nil {
// The snapshot document has to move before the alias is rewritten: readers
// derive its account from the alias, so a system alias over a tenant-keyed
// snapshot resolves to nothing.
snap, moveSnapshot, err := readPromotedSnapshot(metaStore, prev, meta.SnapshotID)
switch {
case err == nil:
// ok
case errors.Is(err, ebsmetadata.ErrCorruptDocument):
return nil, fmt.Errorf("%s references snapshot %s, whose document is corrupt and cannot be re-keyed; promotion not allowed",
opts.ImageID, meta.SnapshotID)
default:
slog.Error("PromoteSystemImage: read snapshot document", "imageId", opts.ImageID, "snapshotId", meta.SnapshotID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
if moveSnapshot {
snap.OwnerID = utils.GlobalAccountID
if err := metaStore.PutSnapshot(context.Background(), snap); err != nil {
slog.Error("PromoteSystemImage: write snapshot document under the global account",
"imageId", opts.ImageID, "snapshotId", meta.SnapshotID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
}
meta.ImageOwnerAlias = SystemOwnerAlias
if err := metaStore.PutAMI(context.Background(), meta); err != nil {
slog.Error("PromoteSystemImage: write AMI document", "imageId", opts.ImageID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
slog.Info("PromoteSystemImage completed", "imageId", opts.ImageID, "previousOwner", prev, "newOwner", SystemOwnerAlias)
// Last, so every intermediate state resolves: until the alias is rewritten
// readers look under prev, and afterwards under the global account.
if moveSnapshot {
if err := metaStore.DeleteSnapshot(context.Background(), prev, meta.SnapshotID); err != nil {
slog.Error("PromoteSystemImage: delete the superseded snapshot document; the promoting account can still delete its blocks",
"imageId", opts.ImageID, "snapshotId", meta.SnapshotID, "previousOwner", prev, "err", err)
}
}
slog.Info("PromoteSystemImage completed", "imageId", opts.ImageID, "previousOwner", prev,
"newOwner", SystemOwnerAlias, "snapshotMoved", moveSnapshot)
return &PromoteImageResult{PreviousOwner: prev}, nil
}
// readPromotedSnapshot reads the document an AMI's snapshot is keyed under
// before promotion. A bundled image has no snapshot of its own, which is not an
// error: it already resolves under the global account by falling back.
func readPromotedSnapshot(store *ebsmetadata.Store, owner, snapshotID string) (ebsmetadata.Snapshot, bool, error) {
if snapshotID == "" {
return ebsmetadata.Snapshot{}, false, nil
}
snap, err := store.GetSnapshot(context.Background(), owner, snapshotID)
switch {
case err == nil:
return snap, true, nil
case objectstore.IsNoSuchKeyError(err):
return ebsmetadata.Snapshot{}, false, nil
default:
return ebsmetadata.Snapshot{}, false, err
}
}
// GetAMIMetadata reads and returns the control-plane document for the given
// image ID. Returns ErrorInvalidAMIIDNotFound for missing or corrupt documents.
func GetAMIMetadata(store objectstore.ObjectStore, bucket, imageID string) (ebsmetadata.AMI, error) {
+69
View File
@@ -1,10 +1,15 @@
package admin
import (
"bytes"
"testing"
"github.com/aws/aws-sdk-go/aws"
awss3 "github.com/aws/aws-sdk-go/service/s3"
"github.com/mulgadc/spinifex/spinifex/awserrors"
"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
"github.com/mulgadc/spinifex/spinifex/objectstore"
"github.com/mulgadc/spinifex/spinifex/utils"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@@ -27,6 +32,70 @@ func TestPromoteSystemImage_HappyPath(t *testing.T) {
assert.Equal(t, "snap-user-001", meta.SnapshotID)
}
// A promoted AMI's readers derive the snapshot account from the alias, so the
// document has to end up under the global account or every launch of it falls
// back to the image ID and the provider refuses the clone.
func TestPromoteSystemImage_MovesSnapshotToGlobalAccount(t *testing.T) {
store := objectstore.NewMemoryObjectStore()
const id = "ami-user-snap"
const snapID = "snap-user-snap"
putAMI(t, store, id, "my-app", testRemoveAccountID, snapID)
putSnapMetadata(t, store, snapID, "vol-source-001")
_, err := PromoteSystemImage(store, testRemoveBucket, PromoteImageOpts{ImageID: id})
require.NoError(t, err)
metaStore := ebsmetadata.NewStore(store, testRemoveBucket)
moved, err := metaStore.GetSnapshot(t.Context(), utils.GlobalAccountID, snapID)
require.NoError(t, err)
assert.Equal(t, utils.GlobalAccountID, moved.OwnerID)
assert.Equal(t, "vol-source-001", moved.VolumeID)
_, err = metaStore.GetSnapshot(t.Context(), testRemoveAccountID, snapID)
require.Error(t, err)
assert.True(t, objectstore.IsNoSuchKeyError(err), "the superseded document must not survive: %v", err)
}
// A bundled image has no snapshot document; promotion still succeeds and the
// AMI keeps resolving through the fallback.
func TestPromoteSystemImage_NoSnapshotDocument_Promotes(t *testing.T) {
store := objectstore.NewMemoryObjectStore()
const id = "ami-user-nosnap"
putAMI(t, store, id, "bundled", testRemoveAccountID, "snap-absent")
_, err := PromoteSystemImage(store, testRemoveBucket, PromoteImageOpts{ImageID: id})
require.NoError(t, err)
meta, err := readAMI(store, testRemoveBucket, id)
require.NoError(t, err)
assert.Equal(t, SystemOwnerAlias, meta.ImageOwnerAlias)
}
// Refusing beats promoting an AMI whose snapshot cannot be re-keyed, which
// would leave the alias pointing at a document no reader can find.
func TestPromoteSystemImage_CorruptSnapshotDocument_Refused(t *testing.T) {
store := objectstore.NewMemoryObjectStore()
const id = "ami-user-corruptsnap"
const snapID = "snap-corrupt-001"
putAMI(t, store, id, "my-app", testRemoveAccountID, snapID)
key, err := ebsmetadata.SnapshotKey(testRemoveAccountID, snapID)
require.NoError(t, err)
_, err = store.PutObject(t.Context(), &awss3.PutObjectInput{
Bucket: aws.String(testRemoveBucket),
Key: aws.String(key),
Body: bytes.NewReader([]byte("{not valid json")),
})
require.NoError(t, err)
_, err = PromoteSystemImage(store, testRemoveBucket, PromoteImageOpts{ImageID: id})
require.Error(t, err)
assert.Contains(t, err.Error(), "corrupt")
meta, err := readAMI(store, testRemoveBucket, id)
require.NoError(t, err)
assert.Equal(t, testRemoveAccountID, meta.ImageOwnerAlias, "the alias must not move without its snapshot")
}
func TestPromoteSystemImage_AlreadySystem_Refused(t *testing.T) {
store := objectstore.NewMemoryObjectStore()
const id = "ami-sys-001"
+17 -59
View File
@@ -12,7 +12,6 @@ import (
"github.com/aws/aws-sdk-go/service/s3"
"github.com/mulgadc/spinifex/spinifex/awserrors"
"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/utils"
)
@@ -128,35 +127,25 @@ func PreviewRemoveSystemImage(store objectstore.ObjectStore, bucket, imageID str
func FindAMIDependents(store objectstore.ObjectStore, bucket, imageID string) (Dependents, error) {
var deps Dependents
prefixes, err := listCommonPrefixes(store, bucket)
if err != nil {
return Dependents{}, fmt.Errorf("list bucket prefixes: %w", err)
}
// Pass 1: collect derived snaps (CopyImage of this AMI writes a snap whose
// VolumeID points back at the source AMI ID).
// VolumeID points back at the source AMI ID). Whole-cluster: a copy made by
// any account still pins this AMI's blocks. Strict: this answer gates
// deleting those blocks, and a document that failed to read is not evidence
// that no copy depends on them.
snapDocs, err := ebsmetadata.NewStore(store, bucket).ListAllSnapshotsStrict(context.Background())
if err != nil {
return Dependents{}, fmt.Errorf("list ebsmetadata snapshots: %w", err)
}
derived := map[string]bool{}
for _, p := range prefixes {
if !strings.HasPrefix(p, "snap-") {
for _, cfg := range snapDocs {
// Skip the viperblock-internal snap for this AMI — it has no document
// (it was written by viperblock.CreateSnapshot, not the EC2 service).
if cfg.SnapshotID == SnapPrefix(imageID) {
continue
}
snapID := strings.TrimSuffix(p, "/")
// Skip the viperblock-internal snap prefix for this AMI — it has no
// metadata.json (it was written by viperblock.CreateSnapshot, not by
// the EC2 snapshot service).
if snapID == SnapPrefix(imageID) {
continue
}
cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, bucket, snapID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) {
continue
}
return Dependents{}, fmt.Errorf("read snapshot %s: %w", snapID, err)
}
if cfg.VolumeID == imageID {
derived[snapID] = true
deps.Snapshots = append(deps.Snapshots, snapID)
derived[cfg.SnapshotID] = true
deps.Snapshots = append(deps.Snapshots, cfg.SnapshotID)
}
}
@@ -170,8 +159,9 @@ func FindAMIDependents(store objectstore.ObjectStore, bucket, imageID string) (D
// Pass 2: volumes whose SnapshotID matches. The ebsmetadata documents are
// the whole fleet — a volume with blocks but no document does not exist as
// far as the control plane is concerned — and they carry SnapshotID
// already, so no second read per volume is needed.
volDocs, err := ebsmetadata.NewStore(store, bucket).ListVolumes(context.Background())
// already, so no second read per volume is needed. Strict for the same
// reason as pass 1.
volDocs, err := ebsmetadata.NewStore(store, bucket).ListAllVolumesStrict(context.Background())
if err != nil {
return Dependents{}, fmt.Errorf("list ebsmetadata volumes: %w", err)
}
@@ -296,38 +286,6 @@ func readAMI(store objectstore.ObjectStore, bucket, imageID string) (ebsmetadata
return ebsmetadata.NewStore(store, bucket).GetAMI(context.Background(), imageID)
}
// listCommonPrefixes returns the top-level "directory" prefixes in the bucket
// (e.g. "ami-xxx/", "vol-yyy/", "snap-zzz/"), exhausting any pagination.
func listCommonPrefixes(store objectstore.ObjectStore, bucket string) ([]string, error) {
seen := map[string]bool{}
var token *string
for {
out, err := store.ListObjectsV2(context.Background(), &s3.ListObjectsV2Input{
Bucket: aws.String(bucket),
Delimiter: aws.String("/"),
ContinuationToken: token,
})
if err != nil {
return nil, err
}
for _, cp := range out.CommonPrefixes {
if cp.Prefix == nil {
continue
}
seen[*cp.Prefix] = true
}
if !aws.BoolValue(out.IsTruncated) {
break
}
token = out.NextContinuationToken
}
prefixes := make([]string, 0, len(seen))
for p := range seen {
prefixes = append(prefixes, p)
}
return prefixes, nil
}
// sumPrefix returns (object count, total bytes) for every object under prefix.
// Used by the preview to surface what an operator is about to delete.
func sumPrefix(store objectstore.ObjectStore, bucket, prefix string) (int, int64, error) {
+5 -5
View File
@@ -9,7 +9,6 @@ import (
awss3 "github.com/aws/aws-sdk-go/service/s3"
"github.com/mulgadc/spinifex/spinifex/awserrors"
"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/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@@ -80,13 +79,13 @@ func putSnapBlocks(t *testing.T, store *objectstore.MemoryObjectStore, snapID st
// VolumeID back at an admin-imported AMI's ID.
func putSnapMetadata(t *testing.T, store *objectstore.MemoryObjectStore, snapID, volumeID string) {
t.Helper()
cfg := &handlers_ec2_snapshot.SnapshotConfig{
require.NoError(t, ebsmetadata.NewStore(store, testRemoveBucket).PutSnapshot(t.Context(), ebsmetadata.Snapshot{
SnapshotID: snapID,
VolumeID: volumeID,
VolumeSize: 8,
State: "completed",
}
require.NoError(t, handlers_ec2_snapshot.WriteSnapshotConfig(store, testRemoveBucket, snapID, cfg))
OwnerID: testRemoveAccountID,
}))
}
// putVolume registers a volume document carrying the given SnapshotID.
@@ -94,6 +93,7 @@ func putVolume(t *testing.T, store *objectstore.MemoryObjectStore, volID, snapsh
t.Helper()
require.NoError(t, ebsmetadata.NewStore(store, testRemoveBucket).PutVolume(t.Context(), ebsmetadata.Volume{
VolumeID: volID,
TenantID: testRemoveAccountID,
SnapshotID: snapshotID,
CapacityGiB: 8,
}))
@@ -268,7 +268,7 @@ func TestRemoveSystemImage_Force_OverridesDependents(t *testing.T) {
metadata := ebsmetadata.NewStore(store, testRemoveBucket)
_, err = metadata.GetAMI(t.Context(), id)
require.Error(t, err)
_, err = metadata.GetVolume(t.Context(), "vol-orphan")
_, err = metadata.GetVolume(t.Context(), testRemoveAccountID, "vol-orphan")
require.NoError(t, err)
}
+1 -1
View File
@@ -51,7 +51,7 @@ func FindOrphanVolumes(ctx context.Context, provider ebsprovider.EBSProvider, me
// Strict listing on purpose: a document this cannot decode must abort the
// scan, never be treated as absent. Reporting a volume as orphaned because
// its document was unreadable would point an operator at live data.
documented, err := metadata.ListVolumesStrict(ctx)
documented, err := metadata.ListAllVolumesStrict(ctx)
if err != nil {
return nil, fmt.Errorf("orphan scan: list control-plane volumes: %w", err)
}
+1 -1
View File
@@ -44,7 +44,7 @@ func documentVolume(t *testing.T, metadata *ebsmetadata.Store, volumeID string)
// so a test can plant one the store cannot decode.
func putRawVolumeDocument(t *testing.T, store objectstore.ObjectStore, volumeID string, body []byte) {
t.Helper()
key, err := ebsmetadata.VolumeKey(volumeID)
key, err := ebsmetadata.VolumeKey("123456789012", volumeID)
require.NoError(t, err)
_, err = store.PutObject(t.Context(), &s3.PutObjectInput{
Bucket: aws.String(orphanTestBucket),
+6 -1
View File
@@ -97,6 +97,11 @@ func wireTestEBSProvider(daemon *Daemon, store objectstore.ObjectStore) {
// here rather than writing storage state the control plane never reads.
func seedVolumeDocument(t *testing.T, store objectstore.ObjectStore, volume ebsmetadata.Volume) {
t.Helper()
// The owning account is a key segment, so a fixture that does not care
// which account owns it still has to have one.
if volume.TenantID == "" {
volume.TenantID = testAccountID
}
require.NoError(t, ebsmetadata.NewStore(store, "test-bucket").PutVolume(t.Context(), volume))
}
@@ -2658,7 +2663,7 @@ func TestHandleEC2ModifyVolume_Success(t *testing.T) {
Size: aws.Int64(20),
}
reqData, _ := json.Marshal(input)
reply, err := daemon.natsConn.Request("ec2.ModifyVolume", reqData, 5*time.Second)
reply, err := natsRequest(daemon.natsConn, "ec2.ModifyVolume", reqData, 5*time.Second)
require.NoError(t, err)
var output ec2.ModifyVolumeOutput
+9 -3
View File
@@ -40,14 +40,20 @@ func (d *Daemon) handleAttachVolume(ctx context.Context, msg *nats.Msg, command
return respondErrorOutcome(d.node, msg, awserrors.ErrorIncorrectInstanceState)
}
volMeta, err := d.volumeService.GetVolumeMetadata(volumeID)
// The caller's account is a key segment, so it is needed before the read
// rather than after it.
callerAccountID := utils.AccountIDFromMsg(msg)
volMeta, err := d.volumeService.GetVolumeMetadata(callerAccountID, volumeID)
if err != nil {
slog.ErrorContext(ctx, "AttachVolume: failed to get volume metadata", "volumeId", volumeID, "err", err)
return respondErrorOutcome(d.node, msg, awserrors.ErrorInvalidVolumeNotFound)
}
callerAccountID := utils.AccountIDFromMsg(msg)
if !volumeVisibleTo(volMeta.TenantID, callerAccountID) {
// The read was keyed on the caller's account, so the document is theirs by
// construction and an empty caller never got this far. The comparison stays
// as an assertion.
if volMeta.TenantID != callerAccountID {
slog.WarnContext(ctx, "AttachVolume: account does not own volume",
"volumeId", volumeID,
"callerAccount", callerAccountID,
+25 -10
View File
@@ -1595,7 +1595,7 @@ func TestInstanceCleanerAdapter_DeleteVolumes_BootVolumeDeletedAfterAttachmentCl
err = cleaner.DeleteVolumes(instance)
require.NoError(t, err, "the still-attached boot volume must be deleted, not rejected as VolumeInUse")
_, err = daemon.volumeService.GetVolumeMetadata(volumeID)
_, err = daemon.volumeService.GetVolumeMetadata(testAccountID, volumeID)
require.Error(t, err, "the volume must actually be deleted")
assert.Contains(t, err.Error(), awserrors.ErrorInvalidVolumeNotFound)
}
@@ -1633,7 +1633,7 @@ func TestInstanceCleanerAdapter_DeleteVolumes_NonDoTBootVolumeDetachedNotDeleted
err := cleaner.DeleteVolumes(instance)
require.NoError(t, err)
meta, err := daemon.volumeService.GetVolumeMetadata(volumeID)
meta, err := daemon.volumeService.GetVolumeMetadata(testAccountID, volumeID)
require.NoError(t, err, "a DeleteOnTermination=false volume must survive terminate")
assert.Equal(t, "available", meta.State)
assert.Empty(t, meta.AttachedInstance, "terminate must still detach it")
@@ -1673,7 +1673,7 @@ func TestInstanceCleanerAdapter_DeleteVolumes_NonBootNonDoTVolumeDetachedNotDele
err := cleaner.DeleteVolumes(instance)
require.NoError(t, err)
meta, err := daemon.volumeService.GetVolumeMetadata(volumeID)
meta, err := daemon.volumeService.GetVolumeMetadata(testAccountID, volumeID)
require.NoError(t, err, "a DeleteOnTermination=false volume must survive terminate")
assert.Equal(t, "available", meta.State)
assert.Empty(t, meta.AttachedInstance, "terminate must still detach it")
@@ -1743,7 +1743,7 @@ func TestTerminatedTeardownReaper_SelfHealsFailedVolumeTeardown(t *testing.T) {
_, err = reaper.Sweep(context.Background())
require.NoError(t, err)
_, err = daemon.volumeService.GetVolumeMetadata(volumeID)
_, err = daemon.volumeService.GetVolumeMetadata(testAccountID, volumeID)
require.Error(t, err, "the retried delete must actually remove the volume")
assert.Contains(t, err.Error(), awserrors.ErrorInvalidVolumeNotFound)
@@ -1804,7 +1804,7 @@ func TestTerminatedTeardownReaper_SelfHealsFailedVolumeDetach(t *testing.T) {
_, err := reaper.Sweep(context.Background())
require.NoError(t, err)
meta, err := daemon.volumeService.GetVolumeMetadata(volumeID)
meta, err := daemon.volumeService.GetVolumeMetadata(testAccountID, volumeID)
require.NoError(t, err, "the volume must survive: DeleteOnTermination=false")
assert.Equal(t, "available", meta.State)
assert.Empty(t, meta.AttachedInstance, "the retry must clear the stale attachment")
@@ -1862,7 +1862,7 @@ func TestStuckTerminateReaper_DetachesNonDoTVolumeWithoutUnmount(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, 1, reaped, "the wedged terminate must be force-completed")
meta, err := daemon.volumeService.GetVolumeMetadata(volumeID)
meta, err := daemon.volumeService.GetVolumeMetadata(testAccountID, volumeID)
require.NoError(t, err)
assert.Equal(t, "available", meta.State,
"the force-complete path must still detach a non-Boot DeleteOnTermination=false volume")
@@ -3396,15 +3396,21 @@ func TestVolumeMounterAdapter_UnmountOne_RequestFailure(t *testing.T) {
// recordingVolState records UpdateVolumeState calls so the bulk-unmount test can
// assert which volumes transitioned to available.
// accounts is recorded separately because the account is a key segment of the
// document, not a label: an unmount that writes under anything but the
// instance's own account leaves the volume stuck in-use, and the adapter only
// logs the error.
type recordingVolState struct {
mu sync.Mutex
calls []string
mu sync.Mutex
calls []string
accounts []string
}
func (r *recordingVolState) UpdateVolumeState(volumeID, state, instanceID, attachmentDevice string) error {
func (r *recordingVolState) UpdateVolumeState(accountID, volumeID, state, instanceID, attachmentDevice string) error {
r.mu.Lock()
defer r.mu.Unlock()
r.calls = append(r.calls, volumeID+":"+state)
r.accounts = append(r.accounts, accountID)
return nil
}
@@ -3414,6 +3420,12 @@ func (r *recordingVolState) snapshot() []string {
return append([]string(nil), r.calls...)
}
func (r *recordingVolState) accountsSnapshot() []string {
r.mu.Lock()
defer r.mu.Unlock()
return append([]string(nil), r.accounts...)
}
var _ vm.VolumeStateUpdater = (*recordingVolState)(nil)
// TestVolumeMounterAdapter_Unmount_SealFailureSkipsAvailable verifies the
@@ -3482,7 +3494,8 @@ func TestVolumeMounterAdapter_Unmount_NotFoundFlipsToAvailable(t *testing.T) {
defer sub.Unsubscribe()
inst := &vm.VM{
ID: "i-unmount-retry",
ID: "i-unmount-retry",
AccountID: "000000000042",
EBSRequests: types.EBSRequests{
Requests: []types.EBSRequest{
{Name: "vol-boot-retry", Boot: true, EFI: false},
@@ -3497,6 +3510,8 @@ func TestVolumeMounterAdapter_Unmount_NotFoundFlipsToAvailable(t *testing.T) {
"a NotFound response (already-completed seal) must still flip a non-boot data volume to available")
assert.NotContains(t, calls, "vol-boot-retry:available",
"a boot volume must stay attached even when the seal retry reports NotFound")
assert.Equal(t, []string{"000000000042"}, volState.accountsSnapshot(),
"the state write must be keyed on the instance's own account, not its ID")
}
// TestUnmountResponseError covers the shared seal-result parser used by both the
-9
View File
@@ -31,12 +31,3 @@ func checkInstanceOwnership(nodeID string, msg *nats.Msg, instanceID, ownerAccou
}
return true
}
// volumeVisibleTo reports whether callerAccountID may access a volume.
// Empty tenantID is root-only to prevent untenanted volumes from leaking.
func volumeVisibleTo(tenantID, callerAccountID string) bool {
if tenantID == "" {
return callerAccountID == utils.GlobalAccountID
}
return callerAccountID == tenantID
}
-75
View File
@@ -1,75 +0,0 @@
package daemon
import (
"testing"
"github.com/mulgadc/spinifex/spinifex/utils"
)
func TestVolumeVisibleTo(t *testing.T) {
tests := []struct {
name string
tenantID string
callerAccountID string
expected bool
}{
{
name: "same tenant",
tenantID: "123456789012",
callerAccountID: "123456789012",
expected: true,
},
{
name: "different tenant denied",
tenantID: "123456789012",
callerAccountID: "999999999999",
expected: false,
},
{
name: "untenanted volume visible to root",
tenantID: "",
callerAccountID: utils.GlobalAccountID,
expected: true,
},
{
name: "untenanted volume hidden from non-root tenant",
tenantID: "",
callerAccountID: "123456789012",
expected: false,
},
{
name: "untenanted volume denied to empty caller",
tenantID: "",
callerAccountID: "",
expected: false,
},
{
name: "tenant volume denied to empty caller",
tenantID: "123456789012",
callerAccountID: "",
expected: false,
},
{
name: "root cannot impersonate tenant volume",
tenantID: "123456789012",
callerAccountID: utils.GlobalAccountID,
expected: false,
},
{
name: "root accessing root-owned volume",
tenantID: utils.GlobalAccountID,
callerAccountID: utils.GlobalAccountID,
expected: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := volumeVisibleTo(tt.tenantID, tt.callerAccountID)
if got != tt.expected {
t.Errorf("volumeVisibleTo(%q, %q) = %v, want %v",
tt.tenantID, tt.callerAccountID, got, tt.expected)
}
})
}
}
+1 -1
View File
@@ -258,7 +258,7 @@ func (a *volumeMounterAdapter) Unmount(ctx context.Context, instance *vm.VM) err
// Boot too. Only DetachVolume and terminate release a boot volume; flipping
// it available here while the instance restarts splits the state record.
if sealed && !ebsRequest.EFI && !ebsRequest.Boot && a.volState != nil {
if err := a.volState.UpdateVolumeState(ebsRequest.Name, "available", "", ""); err != nil {
if err := a.volState.UpdateVolumeState(instance.AccountID, ebsRequest.Name, "available", "", ""); err != nil {
slog.Error("Failed to update volume state to available after unmount",
"volumeId", ebsRequest.Name, "err", err)
}
+69 -4
View File
@@ -8,9 +8,14 @@ import (
"fmt"
"strings"
"time"
"github.com/mulgadc/spinifex/spinifex/utils"
)
const SchemaVersion uint16 = 1
// SchemaVersion is the document's field generation, and moves independently of
// the key layout's v2 prefix. They start aligned on a fresh install and are not
// promised to stay so: adding a field bumps this without re-keying anything.
const SchemaVersion uint16 = 2
// ErrCorruptDocument wraps decode and schema-version failures so callers can
// tell a document that exists but cannot be read from one that is absent. The
@@ -61,6 +66,25 @@ type VolumeModification struct {
EndTime time.Time `json:"end_time,omitzero"`
}
// Snapshot is the control-plane record used for EC2 snapshot operations. It
// deliberately excludes viperblock's own half of the snapshot prefix — the
// block checkpoint and config.json — which stays at the bucket root.
type Snapshot struct {
SchemaVersion uint16 `json:"schema_version"`
SnapshotID string `json:"snapshot_id"`
VolumeID string `json:"volume_id"`
VolumeSize int64 `json:"volume_size"`
State string `json:"state"`
Progress string `json:"progress"`
StartTime time.Time `json:"start_time"`
Description string `json:"description"`
Encrypted bool `json:"encrypted"`
OwnerID string `json:"owner_id"`
AvailabilityZone string `json:"availability_zone"`
Tags map[string]string `json:"tags,omitempty"`
ProviderHandle string `json:"provider_handle,omitempty"`
}
// AMI is the control-plane record used for EC2 image operations.
type AMI struct {
SchemaVersion uint16 `json:"schema_version"`
@@ -82,14 +106,39 @@ type AMI struct {
State string `json:"state,omitempty"`
}
func VolumeKey(volumeID string) (string, error) { return key("volumes", volumeID) }
func AMIKey(imageID string) (string, error) { return key("amis", imageID) }
// VolumeKey keys a volume document under its owning account, so a listing of
// one account's prefix cannot reach another account's document. accountID comes
// from the document's own TenantID, never from the caller.
func VolumeKey(accountID, volumeID string) (string, error) {
return partitionedKey("volumes", accountID, volumeID)
}
// SnapshotKey keys a snapshot document under its owning account, taken from the
// document's own OwnerID rather than from the caller.
func SnapshotKey(accountID, snapshotID string) (string, error) {
return partitionedKey("snapshots", accountID, snapshotID)
}
// AMIKey is unpartitioned: an AMI's owner is an alias rather than an account,
// and system images are visible to every account.
func AMIKey(imageID string) (string, error) { return key("amis", imageID) }
// partitionedKey refuses an owning account that is not an account ID, so an
// untenanted document cannot be written, cannot exist, and never has to be
// read. A transposed (id, account) pair fails here too.
func partitionedKey(kind, accountID, id string) (string, error) {
if !utils.IsAccountID(accountID) {
return "", fmt.Errorf("invalid EBS metadata account ID %q", accountID)
}
return key(kind+"/"+accountID, id)
}
// key rejects an ID that would escape or rename its own path segment.
func key(kind, id string) (string, error) {
if id == "" || id == "." || id == ".." || strings.ContainsAny(id, "/\\") {
return "", fmt.Errorf("invalid EBS metadata ID %q", id)
}
return "spinifex/ebsmetadata/v1/" + kind + "/" + id + ".json", nil
return "spinifex/ebsmetadata/v2/" + kind + "/" + id + ".json", nil
}
func MarshalVolume(volume Volume) ([]byte, error) {
@@ -108,6 +157,22 @@ func UnmarshalVolume(data []byte) (Volume, error) {
return volume, nil
}
func MarshalSnapshot(snapshot Snapshot) ([]byte, error) {
snapshot.SchemaVersion = SchemaVersion
return json.Marshal(snapshot)
}
func UnmarshalSnapshot(data []byte) (Snapshot, error) {
var snapshot Snapshot
if err := json.Unmarshal(data, &snapshot); err != nil {
return Snapshot{}, fmt.Errorf("%w: %w", ErrCorruptDocument, err)
}
if snapshot.SchemaVersion != SchemaVersion {
return Snapshot{}, fmt.Errorf("%w: unsupported snapshot metadata schema version %d", ErrCorruptDocument, snapshot.SchemaVersion)
}
return snapshot, nil
}
func MarshalAMI(ami AMI) ([]byte, error) {
ami.SchemaVersion = SchemaVersion
return json.Marshal(ami)
+54 -4
View File
@@ -4,13 +4,14 @@ import (
"testing"
"time"
"github.com/mulgadc/spinifex/spinifex/utils"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestVolumeMetadataRoundTripOwnsItsSchema(t *testing.T) {
want := Volume{
VolumeID: "vol-1", TenantID: "acct-1", CapacityGiB: 20,
VolumeID: "vol-1", TenantID: "000000000001", CapacityGiB: 20,
State: "available", CreatedAt: time.Date(2026, 8, 5, 0, 0, 0, 0, time.UTC),
ProviderHandle: "opaque-provider-handle", Tags: map[string]string{"env": "test"},
}
@@ -30,7 +31,7 @@ func TestVolumeMetadataRoundTripOwnsItsSchema(t *testing.T) {
// survive a marshal/unmarshal round-trip.
func TestVolumeRoundTripsEncryptedAndModification(t *testing.T) {
want := Volume{
VolumeID: "vol-1", TenantID: "acct-1", CapacityGiB: 20,
VolumeID: "vol-1", TenantID: "000000000001", CapacityGiB: 20,
State: "available", Encrypted: true,
Modification: &VolumeModification{
ModificationState: "completed", Progress: 100,
@@ -57,10 +58,59 @@ func TestMetadataRejectsUnknownSchemaVersion(t *testing.T) {
func TestMetadataKeysRejectPathTraversal(t *testing.T) {
for _, id := range []string{"", "..", "../escape", "a/b", "a\\b"} {
_, err := VolumeKey(id)
_, err := VolumeKey("000000000001", id)
require.Error(t, err, "ID %q must not escape metadata prefix", id)
_, err = SnapshotKey("000000000001", id)
require.Error(t, err, "ID %q must not escape metadata prefix", id)
_, err = VolumeKey(id, "vol-1")
require.Error(t, err, "account %q must not escape metadata prefix", id)
}
key, err := AMIKey("ami-1")
require.NoError(t, err)
assert.Equal(t, "spinifex/ebsmetadata/v1/amis/ami-1.json", key)
assert.Equal(t, "spinifex/ebsmetadata/v2/amis/ami-1.json", key)
}
// An untenanted document cannot be written, so it cannot exist, so nothing
// needs to read one.
func TestPartitionedKeysRejectNonAccountOwner(t *testing.T) {
for _, accountID := range []string{"", "acct-1", "12345678901", "1234567890123", "00000000000a", "system"} {
_, err := VolumeKey(accountID, "vol-1")
require.Error(t, err, "account %q is not an account ID", accountID)
_, err = SnapshotKey(accountID, "snap-1")
require.Error(t, err, "account %q is not an account ID", accountID)
}
// A transposed (id, account) pair: a vol- string is not twelve digits.
_, err := VolumeKey("vol-1", "000000000001")
require.Error(t, err)
// The system account is an ordinary account and partitions like any other.
key, err := VolumeKey(utils.GlobalAccountID, "vol-1")
require.NoError(t, err)
assert.Equal(t, "spinifex/ebsmetadata/v2/volumes/000000000000/vol-1.json", key)
}
func TestSnapshotKeyPartitionsByOwningAccount(t *testing.T) {
key, err := SnapshotKey("000000000001", "snap-1")
require.NoError(t, err)
assert.Equal(t, "spinifex/ebsmetadata/v2/snapshots/000000000001/snap-1.json", key)
other, err := SnapshotKey("000000000002", "snap-1")
require.NoError(t, err)
assert.NotEqual(t, key, other, "the same snapshot ID under two accounts must be two keys")
}
// The owning account is a key segment, which is what lets a listing of one
// account's prefix answer without reading another account's documents.
func TestVolumeKeyPartitionsByOwningAccount(t *testing.T) {
key, err := VolumeKey("000000000001", "vol-1")
require.NoError(t, err)
assert.Equal(t, "spinifex/ebsmetadata/v2/volumes/000000000001/vol-1.json", key)
other, err := VolumeKey("000000000002", "vol-1")
require.NoError(t, err)
assert.NotEqual(t, key, other, "the same volume ID under two accounts must be two keys")
}
+215
View File
@@ -0,0 +1,215 @@
package ebsmetadata_test
import (
"context"
"sort"
"strings"
"testing"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/s3"
. "github.com/mulgadc/spinifex/spinifex/ebsmetadata"
"github.com/mulgadc/spinifex/spinifex/objectstore"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
const (
accountA = "000000000001"
accountB = "000000000002"
)
func TestStoreRoundTripsSnapshot(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
snapshot := Snapshot{SnapshotID: "snap-1", VolumeID: "vol-1", OwnerID: accountA,
VolumeSize: 8, State: "completed", ProviderHandle: "opaque"}
require.NoError(t, store.PutSnapshot(ctx, snapshot))
got, err := store.GetSnapshot(ctx, accountA, "snap-1")
require.NoError(t, err)
assert.Equal(t, snapshot.VolumeID, got.VolumeID)
assert.Equal(t, snapshot.ProviderHandle, got.ProviderHandle)
assert.Equal(t, SchemaVersion, got.SchemaVersion)
require.NoError(t, store.DeleteSnapshot(ctx, accountA, "snap-1"))
_, err = store.GetSnapshot(ctx, accountA, "snap-1")
assert.True(t, objectstore.IsNoSuchKeyError(err))
}
// PutSnapshot takes the key segment from the document's own OwnerID, so an
// untenanted snapshot has nowhere to be written and cannot exist.
func TestPutSnapshot_RefusesUntenantedDocument(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
err := store.PutSnapshot(context.Background(), Snapshot{SnapshotID: "snap-1", VolumeID: "vol-1"})
require.Error(t, err)
assert.Contains(t, err.Error(), "invalid EBS metadata account ID")
}
// A snapshot outside the caller's prefix is absent, not denied: the key is the
// ownership check.
func TestGetSnapshot_OtherAccountIsNotFound(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
require.NoError(t, store.PutSnapshot(ctx, Snapshot{SnapshotID: "snap-1", VolumeID: "vol-1", OwnerID: accountA}))
_, err := store.GetSnapshot(ctx, accountB, "snap-1")
assert.True(t, objectstore.IsNoSuchKeyError(err))
}
func TestListSnapshots_ScopedToOneAccount(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
for _, snapshot := range []Snapshot{
{SnapshotID: "snap-a1", OwnerID: accountA},
{SnapshotID: "snap-a2", OwnerID: accountA},
{SnapshotID: "snap-b1", OwnerID: accountB},
} {
require.NoError(t, store.PutSnapshot(ctx, snapshot))
}
got, err := store.ListSnapshots(ctx, accountA)
require.NoError(t, err)
assert.Equal(t, []string{"snap-a1", "snap-a2"}, snapshotIDs(got))
got, err = store.ListSnapshots(ctx, accountB)
require.NoError(t, err)
assert.Equal(t, []string{"snap-b1"}, snapshotIDs(got))
}
// The two verbs must partition and list the same way: a key builder that wrote
// one shape and listed another would show up here and nowhere else.
func TestListAllSnapshots_IsTheUnionOfEveryAccount(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
for _, snapshot := range []Snapshot{
{SnapshotID: "snap-a1", OwnerID: accountA},
{SnapshotID: "snap-b1", OwnerID: accountB},
{SnapshotID: "snap-b2", OwnerID: accountB},
} {
require.NoError(t, store.PutSnapshot(ctx, snapshot))
}
all, err := store.ListAllSnapshots(ctx)
require.NoError(t, err)
var union []Snapshot
for _, account := range []string{accountA, accountB} {
scoped, err := store.ListSnapshots(ctx, account)
require.NoError(t, err)
union = append(union, scoped...)
}
assert.Equal(t, snapshotIDs(all), snapshotIDs(union))
}
// An empty account must not widen a listing to every account's documents.
func TestListSnapshots_RefusesANonAccountPrefix(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
require.NoError(t, store.PutSnapshot(ctx, Snapshot{SnapshotID: "snap-a1", OwnerID: accountA}))
for _, account := range []string{"", "not-an-account", "snap-a1"} {
_, err := store.ListSnapshots(ctx, account)
require.Error(t, err, "account %q must not list", account)
_, err = store.ListSnapshotsStrict(ctx, account)
require.Error(t, err, "account %q must not list strictly", account)
}
}
func TestListSnapshots_SkipsCorruptButStrictDoesNot(t *testing.T) {
objects := objectstore.NewMemoryObjectStore()
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutSnapshot(ctx, Snapshot{SnapshotID: "snap-a1", OwnerID: accountA}))
key, err := SnapshotKey(accountA, "snap-corrupt")
require.NoError(t, err)
_, err = objects.PutObject(ctx, &s3.PutObjectInput{
Bucket: aws.String("control-plane"), Key: aws.String(key),
Body: strings.NewReader("{not json"),
})
require.NoError(t, err)
tolerant, err := store.ListSnapshots(ctx, accountA)
require.NoError(t, err)
assert.Equal(t, []string{"snap-a1"}, snapshotIDs(tolerant))
_, err = store.ListSnapshotsStrict(ctx, accountA)
require.ErrorIs(t, err, ErrCorruptDocument)
}
// A snapshot document written by an older schema is corrupt, not absent: the
// two deserve different answers.
func TestUnmarshalSnapshot_RejectsUnknownSchemaVersion(t *testing.T) {
data, err := MarshalSnapshot(Snapshot{SnapshotID: "snap-1", OwnerID: accountA})
require.NoError(t, err)
stale := strings.Replace(string(data), `"schema_version":2`, `"schema_version":1`, 1)
require.NotEqual(t, string(data), stale)
_, err = UnmarshalSnapshot([]byte(stale))
require.ErrorIs(t, err, ErrCorruptDocument)
}
// A malformed account reaches the key builder only through a Spinifex bug, so
// it stays a plain error rather than a not-found the caller would report as a
// missing snapshot.
func TestSnapshotVerbs_RefuseAMalformedAccount(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
_, err := store.GetSnapshot(ctx, "not-an-account", "snap-1")
require.Error(t, err)
assert.False(t, objectstore.IsNoSuchKeyError(err))
require.Error(t, store.DeleteSnapshot(ctx, "", "snap-1"))
}
func TestListAllSnapshotsStrict_FailsOnACorruptDocument(t *testing.T) {
objects := objectstore.NewMemoryObjectStore()
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutSnapshot(ctx, Snapshot{SnapshotID: "snap-a1", OwnerID: accountA}))
key, err := SnapshotKey(accountB, "snap-corrupt")
require.NoError(t, err)
_, err = objects.PutObject(ctx, &s3.PutObjectInput{
Bucket: aws.String("control-plane"), Key: aws.String(key),
Body: strings.NewReader("{not json"),
})
require.NoError(t, err)
tolerant, err := store.ListAllSnapshots(ctx)
require.NoError(t, err)
assert.Equal(t, []string{"snap-a1"}, snapshotIDs(tolerant))
_, err = store.ListAllSnapshotsStrict(ctx)
require.ErrorIs(t, err, ErrCorruptDocument)
}
// An unconfigured store must say so rather than answer "no snapshots", which a
// caller would act on as authoritative absence.
func TestSnapshotListings_RefuseAnUnconfiguredStore(t *testing.T) {
store := NewStore(nil, "control-plane")
ctx := context.Background()
for name, list := range map[string]func() ([]Snapshot, error){
"ListSnapshots": func() ([]Snapshot, error) { return store.ListSnapshots(ctx, accountA) },
"ListSnapshotsStrict": func() ([]Snapshot, error) { return store.ListSnapshotsStrict(ctx, accountA) },
"ListAllSnapshots": func() ([]Snapshot, error) { return store.ListAllSnapshots(ctx) },
"ListAllSnapshotsStrict": func() ([]Snapshot, error) { return store.ListAllSnapshotsStrict(ctx) },
} {
_, err := list()
require.Error(t, err, name)
assert.Contains(t, err.Error(), "not configured", name)
}
}
func snapshotIDs(snapshots []Snapshot) []string {
ids := make([]string, 0, len(snapshots))
for _, snapshot := range snapshots {
ids = append(ids, snapshot.SnapshotID)
}
sort.Strings(ids)
return ids
}
+133 -19
View File
@@ -13,6 +13,7 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/mulgadc/spinifex/spinifex/objectstore"
"github.com/mulgadc/spinifex/spinifex/utils"
)
// Store persists Spinifex-owned metadata documents in the existing object
@@ -26,8 +27,11 @@ func NewStore(objects objectstore.ObjectStore, bucket string) *Store {
return &Store{objects: objects, bucket: bucket}
}
// PutVolume takes no account: the key segment is the document's own TenantID.
// An account passed alongside a document that already carries one is how the
// two come to disagree.
func (s *Store) PutVolume(ctx context.Context, volume Volume) error {
key, err := VolumeKey(volume.VolumeID)
key, err := VolumeKey(volume.TenantID, volume.VolumeID)
if err != nil {
return err
}
@@ -40,8 +44,8 @@ func (s *Store) PutVolume(ctx context.Context, volume Volume) error {
// GetVolume returns the volume's ebsmetadata document. A volume with no
// document does not exist as far as the control plane is concerned.
func (s *Store) GetVolume(ctx context.Context, volumeID string) (Volume, error) {
key, err := VolumeKey(volumeID)
func (s *Store) GetVolume(ctx context.Context, accountID, volumeID string) (Volume, error) {
key, err := VolumeKey(accountID, volumeID)
if err != nil {
return Volume{}, err
}
@@ -52,8 +56,8 @@ func (s *Store) GetVolume(ctx context.Context, volumeID string) (Volume, error)
return UnmarshalVolume(data)
}
func (s *Store) DeleteVolume(ctx context.Context, volumeID string) error {
key, err := VolumeKey(volumeID)
func (s *Store) DeleteVolume(ctx context.Context, accountID, volumeID string) error {
key, err := VolumeKey(accountID, volumeID)
if err != nil {
return err
}
@@ -61,28 +65,137 @@ func (s *Store) DeleteVolume(ctx context.Context, volumeID string) error {
return err
}
// ListVolumes returns every ebsmetadata volume document, skipping any it
// cannot read or decode. One unreadable document must not make every volume in
// the cluster invisible, so the tolerance is deliberate and each skip is logged.
func (s *Store) ListVolumes(ctx context.Context) ([]Volume, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
return s.listVolumeDocuments(ctx, true)
// ListVolumes returns one account's volume documents, skipping any it cannot
// read or decode. The prefix is the isolation: no other account's document is
// read, so none has to be filtered out afterwards. One unreadable document must
// not make every volume invisible, so the tolerance is deliberate and logged.
func (s *Store) ListVolumes(ctx context.Context, accountID string) ([]Volume, error) {
return s.listVolumeDocuments(ctx, accountID, true)
}
// ListVolumesStrict is ListVolumes without the tolerance. It is for callers
// whose answer would be wrong rather than merely partial: a volume that failed
// to read is not evidence that no volume holds the resource being checked.
func (s *Store) ListVolumesStrict(ctx context.Context) ([]Volume, error) {
func (s *Store) ListVolumesStrict(ctx context.Context, accountID string) ([]Volume, error) {
return s.listVolumeDocuments(ctx, accountID, false)
}
// ListAllVolumes lists across every account. Whole-cluster by intent: the name
// is deliberately not ListVolumes so no caller gets this reach by leaving a
// call site untouched.
func (s *Store) ListAllVolumes(ctx context.Context) ([]Volume, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
return s.listVolumeDocuments(ctx, false)
return listDocuments(ctx, s, volumePrefix, "volume", UnmarshalVolume, true)
}
func (s *Store) listVolumeDocuments(ctx context.Context, skipCorrupt bool) ([]Volume, error) {
return listDocuments(ctx, s, volumePrefix, "volume", UnmarshalVolume, skipCorrupt)
// ListAllVolumesStrict is ListAllVolumes without the tolerance.
func (s *Store) ListAllVolumesStrict(ctx context.Context) ([]Volume, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
return listDocuments(ctx, s, volumePrefix, "volume", UnmarshalVolume, false)
}
func (s *Store) listVolumeDocuments(ctx context.Context, accountID string, skipCorrupt bool) ([]Volume, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
prefix, err := accountPrefix(volumePrefix, accountID)
if err != nil {
return nil, err
}
return listDocuments(ctx, s, prefix, "volume", UnmarshalVolume, skipCorrupt)
}
// PutSnapshot takes no account: like PutVolume, the key segment is the
// document's own OwnerID.
func (s *Store) PutSnapshot(ctx context.Context, snapshot Snapshot) error {
key, err := SnapshotKey(snapshot.OwnerID, snapshot.SnapshotID)
if err != nil {
return err
}
data, err := MarshalSnapshot(snapshot)
if err != nil {
return fmt.Errorf("marshal snapshot metadata: %w", err)
}
return s.put(ctx, key, data)
}
// GetSnapshot returns the snapshot's ebsmetadata document. A snapshot outside
// accountID's prefix is not found rather than denied: the key is the ownership
// check.
func (s *Store) GetSnapshot(ctx context.Context, accountID, snapshotID string) (Snapshot, error) {
key, err := SnapshotKey(accountID, snapshotID)
if err != nil {
return Snapshot{}, err
}
data, err := s.get(ctx, key)
if err != nil {
return Snapshot{}, err
}
return UnmarshalSnapshot(data)
}
func (s *Store) DeleteSnapshot(ctx context.Context, accountID, snapshotID string) error {
key, err := SnapshotKey(accountID, snapshotID)
if err != nil {
return err
}
_, err = s.objects.DeleteObject(ctx, &s3.DeleteObjectInput{Bucket: aws.String(s.bucket), Key: aws.String(key)})
return err
}
// ListSnapshots returns one account's snapshot documents, skipping any it
// cannot read or decode. The prefix is the isolation: no other account's
// document is read, so none has to be filtered out afterwards.
func (s *Store) ListSnapshots(ctx context.Context, accountID string) ([]Snapshot, error) {
return s.listSnapshotDocuments(ctx, accountID, true)
}
// ListSnapshotsStrict is ListSnapshots without the tolerance, for callers whose
// answer would be wrong rather than merely partial.
func (s *Store) ListSnapshotsStrict(ctx context.Context, accountID string) ([]Snapshot, error) {
return s.listSnapshotDocuments(ctx, accountID, false)
}
// ListAllSnapshots lists across every account. Whole-cluster by intent: the
// name is deliberately not ListSnapshots so no caller gets this reach by
// leaving a call site untouched.
func (s *Store) ListAllSnapshots(ctx context.Context) ([]Snapshot, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
return listDocuments(ctx, s, snapshotPrefix, "snapshot", UnmarshalSnapshot, true)
}
// ListAllSnapshotsStrict is ListAllSnapshots without the tolerance.
func (s *Store) ListAllSnapshotsStrict(ctx context.Context) ([]Snapshot, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
return listDocuments(ctx, s, snapshotPrefix, "snapshot", UnmarshalSnapshot, false)
}
func (s *Store) listSnapshotDocuments(ctx context.Context, accountID string, skipCorrupt bool) ([]Snapshot, error) {
if s == nil || s.objects == nil {
return nil, errors.New("metadata store is not configured")
}
prefix, err := accountPrefix(snapshotPrefix, accountID)
if err != nil {
return nil, err
}
return listDocuments(ctx, s, prefix, "snapshot", UnmarshalSnapshot, skipCorrupt)
}
// accountPrefix rejects the same accounts the key builders do, so a listing
// cannot widen to the whole tree by being handed an empty account.
func accountPrefix(kindPrefix, accountID string) (string, error) {
if !utils.IsAccountID(accountID) {
return "", fmt.Errorf("invalid EBS metadata account ID %q", accountID)
}
return kindPrefix + accountID + "/", nil
}
// ListAMIs returns every AMI document, skipping any it cannot decode. One
@@ -111,8 +224,9 @@ func (s *Store) listAMIDocuments(ctx context.Context, skipCorrupt bool) ([]AMI,
// Prefixes the metadata documents live under, one per document kind.
const (
volumePrefix = "spinifex/ebsmetadata/v1/volumes/"
amiPrefix = "spinifex/ebsmetadata/v1/amis/"
volumePrefix = "spinifex/ebsmetadata/v2/volumes/"
snapshotPrefix = "spinifex/ebsmetadata/v2/snapshots/"
amiPrefix = "spinifex/ebsmetadata/v2/amis/"
)
// listFetchConcurrency bounds the object fetches a single listing may have in
+140 -45
View File
@@ -5,6 +5,7 @@ import (
"context"
"errors"
"fmt"
"sort"
"strconv"
"sync"
"sync/atomic"
@@ -22,9 +23,9 @@ func TestStoreRoundTripsVolumeAndAMI(t *testing.T) {
objects := objectstore.NewMemoryObjectStore()
store := NewStore(objects, "control-plane")
volume := Volume{VolumeID: "vol-1", TenantID: "acct-1", CapacityGiB: 10, State: "available", ProviderHandle: "opaque"}
volume := Volume{VolumeID: "vol-1", TenantID: "000000000001", CapacityGiB: 10, State: "available", ProviderHandle: "opaque"}
require.NoError(t, store.PutVolume(context.Background(), volume))
gotVolume, err := store.GetVolume(context.Background(), volume.VolumeID)
gotVolume, err := store.GetVolume(context.Background(), volume.TenantID, volume.VolumeID)
require.NoError(t, err)
assert.Equal(t, volume.VolumeID, gotVolume.VolumeID)
assert.Equal(t, volume.ProviderHandle, gotVolume.ProviderHandle)
@@ -38,31 +39,31 @@ func TestStoreRoundTripsVolumeAndAMI(t *testing.T) {
assert.Equal(t, ami.SnapshotID, gotAMI.SnapshotID)
assert.Equal(t, SchemaVersion, gotAMI.SchemaVersion)
require.NoError(t, store.DeleteVolume(context.Background(), volume.VolumeID))
require.NoError(t, store.DeleteVolume(context.Background(), volume.TenantID, volume.VolumeID))
require.NoError(t, store.DeleteAMI(context.Background(), ami.ImageID))
}
func TestListVolumes_Empty(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
volumes, err := store.ListVolumes(context.Background())
volumes, err := store.ListVolumes(context.Background(), "000000000001")
require.NoError(t, err)
assert.Empty(t, volumes)
}
func TestListVolumes_ReturnsAllStoredVolumes(t *testing.T) {
func TestListAllVolumes_ReturnsEveryAccountsVolumes(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
want := []Volume{
{VolumeID: "vol-1", TenantID: "acct-1", CapacityGiB: 8, State: "available"},
{VolumeID: "vol-2", TenantID: "acct-2", CapacityGiB: 16, State: "in-use"},
{VolumeID: "vol-3", TenantID: "acct-1", CapacityGiB: 32, State: "available"},
{VolumeID: "vol-1", TenantID: "000000000001", CapacityGiB: 8, State: "available"},
{VolumeID: "vol-2", TenantID: "000000000002", CapacityGiB: 16, State: "in-use"},
{VolumeID: "vol-3", TenantID: "000000000001", CapacityGiB: 32, State: "available"},
}
for _, v := range want {
require.NoError(t, store.PutVolume(ctx, v))
}
got, err := store.ListVolumes(ctx)
got, err := store.ListAllVolumes(ctx)
require.NoError(t, err)
require.Len(t, got, len(want))
@@ -83,9 +84,9 @@ func corruptVolumeStore(t *testing.T) (*Store, context.Context) {
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-good", CapacityGiB: 1}))
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-good", TenantID: "000000000001", CapacityGiB: 1}))
key, err := VolumeKey("vol-corrupt")
key, err := VolumeKey("000000000001", "vol-corrupt")
require.NoError(t, err)
_, err = objects.PutObject(ctx, &s3.PutObjectInput{
Bucket: aws.String("control-plane"), Key: aws.String(key), Body: bytes.NewReader([]byte("not json")),
@@ -100,7 +101,7 @@ func corruptVolumeStore(t *testing.T) (*Store, context.Context) {
func TestListVolumes_SkipsCorruptObject(t *testing.T) {
store, ctx := corruptVolumeStore(t)
volumes, err := store.ListVolumes(ctx)
volumes, err := store.ListVolumes(ctx, "000000000001")
require.NoError(t, err)
require.Len(t, volumes, 1, "the readable volume must still be listed")
assert.Equal(t, "vol-good", volumes[0].VolumeID)
@@ -111,7 +112,7 @@ func TestListVolumes_SkipsCorruptObject(t *testing.T) {
func TestListVolumesStrict_CorruptObjectReturnsError(t *testing.T) {
store, ctx := corruptVolumeStore(t)
_, err := store.ListVolumesStrict(ctx)
_, err := store.ListVolumesStrict(ctx, "000000000001")
require.Error(t, err)
}
@@ -123,20 +124,20 @@ func TestListVolumes_SkipsUnreadableObject(t *testing.T) {
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-good", CapacityGiB: 1}))
key, err := VolumeKey("vol-unreadable")
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-good", TenantID: "000000000001", CapacityGiB: 1}))
key, err := VolumeKey("000000000001", "vol-unreadable")
require.NoError(t, err)
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-unreadable", CapacityGiB: 1}))
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-unreadable", TenantID: "000000000001", CapacityGiB: 1}))
failing := &getFailsStore{ObjectStore: objects, failKey: key}
broken := NewStore(failing, "control-plane")
volumes, err := broken.ListVolumes(ctx)
volumes, err := broken.ListVolumes(ctx, "000000000001")
require.NoError(t, err, "an unreadable document must not fail the whole listing")
require.Len(t, volumes, 1)
assert.Equal(t, "vol-good", volumes[0].VolumeID)
_, err = broken.ListVolumesStrict(ctx)
_, err = broken.ListVolumesStrict(ctx, "000000000001")
require.Error(t, err, "the strict listing must still report the failure")
}
@@ -159,13 +160,107 @@ func (s *getFailsStore) GetObject(ctx context.Context, in *s3.GetObjectInput) (*
// Store method: a zero-value *Store (no ObjectStore wired up) must error
// instead of panicking on a nil dereference.
func TestListVolumes_NotConfigured(t *testing.T) {
var store *Store
_, err := store.ListVolumes(context.Background())
require.Error(t, err)
ctx := context.Background()
for _, store := range []*Store{nil, {}} {
for name, list := range map[string]func() ([]Volume, error){
"ListVolumes": func() ([]Volume, error) { return store.ListVolumes(ctx, "000000000001") },
"ListVolumesStrict": func() ([]Volume, error) { return store.ListVolumesStrict(ctx, "000000000001") },
"ListAllVolumes": func() ([]Volume, error) { return store.ListAllVolumes(ctx) },
"ListAllVolumesStrict": func() ([]Volume, error) { return store.ListAllVolumesStrict(ctx) },
} {
_, err := list()
require.Error(t, err, name)
}
}
}
empty := &Store{}
_, err = empty.ListVolumes(context.Background())
require.Error(t, err)
// The account prefix is the isolation, so a listing must return one account's
// documents without reading another's.
func TestListVolumes_ScopedToOneAccount(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
for _, volume := range []Volume{
{VolumeID: "vol-a1", TenantID: "000000000001"},
{VolumeID: "vol-a2", TenantID: "000000000001"},
{VolumeID: "vol-b1", TenantID: "000000000002"},
} {
require.NoError(t, store.PutVolume(ctx, volume))
}
got, err := store.ListVolumes(ctx, "000000000001")
require.NoError(t, err)
assert.Equal(t, []string{"vol-a1", "vol-a2"}, volumeIDs(got))
got, err = store.ListVolumes(ctx, "000000000002")
require.NoError(t, err)
assert.Equal(t, []string{"vol-b1"}, volumeIDs(got))
}
// The two verbs must partition and list the same way: a key builder that wrote
// one shape and listed another would show up here and nowhere else.
func TestListAllVolumes_IsTheUnionOfEveryAccount(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
for _, volume := range []Volume{
{VolumeID: "vol-a1", TenantID: "000000000001"},
{VolumeID: "vol-b1", TenantID: "000000000002"},
{VolumeID: "vol-b2", TenantID: "000000000002"},
} {
require.NoError(t, store.PutVolume(ctx, volume))
}
all, err := store.ListAllVolumes(ctx)
require.NoError(t, err)
var union []Volume
for _, account := range []string{"000000000001", "000000000002"} {
scoped, err := store.ListVolumes(ctx, account)
require.NoError(t, err)
union = append(union, scoped...)
}
assert.Equal(t, volumeIDs(all), volumeIDs(union))
}
// An empty account must not widen a listing to every account's documents.
func TestListVolumes_RefusesANonAccountPrefix(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-a1", TenantID: "000000000001"}))
for _, account := range []string{"", "not-an-account", "vol-a1"} {
_, err := store.ListVolumes(ctx, account)
require.Error(t, err, "account %q must not list", account)
_, err = store.ListVolumesStrict(ctx, account)
require.Error(t, err, "account %q must not list strictly", account)
}
}
// The whole-cluster verbs keep the tolerant/strict split the scoped ones have.
func TestListAllVolumesStrict_FailsOnACorruptDocument(t *testing.T) {
objects := objectstore.NewMemoryObjectStore()
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-a1", TenantID: "000000000001"}))
key, err := VolumeKey("000000000002", "vol-corrupt")
require.NoError(t, err)
writeRaw(t, objects, key, []byte("{not json"))
tolerant, err := store.ListAllVolumes(ctx)
require.NoError(t, err)
assert.Equal(t, []string{"vol-a1"}, volumeIDs(tolerant))
_, err = store.ListAllVolumesStrict(ctx)
require.ErrorIs(t, err, ErrCorruptDocument)
}
func volumeIDs(volumes []Volume) []string {
ids := make([]string, 0, len(volumes))
for _, volume := range volumes {
ids = append(ids, volume.VolumeID)
}
sort.Strings(ids)
return ids
}
// TestListAMIs_Empty mirrors TestListVolumes_Empty: an AMI-less store lists
@@ -177,7 +272,7 @@ func TestListAMIs_Empty(t *testing.T) {
assert.Empty(t, amis)
}
// TestListAMIs_ReturnsAllStoredAMIs mirrors TestListVolumes_ReturnsAllStoredVolumes.
// TestListAMIs_ReturnsAllStoredAMIs mirrors TestListAllVolumes_ReturnsEveryAccountsVolumes.
func TestListAMIs_ReturnsAllStoredAMIs(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
ctx := context.Background()
@@ -220,7 +315,7 @@ func TestListAMIs_NotConfigured(t *testing.T) {
// concerned, reported as the object store's not-found rather than a zero value.
func TestGetVolume_MissingDocumentIsNotFound(t *testing.T) {
store := NewStore(objectstore.NewMemoryObjectStore(), "control-plane")
_, err := store.GetVolume(context.Background(), "vol-missing")
_, err := store.GetVolume(context.Background(), "000000000001", "vol-missing")
require.Error(t, err)
assert.True(t, objectstore.IsNoSuchKeyError(err))
}
@@ -241,14 +336,14 @@ func TestGet_CorruptDocumentIsDistinguishable(t *testing.T) {
store := NewStore(objects, "control-plane")
ctx := context.Background()
volKey, err := VolumeKey("vol-corrupt")
volKey, err := VolumeKey("000000000001", "vol-corrupt")
require.NoError(t, err)
writeRaw(t, objects, volKey, []byte("{not json"))
amiKey, err := AMIKey("ami-corrupt")
require.NoError(t, err)
writeRaw(t, objects, amiKey, []byte("{not json"))
_, err = store.GetVolume(ctx, "vol-corrupt")
_, err = store.GetVolume(ctx, "000000000001", "vol-corrupt")
require.ErrorIs(t, err, ErrCorruptDocument)
assert.False(t, objectstore.IsNoSuchKeyError(err), "corrupt must not read as absent")
@@ -302,7 +397,7 @@ func TestListVolumes_FetchesDocumentsConcurrently(t *testing.T) {
const delay = 20 * time.Millisecond
for i := range documents {
require.NoError(t, store.PutVolume(ctx, Volume{
VolumeID: fmt.Sprintf("vol-%02d", i), TenantID: "acct-1", CapacityGiB: 1,
VolumeID: fmt.Sprintf("vol-%02d", i), TenantID: "000000000001", CapacityGiB: 1,
}))
}
@@ -310,7 +405,7 @@ func TestListVolumes_FetchesDocumentsConcurrently(t *testing.T) {
measured := NewStore(slow, "control-plane")
start := time.Now()
volumes, err := measured.ListVolumes(ctx)
volumes, err := measured.ListVolumes(ctx, "000000000001")
elapsed := time.Since(start)
require.NoError(t, err)
@@ -338,8 +433,8 @@ func TestListVolumes_ConcurrentFetchPreservesListingOrder(t *testing.T) {
keys := make([]string, 0, documents)
for i := range documents {
id := fmt.Sprintf("vol-%02d", i)
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: id, TenantID: "acct-1", CapacityGiB: 1}))
key, err := VolumeKey(id)
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: id, TenantID: "000000000001", CapacityGiB: 1}))
key, err := VolumeKey("000000000001", id)
require.NoError(t, err)
want = append(want, id)
keys = append(keys, key)
@@ -347,7 +442,7 @@ func TestListVolumes_ConcurrentFetchPreservesListingOrder(t *testing.T) {
fixed := NewStore(&fixedOrderStore{ObjectStore: objects, keys: keys}, "control-plane")
for range 5 {
volumes, err := fixed.ListVolumes(ctx)
volumes, err := fixed.ListVolumes(ctx, "000000000001")
require.NoError(t, err)
got := make([]string, 0, len(volumes))
for _, v := range volumes {
@@ -429,15 +524,15 @@ func TestListVolumes_BoundsOneSlowDocument(t *testing.T) {
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-good", CapacityGiB: 1}))
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-slow", CapacityGiB: 1}))
key, err := VolumeKey("vol-slow")
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-good", TenantID: "000000000001", CapacityGiB: 1}))
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-slow", TenantID: "000000000001", CapacityGiB: 1}))
key, err := VolumeKey("000000000001", "vol-slow")
require.NoError(t, err)
hanging := NewStore(&hangingGetStore{ObjectStore: objects, hangKey: key}, "control-plane")
start := time.Now()
volumes, err := hanging.ListVolumes(ctx)
volumes, err := hanging.ListVolumes(ctx, "000000000001")
elapsed := time.Since(start)
require.NoError(t, err, "one slow document must not fail the whole listing")
@@ -445,7 +540,7 @@ func TestListVolumes_BoundsOneSlowDocument(t *testing.T) {
assert.Equal(t, "vol-good", volumes[0].VolumeID)
assert.Less(t, elapsed, time.Second, "the listing must not wait on the slow document indefinitely")
_, err = hanging.ListVolumesStrict(ctx)
_, err = hanging.ListVolumesStrict(ctx, "000000000001")
require.Error(t, err, "the strict listing must still report the document it could not read")
}
@@ -462,8 +557,8 @@ func TestListVolumesFollowsEveryPage(t *testing.T) {
keys := make([]string, 0, documents)
for i := range documents {
id := fmt.Sprintf("vol-%02d", i)
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: id, TenantID: "acct-1", CapacityGiB: 1}))
key, err := VolumeKey(id)
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: id, TenantID: "000000000001", CapacityGiB: 1}))
key, err := VolumeKey("000000000001", id)
require.NoError(t, err)
want = append(want, id)
keys = append(keys, key)
@@ -478,7 +573,7 @@ func TestListVolumesFollowsEveryPage(t *testing.T) {
if strict {
list = paged.ListVolumesStrict
}
volumes, err := list(ctx)
volumes, err := list(ctx, "000000000001")
require.NoError(t, err)
got := make([]string, 0, len(volumes))
@@ -498,17 +593,17 @@ func TestListVolumesRefusesTruncationItCannotFollow(t *testing.T) {
store := NewStore(objects, "control-plane")
ctx := context.Background()
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-1", TenantID: "acct-1", CapacityGiB: 1}))
key, err := VolumeKey("vol-1")
require.NoError(t, store.PutVolume(ctx, Volume{VolumeID: "vol-1", TenantID: "000000000001", CapacityGiB: 1}))
key, err := VolumeKey("000000000001", "vol-1")
require.NoError(t, err)
broken := NewStore(&truncatedNoTokenStore{ObjectStore: objects, keys: []string{key}}, "control-plane")
_, err = broken.ListVolumes(ctx)
_, err = broken.ListVolumes(ctx, "000000000001")
require.Error(t, err)
assert.Contains(t, err.Error(), "continuation token")
_, err = broken.ListVolumesStrict(ctx)
_, err = broken.ListVolumesStrict(ctx, "000000000001")
require.Error(t, err)
}
@@ -8,29 +8,32 @@ import (
"github.com/mulgadc/spinifex/spinifex/awserrors"
"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/utils"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// putSourceVolumeAMI registers an AMI document plus, when volumeID is set, the
// snapshot metadata recording which volume the snapshot was taken from.
func putSourceVolumeAMI(t *testing.T, svc *ImageServiceImpl, store *objectstore.MemoryObjectStore, ami ebsmetadata.AMI, volumeID string) {
// snapshot document recording which volume the snapshot was taken from. The
// document is keyed under the account the AMI's owner alias derives to, which
// is what GetAMISourceVolumeID has to reproduce to find it.
func putSourceVolumeAMI(t *testing.T, svc *ImageServiceImpl, ami ebsmetadata.AMI, volumeID string) {
t.Helper()
require.NoError(t, svc.MetadataStore().PutAMI(context.Background(), ami))
if volumeID == "" {
return
}
require.NoError(t, handlers_ec2_snapshot.WriteSnapshotConfig(store, testBucket, ami.SnapshotID,
&handlers_ec2_snapshot.SnapshotConfig{SnapshotID: ami.SnapshotID, VolumeID: volumeID}))
owner, _ := snapshotAccountForAMI(ami)
require.NoError(t, svc.MetadataStore().PutSnapshot(context.Background(), ebsmetadata.Snapshot{
SnapshotID: ami.SnapshotID, VolumeID: volumeID, OwnerID: owner,
}))
}
// TestGetAMISourceVolumeID_ReadsSnapshotMetadata locks the normal case: the
// source volume comes from the snapshot's own metadata.json.
// source volume comes from the snapshot's own document.
func TestGetAMISourceVolumeID_ReadsSnapshotMetadata(t *testing.T) {
svc, store := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, store, ebsmetadata.AMI{
svc, _ := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, ebsmetadata.AMI{
ImageID: "ami-src01", SnapshotID: "snap-src01", ImageOwnerAlias: testAccountID,
}, "vol-origin")
@@ -42,8 +45,8 @@ func TestGetAMISourceVolumeID_ReadsSnapshotMetadata(t *testing.T) {
// TestGetAMISourceVolumeID_BundledSystemAMI locks the fallback for bundled
// system AMIs, whose snapshot is named after the AMI and carries no metadata.json.
func TestGetAMISourceVolumeID_BundledSystemAMI(t *testing.T) {
svc, store := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, store, ebsmetadata.AMI{
svc, _ := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, ebsmetadata.AMI{
ImageID: "ami-sys01", SnapshotID: "snap-ami-sys01", ImageOwnerAlias: "system",
}, "")
@@ -57,8 +60,8 @@ func TestGetAMISourceVolumeID_BundledSystemAMI(t *testing.T) {
// document, so a caller relying on it (e.g. a stale catalog import predating
// this fix) must be able to spot it in the logs rather than launch silently.
func TestGetAMISourceVolumeID_BundledSystemAMI_LogsFallbackWarning(t *testing.T) {
svc, store := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, store, ebsmetadata.AMI{
svc, _ := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, ebsmetadata.AMI{
ImageID: "ami-sys02", SnapshotID: "snap-ami-sys02", ImageOwnerAlias: "system",
}, "")
@@ -76,11 +79,49 @@ func TestGetAMISourceVolumeID_BundledSystemAMI_LogsFallbackWarning(t *testing.T)
assert.Contains(t, logs, "ami-sys02")
}
// TestGetAMISourceVolumeID_ImportedSystemAMI locks that a system AMI whose
// snapshot document exists under the global account resolves through it rather
// than taking the bundled fallback. The bundled path logs a warn; a wrongly
// derived account here would silently return an image ID where a volume ID
// belongs, and the launch would clone the wrong blocks.
func TestGetAMISourceVolumeID_ImportedSystemAMI(t *testing.T) {
svc, _ := setupProviderImageService(t)
require.NoError(t, svc.MetadataStore().PutAMI(context.Background(), ebsmetadata.AMI{
ImageID: "ami-sysimp01", SnapshotID: "snap-sysimp01", ImageOwnerAlias: "system",
}))
// The owner is spelled out rather than derived, reproducing what
// registerImportedAMISnapshot writes. Deriving it here would let the
// fixture follow the read path and pin nothing.
require.NoError(t, svc.MetadataStore().PutSnapshot(context.Background(), ebsmetadata.Snapshot{
SnapshotID: "snap-sysimp01", VolumeID: "vol-imported", OwnerID: utils.GlobalAccountID,
}))
got, err := svc.GetAMISourceVolumeID(context.Background(), "ami-sysimp01")
require.NoError(t, err)
assert.Equal(t, "vol-imported", got, "an imported system AMI must resolve its recorded source volume, not fall back")
}
// TestGetAMISourceVolumeID_EmptyOwnerAliasDoesNotFallBack locks that an AMI with
// no owner alias is treated as corrupt rather than as a system image. Deriving
// it to the global account and letting the bundled fallback fire would return
// the image ID where a volume ID belongs, and the launch would clone holes.
func TestGetAMISourceVolumeID_EmptyOwnerAliasDoesNotFallBack(t *testing.T) {
svc, _ := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, ebsmetadata.AMI{
ImageID: "ami-noalias", SnapshotID: "snap-noalias", ImageOwnerAlias: "",
}, "")
got, err := svc.GetAMISourceVolumeID(context.Background(), "ami-noalias")
require.Error(t, err)
assert.Empty(t, got, "a corrupt AMI must not resolve to its own image ID")
assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, err.Error())
}
// TestGetAMISourceVolumeID_AccountAMIMissingSnapshotMetadata locks that the
// bundled fallback does not apply to account-owned AMIs.
func TestGetAMISourceVolumeID_AccountAMIMissingSnapshotMetadata(t *testing.T) {
svc, store := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, store, ebsmetadata.AMI{
svc, _ := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, ebsmetadata.AMI{
ImageID: "ami-acct01", SnapshotID: "snap-acct01", ImageOwnerAlias: testAccountID,
}, "")
@@ -92,8 +133,8 @@ func TestGetAMISourceVolumeID_AccountAMIMissingSnapshotMetadata(t *testing.T) {
// TestGetAMISourceVolumeID_AMIWithoutSnapshot locks that an AMI carrying no
// snapshot reference is reported as not found rather than resolving to "".
func TestGetAMISourceVolumeID_AMIWithoutSnapshot(t *testing.T) {
svc, store := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, store, ebsmetadata.AMI{
svc, _ := setupProviderImageService(t)
putSourceVolumeAMI(t, svc, ebsmetadata.AMI{
ImageID: "ami-nosnap", ImageOwnerAlias: testAccountID,
}, "")
@@ -114,12 +155,13 @@ func TestGetAMISourceVolumeID_UnknownAMI(t *testing.T) {
// TestGetAMISourceVolumeID_EmptySnapshotSourceVolume locks that corrupt
// snapshot metadata naming no source volume fails instead of returning "".
func TestGetAMISourceVolumeID_EmptySnapshotSourceVolume(t *testing.T) {
svc, store := setupProviderImageService(t)
svc, _ := setupProviderImageService(t)
require.NoError(t, svc.MetadataStore().PutAMI(context.Background(), ebsmetadata.AMI{
ImageID: "ami-empty", SnapshotID: "snap-empty", ImageOwnerAlias: testAccountID,
}))
require.NoError(t, handlers_ec2_snapshot.WriteSnapshotConfig(store, testBucket, "snap-empty",
&handlers_ec2_snapshot.SnapshotConfig{SnapshotID: "snap-empty"}))
require.NoError(t, svc.MetadataStore().PutSnapshot(context.Background(), ebsmetadata.Snapshot{
SnapshotID: "snap-empty", OwnerID: testAccountID,
}))
_, err := svc.GetAMISourceVolumeID(context.Background(), "ami-empty")
require.Error(t, err)
@@ -77,7 +77,7 @@ func TestGetVolumeMetadata_Provider_ResolvesProviderCreatedVolume(t *testing.T)
svc, _, provider := setupProviderSnapshotImageService(t)
doc := seedProviderVolume(t, svc, provider, "vol-provider001", 10)
meta, err := svc.getVolumeMetadata(context.Background(), doc.VolumeID)
meta, err := svc.getVolumeMetadata(context.Background(), testAccountID, doc.VolumeID)
require.NoError(t, err, "a provider-created volume with no config.json must resolve")
assert.Equal(t, doc.VolumeID, meta.VolumeID)
assert.Equal(t, testAccountID, meta.TenantID)
@@ -91,7 +91,7 @@ func TestGetVolumeMetadata_Provider_ResolvesProviderCreatedVolume(t *testing.T)
func TestGetVolumeMetadata_Provider_UnknownVolumeIsNotFound(t *testing.T) {
svc, _, _ := setupProviderSnapshotImageService(t)
_, err := svc.getVolumeMetadata(context.Background(), "vol-does-not-exist")
_, err := svc.getVolumeMetadata(context.Background(), testAccountID, "vol-does-not-exist")
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, err.Error())
}
@@ -104,7 +104,7 @@ func TestSnapshotStoppedVolume_Provider_CreatesThroughProvider(t *testing.T) {
svc, _, provider := setupProviderSnapshotImageService(t)
doc := seedProviderVolume(t, svc, provider, "vol-stopped001", 8)
err := svc.snapshotStoppedVolume(doc.VolumeID, "snap-stopped001")
err := svc.snapshotStoppedVolume(doc.VolumeID, "snap-stopped001", testAccountID)
require.NoError(t, err, "the provider path must not fall through to the embedded engine against the closed Predastore port")
// CreateSnapshot is idempotent on (SnapshotID, VolumeID): re-issuing it
+55 -36
View File
@@ -372,14 +372,14 @@ func (s *ImageServiceImpl) CreateImageFromInstance(params CreateImageParams, acc
if params.IsRunning {
snapshotErr = s.snapshotRunningVolume(params.RootVolumeID, snapshotID, accountID)
} else {
snapshotErr = s.snapshotStoppedVolume(params.RootVolumeID, snapshotID)
snapshotErr = s.snapshotStoppedVolume(params.RootVolumeID, snapshotID, accountID)
}
if snapshotErr != nil {
return nil, snapshotErr
}
// Step 2: Read source volume config for size
volMeta, err := s.getVolumeMetadata(context.Background(), params.RootVolumeID)
volMeta, err := s.getVolumeMetadata(context.Background(), accountID, params.RootVolumeID)
if err != nil {
slog.Error("CreateImageFromInstance: failed to read volume metadata", "volumeId", params.RootVolumeID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
@@ -449,7 +449,7 @@ func (s *ImageServiceImpl) snapshotRunningVolume(volumeID, snapshotID, accountID
return errors.New(awserrors.ErrorServerInternal)
}
volMeta, err := s.getVolumeMetadata(context.Background(), volumeID)
volMeta, err := s.getVolumeMetadata(context.Background(), accountID, volumeID)
if err != nil {
slog.Error("snapshotRunningVolume: failed to read volume metadata", "volumeId", volumeID, "err", err)
return errors.New(awserrors.ErrorServerInternal)
@@ -472,12 +472,12 @@ func (s *ImageServiceImpl) snapshotRunningVolume(volumeID, snapshotID, accountID
// volume is stopped/detached, so nothing is buffered on a serving node.
// viperblockd's handleCreateSnapshot prefers a live mounted engine over opening
// a second one, so it handles the mounted case itself.
func (s *ImageServiceImpl) snapshotStoppedVolume(volumeID, snapshotID string) error {
func (s *ImageServiceImpl) snapshotStoppedVolume(volumeID, snapshotID, accountID string) error {
if s.config == nil {
return errors.New(awserrors.ErrorServerInternal)
}
volMeta, err := s.getVolumeMetadata(context.Background(), volumeID)
volMeta, err := s.getVolumeMetadata(context.Background(), accountID, volumeID)
if err != nil {
slog.Error("snapshotStoppedVolume: failed to read volume metadata", "volumeId", volumeID, "err", err)
return errors.New(awserrors.ErrorServerInternal)
@@ -506,8 +506,8 @@ func (s *ImageServiceImpl) createSnapshot(ctx context.Context, volMeta ebsmetada
// getVolumeMetadata reads a volume's control-plane document, remapping a
// missing document to InvalidVolume.NotFound.
func (s *ImageServiceImpl) getVolumeMetadata(ctx context.Context, volumeID string) (ebsmetadata.Volume, error) {
meta, err := s.metadata.GetVolume(ctx, volumeID)
func (s *ImageServiceImpl) getVolumeMetadata(ctx context.Context, accountID, volumeID string) (ebsmetadata.Volume, error) {
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
return ebsmetadata.Volume{}, errors.New(awserrors.ErrorInvalidVolumeNotFound)
@@ -539,9 +539,21 @@ func (s *ImageServiceImpl) GetAMIConfig(ctx context.Context, imageID string) (eb
return s.metadata.GetAMI(ctx, imageID)
}
// snapshotAccountForAMI derives the account an AMI's snapshot document is keyed
// under: an account-ID alias keys under itself, a system alias under the global
// account. derived reports which, so the bundled-AMI fallback below cannot
// drift from this predicate. An empty alias is a corrupt document rather than a
// system image, so it derives nothing and the fallback cannot answer for it.
func snapshotAccountForAMI(meta ebsmetadata.AMI) (accountID string, derived bool) {
if utils.IsAccountID(meta.ImageOwnerAlias) {
return meta.ImageOwnerAlias, false
}
return utils.GlobalAccountID, meta.ImageOwnerAlias != ""
}
// GetAMISourceVolumeID returns the volume whose blocks imageID's snapshot
// references, read from the snapshot's metadata.json. Bundled system AMIs have
// no standalone snapshot metadata; their snapshot is named after the AMI.
// references, read from the snapshot's document. Bundled system AMIs have no
// standalone snapshot metadata; their snapshot is named after the AMI.
func (s *ImageServiceImpl) GetAMISourceVolumeID(ctx context.Context, imageID string) (string, error) {
meta, err := s.GetAMIConfig(ctx, imageID)
if err != nil {
@@ -555,14 +567,19 @@ func (s *ImageServiceImpl) GetAMISourceVolumeID(ctx context.Context, imageID str
return "", errors.New(awserrors.ErrorInvalidAMIIDNotFound)
}
snapCfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(ctx, s.store, s.bucketName, meta.SnapshotID)
// The snapshot is keyed under its own owner, which for a system AMI is the
// global account rather than the alias and rather than the caller. Falling
// back on the caller's miss would silently return an image ID where a
// volume ID belongs for an imported system AMI.
snapAccount, derived := snapshotAccountForAMI(meta)
snapCfg, err := s.metadata.GetSnapshot(ctx, snapAccount, meta.SnapshotID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) && meta.ImageOwnerAlias != "" && !utils.IsAccountID(meta.ImageOwnerAlias) {
if objectstore.IsNoSuchKeyError(err) && derived {
slog.WarnContext(ctx, "GetAMISourceVolumeID: system AMI has no snapshot metadata document, falling back to imageID as volume ID",
"imageId", imageID, "snapshotId", meta.SnapshotID, "imageOwnerAlias", meta.ImageOwnerAlias)
return imageID, nil
}
if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) {
if objectstore.IsNoSuchKeyError(err) || errors.Is(err, ebsmetadata.ErrCorruptDocument) {
return "", errors.New(awserrors.ErrorInvalidSnapshotNotFound)
}
slog.ErrorContext(ctx, "GetAMISourceVolumeID: failed to read snapshot metadata",
@@ -696,9 +713,10 @@ func (s *ImageServiceImpl) loadAMIForMutation(ctx context.Context, imageID, acco
return meta, nil
}
// putSnapshotMetadata stores snapshot metadata in S3 using the canonical SnapshotConfig type.
// putSnapshotMetadata stores the snapshot's control-plane document under the
// account that owns it.
func (s *ImageServiceImpl) putSnapshotMetadata(ctx context.Context, snapshotID, volumeID string, volumeSizeGiB uint64, accountID string) error {
cfg := handlers_ec2_snapshot.SnapshotConfig{
return s.metadata.PutSnapshot(ctx, ebsmetadata.Snapshot{
SnapshotID: snapshotID,
VolumeID: volumeID,
VolumeSize: safecast.Uint64ToInt64(volumeSizeGiB),
@@ -706,8 +724,7 @@ func (s *ImageServiceImpl) putSnapshotMetadata(ctx context.Context, snapshotID,
Progress: "100%",
StartTime: time.Now(),
OwnerID: accountID,
}
return handlers_ec2_snapshot.WriteSnapshotConfig(s.store, s.bucketName, snapshotID, &cfg)
})
}
// CopyImage clones an AMI same-region, metadata-only: the new snapshot shares the
@@ -742,17 +759,20 @@ func (s *ImageServiceImpl) CopyImage(ctx context.Context, input *ec2.CopyImageIn
if srcMeta.SnapshotID == "" {
return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
}
srcSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(ctx, s.store, s.bucketName, srcMeta.SnapshotID)
// Read the source snapshot under its own owner, not the caller's: a system
// AMI's snapshot is keyed under the global account.
srcSnapAccount, srcSnapDerived := snapshotAccountForAMI(srcMeta)
srcSnap, err := s.metadata.GetSnapshot(ctx, srcSnapAccount, srcMeta.SnapshotID)
if err != nil {
// Bundled system AMIs have no standalone snap-xxx/metadata.json; synthesize
// a minimal snap view using VolumeID = sourceImageID so CopyImage succeeds.
if objectstore.IsNoSuchKeyError(err) && srcMeta.ImageOwnerAlias != "" && !utils.IsAccountID(srcMeta.ImageOwnerAlias) {
srcSnap = &handlers_ec2_snapshot.SnapshotConfig{
// Bundled system AMIs have no standalone snapshot document; synthesize a
// minimal view using VolumeID = sourceImageID so CopyImage succeeds.
if objectstore.IsNoSuchKeyError(err) && srcSnapDerived {
srcSnap = ebsmetadata.Snapshot{
SnapshotID: srcMeta.SnapshotID,
VolumeID: sourceImageID,
VolumeSize: safecast.Uint64ToInt64(srcMeta.VolumeSizeGiB),
}
} else if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) {
} else if objectstore.IsNoSuchKeyError(err) || errors.Is(err, ebsmetadata.ErrCorruptDocument) {
return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
} else {
slog.ErrorContext(ctx, "CopyImage: failed to read source snapshot metadata",
@@ -816,11 +836,7 @@ func (s *ImageServiceImpl) CopyImage(ctx context.Context, input *ec2.CopyImageIn
slog.ErrorContext(ctx, "CopyImage: failed to write AMI config",
"amiId", newImageID, "orphanSnapshotId", newSnapshotID, "err", err)
// Best-effort rollback of the orphaned snapshot metadata.
snapKey := handlers_ec2_snapshot.GetSnapshotKey(newSnapshotID)
if _, delErr := s.store.DeleteObject(ctx, &s3.DeleteObjectInput{
Bucket: aws.String(s.bucketName),
Key: aws.String(snapKey),
}); delErr != nil {
if delErr := s.metadata.DeleteSnapshot(ctx, accountID, newSnapshotID); delErr != nil {
slog.ErrorContext(ctx, "CopyImage: failed to roll back orphaned snapshot metadata",
"snapshotId", newSnapshotID, "err", delErr)
}
@@ -937,6 +953,13 @@ func (s *ImageServiceImpl) RegisterImage(ctx context.Context, input *ec2.Registe
return nil, errors.New(awserrors.ErrorMissingParameter)
}
// The snapshot is read under the caller's account, so an untenanted caller
// has no prefix to read from. Refuse at the boundary rather than in the key
// builder, where the rejection would surface as an internal error.
if accountID == "" {
return nil, errors.New(awserrors.ErrorAuthFailure)
}
name := *input.Name
rootBDM := pickRootSnapshotBDM(input.BlockDeviceMappings, input.RootDeviceName)
@@ -945,23 +968,19 @@ func (s *ImageServiceImpl) RegisterImage(ctx context.Context, input *ec2.Registe
}
snapshotID := *rootBDM.Ebs.SnapshotId
snapCfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(ctx, s.store, s.bucketName, snapshotID)
// The snapshot arrives through a caller-supplied BlockDeviceMapping, not
// through an AMI, so there is no owner alias to derive from: it is read
// under the caller's own account and the key is the ownership check.
snapCfg, err := s.metadata.GetSnapshot(ctx, accountID, snapshotID)
if err != nil {
// Corrupt snapshot is surfaced as NotFound, same as CopyImage.
if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) {
if objectstore.IsNoSuchKeyError(err) || errors.Is(err, ebsmetadata.ErrCorruptDocument) {
return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
}
slog.ErrorContext(ctx, "RegisterImage: failed to read snapshot metadata", "snapshotId", snapshotID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
// Only the snapshot owner (or any caller for system snapshots) can register.
if utils.IsAccountID(snapCfg.OwnerID) && snapCfg.OwnerID != accountID {
slog.WarnContext(ctx, "RegisterImage: rejected cross-account snapshot",
"snapshotId", snapshotID, "snapshotOwner", snapCfg.OwnerID, "accountId", accountID)
return nil, errors.New(awserrors.ErrorUnauthorizedOperation)
}
snapSizeGiB := uint64(0)
if snapCfg.VolumeSize > 0 {
snapSizeGiB = uint64(snapCfg.VolumeSize)
@@ -2,7 +2,6 @@ package handlers_ec2_image
import (
"context"
"encoding/json"
"io"
"log/slog"
"net/http"
@@ -17,8 +16,9 @@ import (
"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/mulgadc/spinifex/spinifex/utils"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@@ -64,6 +64,11 @@ func amiDocumentKey(t *testing.T, imageID string) string {
// putVolumeDocument writes a volume's control-plane document.
func putVolumeDocument(t *testing.T, store *objectstore.MemoryObjectStore, volume ebsmetadata.Volume) {
t.Helper()
// The owning account is a key segment, so a fixture that does not care
// which account owns it still has to have one.
if volume.TenantID == "" {
volume.TenantID = testAccountID
}
require.NoError(t, ebsmetadata.NewStore(store, testBucket).PutVolume(context.Background(), volume))
}
@@ -275,7 +280,7 @@ func TestGetVolumeMetadata(t *testing.T) {
createTestVolumeConfig(t, store, "vol-abc123", 20)
meta, err := svc.getVolumeMetadata(context.Background(), "vol-abc123")
meta, err := svc.getVolumeMetadata(context.Background(), testAccountID, "vol-abc123")
require.NoError(t, err)
assert.Equal(t, uint64(20), meta.CapacityGiB)
}
@@ -283,7 +288,7 @@ func TestGetVolumeMetadata(t *testing.T) {
func TestGetVolumeMetadata_NotFound(t *testing.T) {
svc, _ := setupTestImageService(t)
_, err := svc.getVolumeMetadata(context.Background(), "vol-nonexistent")
_, err := svc.getVolumeMetadata(context.Background(), testAccountID, "vol-nonexistent")
require.Error(t, err)
}
@@ -314,16 +319,8 @@ func TestPutSnapshotMetadata(t *testing.T) {
err := svc.putSnapshotMetadata(context.Background(), "snap-abc123", "vol-xyz789", 10, testAccountID)
require.NoError(t, err)
// Verify the metadata was written correctly
result, err := store.GetObject(context.Background(), &awss3.GetObjectInput{
Bucket: aws.String(testBucket),
Key: aws.String("snap-abc123/metadata.json"),
})
require.NoError(t, err)
defer result.Body.Close()
var cfg handlers_ec2_snapshot.SnapshotConfig
err = json.NewDecoder(result.Body).Decode(&cfg)
// The document must land under the owning account's prefix.
cfg, err := ebsmetadata.NewStore(store, testBucket).GetSnapshot(context.Background(), testAccountID, "snap-abc123")
require.NoError(t, err)
assert.Equal(t, "snap-abc123", cfg.SnapshotID)
assert.Equal(t, "vol-xyz789", cfg.VolumeID)
@@ -590,10 +587,7 @@ func TestDeregisterImage_DoesNotTouchSnapshot(t *testing.T) {
require.NoError(t, err)
// Snapshot metadata still present
_, snapErr := store.GetObject(context.Background(), &awss3.GetObjectInput{
Bucket: aws.String(testBucket),
Key: aws.String(snapID + "/metadata.json"),
})
_, snapErr := ebsmetadata.NewStore(store, testBucket).GetSnapshot(context.Background(), testAccountID, snapID)
require.NoError(t, snapErr)
}
@@ -835,7 +829,7 @@ func TestGetAMIConfig_InvalidJSON(t *testing.T) {
func TestGetVolumeMetadata_InvalidJSON(t *testing.T) {
svc, store := setupTestImageService(t)
key, err := ebsmetadata.VolumeKey("vol-badjson")
key, err := ebsmetadata.VolumeKey(testAccountID, "vol-badjson")
require.NoError(t, err)
_, err = store.PutObject(context.Background(), &awss3.PutObjectInput{
Bucket: aws.String(testBucket),
@@ -844,7 +838,7 @@ func TestGetVolumeMetadata_InvalidJSON(t *testing.T) {
})
require.NoError(t, err)
_, err = svc.getVolumeMetadata(context.Background(), "vol-badjson")
_, err = svc.getVolumeMetadata(context.Background(), testAccountID, "vol-badjson")
assert.Error(t, err)
}
@@ -1146,28 +1140,32 @@ func TestDescribeImages_FilterNoFilters(t *testing.T) {
// --- RegisterImage tests ---
// putTestSnapshotConfig writes a SnapshotConfig at {snapshotID}/metadata.json,
// putTestSnapshotConfig writes a snapshot document under its owning account,
// matching the layout that the snapshot service uses.
func putTestSnapshotConfig(t *testing.T, store *objectstore.MemoryObjectStore, snapshotID string, sizeGiB int64, ownerID string) {
t.Helper()
cfg := handlers_ec2_snapshot.SnapshotConfig{
putTestSnapshotDoc(t, store, ebsmetadata.Snapshot{
SnapshotID: snapshotID,
VolumeID: "vol-source-" + snapshotID,
VolumeSize: sizeGiB,
State: "completed",
Progress: "100%",
OwnerID: ownerID,
}
data, err := json.Marshal(cfg)
require.NoError(t, err)
_, err = store.PutObject(context.Background(), &awss3.PutObjectInput{
Bucket: aws.String(testBucket),
Key: aws.String(snapshotID + "/metadata.json"),
Body: strings.NewReader(string(data)),
ContentType: aws.String("application/json"),
})
}
// putTestSnapshotDoc writes one snapshot document through the metadata store.
func putTestSnapshotDoc(t *testing.T, store *objectstore.MemoryObjectStore, snapshot ebsmetadata.Snapshot) {
t.Helper()
require.NoError(t, ebsmetadata.NewStore(store, testBucket).PutSnapshot(context.Background(), snapshot))
}
// readTestSnapshotDoc reads a snapshot document from the account that owns it.
func readTestSnapshotDoc(t *testing.T, store *objectstore.MemoryObjectStore, accountID, snapshotID string) ebsmetadata.Snapshot {
t.Helper()
snapshot, err := ebsmetadata.NewStore(store, testBucket).GetSnapshot(context.Background(), accountID, snapshotID)
require.NoError(t, err)
return snapshot
}
func validRegisterImageServiceInput(snapshotID string) *ec2.RegisterImageInput {
@@ -1230,6 +1228,8 @@ func TestRegisterImage_SnapshotNotFound(t *testing.T) {
assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, err.Error())
}
// A snapshot outside the caller's prefix is not found rather than refused, so
// the endpoint does not confirm another account's snapshot IDs.
func TestRegisterImage_CrossAccountSnapshot(t *testing.T) {
svc, store := setupTestImageService(t)
@@ -1237,23 +1237,37 @@ func TestRegisterImage_CrossAccountSnapshot(t *testing.T) {
_, err := svc.RegisterImage(context.Background(), validRegisterImageServiceInput("snap-other01"), testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error())
assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, err.Error())
}
func TestRegisterImage_SystemSnapshotAllowed(t *testing.T) {
// An untenanted caller is refused at the boundary. Asserting that nothing was
// read is what stops the guard being deleted later as redundant with the key
// builder, whose rejection is an internal error rather than an AuthFailure.
func TestRegisterImage_EmptyCallerAccountIsRefusedAtTheBoundary(t *testing.T) {
objects := recordingstore.New()
svc := NewImageServiceImplWithStore(objects, testBucket)
_, err := svc.RegisterImage(context.Background(), validRegisterImageServiceInput("snap-guard01"), "")
require.Error(t, err)
assert.Equal(t, awserrors.ErrorAuthFailure, err.Error())
assert.Empty(t, objects.Gets(), "the guard must fire before any read")
assert.Empty(t, objects.ListPrefixes(), "the guard must fire before any listing")
}
// The system account is an ordinary account: it registers against its own
// snapshot with no special path.
func TestRegisterImage_SystemAccountSnapshot(t *testing.T) {
svc, store := setupTestImageService(t)
// System-owned snapshot (non-account-ID owner) is launchable by anyone,
// matching how system AMIs work.
putTestSnapshotConfig(t, store, "snap-sys01", 8, "spinifex")
putTestSnapshotConfig(t, store, "snap-sys01", 8, utils.GlobalAccountID)
out, err := svc.RegisterImage(context.Background(), validRegisterImageServiceInput("snap-sys01"), testAccountID)
out, err := svc.RegisterImage(context.Background(), validRegisterImageServiceInput("snap-sys01"), utils.GlobalAccountID)
require.NoError(t, err)
require.NotNil(t, out.ImageId)
meta, err := svc.GetAMIConfig(context.Background(), *out.ImageId)
require.NoError(t, err)
assert.Equal(t, testAccountID, meta.ImageOwnerAlias)
assert.Equal(t, utils.GlobalAccountID, meta.ImageOwnerAlias)
}
func TestRegisterImage_ArchitectureAndVirtualizationDefaults(t *testing.T) {
@@ -1518,12 +1532,14 @@ func readAMIConfigBytes(t *testing.T, store *objectstore.MemoryObjectStore, imag
return data
}
// readSnapshotConfigBytes returns the raw bytes of {snapshotID}/metadata.json.
func readSnapshotConfigBytes(t *testing.T, store *objectstore.MemoryObjectStore, snapshotID string) []byte {
// readSnapshotConfigBytes returns the raw bytes of the snapshot's document.
func readSnapshotConfigBytes(t *testing.T, store *objectstore.MemoryObjectStore, accountID, snapshotID string) []byte {
t.Helper()
key, err := ebsmetadata.SnapshotKey(accountID, snapshotID)
require.NoError(t, err)
result, err := store.GetObject(context.Background(), &awss3.GetObjectInput{
Bucket: aws.String(testBucket),
Key: aws.String(snapshotID + "/metadata.json"),
Key: aws.String(key),
})
require.NoError(t, err)
defer result.Body.Close()
@@ -1563,23 +1579,17 @@ func putTestAMIConfigWithSnapshot(t *testing.T, store *objectstore.MemoryObjectS
// end-to-end.
func seedCopyableAMI(t *testing.T, store *objectstore.MemoryObjectStore, imageID, name, owner, snapshotID, volumeID string, sizeGiB int64) {
t.Helper()
cfg := handlers_ec2_snapshot.SnapshotConfig{
// A system AMI's owner is an alias rather than an account, and its snapshot
// is keyed under the global account. Mirror the service's derivation.
snapOwner, _ := snapshotAccountForAMI(ebsmetadata.AMI{ImageOwnerAlias: owner})
putTestSnapshotDoc(t, store, ebsmetadata.Snapshot{
SnapshotID: snapshotID,
VolumeID: volumeID,
VolumeSize: sizeGiB,
State: "completed",
Progress: "100%",
OwnerID: owner,
}
data, err := json.Marshal(cfg)
require.NoError(t, err)
_, err = store.PutObject(context.Background(), &awss3.PutObjectInput{
Bucket: aws.String(testBucket),
Key: aws.String(snapshotID + "/metadata.json"),
Body: strings.NewReader(string(data)),
ContentType: aws.String("application/json"),
OwnerID: snapOwner,
})
require.NoError(t, err)
putTestAMIConfigWithSnapshot(t, store, imageID, name, owner, snapshotID, ebsmetadata.AMI{
VolumeSizeGiB: uint64(sizeGiB),
@@ -1603,7 +1613,7 @@ func TestCopyImage_HappyPath(t *testing.T) {
// Capture the source config bytes before the copy so we can prove the
// source wasn't mutated (not just that name still resolves).
srcBefore := readAMIConfigBytes(t, store, "ami-src001")
srcSnapBefore := readSnapshotConfigBytes(t, store, "snap-src001")
srcSnapBefore := readSnapshotConfigBytes(t, store, testAccountID, "snap-src001")
out, err := svc.CopyImage(context.Background(), validCopyImageServiceInput("ami-src001", "copy-of-source"), testAccountID)
require.NoError(t, err)
@@ -1625,7 +1635,7 @@ func TestCopyImage_HappyPath(t *testing.T) {
// Source AMI config and source snapshot metadata must be byte-identical.
assert.Equal(t, srcBefore, readAMIConfigBytes(t, store, "ami-src001"),
"source AMI config was mutated by CopyImage")
assert.Equal(t, srcSnapBefore, readSnapshotConfigBytes(t, store, "snap-src001"),
assert.Equal(t, srcSnapBefore, readSnapshotConfigBytes(t, store, testAccountID, "snap-src001"),
"source snapshot metadata was mutated by CopyImage")
}
@@ -1633,19 +1643,10 @@ func TestCopyImage_InheritsSourceFields(t *testing.T) {
svc, store := setupTestImageService(t)
// Seed snapshot for the source AMI.
_, err := store.PutObject(context.Background(), &awss3.PutObjectInput{
Bucket: aws.String(testBucket),
Key: aws.String("snap-arm001/metadata.json"),
Body: strings.NewReader(func() string {
b, _ := json.Marshal(handlers_ec2_snapshot.SnapshotConfig{
SnapshotID: "snap-arm001", VolumeID: "vol-arm", VolumeSize: 32,
State: "completed", Progress: "100%", OwnerID: testAccountID,
})
return string(b)
}()),
ContentType: aws.String("application/json"),
putTestSnapshotDoc(t, store, ebsmetadata.Snapshot{
SnapshotID: "snap-arm001", VolumeID: "vol-arm", VolumeSize: 32,
State: "completed", Progress: "100%", OwnerID: testAccountID,
})
require.NoError(t, err)
// Source AMI with non-default fields that must propagate.
putTestAMIConfigWithSnapshot(t, store, "ami-arm001", "arm-source", testAccountID, "snap-arm001", ebsmetadata.AMI{
@@ -1678,8 +1679,7 @@ func TestCopyImage_NewSnapshotSharesSourceVolumeID(t *testing.T) {
seedCopyableAMI(t, store, "ami-shareblocks", "shareblocks", testAccountID, "snap-orig", "vol-shared", 16)
srcSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, testBucket, "snap-orig")
require.NoError(t, err)
srcSnap := readTestSnapshotDoc(t, store, testAccountID, "snap-orig")
out, err := svc.CopyImage(context.Background(), validCopyImageServiceInput("ami-shareblocks", "shared-copy"), testAccountID)
require.NoError(t, err)
@@ -1688,8 +1688,7 @@ func TestCopyImage_NewSnapshotSharesSourceVolumeID(t *testing.T) {
require.NoError(t, err)
require.NotEqual(t, "snap-orig", newMeta.SnapshotID)
newSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, testBucket, newMeta.SnapshotID)
require.NoError(t, err)
newSnap := readTestSnapshotDoc(t, store, testAccountID, newMeta.SnapshotID)
// Compare against the source snapshot, not hard-coded literals — proves
// the new snap truly inherits the source's VolumeID rather than happening
// to match a test fixture value.
@@ -1745,8 +1744,7 @@ func TestCopyImage_BundledSystemAMINoStandaloneSnap(t *testing.T) {
// snap-ami-bundled01 viperblock reference.
require.NotEqual(t, "snap-ami-bundled01", newMeta.SnapshotID,
"copy must mint a new user-owned snap id, not borrow the source's")
newSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(context.Background(), store, testBucket, newMeta.SnapshotID)
require.NoError(t, err)
newSnap := readTestSnapshotDoc(t, store, testAccountID, newMeta.SnapshotID)
assert.Equal(t, "ami-bundled01", newSnap.VolumeID,
"bundled fallback must point the new snap at the source AMI's prefix")
assert.Equal(t, int64(8), newSnap.VolumeSize)
@@ -1759,7 +1757,7 @@ func TestCopyImage_CrossAccountHidesExistence(t *testing.T) {
seedCopyableAMI(t, store, "ami-other001", "other-acct", "000000000002", "snap-other001", "vol-other", 8)
srcBefore := readAMIConfigBytes(t, store, "ami-other001")
srcSnapBefore := readSnapshotConfigBytes(t, store, "snap-other001")
srcSnapBefore := readSnapshotConfigBytes(t, store, "000000000002", "snap-other001")
_, err := svc.CopyImage(context.Background(), validCopyImageServiceInput("ami-other001", "stolen-copy"), testAccountID)
require.Error(t, err)
@@ -1768,7 +1766,7 @@ func TestCopyImage_CrossAccountHidesExistence(t *testing.T) {
// Rejection must not touch the source AMI config or its snapshot metadata.
assert.Equal(t, srcBefore, readAMIConfigBytes(t, store, "ami-other001"),
"source AMI config altered after rejected cross-account copy")
assert.Equal(t, srcSnapBefore, readSnapshotConfigBytes(t, store, "snap-other001"),
assert.Equal(t, srcSnapBefore, readSnapshotConfigBytes(t, store, "000000000002", "snap-other001"),
"source snapshot metadata altered after rejected cross-account copy")
}
@@ -2400,7 +2398,7 @@ func TestSnapshotStoppedVolume_VolumeConfigMissing(t *testing.T) {
store: store,
bucketName: testBucket,
}
err := svc.snapshotStoppedVolume("vol-missing", "snap-s1")
err := svc.snapshotStoppedVolume("vol-missing", "snap-s1", testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
}
@@ -2425,7 +2423,7 @@ func TestSnapshotStoppedVolume_BackendInitFails(t *testing.T) {
}
createTestVolumeConfig(t, store, "vol-stopped1", 10)
err := svc.snapshotStoppedVolume("vol-stopped1", "snap-s2")
err := svc.snapshotStoppedVolume("vol-stopped1", "snap-s2", testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
}
@@ -2527,7 +2525,7 @@ func TestSnapshotStoppedVolume_EncryptionKeyLoadError(t *testing.T) {
}
createTestVolumeConfig(t, store, "vol-stopped-enckey", 10)
err := svc.snapshotStoppedVolume("vol-stopped-enckey", "snap-stop-enckey")
err := svc.snapshotStoppedVolume("vol-stopped-enckey", "snap-stop-enckey", testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
}
@@ -2552,7 +2550,7 @@ func TestSnapshotStoppedVolume_LoadStateFails(t *testing.T) {
}
createTestVolumeConfig(t, store, "vol-stopped-lsf", 10)
err := svc.snapshotStoppedVolume("vol-stopped-lsf", "snap-stop-lsf")
err := svc.snapshotStoppedVolume("vol-stopped-lsf", "snap-stop-lsf", testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
}
@@ -120,7 +120,7 @@ func TestPrepareRootVolume_Provider_WritesMetadataDocument(t *testing.T) {
"vol-root", testRootVolumeBytes, 0, instance, true)
require.NoError(t, err)
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), "vol-root")
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), testRootAccount, "vol-root")
require.NoError(t, err, "the root volume must have an ebsmetadata document")
assert.Equal(t, "vol-root", doc.VolumeID)
@@ -154,7 +154,7 @@ func TestPrepareRootVolume_Provider_HonoursRequestedIOPS(t *testing.T) {
"vol-root", testRootVolumeBytes, 5000, &vm.VM{AccountID: testRootAccount}, true)
require.NoError(t, err)
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), "vol-root")
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), testRootAccount, "vol-root")
require.NoError(t, err)
assert.Equal(t, 5000, doc.IOPS)
}
@@ -184,7 +184,7 @@ func TestPrepareRootVolume_Provider_RollbackOnMetadataWriteFailure(t *testing.T)
seedProviderSnapshot(t, provider, "vol-origin", "snap-source")
store := &failingPutObjectStore{
ObjectStore: objectstore.NewMemoryObjectStore(),
failPrefix: "spinifex/ebsmetadata/v1/volumes/",
failPrefix: "spinifex/ebsmetadata/v2/volumes/",
}
svc := providerRootVolumeService(t, provider, rootVolumeAMILoader(), store)
@@ -45,7 +45,7 @@ func TestDeleteSnapshot_RemovesEveryObjectAcrossMultiplePages(t *testing.T) {
})
require.NoError(t, err)
require.NoError(t, ebsmetadata.NewStore(store, "test-bucket").PutVolume(context.Background(), ebsmetadata.Volume{
VolumeID: "vol-1", CapacityGiB: 10, AvailabilityZone: "us-east-1a",
VolumeID: "vol-1", TenantID: testAccountID, CapacityGiB: 10, AvailabilityZone: "us-east-1a",
}))
snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
@@ -10,6 +10,7 @@ import (
"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/stretchr/testify/assert"
@@ -55,13 +56,13 @@ func seedSnapshots(t *testing.T, store objectstore.ObjectStore, n int) *handlers
t.Helper()
cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := handlers_ec2_snapshot.NewSnapshotServiceImplWithStore(cfg, store, nil)
metadata := ebsmetadata.NewStore(store, "test-bucket")
for i := range n {
id := "snap-fanout" + string(rune('a'+i%26)) + string(rune('a'+i/26))
require.NoError(t, handlers_ec2_snapshot.WriteSnapshotConfig(store, "test-bucket", id,
&handlers_ec2_snapshot.SnapshotConfig{
SnapshotID: id, VolumeID: "vol-fanout", State: "completed",
OwnerID: fanoutAccountID, StartTime: time.Now(),
}))
require.NoError(t, metadata.PutSnapshot(context.Background(), ebsmetadata.Snapshot{
SnapshotID: id, VolumeID: "vol-fanout", State: "completed",
OwnerID: fanoutAccountID, StartTime: time.Now(),
}))
}
return svc
}
@@ -22,7 +22,7 @@ func TestSnapshotRecordTagsMirror(t *testing.T) {
},
}, testAccountID))
cfg, err := svc.getSnapshotConfig(context.Background(), snapID)
cfg, err := svc.getSnapshotConfig(context.Background(), testAccountID, snapID)
require.NoError(t, err)
assert.Equal(t, "yes", cfg.Tags["keep"])
assert.Equal(t, "v", cfg.Tags["drop"])
@@ -36,7 +36,7 @@ func TestSnapshotRecordTagsMirror(t *testing.T) {
},
}, testAccountID))
cfg, err = svc.getSnapshotConfig(context.Background(), snapID)
cfg, err = svc.getSnapshotConfig(context.Background(), testAccountID, snapID)
require.NoError(t, err)
assert.Equal(t, "yes", cfg.Tags["keep"])
_, ok := cfg.Tags["drop"]
@@ -53,7 +53,7 @@ func TestSnapshotRecordTagsMirror_CrossTenantNoMutation(t *testing.T) {
Tags: []*ec2.Tag{{Key: aws.String("evil"), Value: aws.String("1")}},
}, "999999999999"))
cfg, err := svc.getSnapshotConfig(context.Background(), snapID)
cfg, err := svc.getSnapshotConfig(context.Background(), testAccountID, snapID)
require.NoError(t, err)
assert.Equal(t, map[string]string{"orig": "1"}, cfg.Tags)
}
+116 -192
View File
@@ -1,7 +1,6 @@
package handlers_ec2_snapshot
import (
"bytes"
"context"
"encoding/json"
"errors"
@@ -71,22 +70,6 @@ func (s *SnapshotServiceImpl) MetadataStore() *ebsmetadata.Store {
return s.metadata
}
// SnapshotConfig represents snapshot metadata stored in S3.
type SnapshotConfig struct {
SnapshotID string `json:"snapshot_id"`
VolumeID string `json:"volume_id"`
VolumeSize int64 `json:"volume_size"`
State string `json:"state"`
Progress string `json:"progress"`
StartTime time.Time `json:"start_time"`
Description string `json:"description"`
Encrypted bool `json:"encrypted"`
OwnerID string `json:"owner_id"`
AvailabilityZone string `json:"availability_zone"`
Tags map[string]string `json:"tags"`
ProviderHandle string `json:"provider_handle,omitempty"`
}
// NewSnapshotServiceImplWithNATS creates a snapshot service with JetStream KV for volume-snapshot tracking.
func NewSnapshotServiceImplWithNATS(ctx context.Context, cfg *config.Config, natsConn *nats.Conn) (*SnapshotServiceImpl, jetstream.KeyValue, error) {
store := objectstore.NewS3ObjectStoreFromConfig(
@@ -135,73 +118,21 @@ func NewSnapshotServiceImplWithStore(cfg *config.Config, store objectstore.Objec
return svc
}
// GetSnapshotKey uses metadata.json to avoid colliding with viperblock's
// config.json (which stores SnapshotState: block map, source volume, etc).
func GetSnapshotKey(snapshotID string) string {
return fmt.Sprintf("%s/metadata.json", snapshotID)
}
// ErrCorruptSnapshotMetadata lets callers distinguish a missing snapshot from
// one whose metadata.json can't be parsed.
var ErrCorruptSnapshotMetadata = errors.New("corrupt snapshot metadata")
// ReadSnapshotConfig reads {snapshotID}/metadata.json. Object-store errors are
// returned unchanged; callers map NoSuchKey to their preferred AWS error.
// Decode failures wrap ErrCorruptSnapshotMetadata. ctx carries the caller's
// deadline: a describe that has already given up must not keep the object
// store busy on its behalf.
func ReadSnapshotConfig(ctx context.Context, store objectstore.ObjectStore, bucket, snapshotID string) (*SnapshotConfig, error) {
key := GetSnapshotKey(snapshotID)
result, err := store.GetObject(ctx, &s3.GetObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(key),
})
if err != nil {
return nil, err
}
defer result.Body.Close()
var cfg SnapshotConfig
if err := json.NewDecoder(result.Body).Decode(&cfg); err != nil {
return nil, fmt.Errorf("%w: %w", ErrCorruptSnapshotMetadata, err)
}
return &cfg, nil
}
// WriteSnapshotConfig writes the SnapshotConfig to {snapshotID}/metadata.json.
func WriteSnapshotConfig(store objectstore.ObjectStore, bucket, snapshotID string, cfg *SnapshotConfig) error {
data, err := json.Marshal(cfg)
if err != nil {
return err
}
_, err = store.PutObject(context.Background(), &s3.PutObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(GetSnapshotKey(snapshotID)),
Body: bytes.NewReader(data),
ContentType: aws.String("application/json"),
})
return err
}
// getSnapshotConfig translates NoSuchKey to InvalidSnapshot.NotFound.
func (s *SnapshotServiceImpl) getSnapshotConfig(ctx context.Context, snapshotID string) (*SnapshotConfig, error) {
cfg, err := ReadSnapshotConfig(ctx, s.store, s.config.Predastore.Bucket, snapshotID)
// getSnapshotConfig reads the caller's own snapshot document. A snapshot
// outside the caller's prefix is absent, so ownership needs no second check.
func (s *SnapshotServiceImpl) getSnapshotConfig(ctx context.Context, accountID, snapshotID string) (ebsmetadata.Snapshot, error) {
cfg, err := s.metadata.GetSnapshot(ctx, accountID, snapshotID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
return ebsmetadata.Snapshot{}, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
}
return nil, err
return ebsmetadata.Snapshot{}, err
}
return cfg, nil
}
// putSnapshotConfig stores snapshot config to S3.
func (s *SnapshotServiceImpl) putSnapshotConfig(snapshotID string, cfg *SnapshotConfig) error {
return WriteSnapshotConfig(s.store, s.config.Predastore.Bucket, snapshotID, cfg)
}
// snapshotConfigToEC2 converts a SnapshotConfig to an EC2 Snapshot response object.
func snapshotConfigToEC2(cfg *SnapshotConfig) *ec2.Snapshot {
// snapshotConfigToEC2 converts a snapshot document to an EC2 response object.
func snapshotConfigToEC2(cfg ebsmetadata.Snapshot) *ec2.Snapshot {
snapshot := &ec2.Snapshot{
SnapshotId: aws.String(cfg.SnapshotID),
VolumeId: aws.String(cfg.VolumeID),
@@ -225,6 +156,13 @@ func (s *SnapshotServiceImpl) CreateSnapshot(ctx context.Context, input *ec2.Cre
return nil, errors.New(awserrors.ErrorInvalidParameterValue)
}
// The caller's account is a key segment, so an untenanted caller has no
// prefix to read or write under. Refuse at the boundary rather than in the
// key builder, where the rejection would surface as an internal error.
if accountID == "" {
return nil, errors.New(awserrors.ErrorAuthFailure)
}
volumeID := *input.VolumeId
slog.InfoContext(ctx, "CreateSnapshot request", "volumeId", volumeID)
@@ -235,11 +173,10 @@ func (s *SnapshotServiceImpl) CreateSnapshot(ctx context.Context, input *ec2.Cre
return nil, errors.New(awserrors.ErrorServerInternal)
}
// A volume predating tenancy records no owner. Refusing it would strand
// every pre-tenancy volume as unsnapshottable, so an absent TenantID stays
// open to any caller; a recorded one must match.
volume, err := s.metadata.GetVolume(ctx, volumeID)
if err != nil || (accountID != "" && volume.TenantID != "" && volume.TenantID != accountID) {
// The read is already scoped to the caller's prefix, so a returned document
// is the caller's by construction. The comparison stays as an assertion.
volume, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil || volume.TenantID != accountID {
return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound)
}
if volume.CapacityGiB == 0 {
@@ -261,7 +198,7 @@ func (s *SnapshotServiceImpl) CreateSnapshot(ctx context.Context, input *ec2.Cre
return nil, errors.New(awserrors.ErrorServerInternal)
}
now := time.Now()
snapshotCfg := &SnapshotConfig{
snapshotCfg := ebsmetadata.Snapshot{
SnapshotID: snapshotID, VolumeID: volumeID,
VolumeSize: safecast.Uint64ToInt64(volume.CapacityGiB),
State: string(created.State), Progress: "100%", StartTime: now,
@@ -284,7 +221,7 @@ func (s *SnapshotServiceImpl) CreateSnapshot(ctx context.Context, input *ec2.Cre
_ = s.provider.DeleteSnapshot(ctx, ebsprovider.DeleteSnapshotRequest{Versioned: ebsprovider.NewVersioned(), SnapshotID: snapshotID, Handle: created.Handle})
return nil, errors.New(awserrors.ErrorServerInternal)
}
if err := s.putSnapshotConfig(snapshotID, snapshotCfg); err != nil {
if err := s.metadata.PutSnapshot(ctx, snapshotCfg); err != nil {
slog.ErrorContext(ctx, "CreateSnapshot failed to write config", "snapshotId", snapshotID, "err", err)
_ = s.removeSnapshotRefForCleanup(ctx, volumeID, snapshotID)
_ = s.provider.DeleteSnapshot(ctx, ebsprovider.DeleteSnapshotRequest{Versioned: ebsprovider.NewVersioned(), SnapshotID: snapshotID, Handle: created.Handle})
@@ -508,70 +445,42 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2.
return nil, errors.New(awserrors.ErrorInvalidParameterValue)
}
_, commonPrefixes, err := objectstore.ListAll(ctx, s.store, &s3.ListObjectsV2Input{
Bucket: aws.String(s.config.Predastore.Bucket),
Prefix: aws.String("snap-"),
Delimiter: aws.String("/"),
})
if err != nil {
slog.ErrorContext(ctx, "DescribeSnapshots failed to list objects", "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
// Extract snapshot-id filter values for early prefix skipping to avoid
// unnecessary S3 GetObject calls on non-matching snapshots.
var snapshotIDFilterValues []string
if parsedFilters != nil {
snapshotIDFilterValues = parsedFilters["snapshot-id"]
}
var wanted []string
for _, prefix := range commonPrefixes {
if prefix == nil || prefix.Prefix == nil {
continue
// 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.
var configs []ebsmetadata.Snapshot
if ownerIDsIncludeCaller(input.OwnerIds, accountID) {
if strict {
configs, err = s.metadata.ListSnapshotsStrict(ctx, accountID)
} else {
configs, err = s.metadata.ListSnapshots(ctx, accountID)
}
snapshotID := strings.TrimSuffix(*prefix.Prefix, "/")
if len(snapshotIDFilter) > 0 && !snapshotIDFilter[snapshotID] {
continue
}
// Early skip: if snapshot-id filter is set, check the prefix against
// filter values before fetching config from S3.
if len(snapshotIDFilterValues) > 0 {
if !filterutil.MatchesAny(snapshotIDFilterValues, snapshotID) {
continue
if err != nil {
slog.ErrorContext(ctx, "DescribeSnapshots failed to list snapshot documents", "accountID", accountID, "err", err)
if strict {
return nil, fmt.Errorf("describe snapshots for %s: %w", accountID, err)
}
return nil, errors.New(awserrors.ErrorServerInternal)
}
wanted = append(wanted, snapshotID)
}
configs, readErrs := s.readSnapshotConfigs(ctx, wanted)
// Every read fails once the caller's deadline passes, and reporting that as
// an empty list would read as "this account has no snapshots".
// Every read fails once the caller's deadline passes, and a tolerant listing
// reports that as an empty list, which reads as "this account has no
// snapshots".
if err := ctx.Err(); err != nil {
slog.WarnContext(ctx, "DescribeSnapshots gave up reading metadata",
"requested", len(wanted), "read", len(configs), "err", err)
"read", len(configs), "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
var snapshots []*ec2.Snapshot
for _, snapshotID := range wanted {
cfg, ok := configs[snapshotID]
if !ok {
slog.WarnContext(ctx, "DescribeSnapshots failed to get config",
"snapshotId", snapshotID, "err", readErrs[snapshotID])
if strict {
return nil, fmt.Errorf("describe snapshot %s metadata: %w", snapshotID, readErrs[snapshotID])
}
continue
}
// Filter by account: only return snapshots owned by the caller
if accountID != "" && cfg.OwnerID != "" && cfg.OwnerID != accountID {
for _, cfg := range configs {
if len(snapshotIDFilter) > 0 && !snapshotIDFilter[cfg.SnapshotID] {
continue
}
@@ -605,48 +514,29 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2.
}, nil
}
// describeSnapshotFanout bounds how many metadata reads a single describe has
// in flight. A describe used to read every snapshot's metadata.json one after
// the other, so its latency was the sum of them all and a single slow object
// made the whole listing miss its caller's deadline. The bound keeps a large
// account from turning one describe into a burst against the object store.
const describeSnapshotFanout = 16
// readSnapshotConfigs fetches metadata for each snapshot concurrently. It
// returns what it could read plus the error for each one it could not, so the
// caller decides whether an unreadable snapshot is fatal or simply skipped.
func (s *SnapshotServiceImpl) readSnapshotConfigs(
ctx context.Context, ids []string,
) (map[string]*SnapshotConfig, map[string]error) {
configs := make(map[string]*SnapshotConfig, len(ids))
readErrs := make(map[string]error)
if len(ids) == 0 {
return configs, readErrs
// ownerIDsIncludeCaller reports whether an OwnerIds request parameter selects
// the account whose prefix is being listed. "self" and the caller's own account
// ID both name that prefix; any other value names an account the caller's
// prefix cannot contain.
func ownerIDsIncludeCaller(ownerIDs []*string, accountID string) bool {
if len(ownerIDs) == 0 {
return true
}
var mu sync.Mutex
sem := make(chan struct{}, describeSnapshotFanout)
var wg sync.WaitGroup
for _, id := range ids {
sem <- struct{}{}
wg.Go(func() {
defer func() { <-sem }()
cfg, err := s.getSnapshotConfig(ctx, id)
mu.Lock()
defer mu.Unlock()
if err != nil {
readErrs[id] = err
return
}
configs[id] = cfg
})
for _, owner := range ownerIDs {
if owner == nil {
continue
}
if *owner == "self" || *owner == accountID {
return true
}
}
wg.Wait()
return configs, readErrs
return false
}
// snapshotMatchesFilters checks whether a SnapshotConfig satisfies all parsed filters.
func snapshotMatchesFilters(cfg *SnapshotConfig, filters map[string][]string) bool {
// snapshotMatchesFilters checks whether a snapshot document satisfies all parsed filters.
func snapshotMatchesFilters(cfg ebsmetadata.Snapshot, filters map[string][]string) bool {
for name, values := range filters {
if strings.HasPrefix(name, "tag:") {
continue
@@ -680,9 +570,14 @@ func snapshotMatchesFilters(cfg *SnapshotConfig, filters map[string][]string) bo
// snapshot. A clone records its source in its ebsmetadata document; missing one
// here would let a delete strip the chunks a live clone still reads.
func (s *SnapshotServiceImpl) snapshotInUseByVolumes(ctx context.Context, snapshotID string) (bool, error) {
// Whole-cluster, and not scoping this to the snapshot's own account is the
// point: launching from a system AMI writes a root volume owned by the
// launching tenant whose SnapshotID is the system snapshot, so a clone
// routinely lives outside the snapshot owner's prefix.
//
// Strict: a volume that failed to read is not evidence that no clone reads
// the snapshot's chunks, and this answer gates deleting them.
volumes, err := s.metadata.ListVolumesStrict(ctx)
volumes, err := s.metadata.ListAllVolumesStrict(ctx)
if err != nil {
return false, fmt.Errorf("snapshotInUseByVolumes: failed to list volumes: %w", err)
}
@@ -700,18 +595,25 @@ func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.Del
return nil, errors.New(awserrors.ErrorInvalidParameterValue)
}
// An untenanted caller has no prefix to delete under; refuse here rather
// than in the key builder.
if accountID == "" {
return nil, errors.New(awserrors.ErrorAuthFailure)
}
snapshotID := *input.SnapshotId
slog.InfoContext(ctx, "DeleteSnapshot request", "snapshotId", snapshotID, "accountID", accountID)
cfg, err := s.getSnapshotConfig(ctx, snapshotID)
cfg, err := s.getSnapshotConfig(ctx, accountID, snapshotID)
if err != nil {
slog.ErrorContext(ctx, "DeleteSnapshot snapshot not found", "snapshotId", snapshotID, "err", err)
return nil, err
}
// Verify ownership: caller must own the snapshot
if accountID != "" && cfg.OwnerID != "" && cfg.OwnerID != accountID {
// The read was scoped to the caller's prefix, so the document is theirs by
// construction. The comparison stays as an assertion.
if cfg.OwnerID != accountID {
slog.WarnContext(ctx, "DeleteSnapshot: account does not own snapshot", "snapshotId", snapshotID, "accountID", accountID, "ownerID", cfg.OwnerID)
return nil, errors.New(awserrors.ErrorUnauthorizedOperation)
}
@@ -743,6 +645,10 @@ func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.Del
return nil, errors.New(awserrors.ErrorServerInternal)
}
// A key the sweep could not delete has to keep the document alive. The
// listing no longer enumerates snap- prefixes, so deleting the document over
// surviving chunks strands them where nothing can see or reclaim them.
var sweepErr error
for _, obj := range objects {
if obj == nil || obj.Key == nil {
continue
@@ -752,9 +658,24 @@ func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.Del
Key: obj.Key,
})
if err != nil {
slog.WarnContext(ctx, "DeleteSnapshot failed to delete object", "key", *obj.Key, "err", err)
slog.ErrorContext(ctx, "DeleteSnapshot failed to delete object", "key", *obj.Key, "err", err)
sweepErr = errors.Join(sweepErr, err)
}
}
if sweepErr != nil {
slog.ErrorContext(ctx, "DeleteSnapshot leaving the document in place after an incomplete sweep",
"snapshotId", snapshotID, "err", sweepErr)
return nil, errors.New(awserrors.ErrorServerInternal)
}
// The document no longer sits inside the swept prefix, so it has to go
// explicitly, and last: a crash with it still present leaves a snapshot that
// DescribeSnapshots shows and DeleteSnapshot can retry, rather than chunks
// nothing enumerates.
if err := s.metadata.DeleteSnapshot(ctx, accountID, snapshotID); err != nil {
slog.ErrorContext(ctx, "DeleteSnapshot failed to delete metadata document", "snapshotId", snapshotID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
// Remove from KV after S3 cleanup. Failure is logged but not fatal —
// a phantom entry safely blocks volume deletion rather than allowing it.
@@ -776,17 +697,24 @@ func (s *SnapshotServiceImpl) CopySnapshot(ctx context.Context, input *ec2.CopyS
return nil, errors.New(awserrors.ErrorInvalidParameterValue)
}
// The copy is written under the caller's account, so an untenanted caller
// has nowhere to put it; refuse here rather than in the key builder.
if accountID == "" {
return nil, errors.New(awserrors.ErrorAuthFailure)
}
sourceSnapshotID := *input.SourceSnapshotId
slog.InfoContext(ctx, "CopySnapshot request", "sourceSnapshotId", sourceSnapshotID, "accountID", accountID)
sourceCfg, err := s.getSnapshotConfig(ctx, sourceSnapshotID)
sourceCfg, err := s.getSnapshotConfig(ctx, accountID, sourceSnapshotID)
if err != nil {
slog.ErrorContext(ctx, "CopySnapshot source snapshot not found", "snapshotId", sourceSnapshotID, "err", err)
return nil, err
}
if accountID != "" && sourceCfg.OwnerID != "" && sourceCfg.OwnerID != accountID {
// Assertion: the read was already scoped to the caller's prefix.
if sourceCfg.OwnerID != accountID {
slog.WarnContext(ctx, "CopySnapshot: account does not own source snapshot", "snapshotId", sourceSnapshotID, "accountID", accountID, "ownerID", sourceCfg.OwnerID)
return nil, errors.New(awserrors.ErrorUnauthorizedOperation)
}
@@ -812,7 +740,7 @@ func (s *SnapshotServiceImpl) CopySnapshot(ctx context.Context, input *ec2.CopyS
}
now := time.Now()
snapshotCfg := &SnapshotConfig{
snapshotCfg := ebsmetadata.Snapshot{
SnapshotID: newSnapshotID, VolumeID: sourceCfg.VolumeID,
VolumeSize: sourceCfg.VolumeSize,
State: string(copied.State), Progress: "100%", StartTime: now,
@@ -836,7 +764,7 @@ func (s *SnapshotServiceImpl) CopySnapshot(ctx context.Context, input *ec2.CopyS
_ = s.provider.DeleteSnapshot(ctx, ebsprovider.DeleteSnapshotRequest{Versioned: ebsprovider.NewVersioned(), SnapshotID: newSnapshotID, Handle: copied.Handle})
return nil, errors.New(awserrors.ErrorServerInternal)
}
if err := s.putSnapshotConfig(newSnapshotID, snapshotCfg); err != nil {
if err := s.metadata.PutSnapshot(ctx, snapshotCfg); err != nil {
_ = s.removeSnapshotRefForCleanup(ctx, sourceCfg.VolumeID, newSnapshotID)
_ = s.provider.DeleteSnapshot(ctx, ebsprovider.DeleteSnapshotRequest{Versioned: ebsprovider.NewVersioned(), SnapshotID: newSnapshotID, Handle: copied.Handle})
return nil, errors.New(awserrors.ErrorServerInternal)
@@ -1011,30 +939,26 @@ func (s *SnapshotServiceImpl) RemoveRecordTags(input *ec2.DeleteTagsInput, accou
return s.mirrorSnapshotTags(input.Resources, accountID, utils.RemoveTagsMut(input))
}
// mirrorSnapshotTags read-modify-writes SnapshotConfig.Tags for each snap- id.
// metadata.json lives at a global ID-keyed path, so the mutation is gated on
// the caller owning the snapshot (OwnerID match); mismatch or absence no-ops.
// mirrorSnapshotTags read-modify-writes the snapshot document's Tags for each
// snap- id. The document is read under the caller's own prefix, so a snapshot
// they do not own is simply absent and no-ops.
func (s *SnapshotServiceImpl) mirrorSnapshotTags(resources []*string, accountID string, mut func(map[string]string)) error {
for _, res := range resources {
if res == nil || !strings.HasPrefix(*res, "snap-") {
continue
}
cfg, err := ReadSnapshotConfig(context.Background(), s.store, s.config.Predastore.Bucket, *res)
cfg, err := s.metadata.GetSnapshot(context.Background(), accountID, *res)
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
continue
}
return err
}
if cfg.OwnerID != accountID {
slog.Debug("mirrorSnapshotTags: skipping snapshot not owned by caller", "snapshotId", *res)
continue
}
if cfg.Tags == nil {
cfg.Tags = map[string]string{}
}
mut(cfg.Tags)
if err := s.putSnapshotConfig(*res, cfg); err != nil {
if err := s.metadata.PutSnapshot(context.Background(), cfg); err != nil {
return err
}
}
@@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"errors"
"path"
"strings"
"testing"
@@ -44,10 +45,18 @@ func setupTestSnapshotService(t *testing.T) (*SnapshotServiceImpl, *objectstore.
// createTestVolume seeds a volume on both sides: the provider holds the blocks
// a snapshot copies, the document is what the control plane resolves.
func createTestVolume(t *testing.T, svc *SnapshotServiceImpl, store *objectstore.MemoryObjectStore, volumeID string, sizeGiB int) {
t.Helper()
createTestVolumeForAccount(t, svc, store, volumeID, sizeGiB, testAccountID)
}
// createTestVolumeForAccount is createTestVolume for a volume owned by an
// account other than the default one.
func createTestVolumeForAccount(t *testing.T, svc *SnapshotServiceImpl, store *objectstore.MemoryObjectStore, volumeID string, sizeGiB int, accountID string) {
t.Helper()
seedProviderVolume(t, svc, volumeID, sizeGiB)
seedVolumeDocument(t, store, ebsmetadata.Volume{
VolumeID: volumeID,
TenantID: accountID,
CapacityGiB: uint64(sizeGiB),
AvailabilityZone: "us-east-1a",
})
@@ -56,9 +65,24 @@ func createTestVolume(t *testing.T, svc *SnapshotServiceImpl, store *objectstore
// seedVolumeDocument writes the control-plane document for a volume.
func seedVolumeDocument(t *testing.T, store objectstore.ObjectStore, volume ebsmetadata.Volume) {
t.Helper()
// The owning account is a key segment, so a fixture that does not care
// which account owns it still has to have one.
if volume.TenantID == "" {
volume.TenantID = testAccountID
}
require.NoError(t, ebsmetadata.NewStore(store, "test-bucket").PutVolume(context.Background(), volume))
}
// seedSnapshotDocument writes the control-plane document for a snapshot,
// keyed under its own OwnerID.
func seedSnapshotDocument(t *testing.T, store objectstore.ObjectStore, snapshot ebsmetadata.Snapshot) {
t.Helper()
if snapshot.OwnerID == "" {
snapshot.OwnerID = testAccountID
}
require.NoError(t, ebsmetadata.NewStore(store, "test-bucket").PutSnapshot(context.Background(), snapshot))
}
// seedProviderVolume gives the service's provider a volume to snapshot. A
// control-plane document alone is not a volume: the provider owns the blocks,
// and CreateSnapshot fails against one it has never allocated.
@@ -232,9 +256,11 @@ func TestDescribeSnapshots(t *testing.T) {
func TestDescribeSnapshotsStrict_RejectsPartialMetadataResults(t *testing.T) {
svc, store := setupTestSnapshotService(t)
_, err := store.PutObject(t.Context(), &s3.PutObjectInput{
key, err := ebsmetadata.SnapshotKey(testAccountID, "snap-corrupt")
require.NoError(t, err)
_, err = store.PutObject(t.Context(), &s3.PutObjectInput{
Bucket: aws.String("test-bucket"),
Key: aws.String(GetSnapshotKey("snap-corrupt")),
Key: aws.String(key),
Body: strings.NewReader("not-json"),
})
require.NoError(t, err)
@@ -248,7 +274,7 @@ func TestDescribeSnapshotsStrict_RejectsPartialMetadataResults(t *testing.T) {
// absence, so its internal lookup surfaces the metadata failure.
_, err = svc.DescribeSnapshotsStrict(t.Context(), &ec2.DescribeSnapshotsInput{}, testAccountID)
require.Error(t, err)
assert.ErrorIs(t, err, ErrCorruptSnapshotMetadata)
assert.ErrorIs(t, err, ebsmetadata.ErrCorruptDocument)
}
// TestDescribeSnapshots_ByID tests listing specific snapshots by ID.
@@ -310,22 +336,21 @@ func TestDescribeSnapshots_Empty(t *testing.T) {
}
// TestDescribeSnapshots_ImportedAMISnapshotVisible locks that a snapshot
// registered directly via WriteSnapshotConfig -- the way a provider-backed
// AMI import registers its snapshot, without ever calling CreateSnapshot --
// is still listed. Before that registration exists, ListObjectsV2 finds the
// provider-written "snap-.../" prefix but getSnapshotConfig 404s on it, and
// DescribeSnapshots silently skips it.
// whose document is written directly -- the way a provider-backed AMI import
// registers its snapshot, without ever calling CreateSnapshot -- is still
// listed. Without that document the snapshot is not in the account's prefix
// and DescribeSnapshots cannot see it at all.
func TestDescribeSnapshots_ImportedAMISnapshotVisible(t *testing.T) {
svc, store := setupTestSnapshotService(t)
require.NoError(t, WriteSnapshotConfig(store, "test-bucket", "snap-ami-import01", &SnapshotConfig{
seedSnapshotDocument(t, store, ebsmetadata.Snapshot{
SnapshotID: "snap-ami-import01",
VolumeID: "ami-import01",
VolumeSize: 8,
State: "completed",
Progress: "100%",
OwnerID: testAccountID,
}))
})
result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{}, testAccountID)
require.NoError(t, err)
@@ -339,6 +364,7 @@ func TestDescribeSnapshots_AccountScoping(t *testing.T) {
svc, store := setupTestSnapshotService(t)
createTestVolume(t, svc, store, "vol-1", 50)
createTestVolumeForAccount(t, svc, store, "vol-2", 50, otherAccountID)
// Account A creates a snapshot
snapA, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
@@ -346,9 +372,9 @@ func TestDescribeSnapshots_AccountScoping(t *testing.T) {
}, testAccountID)
require.NoError(t, err)
// Account B creates a snapshot
// Account B creates a snapshot of its own volume
snapB, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
VolumeId: aws.String("vol-1"),
VolumeId: aws.String("vol-2"),
}, otherAccountID)
require.NoError(t, err)
@@ -403,7 +429,9 @@ func TestDeleteSnapshot(t *testing.T) {
assert.Empty(t, result.Snapshots)
}
// TestDeleteSnapshot_WrongAccount tests that account B cannot delete account A's snapshot.
// TestDeleteSnapshot_WrongAccount tests that account B cannot delete account
// A's snapshot. It is not found rather than refused: the caller's prefix is
// the read, so the endpoint never confirms another account's snapshot IDs.
func TestDeleteSnapshot_WrongAccount(t *testing.T) {
svc, store := setupTestSnapshotService(t)
@@ -418,7 +446,7 @@ func TestDeleteSnapshot_WrongAccount(t *testing.T) {
SnapshotId: snap.SnapshotId,
}, otherAccountID)
require.Error(t, err)
assert.Contains(t, err.Error(), awserrors.ErrorUnauthorizedOperation)
assert.Contains(t, err.Error(), awserrors.ErrorInvalidSnapshotNotFound)
// Verify snapshot still exists
result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{
@@ -524,12 +552,13 @@ func TestCopySnapshot_WrongAccount(t *testing.T) {
require.NoError(t, err)
require.Len(t, result.Snapshots, 1)
// Account B tries to copy account A's snapshot — should fail
// Account B tries to copy account A's snapshot — not found, not refused:
// the source is read under the caller's own prefix.
_, err = svc.CopySnapshot(context.Background(), &ec2.CopySnapshotInput{
SourceSnapshotId: result.Snapshots[0].SnapshotId,
}, otherAccountID)
require.Error(t, err)
assert.Contains(t, err.Error(), awserrors.ErrorUnauthorizedOperation)
assert.Contains(t, err.Error(), awserrors.ErrorInvalidSnapshotNotFound)
}
// TestCopySnapshot_NotFound tests copying a non-existent snapshot.
@@ -750,20 +779,15 @@ func TestCreateSnapshot_CrossAccountVolumeRejected(t *testing.T) {
assert.Equal(t, testAccountID, *result.OwnerId)
}
// TestCreateSnapshot_PrePhase4VolumeAllowed tests that volumes without TenantID (pre-phase4) are allowed.
func TestCreateSnapshot_PrePhase4VolumeAllowed(t *testing.T) {
svc, store := setupTestSnapshotService(t)
// A volume with no owning account cannot be keyed, so the untenanted volume
// any caller used to be able to snapshot is not a state the store can hold.
func TestCreateSnapshot_UntenantedVolumeIsUnwritable(t *testing.T) {
_, store := setupTestSnapshotService(t)
// Create a volume with no TenantID (pre-phase4)
volumeID := "vol-legacy"
createTestVolume(t, svc, store, volumeID, 50)
// Any account can snapshot — backward compatibility
result, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
VolumeId: aws.String(volumeID),
}, otherAccountID)
require.NoError(t, err)
assert.Equal(t, otherAccountID, *result.OwnerId)
err := ebsmetadata.NewStore(store, "test-bucket").PutVolume(context.Background(), ebsmetadata.Volume{
VolumeID: "vol-untenanted", CapacityGiB: 50, AvailabilityZone: "us-east-1a",
})
require.Error(t, err)
}
// --- DescribeSnapshots filter tests ---
@@ -998,7 +1022,7 @@ func TestCreateDeleteSnapshot_UsesInjectedProvider(t *testing.T) {
snapshot, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-provider")}, testAccountID)
require.NoError(t, err)
require.NotNil(t, snapshot)
cfgStored, err := svc.getSnapshotConfig(context.Background(), aws.StringValue(snapshot.SnapshotId))
cfgStored, err := svc.getSnapshotConfig(context.Background(), testAccountID, aws.StringValue(snapshot.SnapshotId))
require.NoError(t, err)
assert.Equal(t, "memory://snapshot/"+aws.StringValue(snapshot.SnapshotId), cfgStored.ProviderHandle)
_, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{SnapshotId: snapshot.SnapshotId}, testAccountID)
@@ -1041,23 +1065,28 @@ func TestDeleteSnapshot_Provider_BlockedByCloneRecordedInMetadata(t *testing.T)
assert.EqualError(t, err, awserrors.ErrorInvalidSnapshotInUse)
// Once the clone is gone the snapshot is deletable again.
require.NoError(t, svc.metadata.DeleteVolume(ctx, "vol-clone"))
require.NoError(t, svc.metadata.DeleteVolume(ctx, testAccountID, "vol-clone"))
_, err = svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: aws.String(snapshotID)}, testAccountID)
require.NoError(t, err)
}
// snapshotMetadataFailingObjectStore fails PutObject for any snapshot
// metadata.json write and records the last such key, so a test can recover
// the randomly generated snapshot ID CreateSnapshot attempted to persist.
// snapshotMetadataFailingObjectStore fails PutObject for any snapshot document
// write and records the last such key, so a test can recover the randomly
// generated snapshot ID CreateSnapshot attempted to persist.
type snapshotMetadataFailingObjectStore struct {
objectstore.ObjectStore
attemptedKey string
}
// snapshotDocumentPrefix is where the store keys snapshot documents. Spelled
// out here so the fake matches only those writes, not the volume document
// CreateSnapshot reads through on the way.
const snapshotDocumentPrefix = "spinifex/ebsmetadata/v2/snapshots/"
func (s *snapshotMetadataFailingObjectStore) PutObject(ctx context.Context, input *s3.PutObjectInput) (*s3.PutObjectOutput, error) {
key := aws.StringValue(input.Key)
if strings.HasSuffix(key, "/metadata.json") {
if strings.HasPrefix(key, snapshotDocumentPrefix) {
s.attemptedKey = key
return nil, errors.New("simulated metadata write failure")
}
@@ -1089,7 +1118,7 @@ func TestCreateSnapshot_Provider_RollbackOnMetadataWriteFailure(t *testing.T) {
require.EqualError(t, err, awserrors.ErrorServerInternal)
require.NotEmpty(t, store.attemptedKey, "the snapshot metadata write must have been attempted")
snapshotID := strings.TrimSuffix(store.attemptedKey, "/metadata.json")
snapshotID := strings.TrimSuffix(path.Base(store.attemptedKey), ".json")
_, err = provider.GetVolume(context.Background(), ebsprovider.GetVolumeRequest{Versioned: ebsprovider.NewVersioned(), VolumeID: "vol-rollback"})
require.NoError(t, err, "the source volume must be unaffected by the snapshot rollback")
@@ -1152,7 +1181,7 @@ func TestCopySnapshot_Provider_CreatesBackendSnapshot(t *testing.T) {
require.True(t, ok, "the copy must exist in the provider, not only in the control plane")
assert.Equal(t, "vol-copysrc", got.SourceVolumeID)
stored, err := svc.getSnapshotConfig(context.Background(), newID)
stored, err := svc.getSnapshotConfig(context.Background(), testAccountID, newID)
require.NoError(t, err)
assert.Equal(t, "vol-copysrc", stored.VolumeID)
assert.Equal(t, "copied", stored.Description)
@@ -1194,7 +1223,7 @@ func TestCopySnapshot_Provider_InheritsSourceDescription(t *testing.T) {
out, err := svc.CopySnapshot(ctx, &ec2.CopySnapshotInput{SourceSnapshotId: src.SnapshotId}, testAccountID)
require.NoError(t, err)
stored, err := svc.getSnapshotConfig(context.Background(), aws.StringValue(out.SnapshotId))
stored, err := svc.getSnapshotConfig(context.Background(), testAccountID, aws.StringValue(out.SnapshotId))
require.NoError(t, err)
assert.Equal(t, "original text", stored.Description)
}
@@ -1229,7 +1258,7 @@ func TestCopySnapshot_Provider_ForeignSnapshotRefused(t *testing.T) {
_, err = svc.CopySnapshot(ctx, &ec2.CopySnapshotInput{SourceSnapshotId: src.SnapshotId}, "999988887777")
require.Error(t, err)
assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error())
assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, err.Error())
_, ok := provider.Snapshot(aws.StringValue(src.SnapshotId))
require.True(t, ok, "the source must survive a refused copy")
@@ -0,0 +1,345 @@
// These tests reach svc.metadata and reuse setupTestSnapshotService and
// createTestVolume, all package-internal.
//
//test:in-package
package handlers_ec2_snapshot
import (
"context"
"errors"
"strings"
"testing"
"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"
"github.com/mulgadc/spinifex/spinifex/ebsprovider"
"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"
)
// The document no longer lives inside the prefix DeleteSnapshot sweeps, so it
// has to be deleted explicitly. Miss that and DescribeSnapshots keeps returning
// a snapshot whose chunks are gone.
func TestDeleteSnapshot_LeavesNoDocument(t *testing.T) {
ctx := context.Background()
svc, store := setupTestSnapshotService(t)
createTestVolume(t, svc, store, "vol-del", 8)
snap, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-del")}, testAccountID)
require.NoError(t, err)
_, err = svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: snap.SnapshotId}, testAccountID)
require.NoError(t, err)
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{}, testAccountID)
require.NoError(t, err)
assert.Empty(t, listed.Snapshots, "a deleted snapshot must leave no document behind")
_, err = svc.metadata.GetSnapshot(ctx, testAccountID, aws.StringValue(snap.SnapshotId))
require.Error(t, err)
}
// The system account partitions like any tenant: GlobalAccountID is a valid
// account ID, so nothing on this path needs a special case. This fails if
// IsAccountID is ever tightened in a way that rejects "000000000000".
func TestSnapshot_SystemAccountIsAnOrdinaryAccount(t *testing.T) {
ctx := context.Background()
svc, store := setupTestSnapshotService(t)
createTestVolumeForAccount(t, svc, store, "vol-sys", 8, utils.GlobalAccountID)
snap, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-sys")}, utils.GlobalAccountID)
require.NoError(t, err)
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{}, utils.GlobalAccountID)
require.NoError(t, err)
require.Len(t, listed.Snapshots, 1)
assert.Equal(t, aws.StringValue(snap.SnapshotId), aws.StringValue(listed.Snapshots[0].SnapshotId))
_, err = svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: snap.SnapshotId}, utils.GlobalAccountID)
require.NoError(t, err)
}
// A caller with no account cannot name a prefix, so the listing fails rather
// than widening to every account's snapshots. The strict form reports the
// underlying failure; the tolerant one maps it to an internal error.
func TestDescribeSnapshots_RefusesACallerWithNoAccount(t *testing.T) {
ctx := context.Background()
svc, _ := setupTestSnapshotService(t)
_, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{}, "")
require.Error(t, err)
_, err = svc.DescribeSnapshotsStrict(ctx, &ec2.DescribeSnapshotsInput{}, "")
require.Error(t, err)
assert.Contains(t, err.Error(), "invalid EBS metadata account ID")
}
// A document that survives the delete leaves DescribeSnapshots returning a
// snapshot whose chunks are gone, so the failure must surface rather than be
// logged and swallowed.
func TestDeleteSnapshot_DocumentDeleteFailureSurfaces(t *testing.T) {
ctx := context.Background()
objects := &deleteFailingStore{MemoryObjectStore: objectstore.NewMemoryObjectStore(),
failPrefix: "spinifex/ebsmetadata/v2/snapshots/"}
cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := NewSnapshotServiceImplWithStore(cfg, objects, nil)
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
createTestVolume(t, svc, objects.MemoryObjectStore, "vol-delfail", 8)
snap, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-delfail")}, testAccountID)
require.NoError(t, err)
_, err = svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: snap.SnapshotId}, testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
}
// A chunk the sweep could not delete has to keep the document alive. The
// listing no longer enumerates snap- prefixes, so deleting the document over
// surviving chunks strands them where no API can see or reclaim them, and the
// delete cannot be retried to finish.
func TestDeleteSnapshot_ChunkSweepFailureRetainsDocument(t *testing.T) {
ctx := context.Background()
objects := &deleteFailingStore{MemoryObjectStore: objectstore.NewMemoryObjectStore()}
cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := NewSnapshotServiceImplWithStore(cfg, objects, nil)
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
createTestVolume(t, svc, objects.MemoryObjectStore, "vol-sweepfail", 8)
snap, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-sweepfail")}, testAccountID)
require.NoError(t, err)
snapshotID := aws.StringValue(snap.SnapshotId)
// A block chunk under the swept prefix, which is viperblock's half of the
// snapshot and the thing the document must not outlive.
_, err = objects.PutObject(ctx, &s3.PutObjectInput{
Bucket: aws.String("test-bucket"), Key: aws.String(snapshotID + "/chunk-0"),
Body: strings.NewReader("blocks"),
})
require.NoError(t, err)
objects.failPrefix = snapshotID + "/"
_, err = svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: snap.SnapshotId}, testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
_, err = svc.metadata.GetSnapshot(ctx, testAccountID, snapshotID)
require.NoError(t, err, "an incomplete sweep must leave the document so the delete can be retried")
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{}, testAccountID)
require.NoError(t, err)
require.Len(t, listed.Snapshots, 1, "the snapshot must stay visible while its chunks survive")
assert.Equal(t, snapshotID, aws.StringValue(listed.Snapshots[0].SnapshotId))
}
// deleteFailingStore fails DeleteObject for keys under failPrefix, leaving
// every other operation intact. failPrefix is settable after construction so a
// test can target a generated snapshot ID.
type deleteFailingStore struct {
*objectstore.MemoryObjectStore
failPrefix string
}
func (s *deleteFailingStore) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error) {
if strings.HasPrefix(aws.StringValue(input.Key), s.failPrefix) {
return nil, errors.New("simulated document delete failure")
}
return s.MemoryObjectStore.DeleteObject(ctx, input)
}
// Isolation must be enforced *by* the read, not by a filter after it. A
// whole-tree listing plus a correct in-memory filter would return the same
// snapshots, so this asserts the access pattern: the listing prefix is the
// caller's own, and no key under another account's prefix is fetched.
func TestDescribeSnapshots_TouchesNoOtherAccountsPrefix(t *testing.T) {
ctx := context.Background()
objects := recordingstore.New()
cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := NewSnapshotServiceImplWithStore(cfg, objects, nil)
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
for _, snapshot := range []ebsmetadata.Snapshot{
{SnapshotID: "snap-mine1", VolumeID: "vol-1", OwnerID: testAccountID, State: "completed"},
{SnapshotID: "snap-theirs1", VolumeID: "vol-2", OwnerID: otherAccountID, State: "completed"},
} {
require.NoError(t, svc.metadata.PutSnapshot(ctx, snapshot))
}
objects.Reset()
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{}, testAccountID)
require.NoError(t, err)
require.Len(t, listed.Snapshots, 1)
assert.Equal(t, "snap-mine1", aws.StringValue(listed.Snapshots[0].SnapshotId))
// Spelled out rather than derived: this is the layout claim itself.
assert.False(t, objects.TouchedPrefix("spinifex/ebsmetadata/v2/snapshots/"+otherAccountID+"/"),
"a describe must not read or list another account's prefix: %v %v", objects.ListPrefixes(), objects.Gets())
}
// OwnerIds used to be read by nobody: the account-teardown reaper set it and
// the describe ignored it, so the parameter promised a scoping it never
// applied. Under the account prefix the answer is structural.
func TestDescribeSnapshots_HonoursOwnerIds(t *testing.T) {
ctx := context.Background()
svc, store := setupTestSnapshotService(t)
createTestVolume(t, svc, store, "vol-owner", 8)
snap, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-owner")}, testAccountID)
require.NoError(t, err)
for _, tc := range []struct {
name string
ownerIDs []*string
want int
}{
{name: "unset", want: 1},
{name: "self", ownerIDs: []*string{aws.String("self")}, want: 1},
{name: "own account ID", ownerIDs: []*string{aws.String(testAccountID)}, want: 1},
{name: "another account", ownerIDs: []*string{aws.String(otherAccountID)}},
{name: "another account and self", ownerIDs: []*string{aws.String(otherAccountID), aws.String("self")}, want: 1},
} {
t.Run(tc.name, func(t *testing.T) {
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{OwnerIds: tc.ownerIDs}, testAccountID)
require.NoError(t, err)
require.Len(t, listed.Snapshots, tc.want)
if tc.want > 0 {
assert.Equal(t, aws.StringValue(snap.SnapshotId), aws.StringValue(listed.Snapshots[0].SnapshotId))
}
})
}
// Naming an id the owner filter excludes is the same answer as naming one
// that does not exist.
_, err = svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{
SnapshotIds: []*string{snap.SnapshotId},
OwnerIds: []*string{aws.String(otherAccountID)},
}, testAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, err.Error())
}
// An account the caller's prefix cannot contain is answered without a read at
// all, rather than by listing and filtering afterwards.
func TestDescribeSnapshots_ForeignOwnerIDReadsNothing(t *testing.T) {
ctx := context.Background()
objects := recordingstore.New()
cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := NewSnapshotServiceImplWithStore(cfg, objects, nil)
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
require.NoError(t, svc.metadata.PutSnapshot(ctx,
ebsmetadata.Snapshot{SnapshotID: "snap-mine", VolumeID: "vol-1", OwnerID: testAccountID, State: "completed"}))
objects.Reset()
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{
OwnerIds: []*string{aws.String(otherAccountID)},
}, testAccountID)
require.NoError(t, err)
assert.Empty(t, listed.Snapshots)
assert.Empty(t, objects.ListPrefixes(), "an owner the prefix cannot contain needs no listing")
assert.Empty(t, objects.Gets(), "an owner the prefix cannot contain needs no read")
}
// The dependency check must stay whole-cluster. Launching from a system AMI
// writes a root volume owned by the launching tenant whose SnapshotID is the
// system snapshot, so a clone routinely lives outside the snapshot owner's
// prefix. Scope the check and the delete strips chunks a live tenant volume
// still reads.
func TestDeleteSnapshot_BlockedByACloneInAnotherAccount(t *testing.T) {
ctx := context.Background()
svc, store := setupTestSnapshotService(t)
createTestVolumeForAccount(t, svc, store, "vol-system", 8, utils.GlobalAccountID)
snap, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-system")}, utils.GlobalAccountID)
require.NoError(t, err)
seedVolumeDocument(t, store, ebsmetadata.Volume{
VolumeID: "vol-tenant-root",
TenantID: otherAccountID,
SnapshotID: aws.StringValue(snap.SnapshotId),
})
_, err = svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: snap.SnapshotId}, utils.GlobalAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidSnapshotInUse, err.Error())
}
// The irreversible case. The teardown reaper lists with OwnerIds set to the
// account being removed and deletes everything it gets back, so a listing that
// reached past that account once destroyed every other account's snapshots.
// This walks the reaper's exact shape rather than asserting the input again.
func TestAccountTeardown_LeavesAnotherAccountsSnapshotsAlone(t *testing.T) {
ctx := context.Background()
svc, store := setupTestSnapshotService(t)
createTestVolumeForAccount(t, svc, store, "vol-doomed", 8, testAccountID)
createTestVolumeForAccount(t, svc, store, "vol-bystander", 8, otherAccountID)
doomed, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-doomed")}, testAccountID)
require.NoError(t, err)
bystander, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-bystander")}, otherAccountID)
require.NoError(t, err)
listed, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{
OwnerIds: []*string{aws.String(testAccountID)},
}, testAccountID)
require.NoError(t, err)
require.Len(t, listed.Snapshots, 1)
assert.Equal(t, aws.StringValue(doomed.SnapshotId), aws.StringValue(listed.Snapshots[0].SnapshotId))
for _, snapshot := range listed.Snapshots {
_, err := svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: snapshot.SnapshotId}, testAccountID)
require.NoError(t, err)
}
survived, err := svc.metadata.GetSnapshot(ctx, otherAccountID, aws.StringValue(bystander.SnapshotId))
require.NoError(t, err, "tearing down one account must not delete another account's snapshot")
assert.Equal(t, otherAccountID, survived.OwnerID)
}
// An untenanted caller is refused at the boundary, with the client-facing
// answer the boundary owns. Asserting that nothing was read is what stops the
// guard being deleted later as redundant with the key builder, whose rejection
// is an internal error rather than an AuthFailure.
func TestSnapshot_EmptyCallerAccountIsRefusedAtTheBoundary(t *testing.T) {
ctx := context.Background()
objects := recordingstore.New()
cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := NewSnapshotServiceImplWithStore(cfg, objects, nil)
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
for _, tc := range []struct {
name string
call func() error
}{
{name: "CreateSnapshot", call: func() error {
_, err := svc.CreateSnapshot(ctx, &ec2.CreateSnapshotInput{VolumeId: aws.String("vol-1")}, "")
return err
}},
{name: "DeleteSnapshot", call: func() error {
_, err := svc.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: aws.String("snap-1")}, "")
return err
}},
{name: "CopySnapshot", call: func() error {
_, err := svc.CopySnapshot(ctx, &ec2.CopySnapshotInput{SourceSnapshotId: aws.String("snap-1")}, "")
return err
}},
} {
t.Run(tc.name, func(t *testing.T) {
objects.Reset()
err := tc.call()
require.Error(t, err)
assert.Equal(t, awserrors.ErrorAuthFailure, err.Error())
assert.Empty(t, objects.Gets(), "the guard must fire before any read")
assert.Empty(t, objects.ListPrefixes(), "the guard must fire before any listing")
})
}
}
@@ -26,13 +26,13 @@ func TestUpdateVolumeState_WritesDocumentNotConfig(t *testing.T) {
})
before := getStoredConfig(t, store, volumeID)
require.NoError(t, svc.UpdateVolumeState(volumeID, "in-use", "i-abc", "/dev/nbd0"))
require.NoError(t, svc.UpdateVolumeState(testVolAccountID, volumeID, "in-use", "i-abc", "/dev/nbd0"))
after := getStoredConfig(t, store, volumeID)
assert.Equal(t, string(before), string(after),
"UpdateVolumeState must not write config.json; the live VB owns it")
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.NoError(t, err)
assert.Equal(t, "in-use", meta.State)
assert.Equal(t, "i-abc", meta.AttachedInstance)
@@ -55,13 +55,13 @@ func TestUpdateVolumeState_EncryptedConfigUntouched(t *testing.T) {
})
before := getStoredConfig(t, store, volumeID)
require.NoError(t, svc.UpdateVolumeState(volumeID, "in-use", "i-enc", "/dev/nbd0"))
require.NoError(t, svc.UpdateVolumeState(testVolAccountID, volumeID, "in-use", "i-enc", "/dev/nbd0"))
after := getStoredConfig(t, store, volumeID)
assert.Equal(t, string(before), string(after),
"UpdateVolumeState must not rewrite the sealed config.json (AES-GCM nonce reuse)")
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.NoError(t, err)
assert.Equal(t, "in-use", meta.State)
}
@@ -82,7 +82,7 @@ func TestGetVolumeMetadata_DocumentBeatsConfig(t *testing.T) {
State: "in-use", AttachedInstance: "i-overlay", DeviceName: "/dev/nbd0",
})
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.NoError(t, err)
assert.Equal(t, "in-use", meta.State,
"the ebsmetadata document must override any State in config.json")
@@ -101,7 +101,7 @@ func TestGetVolumeMetadata_ConfigAloneIsNotAVolume(t *testing.T) {
volumeID := "vol-config-only"
seedProviderConfig(t, store, volumeID)
_, err := svc.GetVolumeMetadata(volumeID)
_, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.Error(t, err)
assert.Contains(t, err.Error(), "InvalidVolume.NotFound")
}
@@ -50,14 +50,14 @@ func BenchmarkDescribeVolumes_Provider_SingleID(b *testing.B) {
b.Run(fmt.Sprintf("volumes=%d", n), func(b *testing.B) {
svc := newTestVolumeService("ap-southeast-2a")
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
targetID := seedProviderVolumes(b, svc, n, "acct-bench")
targetID := seedProviderVolumes(b, svc, n, "000000000013")
ctx := context.Background()
input := &ec2.DescribeVolumesInput{VolumeIds: []*string{aws.String(targetID)}}
defer silenceLogs(b)()
b.ResetTimer()
for i := 0; i < b.N; i++ {
if _, err := svc.DescribeVolumes(ctx, input, "acct-bench"); err != nil {
if _, err := svc.DescribeVolumes(ctx, input, "000000000013"); err != nil {
b.Fatalf("DescribeVolumes: %v", err)
}
}
@@ -75,19 +75,19 @@ func BenchmarkDescribeVolumes_Provider_ListAndFilter(b *testing.B) {
b.Run(fmt.Sprintf("volumes=%d", n), func(b *testing.B) {
svc := newTestVolumeService("ap-southeast-2a")
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
targetID := seedProviderVolumes(b, svc, n, "acct-bench")
targetID := seedProviderVolumes(b, svc, n, "000000000013")
ctx := context.Background()
defer silenceLogs(b)()
b.ResetTimer()
for i := 0; i < b.N; i++ {
metadata, err := svc.metadata.ListVolumes(ctx)
metadata, err := svc.metadata.ListAllVolumes(ctx)
if err != nil {
b.Fatalf("ListVolumes: %v", err)
b.Fatalf("ListAllVolumes: %v", err)
}
var found *ec2.Volume
for _, meta := range metadata {
if meta.TenantID != "acct-bench" || meta.VolumeID != targetID {
if meta.TenantID != "000000000013" || meta.VolumeID != targetID {
continue
}
found = metadataVolumeToEC2(meta)
@@ -21,10 +21,10 @@ func TestDescribeVolumes_Provider_FastPath_SingleID(t *testing.T) {
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
ctx := context.Background()
vol, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-1")
vol, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000001")
require.NoError(t, err)
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{vol.VolumeId}}, "acct-1")
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{vol.VolumeId}}, "000000000001")
require.NoError(t, err)
require.Len(t, out.Volumes, 1)
assert.Equal(t, aws.StringValue(vol.VolumeId), aws.StringValue(out.Volumes[0].VolumeId))
@@ -40,10 +40,10 @@ func TestDescribeVolumes_Provider_FastPath_CrossTenantIsNotFound(t *testing.T) {
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
ctx := context.Background()
otherTenantVol, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-2")
otherTenantVol, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000002")
require.NoError(t, err)
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{otherTenantVol.VolumeId}}, "acct-1")
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{otherTenantVol.VolumeId}}, "000000000001")
require.Error(t, err)
assert.EqualError(t, err, awserrors.ErrorInvalidVolumeNotFound)
assert.Nil(t, out)
@@ -56,7 +56,7 @@ func TestDescribeVolumes_Provider_FastPath_UnknownIDIsNotFound(t *testing.T) {
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
ctx := context.Background()
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{aws.String("vol-doesnotexist0")}}, "acct-1")
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{aws.String("vol-doesnotexist0")}}, "000000000001")
require.Error(t, err)
assert.EqualError(t, err, awserrors.ErrorInvalidVolumeNotFound)
assert.Nil(t, out)
@@ -70,14 +70,14 @@ func TestDescribeVolumes_Provider_FastPath_FiltersApply(t *testing.T) {
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
ctx := context.Background()
vol, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-1")
vol, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000001")
require.NoError(t, err)
t.Run("matching filter keeps the volume", func(t *testing.T) {
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{
VolumeIds: []*string{vol.VolumeId},
Filters: []*ec2.Filter{{Name: aws.String("size"), Values: []*string{aws.String("8")}}},
}, "acct-1")
}, "000000000001")
require.NoError(t, err)
require.Len(t, out.Volumes, 1)
})
@@ -86,7 +86,7 @@ func TestDescribeVolumes_Provider_FastPath_FiltersApply(t *testing.T) {
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{
VolumeIds: []*string{vol.VolumeId},
Filters: []*ec2.Filter{{Name: aws.String("size"), Values: []*string{aws.String("16")}}},
}, "acct-1")
}, "000000000001")
require.NoError(t, err)
assert.Empty(t, out.Volumes)
})
@@ -99,12 +99,12 @@ func TestDescribeVolumes_Provider_FastPath_MultipleIDs(t *testing.T) {
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
ctx := context.Background()
volA, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-1")
volA, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000001")
require.NoError(t, err)
volB, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(16), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-1")
volB, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(16), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000001")
require.NoError(t, err)
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{volA.VolumeId, volB.VolumeId}}, "acct-1")
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{VolumeIds: []*string{volA.VolumeId, volB.VolumeId}}, "000000000001")
require.NoError(t, err)
require.Len(t, out.Volumes, 2)
@@ -123,12 +123,12 @@ func TestDescribeVolumes_Provider_NoIDs_StillEnumerates(t *testing.T) {
svc.SetEBSProvider(ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}))
ctx := context.Background()
volA, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-1")
volA, err := svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(8), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000001")
require.NoError(t, err)
_, err = svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(16), AvailabilityZone: aws.String("ap-southeast-2a")}, "acct-2")
_, err = svc.CreateVolume(ctx, &ec2.CreateVolumeInput{Size: aws.Int64(16), AvailabilityZone: aws.String("ap-southeast-2a")}, "000000000002")
require.NoError(t, err)
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{}, "acct-1")
out, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{}, "000000000001")
require.NoError(t, err)
require.Len(t, out.Volumes, 1)
assert.Equal(t, aws.StringValue(volA.VolumeId), aws.StringValue(out.Volumes[0].VolumeId))
@@ -29,7 +29,7 @@ func seedAttachedVolume(t *testing.T, svc *VolumeServiceImpl, volumeID, accountI
VolumeID: volumeID, TenantID: accountID, CapacityGiB: 1,
State: "available", AvailabilityZone: "ap-southeast-2a", VolumeType: "gp3",
}))
require.NoError(t, svc.UpdateVolumeState(volumeID, "in-use", instanceID, "/dev/sdf"))
require.NoError(t, svc.UpdateVolumeState(accountID, volumeID, "in-use", instanceID, "/dev/sdf"))
}
// This is the escape from the deadlock teardown exists to break: a volume
@@ -37,11 +37,11 @@ func seedAttachedVolume(t *testing.T, svc *VolumeServiceImpl, volumeID, accountI
// the attachment is cleared without the guest's cooperation.
func TestForceDetachVolumeClearsTheAttachment(t *testing.T) {
svc := newForceDetachService(t)
seedAttachedVolume(t, svc, "vol-forcedetach1", "acct-tenant", "i-stuck0000000000")
seedAttachedVolume(t, svc, "vol-forcedetach1", "000000000011", "i-stuck0000000000")
attachment, err := svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{
VolumeId: aws.String("vol-forcedetach1"), Force: aws.Bool(true),
}, "acct-tenant")
}, "000000000011")
require.NoError(t, err)
// The previous instance is reported back: it is the only record of what
@@ -49,7 +49,7 @@ func TestForceDetachVolumeClearsTheAttachment(t *testing.T) {
assert.Equal(t, "i-stuck0000000000", aws.StringValue(attachment.InstanceId))
assert.Equal(t, "detached", aws.StringValue(attachment.State))
meta, err := svc.GetVolumeMetadata("vol-forcedetach1")
meta, err := svc.GetVolumeMetadata("000000000011", "vol-forcedetach1")
require.NoError(t, err)
assert.Equal(t, "available", meta.State)
assert.Empty(t, meta.AttachedInstance)
@@ -60,18 +60,18 @@ func TestForceDetachVolumeClearsTheAttachment(t *testing.T) {
// guard; it may never reach a volume belonging to another tenant.
func TestForceDetachVolumeRefusesAnotherAccountsVolume(t *testing.T) {
svc := newForceDetachService(t)
seedAttachedVolume(t, svc, "vol-forcedetach2", "acct-owner", "i-stuck0000000000")
seedAttachedVolume(t, svc, "vol-forcedetach2", "000000000014", "i-stuck0000000000")
_, err := svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{
VolumeId: aws.String("vol-forcedetach2"),
}, "acct-intruder")
}, "000000000015")
require.Error(t, err)
// The same answer an absent volume gives, so this cannot be used to prove
// a volume id exists in someone else's account.
assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, err.Error())
meta, err := svc.GetVolumeMetadata("vol-forcedetach2")
meta, err := svc.GetVolumeMetadata("000000000014", "vol-forcedetach2")
require.NoError(t, err)
assert.Equal(t, "i-stuck0000000000", meta.AttachedInstance)
}
@@ -80,7 +80,7 @@ func TestForceDetachVolumeRejectsBadInput(t *testing.T) {
svc := newForceDetachService(t)
for _, input := range []*ec2.DetachVolumeInput{nil, {}, {VolumeId: aws.String("")}} {
_, err := svc.ForceDetachVolume(context.Background(), input, "acct-tenant")
_, err := svc.ForceDetachVolume(context.Background(), input, "000000000011")
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error())
}
@@ -91,7 +91,7 @@ func TestForceDetachVolumeReportsAMissingVolume(t *testing.T) {
_, err := svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{
VolumeId: aws.String("vol-doesnotexist"),
}, "acct-tenant")
}, "000000000011")
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, err.Error())
@@ -102,13 +102,13 @@ func TestForceDetachVolumeReportsAMissingVolume(t *testing.T) {
func TestForceDetachVolumeOnAnUnattachedVolume(t *testing.T) {
svc := newForceDetachService(t)
require.NoError(t, svc.metadata.PutVolume(context.Background(), ebsmetadata.Volume{
VolumeID: "vol-forcedetach3", TenantID: "acct-tenant", CapacityGiB: 1,
VolumeID: "vol-forcedetach3", TenantID: "000000000011", CapacityGiB: 1,
State: "available", AvailabilityZone: "ap-southeast-2a", VolumeType: "gp3",
}))
attachment, err := svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{
VolumeId: aws.String("vol-forcedetach3"),
}, "acct-tenant")
}, "000000000011")
require.NoError(t, err)
assert.Empty(t, aws.StringValue(attachment.InstanceId))
@@ -29,7 +29,7 @@ func TestGetVolumeMetadata_Provider_ResolvesProviderCreatedVolume(t *testing.T)
volumeID := "vol-provider00001"
require.NoError(t, svc.metadata.PutVolume(context.Background(), ebsmetadata.Volume{
VolumeID: volumeID,
TenantID: "acct-provider",
TenantID: "000000000012",
CapacityGiB: 8,
State: "available",
CreatedAt: time.Now(),
@@ -39,10 +39,10 @@ func TestGetVolumeMetadata_Provider_ResolvesProviderCreatedVolume(t *testing.T)
Encrypted: true,
}))
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata("000000000012", volumeID)
require.NoError(t, err, "a provider-created volume must resolve without a legacy config.json")
assert.Equal(t, volumeID, meta.VolumeID)
assert.Equal(t, "acct-provider", meta.TenantID)
assert.Equal(t, "000000000012", meta.TenantID)
assert.Equal(t, "available", meta.State)
assert.Equal(t, "ap-southeast-2a", meta.AvailabilityZone)
assert.Equal(t, uint64(8), meta.CapacityGiB)
@@ -56,13 +56,13 @@ func TestGetVolumeMetadata_Provider_ReflectsAttachmentState(t *testing.T) {
volumeID := "vol-provider00002"
require.NoError(t, svc.metadata.PutVolume(context.Background(), ebsmetadata.Volume{
VolumeID: volumeID, TenantID: "acct-provider", CapacityGiB: 1,
VolumeID: volumeID, TenantID: "000000000012", CapacityGiB: 1,
State: "available", AvailabilityZone: "ap-southeast-2a", VolumeType: "gp3",
}))
require.NoError(t, svc.UpdateVolumeState(volumeID, "in-use", "i-0123456789abcdef0", "/dev/sdf"))
require.NoError(t, svc.UpdateVolumeState("000000000012", volumeID, "in-use", "i-0123456789abcdef0", "/dev/sdf"))
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata("000000000012", volumeID)
require.NoError(t, err)
assert.Equal(t, "in-use", meta.State)
assert.Equal(t, "i-0123456789abcdef0", meta.AttachedInstance)
@@ -72,7 +72,7 @@ func TestGetVolumeMetadata_Provider_ReflectsAttachmentState(t *testing.T) {
func TestGetVolumeMetadata_Provider_UnknownVolumeIsNotFound(t *testing.T) {
svc := newProviderVolumeService(t)
_, err := svc.GetVolumeMetadata("vol-doesnotexist0")
_, err := svc.GetVolumeMetadata(testVolAccountID, "vol-doesnotexist0")
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, err.Error())
}
@@ -61,7 +61,7 @@ func seedVolume(t *testing.T, svc *VolumeServiceImpl, volID, state, attachedInst
// metadataKey is the object key the control-plane document for volumeID lives at.
func metadataKey(t *testing.T, volumeID string) string {
t.Helper()
key, err := ebsmetadata.VolumeKey(volumeID)
key, err := ebsmetadata.VolumeKey(testVolAccountID, volumeID)
require.NoError(t, err)
return key
}
@@ -109,7 +109,7 @@ func TestVolumeTagMirror_WritesOnlyControlPlaneDocument(t *testing.T) {
seedVolumeDocument(t, memoryStore, ebsmetadata.Volume{
VolumeID: volumeID, TenantID: testVolAccountID, CapacityGiB: 10, State: "available",
})
require.NoError(t, svc.UpdateVolumeState(volumeID, "in-use", "i-live0000000000", "/dev/nbd0"))
require.NoError(t, svc.UpdateVolumeState(testVolAccountID, volumeID, "in-use", "i-live0000000000", "/dev/nbd0"))
before := getStoredConfig(t, memoryStore, volumeID)
store := &recordingObjectStore{ObjectStore: memoryStore}
svc.store = store
@@ -191,7 +191,7 @@ func TestRLC5_VolumeGCMarksLeakedVolumeNeverDeletes(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, 1, marked, "ADR-0005 §3: the GC must surface exactly the leaked volume")
leaked, err := svc.GetVolumeMetadata("vol-leaked0000000")
leaked, err := svc.GetVolumeMetadata(testVolAccountID, "vol-leaked0000000")
require.NoErrorf(t, err, "ADR-0005 §3: the GC must NOT delete the leaked volume — it carries data; it may only mark + alarm")
assert.NotEmpty(t, leaked.Tags[orphanTagKey],
"ADR-0005 §3: a leaked volume must be marked orphaned")
@@ -200,7 +200,7 @@ func TestRLC5_VolumeGCMarksLeakedVolumeNeverDeletes(t *testing.T) {
// A volume attached to an instance NOT in the leaked set must be untouched —
// node-local detection never false-marks another node's live-instance volume.
live, err := svc.GetVolumeMetadata("vol-live000000000")
live, err := svc.GetVolumeMetadata(testVolAccountID, "vol-live000000000")
require.NoError(t, err)
assert.Empty(t, live.Tags[orphanTagKey],
"a volume on a live instance must never be marked orphaned")
@@ -209,6 +209,6 @@ func TestRLC5_VolumeGCMarksLeakedVolumeNeverDeletes(t *testing.T) {
marked, err = reaper.Sweep(context.Background())
require.NoError(t, err)
assert.Equal(t, 0, marked, "an already-marked orphan must not be re-marked")
_, err = svc.GetVolumeMetadata("vol-leaked0000000")
_, err = svc.GetVolumeMetadata(testVolAccountID, "vol-leaked0000000")
require.NoError(t, err, "ADR-0005 §3: the volume must still exist after repeated sweeps")
}
@@ -2,14 +2,11 @@ package handlers_ec2_volume
import (
"context"
"encoding/json"
"fmt"
"strings"
"testing"
"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/config"
"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
"github.com/mulgadc/spinifex/spinifex/ebsprovider"
@@ -52,14 +49,9 @@ func newSnapshotSourceProvider() *snapshotSourceProvider {
// CreateVolume fails closed on a snapshot whose owner it cannot match.
func writeSnapshotMetadata(t *testing.T, store objectstore.ObjectStore, snapshotID, sourceVolumeID, ownerID string, sizeGiB int64) {
t.Helper()
data, err := json.Marshal(snapshotMetadata{VolumeID: sourceVolumeID, VolumeSize: sizeGiB, OwnerID: ownerID})
require.NoError(t, err)
_, err = store.PutObject(context.Background(), &s3.PutObjectInput{
Bucket: aws.String("test-bucket"),
Key: aws.String(snapshotID + "/metadata.json"),
Body: strings.NewReader(string(data)),
})
require.NoError(t, err)
require.NoError(t, ebsmetadata.NewStore(store, "test-bucket").PutSnapshot(context.Background(), ebsmetadata.Snapshot{
SnapshotID: snapshotID, VolumeID: sourceVolumeID, VolumeSize: sizeGiB, OwnerID: ownerID,
}))
}
// TestCreateVolume_Provider_FromSnapshotSendsSourceVolumeID locks that a
@@ -72,11 +64,11 @@ func TestCreateVolume_Provider_FromSnapshotSendsSourceVolumeID(t *testing.T) {
provider := newSnapshotSourceProvider()
svc.SetEBSProvider(provider)
writeSnapshotMetadata(t, store, "snap-src", "vol-origin", "acct-1", 8)
writeSnapshotMetadata(t, store, "snap-src", "vol-origin", "000000000001", 8)
created, err := svc.CreateVolume(context.Background(), &ec2.CreateVolumeInput{
Size: aws.Int64(8), AvailabilityZone: aws.String(providerTestAZ), SnapshotId: aws.String("snap-src"),
}, "acct-1")
}, "000000000001")
require.NoError(t, err)
require.NotNil(t, created)
@@ -96,7 +88,7 @@ func TestCreateVolume_Provider_WithoutSnapshotSendsNoSource(t *testing.T) {
_, err := svc.CreateVolume(context.Background(), &ec2.CreateVolumeInput{
Size: aws.Int64(8), AvailabilityZone: aws.String(providerTestAZ),
}, "acct-1")
}, "000000000001")
require.NoError(t, err)
assert.Empty(t, provider.lastRequest.SourceSnapshotID)
@@ -150,7 +142,7 @@ func TestDescribeVolumes_Provider_SeesInstanceRootVolume(t *testing.T) {
sourceVolumeID: "vol-origin",
}, nil, nil, nil)
instance := &vm.VM{AccountID: "acct-1"}
instance := &vm.VM{AccountID: "000000000001"}
volumeInfos, err := instanceSvc.GenerateVolumes(context.Background(), &ec2.RunInstancesInput{
ImageId: aws.String("ami-1"),
}, instance)
@@ -161,7 +153,7 @@ func TestDescribeVolumes_Provider_SeesInstanceRootVolume(t *testing.T) {
volumeSvc := NewVolumeServiceImplWithStore(cfg, store, nil)
volumeSvc.SetEBSProvider(provider)
out, err := volumeSvc.DescribeVolumes(context.Background(), &ec2.DescribeVolumesInput{}, "acct-1")
out, err := volumeSvc.DescribeVolumes(context.Background(), &ec2.DescribeVolumesInput{}, "000000000001")
require.NoError(t, err)
require.Len(t, out.Volumes, 1, "the instance root volume must be visible to DescribeVolumes")
@@ -27,7 +27,7 @@ func TestVolumeRecordTagsMirror(t *testing.T) {
},
}, testVolAccountID))
meta, err := svc.GetVolumeMetadata("vol-tagmirror0001")
meta, err := svc.GetVolumeMetadata(testVolAccountID, "vol-tagmirror0001")
require.NoError(t, err)
assert.Equal(t, "yes", meta.Tags["keep"])
assert.Equal(t, "v", meta.Tags["drop"])
@@ -41,7 +41,7 @@ func TestVolumeRecordTagsMirror(t *testing.T) {
},
}, testVolAccountID))
meta, err = svc.GetVolumeMetadata("vol-tagmirror0001")
meta, err = svc.GetVolumeMetadata(testVolAccountID, "vol-tagmirror0001")
require.NoError(t, err)
assert.Equal(t, "yes", meta.Tags["keep"])
_, ok := meta.Tags["drop"]
@@ -94,7 +94,7 @@ func TestApplyRecordTags_SurvivesStaleConfigRewrite(t *testing.T) {
})
require.NoError(t, err)
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.NoError(t, err)
assert.Equal(t, "retain", meta.Tags["sweep"])
@@ -126,7 +126,7 @@ func TestApplyRecordTags_MergesWithExistingTags(t *testing.T) {
Tags: []*ec2.Tag{{Key: aws.String("added-later"), Value: aws.String("yes")}},
}, testVolAccountID))
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.NoError(t, err)
assert.Equal(t, map[string]string{
"created-with": "volume",
@@ -151,7 +151,7 @@ func TestRemoveRecordTags_LastTagLeavesEmptySet(t *testing.T) {
Tags: []*ec2.Tag{{Key: aws.String("legacy"), Value: aws.String("tag")}},
}, testVolAccountID))
meta, err := svc.GetVolumeMetadata(volumeID)
meta, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.NoError(t, err)
assert.Empty(t, meta.Tags, "the deleted tag must not come back from anywhere")
}
@@ -167,7 +167,7 @@ func TestVolumeRecordTagsMirror_CrossTenantNoMutation(t *testing.T) {
Tags: []*ec2.Tag{{Key: aws.String("evil"), Value: aws.String("1")}},
}, "999999999999"))
meta, err := svc.GetVolumeMetadata("vol-tenantguard01")
meta, err := svc.GetVolumeMetadata(testVolAccountID, "vol-tenantguard01")
require.NoError(t, err)
assert.Empty(t, meta.Tags)
}
+78 -115
View File
@@ -5,7 +5,6 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"log/slog"
"strings"
"sync"
@@ -15,7 +14,6 @@ import (
"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/bluebottle/pkg/safecast"
"github.com/mulgadc/spinifex/spinifex/awserrors"
"github.com/mulgadc/spinifex/spinifex/config"
@@ -120,6 +118,12 @@ func (s *VolumeServiceImpl) CreateVolume(ctx context.Context, input *ec2.CreateV
return nil, errors.New(awserrors.ErrorInvalidParameterValue)
}
// The owning account is a key segment, so an untenanted volume has nowhere
// to be written. Refuse at the boundary rather than in the key builder.
if accountID == "" {
return nil, errors.New(awserrors.ErrorAuthFailure)
}
// Validate volume type: only gp3 supported (or empty defaults to gp3)
if input.VolumeType != nil && *input.VolumeType != "" && *input.VolumeType != types.VolumeTypeGP3 {
return nil, errors.New(awserrors.ErrorUnknownVolumeType)
@@ -141,19 +145,13 @@ func (s *VolumeServiceImpl) CreateVolume(ctx context.Context, input *ec2.CreateV
if input.SnapshotId != nil && *input.SnapshotId != "" {
snapshotID = *input.SnapshotId
snapMeta, err := s.getSnapshotMetadata(ctx, snapshotID)
// The ownership check is the key: a snapshot outside the caller's prefix
// is not found, so the endpoint never confirms another account's IDs.
snapMeta, err := s.metadata.GetSnapshot(ctx, accountID, snapshotID)
if err != nil {
slog.ErrorContext(ctx, "CreateVolume: snapshot not found", "snapshotId", snapshotID, "err", err)
return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
}
// Not-found rather than access-denied so the endpoint does not confirm
// another account's snapshot IDs. An unset owner_id (pre-ownership
// snapshot) fails closed.
if snapMeta.OwnerID == "" || snapMeta.OwnerID != accountID {
slog.WarnContext(ctx, "CreateVolume: account does not own snapshot",
"snapshotId", snapshotID, "accountID", accountID, "ownerID", snapMeta.OwnerID)
return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
}
sourceVolumeName = snapMeta.VolumeID
snapshotSizeGiB = snapMeta.VolumeSize
}
@@ -322,16 +320,14 @@ func (s *VolumeServiceImpl) DescribeVolumes(ctx context.Context, input *ec2.Desc
return &ec2.DescribeVolumesOutput{Volumes: volumes}, nil
}
// Slow path: no specific IDs requested, enumerate the bucket.
metadata, err := s.metadata.ListVolumes(ctx)
// Slow path: no specific IDs requested, list the caller's prefix. No other
// account's document is read, so none has to be filtered out here.
metadata, err := s.metadata.ListVolumes(ctx, accountID)
if err != nil {
return nil, errors.New(awserrors.ErrorServerInternal)
}
volumes := make([]*ec2.Volume, 0, len(metadata))
for _, meta := range metadata {
if meta.TenantID != accountID {
continue
}
volume := metadataVolumeToEC2(meta)
if len(parsedFilters) == 0 || volumeMatchesFilters(volume, parsedFilters) {
volumes = append(volumes, volume)
@@ -488,27 +484,19 @@ func volumeStatusMatchesFilters(item *ec2.VolumeStatusItem, filters map[string][
// static: the control plane has no per-volume health signal to report, so
// every known volume reports ok.
func (s *VolumeServiceImpl) describeVolumeStatus(ctx context.Context, input *ec2.DescribeVolumeStatusInput, accountID string, parsedFilters map[string][]string) (*ec2.DescribeVolumeStatusOutput, error) {
metas, err := s.metadata.ListVolumes(ctx)
if err != nil {
return nil, errors.New(awserrors.ErrorServerInternal)
}
byID := make(map[string]ebsmetadata.Volume, len(metas))
for _, meta := range metas {
byID[meta.VolumeID] = meta
}
var statusItems []*ec2.VolumeStatusItem
// Fast path: specific volume IDs requested. Fetch each document directly
// rather than listing the prefix to answer for a handful of volumes.
if len(input.VolumeIds) > 0 {
for _, vid := range input.VolumeIds {
if vid == nil {
continue
for _, r := range s.fetchVolumeMetasByIDs(ctx, input.VolumeIds, accountID) {
if r.err != nil {
return nil, r.err
}
meta, ok := byID[*vid]
if !ok || meta.TenantID != accountID {
return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound)
if r.absent {
continue // nil VolumeIds entry
}
item := metadataVolumeStatusItem(meta)
item := metadataVolumeStatusItem(r.meta)
if len(parsedFilters) > 0 && !volumeStatusMatchesFilters(item, parsedFilters) {
continue
}
@@ -518,10 +506,11 @@ func (s *VolumeServiceImpl) describeVolumeStatus(ctx context.Context, input *ec2
return &ec2.DescribeVolumeStatusOutput{VolumeStatuses: statusItems}, nil
}
metas, err := s.metadata.ListVolumes(ctx, accountID)
if err != nil {
return nil, errors.New(awserrors.ErrorServerInternal)
}
for _, meta := range metas {
if meta.TenantID != accountID {
continue
}
item := metadataVolumeStatusItem(meta)
if len(parsedFilters) > 0 && !volumeStatusMatchesFilters(item, parsedFilters) {
continue
@@ -699,7 +688,7 @@ func (s *VolumeServiceImpl) DescribeVolumesModifications(ctx context.Context, in
return &ec2.DescribeVolumesModificationsOutput{VolumesModifications: modifications}, nil
}
metas, err := s.metadata.ListVolumes(ctx)
metas, err := s.metadata.ListVolumes(ctx, accountID)
if err != nil {
return nil, errors.New(awserrors.ErrorServerInternal)
}
@@ -710,9 +699,6 @@ func (s *VolumeServiceImpl) DescribeVolumesModifications(ctx context.Context, in
}
for _, meta := range metas {
if meta.TenantID != accountID {
continue
}
if len(volumeIDFilterValues) > 0 && !filterutil.MatchesAny(volumeIDFilterValues, meta.VolumeID) {
continue
}
@@ -751,7 +737,7 @@ func (s *VolumeServiceImpl) fetchVolumeModificationsByIDs(ctx context.Context, v
wg.Add(1)
go func(idx int, volID string) {
defer wg.Done()
meta, err := s.metadata.GetVolume(ctx, volID)
meta, err := s.metadata.GetVolume(ctx, accountID, volID)
if err != nil || meta.TenantID != accountID {
results[idx] = volumeModificationResult{err: errors.New(awserrors.ErrorInvalidVolumeNotFound)}
return
@@ -764,20 +750,6 @@ func (s *VolumeServiceImpl) fetchVolumeModificationsByIDs(ctx context.Context, v
return results
}
// listAllVolumeIDs lists every volume ID the control plane knows about, taking
// ebsmetadata as the index.
func (s *VolumeServiceImpl) listAllVolumeIDs(ctx context.Context) ([]string, error) {
volumes, err := s.metadata.ListVolumes(ctx)
if err != nil {
return nil, errors.New(awserrors.ErrorServerInternal)
}
ids := make([]string, 0, len(volumes))
for _, v := range volumes {
ids = append(ids, v.VolumeID)
}
return ids, nil
}
// volumeFetchResult bundles a single volume-ID lookup result so the fast path
// can preserve input ordering and surface per-ID errors after the parallel
// fan-out, matching fetchVolumeModificationsByIDs.
@@ -787,34 +759,62 @@ type volumeFetchResult struct {
}
// fetchVolumesByIDs fetches each requested volume's ebsmetadata document
// directly via GetVolume, instead of enumerating every volume in the bucket
// via ListVolumes. A missing document or a cross-tenant volume both surface as
// InvalidVolume.NotFound; any other store error surfaces as ErrorServerInternal.
// directly via GetVolume, instead of listing the account's prefix.
func (s *VolumeServiceImpl) fetchVolumesByIDs(ctx context.Context, volumeIDs []*string, accountID string) []volumeFetchResult {
results := make([]volumeFetchResult, len(volumeIDs))
metas := s.fetchVolumeMetasByIDs(ctx, volumeIDs, accountID)
results := make([]volumeFetchResult, len(metas))
for i, r := range metas {
switch {
case r.err != nil:
results[i] = volumeFetchResult{err: r.err}
case r.absent:
default:
results[i] = volumeFetchResult{volume: metadataVolumeToEC2(r.meta)}
}
}
return results
}
// volumeMetaResult bundles a per-ID document lookup. absent marks a nil entry
// in the requested IDs, which is neither a document nor an error.
type volumeMetaResult struct {
meta ebsmetadata.Volume
absent bool
err error
}
// fetchVolumeMetasByIDs reads each requested volume's document in parallel,
// returning results positionally aligned with volumeIDs. A missing document or
// a cross-tenant volume both surface as InvalidVolume.NotFound; any other store
// error surfaces as ErrorServerInternal.
func (s *VolumeServiceImpl) fetchVolumeMetasByIDs(ctx context.Context, volumeIDs []*string, accountID string) []volumeMetaResult {
results := make([]volumeMetaResult, len(volumeIDs))
var wg sync.WaitGroup
for i, id := range volumeIDs {
if id == nil {
results[i] = volumeMetaResult{absent: true}
continue
}
wg.Add(1)
go func(idx int, volID string) {
defer wg.Done()
meta, err := s.metadata.GetVolume(ctx, volID)
meta, err := s.metadata.GetVolume(ctx, accountID, volID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
results[idx] = volumeFetchResult{err: errors.New(awserrors.ErrorInvalidVolumeNotFound)}
results[idx] = volumeMetaResult{err: errors.New(awserrors.ErrorInvalidVolumeNotFound)}
} else {
results[idx] = volumeFetchResult{err: errors.New(awserrors.ErrorServerInternal)}
results[idx] = volumeMetaResult{err: errors.New(awserrors.ErrorServerInternal)}
}
return
}
// The read was scoped to the caller's prefix, so the document is
// theirs by construction. The comparison stays as an assertion.
if meta.TenantID != accountID {
results[idx] = volumeFetchResult{err: errors.New(awserrors.ErrorInvalidVolumeNotFound)}
results[idx] = volumeMetaResult{err: errors.New(awserrors.ErrorInvalidVolumeNotFound)}
return
}
results[idx] = volumeFetchResult{volume: metadataVolumeToEC2(meta)}
results[idx] = volumeMetaResult{meta: meta}
}(i, *id)
}
@@ -869,15 +869,15 @@ func metadataVolumeToEC2(meta ebsmetadata.Volume) *ec2.Volume {
// GetVolumeMetadata reads the control-plane document for a volume. It is the
// ownership and attachment record the daemon validates attach/detach against.
func (s *VolumeServiceImpl) GetVolumeMetadata(volumeID string) (ebsmetadata.Volume, error) {
return s.getVolumeMetadata(context.Background(), volumeID)
func (s *VolumeServiceImpl) GetVolumeMetadata(accountID, volumeID string) (ebsmetadata.Volume, error) {
return s.getVolumeMetadata(context.Background(), accountID, volumeID)
}
// getVolumeMetadata is GetVolumeMetadata carrying the caller's context. A
// missing document is remapped to InvalidVolume.NotFound so callers surface the
// AWS error rather than a store error.
func (s *VolumeServiceImpl) getVolumeMetadata(ctx context.Context, volumeID string) (ebsmetadata.Volume, error) {
meta, err := s.metadata.GetVolume(ctx, volumeID)
func (s *VolumeServiceImpl) getVolumeMetadata(ctx context.Context, accountID, volumeID string) (ebsmetadata.Volume, error) {
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
slog.WarnContext(ctx, "volume metadata not found", "volumeId", volumeID)
@@ -890,9 +890,9 @@ func (s *VolumeServiceImpl) getVolumeMetadata(ctx context.Context, volumeID stri
// UpdateVolumeState updates the control-plane-owned attachment state (state,
// attachment, device) on the volume's ebsmetadata document.
func (s *VolumeServiceImpl) UpdateVolumeState(volumeID, state, attachedInstance, deviceName string) error {
func (s *VolumeServiceImpl) UpdateVolumeState(accountID, volumeID, state, attachedInstance, deviceName string) error {
ctx := context.Background()
meta, err := s.metadata.GetVolume(ctx, volumeID)
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
return fmt.Errorf("failed to get volume metadata: %w", err)
}
@@ -932,7 +932,7 @@ func (s *VolumeServiceImpl) ModifyVolume(ctx context.Context, input *ec2.ModifyV
return nil, err
}
meta, err := s.metadata.GetVolume(ctx, volumeID)
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil || meta.TenantID != accountID {
return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound)
}
@@ -1007,7 +1007,7 @@ func (s *VolumeServiceImpl) ModifyVolume(ctx context.Context, input *ec2.ModifyV
// is always a metadata-only clear, never a QMP call. Errors from either step
// are returned, not swallowed.
func (s *VolumeServiceImpl) DeleteVolumeOnTerminate(ctx context.Context, volumeID, accountID string) error {
if err := s.UpdateVolumeState(volumeID, "available", "", ""); err != nil {
if err := s.UpdateVolumeState(accountID, volumeID, "available", "", ""); err != nil {
// No metadata doc means the volume is already gone, per GetVolume's own
// contract, so absence here is success, not a teardown failure.
if objectstore.IsNoSuchKeyError(err) {
@@ -1025,8 +1025,8 @@ func (s *VolumeServiceImpl) DeleteVolumeOnTerminate(ctx context.Context, volumeI
// available rather than deleting it. Metadata-only, no QMP — same rationale
// as DeleteVolumeOnTerminate: there is no live QEMU to hot-unplug on either
// terminate path.
func (s *VolumeServiceImpl) DetachVolumeOnTerminate(_ context.Context, volumeID, _ string) error {
if err := s.UpdateVolumeState(volumeID, "available", "", ""); err != nil {
func (s *VolumeServiceImpl) DetachVolumeOnTerminate(_ context.Context, volumeID, accountID string) error {
if err := s.UpdateVolumeState(accountID, volumeID, "available", "", ""); err != nil {
// No metadata doc means the volume is already gone, per GetVolume's own
// contract, so absence here is success, not a teardown failure.
if objectstore.IsNoSuchKeyError(err) {
@@ -1052,7 +1052,7 @@ func (s *VolumeServiceImpl) ForceDetachVolume(ctx context.Context, input *ec2.De
}
volumeID := *input.VolumeId
meta, err := s.metadata.GetVolume(ctx, volumeID)
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound)
}
@@ -1061,7 +1061,7 @@ func (s *VolumeServiceImpl) ForceDetachVolume(ctx context.Context, input *ec2.De
}
previous := meta.AttachedInstance
if err := s.UpdateVolumeState(volumeID, "available", "", ""); err != nil {
if err := s.UpdateVolumeState(accountID, volumeID, "available", "", ""); err != nil {
slog.ErrorContext(ctx, "ForceDetachVolume: failed to clear attachment", "volumeId", volumeID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
@@ -1090,7 +1090,7 @@ func (s *VolumeServiceImpl) DeleteVolume(ctx context.Context, input *ec2.DeleteV
}
// AWS-faithful: an absent volume returns InvalidVolume.NotFound.
meta, err := s.metadata.GetVolume(ctx, volumeID)
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound)
}
@@ -1118,7 +1118,7 @@ func (s *VolumeServiceImpl) DeleteVolume(ctx context.Context, input *ec2.DeleteV
slog.ErrorContext(ctx, "DeleteVolume: provider deletion failed", "volumeId", volumeID, "err", err)
return nil, errors.New(awserrors.ErrorServerInternal)
}
if err := s.metadata.DeleteVolume(ctx, volumeID); err != nil {
if err := s.metadata.DeleteVolume(ctx, accountID, volumeID); err != nil {
return nil, errors.New(awserrors.ErrorServerInternal)
}
@@ -1127,43 +1127,6 @@ func (s *VolumeServiceImpl) DeleteVolume(ctx context.Context, input *ec2.DeleteV
return &ec2.DeleteVolumeOutput{}, nil
}
// snapshotMetadata holds the subset of snapshot metadata needed by CreateVolume.
// Matches the JSON written by the snapshot service's SnapshotConfig.
type snapshotMetadata struct {
VolumeID string `json:"volume_id"`
VolumeSize int64 `json:"volume_size"`
OwnerID string `json:"owner_id"`
}
// getSnapshotMetadata reads snapshot metadata.json from S3 for CreateVolume.
func (s *VolumeServiceImpl) getSnapshotMetadata(ctx context.Context, snapshotID string) (*snapshotMetadata, error) {
key := snapshotID + "/metadata.json"
getResult, err := s.store.GetObject(ctx, &s3.GetObjectInput{
Bucket: aws.String(s.bucketName),
Key: aws.String(key),
})
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
}
return nil, fmt.Errorf("failed to get snapshot metadata: %w", err)
}
defer getResult.Body.Close()
body, err := io.ReadAll(getResult.Body)
if err != nil {
return nil, fmt.Errorf("failed to read snapshot metadata: %w", err)
}
var meta snapshotMetadata
if err := json.Unmarshal(body, &meta); err != nil {
return nil, fmt.Errorf("failed to decode snapshot metadata: %w", err)
}
return &meta, nil
}
// checkVolumeHasNoSnapshots checks if a volume has dependent snapshots
// using the JetStream KV index.
func (s *VolumeServiceImpl) checkVolumeHasNoSnapshots(ctx context.Context, volumeID string) error {
@@ -1237,7 +1200,7 @@ func (s *VolumeServiceImpl) mirrorVolumeTags(ctx context.Context, resources []*s
// mirrorVolumeTagsOne read-modify-writes one ebsmetadata.Volume's Tags field.
func (s *VolumeServiceImpl) mirrorVolumeTagsOne(ctx context.Context, volumeID, accountID string, mut func(map[string]string)) error {
meta, err := s.metadata.GetVolume(ctx, volumeID)
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
if objectstore.IsNoSuchKeyError(err) {
return nil
@@ -26,7 +26,7 @@ func TestGetVolumeMetadata_CorruptJSON(t *testing.T) {
const volumeID = "vol-bad-json"
putRawVolumeDocument(t, store, volumeID, "not valid json {{{")
_, err := svc.GetVolumeMetadata(volumeID)
_, err := svc.GetVolumeMetadata(testVolAccountID, volumeID)
require.Error(t, err)
assert.ErrorIs(t, err, ebsmetadata.ErrCorruptDocument)
}
@@ -61,11 +61,11 @@ func TestCreateVolume_EncryptionKeyLoadError(t *testing.T) {
_, err := svc.CreateVolume(context.Background(), &ec2.CreateVolumeInput{
Size: aws.Int64(1),
AvailabilityZone: aws.String("ap-southeast-2a"),
}, "")
}, testVolAccountID)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
out, err := svc.DescribeVolumes(context.Background(), &ec2.DescribeVolumesInput{}, "")
out, err := svc.DescribeVolumes(context.Background(), &ec2.DescribeVolumesInput{}, testVolAccountID)
require.NoError(t, err)
assert.Empty(t, out.Volumes, "a failed create must not leave a volume behind")
}
File diff suppressed because it is too large Load Diff
@@ -26,6 +26,11 @@ func startTestNATS(t *testing.T) *nats.Conn {
// exist as far as the control plane is concerned.
func seedVolumeDocument(t *testing.T, store objectstore.ObjectStore, volume ebsmetadata.Volume) {
t.Helper()
// The owning account is a key segment, so a fixture that does not care
// which account owns it still has to have one.
if volume.TenantID == "" {
volume.TenantID = testVolAccountID
}
require.NoError(t, ebsmetadata.NewStore(store, "test-bucket").PutVolume(context.Background(), volume))
}
@@ -53,23 +53,23 @@ func (r *VolumeLeakReaper) Sweep(ctx context.Context) (int, error) {
return 0, nil
}
ids, err := r.svc.listAllVolumeIDs(ctx)
// The sweep is node-local GC with no caller account, so each volume's own
// document supplies the account its key is under. Listing documents rather
// than IDs is what makes that available without a second read.
volumes, err := r.svc.metadata.ListAllVolumes(ctx)
if err != nil {
return 0, err
}
marked := 0
for _, id := range ids {
for _, meta := range volumes {
select {
case <-ctx.Done():
return marked, ctx.Err()
default:
}
meta, err := r.svc.metadata.GetVolume(ctx, id)
if err != nil {
continue
}
id := meta.VolumeID
attachedInstance, tags, sizeGiB := meta.AttachedInstance, meta.Tags, meta.CapacityGiB
if attachedInstance == "" || !leaked[attachedInstance] {
@@ -79,7 +79,7 @@ func (r *VolumeLeakReaper) Sweep(ctx context.Context) (int, error) {
continue // already surfaced; idempotent
}
if err := r.svc.markVolumeOrphaned(ctx, id, tags, attachedInstance); err != nil {
if err := r.svc.markVolumeOrphaned(ctx, meta.TenantID, id, tags, attachedInstance); err != nil {
slog.Error("volume-leak: failed to mark orphaned volume", "volumeId", id, "err", err)
continue
}
@@ -92,7 +92,7 @@ func (r *VolumeLeakReaper) Sweep(ctx context.Context) (int, error) {
// owning instance is definitively terminated (this node's leaked
// set), so nothing still claims the volume; clearing the stale
// attachment lets an operator DeleteVolume it after seeing the alarm.
if err := r.svc.UpdateVolumeState(id, "available", "", ""); err != nil {
if err := r.svc.UpdateVolumeState(meta.TenantID, id, "available", "", ""); err != nil {
slog.Error("volume-leak: failed to reconcile orphaned volume attachment", "volumeId", id, "err", err)
}
marked++
@@ -103,14 +103,14 @@ func (r *VolumeLeakReaper) Sweep(ctx context.Context) (int, error) {
// markVolumeOrphaned tags the volume as orphaned. tags is the volume's
// current, already-authoritative tag set; the volume's data and state remain
// untouched.
func (s *VolumeServiceImpl) markVolumeOrphaned(ctx context.Context, volumeID string, tags map[string]string, attachedInstance string) error {
func (s *VolumeServiceImpl) markVolumeOrphaned(ctx context.Context, accountID, volumeID string, tags map[string]string, attachedInstance string) error {
if tags == nil {
tags = make(map[string]string)
}
tags[orphanTagKey] = time.Now().UTC().Format(time.RFC3339)
tags[orphanInstanceTagKey] = attachedInstance
meta, err := s.metadata.GetVolume(ctx, volumeID)
meta, err := s.metadata.GetVolume(ctx, accountID, volumeID)
if err != nil {
return err
}
@@ -28,7 +28,7 @@ func TestVolumeLeakReaper_ReconcilesAttachmentNeverDeletesData(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, 1, marked)
meta, err := svc.GetVolumeMetadata("vol-leaked-reconcile")
meta, err := svc.GetVolumeMetadata(testVolAccountID, "vol-leaked-reconcile")
require.NoError(t, err, "the reaper must never delete the volume, only mark + reconcile it")
assert.NotEmpty(t, meta.Tags[orphanTagKey], "the volume must still be marked orphaned")
assert.Equal(t, "available", meta.State,
@@ -62,7 +62,7 @@ func TestVolumeLeakReaper_LiveOwnerAttachmentUntouched(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, 0, marked)
meta, err := svc.GetVolumeMetadata("vol-live-reconcile")
meta, err := svc.GetVolumeMetadata(testVolAccountID, "vol-live-reconcile")
require.NoError(t, err)
assert.Equal(t, "in-use", meta.State, "a live-owner volume's attachment must never be reconciled")
assert.Equal(t, "i-running00000000", meta.AttachedInstance)
@@ -0,0 +1,150 @@
// These tests reach svc.metadata and the unexported describes, all
// package-internal.
//
//test:in-package
package handlers_ec2_volume
import (
"context"
"testing"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/mulgadc/spinifex/spinifex/config"
"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
"github.com/mulgadc/spinifex/spinifex/testutil/recordingstore"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
const otherVolAccountID = "210987654321"
// volumePartitionService returns a service over a recording store holding one
// volume for each of two accounts, with the seeding writes already forgotten.
func volumePartitionService(t *testing.T) (*VolumeServiceImpl, *recordingstore.Store) {
t.Helper()
objects := recordingstore.New()
cfg := &config.Config{AZ: "ap-southeast-2a", Predastore: config.PredastoreConfig{Bucket: "test-bucket"}}
svc := NewVolumeServiceImplWithStore(cfg, objects, nil)
ctx := context.Background()
for _, volume := range []ebsmetadata.Volume{
{VolumeID: "vol-mine", TenantID: testVolAccountID, CapacityGiB: 8, State: "available",
AvailabilityZone: "ap-southeast-2a"},
{VolumeID: "vol-theirs", TenantID: otherVolAccountID, CapacityGiB: 16, State: "available",
AvailabilityZone: "ap-southeast-2a"},
} {
require.NoError(t, svc.metadata.PutVolume(ctx, volume))
}
objects.Reset()
return svc, objects
}
// otherAccountPrefix is spelled out rather than derived: it is the layout claim
// itself, and deriving it from the code under test would assert nothing.
const otherAccountPrefix = "spinifex/ebsmetadata/v2/volumes/" + otherVolAccountID + "/"
// Isolation must be enforced *by* the read, not by a filter after it. A
// whole-tree listing plus a correct in-memory filter would return the same
// volumes, so this asserts the access pattern: the listing prefix is the
// caller's own, and no key under another account's prefix is fetched.
func TestDescribeVolumes_TouchesNoOtherAccountsPrefix(t *testing.T) {
svc, objects := volumePartitionService(t)
listed, err := svc.DescribeVolumes(context.Background(), &ec2.DescribeVolumesInput{}, testVolAccountID)
require.NoError(t, err)
require.Len(t, listed.Volumes, 1)
assert.Equal(t, "vol-mine", aws.StringValue(listed.Volumes[0].VolumeId))
assert.False(t, objects.TouchedPrefix(otherAccountPrefix),
"a describe must not read or list another account's prefix: %v %v", objects.ListPrefixes(), objects.Gets())
}
func TestDescribeVolumeStatus_TouchesNoOtherAccountsPrefix(t *testing.T) {
svc, objects := volumePartitionService(t)
listed, err := svc.DescribeVolumeStatus(context.Background(), &ec2.DescribeVolumeStatusInput{}, testVolAccountID)
require.NoError(t, err)
require.Len(t, listed.VolumeStatuses, 1)
assert.Equal(t, "vol-mine", aws.StringValue(listed.VolumeStatuses[0].VolumeId))
assert.False(t, objects.TouchedPrefix(otherAccountPrefix),
"a status describe must not read or list another account's prefix: %v %v", objects.ListPrefixes(), objects.Gets())
}
func TestDescribeVolumesModifications_TouchesNoOtherAccountsPrefix(t *testing.T) {
svc, objects := volumePartitionService(t)
_, err := svc.DescribeVolumesModifications(context.Background(), &ec2.DescribeVolumesModificationsInput{}, testVolAccountID)
require.NoError(t, err)
assert.False(t, objects.TouchedPrefix(otherAccountPrefix),
"a modifications describe must not read or list another account's prefix: %v %v", objects.ListPrefixes(), objects.Gets())
}
// Given explicit IDs the status describe fetches each document, exactly as
// DescribeVolumes does. Enumerating the prefix to answer for one volume is the
// bug this fast path exists to remove, so the assertion is that nothing lists.
func TestDescribeVolumeStatus_ByIDDoesNotList(t *testing.T) {
svc, objects := volumePartitionService(t)
listed, err := svc.DescribeVolumeStatus(context.Background(),
&ec2.DescribeVolumeStatusInput{VolumeIds: []*string{aws.String("vol-mine")}}, testVolAccountID)
require.NoError(t, err)
require.Len(t, listed.VolumeStatuses, 1)
assert.Equal(t, "vol-mine", aws.StringValue(listed.VolumeStatuses[0].VolumeId))
assert.Empty(t, objects.ListPrefixes(), "a by-ID status describe must fetch documents, not enumerate a prefix")
}
// A volume outside the caller's prefix is absent rather than denied: the key is
// the ownership check, and the by-ID path reports it as not-found.
func TestDescribeVolumeStatus_ByIDOtherAccountIsNotFound(t *testing.T) {
svc, _ := volumePartitionService(t)
_, err := svc.DescribeVolumeStatus(context.Background(),
&ec2.DescribeVolumeStatusInput{VolumeIds: []*string{aws.String("vol-theirs")}}, testVolAccountID)
require.Error(t, err)
assert.Contains(t, err.Error(), "InvalidVolume.NotFound")
}
// A caller with no account cannot name a prefix, so the listing fails rather
// than widening to every account's volumes.
func TestVolumeDescribes_RefuseACallerWithNoAccount(t *testing.T) {
svc, _ := volumePartitionService(t)
ctx := context.Background()
for name, describe := range map[string]func() error{
"DescribeVolumes": func() error {
_, err := svc.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{}, "")
return err
},
"DescribeVolumeStatus": func() error {
_, err := svc.DescribeVolumeStatus(ctx, &ec2.DescribeVolumeStatusInput{}, "")
return err
},
"DescribeVolumesModifications": func() error {
_, err := svc.DescribeVolumesModifications(ctx, &ec2.DescribeVolumesModificationsInput{}, "")
return err
},
} {
require.Error(t, describe(), name)
}
}
// The whole-cluster verb is what the leak reaper and the admin scans want, and
// it must reach every account's documents rather than the caller's alone.
func TestListAllVolumes_ReachesEveryAccount(t *testing.T) {
svc, objects := volumePartitionService(t)
all, err := svc.metadata.ListAllVolumes(context.Background())
require.NoError(t, err)
ids := make([]string, 0, len(all))
for _, volume := range all {
ids = append(ids, volume.VolumeID)
}
assert.ElementsMatch(t, []string{"vol-mine", "vol-theirs"}, ids)
assert.Equal(t, []string{"spinifex/ebsmetadata/v2/volumes/"}, objects.ListPrefixes(),
"the whole-cluster verb lists across accounts, not under one")
}
@@ -33,7 +33,7 @@ func TestListVolumePrefixes_SkipsWhatIsNotAVolume(t *testing.T) {
putListObject(t, store, "ami-dd7f063caded1ff82/config.json")
putListObject(t, store, "snap-ami-dd7f063caded1ff82/config.json")
putListObject(t, store, "keys/cluster.key")
putListObject(t, store, "spinifex/ebsmetadata/v1/volumes/vol-0123456789abc.json")
putListObject(t, store, "spinifex/ebsmetadata/v2/volumes/000000000001/vol-0123456789abc.json")
ids, err := listVolumePrefixes(t.Context(), store, listTestBucket)
require.NoError(t, err)
@@ -0,0 +1,86 @@
// Package recordingstore provides an objectstore.ObjectStore fake that records
// every key it is asked to read and every prefix it is asked to list. It exists
// so tests can assert an access pattern rather than only an answer: that an
// account-scoped listing never touched another account's prefix, which an
// in-memory filter over a whole-cluster read would satisfy just as well.
package recordingstore
import (
"context"
"strings"
"sync"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/mulgadc/spinifex/spinifex/objectstore"
)
// Store wraps a MemoryObjectStore, appending to Gets and ListPrefixes before
// delegating. Both slices are safe to read once the call under test returns.
type Store struct {
*objectstore.MemoryObjectStore
mu sync.Mutex
gets []string
listPrefixes []string
}
// New returns a recording store backed by an empty in-memory store.
func New() *Store {
return &Store{MemoryObjectStore: objectstore.NewMemoryObjectStore()}
}
var _ objectstore.ObjectStore = (*Store)(nil)
func (s *Store) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) {
s.mu.Lock()
s.gets = append(s.gets, aws.StringValue(input.Key))
s.mu.Unlock()
return s.MemoryObjectStore.GetObject(ctx, input)
}
func (s *Store) ListObjectsV2(ctx context.Context, input *s3.ListObjectsV2Input) (*s3.ListObjectsV2Output, error) {
s.mu.Lock()
s.listPrefixes = append(s.listPrefixes, aws.StringValue(input.Prefix))
s.mu.Unlock()
return s.MemoryObjectStore.ListObjectsV2(ctx, input)
}
// Gets returns every key read so far.
func (s *Store) Gets() []string {
s.mu.Lock()
defer s.mu.Unlock()
return append([]string(nil), s.gets...)
}
// ListPrefixes returns every prefix listed so far.
func (s *Store) ListPrefixes() []string {
s.mu.Lock()
defer s.mu.Unlock()
return append([]string(nil), s.listPrefixes...)
}
// Reset drops what has been recorded, so a test can seed fixtures and then
// assert only on the call it is actually exercising.
func (s *Store) Reset() {
s.mu.Lock()
defer s.mu.Unlock()
s.gets, s.listPrefixes = nil, nil
}
// TouchedPrefix reports whether any recorded get or listing named a key under
// prefix. This is the blast-radius assertion: it must be false for every
// account other than the caller's.
func (s *Store) TouchedPrefix(prefix string) bool {
for _, key := range s.Gets() {
if strings.HasPrefix(key, prefix) {
return true
}
}
for _, listed := range s.ListPrefixes() {
if strings.HasPrefix(listed, prefix) || strings.HasPrefix(prefix, listed) {
return true
}
}
return false
}
+1 -1
View File
@@ -35,5 +35,5 @@ type ResourceController interface {
// VolumeStateUpdater is the narrow slice of the volume service the manager
// touches: marking boot volumes "in-use" once a VM is confirmed running.
type VolumeStateUpdater interface {
UpdateVolumeState(volumeID, state, instanceID, attachmentDevice string) error
UpdateVolumeState(accountID, volumeID, state, instanceID, attachmentDevice string) error
}
+1 -1
View File
@@ -331,7 +331,7 @@ func (m *Manager) markAttachedVolumesInUse(instance *VM) error {
if m.deps.VolumeStateUpdater == nil {
return errors.New("volume state updater not wired")
}
if err := m.deps.VolumeStateUpdater.UpdateVolumeState(ebsReq.Name, "in-use", instance.ID, ebsReq.DeviceName); err != nil {
if err := m.deps.VolumeStateUpdater.UpdateVolumeState(instance.AccountID, ebsReq.Name, "in-use", instance.ID, ebsReq.DeviceName); err != nil {
return fmt.Errorf("persist in-use state for volume %s: %w", ebsReq.Name, err)
}
}
+6 -1
View File
@@ -214,15 +214,20 @@ type fakeVolumeStateUpdater struct {
onUpdate func(volumeStateUpdate)
}
// AccountID is recorded because it is the account segment of the document's
// key: a call that passes anything but the instance's own account writes to a
// key that does not exist, and nothing else in the suite would catch it.
type volumeStateUpdate struct {
AccountID string
VolumeID string
State string
InstanceID string
AttachmentDevice string
}
func (f *fakeVolumeStateUpdater) UpdateVolumeState(volumeID, state, instanceID, attachmentDevice string) error {
func (f *fakeVolumeStateUpdater) UpdateVolumeState(accountID, volumeID, state, instanceID, attachmentDevice string) error {
update := volumeStateUpdate{
AccountID: accountID,
VolumeID: volumeID,
State: state,
InstanceID: instanceID,
+2 -2
View File
@@ -118,8 +118,8 @@ type VM struct {
// Health tracks crash detection and auto-restart state
Health InstanceHealthState `json:"health"`
// AccountID is the AWS account that owns this instance. Empty for legacy
// resources visible only to the root account.
// AccountID is the AWS account that owns this instance, and is the account
// its volume documents are keyed under.
AccountID string `json:"account_id,omitempty"`
// VPC networking: ENI attached to this instance (set by RunInstances when VPC mode is active)
+148
View File
@@ -0,0 +1,148 @@
// These tests reuse fakeVolumeStateUpdater, newMockQMPClient and qmpRecorder,
// all package-internal.
//
//test:in-package
package vm
import (
"errors"
"testing"
"time"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/mulgadc/spinifex/spinifex/qmp"
"github.com/mulgadc/spinifex/spinifex/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// stateAccountID is the instance-owning account these tests seed. It is a real
// account ID rather than a placeholder because the production key builder
// rejects anything utils.IsAccountID refuses.
const stateAccountID = "000000000042"
// Volume documents are keyed under the owning account, so UpdateVolumeState's
// leading argument is a key segment rather than a label. The five production
// call sites all pass instance.AccountID; passing anything else — the instance
// ID is the very next argument and is also an untyped string — builds a key
// that does not exist and leaves the attachment record stranded. These tests
// are the only thing asserting that argument.
// TestAttachVolume_KeysStateOnTheInstanceAccount covers vm/volumes.go:190.
func TestAttachVolume_KeysStateOnTheInstanceAccount(t *testing.T) {
qmpClient, cancel := newMockQMPClient(t, func(cmd qmp.QMPCommand) map[string]any { return nil })
defer cancel()
stateUpdater := &fakeVolumeStateUpdater{}
mounter := &fakeVolumeMounter{mountOneURI: "nbd:unix:/tmp/test.sock"}
m := NewManagerWithDeps(Deps{VolumeMounter: mounter, VolumeStateUpdater: stateUpdater})
m.Insert(&VM{
ID: "i-1", Status: StateRunning, Instance: &ec2.Instance{},
QMPClient: qmpClient, AccountID: stateAccountID,
})
_, err := m.AttachVolume(t.Context(), "i-1", "vol-1", "/dev/sdf")
require.NoError(t, err)
calls := stateUpdater.snapshot()
require.Len(t, calls, 1)
assert.Equal(t, stateAccountID, calls[0].AccountID,
"the in-use write must be keyed on the instance's own account")
assert.Equal(t, "in-use", calls[0].State)
}
// TestAttachVolume_RollbackKeysStateOnTheInstanceAccount covers the rollback at
// vm/volumes.go:213, which runs after device_add fails and is the path that
// would silently leave a volume stuck in-use if it wrote to the wrong key.
func TestAttachVolume_RollbackKeysStateOnTheInstanceAccount(t *testing.T) {
qmpClient, cancel := newMockQMPClient(t, func(cmd qmp.QMPCommand) map[string]any {
switch cmd.Execute {
case "query-block":
return map[string]any{"return": []qmp.BlockDevice{}}
case "device_add":
return map[string]any{"error": map[string]any{"class": "GenericError", "desc": "no PCI slot"}}
}
return nil
})
defer cancel()
stateUpdater := &fakeVolumeStateUpdater{}
mounter := &fakeVolumeMounter{mountOneURI: "nbd:unix:/tmp/test.sock"}
m := NewManagerWithDeps(Deps{VolumeMounter: mounter, VolumeStateUpdater: stateUpdater})
m.Insert(&VM{
ID: "i-1", Status: StateRunning, Instance: &ec2.Instance{},
QMPClient: qmpClient, AccountID: stateAccountID,
})
_, err := m.AttachVolume(t.Context(), "i-1", "vol-1", "/dev/sdf")
require.Error(t, err)
calls := stateUpdater.snapshot()
require.Len(t, calls, 2)
for i, call := range calls {
assert.Equalf(t, stateAccountID, call.AccountID,
"call %d (%s) must be keyed on the instance's own account", i, call.State)
}
assert.Equal(t, "available", calls[1].State)
}
// TestDetachVolume_KeysStateOnTheInstanceAccount covers vm/volumes.go:444.
// Detach only logs an UpdateVolumeState failure, so a wrong account here leaves
// the document reading in-use with no disk in the guest and no error anywhere.
func TestDetachVolume_KeysStateOnTheInstanceAccount(t *testing.T) {
qmpClient, cancel := newMockQMPClientWithEvents(t, func(cmd qmp.QMPCommand, srv *mockQMPServer) map[string]any {
return nil
})
defer cancel()
stateUpdater := &fakeVolumeStateUpdater{}
mounter := &fakeVolumeMounter{}
m := NewManagerWithDeps(Deps{
VolumeMounter: mounter, VolumeStateUpdater: stateUpdater,
DeviceDeletedTimeout: 30 * time.Millisecond,
})
m.Insert(&VM{
ID: "i-1", Status: StateRunning, Instance: &ec2.Instance{},
QMPClient: qmpClient, AccountID: stateAccountID,
EBSRequests: types.EBSRequests{
Requests: []types.EBSRequest{{Name: "vol-1", DeviceName: "/dev/sdf"}},
},
})
_, err := m.DetachVolume(t.Context(), "i-1", "vol-1", "", false)
require.NoError(t, err)
calls := stateUpdater.snapshot()
require.Len(t, calls, 1)
assert.Equal(t, stateAccountID, calls[0].AccountID,
"the available write must be keyed on the instance's own account")
assert.Equal(t, "available", calls[0].State)
}
// TestAttachVolume_StateUpdateErrorSurfacesTheAccount pins that a key-builder
// rejection is not swallowed: an instance whose AccountID cannot key a document
// must fail the attach rather than proceed to device_add.
func TestAttachVolume_StateUpdateErrorSurfacesTheAccount(t *testing.T) {
recorder := &qmpRecorder{}
qmpClient, cancel := newMockQMPClient(t, func(cmd qmp.QMPCommand) map[string]any {
recorder.record(cmd)
return nil
})
defer cancel()
keyErr := errors.New(`invalid EBS metadata account ID ""`)
stateUpdater := &fakeVolumeStateUpdater{err: keyErr}
mounter := &fakeVolumeMounter{mountOneURI: "nbd:unix:/tmp/test.sock"}
m := NewManagerWithDeps(Deps{VolumeMounter: mounter, VolumeStateUpdater: stateUpdater})
m.Insert(&VM{ID: "i-1", Status: StateRunning, Instance: &ec2.Instance{}, QMPClient: qmpClient})
_, err := m.AttachVolume(t.Context(), "i-1", "vol-1", "/dev/sdf")
require.Error(t, err)
assert.ErrorIs(t, err, keyErr)
assert.NotContains(t, recorder.executes(), "device_add",
"an unkeyable account must not reach the guest")
calls := stateUpdater.snapshot()
require.Len(t, calls, 1)
assert.Empty(t, calls[0].AccountID, "the untenanted instance's account is what reached the store")
}
+3 -3
View File
@@ -187,7 +187,7 @@ func (m *Manager) AttachVolume(ctx context.Context, id, volumeID, device string)
m.rollbackHotAttach(ctx, instance, ebsRequest, nodeName, iothreadID)
return "", errors.New("volume state updater not wired")
}
if err := m.deps.VolumeStateUpdater.UpdateVolumeState(volumeID, "in-use", instance.ID, device); err != nil {
if err := m.deps.VolumeStateUpdater.UpdateVolumeState(instance.AccountID, volumeID, "in-use", instance.ID, device); err != nil {
m.rollbackHotAttach(ctx, instance, ebsRequest, nodeName, iothreadID)
return "", fmt.Errorf("persist in-use state for volume %s: %w", volumeID, err)
}
@@ -210,7 +210,7 @@ func (m *Manager) AttachVolume(ctx context.Context, id, volumeID, device string)
slog.ErrorContext(ctx, "AttachVolume: QMP device_add failed, rolling back blockdev",
"volumeId", volumeID, "err", err)
if m.rollbackHotAttach(ctx, instance, ebsRequest, nodeName, iothreadID) {
if stateErr := m.deps.VolumeStateUpdater.UpdateVolumeState(volumeID, "available", "", ""); stateErr != nil {
if stateErr := m.deps.VolumeStateUpdater.UpdateVolumeState(instance.AccountID, volumeID, "available", "", ""); stateErr != nil {
slog.ErrorContext(ctx, "AttachVolume: failed to restore available state after rollback",
"volumeId", volumeID, "err", stateErr)
}
@@ -441,7 +441,7 @@ func (m *Manager) DetachVolume(ctx context.Context, id, volumeID, device string,
})
if m.deps.VolumeStateUpdater != nil {
if err := m.deps.VolumeStateUpdater.UpdateVolumeState(volumeID, "available", "", ""); err != nil {
if err := m.deps.VolumeStateUpdater.UpdateVolumeState(instance.AccountID, volumeID, "available", "", ""); err != nil {
slog.ErrorContext(ctx, "DetachVolume: failed to update volume metadata",
"volumeId", volumeID, "err", err)
}
+2 -2
View File
@@ -7,7 +7,7 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
"github.com/stretchr/testify/require"
)
@@ -26,7 +26,7 @@ func TestImage(t *testing.T) {
ec2Cli := gw.EC2Client(t)
const snapshotID = "snap-integration-test-image"
require.NoError(t, handlers_ec2_snapshot.WriteSnapshotConfig(store, testImageBucket, snapshotID, &handlers_ec2_snapshot.SnapshotConfig{
require.NoError(t, ebsmetadata.NewStore(store, testImageBucket).PutSnapshot(t.Context(), ebsmetadata.Snapshot{
SnapshotID: snapshotID,
VolumeID: "vol-integration-test-image",
VolumeSize: 8,