diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index f55c35a03..c4d37991b 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -17,6 +17,7 @@ along with this program. If not, see . package cmd import ( + "bufio" "bytes" "context" "crypto/tls" @@ -48,6 +49,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/gpu" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/mulgadc/spinifex/spinifex/objectstore" "github.com/mulgadc/spinifex/spinifex/utils" "github.com/mulgadc/viperblock/viperblock" "github.com/mulgadc/viperblock/viperblock/backends/s3" @@ -151,6 +153,19 @@ var imagesListCmd = &cobra.Command{ Run: runimagesListCmd, } +var imagesRemoveCmd = &cobra.Command{ + Use: "remove", + Short: "Remove an admin-imported system AMI", + Long: `Delete an AMI imported via 'spx admin images import', including its +backing block storage and snapshot artefacts. Only operates on AMIs with a +non-account owner (e.g. "system"); account-owned AMIs must be removed via +'aws ec2 deregister-image' followed by 'aws ec2 delete-snapshot'. + +Refuses to delete an AMI that has dependent volumes or copied snapshots/AMIs +unless --force is passed. Prompts for confirmation unless --yes is passed.`, + Run: runimagesRemoveCmd, +} + var accountCmd = &cobra.Command{ Use: "account", Short: "Manage Spinifex accounts", @@ -233,6 +248,7 @@ func init() { adminCmd.AddCommand(imagesCmd) imagesCmd.AddCommand(imagesImportCmd) imagesCmd.AddCommand(imagesListCmd) + imagesCmd.AddCommand(imagesRemoveCmd) adminCmd.AddCommand(accountCmd) accountCmd.AddCommand(accountCreateCmd) @@ -316,6 +332,13 @@ func init() { imagesImportCmd.Flags().StringSlice("tag", nil, "Tag to apply to the imported AMI as key=value (repeatable; e.g. --tag spinifex:managed-by=elbv2)") imagesImportCmd.Flags().Bool("force", false, "Force command execution (overwrites existing files)") imagesImportCmd.Flags().Bool("skip-verify", false, "Skip catalog-image checksum verification (INSECURE; operator assumes integrity responsibility)") + + imagesRemoveCmd.Flags().String("image-id", "", "AMI ID to remove (required)") + imagesRemoveCmd.Flags().Bool("force", false, "Bypass dependency, ownership and config-corrupt checks (salvage mode)") + imagesRemoveCmd.Flags().Bool("yes", false, "Skip interactive confirmation prompt") + if err := imagesRemoveCmd.MarkFlagRequired("image-id"); err != nil { + panic(err) + } } func runimagesImportCmd(cmd *cobra.Command, args []string) { @@ -602,6 +625,136 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) { fmt.Printf("✅ Image import complete. Image-ID (AMI): %s\n", volumeId) } +func runimagesRemoveCmd(cmd *cobra.Command, args []string) { + imageID, _ := cmd.Flags().GetString("image-id") + force, _ := cmd.Flags().GetBool("force") + yes, _ := cmd.Flags().GetBool("yes") + + cfgFile, _ := cmd.Flags().GetString("config") + if cfgFile == "" { + cfgFile = DefaultConfigFile() + } + + appConfig, err := config.LoadConfig(cfgFile) + if err != nil { + fmt.Fprintln(os.Stderr, "Error loading config file:", err) + os.Exit(1) + } + + node := appConfig.Nodes[appConfig.Node] + store := objectstore.NewS3ObjectStoreFromConfig( + node.Predastore.Host, + node.Predastore.Region, + node.Predastore.AccessKey, + node.Predastore.SecretKey, + ) + bucket := node.Predastore.Bucket + + preview, err := admin.PreviewRemoveSystemImage(store, bucket, imageID) + if err != nil { + fmt.Fprintln(os.Stderr, "Failed to inspect AMI:", err) + os.Exit(1) + } + + // Print metadata block. + fmt.Println("About to remove system AMI:") + fmt.Println() + fmt.Printf(" Image ID: %s\n", preview.ImageID) + switch { + case !preview.ConfigPresent && !preview.ConfigCorrupt: + fmt.Println(" Name: ") + fmt.Println(" Owner: ") + case preview.ConfigCorrupt: + fmt.Println(" Name: ") + fmt.Println(" Owner: ") + default: + fmt.Printf(" Name: %s\n", preview.Name) + fmt.Printf(" Owner: %s\n", preview.Owner) + if !preview.Created.IsZero() { + fmt.Printf(" Created: %s\n", preview.Created.UTC().Format("2006-01-02T15:04:05Z")) + } + } + fmt.Printf(" Backing storage: %s/ (%d objects, %s)\n", + preview.ImageID, preview.AMIObjectCount, utils.HumanBytes(utils.SafeInt64ToUint64(preview.AMIBytesTotal))) + fmt.Printf(" %s/ (%d objects, %s)\n", + admin.SnapPrefix(preview.ImageID), preview.SnapObjectCount, utils.HumanBytes(utils.SafeInt64ToUint64(preview.SnapBytesTotal))) + fmt.Println() + + // Account-owned guard before salvage / dependents — the AWS-flow hint is + // the most useful thing to surface for this kind of mistake. + if preview.ConfigPresent && !preview.IsSystemOwned && !force { + fmt.Fprintf(os.Stderr, + "Refusing to remove: %s is account-owned (%s).\n"+ + "Use 'aws ec2 deregister-image --image-id %s' followed by 'aws ec2 delete-snapshot ...'.\n", + preview.ImageID, preview.Owner, preview.ImageID) + os.Exit(1) + } + + if !preview.ConfigPresent && !force { + fmt.Fprintln(os.Stderr, "AMI config.json missing or corrupt; re-run with --force to salvage backing blocks.") + os.Exit(1) + } + + if !preview.Dependents.Empty() && !force { + fmt.Fprintln(os.Stderr, "Refusing to remove: dependent resources reference this image.") + printDependents(os.Stderr, preview.Dependents) + fmt.Fprintln(os.Stderr, "Remove them first (e.g. 'aws ec2 terminate-instances', 'aws ec2 delete-snapshot', 'aws ec2 deregister-image') or re-run with --force.") + os.Exit(1) + } + + if force && (!preview.ConfigPresent || !preview.Dependents.Empty()) { + fmt.Println("⚠️ --force: skipping dependency check and ownership check.") + if !preview.Dependents.Empty() { + printDependents(os.Stdout, preview.Dependents) + } + fmt.Println() + } + + fmt.Println("This is permanent and cannot be undone.") + if !yes { + fmt.Print("Type 'yes' to proceed: ") + reader := bufio.NewReader(os.Stdin) + answer, _ := reader.ReadString('\n') + if strings.TrimSpace(answer) != "yes" { + fmt.Println("Aborted.") + return + } + } + + res, err := admin.RemoveSystemImage(store, bucket, admin.RemoveImageOpts{ + ImageID: imageID, + Force: force, + }) + if err != nil { + fmt.Fprintln(os.Stderr, "Remove failed:", err) + os.Exit(1) + } + + fmt.Printf("✅ Removed AMI %s (freed %s across %d objects).\n", + imageID, utils.HumanBytes(utils.SafeInt64ToUint64(res.BytesFreed)), res.ObjectsDeleted) +} + +func printDependents(w io.Writer, d admin.Dependents) { + if len(d.Volumes) > 0 { + fmt.Fprintf(w, " Volumes (%d):\n", len(d.Volumes)) + for _, v := range d.Volumes { + fmt.Fprintf(w, " - %s\n", v) + } + } + if len(d.Snapshots) > 0 { + fmt.Fprintf(w, " Snapshots (%d):\n", len(d.Snapshots)) + for _, s := range d.Snapshots { + fmt.Fprintf(w, " - %s\n", s) + } + } + if len(d.AMIs) > 0 { + fmt.Fprintf(w, " AMIs (%d):\n", len(d.AMIs)) + for _, a := range d.AMIs { + fmt.Fprintf(w, " - %s\n", a) + } + } +} + // List remote images available func runimagesListCmd(cmd *cobra.Command, args []string) { //fmt.Println(availableImages) diff --git a/cmd/spinifex/cmd/admin_test.go b/cmd/spinifex/cmd/admin_test.go index 4089c2e25..9a5e9f0da 100644 --- a/cmd/spinifex/cmd/admin_test.go +++ b/cmd/spinifex/cmd/admin_test.go @@ -310,3 +310,24 @@ func TestWritePredastoreEncryptionKey_CreatesMissingDir(t *testing.T) { require.NoError(t, err) assert.True(t, dirInfo.IsDir()) } + +// TestImagesRemoveCmd_FlagSchema guards the public flag surface for +// `spx admin images remove`. --image-id is required so cobra rejects a bare +// invocation; --force/--yes default to false. +func TestImagesRemoveCmd_FlagSchema(t *testing.T) { + imageIDFlag := imagesRemoveCmd.Flags().Lookup("image-id") + require.NotNil(t, imageIDFlag, "--image-id must be defined") + assert.Equal(t, []string{"true"}, imageIDFlag.Annotations[cobraRequiredAnnotation], + "--image-id must be marked required") + + forceFlag := imagesRemoveCmd.Flags().Lookup("force") + require.NotNil(t, forceFlag) + assert.Equal(t, "false", forceFlag.DefValue) + + yesFlag := imagesRemoveCmd.Flags().Lookup("yes") + require.NotNil(t, yesFlag) + assert.Equal(t, "false", yesFlag.DefValue) +} + +// cobraRequiredAnnotation is the annotation key cobra uses to mark required flags. +const cobraRequiredAnnotation = "cobra_annotation_bash_completion_one_required_flag" diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 4d45dbd45..89952e1a0 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -91,6 +91,7 @@ Operational commands for inspecting cluster state. These fan out NATS requests t |---------|-------|---------------|-------------|------------|--------| | `spx admin images import` | `--name`, `--file`, `--distro`, `--version`, `--arch`, `--platform`, `--boot-mode` (bios/uefi/uefi-preferred), `--tag`, `--force`, `--skip-verify` | Cluster must be running; either `--name` (catalog download) or `--file` (operator-supplied media). `--file` mode additionally requires `--distro`, `--version`, `--arch`, `--boot-mode`. | Catalog imports (`--name`) download the image, fetch the catalog `Checksum` URL, verify the SHA-256/SHA-512 digest, and inherit `BootMode` from the catalog entry. `--boot-mode` overrides the catalog value when set. Mismatch fails closed; the cached file is left on disk and `--force` re-downloads. `--file` imports skip checksum verification (operator-supplied media is outside Spinifex's trust boundary, the skip is logged at INFO for audit) and require an explicit `--boot-mode` because there is no catalog metadata to inherit from. `--skip-verify` bypasses verification for catalog imports and emits a WARN slog + stderr notice; use only for debugging or when upstream mirrors are confirmed-broken. | 1. Import valid catalog image (verifies checksum)
2. Tampered cache hit fails with `ErrChecksumMismatch`
3. `--force` recovers after a mismatch
4. `--file` import skips verification
5. `--skip-verify` bypasses checksum with WARN
6. `--file` without `--boot-mode` rejected
7. `--boot-mode=invalid` rejected | **DONE** | | `spx admin images list` | — | None | Lists available OS images that can be imported or downloaded | 1. List available images | **DONE** | +| `spx admin images remove` | `--image-id` (required), `--force`, `--yes` | Cluster must be running; predastore reachable. Targets AMIs imported via `spx admin images import` (non-account `ImageOwnerAlias`, e.g. `"system"`). | Loads `ami-/config.json`, walks transitive dependents — copied snapshots whose `VolumeID == imageID`, volumes whose `SnapshotID` references the internal `snap-ami-` or any derived snap, and account AMIs created via `CopyImage` whose `SnapshotID` is a derived snap — then prompts (skipped with `--yes`) before deleting `ami-/config.json` (the DescribeImages barrier) followed by the rest of `ami-/` and `snap-ami-/`. Account-owned AMIs are refused with a hint pointing at `aws ec2 deregister-image` + `aws ec2 delete-snapshot`. `--force` bypasses the dependency, ownership and config-corrupt checks for salvage of orphaned blocks. | 1. Happy path: system AMI, no deps → deleted
2. Account-owned → refused with AWS-flow hint
3. Missing/corrupt config → `InvalidAMIID.NotFound` (salvageable with `--force`)
4. Dependent volume (direct or via copied snap) → refused
5. Dependent account AMI → refused
6. `--force` overrides dependents
7. Idempotent re-run after partial delete | **DONE** | #### Image integrity verification (CMMC SI.L1-3.14.2) @@ -123,6 +124,35 @@ served. A mirror compromise that swaps both image and sums file is not detected; closing that gap requires GPG signature verification of the sums file, deferred to a later phase. +#### `spx admin images remove` caveats + +Admin-imported AMIs (`ImageOwnerAlias = "system"`) live +under the `ami-/` S3 prefix and use a viperblock-internal snap checkpoint +at `snap-ami-/` — there is no `snap-/metadata.json`. The AWS handlers +(`DeregisterImage`, `DeleteSnapshot`) reject system owners with +`UnauthorizedOperation`, which is the right behaviour for tenant API callers +but leaves no AWS-flow path to reclaim space. `spx admin images remove` is +the admin-trust-boundary counterpart that performs the dependency walk and +hard-deletes the blocks directly against predastore. + +`CopyImage` of a system AMI is metadata-only: it writes a fresh +`snap-/metadata.json` whose `VolumeID` points at `ami-` and a new +`ami-/config.json` referencing that snap. Volumes launched from the +copied AMI read transitively from `ami-/chunks/...`. The remove command +walks this transitive set and refuses if anything references the target. + +**TOCTOU window:** between the safety scan and the `config.json` delete a +concurrent `RunInstances` against the AMI could create a new dependent +volume. The window is sub-second on a healthy cluster. The admin running +this command is expected to know the fleet's operational state; if the race +fires the result is a `vol-` with deleted backing blocks, recovered by +terminating the orphaned instance. + +**`--force` bypasses every safety check** (dependents, ownership, missing / +corrupt `config.json`). Use only for salvage of orphaned blocks. Running it +against a live system AMI corrupts every dependent volume on the next disk +read. + ### GPU Management | Command | Flags | Prerequisites | Basic Logic | Test Cases | Status | diff --git a/scripts/dev-install.sh b/scripts/dev-install.sh index f9ee66ea0..f3f221534 100755 --- a/scripts/dev-install.sh +++ b/scripts/dev-install.sh @@ -79,10 +79,6 @@ SPINIFEX_AZ="${SPINIFEX_AZ:-${SPINIFEX_REGION}a}" echo "=== Initializing (region=$SPINIFEX_REGION az=$SPINIFEX_AZ) ===" sudo spx admin init --force --region "$SPINIFEX_REGION" --az "$SPINIFEX_AZ" --node node1 --nodes 1 -echo "=== Installing CA certificate ===" -sudo cp /etc/spinifex/ca.pem /usr/local/share/ca-certificates/spinifex-ca.crt -sudo update-ca-certificates - echo "=== Starting services ===" sudo systemctl start spinifex.target diff --git a/spinifex/admin/images_remove.go b/spinifex/admin/images_remove.go new file mode 100644 index 000000000..ac12b08cb --- /dev/null +++ b/spinifex/admin/images_remove.go @@ -0,0 +1,479 @@ +package admin + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "strings" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image" + handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot" + "github.com/mulgadc/spinifex/spinifex/objectstore" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/mulgadc/viperblock/viperblock" +) + +// RemoveImageOpts configures RemoveSystemImage. +type RemoveImageOpts struct { + ImageID string + // Force bypasses the dependency check, the ownership-shape check, and + // the "config.json missing/corrupt" check. Salvage-mode lever. + Force bool +} + +// RemoveImageResult summarises what was deleted from object storage. +type RemoveImageResult struct { + ObjectsDeleted int + BytesFreed int64 +} + +// Dependents lists every resource that transitively backs an admin-imported +// AMI. Removing the AMI while any of these exist would corrupt the dependent. +type Dependents struct { + Snapshots []string // EC2 snapshots whose VolumeID == the AMI ID (i.e. CopyImage-derived snaps) + Volumes []string // Volumes whose SnapshotID is snap-ami- or a derived snap + AMIs []string // AMIs whose SnapshotID is a derived snap (CopyImage of a system AMI then re-copy) +} + +// Empty is true when nothing depends on the AMI. +func (d Dependents) Empty() bool { + return len(d.Snapshots) == 0 && len(d.Volumes) == 0 && len(d.AMIs) == 0 +} + +// RemovePreview captures everything the CLI needs to render the confirmation +// prompt and decide whether to proceed: AMI metadata (or zero values when the +// config is missing/corrupt), backing-storage byte counts, and the dependent +// resource set. PreviewRemoveSystemImage performs no deletions. +type RemovePreview struct { + ImageID string + Name string + Owner string + Created time.Time + ConfigPresent bool // false when ami-/config.json is missing + ConfigCorrupt bool // true when config.json exists but is undecodable + IsSystemOwned bool // ImageOwnerAlias is set and not an account ID + + AMIObjectCount int + AMIBytesTotal int64 + SnapObjectCount int + SnapBytesTotal int64 + + Dependents Dependents +} + +// SnapPrefix returns the viperblock-internal snapshot prefix backing an +// admin-imported AMI. v_utils.ImportDiskImage writes the snapshot as +// "snap-" where the volume name IS the AMI ID. +func SnapPrefix(imageID string) string { + return "snap-" + imageID +} + +// PreviewRemoveSystemImage walks the object store and gathers everything the +// caller needs to decide whether to remove an admin-imported AMI. It performs +// no mutations. Returned errors indicate unexpected I/O failures only — +// AMI-not-found, corrupt config, and account-owned AMIs are reflected in the +// preview fields, not as errors. +func PreviewRemoveSystemImage(store objectstore.ObjectStore, bucket, imageID string) (*RemovePreview, error) { + if !strings.HasPrefix(imageID, "ami-") { + return nil, errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + + preview := &RemovePreview{ImageID: imageID} + + meta, configErr := readAMIConfig(store, bucket, imageID) + switch { + case configErr == nil: + preview.ConfigPresent = true + preview.Name = meta.Name + preview.Owner = meta.ImageOwnerAlias + preview.Created = meta.CreationDate + preview.IsSystemOwned = meta.ImageOwnerAlias != "" && !utils.IsAccountID(meta.ImageOwnerAlias) + case objectstore.IsNoSuchKeyError(configErr): + // Config absent — salvage candidate. Leave ConfigPresent=false. + case errors.Is(configErr, handlers_ec2_image.ErrCorruptAMIConfig): + preview.ConfigCorrupt = true + default: + return nil, fmt.Errorf("preview: read AMI config: %w", configErr) + } + + amiCount, amiBytes, err := sumPrefix(store, bucket, imageID+"/") + if err != nil { + return nil, fmt.Errorf("preview: sum ami prefix: %w", err) + } + preview.AMIObjectCount = amiCount + preview.AMIBytesTotal = amiBytes + + snapCount, snapBytes, err := sumPrefix(store, bucket, SnapPrefix(imageID)+"/") + if err != nil { + return nil, fmt.Errorf("preview: sum snap prefix: %w", err) + } + preview.SnapObjectCount = snapCount + preview.SnapBytesTotal = snapBytes + + deps, err := FindAMIDependents(store, bucket, imageID) + if err != nil { + return nil, fmt.Errorf("preview: find dependents: %w", err) + } + preview.Dependents = deps + + return preview, nil +} + +// FindAMIDependents walks the bucket to find every resource that transitively +// depends on the given admin-imported AMI: +// +// 1. snap-*/metadata.json whose VolumeID == imageID → derivedSnaps +// (these are CopyImage-created snapshots that point back at the system AMI) +// 2. vol-*/config.json whose VolumeMetadata.SnapshotID ∈ {snap-ami-} ∪ derivedSnaps +// 3. ami-*/config.json whose AMIMetadata.SnapshotID ∈ derivedSnaps +// +// The walk terminates one hop deep. CopyImage of an account AMI uses its own +// VolumeID, not the system AMI's, so further recursion is not required today. +func FindAMIDependents(store objectstore.ObjectStore, bucket, imageID string) (Dependents, error) { + var deps Dependents + + prefixes, err := listCommonPrefixes(store, bucket) + if err != nil { + return Dependents{}, fmt.Errorf("list bucket prefixes: %w", err) + } + + // Pass 1: collect derived snaps (CopyImage of this AMI writes a snap whose + // VolumeID points back at the source AMI ID). + derived := map[string]bool{} + for _, p := range prefixes { + if !strings.HasPrefix(p, "snap-") { + continue + } + snapID := strings.TrimSuffix(p, "/") + // Skip the viperblock-internal snap prefix for this AMI — it has no + // metadata.json (it was written by viperblock.CreateSnapshot, not by + // the EC2 snapshot service). + if snapID == SnapPrefix(imageID) { + continue + } + cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(store, bucket, snapID) + if err != nil { + if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) { + continue + } + return Dependents{}, fmt.Errorf("read snapshot %s: %w", snapID, err) + } + if cfg.VolumeID == imageID { + derived[snapID] = true + deps.Snapshots = append(deps.Snapshots, snapID) + } + } + + // The set of snap IDs that a dependent volume might reference: the + // admin-import's internal snap plus every CopyImage-derived snap. + volSnapRefs := map[string]bool{SnapPrefix(imageID): true} + for s := range derived { + volSnapRefs[s] = true + } + + // Pass 2: volumes (vol-*/config.json) whose SnapshotID matches. + for _, p := range prefixes { + if !strings.HasPrefix(p, "vol-") { + continue + } + volID := strings.TrimSuffix(p, "/") + cfg, err := readVolumeConfig(store, bucket, volID) + if err != nil { + if objectstore.IsNoSuchKeyError(err) || errors.Is(err, errCorruptVolumeConfig) { + continue + } + return Dependents{}, fmt.Errorf("read volume %s: %w", volID, err) + } + if volSnapRefs[cfg.VolumeMetadata.SnapshotID] { + deps.Volumes = append(deps.Volumes, volID) + } + } + + // Pass 3: AMIs (ami-*/config.json) whose SnapshotID is a derived snap. + // Skip the target AMI itself. + for _, p := range prefixes { + if !strings.HasPrefix(p, "ami-") { + continue + } + otherAMI := strings.TrimSuffix(p, "/") + if otherAMI == imageID { + continue + } + meta, err := readAMIConfig(store, bucket, otherAMI) + if err != nil { + if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_image.ErrCorruptAMIConfig) { + continue + } + return Dependents{}, fmt.Errorf("read AMI %s: %w", otherAMI, err) + } + if derived[meta.SnapshotID] { + deps.AMIs = append(deps.AMIs, otherAMI) + } + } + + return deps, nil +} + +// RemoveSystemImage deletes an admin-imported AMI and its backing storage +// after re-validating the preconditions: +// +// 1. ami-/config.json exists and parses (skipped under --force = salvage) +// 2. AMI is system-owned, not account-owned (--force bypasses) +// 3. No volumes / snapshots / AMIs depend on it (--force bypasses) +// +// Deletion order is intentional: the config.json is deleted FIRST so the AMI +// disappears from DescribeImages immediately, preventing new RunInstances +// from racing the block cleanup. +func RemoveSystemImage(store objectstore.ObjectStore, bucket string, opts RemoveImageOpts) (*RemoveImageResult, error) { + if !strings.HasPrefix(opts.ImageID, "ami-") { + return nil, errors.New(awserrors.ErrorInvalidAMIIDMalformed) + } + + meta, configErr := readAMIConfig(store, bucket, opts.ImageID) + configMissing := objectstore.IsNoSuchKeyError(configErr) + configCorrupt := errors.Is(configErr, handlers_ec2_image.ErrCorruptAMIConfig) + switch { + case configErr == nil: + // fine + case configMissing, configCorrupt: + if !opts.Force { + return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound) + } + default: + slog.Error("RemoveSystemImage: read AMI config", "imageId", opts.ImageID, "err", configErr) + return nil, errors.New(awserrors.ErrorServerInternal) + } + + if configErr == nil && !opts.Force { + if meta.ImageOwnerAlias != "" && utils.IsAccountID(meta.ImageOwnerAlias) { + return nil, fmt.Errorf("%s is account-owned (%s); use `aws ec2 deregister-image` followed by `aws ec2 delete-snapshot`", + opts.ImageID, meta.ImageOwnerAlias) + } + } + + if !opts.Force { + deps, err := FindAMIDependents(store, bucket, opts.ImageID) + if err != nil { + slog.Error("RemoveSystemImage: dependency walk", "imageId", opts.ImageID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + if !deps.Empty() { + return nil, &DependentError{ImageID: opts.ImageID, Dependents: deps} + } + } + + result := &RemoveImageResult{} + + // Step 1: drop config.json first — the barrier that hides the AMI from + // DescribeImages so no new launches can land on the blocks we're deleting. + if configErr == nil || (opts.Force && !configMissing) { + n, b, err := deletePrefix(store, bucket, opts.ImageID+"/config.json") + if err != nil { + return nil, fmt.Errorf("delete config: %w", err) + } + result.ObjectsDeleted += n + result.BytesFreed += b + } + + // Step 2: drop the rest of ami-/ (chunks, WAL, checkpoints). + n, b, err := deletePrefix(store, bucket, opts.ImageID+"/") + if err != nil { + return nil, fmt.Errorf("delete ami prefix: %w", err) + } + result.ObjectsDeleted += n + result.BytesFreed += b + + // Step 3: drop snap-/ (the viperblock-internal snap checkpoint). + n, b, err = deletePrefix(store, bucket, SnapPrefix(opts.ImageID)+"/") + if err != nil { + return nil, fmt.Errorf("delete snap prefix: %w", err) + } + result.ObjectsDeleted += n + result.BytesFreed += b + + slog.Info("RemoveSystemImage completed", + "imageId", opts.ImageID, + "objectsDeleted", result.ObjectsDeleted, + "bytesFreed", result.BytesFreed, + "force", opts.Force, + ) + return result, nil +} + +// DependentError is returned by RemoveSystemImage when dependent resources +// block deletion. The CLI prints the dependents list and exits non-zero. +type DependentError struct { + ImageID string + Dependents Dependents +} + +func (e *DependentError) Error() string { + return fmt.Sprintf("refusing to remove %s: %d volumes, %d snapshots, %d AMIs depend on this image", + e.ImageID, len(e.Dependents.Volumes), len(e.Dependents.Snapshots), len(e.Dependents.AMIs)) +} + +// readAMIConfig reads ami-/config.json and returns the AMIMetadata. +// Mirrors ImageServiceImpl.GetAMIConfig but operates package-locally so the +// admin tooling doesn't require an ImageServiceImpl (which carries NATS). +func readAMIConfig(store objectstore.ObjectStore, bucket, imageID string) (viperblock.AMIMetadata, error) { + key := imageID + "/config.json" + res, err := store.GetObject(&s3.GetObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + }) + if err != nil { + return viperblock.AMIMetadata{}, err + } + defer res.Body.Close() + + body, err := io.ReadAll(res.Body) + if err != nil { + return viperblock.AMIMetadata{}, err + } + + var state viperblock.VBState + if err := json.Unmarshal(body, &state); err != nil { + return viperblock.AMIMetadata{}, fmt.Errorf("%w: %s: %v", handlers_ec2_image.ErrCorruptAMIConfig, key, err) + } + return state.VolumeConfig.AMIMetadata, nil +} + +// errCorruptVolumeConfig lets the dependency walk distinguish a vol-*/config.json +// that can't be parsed (recovery is operator-owned, walk continues) from a +// transient transport error (walk must fail closed — silently dropping a +// dependent could lead to deleting blocks underneath a live volume). +var errCorruptVolumeConfig = errors.New("corrupt volume config") + +// readVolumeConfig reads vol-/config.json into VolumeConfig. +type volumeConfigWrapper struct { + VolumeConfig viperblock.VolumeConfig `json:"VolumeConfig"` +} + +func readVolumeConfig(store objectstore.ObjectStore, bucket, volumeID string) (*viperblock.VolumeConfig, error) { + key := volumeID + "/config.json" + res, err := store.GetObject(&s3.GetObjectInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + }) + if err != nil { + return nil, err + } + defer res.Body.Close() + + body, err := io.ReadAll(res.Body) + if err != nil { + return nil, err + } + + var w volumeConfigWrapper + if err := json.Unmarshal(body, &w); err != nil { + return nil, fmt.Errorf("%w: %s: %v", errCorruptVolumeConfig, key, err) + } + return &w.VolumeConfig, nil +} + +// listCommonPrefixes returns the top-level "directory" prefixes in the bucket +// (e.g. "ami-xxx/", "vol-yyy/", "snap-zzz/"), exhausting any pagination. +func listCommonPrefixes(store objectstore.ObjectStore, bucket string) ([]string, error) { + seen := map[string]bool{} + var token *string + for { + out, err := store.ListObjectsV2(&s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + Delimiter: aws.String("/"), + ContinuationToken: token, + }) + if err != nil { + return nil, err + } + for _, cp := range out.CommonPrefixes { + if cp.Prefix == nil { + continue + } + seen[*cp.Prefix] = true + } + if !aws.BoolValue(out.IsTruncated) { + break + } + token = out.NextContinuationToken + } + prefixes := make([]string, 0, len(seen)) + for p := range seen { + prefixes = append(prefixes, p) + } + return prefixes, nil +} + +// sumPrefix returns (object count, total bytes) for every object under prefix. +// Used by the preview to surface what an operator is about to delete. +func sumPrefix(store objectstore.ObjectStore, bucket, prefix string) (int, int64, error) { + var count int + var bytes int64 + var token *string + for { + out, err := store.ListObjectsV2(&s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + Prefix: aws.String(prefix), + ContinuationToken: token, + }) + if err != nil { + return 0, 0, err + } + for _, obj := range out.Contents { + count++ + if obj.Size != nil { + bytes += *obj.Size + } + } + if !aws.BoolValue(out.IsTruncated) { + break + } + token = out.NextContinuationToken + } + return count, bytes, nil +} + +// deletePrefix removes every object under prefix and returns count+bytes +// deleted. Used by RemoveSystemImage for both the single-key config.json +// barrier and the bulk ami-/ and snap-/ teardown. +func deletePrefix(store objectstore.ObjectStore, bucket, prefix string) (int, int64, error) { + var count int + var bytes int64 + var token *string + for { + out, err := store.ListObjectsV2(&s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + Prefix: aws.String(prefix), + ContinuationToken: token, + }) + if err != nil { + return count, bytes, err + } + for _, obj := range out.Contents { + if obj.Key == nil { + continue + } + if _, err := store.DeleteObject(&s3.DeleteObjectInput{ + Bucket: aws.String(bucket), + Key: obj.Key, + }); err != nil { + return count, bytes, fmt.Errorf("delete %s: %w", *obj.Key, err) + } + count++ + if obj.Size != nil { + bytes += *obj.Size + } + } + if !aws.BoolValue(out.IsTruncated) { + break + } + token = out.NextContinuationToken + } + return count, bytes, nil +} diff --git a/spinifex/admin/images_remove_test.go b/spinifex/admin/images_remove_test.go new file mode 100644 index 000000000..ceeebb203 --- /dev/null +++ b/spinifex/admin/images_remove_test.go @@ -0,0 +1,369 @@ +package admin + +import ( + "bytes" + "encoding/json" + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + awss3 "github.com/aws/aws-sdk-go/service/s3" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot" + "github.com/mulgadc/spinifex/spinifex/objectstore" + "github.com/mulgadc/viperblock/viperblock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testRemoveBucket = "test-bucket" + testRemoveAccountID = "000000000001" +) + +// putAMI writes an ami-/config.json with the given owner. Owner "" produces +// a corrupt config (empty ImageOwnerAlias is treated as corrupt by readers). +func putAMI(t *testing.T, store *objectstore.MemoryObjectStore, imageID, name, owner, snapshotID string) { + t.Helper() + state := viperblock.VBState{ + VolumeConfig: viperblock.VolumeConfig{ + AMIMetadata: viperblock.AMIMetadata{ + ImageID: imageID, + Name: name, + ImageOwnerAlias: owner, + SnapshotID: snapshotID, + VolumeSizeGiB: 8, + }, + }, + } + data, err := json.Marshal(state) + require.NoError(t, err) + _, err = store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(imageID + "/config.json"), + Body: bytes.NewReader(data), + }) + require.NoError(t, err) +} + +// putAMIBlocks writes a few dummy chunk objects under ami-/ so deletion +// has bulk work and byte counts to verify. +func putAMIBlocks(t *testing.T, store *objectstore.MemoryObjectStore, imageID string, n int, size int) { + t.Helper() + body := bytes.Repeat([]byte{0xab}, size) + for i := range n { + _, err := store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(imageID + "/chunks/" + string(rune('a'+i)) + ".dat"), + Body: bytes.NewReader(body), + }) + require.NoError(t, err) + } +} + +func putSnapBlocks(t *testing.T, store *objectstore.MemoryObjectStore, snapID string, n int, size int) { + t.Helper() + body := bytes.Repeat([]byte{0xcd}, size) + for i := range n { + _, err := store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(snapID + "/cp/" + string(rune('a'+i)) + ".bin"), + Body: bytes.NewReader(body), + }) + require.NoError(t, err) + } +} + +// putSnapMetadata writes a CopyImage-derived EC2 snapshot metadata that points +// VolumeID back at an admin-imported AMI's ID. +func putSnapMetadata(t *testing.T, store *objectstore.MemoryObjectStore, snapID, volumeID string) { + t.Helper() + cfg := &handlers_ec2_snapshot.SnapshotConfig{ + SnapshotID: snapID, + VolumeID: volumeID, + VolumeSize: 8, + State: "completed", + } + require.NoError(t, handlers_ec2_snapshot.WriteSnapshotConfig(store, testRemoveBucket, snapID, cfg)) +} + +// putVolume writes vol-/config.json with the given SnapshotID reference. +func putVolume(t *testing.T, store *objectstore.MemoryObjectStore, volID, snapshotID string) { + t.Helper() + wrapper := volumeConfigWrapper{ + VolumeConfig: viperblock.VolumeConfig{ + VolumeMetadata: viperblock.VolumeMetadata{ + VolumeID: volID, + SnapshotID: snapshotID, + SizeGiB: 8, + }, + }, + } + data, err := json.Marshal(wrapper) + require.NoError(t, err) + _, err = store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(volID + "/config.json"), + Body: bytes.NewReader(data), + }) + require.NoError(t, err) +} + +func TestRemoveSystemImage_HappyPath(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-sys-deb13" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + putAMIBlocks(t, store, id, 3, 128) + putSnapBlocks(t, store, SnapPrefix(id), 2, 16) + + preview, err := PreviewRemoveSystemImage(store, testRemoveBucket, id) + require.NoError(t, err) + assert.True(t, preview.ConfigPresent) + assert.True(t, preview.IsSystemOwned) + assert.Equal(t, "system", preview.Owner) + // config.json + 3 chunks = 4 ami-prefix objects. + assert.Equal(t, 4, preview.AMIObjectCount) + assert.Equal(t, 2, preview.SnapObjectCount) + assert.True(t, preview.Dependents.Empty()) + + res, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.NoError(t, err) + assert.Equal(t, preview.AMIObjectCount+preview.SnapObjectCount, res.ObjectsDeleted) + assert.Equal(t, preview.AMIBytesTotal+preview.SnapBytesTotal, res.BytesFreed) + assert.Equal(t, 0, store.Count()) +} + +func TestRemoveSystemImage_AccountOwned_Refused(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-acct-001" + putAMI(t, store, id, "user-ami", testRemoveAccountID, "snap-acct-001") + putAMIBlocks(t, store, id, 1, 16) + + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + assert.Contains(t, err.Error(), id) + assert.Contains(t, err.Error(), testRemoveAccountID) + assert.Contains(t, err.Error(), "account-owned") + assert.Contains(t, err.Error(), "aws ec2 deregister-image") + // Objects untouched. + assert.Equal(t, 2, store.Count()) +} + +func TestRemoveSystemImage_AccountOwned_ForceBypasses(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-acct-002" + putAMI(t, store, id, "user-ami", testRemoveAccountID, "snap-acct-002") + putAMIBlocks(t, store, id, 2, 16) + + res, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id, Force: true}) + require.NoError(t, err) + assert.Greater(t, res.ObjectsDeleted, 0) + assert.Equal(t, 0, store.Count()) +} + +func TestRemoveSystemImage_MissingConfig_NotFound(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-missing" + putAMIBlocks(t, store, id, 1, 16) + + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestRemoveSystemImage_CorruptConfig_NotFound(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-corrupt" + _, err := store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(id + "/config.json"), + Body: bytes.NewReader([]byte("{not valid json")), + }) + require.NoError(t, err) + + _, err = RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) +} + +func TestRemoveSystemImage_DependentVolume_Direct_Refused(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-sys-deb13" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + // Direct dependent: a volume launched from the admin import. + putVolume(t, store, "vol-aaa", SnapPrefix(id)) + + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + var depErr *DependentError + require.True(t, errors.As(err, &depErr)) + assert.Contains(t, depErr.Dependents.Volumes, "vol-aaa") +} + +func TestRemoveSystemImage_DependentVolume_Transitive_Refused(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-sys-deb13" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + + // CopyImage of the system AMI wrote a snap whose VolumeID points back at the AMI ID. + const derivedSnap = "snap-derived-001" + putSnapMetadata(t, store, derivedSnap, id) + // A volume launched from the copied AMI references the derived snap. + putVolume(t, store, "vol-bbb", derivedSnap) + + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + var depErr *DependentError + require.True(t, errors.As(err, &depErr)) + assert.Contains(t, depErr.Dependents.Volumes, "vol-bbb") + assert.Contains(t, depErr.Dependents.Snapshots, derivedSnap) +} + +func TestRemoveSystemImage_DependentAMI_Refused(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-sys-deb13" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + + const derivedSnap = "snap-derived-002" + putSnapMetadata(t, store, derivedSnap, id) + // Account AMI created via CopyImage; its SnapshotID is the derived snap. + putAMI(t, store, "ami-acct-002", "copied", testRemoveAccountID, derivedSnap) + + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + var depErr *DependentError + require.True(t, errors.As(err, &depErr)) + assert.Contains(t, depErr.Dependents.AMIs, "ami-acct-002") +} + +func TestRemoveSystemImage_Force_OverridesDependents(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-sys-deb13" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + putAMIBlocks(t, store, id, 2, 64) + putVolume(t, store, "vol-orphan", SnapPrefix(id)) + + res, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id, Force: true}) + require.NoError(t, err) + assert.Greater(t, res.ObjectsDeleted, 0) + // vol-orphan/config.json remains; the AMI is gone. + _, err = store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(id + "/config.json"), + }) + require.Error(t, err) + _, err = store.GetObject(&awss3.GetObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String("vol-orphan/config.json"), + }) + require.NoError(t, err) +} + +func TestRemoveSystemImage_Salvage_MissingConfig_ForceCleans(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-salvage-1" + // No config.json — just orphaned blocks. + putAMIBlocks(t, store, id, 3, 32) + putSnapBlocks(t, store, SnapPrefix(id), 1, 8) + + // Without --force: NotFound. + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) + + // With --force: salvage proceeds. + res, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id, Force: true}) + require.NoError(t, err) + assert.Equal(t, 4, res.ObjectsDeleted) + assert.Equal(t, 0, store.Count()) +} + +func TestRemoveSystemImage_Salvage_CorruptConfig_ForceCleans(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-salvage-2" + _, err := store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(id + "/config.json"), + Body: bytes.NewReader([]byte("garbage")), + }) + require.NoError(t, err) + putAMIBlocks(t, store, id, 1, 8) + + // Without --force. + _, err = RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) + + // With --force: corrupt config + blocks all go. + res, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id, Force: true}) + require.NoError(t, err) + assert.Equal(t, 2, res.ObjectsDeleted) + assert.Equal(t, 0, store.Count()) +} + +func TestRemoveSystemImage_IdempotentRerun(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-rerun" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + putAMIBlocks(t, store, id, 2, 16) + + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.NoError(t, err) + require.Equal(t, 0, store.Count()) + + // Second call without --force should report NotFound. + _, err = RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error()) + + // Salvage re-run is a no-op. + res, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: id, Force: true}) + require.NoError(t, err) + assert.Equal(t, 0, res.ObjectsDeleted) +} + +func TestRemoveSystemImage_MalformedID(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + _, err := RemoveSystemImage(store, testRemoveBucket, RemoveImageOpts{ImageID: "vol-wrong"}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAMIIDMalformed, err.Error()) +} + +func TestPreviewRemoveSystemImage_Salvage(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-salvage-preview" + putAMIBlocks(t, store, id, 2, 4) + + preview, err := PreviewRemoveSystemImage(store, testRemoveBucket, id) + require.NoError(t, err) + assert.False(t, preview.ConfigPresent) + assert.False(t, preview.ConfigCorrupt) + assert.Equal(t, 2, preview.AMIObjectCount) + assert.Equal(t, int64(8), preview.AMIBytesTotal) +} + +func TestPreviewRemoveSystemImage_CorruptConfig(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-corrupt-preview" + _, err := store.PutObject(&awss3.PutObjectInput{ + Bucket: aws.String(testRemoveBucket), + Key: aws.String(id + "/config.json"), + Body: bytes.NewReader([]byte("{nope")), + }) + require.NoError(t, err) + + preview, err := PreviewRemoveSystemImage(store, testRemoveBucket, id) + require.NoError(t, err) + assert.False(t, preview.ConfigPresent) + assert.True(t, preview.ConfigCorrupt) +} + +func TestFindAMIDependents_SkipsTargetAMI(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + const id = "ami-sys-only" + putAMI(t, store, id, "debian-13", "system", SnapPrefix(id)) + + deps, err := FindAMIDependents(store, testRemoveBucket, id) + require.NoError(t, err) + assert.True(t, deps.Empty()) +} diff --git a/spinifex/utils/utils.go b/spinifex/utils/utils.go index f655ee12a..8f7e095dc 100644 --- a/spinifex/utils/utils.go +++ b/spinifex/utils/utils.go @@ -270,7 +270,7 @@ func DownloadFileWithProgress(url string, name string, filename string, timeout var written int64 reader := io.TeeReader(resp.Body, progressWriter(func(n int) { written += int64(n) - spin.UpdateText(fmt.Sprintf("Downloading %s (%s) ...", name, humanBytes(SafeInt64ToUint64(written)))) + spin.UpdateText(fmt.Sprintf("Downloading %s (%s) ...", name, HumanBytes(SafeInt64ToUint64(written)))) })) _, err = io.Copy(f, reader) _ = spin.Stop() @@ -291,7 +291,9 @@ func (pw progressWriter) Write(p []byte) (int, error) { return len(p), nil } -func humanBytes(b uint64) string { +// HumanBytes formats a byte count using IEC binary suffixes (KiB, MiB, ...). +// Values below 1024 render as exact bytes. +func HumanBytes(b uint64) string { const unit = 1024 if b < unit { return fmt.Sprintf("%d B", b) diff --git a/spinifex/utils/utils_test.go b/spinifex/utils/utils_test.go index 8cc7465cf..851cfdf6f 100644 --- a/spinifex/utils/utils_test.go +++ b/spinifex/utils/utils_test.go @@ -831,7 +831,7 @@ func TestHumanBytes(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, humanBytes(tt.b)) + assert.Equal(t, tt.want, HumanBytes(tt.b)) }) } }