daemon: use exact-name lookup for VM-create uniqueness
reserveVM's duplicate-name guard routed through Daemon.FindVM, which falls back to prefix-matching on both ids and names when no exact match is found. That turns the uniqueness check into a correctness bug: a brand-new VM name can be rejected because it happens to prefix an existing VM's id, or an existing VM's name. So `vm create --name beta` fails when `beta-sandbox` already exists. Swap in a dedicated store.GetVMByName that does a literal `WHERE name = ?` lookup, and use it from reserveVM. FindVM keeps its prefix-matching behaviour for user-facing lookup paths (`vm ssh <partial>`, `vm stop <partial>`) where "did you mean" semantics are the feature. Tests: - TestReserveVMAllowsNameThatPrefixesExistingVM — seeds a VM whose id + name both start with "longname", then reserves two new VMs named "longname" and "longname-sandbox". Both must succeed. Under the old FindVM-based check, both would fail. - TestReserveVMRejectsExactDuplicateName — actual collisions are still rejected after the swap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
108f7a0600
commit
eba9a553bf
3 changed files with 105 additions and 2 deletions
|
|
@ -115,9 +115,12 @@ func (d *Daemon) reserveVM(ctx context.Context, requestedName string, image mode
|
|||
}
|
||||
name = generated
|
||||
}
|
||||
if _, err := d.FindVM(ctx, name); err == nil {
|
||||
// Exact-name lookup. Using FindVM here would also match a new name
|
||||
// that merely prefixes some existing VM's id or another VM's name,
|
||||
// falsely rejecting perfectly valid names.
|
||||
if _, err := d.store.GetVMByName(ctx, name); err == nil {
|
||||
return model.VMRecord{}, fmt.Errorf("vm name already exists: %s", name)
|
||||
} else if !errors.Is(err, sql.ErrNoRows) && !strings.Contains(err.Error(), "not found") {
|
||||
} else if !errors.Is(err, sql.ErrNoRows) {
|
||||
return model.VMRecord{}, err
|
||||
}
|
||||
|
||||
|
|
|
|||
86
internal/daemon/vm_create_test.go
Normal file
86
internal/daemon/vm_create_test.go
Normal file
|
|
@ -0,0 +1,86 @@
|
|||
package daemon
|
||||
|
||||
import (
|
||||
"context"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"banger/internal/model"
|
||||
"banger/internal/paths"
|
||||
)
|
||||
|
||||
// TestReserveVMAllowsNameThatPrefixesExistingVM is a regression for a
|
||||
// correctness bug in the name-uniqueness check: reserveVM used to
|
||||
// route through FindVM, which falls back to prefix-matching on both
|
||||
// ids and names. That meant a perfectly valid new name like "beta"
|
||||
// could be rejected simply because an existing VM's id or name
|
||||
// started with "beta". Exact-name lookup via store.GetVMByName fixes
|
||||
// it. The test seeds a VM whose id and name are long strings, then
|
||||
// tries to reserve a new VM with a name that's a prefix of each —
|
||||
// both must succeed.
|
||||
func TestReserveVMAllowsNameThatPrefixesExistingVM(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
tmp := t.TempDir()
|
||||
d := &Daemon{
|
||||
store: openDaemonStore(t),
|
||||
layout: paths.Layout{VMsDir: filepath.Join(tmp, "vms"), RuntimeDir: filepath.Join(tmp, "runtime")},
|
||||
config: model.DaemonConfig{BridgeIP: model.DefaultBridgeIP},
|
||||
}
|
||||
|
||||
existing := testVM("longname-sandbox-foobar", "image-x", "172.16.0.50")
|
||||
upsertDaemonVM(t, ctx, d.store, existing)
|
||||
|
||||
image := testImage("image-x")
|
||||
image.ID = "image-x"
|
||||
image.Name = "image-x"
|
||||
if err := d.store.UpsertImage(ctx, image); err != nil {
|
||||
t.Fatalf("UpsertImage: %v", err)
|
||||
}
|
||||
|
||||
// New VM name is a prefix of the existing id (which is
|
||||
// "longname-sandbox-foobar-id" per testVM). Old FindVM-based check
|
||||
// would reject this.
|
||||
if vm, err := d.reserveVM(ctx, "longname", image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128}); err != nil {
|
||||
t.Fatalf("reserveVM(prefix of id): %v", err)
|
||||
} else if vm.Name != "longname" {
|
||||
t.Fatalf("reserveVM returned name=%q, want longname", vm.Name)
|
||||
}
|
||||
|
||||
// Prefix of the existing name ("longname-sandbox") must also work.
|
||||
if vm, err := d.reserveVM(ctx, "longname-sandbox", image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128}); err != nil {
|
||||
t.Fatalf("reserveVM(prefix of name): %v", err)
|
||||
} else if vm.Name != "longname-sandbox" {
|
||||
t.Fatalf("reserveVM returned name=%q, want longname-sandbox", vm.Name)
|
||||
}
|
||||
}
|
||||
|
||||
// TestReserveVMRejectsExactDuplicateName confirms the uniqueness
|
||||
// check still catches actual collisions after the FindVM → GetVMByName
|
||||
// switch.
|
||||
func TestReserveVMRejectsExactDuplicateName(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
tmp := t.TempDir()
|
||||
d := &Daemon{
|
||||
store: openDaemonStore(t),
|
||||
layout: paths.Layout{VMsDir: filepath.Join(tmp, "vms"), RuntimeDir: filepath.Join(tmp, "runtime")},
|
||||
config: model.DaemonConfig{BridgeIP: model.DefaultBridgeIP},
|
||||
}
|
||||
existing := testVM("sandbox", "image-x", "172.16.0.51")
|
||||
upsertDaemonVM(t, ctx, d.store, existing)
|
||||
|
||||
image := testImage("image-x")
|
||||
image.ID = "image-x"
|
||||
image.Name = "image-x"
|
||||
if err := d.store.UpsertImage(ctx, image); err != nil {
|
||||
t.Fatalf("UpsertImage: %v", err)
|
||||
}
|
||||
|
||||
_, err := d.reserveVM(ctx, "sandbox", image, model.VMSpec{VCPUCount: 1, MemoryMiB: 128})
|
||||
if err == nil {
|
||||
t.Fatal("reserveVM with duplicate name should have failed")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "already exists") {
|
||||
t.Fatalf("err = %v, want 'already exists'", err)
|
||||
}
|
||||
}
|
||||
|
|
@ -203,6 +203,20 @@ func (s *Store) GetVMByID(ctx context.Context, id string) (model.VMRecord, error
|
|||
return scanVMRow(row)
|
||||
}
|
||||
|
||||
// GetVMByName is the exact-name lookup used for creation-time
|
||||
// uniqueness checks. Unlike GetVM (which matches id OR name) and
|
||||
// Daemon.FindVM (which also falls back to prefix-matching), this
|
||||
// returns sql.ErrNoRows for anything except a literal name hit, so
|
||||
// a new VM can't be rejected just because its name prefixes an
|
||||
// existing VM's id or an existing VM's name.
|
||||
func (s *Store) GetVMByName(ctx context.Context, name string) (model.VMRecord, error) {
|
||||
row := s.db.QueryRowContext(ctx, `
|
||||
SELECT id, name, image_id, guest_ip, state, created_at, updated_at, last_touched_at,
|
||||
spec_json, runtime_json, stats_json
|
||||
FROM vms WHERE name = ?`, name)
|
||||
return scanVMRow(row)
|
||||
}
|
||||
|
||||
func (s *Store) ListVMs(ctx context.Context) ([]model.VMRecord, error) {
|
||||
rows, err := s.db.QueryContext(ctx, `
|
||||
SELECT id, name, image_id, guest_ip, state, created_at, updated_at, last_touched_at,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue