From 0a7832440504442f685e85048d545848a8f2b8f6 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:10:09 +1000 Subject: [PATCH] feat: missing image commands (#156) --- docs/COMMANDS.md | 12 +- spinifex/daemon/daemon.go | 6 + spinifex/daemon/daemon_handlers_image.go | 24 + spinifex/gateway/ec2.go | 18 + spinifex/gateway/ec2/image/CopyImage.go | 69 + spinifex/gateway/ec2/image/CopyImage_test.go | 200 +++ spinifex/gateway/ec2/image/DeregisterImage.go | 36 + .../gateway/ec2/image/DeregisterImage_test.go | 67 + .../ec2/image/DescribeImageAttribute.go | 53 + .../ec2/image/DescribeImageAttribute_test.go | 111 ++ .../gateway/ec2/image/ModifyImageAttribute.go | 87 + .../ec2/image/ModifyImageAttribute_test.go | 177 ++ spinifex/gateway/ec2/image/RegisterImage.go | 129 ++ .../gateway/ec2/image/RegisterImage_test.go | 253 +++ .../gateway/ec2/image/ResetImageAttribute.go | 47 + .../ec2/image/ResetImageAttribute_test.go | 93 ++ spinifex/gateway/gateway_test.go | 3 +- spinifex/handlers/ec2/image/service_impl.go | 600 ++++++- .../handlers/ec2/image/service_impl_test.go | 1462 ++++++++++++++++- spinifex/handlers/ec2/image/service_nats.go | 13 +- .../handlers/ec2/snapshot/service_impl.go | 59 +- 21 files changed, 3367 insertions(+), 152 deletions(-) create mode 100644 spinifex/gateway/ec2/image/CopyImage.go create mode 100644 spinifex/gateway/ec2/image/CopyImage_test.go create mode 100644 spinifex/gateway/ec2/image/DeregisterImage.go create mode 100644 spinifex/gateway/ec2/image/DeregisterImage_test.go create mode 100644 spinifex/gateway/ec2/image/DescribeImageAttribute.go create mode 100644 spinifex/gateway/ec2/image/DescribeImageAttribute_test.go create mode 100644 spinifex/gateway/ec2/image/ModifyImageAttribute.go create mode 100644 spinifex/gateway/ec2/image/ModifyImageAttribute_test.go create mode 100644 spinifex/gateway/ec2/image/RegisterImage.go create mode 100644 spinifex/gateway/ec2/image/RegisterImage_test.go create mode 100644 spinifex/gateway/ec2/image/ResetImageAttribute.go create mode 100644 spinifex/gateway/ec2/image/ResetImageAttribute_test.go diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index cca285a4b..494c65aae 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -158,13 +158,13 @@ file, deferred to a later phase. |---------|-------------------|---------------|---------------|-------------|------------|--------| | `describe-images` | `--image-ids` (format validation only), `--owners` (self, account ID, alias), `--filters` (name, state, architecture, image-id, is-public, owner-id, description, image-type, tag:\*) | `--executable-users`, `--include-deprecated`, `--include-disabled`, `--max-results`, `--next-token`, `--dry-run` | None | NATS `ec2.DescribeImages` → daemon reads AMI metadata from Predastore S3 buckets (ami-*) → filters by ImageIds, Owners, and Filters → returns image list with state, architecture, block device mappings | 1. List all images
2. Filter by image ID
3. Filter by owner (self/amazon)
4. Non-existent AMI returns empty
5. Verify metadata fields (architecture, state, rootDeviceName)
6. Filter by name wildcard
7. Unknown filter returns InvalidParameterValue | **DONE** | | `create-image` | `--instance-id`, `--name`, `--description`, `--tag-specifications` | `--no-reboot`, `--block-device-mappings`, `--dry-run` | Instance must exist (running or stopped) | Gateway validates input → NATS `ec2.{instanceId}.CreateImage` (per-instance topic) → daemon extracts root volume → snapshots via `ebs.snapshot` NATS (running) or offline S3 copy (stopped) → creates AMI metadata in S3 (`{amiId}/config.json`) → stores tags → returns ami-ID. Duplicate AMI name validation enforced. | 1. Create image from running instance
2. Create image from stopped instance
3. Invalid instance ID (error)
4. Duplicate AMI name (error)
5. Verify new AMI appears in describe-images
6. Launch new instance from created AMI | **DONE** | -| `register-image` | — | `--name`, `--description`, `--architecture`, `--root-device-name`, `--virtualization-type`, `--block-device-mappings`, `--boot-mode`, `--ena-support`, `--sriov-net-support`, `--tpm-support`, `--imds-support` | Image data must exist in S3 | Create AMI metadata entry in Predastore → assign ami-ID → set state to available → return ami-ID | 1. Register with valid metadata
2. Missing required name (error)
3. Verify registered image in describe-images | **NOT STARTED** | -| `deregister-image` | — | `--image-id`, `--dry-run` | AMI must exist, no instances using it | Mark AMI metadata as deregistered in S3 → AMI no longer available for new launches → existing instances unaffected | 1. Deregister existing AMI
2. Deregister non-existent AMI (error)
3. Verify deregistered AMI not in describe-images
4. Existing instances from AMI still run | **NOT STARTED** | -| `copy-image` | — | `--source-image-id`, `--source-region`, `--name`, `--description`, `--encrypted`, `--kms-key-id`, `--client-token`, `--copy-image-tags`, `--tag-specifications` | Source AMI must exist | Copy AMI data between regions/nodes in Predastore → create new AMI metadata with new ami-ID → return new ami-ID | 1. Copy image within same region
2. Copy non-existent image (error)
3. Verify copied image independent of source | **NOT STARTED** | +| `register-image` | `--name`, `--description`, `--architecture` (x86_64/arm64/i386), `--root-device-name`, `--virtualization-type` (hvm only), `--block-device-mappings` (root with `Ebs.SnapshotId`+optional `VolumeSize`), `--tag-specifications` | `--billing-products`, `--uefi-data` | Backing snapshot must exist in Predastore (`{snapshotId}/metadata.json`); caller must own snapshot or it must be system-owned | Gateway validates name length (3–128), `snap-` prefix, architecture/virtualization values → NATS `ec2.RegisterImage` → daemon checks AMI name uniqueness, reads snapshot metadata, verifies snapshot ownership, builds `viperblock.AMIMetadata` (defaults: `Architecture=x86_64`, `Virtualization=hvm`, `PlatformDetails=Linux/UNIX`, `RootDeviceType=ebs`), writes `{amiId}/config.json`. Pointer-only — never touches block data. `BootMode`, `KernelId`, `RamdiskId`, `TpmSupport`, `ImdsSupport`, `EnaSupport`, `SriovNetSupport`, `ImageLocation`, `paravirtual` virtualization rejected with `InvalidParameterValue`. `VolumeSize` smaller than snapshot rejected. | 1. Register with valid snapshot
2. Missing name/snapshot (error)
3. Duplicate AMI name (`InvalidAMIName.Duplicate`)
4. Snapshot not found (`InvalidSnapshot.NotFound`)
5. Cross-account snapshot (`UnauthorizedOperation`)
6. Tags from `TagSpecifications` persisted
7. Verify registered image in describe-images | **DONE** | +| `deregister-image` | `--image-id` | `--dry-run` | AMI must exist; caller must own it (system AMIs immutable via this API) | Gateway validates `ami-` prefix → NATS `ec2.DeregisterImage` → daemon hard-deletes `{amiId}/config.json` from Predastore. Backing snapshot is left intact (matches AWS — operators run `delete-snapshot` separately to reclaim block storage). Cross-account/system-AMI mutations rejected with `UnauthorizedOperation`. Re-deregister returns `InvalidAMIID.NotFound` (no tombstone). | 1. Deregister existing AMI
2. Deregister non-existent AMI (`InvalidAMIID.NotFound`)
3. Re-deregister already-deleted AMI (`InvalidAMIID.NotFound`)
4. Cross-account AMI (`UnauthorizedOperation`)
5. System AMI (`UnauthorizedOperation`)
6. Verify deregistered AMI not in describe-images
7. Backing snapshot untouched | **DONE** | +| `copy-image` | `--source-image-id`, `--source-region` (must equal gateway region), `--name`, `--description`, `--client-token` (accepted, not honoured), `--copy-image-tags`, `--tag-specifications` (image only) | `--encrypted`, `--kms-key-id`, `--destination-outpost-arn`, `--dry-run` | Source AMI must exist and be owned by caller OR be a system AMI; backing snapshot must still exist | Gateway validates `ami-` prefix, name length (3–128), same-region — cross-region / `Encrypted` / `KmsKeyId` / `DestinationOutpostArn` rejected with `InvalidParameterValue`. NATS `ec2.CopyImage` → daemon checks name uniqueness, reads source `ami-xxx/config.json`, visibility-filters cross-account sources as `InvalidAMIID.NotFound`, reads source `{snap-xxx}/metadata.json` (missing or empty SnapshotID → `InvalidAMIID.NotFound`), writes new `snap-yyy/metadata.json` that inherits source `VolumeID` (no block copy), writes new `ami-yyy/config.json` owned by caller. `Description` inherits from source when unset. Tag merge: `CopyImageTags=true` seeds tags from source, explicit image-resource `TagSpecifications` override colliding keys and append new ones; non-image tag specs ignored. | 1. Same-region copy produces new `ami-`/`snap-` pair with distinct IDs
2. New snap shares source `VolumeID` (metadata-only)
3. New AMI owned by caller, source untouched
4. System AMI copied into caller's account
5. Cross-account source (`InvalidAMIID.NotFound`)
6. Missing source AMI (`InvalidAMIID.NotFound`)
7. Orphaned source (missing/empty `SnapshotID`) → `InvalidAMIID.NotFound`
8. Duplicate name (`InvalidAMIName.Duplicate`)
9. Cross-region / `Encrypted` / `KmsKeyId` rejected (`InvalidParameterValue`)
10. `CopyImageTags` true/false tag inheritance semantics | **DONE** | | `import-image` | — | `--disk-containers` (Format, Url/S3Bucket+S3Key), `--description`, `--architecture`, `--platform` | S3 bucket with disk image | Download disk image from S3 → convert format (VMDK/VHD/RAW→QCOW2) → create viperblock volume → register as AMI | 1. Import QCOW2 image
2. Import RAW image
3. Invalid format (error)
4. Verify imported image launchable | **NOT STARTED** | -| `describe-image-attribute` | — | `--image-id`, `--attribute`, `--dry-run` | AMI must exist | Read specific attribute from AMI metadata in S3 → return attribute value | 1. Get description attribute
2. Get launch permission
3. Invalid attribute name (error) | **NOT STARTED** | -| `modify-image-attribute` | — | `--image-id`, `--launch-permission`, `--description`, `--operation-type`, `--user-ids`, `--user-groups`, `--organization-arns`, `--dry-run` | AMI must exist | Update specific attribute in AMI metadata → persist to S3 | 1. Add launch permission
2. Modify description
3. Invalid AMI (error) | **NOT STARTED** | -| `reset-image-attribute` | — | `--image-id`, `--attribute`, `--dry-run` | AMI must exist | Reset attribute to default value in AMI metadata | 1. Reset launch permission to owner-only
2. Invalid AMI (error) | **NOT STARTED** | +| `describe-image-attribute` | `--image-id`, `--attribute` (`description`, `blockDeviceMapping` only) | `--dry-run`, `--attribute` for `launchPermission`/`bootMode`/`kernel`/`ramdisk`/`sriovNetSupport`/`productCodes`/`tpmSupport`/`uefiData`/`imdsSupport`/`lastLaunchedTime`/`deregistrationProtection` | AMI must exist and be owned by caller OR be a system AMI | Gateway validates `ami-` prefix + allowlisted attribute → NATS `ec2.DescribeImageAttribute` → daemon reads `ami-xxx/config.json`, hides cross-account reads as `InvalidAMIID.NotFound` (matches `DescribeImages`), returns `description` from stored metadata or synthesises a single root `blockDeviceMapping` from `AMIMetadata` (same shape as `DescribeImages`). Unsupported attributes rejected with `InvalidParameterValue` — notably `launchPermission` is deferred (breaks Terraform's `aws_ami` refresh; multi-account AMI sharing needs a separate design). | 1. Read `description`
2. Read synthesised `blockDeviceMapping`
3. AMI not found (`InvalidAMIID.NotFound`)
4. Cross-account AMI hidden (`InvalidAMIID.NotFound`)
5. System AMI readable by any caller
6. Unsupported attributes (`launchPermission`, `bootMode`, …) rejected with `InvalidParameterValue` | **DONE** | +| `modify-image-attribute` | `--image-id`, `--description` (top-level `Value=…` form), `--attribute description --value …` (structured form) | `--launch-permission`, `--imds-support`, `--operation-type`, `--user-ids`, `--user-groups`, `--organization-arns`, `--product-codes`, `--dry-run`, `--attribute` for anything other than `description` | AMI must exist and be owned by caller (system AMIs immutable via this API) | Gateway validates `ami-` prefix, rejects `LaunchPermission`/`ImdsSupport`/`ProductCodes`/org+user-id flags with `InvalidParameterValue`, and normalises the overloaded input shape (top-level `Description` → `Attribute=description`+`Value=…`). Both forms set together → `InvalidParameterCombination`; neither → `MissingParameter`. NATS `ec2.ModifyImageAttribute` → daemon loads AMI, runs ownership check (cross-account → `UnauthorizedOperation`, system AMIs also rejected), writes `AMIMetadata.Description` back to S3. Empty `Value` clears the description. | 1. Modify description (top-level form)
2. Modify description (structured form)
3. Clear description with empty value
4. Both top-level + structured set (`InvalidParameterCombination`)
5. Cross-account / system AMI (`UnauthorizedOperation`)
6. AMI not found (`InvalidAMIID.NotFound`)
7. `LaunchPermission` / `ImdsSupport` / unsupported `Attribute` rejected (`InvalidParameterValue`)
8. Round-trips via `describe-image-attribute` | **DONE** | +| `reset-image-attribute` | `--image-id`, `--attribute description` | `--attribute launchPermission`, `--dry-run` | AMI must exist and be owned by caller | Gateway validates `ami-` prefix; only `description` is accepted (`launchPermission` — AWS's default reset target — is out of scope). NATS `ec2.ResetImageAttribute` → daemon loads AMI, ownership check, clears `AMIMetadata.Description` to empty string, persists. | 1. Reset description to empty
2. Cross-account / system AMI (`UnauthorizedOperation`)
3. AMI not found (`InvalidAMIID.NotFound`)
4. `launchPermission` / other attributes rejected (`InvalidParameterValue`)
5. Round-trips via `describe-image-attribute` | **DONE** | ### EC2 - Volume (EBS) Management diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 57edee88d..ea7a79759 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -401,6 +401,12 @@ func (d *Daemon) subscribeAll() error { {"ec2.ImportKeyPair", d.handleEC2ImportKeyPair, "spinifex-workers"}, {"ec2.DescribeImages", d.handleEC2DescribeImages, "spinifex-workers"}, {"ec2.CreateImage", d.handleEC2CreateImage, ""}, + {"ec2.DeregisterImage", d.handleEC2DeregisterImage, "spinifex-workers"}, + {"ec2.RegisterImage", d.handleEC2RegisterImage, "spinifex-workers"}, + {"ec2.CopyImage", d.handleEC2CopyImage, "spinifex-workers"}, + {"ec2.DescribeImageAttribute", d.handleEC2DescribeImageAttribute, "spinifex-workers"}, + {"ec2.ModifyImageAttribute", d.handleEC2ModifyImageAttribute, "spinifex-workers"}, + {"ec2.ResetImageAttribute", d.handleEC2ResetImageAttribute, "spinifex-workers"}, {"ec2.CreateVolume", d.handleEC2CreateVolume, "spinifex-workers"}, {"ec2.DescribeVolumes", d.handleEC2DescribeVolumes, "spinifex-workers"}, {"ec2.ModifyVolume", d.handleEC2ModifyVolume, "spinifex-workers"}, diff --git a/spinifex/daemon/daemon_handlers_image.go b/spinifex/daemon/daemon_handlers_image.go index e2797257c..f15704c58 100644 --- a/spinifex/daemon/daemon_handlers_image.go +++ b/spinifex/daemon/daemon_handlers_image.go @@ -15,6 +15,30 @@ func (d *Daemon) handleEC2DescribeImages(msg *nats.Msg) { handleNATSRequest(msg, d.imageService.DescribeImages) } +func (d *Daemon) handleEC2DeregisterImage(msg *nats.Msg) { + handleNATSRequest(msg, d.imageService.DeregisterImage) +} + +func (d *Daemon) handleEC2RegisterImage(msg *nats.Msg) { + handleNATSRequest(msg, d.imageService.RegisterImage) +} + +func (d *Daemon) handleEC2CopyImage(msg *nats.Msg) { + handleNATSRequest(msg, d.imageService.CopyImage) +} + +func (d *Daemon) handleEC2DescribeImageAttribute(msg *nats.Msg) { + handleNATSRequest(msg, d.imageService.DescribeImageAttribute) +} + +func (d *Daemon) handleEC2ModifyImageAttribute(msg *nats.Msg) { + handleNATSRequest(msg, d.imageService.ModifyImageAttribute) +} + +func (d *Daemon) handleEC2ResetImageAttribute(msg *nats.Msg) { + handleNATSRequest(msg, d.imageService.ResetImageAttribute) +} + // handleEC2CreateImage is a stateful handler that extracts instance context // (root volume ID, source AMI, running state) before delegating to the image service. func (d *Daemon) handleEC2CreateImage(msg *nats.Msg) { diff --git a/spinifex/gateway/ec2.go b/spinifex/gateway/ec2.go index 918a3c928..a616feb99 100644 --- a/spinifex/gateway/ec2.go +++ b/spinifex/gateway/ec2.go @@ -114,6 +114,24 @@ var ec2Actions = map[string]EC2Handler{ "CreateImage": ec2Handler(func(input *ec2.CreateImageInput, gw *GatewayConfig, accountID string) (any, error) { return gateway_ec2_image.CreateImage(input, gw.NATSConn, gw.DiscoverActiveNodes(), accountID) }), + "DeregisterImage": ec2Handler(func(input *ec2.DeregisterImageInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_image.DeregisterImage(input, gw.NATSConn, accountID) + }), + "RegisterImage": ec2Handler(func(input *ec2.RegisterImageInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_image.RegisterImage(input, gw.NATSConn, accountID) + }), + "CopyImage": ec2Handler(func(input *ec2.CopyImageInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_image.CopyImage(input, gw.NATSConn, gw.Region, accountID) + }), + "DescribeImageAttribute": ec2Handler(func(input *ec2.DescribeImageAttributeInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_image.DescribeImageAttribute(input, gw.NATSConn, accountID) + }), + "ModifyImageAttribute": ec2Handler(func(input *ec2.ModifyImageAttributeInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_image.ModifyImageAttribute(input, gw.NATSConn, accountID) + }), + "ResetImageAttribute": ec2Handler(func(input *ec2.ResetImageAttributeInput, gw *GatewayConfig, accountID string) (any, error) { + return gateway_ec2_image.ResetImageAttribute(input, gw.NATSConn, accountID) + }), "DescribeRegions": ec2Handler(func(input *ec2.DescribeRegionsInput, gw *GatewayConfig, accountID string) (any, error) { return gateway_ec2_zone.DescribeRegions(input, gw.Region) }), diff --git a/spinifex/gateway/ec2/image/CopyImage.go b/spinifex/gateway/ec2/image/CopyImage.go new file mode 100644 index 000000000..e2a602679 --- /dev/null +++ b/spinifex/gateway/ec2/image/CopyImage.go @@ -0,0 +1,69 @@ +package gateway_ec2_image + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/nats-io/nats.go" +) + +// ValidateCopyImageInput validates the input parameters for CopyImage. +// Copy is single-region and metadata-only; cross-region, encryption, and +// Outposts are rejected here. ClientToken is accepted but not honoured. +func ValidateCopyImageInput(input *ec2.CopyImageInput, gwRegion string) error { + if input == nil { + return errors.New(awserrors.ErrorMissingParameter) + } + + if input.Name == nil || *input.Name == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if n := len(*input.Name); n < 3 || n > 128 { + return errors.New(awserrors.ErrorInvalidAMINameMalformed) + } + + if input.SourceImageId == nil || *input.SourceImageId == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !strings.HasPrefix(*input.SourceImageId, "ami-") { + return errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + + if input.SourceRegion == nil || *input.SourceRegion == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if *input.SourceRegion != gwRegion { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + + if input.Encrypted != nil && *input.Encrypted { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.KmsKeyId != nil && *input.KmsKeyId != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.DestinationOutpostArn != nil && *input.DestinationOutpostArn != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + + return nil +} + +// CopyImage handles the EC2 CopyImage API call. +func CopyImage(input *ec2.CopyImageInput, natsConn *nats.Conn, gwRegion, accountID string) (ec2.CopyImageOutput, error) { + var output ec2.CopyImageOutput + + if err := ValidateCopyImageInput(input, gwRegion); err != nil { + return output, err + } + + svc := handlers_ec2_image.NewNATSImageService(natsConn, 0) + result, err := svc.CopyImage(input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/image/CopyImage_test.go b/spinifex/gateway/ec2/image/CopyImage_test.go new file mode 100644 index 000000000..647322541 --- /dev/null +++ b/spinifex/gateway/ec2/image/CopyImage_test.go @@ -0,0 +1,200 @@ +package gateway_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" +) + +const testCopyImageRegion = "ap-southeast-2" + +func validCopyImageInput() *ec2.CopyImageInput { + return &ec2.CopyImageInput{ + Name: aws.String("copied-ami"), + SourceImageId: aws.String("ami-1234567890abcdef0"), + SourceRegion: aws.String(testCopyImageRegion), + } +} + +func TestValidateCopyImageInput(t *testing.T) { + tests := []struct { + name string + mutate func(*ec2.CopyImageInput) + input *ec2.CopyImageInput + wantErr bool + errMsg string + }{ + { + name: "NilInput", + input: nil, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MissingName", + mutate: func(i *ec2.CopyImageInput) { i.Name = nil }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptyName", + mutate: func(i *ec2.CopyImageInput) { i.Name = aws.String("") }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "ShortName", + mutate: func(i *ec2.CopyImageInput) { i.Name = aws.String("ab") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMINameMalformed, + }, + { + name: "LongName", + mutate: func(i *ec2.CopyImageInput) { + name := make([]byte, 129) + for j := range name { + name[j] = 'a' + } + i.Name = aws.String(string(name)) + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMINameMalformed, + }, + { + name: "MissingSourceImageId", + mutate: func(i *ec2.CopyImageInput) { i.SourceImageId = nil }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptySourceImageId", + mutate: func(i *ec2.CopyImageInput) { i.SourceImageId = aws.String("") }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MalformedSourceImageId", + mutate: func(i *ec2.CopyImageInput) { i.SourceImageId = aws.String("not-an-ami") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMIIDMalformed, + }, + { + name: "MissingSourceRegion", + mutate: func(i *ec2.CopyImageInput) { i.SourceRegion = nil }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptySourceRegion", + mutate: func(i *ec2.CopyImageInput) { i.SourceRegion = aws.String("") }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "CrossRegionRejected", + mutate: func(i *ec2.CopyImageInput) { i.SourceRegion = aws.String("us-east-1") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "EncryptedRejected", + mutate: func(i *ec2.CopyImageInput) { i.Encrypted = aws.Bool(true) }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "KmsKeyIdRejected", + mutate: func(i *ec2.CopyImageInput) { i.KmsKeyId = aws.String("alias/some-key") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "DestinationOutpostArnRejected", + mutate: func(i *ec2.CopyImageInput) { + i.DestinationOutpostArn = aws.String("arn:aws:outposts:us-east-1:123:outpost/op-abc") + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ValidMinimal", + wantErr: false, + }, + { + name: "ValidWithDescriptionAndClientToken", + mutate: func(i *ec2.CopyImageInput) { + i.Description = aws.String("renamed clone") + i.ClientToken = aws.String("idempotency-token-ignored") + }, + wantErr: false, + }, + { + name: "ValidWithCopyImageTagsTrue", + mutate: func(i *ec2.CopyImageInput) { + i.CopyImageTags = aws.Bool(true) + }, + wantErr: false, + }, + { + name: "ValidWithCopyImageTagsFalse", + mutate: func(i *ec2.CopyImageInput) { + i.CopyImageTags = aws.Bool(false) + }, + wantErr: false, + }, + { + name: "ValidWithTagSpecifications", + mutate: func(i *ec2.CopyImageInput) { + i.TagSpecifications = []*ec2.TagSpecification{ + { + ResourceType: aws.String("image"), + Tags: []*ec2.Tag{ + {Key: aws.String("Env"), Value: aws.String("prod")}, + }, + }, + } + }, + wantErr: false, + }, + { + name: "EncryptedFalseAllowed", + mutate: func(i *ec2.CopyImageInput) { + i.Encrypted = aws.Bool(false) + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var input *ec2.CopyImageInput + if tt.input == nil && tt.name != "NilInput" { + input = validCopyImageInput() + } else { + input = tt.input + } + if tt.mutate != nil { + tt.mutate(input) + } + + err := ValidateCopyImageInput(input, testCopyImageRegion) + if tt.wantErr { + assert.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestCopyImage_GatewayValidationFailureReturnsEarly(t *testing.T) { + // nil natsConn is fine: validation rejects the input before any NATS + // round-trip is attempted, so we exercise the early-exit path. + _, err := CopyImage(&ec2.CopyImageInput{}, nil, testCopyImageRegion, "000000000001") + assert.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} diff --git a/spinifex/gateway/ec2/image/DeregisterImage.go b/spinifex/gateway/ec2/image/DeregisterImage.go new file mode 100644 index 000000000..f103d5f37 --- /dev/null +++ b/spinifex/gateway/ec2/image/DeregisterImage.go @@ -0,0 +1,36 @@ +package gateway_ec2_image + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/nats-io/nats.go" +) + +func ValidateDeregisterImageInput(input *ec2.DeregisterImageInput) error { + if input == nil || input.ImageId == nil || *input.ImageId == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !strings.HasPrefix(*input.ImageId, "ami-") { + return errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + return nil +} + +func DeregisterImage(input *ec2.DeregisterImageInput, natsConn *nats.Conn, accountID string) (ec2.DeregisterImageOutput, error) { + var output ec2.DeregisterImageOutput + + if err := ValidateDeregisterImageInput(input); err != nil { + return output, err + } + + svc := handlers_ec2_image.NewNATSImageService(natsConn, 0) + result, err := svc.DeregisterImage(input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/image/DeregisterImage_test.go b/spinifex/gateway/ec2/image/DeregisterImage_test.go new file mode 100644 index 000000000..106151a97 --- /dev/null +++ b/spinifex/gateway/ec2/image/DeregisterImage_test.go @@ -0,0 +1,67 @@ +package gateway_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" +) + +func TestValidateDeregisterImageInput(t *testing.T) { + tests := []struct { + name string + input *ec2.DeregisterImageInput + wantErr bool + errMsg string + }{ + { + name: "NilInput", + input: nil, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptyInput", + input: &ec2.DeregisterImageInput{}, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptyImageId", + input: &ec2.DeregisterImageInput{ + ImageId: aws.String(""), + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MalformedImageId", + input: &ec2.DeregisterImageInput{ + ImageId: aws.String("not-an-ami"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMIIDMalformed, + }, + { + name: "ValidImageId", + input: &ec2.DeregisterImageInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateDeregisterImageInput(tt.input) + if tt.wantErr { + assert.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/spinifex/gateway/ec2/image/DescribeImageAttribute.go b/spinifex/gateway/ec2/image/DescribeImageAttribute.go new file mode 100644 index 000000000..f4c323379 --- /dev/null +++ b/spinifex/gateway/ec2/image/DescribeImageAttribute.go @@ -0,0 +1,53 @@ +package gateway_ec2_image + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/nats-io/nats.go" +) + +// supportedImageAttributes lists the AMI attributes spinifex exposes. +// description is modifiable; blockDeviceMapping is synthesised read-only from +// AMIMetadata. Any other attribute is rejected rather than returning empty. +var supportedImageAttributes = map[string]bool{ + ec2.ImageAttributeNameDescription: true, + ec2.ImageAttributeNameBlockDeviceMapping: true, +} + +func ValidateDescribeImageAttributeInput(input *ec2.DescribeImageAttributeInput) error { + if input == nil { + return errors.New(awserrors.ErrorMissingParameter) + } + if input.ImageId == nil || *input.ImageId == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !strings.HasPrefix(*input.ImageId, "ami-") { + return errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + if input.Attribute == nil || *input.Attribute == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !supportedImageAttributes[*input.Attribute] { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + return nil +} + +func DescribeImageAttribute(input *ec2.DescribeImageAttributeInput, natsConn *nats.Conn, accountID string) (ec2.DescribeImageAttributeOutput, error) { + var output ec2.DescribeImageAttributeOutput + + if err := ValidateDescribeImageAttributeInput(input); err != nil { + return output, err + } + + svc := handlers_ec2_image.NewNATSImageService(natsConn, 0) + result, err := svc.DescribeImageAttribute(input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/image/DescribeImageAttribute_test.go b/spinifex/gateway/ec2/image/DescribeImageAttribute_test.go new file mode 100644 index 000000000..338991fa3 --- /dev/null +++ b/spinifex/gateway/ec2/image/DescribeImageAttribute_test.go @@ -0,0 +1,111 @@ +package gateway_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" +) + +func TestValidateDescribeImageAttributeInput(t *testing.T) { + tests := []struct { + name string + input *ec2.DescribeImageAttributeInput + wantErr bool + errMsg string + }{ + { + name: "NilInput", + input: nil, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptyInput", + input: &ec2.DescribeImageAttributeInput{}, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MissingAttribute", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MalformedImageId", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("not-an-ami"), + Attribute: aws.String("description"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMIIDMalformed, + }, + { + name: "UnsupportedAttribute_LaunchPermission", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("launchPermission"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "UnsupportedAttribute_BootMode", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("bootMode"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "UnsupportedAttribute_Kernel", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("kernel"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ValidDescription", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("description"), + }, + wantErr: false, + }, + { + name: "ValidBlockDeviceMapping", + input: &ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("blockDeviceMapping"), + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateDescribeImageAttributeInput(tt.input) + if tt.wantErr { + assert.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestDescribeImageAttribute_GatewayValidationFailureReturnsEarly(t *testing.T) { + // nil natsConn is fine: validation rejects before any NATS round-trip. + _, err := DescribeImageAttribute(&ec2.DescribeImageAttributeInput{}, nil, "000000000001") + assert.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} diff --git a/spinifex/gateway/ec2/image/ModifyImageAttribute.go b/spinifex/gateway/ec2/image/ModifyImageAttribute.go new file mode 100644 index 000000000..1f136e448 --- /dev/null +++ b/spinifex/gateway/ec2/image/ModifyImageAttribute.go @@ -0,0 +1,87 @@ +package gateway_ec2_image + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/nats-io/nats.go" +) + +// ValidateModifyImageAttributeInput validates and normalises the two AWS +// shapes for modifying description (top-level `--description Value=…` and +// structured `--attribute description --value …`) into Attribute+Value. +// Unsupported top-level shortcuts (launchPermission, imdsSupport, productCodes) +// are rejected rather than silently accepted. +func ValidateModifyImageAttributeInput(input *ec2.ModifyImageAttributeInput) error { + if input == nil { + return errors.New(awserrors.ErrorMissingParameter) + } + if input.ImageId == nil || *input.ImageId == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !strings.HasPrefix(*input.ImageId, "ami-") { + return errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + + if input.LaunchPermission != nil { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.ImdsSupport != nil { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if len(input.ProductCodes) > 0 { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if len(input.UserIds) > 0 || len(input.UserGroups) > 0 || + len(input.OrganizationArns) > 0 || len(input.OrganizationalUnitArns) > 0 || + (input.OperationType != nil && *input.OperationType != "") { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + + hasTopLevelDescription := input.Description != nil + // Treat any set Value (even without Attribute) as structured shape so it + // isn't silently discarded by the top-level branch. + hasStructured := (input.Attribute != nil && *input.Attribute != "") || input.Value != nil + + switch { + case hasTopLevelDescription && hasStructured: + return errors.New(awserrors.ErrorInvalidParameterCombination) + case hasTopLevelDescription: + value := "" + if input.Description.Value != nil { + value = *input.Description.Value + } + input.Attribute = aws.String(ec2.ImageAttributeNameDescription) + input.Value = aws.String(value) + case hasStructured: + if input.Attribute == nil || *input.Attribute == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if *input.Attribute != ec2.ImageAttributeNameDescription { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + default: + return errors.New(awserrors.ErrorMissingParameter) + } + + return nil +} + +func ModifyImageAttribute(input *ec2.ModifyImageAttributeInput, natsConn *nats.Conn, accountID string) (ec2.ModifyImageAttributeOutput, error) { + var output ec2.ModifyImageAttributeOutput + + if err := ValidateModifyImageAttributeInput(input); err != nil { + return output, err + } + + svc := handlers_ec2_image.NewNATSImageService(natsConn, 0) + result, err := svc.ModifyImageAttribute(input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/image/ModifyImageAttribute_test.go b/spinifex/gateway/ec2/image/ModifyImageAttribute_test.go new file mode 100644 index 000000000..86c70be0f --- /dev/null +++ b/spinifex/gateway/ec2/image/ModifyImageAttribute_test.go @@ -0,0 +1,177 @@ +package gateway_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateModifyImageAttributeInput(t *testing.T) { + tests := []struct { + name string + input *ec2.ModifyImageAttributeInput + wantErr bool + errMsg string + }{ + { + name: "NilInput", + input: nil, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MissingImageId", + input: &ec2.ModifyImageAttributeInput{ + Description: &ec2.AttributeValue{Value: aws.String("hi")}, + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MalformedImageId", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("not-an-ami"), + Description: &ec2.AttributeValue{Value: aws.String("hi")}, + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMIIDMalformed, + }, + { + name: "NoAttributeSet", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "BothDescriptionAndAttributeSet", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Description: &ec2.AttributeValue{Value: aws.String("hi")}, + Attribute: aws.String("description"), + Value: aws.String("hi"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterCombination, + }, + { + name: "LaunchPermissionRejected", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + LaunchPermission: &ec2.LaunchPermissionModifications{}, + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ImdsSupportRejected", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + ImdsSupport: &ec2.AttributeValue{Value: aws.String("v2.0")}, + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ProductCodesRejected", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + ProductCodes: []*string{aws.String("abc")}, + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "UserIdsRejected", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + UserIds: []*string{aws.String("123456789012")}, + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "UnsupportedAttribute_BootMode", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("bootMode"), + Value: aws.String("uefi"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "UnsupportedAttribute_LaunchPermission", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("launchPermission"), + Value: aws.String("foo"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ValidTopLevelDescription", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Description: &ec2.AttributeValue{Value: aws.String("updated")}, + }, + wantErr: false, + }, + { + name: "ValidStructuredDescription", + input: &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("description"), + Value: aws.String("updated"), + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateModifyImageAttributeInput(tt.input) + if tt.wantErr { + assert.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestValidateModifyImageAttributeInput_NormalizesTopLevelDescription(t *testing.T) { + input := &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Description: &ec2.AttributeValue{Value: aws.String("new-desc")}, + } + require.NoError(t, ValidateModifyImageAttributeInput(input)) + require.NotNil(t, input.Attribute) + assert.Equal(t, "description", *input.Attribute) + require.NotNil(t, input.Value) + assert.Equal(t, "new-desc", *input.Value) +} + +func TestValidateModifyImageAttributeInput_NormalizesEmptyDescriptionToEmptyString(t *testing.T) { + // --description Value="" should clear the field, not error. + input := &ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Description: &ec2.AttributeValue{Value: aws.String("")}, + } + require.NoError(t, ValidateModifyImageAttributeInput(input)) + require.NotNil(t, input.Value) + assert.Equal(t, "", *input.Value) +} + +func TestModifyImageAttribute_GatewayValidationFailureReturnsEarly(t *testing.T) { + _, err := ModifyImageAttribute(&ec2.ModifyImageAttributeInput{}, nil, "000000000001") + assert.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} diff --git a/spinifex/gateway/ec2/image/RegisterImage.go b/spinifex/gateway/ec2/image/RegisterImage.go new file mode 100644 index 000000000..5f221c15f --- /dev/null +++ b/spinifex/gateway/ec2/image/RegisterImage.go @@ -0,0 +1,129 @@ +package gateway_ec2_image + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/nats-io/nats.go" +) + +// i386 is accepted (not scheduled) so pass-through callers don't fail validation. +var validRegisterImageArchitectures = map[string]bool{ + "x86_64": true, + "arm64": true, + "i386": true, +} + +// ValidateRegisterImageInput rejects inputs that ask for behaviour spinifex +// doesn't have (PV, S3 bundles, kernel/ramdisk, IMDS/TPM/ENA hints) rather +// than silently discarding them. Registration is pointer-only: caller supplies +// a snapshot and we write a config.json that references it. +func ValidateRegisterImageInput(input *ec2.RegisterImageInput) error { + if input == nil { + return errors.New(awserrors.ErrorMissingParameter) + } + + if input.Name == nil || *input.Name == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if n := len(*input.Name); n < 3 || n > 128 { + return errors.New(awserrors.ErrorInvalidAMINameMalformed) + } + + if input.ImageLocation != nil && *input.ImageLocation != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + + if input.Architecture != nil && *input.Architecture != "" { + if !validRegisterImageArchitectures[*input.Architecture] { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + } + + if input.VirtualizationType != nil && *input.VirtualizationType != "" && *input.VirtualizationType != "hvm" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + + if input.BootMode != nil && *input.BootMode != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.KernelId != nil && *input.KernelId != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.RamdiskId != nil && *input.RamdiskId != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.TpmSupport != nil && *input.TpmSupport != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.ImdsSupport != nil && *input.ImdsSupport != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.EnaSupport != nil { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + if input.SriovNetSupport != nil && *input.SriovNetSupport != "" { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + + if len(input.BlockDeviceMappings) == 0 { + return errors.New(awserrors.ErrorMissingParameter) + } + + root := selectRootBlockDeviceMapping(input.BlockDeviceMappings, input.RootDeviceName) + if root == nil || root.Ebs == nil || root.Ebs.SnapshotId == nil || *root.Ebs.SnapshotId == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !strings.HasPrefix(*root.Ebs.SnapshotId, "snap-") { + return errors.New(awserrors.ErrorInvalidSnapshotIDMalformed) + } + + return nil +} + +// selectRootBlockDeviceMapping returns the BDM matching RootDeviceName if set, +// else the first BDM carrying an EBS snapshot reference. +func selectRootBlockDeviceMapping(mappings []*ec2.BlockDeviceMapping, rootDeviceName *string) *ec2.BlockDeviceMapping { + wantName := "" + if rootDeviceName != nil { + wantName = *rootDeviceName + } + + if wantName != "" { + for _, bdm := range mappings { + if bdm == nil || bdm.DeviceName == nil { + continue + } + if *bdm.DeviceName == wantName { + return bdm + } + } + return nil + } + + for _, bdm := range mappings { + if bdm == nil || bdm.Ebs == nil || bdm.Ebs.SnapshotId == nil { + continue + } + return bdm + } + return nil +} + +func RegisterImage(input *ec2.RegisterImageInput, natsConn *nats.Conn, accountID string) (ec2.RegisterImageOutput, error) { + var output ec2.RegisterImageOutput + + if err := ValidateRegisterImageInput(input); err != nil { + return output, err + } + + svc := handlers_ec2_image.NewNATSImageService(natsConn, 0) + result, err := svc.RegisterImage(input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/image/RegisterImage_test.go b/spinifex/gateway/ec2/image/RegisterImage_test.go new file mode 100644 index 000000000..a8b026979 --- /dev/null +++ b/spinifex/gateway/ec2/image/RegisterImage_test.go @@ -0,0 +1,253 @@ +package gateway_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" +) + +func validRegisterImageInput() *ec2.RegisterImageInput { + return &ec2.RegisterImageInput{ + Name: aws.String("my-image"), + RootDeviceName: aws.String("/dev/sda1"), + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String("/dev/sda1"), + Ebs: &ec2.EbsBlockDevice{ + SnapshotId: aws.String("snap-1234567890abcdef0"), + VolumeSize: aws.Int64(20), + }, + }, + }, + } +} + +func TestValidateRegisterImageInput(t *testing.T) { + tests := []struct { + name string + mutate func(*ec2.RegisterImageInput) + input *ec2.RegisterImageInput + wantErr bool + errMsg string + }{ + { + name: "NilInput", + input: nil, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MissingName", + mutate: func(i *ec2.RegisterImageInput) { i.Name = nil }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptyName", + mutate: func(i *ec2.RegisterImageInput) { i.Name = aws.String("") }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "ShortName", + mutate: func(i *ec2.RegisterImageInput) { i.Name = aws.String("ab") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMINameMalformed, + }, + { + name: "LongName", + mutate: func(i *ec2.RegisterImageInput) { + name := make([]byte, 129) + for j := range name { + name[j] = 'a' + } + i.Name = aws.String(string(name)) + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMINameMalformed, + }, + { + name: "MissingBlockDeviceMappings", + mutate: func(i *ec2.RegisterImageInput) { i.BlockDeviceMappings = nil }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MissingSnapshotIdInRootBDM", + mutate: func(i *ec2.RegisterImageInput) { + i.BlockDeviceMappings[0].Ebs.SnapshotId = nil + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MalformedSnapshotId", + mutate: func(i *ec2.RegisterImageInput) { + i.BlockDeviceMappings[0].Ebs.SnapshotId = aws.String("not-a-snapshot") + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidSnapshotIDMalformed, + }, + { + name: "RootDeviceNameWithNoMatchingBDM", + mutate: func(i *ec2.RegisterImageInput) { + i.RootDeviceName = aws.String("/dev/xvda") + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "InvalidArchitecture", + mutate: func(i *ec2.RegisterImageInput) { + i.Architecture = aws.String("riscv") + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ParavirtualVirtualizationRejected", + mutate: func(i *ec2.RegisterImageInput) { + i.VirtualizationType = aws.String("paravirtual") + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ImageLocationRejected", + mutate: func(i *ec2.RegisterImageInput) { + i.ImageLocation = aws.String("s3://bundles/my-ami.manifest.xml") + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "BootModeRejected", + mutate: func(i *ec2.RegisterImageInput) { i.BootMode = aws.String("uefi") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "KernelIdRejected", + mutate: func(i *ec2.RegisterImageInput) { i.KernelId = aws.String("aki-123") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "RamdiskIdRejected", + mutate: func(i *ec2.RegisterImageInput) { i.RamdiskId = aws.String("ari-123") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "TpmSupportRejected", + mutate: func(i *ec2.RegisterImageInput) { i.TpmSupport = aws.String("v2.0") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ImdsSupportRejected", + mutate: func(i *ec2.RegisterImageInput) { i.ImdsSupport = aws.String("v2.0") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "EnaSupportRejected", + mutate: func(i *ec2.RegisterImageInput) { i.EnaSupport = aws.Bool(true) }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "SriovNetSupportRejected", + mutate: func(i *ec2.RegisterImageInput) { i.SriovNetSupport = aws.String("simple") }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ValidWithExplicitArch", + mutate: func(i *ec2.RegisterImageInput) { + i.Architecture = aws.String("arm64") + i.VirtualizationType = aws.String("hvm") + }, + wantErr: false, + }, + { + name: "ValidNoRootDeviceName", + mutate: func(i *ec2.RegisterImageInput) { + i.RootDeviceName = nil + }, + wantErr: false, + }, + { + name: "ValidMinimal", + wantErr: false, + }, + { + name: "ValidWithTagSpecifications", + mutate: func(i *ec2.RegisterImageInput) { + i.TagSpecifications = []*ec2.TagSpecification{ + { + ResourceType: aws.String("image"), + Tags: []*ec2.Tag{ + {Key: aws.String("Env"), Value: aws.String("test")}, + }, + }, + } + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var input *ec2.RegisterImageInput + if tt.input == nil && tt.name != "NilInput" { + input = validRegisterImageInput() + } else { + input = tt.input + } + if tt.mutate != nil { + tt.mutate(input) + } + + err := ValidateRegisterImageInput(input) + if tt.wantErr { + assert.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestRegisterImage_GatewayValidationFailureReturnsEarly(t *testing.T) { + // nil natsConn is fine here: validation rejects the input before any + // NATS round-trip is attempted, so we exercise the early-exit path. + _, err := RegisterImage(&ec2.RegisterImageInput{}, nil, "000000000001") + assert.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestSelectRootBlockDeviceMapping_NoRootDeviceName(t *testing.T) { + mappings := []*ec2.BlockDeviceMapping{ + nil, + {Ebs: nil}, + {Ebs: &ec2.EbsBlockDevice{SnapshotId: aws.String("snap-first")}}, + {Ebs: &ec2.EbsBlockDevice{SnapshotId: aws.String("snap-second")}}, + } + bdm := selectRootBlockDeviceMapping(mappings, nil) + assert.NotNil(t, bdm) + assert.Equal(t, "snap-first", *bdm.Ebs.SnapshotId) + + // Empty (non-nil) RootDeviceName behaves the same as nil. + bdm = selectRootBlockDeviceMapping(mappings, aws.String("")) + assert.NotNil(t, bdm) + assert.Equal(t, "snap-first", *bdm.Ebs.SnapshotId) + + // Nothing usable. + bdm = selectRootBlockDeviceMapping([]*ec2.BlockDeviceMapping{nil, {}}, nil) + assert.Nil(t, bdm) +} diff --git a/spinifex/gateway/ec2/image/ResetImageAttribute.go b/spinifex/gateway/ec2/image/ResetImageAttribute.go new file mode 100644 index 000000000..ff1754012 --- /dev/null +++ b/spinifex/gateway/ec2/image/ResetImageAttribute.go @@ -0,0 +1,47 @@ +package gateway_ec2_image + +import ( + "errors" + "strings" + + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + "github.com/nats-io/nats.go" +) + +// ValidateResetImageAttributeInput accepts only description; launchPermission +// is out of scope. +func ValidateResetImageAttributeInput(input *ec2.ResetImageAttributeInput) error { + if input == nil { + return errors.New(awserrors.ErrorMissingParameter) + } + if input.ImageId == nil || *input.ImageId == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if !strings.HasPrefix(*input.ImageId, "ami-") { + return errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + if input.Attribute == nil || *input.Attribute == "" { + return errors.New(awserrors.ErrorMissingParameter) + } + if *input.Attribute != ec2.ImageAttributeNameDescription { + return errors.New(awserrors.ErrorInvalidParameterValue) + } + return nil +} + +func ResetImageAttribute(input *ec2.ResetImageAttributeInput, natsConn *nats.Conn, accountID string) (ec2.ResetImageAttributeOutput, error) { + var output ec2.ResetImageAttributeOutput + + if err := ValidateResetImageAttributeInput(input); err != nil { + return output, err + } + + svc := handlers_ec2_image.NewNATSImageService(natsConn, 0) + result, err := svc.ResetImageAttribute(input, accountID) + if err != nil { + return output, err + } + return *result, nil +} diff --git a/spinifex/gateway/ec2/image/ResetImageAttribute_test.go b/spinifex/gateway/ec2/image/ResetImageAttribute_test.go new file mode 100644 index 000000000..75cba4dc2 --- /dev/null +++ b/spinifex/gateway/ec2/image/ResetImageAttribute_test.go @@ -0,0 +1,93 @@ +package gateway_ec2_image + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" +) + +func TestValidateResetImageAttributeInput(t *testing.T) { + tests := []struct { + name string + input *ec2.ResetImageAttributeInput + wantErr bool + errMsg string + }{ + { + name: "NilInput", + input: nil, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "EmptyInput", + input: &ec2.ResetImageAttributeInput{}, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MissingAttribute", + input: &ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + }, + wantErr: true, + errMsg: awserrors.ErrorMissingParameter, + }, + { + name: "MalformedImageId", + input: &ec2.ResetImageAttributeInput{ + ImageId: aws.String("not-an-ami"), + Attribute: aws.String("description"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidAMIIDMalformed, + }, + { + name: "UnsupportedAttribute_LaunchPermission", + input: &ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("launchPermission"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "UnsupportedAttribute_BlockDeviceMapping", + input: &ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("blockDeviceMapping"), + }, + wantErr: true, + errMsg: awserrors.ErrorInvalidParameterValue, + }, + { + name: "ValidDescription", + input: &ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-1234567890abcdef0"), + Attribute: aws.String("description"), + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateResetImageAttributeInput(tt.input) + if tt.wantErr { + assert.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestResetImageAttribute_GatewayValidationFailureReturnsEarly(t *testing.T) { + _, err := ResetImageAttribute(&ec2.ResetImageAttributeInput{}, nil, "000000000001") + assert.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index ecc09ecbb..98a682fa9 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -918,7 +918,8 @@ func TestEC2ActionMapCompleteness(t *testing.T) { "TerminateInstances", "RebootInstances", "DescribeInstanceTypes", "GetConsoleOutput", "ModifyInstanceAttribute", "DescribeInstanceAttribute", "CreateKeyPair", "DeleteKeyPair", "DescribeKeyPairs", "ImportKeyPair", - "DescribeImages", "CreateImage", + "DescribeImages", "CreateImage", "DeregisterImage", "RegisterImage", "CopyImage", + "DescribeImageAttribute", "ModifyImageAttribute", "ResetImageAttribute", "DescribeRegions", "DescribeAvailabilityZones", "DescribeVolumes", "ModifyVolume", "CreateVolume", "DeleteVolume", "AttachVolume", "DescribeVolumeStatus", "DetachVolume", diff --git a/spinifex/handlers/ec2/image/service_impl.go b/spinifex/handlers/ec2/image/service_impl.go index a082f1d0b..19e7b949b 100644 --- a/spinifex/handlers/ec2/image/service_impl.go +++ b/spinifex/handlers/ec2/image/service_impl.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "log/slog" + "maps" "strconv" "strings" "time" @@ -196,15 +197,12 @@ func (s *ImageServiceImpl) DescribeImages(input *ec2.DescribeImagesInput, accoun } } - // Determine AMI ownership. Phase4+ AMIs store the creator's account ID - // in ImageOwnerAlias. Pre-phase4 AMIs have non-account values like "self" - // or "spinifex" and are treated as system/public images visible to all. + // AMIs with a non-account-ID owner (e.g. "spinifex") are system AMIs + // visible to all; account-ID owners are private. amiOwner := amiMeta.ImageOwnerAlias - isSystemAMI := !utils.IsAccountID(amiOwner) + isSystemAMI := amiOwner != "" && !utils.IsAccountID(amiOwner) - // Visibility check: callers can only see their own AMIs and system AMIs. - // This runs regardless of whether an owner filter is specified. - if !isSystemAMI && amiOwner != accountID { + if !callerCanReadAMI(amiMeta, accountID) { continue } @@ -264,33 +262,12 @@ func (s *ImageServiceImpl) DescribeImages(input *ec2.DescribeImagesInput, accoun Hypervisor: aws.String("xen"), // Default hypervisor } - // Add root device mapping - if amiMeta.RootDeviceType == "ebs" { + if bdms := synthesizeRootBlockDeviceMapping(amiMeta); bdms != nil { image.RootDeviceName = aws.String("/dev/sda1") - image.BlockDeviceMappings = []*ec2.BlockDeviceMapping{ - { - DeviceName: aws.String("/dev/sda1"), - Ebs: &ec2.EbsBlockDevice{ - VolumeSize: aws.Int64(utils.SafeUint64ToInt64(amiMeta.VolumeSizeGiB)), - VolumeType: aws.String("gp3"), - DeleteOnTermination: aws.Bool(true), - Encrypted: aws.Bool(false), - }, - }, - } + image.BlockDeviceMappings = bdms } - // Add tags if available - if len(amiMeta.Tags) > 0 { - tags := make([]*ec2.Tag, 0, len(amiMeta.Tags)) - for key, value := range amiMeta.Tags { - tags = append(tags, &ec2.Tag{ - Key: aws.String(key), - Value: aws.String(value), - }) - } - image.Tags = tags - } + image.Tags = utils.MapToEC2Tags(amiMeta.Tags) // Apply filters against the fully-built image if len(parsedFilters) > 0 && !imageMatchesFilters(image, parsedFilters, amiMeta.Tags) { @@ -446,40 +423,21 @@ func (s *ImageServiceImpl) CreateImageFromInstance(params CreateImageParams, acc } // Step 5: Build and store AMI config - description := aws.StringValue(input.Description) - - amiConfig := viperblock.VBState{ - VolumeConfig: viperblock.VolumeConfig{ - AMIMetadata: viperblock.AMIMetadata{ - ImageID: amiID, - Name: name, - Description: description, - SnapshotID: snapshotID, - Architecture: sourceAMI.Architecture, - PlatformDetails: sourceAMI.PlatformDetails, - Virtualization: sourceAMI.Virtualization, - VolumeSizeGiB: volumeSizeGiB, - CreationDate: time.Now(), - RootDeviceType: "ebs", - ImageOwnerAlias: accountID, - }, - }, + meta := viperblock.AMIMetadata{ + ImageID: amiID, + Name: name, + Description: aws.StringValue(input.Description), + SnapshotID: snapshotID, + Architecture: sourceAMI.Architecture, + PlatformDetails: sourceAMI.PlatformDetails, + Virtualization: sourceAMI.Virtualization, + VolumeSizeGiB: volumeSizeGiB, + CreationDate: time.Now(), + RootDeviceType: ec2.DeviceTypeEbs, + ImageOwnerAlias: accountID, } - configData, err := json.Marshal(amiConfig) - if err != nil { - slog.Error("CreateImageFromInstance: failed to marshal AMI config", "err", err) - return nil, errors.New(awserrors.ErrorServerInternal) - } - - configKey := fmt.Sprintf("%s/config.json", amiID) - _, err = s.store.PutObject(&s3.PutObjectInput{ - Bucket: aws.String(s.bucketName), - Key: aws.String(configKey), - Body: bytes.NewReader(configData), - ContentType: aws.String("application/json"), - }) - if err != nil { + if err := s.putAMIConfig(amiID, meta); err != nil { slog.Error("CreateImageFromInstance: failed to store AMI config", "amiId", amiID, "err", err) return nil, errors.New(awserrors.ErrorServerInternal) } @@ -607,6 +565,8 @@ func (s *ImageServiceImpl) getVolumeConfig(volumeID string) (*viperblock.VolumeC } // amiNameExists checks if any existing AMI already uses the given name. +// NoSuchKey is skipped (concurrent deregister race); any other read/decode +// error is surfaced so we don't silently allow a duplicate. func (s *ImageServiceImpl) amiNameExists(name string) (bool, error) { listResult, err := s.store.ListObjectsV2(&s3.ListObjectsV2Input{ Bucket: aws.String(s.bucketName), @@ -627,14 +587,17 @@ func (s *ImageServiceImpl) amiNameExists(name string) (bool, error) { Key: aws.String(configKey), }) if err != nil { - continue + if objectstore.IsNoSuchKeyError(err) { + continue + } + return false, fmt.Errorf("amiNameExists: failed to read %s: %w", configKey, err) } var vbState viperblock.VBState decodeErr := json.NewDecoder(result.Body).Decode(&vbState) _ = result.Body.Close() if decodeErr != nil { - continue + return false, fmt.Errorf("amiNameExists: failed to decode %s: %w", configKey, decodeErr) } if vbState.VolumeConfig.AMIMetadata.Name == name { @@ -645,8 +608,13 @@ func (s *ImageServiceImpl) amiNameExists(name string) (bool, error) { return false, nil } -// GetAMIConfig retrieves the AMI metadata for a given image ID from S3. -// Returns NoSuchKeyError if the AMI does not exist. +// ErrCorruptAMIConfig wraps JSON decode failures on AMI config.json so callers +// can distinguish a truly-missing AMI from one whose config exists but can't +// be parsed. +var ErrCorruptAMIConfig = errors.New("corrupt AMI config") + +// GetAMIConfig returns NoSuchKeyError if the AMI is missing, or an error +// wrapping ErrCorruptAMIConfig on decode failure. func (s *ImageServiceImpl) GetAMIConfig(imageID string) (viperblock.AMIMetadata, error) { configKey := fmt.Sprintf("%s/config.json", imageID) result, err := s.store.GetObject(&s3.GetObjectInput{ @@ -660,11 +628,85 @@ func (s *ImageServiceImpl) GetAMIConfig(imageID string) (viperblock.AMIMetadata, var vbState viperblock.VBState if err := json.NewDecoder(result.Body).Decode(&vbState); err != nil { - return viperblock.AMIMetadata{}, err + return viperblock.AMIMetadata{}, fmt.Errorf("%w: %s: %v", ErrCorruptAMIConfig, configKey, err) } return vbState.VolumeConfig.AMIMetadata, nil } +// putAMIConfig writes AMI metadata to {imageID}/config.json, preserving the +// VBState wrapper used by GetAMIConfig. +func (s *ImageServiceImpl) putAMIConfig(imageID string, meta viperblock.AMIMetadata) error { + state := viperblock.VBState{ + VolumeConfig: viperblock.VolumeConfig{ + AMIMetadata: meta, + }, + } + + data, err := json.Marshal(state) + if err != nil { + return err + } + + configKey := fmt.Sprintf("%s/config.json", imageID) + _, err = s.store.PutObject(&s3.PutObjectInput{ + Bucket: aws.String(s.bucketName), + Key: aws.String(configKey), + Body: bytes.NewReader(data), + ContentType: aws.String("application/json"), + }) + return err +} + +// checkAMIOwnership rejects cross-account and system-AMI mutations. Empty +// owner is ServerInternal (corrupt config) rather than a misleading 403. +func (s *ImageServiceImpl) checkAMIOwnership(meta viperblock.AMIMetadata, accountID string) error { + owner := meta.ImageOwnerAlias + if owner == "" { + slog.Error("checkAMIOwnership: AMI config has empty ImageOwnerAlias", "imageId", meta.ImageID) + return errors.New(awserrors.ErrorServerInternal) + } + if !utils.IsAccountID(owner) || owner != accountID { + return errors.New(awserrors.ErrorUnauthorizedOperation) + } + return nil +} + +// callerCanReadAMI: empty owner is invisible (corrupt); non-account owner is +// a system AMI visible to all; account owner is private to that account. +func callerCanReadAMI(meta viperblock.AMIMetadata, accountID string) bool { + owner := meta.ImageOwnerAlias + if owner == "" { + return false + } + if !utils.IsAccountID(owner) { + return true + } + return owner == accountID +} + +// loadAMIForMutation fetches the AMI and enforces ownership. Cross-account +// callers see UnauthorizedOperation, not NotFound (they already know the ID). +// No CAS: concurrent writers last-write-wins on the full struct. +func (s *ImageServiceImpl) loadAMIForMutation(imageID, accountID string) (viperblock.AMIMetadata, error) { + if !strings.HasPrefix(imageID, "ami-") { + return viperblock.AMIMetadata{}, errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + + meta, err := s.GetAMIConfig(imageID) + if err != nil { + if objectstore.IsNoSuchKeyError(err) { + return viperblock.AMIMetadata{}, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + slog.Error("loadAMIForMutation: failed to read AMI config", "imageId", imageID, "err", err) + return viperblock.AMIMetadata{}, errors.New(awserrors.ErrorServerInternal) + } + + if err := s.checkAMIOwnership(meta, accountID); err != nil { + return viperblock.AMIMetadata{}, err + } + return meta, nil +} + // putSnapshotMetadata stores snapshot metadata in S3 using the canonical SnapshotConfig type func (s *ImageServiceImpl) putSnapshotMetadata(snapshotID, volumeID string, volumeSizeGiB uint64, accountID string) error { cfg := handlers_ec2_snapshot.SnapshotConfig{ @@ -676,42 +718,428 @@ func (s *ImageServiceImpl) putSnapshotMetadata(snapshotID, volumeID string, volu StartTime: time.Now(), OwnerID: accountID, } + return handlers_ec2_snapshot.WriteSnapshotConfig(s.store, s.bucketName, snapshotID, &cfg) +} - data, err := json.Marshal(cfg) - if err != nil { - return err +// CopyImage clones an AMI same-region, metadata-only: the new snapshot shares +// the source's VolumeID and a fresh config.json points at it. +// Source visibility is checked before the O(n) name-uniqueness scan so typos +// and cross-account sources fast-fail without a full AMI listing. +func (s *ImageServiceImpl) CopyImage(input *ec2.CopyImageInput, accountID string) (*ec2.CopyImageOutput, error) { + if input == nil || input.Name == nil || *input.Name == "" || + input.SourceImageId == nil || *input.SourceImageId == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) } - key := fmt.Sprintf("%s/metadata.json", snapshotID) - _, err = s.store.PutObject(&s3.PutObjectInput{ - Bucket: aws.String(s.bucketName), - Key: aws.String(key), - Body: bytes.NewReader(data), - ContentType: aws.String("application/json"), - }) - return err + name := *input.Name + sourceImageID := *input.SourceImageId + + srcMeta, err := s.GetAMIConfig(sourceImageID) + if err != nil { + // Corrupt source is treated as NotFound so callers can't tell which + // half of the AMI/snapshot pair is broken. + if objectstore.IsNoSuchKeyError(err) || errors.Is(err, ErrCorruptAMIConfig) { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + slog.Error("CopyImage: failed to read source AMI config", "sourceImageId", sourceImageID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + if !callerCanReadAMI(srcMeta, accountID) { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + + // Orphaned source (missing SnapshotID, or snapshot gone/corrupt) is + // reported as NotFound — don't leak the half-broken state. + if srcMeta.SnapshotID == "" { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + srcSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(s.store, s.bucketName, srcMeta.SnapshotID) + if err != nil { + // Bundled system AMIs (admin-imported via `spx admin images import`) + // store blocks under ami-xxx/ with no standalone snap-xxx/metadata.json + // — the SnapshotID field is a viperblock-internal snap reference. To + // keep copy-image AWS-compatible (copy of a public/shared AMI must + // succeed), synthesize a minimal snap view from the AMI itself: the + // bundled prefix IS the volume, so VolumeID = sourceImageID. + if objectstore.IsNoSuchKeyError(err) && srcMeta.ImageOwnerAlias != "" && !utils.IsAccountID(srcMeta.ImageOwnerAlias) { + srcSnap = &handlers_ec2_snapshot.SnapshotConfig{ + SnapshotID: srcMeta.SnapshotID, + VolumeID: sourceImageID, + VolumeSize: utils.SafeUint64ToInt64(srcMeta.VolumeSizeGiB), + } + } else if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } else { + slog.Error("CopyImage: failed to read source snapshot metadata", + "sourceImageId", sourceImageID, "snapshotId", srcMeta.SnapshotID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + } + + if exists, err := s.amiNameExists(name); err != nil { + slog.Error("CopyImage: failed to check AMI name uniqueness", "name", name, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } else if exists { + return nil, errors.New(awserrors.ErrorInvalidAMINameDuplicate) + } + + newSnapshotID := utils.GenerateResourceID("snap") + newImageID := utils.GenerateResourceID("ami") + + // New snap shares source VolumeID — no block copy. + snapSizeGiB := uint64(0) + if srcSnap.VolumeSize > 0 { + snapSizeGiB = uint64(srcSnap.VolumeSize) + } + if err := s.putSnapshotMetadata(newSnapshotID, srcSnap.VolumeID, snapSizeGiB, accountID); err != nil { + slog.Error("CopyImage: failed to write snapshot metadata", "snapshotId", newSnapshotID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + description := srcMeta.Description + if input.Description != nil { + description = *input.Description + } + + rootDeviceType := srcMeta.RootDeviceType + if rootDeviceType == "" { + rootDeviceType = "ebs" + } + + tags := mergeCopyImageTags(srcMeta.Tags, input.TagSpecifications, aws.BoolValue(input.CopyImageTags)) + + meta := viperblock.AMIMetadata{ + ImageID: newImageID, + Name: name, + Description: description, + SnapshotID: newSnapshotID, + Architecture: srcMeta.Architecture, + PlatformDetails: srcMeta.PlatformDetails, + Virtualization: srcMeta.Virtualization, + VolumeSizeGiB: srcMeta.VolumeSizeGiB, + RootDeviceType: rootDeviceType, + ImageOwnerAlias: accountID, + CreationDate: time.Now(), + Tags: tags, + } + + if err := s.putAMIConfig(newImageID, meta); err != nil { + slog.Error("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(&s3.DeleteObjectInput{ + Bucket: aws.String(s.bucketName), + Key: aws.String(snapKey), + }); delErr != nil { + slog.Error("CopyImage: failed to roll back orphaned snapshot metadata", + "snapshotId", newSnapshotID, "err", delErr) + } + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.Info("CopyImage completed", + "sourceImageId", sourceImageID, "newImageId", newImageID, + "sourceSnapshotId", srcMeta.SnapshotID, "newSnapshotId", newSnapshotID, + "accountId", accountID) + + return &ec2.CopyImageOutput{ImageId: aws.String(newImageID)}, nil } -func (s *ImageServiceImpl) CopyImage(input *ec2.CopyImageInput, accountID string) (*ec2.CopyImageOutput, error) { - return nil, errors.New("CopyImage not yet implemented") +// mergeCopyImageTags seeds with source tags when copyImageTags is true, then +// lets image-resource TagSpecifications override colliding keys. Non-image tag +// specs are ignored. +func mergeCopyImageTags(srcTags map[string]string, specs []*ec2.TagSpecification, copyImageTags bool) map[string]string { + merged := make(map[string]string) + if copyImageTags { + maps.Copy(merged, srcTags) + } + maps.Copy(merged, utils.ExtractTags(specs, "image")) + if len(merged) == 0 { + return nil + } + return merged } +// DescribeImageAttribute supports description and blockDeviceMapping only. +// Cross-account reads return NotFound so the caller can't learn the ID exists +// in another account. func (s *ImageServiceImpl) DescribeImageAttribute(input *ec2.DescribeImageAttributeInput, accountID string) (*ec2.DescribeImageAttributeOutput, error) { - return nil, errors.New("DescribeImageAttribute not yet implemented") + if input == nil || input.ImageId == nil || *input.ImageId == "" || + input.Attribute == nil || *input.Attribute == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + + imageID := *input.ImageId + attribute := *input.Attribute + + meta, err := s.GetAMIConfig(imageID) + if err != nil { + if objectstore.IsNoSuchKeyError(err) { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + slog.Error("DescribeImageAttribute: failed to read AMI config", "imageId", imageID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + if !callerCanReadAMI(meta, accountID) { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + + output := &ec2.DescribeImageAttributeOutput{ + ImageId: aws.String(imageID), + } + + switch attribute { + case ec2.ImageAttributeNameDescription: + output.Description = &ec2.AttributeValue{Value: aws.String(meta.Description)} + case ec2.ImageAttributeNameBlockDeviceMapping: + output.BlockDeviceMappings = synthesizeRootBlockDeviceMapping(meta) + default: + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + + slog.Info("DescribeImageAttribute completed", "imageId", imageID, "attribute", attribute, "accountId", accountID) + return output, nil } +// synthesizeRootBlockDeviceMapping returns /dev/sda1 with size+snapshot from +// AMIMetadata, or nil for non-EBS AMIs. +func synthesizeRootBlockDeviceMapping(meta viperblock.AMIMetadata) []*ec2.BlockDeviceMapping { + if meta.RootDeviceType != "ebs" { + return nil + } + ebs := &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(utils.SafeUint64ToInt64(meta.VolumeSizeGiB)), + VolumeType: aws.String("gp3"), + DeleteOnTermination: aws.Bool(true), + Encrypted: aws.Bool(false), + } + if meta.SnapshotID != "" { + ebs.SnapshotId = aws.String(meta.SnapshotID) + } + return []*ec2.BlockDeviceMapping{{ + DeviceName: aws.String("/dev/sda1"), + Ebs: ebs, + }} +} + +// RegisterImage writes AMI metadata pointing at an existing snapshot. Never +// touches block data. The snapshot read runs before the O(n) name-uniqueness +// scan so a missing snapshot fast-fails. func (s *ImageServiceImpl) RegisterImage(input *ec2.RegisterImageInput, accountID string) (*ec2.RegisterImageOutput, error) { - return nil, errors.New("RegisterImage not yet implemented") + if input == nil || input.Name == nil || *input.Name == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + + name := *input.Name + + rootBDM := pickRootSnapshotBDM(input.BlockDeviceMappings, input.RootDeviceName) + if rootBDM == nil { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + snapshotID := *rootBDM.Ebs.SnapshotId + + snapCfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(s.store, s.bucketName, snapshotID) + if err != nil { + // Corrupt snapshot is surfaced as NotFound, same as CopyImage. + if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) + } + slog.Error("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.Warn("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) + } + + volumeSizeGiB := snapSizeGiB + if rootBDM.Ebs.VolumeSize != nil && *rootBDM.Ebs.VolumeSize > 0 { + requested := uint64(*rootBDM.Ebs.VolumeSize) + if requested < snapSizeGiB { + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + volumeSizeGiB = requested + } + + if exists, err := s.amiNameExists(name); err != nil { + slog.Error("RegisterImage: failed to check AMI name uniqueness", "name", name, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } else if exists { + return nil, errors.New(awserrors.ErrorInvalidAMINameDuplicate) + } + + architecture := "x86_64" + if input.Architecture != nil && *input.Architecture != "" { + architecture = *input.Architecture + } + virtualization := "hvm" + if input.VirtualizationType != nil && *input.VirtualizationType != "" { + virtualization = *input.VirtualizationType + } + description := "" + if input.Description != nil { + description = *input.Description + } + + tags := utils.ExtractTags(input.TagSpecifications, "image") + + amiID := utils.GenerateResourceID("ami") + meta := viperblock.AMIMetadata{ + ImageID: amiID, + Name: name, + Description: description, + SnapshotID: snapshotID, + Architecture: architecture, + PlatformDetails: "Linux/UNIX", + Virtualization: virtualization, + VolumeSizeGiB: volumeSizeGiB, + RootDeviceType: "ebs", + ImageOwnerAlias: accountID, + CreationDate: time.Now(), + Tags: tags, + } + + if err := s.putAMIConfig(amiID, meta); err != nil { + slog.Error("RegisterImage: failed to write AMI config", "amiId", amiID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.Info("RegisterImage completed", "amiId", amiID, "snapshotId", snapshotID, "accountId", accountID) + return &ec2.RegisterImageOutput{ImageId: aws.String(amiID)}, nil } +// pickRootSnapshotBDM returns the BDM matching RootDeviceName (if set) that +// carries a non-empty EBS snapshot reference, else the first such BDM. +func pickRootSnapshotBDM(mappings []*ec2.BlockDeviceMapping, rootDeviceName *string) *ec2.BlockDeviceMapping { + wantName := "" + if rootDeviceName != nil { + wantName = *rootDeviceName + } + + for _, bdm := range mappings { + if bdm == nil || bdm.Ebs == nil || bdm.Ebs.SnapshotId == nil || *bdm.Ebs.SnapshotId == "" { + continue + } + if wantName != "" { + if bdm.DeviceName == nil || *bdm.DeviceName != wantName { + continue + } + } + return bdm + } + return nil +} + +// DeregisterImage hard-deletes config.json. Backing snapshot is untouched, so +// operators run delete-snapshot separately to reclaim block storage. func (s *ImageServiceImpl) DeregisterImage(input *ec2.DeregisterImageInput, accountID string) (*ec2.DeregisterImageOutput, error) { - return nil, errors.New("DeregisterImage not yet implemented") + if input == nil || input.ImageId == nil || *input.ImageId == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + + imageID := *input.ImageId + + if _, err := s.loadAMIForMutation(imageID, accountID); err != nil { + return nil, err + } + + configKey := fmt.Sprintf("%s/config.json", imageID) + if _, err := s.store.DeleteObject(&s3.DeleteObjectInput{ + Bucket: aws.String(s.bucketName), + Key: aws.String(configKey), + }); err != nil { + slog.Error("DeregisterImage: failed to delete AMI config", "imageId", imageID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.Info("DeregisterImage completed", "imageId", imageID, "accountId", accountID) + return &ec2.DeregisterImageOutput{}, nil } +// ModifyImageAttribute writes a modifiable AMI attribute. Gateway normalises +// into Attribute+Value; only description is writable. Ownership is checked +// before the attribute switch so cross-account callers always see +// UnauthorizedOperation. func (s *ImageServiceImpl) ModifyImageAttribute(input *ec2.ModifyImageAttributeInput, accountID string) (*ec2.ModifyImageAttributeOutput, error) { - return nil, errors.New("ModifyImageAttribute not yet implemented") + if input == nil || input.ImageId == nil || *input.ImageId == "" || + input.Attribute == nil || *input.Attribute == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + + imageID := *input.ImageId + attribute := *input.Attribute + + meta, err := s.loadAMIForMutation(imageID, accountID) + if err != nil { + return nil, err + } + + if attribute != ec2.ImageAttributeNameDescription { + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + + newValue := "" + if input.Value != nil { + newValue = *input.Value + } + // No-op guard: Terraform aws_ami refresh otherwise churns out no-op writes. + if meta.Description == newValue { + slog.Info("ModifyImageAttribute no-op", "imageId", imageID, "attribute", attribute, "accountId", accountID) + return &ec2.ModifyImageAttributeOutput{}, nil + } + meta.Description = newValue + + if err := s.putAMIConfig(imageID, meta); err != nil { + slog.Error("ModifyImageAttribute: failed to write AMI config", "imageId", imageID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.Info("ModifyImageAttribute completed", "imageId", imageID, "attribute", attribute, "accountId", accountID) + return &ec2.ModifyImageAttributeOutput{}, nil } +// ResetImageAttribute clears the description (the only supported attribute). +// launchPermission — AWS's default reset target — is out of scope. func (s *ImageServiceImpl) ResetImageAttribute(input *ec2.ResetImageAttributeInput, accountID string) (*ec2.ResetImageAttributeOutput, error) { - return nil, errors.New("ResetImageAttribute not yet implemented") + if input == nil || input.ImageId == nil || *input.ImageId == "" || + input.Attribute == nil || *input.Attribute == "" { + return nil, errors.New(awserrors.ErrorMissingParameter) + } + + imageID := *input.ImageId + attribute := *input.Attribute + + meta, err := s.loadAMIForMutation(imageID, accountID) + if err != nil { + return nil, err + } + + if attribute != ec2.ImageAttributeNameDescription { + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + + if meta.Description == "" { + slog.Info("ResetImageAttribute no-op", "imageId", imageID, "attribute", attribute, "accountId", accountID) + return &ec2.ResetImageAttributeOutput{}, nil + } + meta.Description = "" + + if err := s.putAMIConfig(imageID, meta); err != nil { + slog.Error("ResetImageAttribute: failed to write AMI config", "imageId", imageID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + slog.Info("ResetImageAttribute completed", "imageId", imageID, "attribute", attribute, "accountId", accountID) + return &ec2.ResetImageAttributeOutput{}, nil } diff --git a/spinifex/handlers/ec2/image/service_impl_test.go b/spinifex/handlers/ec2/image/service_impl_test.go index b15c770fa..a798700c2 100644 --- a/spinifex/handlers/ec2/image/service_impl_test.go +++ b/spinifex/handlers/ec2/image/service_impl_test.go @@ -2,8 +2,10 @@ package handlers_ec2_image import ( "encoding/json" + "io" "strings" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -74,7 +76,9 @@ func createTestAMIConfig(t *testing.T, store *objectstore.MemoryObjectStore, ima require.NoError(t, err) } -// createTestAMIConfigWithName creates a test AMI config with a specified name +// createTestAMIConfigWithName creates a test AMI config with a specified name. +// Owner defaults to testAccountID so the AMI is visible to the default caller — +// an empty ImageOwnerAlias would be filtered out as a corrupt config. func createTestAMIConfigWithName(t *testing.T, store *objectstore.MemoryObjectStore, imageID, name string) { amiState := viperblock.VBState{ VolumeConfig: viperblock.VolumeConfig{ @@ -86,6 +90,7 @@ func createTestAMIConfigWithName(t *testing.T, store *objectstore.MemoryObjectSt Virtualization: "hvm", RootDeviceType: "ebs", VolumeSizeGiB: 8, + ImageOwnerAlias: testAccountID, }, }, } @@ -375,6 +380,185 @@ func TestCreateImageFromInstance_UniqueNameAllowed(t *testing.T) { assert.Equal(t, awserrors.ErrorServerInternal, err.Error()) } +func TestPutAMIConfig_RoundTrip(t *testing.T) { + svc, _ := setupTestImageService(t) + + meta := viperblock.AMIMetadata{ + ImageID: "ami-roundtrip01", + Name: "round-trip", + Description: "hello", + SnapshotID: "snap-rt01", + Architecture: "x86_64", + PlatformDetails: "Linux/UNIX", + Virtualization: "hvm", + VolumeSizeGiB: 16, + RootDeviceType: "ebs", + ImageOwnerAlias: testAccountID, + } + + require.NoError(t, svc.putAMIConfig(meta.ImageID, meta)) + + got, err := svc.GetAMIConfig(meta.ImageID) + require.NoError(t, err) + assert.Equal(t, meta.Name, got.Name) + assert.Equal(t, meta.Description, got.Description) + assert.Equal(t, meta.SnapshotID, got.SnapshotID) + assert.Equal(t, meta.VolumeSizeGiB, got.VolumeSizeGiB) + assert.Equal(t, meta.ImageOwnerAlias, got.ImageOwnerAlias) +} + +func TestCheckAMIOwnership(t *testing.T) { + svc, _ := setupTestImageService(t) + + caller := testAccountID + other := "000000000002" + + tests := []struct { + name string + owner string + wantErr string // "" = no error + }{ + {"OwnedByCaller", caller, ""}, + {"OwnedByOtherAccount", other, awserrors.ErrorUnauthorizedOperation}, + {"SystemAMI", "spinifex", awserrors.ErrorUnauthorizedOperation}, + // Empty owner indicates corrupt config — callers must see the real + // failure, not a misleading 403. + {"EmptyOwnerIsCorrupt", "", awserrors.ErrorServerInternal}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := svc.checkAMIOwnership(viperblock.AMIMetadata{ImageOwnerAlias: tt.owner}, caller) + if tt.wantErr == "" { + assert.NoError(t, err) + } else { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + } + }) + } +} + +func TestDeregisterImage_HappyPath(t *testing.T) { + svc, store := setupTestImageService(t) + + amiID := "ami-dereg001" + createTestAMIConfigWithOwner(t, store, amiID, "ami-to-delete", testAccountID) + + out, err := svc.DeregisterImage(&ec2.DeregisterImageInput{ImageId: aws.String(amiID)}, testAccountID) + require.NoError(t, err) + assert.NotNil(t, out) + + // AMI config gone from S3 + _, getErr := store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(amiID + "/config.json"), + }) + require.Error(t, getErr) + assert.True(t, objectstore.IsNoSuchKeyError(getErr)) + + // DescribeImages no longer returns it + _, descErr := svc.DescribeImages(&ec2.DescribeImagesInput{ + ImageIds: []*string{aws.String(amiID)}, + }, testAccountID) + require.Error(t, descErr) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, descErr.Error()) +} + +func TestDeregisterImage_NotFound(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.DeregisterImage(&ec2.DeregisterImageInput{ + ImageId: aws.String("ami-doesnotexist"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestDeregisterImage_Idempotent(t *testing.T) { + svc, store := setupTestImageService(t) + + amiID := "ami-idem001" + createTestAMIConfigWithOwner(t, store, amiID, "idempotent", testAccountID) + + _, err := svc.DeregisterImage(&ec2.DeregisterImageInput{ImageId: aws.String(amiID)}, testAccountID) + require.NoError(t, err) + + _, err = svc.DeregisterImage(&ec2.DeregisterImageInput{ImageId: aws.String(amiID)}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestDeregisterImage_CrossAccount(t *testing.T) { + svc, store := setupTestImageService(t) + + amiID := "ami-other001" + createTestAMIConfigWithOwner(t, store, amiID, "other-acct", "000000000002") + + _, err := svc.DeregisterImage(&ec2.DeregisterImageInput{ImageId: aws.String(amiID)}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error()) + + // Confirm AMI still present after rejected mutation. + _, getErr := store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(amiID + "/config.json"), + }) + require.NoError(t, getErr) +} + +func TestDeregisterImage_SystemAMI(t *testing.T) { + svc, store := setupTestImageService(t) + createTestAMIConfigWithOwner(t, store, "ami-sys001", "system-ami", "spinifex") + + _, err := svc.DeregisterImage(&ec2.DeregisterImageInput{ImageId: aws.String("ami-sys001")}, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error()) +} + +func TestDeregisterImage_DoesNotTouchSnapshot(t *testing.T) { + svc, store := setupTestImageService(t) + + amiID := "ami-keepsnap001" + snapID := "snap-keep001" + + // AMI pointing at a snapshot + amiState := viperblock.VBState{ + VolumeConfig: viperblock.VolumeConfig{ + AMIMetadata: viperblock.AMIMetadata{ + ImageID: amiID, + Name: "with-snapshot", + SnapshotID: snapID, + ImageOwnerAlias: testAccountID, + Architecture: "x86_64", + Virtualization: "hvm", + RootDeviceType: "ebs", + VolumeSizeGiB: 8, + }, + }, + } + data, err := json.Marshal(amiState) + require.NoError(t, err) + _, err = store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(amiID + "/config.json"), + Body: strings.NewReader(string(data)), + }) + require.NoError(t, err) + + // Backing snapshot metadata + require.NoError(t, svc.putSnapshotMetadata(snapID, "vol-keep", 8, testAccountID)) + + _, err = svc.DeregisterImage(&ec2.DeregisterImageInput{ImageId: aws.String(amiID)}, testAccountID) + require.NoError(t, err) + + // Snapshot metadata still present + _, snapErr := store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(snapID + "/metadata.json"), + }) + require.NoError(t, snapErr) +} + func TestDescribeImages_AccountScoping(t *testing.T) { svc, store := setupTestImageService(t) @@ -648,41 +832,23 @@ func TestAmiNameExists_InvalidJSON(t *testing.T) { }) require.NoError(t, err) - exists, err := svc.amiNameExists("any-name") - require.NoError(t, err) - assert.False(t, exists) -} - -func TestUnimplementedMethods(t *testing.T) { - svc, _ := setupTestImageService(t) - - _, err := svc.CreateImage(nil, "") - assert.Error(t, err) - - _, err = svc.CopyImage(nil, "") - assert.Error(t, err) - - _, err = svc.DescribeImageAttribute(nil, "") - assert.Error(t, err) - - _, err = svc.RegisterImage(nil, "") - assert.Error(t, err) - - _, err = svc.DeregisterImage(nil, "") - assert.Error(t, err) - - _, err = svc.ModifyImageAttribute(nil, "") - assert.Error(t, err) - - _, err = svc.ResetImageAttribute(nil, "") - assert.Error(t, err) + // A corrupt AMI config is a real store-side problem. Surface it rather + // than silently under-counting names (which would let a caller write a + // duplicate and mask the corruption). + _, err = svc.amiNameExists("any-name") + require.Error(t, err) } // --- DescribeImages filter tests --- // createTestAMIConfigFull creates an AMI with all metadata fields for filter testing. +// Owner defaults to testAccountID when the caller leaves ImageOwnerAlias empty — +// an empty owner would be filtered out as corrupt. func createTestAMIConfigFull(t *testing.T, store *objectstore.MemoryObjectStore, meta viperblock.AMIMetadata) { t.Helper() + if meta.ImageOwnerAlias == "" { + meta.ImageOwnerAlias = testAccountID + } amiState := viperblock.VBState{ VolumeConfig: viperblock.VolumeConfig{ AMIMetadata: meta, @@ -864,3 +1030,1241 @@ func TestDescribeImages_FilterByState(t *testing.T) { require.NoError(t, err) assert.Empty(t, out.Images) } + +// --- RegisterImage tests --- + +// putTestSnapshotConfig writes a SnapshotConfig at {snapshotID}/metadata.json, +// 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{ + 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(&awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(snapshotID + "/metadata.json"), + Body: strings.NewReader(string(data)), + ContentType: aws.String("application/json"), + }) + require.NoError(t, err) +} + +func validRegisterImageServiceInput(snapshotID string) *ec2.RegisterImageInput { + return &ec2.RegisterImageInput{ + Name: aws.String("registered-ami"), + RootDeviceName: aws.String("/dev/sda1"), + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String("/dev/sda1"), + Ebs: &ec2.EbsBlockDevice{ + SnapshotId: aws.String(snapshotID), + }, + }, + }, + } +} + +func TestRegisterImage_HappyPath(t *testing.T) { + svc, store := setupTestImageService(t) + + snapID := "snap-happy001" + putTestSnapshotConfig(t, store, snapID, 8, testAccountID) + + out, err := svc.RegisterImage(validRegisterImageServiceInput(snapID), testAccountID) + require.NoError(t, err) + require.NotNil(t, out) + require.NotNil(t, out.ImageId) + assert.True(t, strings.HasPrefix(*out.ImageId, "ami-")) + + // AMI should be visible via DescribeImages with correct defaults. + desc, err := svc.DescribeImages(&ec2.DescribeImagesInput{ + ImageIds: []*string{out.ImageId}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.Images, 1) + img := desc.Images[0] + assert.Equal(t, "registered-ami", *img.Name) + assert.Equal(t, "x86_64", *img.Architecture) + assert.Equal(t, "hvm", *img.VirtualizationType) + assert.Equal(t, "ebs", *img.RootDeviceType) + assert.Equal(t, testAccountID, *img.OwnerId) +} + +func TestRegisterImage_DuplicateName(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigWithOwner(t, store, "ami-existing01", "registered-ami", testAccountID) + putTestSnapshotConfig(t, store, "snap-dup01", 8, testAccountID) + + _, err := svc.RegisterImage(validRegisterImageServiceInput("snap-dup01"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMINameDuplicate, err.Error()) +} + +func TestRegisterImage_SnapshotNotFound(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.RegisterImage(validRegisterImageServiceInput("snap-missing"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, err.Error()) +} + +func TestRegisterImage_CrossAccountSnapshot(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-other01", 8, "000000000002") + + _, err := svc.RegisterImage(validRegisterImageServiceInput("snap-other01"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error()) +} + +func TestRegisterImage_SystemSnapshotAllowed(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") + + out, err := svc.RegisterImage(validRegisterImageServiceInput("snap-sys01"), testAccountID) + require.NoError(t, err) + require.NotNil(t, out.ImageId) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, testAccountID, meta.ImageOwnerAlias) +} + +func TestRegisterImage_ArchitectureAndVirtualizationDefaults(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-defaults", 8, testAccountID) + + input := validRegisterImageServiceInput("snap-defaults") + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "x86_64", meta.Architecture) + assert.Equal(t, "hvm", meta.Virtualization) + assert.Equal(t, "Linux/UNIX", meta.PlatformDetails) + assert.Equal(t, "ebs", meta.RootDeviceType) +} + +func TestRegisterImage_ExplicitArchitecture(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-arm64", 8, testAccountID) + + input := validRegisterImageServiceInput("snap-arm64") + input.Architecture = aws.String("arm64") + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "arm64", meta.Architecture) +} + +func TestRegisterImage_VolumeSizeSmallerThanSnapshotRejected(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-big", 20, testAccountID) + + input := validRegisterImageServiceInput("snap-big") + input.BlockDeviceMappings[0].Ebs.VolumeSize = aws.Int64(8) + + _, err := svc.RegisterImage(input, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) +} + +func TestRegisterImage_VolumeSizeLargerThanSnapshotHonoured(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-grow", 8, testAccountID) + + input := validRegisterImageServiceInput("snap-grow") + input.BlockDeviceMappings[0].Ebs.VolumeSize = aws.Int64(20) + + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, uint64(20), meta.VolumeSizeGiB) +} + +func TestRegisterImage_VolumeSizeFromSnapshot(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-size", 16, testAccountID) + + out, err := svc.RegisterImage(validRegisterImageServiceInput("snap-size"), testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, uint64(16), meta.VolumeSizeGiB) +} + +func TestRegisterImage_TagsPersisted(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-tags01", 8, testAccountID) + + input := validRegisterImageServiceInput("snap-tags01") + input.TagSpecifications = []*ec2.TagSpecification{ + { + ResourceType: aws.String("image"), + Tags: []*ec2.Tag{ + {Key: aws.String("Env"), Value: aws.String("prod")}, + {Key: aws.String("Owner"), Value: aws.String("team-a")}, + }, + }, + { + // Non-image resource type — tags must be ignored. + ResourceType: aws.String("snapshot"), + Tags: []*ec2.Tag{ + {Key: aws.String("ShouldNotAppear"), Value: aws.String("x")}, + }, + }, + } + + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "prod", meta.Tags["Env"]) + assert.Equal(t, "team-a", meta.Tags["Owner"]) + _, ok := meta.Tags["ShouldNotAppear"] + assert.False(t, ok) +} + +func TestRegisterImage_DescriptionPersisted(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-desc01", 8, testAccountID) + + input := validRegisterImageServiceInput("snap-desc01") + input.Description = aws.String("hand-built golden image") + + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "hand-built golden image", meta.Description) +} + +func TestRegisterImage_RootDeviceNameSelectsBDM(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-data", 4, testAccountID) + putTestSnapshotConfig(t, store, "snap-root", 8, testAccountID) + + input := &ec2.RegisterImageInput{ + Name: aws.String("multi-bdm-ami"), + RootDeviceName: aws.String("/dev/xvda"), + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String("/dev/xvdb"), + Ebs: &ec2.EbsBlockDevice{SnapshotId: aws.String("snap-data")}, + }, + { + DeviceName: aws.String("/dev/xvda"), + Ebs: &ec2.EbsBlockDevice{SnapshotId: aws.String("snap-root")}, + }, + }, + } + + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "snap-root", meta.SnapshotID) + assert.Equal(t, uint64(8), meta.VolumeSizeGiB) +} + +func TestRegisterImage_NilInput(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.RegisterImage(nil, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestRegisterImage_NoBlockDeviceMappings(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.RegisterImage(&ec2.RegisterImageInput{ + Name: aws.String("no-bdms"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestRegisterImage_BDMWithoutMatchingRootDevice(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-mismatch", 8, testAccountID) + + // Root device name doesn't match any BDM device name. + _, err := svc.RegisterImage(&ec2.RegisterImageInput{ + Name: aws.String("mismatched-root"), + RootDeviceName: aws.String("/dev/xvda"), + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String("/dev/sdb"), + Ebs: &ec2.EbsBlockDevice{SnapshotId: aws.String("snap-mismatch")}, + }, + nil, + {Ebs: nil}, // skipped: no Ebs + }, + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestRegisterImage_ExplicitVirtualizationType(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-virt", 8, testAccountID) + + input := validRegisterImageServiceInput("snap-virt") + input.VirtualizationType = aws.String("hvm") + + out, err := svc.RegisterImage(input, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "hvm", meta.Virtualization) +} + +func TestRegisterImage_NoRootDeviceNameUsesFirstSnapshotBDM(t *testing.T) { + svc, store := setupTestImageService(t) + + putTestSnapshotConfig(t, store, "snap-first", 8, testAccountID) + + out, err := svc.RegisterImage(&ec2.RegisterImageInput{ + Name: aws.String("no-rootdevname"), + // No RootDeviceName set; first BDM with a snapshot wins. + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + {Ebs: nil}, // skipped + {Ebs: &ec2.EbsBlockDevice{SnapshotId: aws.String("snap-first")}}, + }, + }, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "snap-first", meta.SnapshotID) +} + +func TestRegisterImage_OwnerSetToCaller(t *testing.T) { + svc, store := setupTestImageService(t) + + // Snapshot owned by caller; resulting AMI must record caller as owner. + putTestSnapshotConfig(t, store, "snap-own", 8, testAccountID) + + out, err := svc.RegisterImage(validRegisterImageServiceInput("snap-own"), testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, testAccountID, meta.ImageOwnerAlias) + assert.False(t, meta.CreationDate.IsZero()) +} + +// --- CopyImage tests --- + +// readAMIConfigBytes returns the raw bytes of {imageID}/config.json from the +// store so tests can prove the source was not mutated by a copy/modify/etc. +func readAMIConfigBytes(t *testing.T, store *objectstore.MemoryObjectStore, imageID string) []byte { + t.Helper() + result, err := store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(imageID + "/config.json"), + }) + require.NoError(t, err) + defer result.Body.Close() + data, err := io.ReadAll(result.Body) + require.NoError(t, err) + return data +} + +// readSnapshotConfigBytes returns the raw bytes of {snapshotID}/metadata.json. +func readSnapshotConfigBytes(t *testing.T, store *objectstore.MemoryObjectStore, snapshotID string) []byte { + t.Helper() + result, err := store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(snapshotID + "/metadata.json"), + }) + require.NoError(t, err) + defer result.Body.Close() + data, err := io.ReadAll(result.Body) + require.NoError(t, err) + return data +} + +// putTestAMIConfigWithSnapshot seeds an AMI config that carries a real +// SnapshotID — distinct from createTestAMIConfigWithOwner which sets no +// snapshot and would be treated as orphaned by CopyImage. +func putTestAMIConfigWithSnapshot(t *testing.T, store *objectstore.MemoryObjectStore, imageID, name, owner, snapshotID string, meta viperblock.AMIMetadata) { + t.Helper() + meta.ImageID = imageID + meta.Name = name + meta.ImageOwnerAlias = owner + meta.SnapshotID = snapshotID + if meta.Architecture == "" { + meta.Architecture = "x86_64" + } + if meta.PlatformDetails == "" { + meta.PlatformDetails = "Linux/UNIX" + } + if meta.Virtualization == "" { + meta.Virtualization = "hvm" + } + if meta.RootDeviceType == "" { + meta.RootDeviceType = "ebs" + } + if meta.VolumeSizeGiB == 0 { + meta.VolumeSizeGiB = 8 + } + amiState := viperblock.VBState{ + VolumeConfig: viperblock.VolumeConfig{AMIMetadata: meta}, + } + data, err := json.Marshal(amiState) + require.NoError(t, err) + _, err = store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(imageID + "/config.json"), + Body: strings.NewReader(string(data)), + ContentType: aws.String("application/json"), + }) + require.NoError(t, err) +} + +// seedCopyableAMI writes a matching (snapshot, AMI) pair so CopyImage can +// complete end-to-end. Returns the AMI metadata that was persisted (callers +// can customise fields before seeding by tweaking the returned VolumeID / +// tags via preceding calls, but the helper takes the simple path for the +// happy case). +func seedCopyableAMI(t *testing.T, store *objectstore.MemoryObjectStore, imageID, name, owner, snapshotID, volumeID string, sizeGiB int64) { + t.Helper() + cfg := handlers_ec2_snapshot.SnapshotConfig{ + SnapshotID: snapshotID, + VolumeID: volumeID, + VolumeSize: sizeGiB, + State: "completed", + Progress: "100%", + OwnerID: owner, + } + data, err := json.Marshal(cfg) + require.NoError(t, err) + _, err = store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(snapshotID + "/metadata.json"), + Body: strings.NewReader(string(data)), + ContentType: aws.String("application/json"), + }) + require.NoError(t, err) + + putTestAMIConfigWithSnapshot(t, store, imageID, name, owner, snapshotID, viperblock.AMIMetadata{ + VolumeSizeGiB: uint64(sizeGiB), + Description: "source desc", + }) +} + +func validCopyImageServiceInput(sourceImageID, newName string) *ec2.CopyImageInput { + return &ec2.CopyImageInput{ + Name: aws.String(newName), + SourceImageId: aws.String(sourceImageID), + SourceRegion: aws.String("ap-southeast-2"), + } +} + +func TestCopyImage_HappyPath(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-src001", "source-ami", testAccountID, "snap-src001", "vol-src001", 8) + + // 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") + + out, err := svc.CopyImage(validCopyImageServiceInput("ami-src001", "copy-of-source"), testAccountID) + require.NoError(t, err) + require.NotNil(t, out) + require.NotNil(t, out.ImageId) + assert.True(t, strings.HasPrefix(*out.ImageId, "ami-")) + assert.NotEqual(t, "ami-src001", *out.ImageId) + + // New AMI visible via DescribeImages, owned by caller. + desc, err := svc.DescribeImages(&ec2.DescribeImagesInput{ + ImageIds: []*string{out.ImageId}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, desc.Images, 1) + img := desc.Images[0] + assert.Equal(t, "copy-of-source", *img.Name) + assert.Equal(t, testAccountID, *img.OwnerId) + + // 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"), + "source snapshot metadata was mutated by CopyImage") +} + +func TestCopyImage_InheritsSourceFields(t *testing.T) { + svc, store := setupTestImageService(t) + + // Seed snapshot for the source AMI. + _, err := store.PutObject(&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"), + }) + require.NoError(t, err) + + // Source AMI with non-default fields that must propagate. + putTestAMIConfigWithSnapshot(t, store, "ami-arm001", "arm-source", testAccountID, "snap-arm001", viperblock.AMIMetadata{ + Architecture: "arm64", + PlatformDetails: "Linux/UNIX (arm64)", + Virtualization: "hvm", + VolumeSizeGiB: 32, + RootDeviceType: "ebs", + Description: "arm source", + }) + + before := time.Now() + out, err := svc.CopyImage(validCopyImageServiceInput("ami-arm001", "arm-copy"), testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "arm64", newMeta.Architecture) + assert.Equal(t, "Linux/UNIX (arm64)", newMeta.PlatformDetails) + assert.Equal(t, "hvm", newMeta.Virtualization) + assert.Equal(t, uint64(32), newMeta.VolumeSizeGiB) + assert.Equal(t, "ebs", newMeta.RootDeviceType) + assert.False(t, newMeta.CreationDate.Before(before), "CreationDate must be refreshed on copy, not inherited") +} + +func TestCopyImage_NewSnapshotSharesSourceVolumeID(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-shareblocks", "shareblocks", testAccountID, "snap-orig", "vol-shared", 16) + + srcSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(store, testBucket, "snap-orig") + require.NoError(t, err) + + out, err := svc.CopyImage(validCopyImageServiceInput("ami-shareblocks", "shared-copy"), testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + require.NotEqual(t, "snap-orig", newMeta.SnapshotID) + + newSnap, err := handlers_ec2_snapshot.ReadSnapshotConfig(store, testBucket, newMeta.SnapshotID) + require.NoError(t, err) + // 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. + assert.Equal(t, srcSnap.VolumeID, newSnap.VolumeID, + "new snapshot must share source's VolumeID (no block copy)") + assert.Equal(t, srcSnap.VolumeSize, newSnap.VolumeSize) + assert.Equal(t, testAccountID, newSnap.OwnerID) +} + +func TestCopyImage_SystemAMICopiedIntoCallerAccount(t *testing.T) { + svc, store := setupTestImageService(t) + + // System AMI (non-account-ID owner) with a snapshot also owned by system. + seedCopyableAMI(t, store, "ami-system001", "debian-system", "spinifex", "snap-system001", "vol-sys", 8) + + out, err := svc.CopyImage(validCopyImageServiceInput("ami-system001", "my-debian"), testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, testAccountID, newMeta.ImageOwnerAlias) + + // Source unchanged — still owned by "spinifex". + srcMeta, err := svc.GetAMIConfig("ami-system001") + require.NoError(t, err) + assert.Equal(t, "spinifex", srcMeta.ImageOwnerAlias) +} + +// Bundled system AMIs (admin-imported) have no standalone snap-xxx/metadata.json +// — their SnapshotID refers to a viperblock-internal snap, and blocks live under +// ami-xxx/. CopyImage must still succeed (AWS parity: copy of a public AMI works), +// falling back to synthesizing a snap view where VolumeID = sourceImageID. +func TestCopyImage_BundledSystemAMINoStandaloneSnap(t *testing.T) { + svc, store := setupTestImageService(t) + + // Seed only the AMI config, no snap-xxx/metadata.json object — matches the + // on-disk layout produced by `spx admin images import`. + putTestAMIConfigWithSnapshot(t, store, "ami-bundled01", "alpine-bundled", + "system", "snap-ami-bundled01", viperblock.AMIMetadata{ + VolumeSizeGiB: 8, + Description: "bundled source", + }) + + out, err := svc.CopyImage(validCopyImageServiceInput("ami-bundled01", "my-alpine"), testAccountID) + require.NoError(t, err) + require.NotNil(t, out.ImageId) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, testAccountID, newMeta.ImageOwnerAlias) + assert.Equal(t, uint64(8), newMeta.VolumeSizeGiB) + + // New snap must exist in predastore, be owned by the caller, and point at + // the bundled AMI's prefix (VolumeID = sourceImageID), not at the borrowed + // 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(store, testBucket, newMeta.SnapshotID) + require.NoError(t, err) + 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) + assert.Equal(t, testAccountID, newSnap.OwnerID) +} + +func TestCopyImage_CrossAccountHidesExistence(t *testing.T) { + svc, store := setupTestImageService(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") + + _, err := svc.CopyImage(validCopyImageServiceInput("ami-other001", "stolen-copy"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) + + // 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"), + "source snapshot metadata altered after rejected cross-account copy") +} + +func TestCopyImage_SourceNotFound(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.CopyImage(validCopyImageServiceInput("ami-missing", "copy"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestCopyImage_OrphanedSource_MissingSnapshot(t *testing.T) { + svc, store := setupTestImageService(t) + + // AMI config points at a snapshot that doesn't exist on S3. + putTestAMIConfigWithSnapshot(t, store, "ami-orphan", "orphan", testAccountID, "snap-ghost", viperblock.AMIMetadata{}) + + _, err := svc.CopyImage(validCopyImageServiceInput("ami-orphan", "orphan-copy"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestCopyImage_OrphanedSource_NoSnapshotID(t *testing.T) { + svc, store := setupTestImageService(t) + + // Admin-imported bundled-storage AMI: no SnapshotID. Not copyable by this API. + createTestAMIConfigWithOwner(t, store, "ami-bundled", "bundled", testAccountID) + + _, err := svc.CopyImage(validCopyImageServiceInput("ami-bundled", "bundled-copy"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestCopyImage_DuplicateName(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-dup-src", "source", testAccountID, "snap-dup-src", "vol-dup", 8) + createTestAMIConfigWithOwner(t, store, "ami-collide", "already-taken", testAccountID) + + _, err := svc.CopyImage(validCopyImageServiceInput("ami-dup-src", "already-taken"), testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMINameDuplicate, err.Error()) +} + +func TestCopyImage_CopyImageTagsInheritsSourceTags(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-tagged-src", "tagged", testAccountID, "snap-tagged", "vol-tagged", 8) + + // Overlay source tags on the seeded AMI. + srcMeta, err := svc.GetAMIConfig("ami-tagged-src") + require.NoError(t, err) + srcMeta.Tags = map[string]string{"Env": "prod", "Owner": "team-a"} + require.NoError(t, svc.putAMIConfig("ami-tagged-src", srcMeta)) + + input := validCopyImageServiceInput("ami-tagged-src", "copy-inherit-tags") + input.CopyImageTags = aws.Bool(true) + + out, err := svc.CopyImage(input, testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "prod", newMeta.Tags["Env"]) + assert.Equal(t, "team-a", newMeta.Tags["Owner"]) +} + +func TestCopyImage_ExplicitTagsOverrideSourceTags(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-merge-src", "merge-src", testAccountID, "snap-merge", "vol-merge", 8) + + srcMeta, err := svc.GetAMIConfig("ami-merge-src") + require.NoError(t, err) + srcMeta.Tags = map[string]string{"Env": "prod", "Owner": "team-a"} + require.NoError(t, svc.putAMIConfig("ami-merge-src", srcMeta)) + + input := validCopyImageServiceInput("ami-merge-src", "merge-copy") + input.CopyImageTags = aws.Bool(true) + input.TagSpecifications = []*ec2.TagSpecification{ + { + ResourceType: aws.String("image"), + Tags: []*ec2.Tag{ + // Override a source tag and add a new one. + {Key: aws.String("Env"), Value: aws.String("staging")}, + {Key: aws.String("Region"), Value: aws.String("apse2")}, + }, + }, + { + // Non-image specs are ignored. + ResourceType: aws.String("snapshot"), + Tags: []*ec2.Tag{ + {Key: aws.String("ShouldNotAppear"), Value: aws.String("x")}, + }, + }, + } + + out, err := svc.CopyImage(input, testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "staging", newMeta.Tags["Env"]) // overridden + assert.Equal(t, "team-a", newMeta.Tags["Owner"]) // inherited + assert.Equal(t, "apse2", newMeta.Tags["Region"]) // added + _, ok := newMeta.Tags["ShouldNotAppear"] + assert.False(t, ok) +} + +func TestCopyImage_CopyImageTagsFalseDropsSourceTags(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-drop-src", "drop-src", testAccountID, "snap-drop", "vol-drop", 8) + + srcMeta, err := svc.GetAMIConfig("ami-drop-src") + require.NoError(t, err) + srcMeta.Tags = map[string]string{"Env": "prod"} + require.NoError(t, svc.putAMIConfig("ami-drop-src", srcMeta)) + + input := validCopyImageServiceInput("ami-drop-src", "drop-copy") + // CopyImageTags unset — default behaviour drops source tags. + input.TagSpecifications = []*ec2.TagSpecification{ + { + ResourceType: aws.String("image"), + Tags: []*ec2.Tag{ + {Key: aws.String("New"), Value: aws.String("yes")}, + }, + }, + } + + out, err := svc.CopyImage(input, testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "yes", newMeta.Tags["New"]) + _, hasEnv := newMeta.Tags["Env"] + assert.False(t, hasEnv) +} + +func TestCopyImage_DescriptionInheritedWhenUnset(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-desc-src", "desc-src", testAccountID, "snap-desc", "vol-desc", 8) + + out, err := svc.CopyImage(validCopyImageServiceInput("ami-desc-src", "desc-inherit"), testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "source desc", newMeta.Description) +} + +func TestCopyImage_DescriptionOverriddenWhenSet(t *testing.T) { + svc, store := setupTestImageService(t) + + seedCopyableAMI(t, store, "ami-desc-ov", "desc-ov", testAccountID, "snap-ov", "vol-ov", 8) + + input := validCopyImageServiceInput("ami-desc-ov", "desc-override") + input.Description = aws.String("explicit override") + + out, err := svc.CopyImage(input, testAccountID) + require.NoError(t, err) + + newMeta, err := svc.GetAMIConfig(*out.ImageId) + require.NoError(t, err) + assert.Equal(t, "explicit override", newMeta.Description) +} + +func TestCopyImage_MissingRequiredFields(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.CopyImage(nil, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) + + _, err = svc.CopyImage(&ec2.CopyImageInput{ + SourceImageId: aws.String("ami-123"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) + + _, err = svc.CopyImage(&ec2.CopyImageInput{ + Name: aws.String("only-name"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +// --- Image attribute tests --- + +func createTestAMIConfigRich(t *testing.T, store *objectstore.MemoryObjectStore, meta viperblock.AMIMetadata) { + t.Helper() + state := viperblock.VBState{ + VolumeConfig: viperblock.VolumeConfig{AMIMetadata: meta}, + } + data, err := json.Marshal(state) + require.NoError(t, err) + + _, err = store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String(meta.ImageID + "/config.json"), + Body: strings.NewReader(string(data)), + ContentType: aws.String("application/json"), + }) + require.NoError(t, err) +} + +func TestDescribeImageAttribute_Description(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-desc01", + Name: "desc-ami", + Description: "the stored description", + Architecture: "x86_64", + PlatformDetails: "Linux/UNIX", + Virtualization: "hvm", + RootDeviceType: "ebs", + VolumeSizeGiB: 8, + ImageOwnerAlias: testAccountID, + }) + + out, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-desc01"), + Attribute: aws.String("description"), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, out) + require.NotNil(t, out.ImageId) + assert.Equal(t, "ami-desc01", *out.ImageId) + require.NotNil(t, out.Description) + require.NotNil(t, out.Description.Value) + assert.Equal(t, "the stored description", *out.Description.Value) + assert.Nil(t, out.BlockDeviceMappings) +} + +func TestDescribeImageAttribute_BlockDeviceMapping(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-bdm01", + Name: "bdm-ami", + SnapshotID: "snap-bdm01", + Architecture: "x86_64", + PlatformDetails: "Linux/UNIX", + Virtualization: "hvm", + RootDeviceType: "ebs", + VolumeSizeGiB: 20, + ImageOwnerAlias: testAccountID, + }) + + out, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-bdm01"), + Attribute: aws.String("blockDeviceMapping"), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, out) + require.Len(t, out.BlockDeviceMappings, 1) + bdm := out.BlockDeviceMappings[0] + require.NotNil(t, bdm.DeviceName) + assert.Equal(t, "/dev/sda1", *bdm.DeviceName) + require.NotNil(t, bdm.Ebs) + require.NotNil(t, bdm.Ebs.SnapshotId) + assert.Equal(t, "snap-bdm01", *bdm.Ebs.SnapshotId) + require.NotNil(t, bdm.Ebs.VolumeSize) + assert.Equal(t, int64(20), *bdm.Ebs.VolumeSize) + assert.Nil(t, out.Description) +} + +func TestDescribeImageAttribute_MissingParameters(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.DescribeImageAttribute(nil, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) + + _, err = svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-xx"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) + + _, err = svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + Attribute: aws.String("description"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestDescribeImageAttribute_NotFound(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-missing"), + Attribute: aws.String("description"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestDescribeImageAttribute_CrossAccountHidesExistence(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-cross01", + Name: "cross-ami", + Description: "secret", + RootDeviceType: "ebs", + ImageOwnerAlias: "000000000002", + }) + + _, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-cross01"), + Attribute: aws.String("description"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestDescribeImageAttribute_SystemAMIReadable(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-sys01", + Name: "system-ami", + Description: "baked-in", + RootDeviceType: "ebs", + ImageOwnerAlias: "system", + }) + + out, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-sys01"), + Attribute: aws.String("description"), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, out.Description) + require.NotNil(t, out.Description.Value) + assert.Equal(t, "baked-in", *out.Description.Value) +} + +func TestDescribeImageAttribute_UnsupportedAttribute(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigWithOwner(t, store, "ami-unsup01", "u", testAccountID) + + for _, attr := range []string{"launchPermission", "bootMode", "kernel", "ramdisk"} { + t.Run(attr, func(t *testing.T) { + _, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-unsup01"), + Attribute: aws.String(attr), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + }) + } +} + +func TestModifyImageAttribute_Description(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-mod01", + Name: "mod-ami", + Description: "old", + RootDeviceType: "ebs", + ImageOwnerAlias: testAccountID, + }) + + _, err := svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-mod01"), + Attribute: aws.String("description"), + Value: aws.String("new description"), + }, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig("ami-mod01") + require.NoError(t, err) + assert.Equal(t, "new description", meta.Description) + + // Round-trips via DescribeImageAttribute. + out, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-mod01"), + Attribute: aws.String("description"), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, out.Description) + require.NotNil(t, out.Description.Value) + assert.Equal(t, "new description", *out.Description.Value) +} + +func TestModifyImageAttribute_DescriptionEmptyValueClears(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-modclr01", + Name: "modclr-ami", + Description: "will-be-cleared", + RootDeviceType: "ebs", + ImageOwnerAlias: testAccountID, + }) + + _, err := svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-modclr01"), + Attribute: aws.String("description"), + Value: aws.String(""), + }, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig("ami-modclr01") + require.NoError(t, err) + assert.Equal(t, "", meta.Description) +} + +func TestModifyImageAttribute_CrossAccount(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-modx01", + Name: "modx-ami", + Description: "dont-touch", + RootDeviceType: "ebs", + ImageOwnerAlias: "000000000002", + }) + + _, err := svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-modx01"), + Attribute: aws.String("description"), + Value: aws.String("evil"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error()) + + // Value untouched on S3. + meta, err := svc.GetAMIConfig("ami-modx01") + require.NoError(t, err) + assert.Equal(t, "dont-touch", meta.Description) +} + +func TestModifyImageAttribute_SystemAMIImmutable(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-modsys01", + Name: "sys-ami", + Description: "baked-in", + RootDeviceType: "ebs", + ImageOwnerAlias: "system", + }) + + _, err := svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-modsys01"), + Attribute: aws.String("description"), + Value: aws.String("tampered"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error()) +} + +func TestModifyImageAttribute_NotFound(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-missing"), + Attribute: aws.String("description"), + Value: aws.String("x"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestModifyImageAttribute_UnsupportedAttribute(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigWithOwner(t, store, "ami-modunsup01", "u", testAccountID) + + for _, attr := range []string{"bootMode", "launchPermission", "blockDeviceMapping"} { + t.Run(attr, func(t *testing.T) { + _, err := svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-modunsup01"), + Attribute: aws.String(attr), + Value: aws.String("x"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + }) + } +} + +func TestModifyImageAttribute_MissingParameters(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.ModifyImageAttribute(nil, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) + + _, err = svc.ModifyImageAttribute(&ec2.ModifyImageAttributeInput{ + ImageId: aws.String("ami-x"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} + +func TestResetImageAttribute_Description(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-reset01", + Name: "reset-ami", + Description: "will-be-cleared", + RootDeviceType: "ebs", + ImageOwnerAlias: testAccountID, + }) + + _, err := svc.ResetImageAttribute(&ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-reset01"), + Attribute: aws.String("description"), + }, testAccountID) + require.NoError(t, err) + + meta, err := svc.GetAMIConfig("ami-reset01") + require.NoError(t, err) + assert.Equal(t, "", meta.Description) + + out, err := svc.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{ + ImageId: aws.String("ami-reset01"), + Attribute: aws.String("description"), + }, testAccountID) + require.NoError(t, err) + require.NotNil(t, out.Description) + require.NotNil(t, out.Description.Value) + assert.Equal(t, "", *out.Description.Value) +} + +func TestResetImageAttribute_CrossAccount(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigRich(t, store, viperblock.AMIMetadata{ + ImageID: "ami-rstx01", + Name: "rstx", + Description: "dont-touch", + RootDeviceType: "ebs", + ImageOwnerAlias: "000000000002", + }) + + _, err := svc.ResetImageAttribute(&ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-rstx01"), + Attribute: aws.String("description"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorUnauthorizedOperation, err.Error()) + + meta, err := svc.GetAMIConfig("ami-rstx01") + require.NoError(t, err) + assert.Equal(t, "dont-touch", meta.Description) +} + +func TestResetImageAttribute_NotFound(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.ResetImageAttribute(&ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-missing"), + Attribute: aws.String("description"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestResetImageAttribute_UnsupportedAttribute(t *testing.T) { + svc, store := setupTestImageService(t) + + createTestAMIConfigWithOwner(t, store, "ami-rstunsup01", "u", testAccountID) + + for _, attr := range []string{"launchPermission", "bootMode", "blockDeviceMapping"} { + t.Run(attr, func(t *testing.T) { + _, err := svc.ResetImageAttribute(&ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-rstunsup01"), + Attribute: aws.String(attr), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + }) + } +} + +func TestResetImageAttribute_MissingParameters(t *testing.T) { + svc, _ := setupTestImageService(t) + + _, err := svc.ResetImageAttribute(nil, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) + + _, err = svc.ResetImageAttribute(&ec2.ResetImageAttributeInput{ + ImageId: aws.String("ami-x"), + }, testAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorMissingParameter, err.Error()) +} diff --git a/spinifex/handlers/ec2/image/service_nats.go b/spinifex/handlers/ec2/image/service_nats.go index 40a58ce36..00ab7d6d7 100644 --- a/spinifex/handlers/ec2/image/service_nats.go +++ b/spinifex/handlers/ec2/image/service_nats.go @@ -1,7 +1,6 @@ package handlers_ec2_image import ( - "errors" "time" "github.com/aws/aws-sdk-go/service/ec2" @@ -31,25 +30,25 @@ func (s *NATSImageService) CreateImage(input *ec2.CreateImageInput, accountID st } func (s *NATSImageService) CopyImage(input *ec2.CopyImageInput, accountID string) (*ec2.CopyImageOutput, error) { - return nil, errors.New("CopyImage not yet implemented") + return utils.NATSRequest[ec2.CopyImageOutput](s.natsConn, "ec2.CopyImage", input, 30*time.Second, accountID) } func (s *NATSImageService) DescribeImageAttribute(input *ec2.DescribeImageAttributeInput, accountID string) (*ec2.DescribeImageAttributeOutput, error) { - return nil, errors.New("DescribeImageAttribute not yet implemented") + return utils.NATSRequest[ec2.DescribeImageAttributeOutput](s.natsConn, "ec2.DescribeImageAttribute", input, 30*time.Second, accountID) } func (s *NATSImageService) RegisterImage(input *ec2.RegisterImageInput, accountID string) (*ec2.RegisterImageOutput, error) { - return nil, errors.New("RegisterImage not yet implemented") + return utils.NATSRequest[ec2.RegisterImageOutput](s.natsConn, "ec2.RegisterImage", input, 30*time.Second, accountID) } func (s *NATSImageService) DeregisterImage(input *ec2.DeregisterImageInput, accountID string) (*ec2.DeregisterImageOutput, error) { - return nil, errors.New("DeregisterImage not yet implemented") + return utils.NATSRequest[ec2.DeregisterImageOutput](s.natsConn, "ec2.DeregisterImage", input, 30*time.Second, accountID) } func (s *NATSImageService) ModifyImageAttribute(input *ec2.ModifyImageAttributeInput, accountID string) (*ec2.ModifyImageAttributeOutput, error) { - return nil, errors.New("ModifyImageAttribute not yet implemented") + return utils.NATSRequest[ec2.ModifyImageAttributeOutput](s.natsConn, "ec2.ModifyImageAttribute", input, 30*time.Second, accountID) } func (s *NATSImageService) ResetImageAttribute(input *ec2.ResetImageAttributeInput, accountID string) (*ec2.ResetImageAttributeOutput, error) { - return nil, errors.New("ResetImageAttribute not yet implemented") + return utils.NATSRequest[ec2.ResetImageAttributeOutput](s.natsConn, "ec2.ResetImageAttribute", input, 30*time.Second, accountID) } diff --git a/spinifex/handlers/ec2/snapshot/service_impl.go b/spinifex/handlers/ec2/snapshot/service_impl.go index be06563a4..9c0efae00 100644 --- a/spinifex/handlers/ec2/snapshot/service_impl.go +++ b/spinifex/handlers/ec2/snapshot/service_impl.go @@ -104,56 +104,69 @@ func NewSnapshotServiceImplWithStore(cfg *config.Config, store objectstore.Objec return svc } -// getSnapshotKey returns the S3 key for storing snapshot metadata. -// Uses metadata.json to avoid conflicting with viperblock's config.json -// which stores the SnapshotState (block map references, source volume, etc). -func getSnapshotKey(snapshotID string) string { +// 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) } -// getSnapshotConfig retrieves snapshot config from S3 -func (s *SnapshotServiceImpl) getSnapshotConfig(snapshotID string) (*SnapshotConfig, error) { - key := getSnapshotKey(snapshotID) +// ErrCorruptSnapshotMetadata lets callers distinguish a missing snapshot from +// one whose metadata.json can't be parsed. +var ErrCorruptSnapshotMetadata = errors.New("corrupt snapshot metadata") - result, err := s.store.GetObject(&s3.GetObjectInput{ - Bucket: aws.String(s.config.Predastore.Bucket), +// ReadSnapshotConfig reads {snapshotID}/metadata.json. Object-store errors are +// returned unchanged; callers map NoSuchKey to their preferred AWS error. +// Decode failures wrap ErrCorruptSnapshotMetadata. +func ReadSnapshotConfig(store objectstore.ObjectStore, bucket, snapshotID string) (*SnapshotConfig, error) { + key := GetSnapshotKey(snapshotID) + result, err := store.GetObject(&s3.GetObjectInput{ + Bucket: aws.String(bucket), Key: aws.String(key), }) if err != nil { - if objectstore.IsNoSuchKeyError(err) { - return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) - } return nil, err } defer result.Body.Close() var cfg SnapshotConfig if err := json.NewDecoder(result.Body).Decode(&cfg); err != nil { - return nil, err + return nil, fmt.Errorf("%w: %v", ErrCorruptSnapshotMetadata, err) } - return &cfg, nil } -// putSnapshotConfig stores snapshot config to S3 -func (s *SnapshotServiceImpl) putSnapshotConfig(snapshotID string, cfg *SnapshotConfig) error { - key := getSnapshotKey(snapshotID) - +// 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 = s.store.PutObject(&s3.PutObjectInput{ - Bucket: aws.String(s.config.Predastore.Bucket), - Key: aws.String(key), + _, err = store.PutObject(&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(snapshotID string) (*SnapshotConfig, error) { + cfg, err := ReadSnapshotConfig(s.store, s.config.Predastore.Bucket, snapshotID) + if err != nil { + if objectstore.IsNoSuchKeyError(err) { + return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound) + } + return nil, 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 { snapshot := &ec2.Snapshot{