diff --git a/internal/api/types.go b/internal/api/types.go index 22ad6a8..eb9ba2c 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -16,8 +16,8 @@ type ShutdownResult struct { type VMCreateParams struct { Name string `json:"name,omitempty"` ImageName string `json:"image_name,omitempty"` - VCPUCount int `json:"vcpu_count,omitempty"` - MemoryMiB int `json:"memory_mib,omitempty"` + VCPUCount *int `json:"vcpu_count,omitempty"` + MemoryMiB *int `json:"memory_mib,omitempty"` SystemOverlaySize string `json:"system_overlay_size,omitempty"` WorkDiskSize string `json:"work_disk_size,omitempty"` NATEnabled bool `json:"nat_enabled,omitempty"` diff --git a/internal/cli/banger.go b/internal/cli/banger.go index 35a16e9..8eb9550 100644 --- a/internal/cli/banger.go +++ b/internal/cli/banger.go @@ -163,12 +163,25 @@ func newVMKillCommand() *cobra.Command { } func newVMCreateCommand() *cobra.Command { - var params api.VMCreateParams + var ( + name string + imageName string + vcpu int + memory int + systemOverlaySize string + workDiskSize string + natEnabled bool + noStart bool + ) cmd := &cobra.Command{ Use: "create", Short: "Create a VM", Args: noArgsUsage("usage: banger vm create"), RunE: func(cmd *cobra.Command, args []string) error { + params, err := vmCreateParamsFromFlags(cmd, name, imageName, vcpu, memory, systemOverlaySize, workDiskSize, natEnabled, noStart) + if err != nil { + return err + } if err := system.EnsureSudo(cmd.Context()); err != nil { return err } @@ -183,14 +196,14 @@ func newVMCreateCommand() *cobra.Command { return printVMSummary(cmd.OutOrStdout(), result.VM) }, } - cmd.Flags().StringVar(¶ms.Name, "name", "", "vm name") - cmd.Flags().StringVar(¶ms.ImageName, "image", "", "image name or id") - cmd.Flags().IntVar(¶ms.VCPUCount, "vcpu", 0, "vcpu count") - cmd.Flags().IntVar(¶ms.MemoryMiB, "memory", 0, "memory in MiB") - cmd.Flags().StringVar(¶ms.SystemOverlaySize, "system-overlay-size", "", "system overlay size") - cmd.Flags().StringVar(¶ms.WorkDiskSize, "disk-size", "", "work disk size") - cmd.Flags().BoolVar(¶ms.NATEnabled, "nat", false, "enable NAT") - cmd.Flags().BoolVar(¶ms.NoStart, "no-start", false, "create without starting") + cmd.Flags().StringVar(&name, "name", "", "vm name") + cmd.Flags().StringVar(&imageName, "image", "", "image name or id") + cmd.Flags().IntVar(&vcpu, "vcpu", 0, "vcpu count") + cmd.Flags().IntVar(&memory, "memory", 0, "memory in MiB") + cmd.Flags().StringVar(&systemOverlaySize, "system-overlay-size", "", "system overlay size") + cmd.Flags().StringVar(&workDiskSize, "disk-size", "", "work disk size") + cmd.Flags().BoolVar(&natEnabled, "nat", false, "enable NAT") + cmd.Flags().BoolVar(&noStart, "no-start", false, "create without starting") return cmd } @@ -648,9 +661,15 @@ func vmSetParamsFromFlags(idOrName string, vcpu, memory int, diskSize string, na } params := api.VMSetParams{IDOrName: idOrName, WorkDiskSize: diskSize} if vcpu >= 0 { + if err := validatePositiveSetting("vcpu", vcpu); err != nil { + return api.VMSetParams{}, err + } params.VCPUCount = &vcpu } if memory >= 0 { + if err := validatePositiveSetting("memory", memory); err != nil { + return api.VMSetParams{}, err + } params.MemoryMiB = &memory } if nat || noNat { @@ -663,6 +682,37 @@ func vmSetParamsFromFlags(idOrName string, vcpu, memory int, diskSize string, na return params, nil } +func vmCreateParamsFromFlags(cmd *cobra.Command, name, imageName string, vcpu, memory int, systemOverlaySize, workDiskSize string, natEnabled, noStart bool) (api.VMCreateParams, error) { + params := api.VMCreateParams{ + Name: name, + ImageName: imageName, + SystemOverlaySize: systemOverlaySize, + WorkDiskSize: workDiskSize, + NATEnabled: natEnabled, + NoStart: noStart, + } + if cmd.Flags().Changed("vcpu") { + if err := validatePositiveSetting("vcpu", vcpu); err != nil { + return api.VMCreateParams{}, err + } + params.VCPUCount = &vcpu + } + if cmd.Flags().Changed("memory") { + if err := validatePositiveSetting("memory", memory); err != nil { + return api.VMCreateParams{}, err + } + params.MemoryMiB = &memory + } + return params, nil +} + +func validatePositiveSetting(label string, value int) error { + if value <= 0 { + return fmt.Errorf("%s must be a positive integer", label) + } + return nil +} + func sshCommandArgs(cfg model.DaemonConfig, guestIP string, extra []string) ([]string, error) { if guestIP == "" { return nil, errors.New("vm has no guest IP") diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 6c7e5d7..6a25834 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -75,12 +75,65 @@ func TestVMSetParamsFromFlags(t *testing.T) { } } +func TestVMCreateParamsFromFlagsUsesNilForOmittedCPUAndMemory(t *testing.T) { + cmd := NewBangerCommand() + vm, _, err := cmd.Find([]string{"vm"}) + if err != nil { + t.Fatalf("find vm: %v", err) + } + create, _, err := vm.Find([]string{"create"}) + if err != nil { + t.Fatalf("find create: %v", err) + } + + params, err := vmCreateParamsFromFlags(create, "devbox", "default", 0, 0, "8G", "16G", false, false) + if err != nil { + t.Fatalf("vmCreateParamsFromFlags: %v", err) + } + if params.VCPUCount != nil || params.MemoryMiB != nil { + t.Fatalf("expected omitted cpu/memory to stay nil: %+v", params) + } +} + +func TestVMCreateParamsFromFlagsRejectsNonPositiveCPUAndMemory(t *testing.T) { + cmd := NewBangerCommand() + vm, _, err := cmd.Find([]string{"vm"}) + if err != nil { + t.Fatalf("find vm: %v", err) + } + create, _, err := vm.Find([]string{"create"}) + if err != nil { + t.Fatalf("find create: %v", err) + } + if err := create.Flags().Set("vcpu", "0"); err != nil { + t.Fatalf("set vcpu flag: %v", err) + } + if _, err := vmCreateParamsFromFlags(create, "devbox", "default", 0, 0, "", "", false, false); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("vmCreateParamsFromFlags(vcpu=0) error = %v", err) + } + if err := create.Flags().Set("memory", "-1"); err != nil { + t.Fatalf("set memory flag: %v", err) + } + if _, err := vmCreateParamsFromFlags(create, "devbox", "default", 1, -1, "", "", false, false); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("vmCreateParamsFromFlags(memory=-1) error = %v", err) + } +} + func TestVMSetParamsFromFlagsConflict(t *testing.T) { if _, err := vmSetParamsFromFlags("devbox", -1, -1, "", true, true); err == nil { t.Fatal("expected nat conflict error") } } +func TestVMSetParamsFromFlagsRejectsNonPositiveCPUAndMemory(t *testing.T) { + if _, err := vmSetParamsFromFlags("devbox", 0, -1, "", false, false); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("vmSetParamsFromFlags(vcpu=0) error = %v", err) + } + if _, err := vmSetParamsFromFlags("devbox", -1, 0, "", false, false); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("vmSetParamsFromFlags(memory=0) error = %v", err) + } +} + func TestSSHCommandArgs(t *testing.T) { args, err := sshCommandArgs(model.DaemonConfig{SSHKeyPath: "/bundle/id_ed25519"}, "172.16.0.2", []string{"--", "uname", "-a"}) if err != nil { diff --git a/internal/cli/tui.go b/internal/cli/tui.go index 6f6e1f8..67a9f13 100644 --- a/internal/cli/tui.go +++ b/internal/cli/tui.go @@ -399,8 +399,8 @@ func (f *vmForm) createRequest() (actionRequest, error) { params := api.VMCreateParams{ Name: strings.TrimSpace(f.fields[0].value()), ImageName: strings.TrimSpace(f.fields[1].value()), - VCPUCount: vcpu, - MemoryMiB: memory, + VCPUCount: &vcpu, + MemoryMiB: &memory, SystemOverlaySize: strings.TrimSpace(f.fields[4].value()), WorkDiskSize: strings.TrimSpace(f.fields[5].value()), NATEnabled: isYes(f.fields[6].value()), diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index 18e37f6..092c48f 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -25,7 +25,7 @@ func TestCreateVMFormSubmit(t *testing.T) { if action.create.Name != "devbox" || action.create.ImageName != "default" { t.Fatalf("unexpected create params: %+v", action.create) } - if action.create.VCPUCount != 4 || action.create.MemoryMiB != 2048 { + if action.create.VCPUCount == nil || *action.create.VCPUCount != 4 || action.create.MemoryMiB == nil || *action.create.MemoryMiB != 2048 { t.Fatalf("unexpected cpu/memory: %+v", action.create) } if action.create.SystemOverlaySize != "12G" || action.create.WorkDiskSize != "24G" { diff --git a/internal/daemon/vm.go b/internal/daemon/vm.go index 21f9b2d..27978be 100644 --- a/internal/daemon/vm.go +++ b/internal/daemon/vm.go @@ -28,6 +28,12 @@ func (d *Daemon) CreateVM(ctx context.Context, params api.VMCreateParams) (vm mo } op.done(vmLogAttrs(vm)...) }() + if err := validateOptionalPositiveSetting("vcpu", params.VCPUCount); err != nil { + return model.VMRecord{}, err + } + if err := validateOptionalPositiveSetting("memory", params.MemoryMiB); err != nil { + return model.VMRecord{}, err + } imageName := params.ImageName if imageName == "" { @@ -76,8 +82,8 @@ func (d *Daemon) CreateVM(ctx context.Context, params api.VMCreateParams) (vm mo } now := model.Now() spec := model.VMSpec{ - VCPUCount: defaultInt(params.VCPUCount, model.DefaultVCPUCount), - MemoryMiB: defaultInt(params.MemoryMiB, model.DefaultMemoryMiB), + VCPUCount: optionalIntOrDefault(params.VCPUCount, model.DefaultVCPUCount), + MemoryMiB: optionalIntOrDefault(params.MemoryMiB, model.DefaultMemoryMiB), SystemOverlaySizeByte: systemOverlaySize, WorkDiskSizeBytes: workDiskSize, NATEnabled: params.NATEnabled, @@ -462,6 +468,9 @@ func (d *Daemon) SetVM(ctx context.Context, params api.VMSetParams) (vm model.VM } running := vm.State == model.VMStateRunning && system.ProcessRunning(vm.Runtime.PID, vm.Runtime.APISockPath) if params.VCPUCount != nil { + if err := validateOptionalPositiveSetting("vcpu", params.VCPUCount); err != nil { + return model.VMRecord{}, err + } if running { return model.VMRecord{}, errors.New("vcpu changes require the VM to be stopped") } @@ -469,6 +478,9 @@ func (d *Daemon) SetVM(ctx context.Context, params api.VMSetParams) (vm model.VM vm.Spec.VCPUCount = *params.VCPUCount } if params.MemoryMiB != nil { + if err := validateOptionalPositiveSetting("memory", params.MemoryMiB); err != nil { + return model.VMRecord{}, err + } if running { return model.VMRecord{}, errors.New("memory changes require the VM to be stopped") } @@ -864,13 +876,23 @@ func bridgePrefix(bridgeIP string) string { return strings.Join(parts[:3], ".") } -func defaultInt(value, fallback int) int { - if value > 0 { - return value +func optionalIntOrDefault(value *int, fallback int) int { + if value != nil { + return *value } return fallback } +func validateOptionalPositiveSetting(label string, value *int) error { + if value == nil { + return nil + } + if *value <= 0 { + return fmt.Errorf("%s must be a positive integer", label) + } + return nil +} + func (d *Daemon) mapdnsBinary() string { if value := strings.TrimSpace(d.config.MapDNSBin); value != "" { return value diff --git a/internal/daemon/vm_test.go b/internal/daemon/vm_test.go index 9e0f9e9..098fc17 100644 --- a/internal/daemon/vm_test.go +++ b/internal/daemon/vm_test.go @@ -13,6 +13,7 @@ import ( "banger/internal/api" "banger/internal/model" + "banger/internal/paths" "banger/internal/store" ) @@ -218,6 +219,63 @@ func TestSetVMDiskResizeFailsPreflightWhenToolsMissing(t *testing.T) { } } +func TestCreateVMRejectsNonPositiveCPUAndMemory(t *testing.T) { + d := &Daemon{} + if _, err := d.CreateVM(context.Background(), api.VMCreateParams{VCPUCount: ptr(0)}); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("CreateVM(vcpu=0) error = %v", err) + } + if _, err := d.CreateVM(context.Background(), api.VMCreateParams{MemoryMiB: ptr(-1)}); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("CreateVM(memory=-1) error = %v", err) + } +} + +func TestCreateVMUsesDefaultsWhenCPUAndMemoryOmitted(t *testing.T) { + ctx := context.Background() + db := openDaemonStore(t) + image := testImage("default") + if err := db.UpsertImage(ctx, image); err != nil { + t.Fatalf("UpsertImage: %v", err) + } + d := &Daemon{ + store: db, + layout: paths.Layout{ + VMsDir: t.TempDir(), + }, + config: model.DaemonConfig{ + DefaultImageName: image.Name, + BridgeIP: model.DefaultBridgeIP, + }, + } + + vm, err := d.CreateVM(ctx, api.VMCreateParams{Name: "defaults", ImageName: image.Name, NoStart: true}) + if err != nil { + t.Fatalf("CreateVM: %v", err) + } + if vm.Spec.VCPUCount != model.DefaultVCPUCount { + t.Fatalf("VCPUCount = %d, want %d", vm.Spec.VCPUCount, model.DefaultVCPUCount) + } + if vm.Spec.MemoryMiB != model.DefaultMemoryMiB { + t.Fatalf("MemoryMiB = %d, want %d", vm.Spec.MemoryMiB, model.DefaultMemoryMiB) + } +} + +func TestSetVMRejectsNonPositiveCPUAndMemory(t *testing.T) { + ctx := context.Background() + db := openDaemonStore(t) + vm := testVM("validate", "image-validate", "172.16.0.13") + if err := db.UpsertVM(ctx, vm); err != nil { + t.Fatalf("UpsertVM: %v", err) + } + d := &Daemon{store: db} + + if _, err := d.SetVM(ctx, api.VMSetParams{IDOrName: vm.ID, VCPUCount: ptr(0)}); err == nil || !strings.Contains(err.Error(), "vcpu must be a positive integer") { + t.Fatalf("SetVM(vcpu=0) error = %v", err) + } + if _, err := d.SetVM(ctx, api.VMSetParams{IDOrName: vm.ID, MemoryMiB: ptr(0)}); err == nil || !strings.Contains(err.Error(), "memory must be a positive integer") { + t.Fatalf("SetVM(memory=0) error = %v", err) + } +} + func TestCollectStatsIgnoresMalformedMetricsFile(t *testing.T) { t.Parallel()