diff --git a/CONTROLLER.md b/CONTROLLER.md index 23c6f3c..f0eb157 100644 --- a/CONTROLLER.md +++ b/CONTROLLER.md @@ -20,10 +20,21 @@ in. Any change requires re-opening the discussion. gap-filling. - **Comparison:** loose. Only manifest-specified fields are tracked. Provider-set defaults are unmanaged. +- **VM observation:** Get and List read authoritative Proxmox configuration, + including configured CPU/RAM, disks, NICs, agent, native cloud-init settings, + firmware, EFI, and PCI passthrough. Configuration-read/decoding failures are + errors, not incomplete observations; numeric JSON strings are accepted. +- **VM comparison:** declared disk/NIC slots are matched by name; undeclared + slots remain unmanaged. Equivalent binary disk-size units and MAC address + casing compare equal. Creation/routing inputs (`context`, `template`, + `image`, `cloudImage`, `startOnCreate`) and unobservable cloud-init inputs + (`password`, `packages`, `runcmd`) are excluded from live drift, not echoed + into observations. They remain in desired manifests and desired-to-desired + dry-run diffs; changing creation/first-boot inputs requires replacement. - **Apply on existing atomic resource (e.g. `VirtualMachine`):** update in place for the fields the provider can change live — for `VirtualMachine`: memory, CPU (cores/sockets), and disk **growth**. Non-resizable differences - (template, networks, cloud-init) are not mutated; they surface as drift and + (networks, native cloud-init settings) are not mutated; they surface as drift and still require delete + re-apply. Disk **shrink** is rejected (Proxmox can't shrink) — delete + re-apply to size down. Re-applying an unchanged spec is a no-op (identical config, disks already at size). @@ -362,9 +373,12 @@ destructive guardrails enforced. `internal/controller/server/drift.go` walks the desired spec and surfaces only the keys where desired ≠ observed; provider-set defaults are unmanaged. -- [x] VirtualMachine drift on Get: works generically via the loose - comparison (proxmox `VMToResource` returns spec keys that overlap - with the manifest, e.g. `cpu.cores`, `memory.size`). +- [x] VirtualMachine drift on Get/List/Watch and periodic reconciliation uses + the same provider-owned comparison projection over native configuration. + Missing declared settings still drift; only explicit creation-only, + routing, write-only, and first-boot execution inputs are excluded. + Failed configuration reads surface as errors, including name-scoped + Watch (an outage must not masquerade as deletion). - [x] Cluster structural drift on Get: the k3s provider synthesizes `spec.nodes.controlPlane.count` and each `spec.nodes.workers[*] .count` from the *actual* children list, so post-apply Gets reveal diff --git a/ROADMAP.md b/ROADMAP.md index 7c47a35..ba9b233 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -45,6 +45,14 @@ plugin protocol → Terraform/OpenTofu host → run-anywhere Linux daemon (OIDC slice) and CUE-WASM validation are parked behind that. (Mobile layout has since shipped.) +**Proxmox VM observation correction (2026-10-08):** Get/List now decode numeric +configuration strings and observe native managed settings instead of partial +runtime inventory. Get/List/Watch and reconciliation share provider-owned drift +semantics: declared disk/NIC slots matched by name, equivalent sizes normalized, +creation/write-only inputs excluded, and read outages reported rather than +mistaken for deletion. Root/plugin tests and a read-only homelab Get/List smoke +on `dev-system` pass with zero drift; no VM configuration was changed. + ## Homelab completion (COMPLETE — 2026-07-12) Backend-vs-vision gap audit for running the full homelab through openctl diff --git a/docs/homelab-verification.md b/docs/homelab-verification.md index 91cd2f2..ca752bb 100644 --- a/docs/homelab-verification.md +++ b/docs/homelab-verification.md @@ -22,12 +22,12 @@ sees every VM on the node (so the collision guard can't be fooled) and its `-o yaml` surfaces the **vmid** — the field that proves an in-place resize did not recreate the VM. -The verify VM is created **stopped** on purpose. A resize test doesn't need a -booted guest, it's faster, and it keeps memory observation reliable: `proxmox -get vms` reports a *running* VM's live `MaxMem`, which lags a config change until -reboot — so a running VM would show the old memory even though the config was -correctly updated. (This nuance was found on the first real-hardware run and is -why the harness now creates stopped.) +The verify VM is created **stopped** on purpose: booting a guest is unnecessary +for a resize test and makes the harness slower. Get/List now report configured +CPU/RAM from Proxmox's configuration API, even while a running guest still uses +its pre-reboot allocation. Guest-visible resources can therefore differ until +the guest applies the change or reboots; the configuration API is authoritative +for validating the resize. ## Safety diff --git a/internal/controller/manifests/git_test.go b/internal/controller/manifests/git_test.go index 3a90f56..a862e27 100644 --- a/internal/controller/manifests/git_test.go +++ b/internal/controller/manifests/git_test.go @@ -327,7 +327,9 @@ func TestRepoStartPeriodicPullReconcilesRemoteCommits(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git binary not on PATH; skipping git-dependent test") } - ctx := t.Context() // canceled when the test ends, stopping the pull goroutine + // T.Context cancels before cleanup; wait for any git command before canceling. + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) base := t.TempDir() bare := filepath.Join(base, "remote.git") @@ -362,6 +364,14 @@ func TestRepoStartPeriodicPullReconcilesRemoteCommits(t *testing.T) { t.Fatalf("NewRepo: %v", err) } + t.Cleanup(func() { + if err := repo.lock.acquire(context.Background()); err != nil { + t.Errorf("wait for repository operations: %v", err) + return + } + cancel() + repo.lock.release() + }) var mu sync.Mutex changes := 0 repo.StartPeriodicPull(ctx, 30*time.Millisecond, func(context.Context) error { diff --git a/internal/controller/operations/graph.go b/internal/controller/operations/graph.go index 81c4141..3553255 100644 --- a/internal/controller/operations/graph.go +++ b/internal/controller/operations/graph.go @@ -3,6 +3,7 @@ package operations import ( "context" "fmt" + "slices" "sort" "strings" ) @@ -115,7 +116,7 @@ func RunGraph(ctx context.Context, concurrency int, tasks []Task) error { continue // drain remaining in-flight, launch nothing new } - newlyReady := ready[:len(ready):len(ready)] + newlyReady := slices.Clip(ready) for _, dep := range dependents[res.id] { indegree[dep]-- if indegree[dep] == 0 { diff --git a/internal/controller/providers/provider.go b/internal/controller/providers/provider.go index 07eb7df..60c69a3 100644 --- a/internal/controller/providers/provider.go +++ b/internal/controller/providers/provider.go @@ -57,6 +57,23 @@ type ObservedOnly interface { ObservedOnlyKinds() []string } +// DriftProjector supplies provider-specific observation comparison semantics. +// Creation-only and write-only inputs cannot be compared to live state, and +// named collections may need alignment. Projections must not mutate either +// spec or substitute desired values for missing live configuration. +type DriftProjector interface { + DriftSpecs(kind string, desired, observed map[string]any) (map[string]any, map[string]any) +} + +// DriftSpecs projects desired-versus-live comparison inputs. Manifest editing +// and dry-run desired-versus-desired diffs must retain the original specs. +func DriftSpecs(p Provider, kind string, desired, observed map[string]any) (map[string]any, map[string]any) { + if projector, ok := p.(DriftProjector); ok { + return projector.DriftSpecs(kind, desired, observed) + } + return desired, observed +} + // DryRunner is an optional provider capability for previewing what an // Apply would do without performing it. Composite providers (k3s Cluster // → VMs) implement this to surface per-child actions and the gates the diff --git a/internal/controller/providers/proxmox/apply_observed_test.go b/internal/controller/providers/proxmox/apply_observed_test.go index d8bd816..5216d8c 100644 --- a/internal/controller/providers/proxmox/apply_observed_test.go +++ b/internal/controller/providers/proxmox/apply_observed_test.go @@ -16,10 +16,8 @@ import ( // nil result here regresses that contract (and would fail the shared // providertest conformance battery once proxmox is bound to it). // -// This drives the apply-on-existing path: the VM "vm0" already exists, so the -// handler's applyVM takes the no-op branch (returns observed state, no clone), -// and Apply then reads it back. A permissive catch-all keeps incidental calls (config, guest-agent -// IP) benign so the test asserts only the return-value contract. +// The desired manifest only specifies placement; the returned resource must +// still expose independently observed configured hardware. func TestApplyReturnsObservedVM(t *testing.T) { srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -28,6 +26,8 @@ func TestApplyReturnsObservedVM(t *testing.T) { _, _ = io.WriteString(w, `{"data":[{"node":"pve1","status":"online"}]}`) case "/api2/json/nodes/pve1/qemu": _, _ = io.WriteString(w, `{"data":[{"vmid":100,"name":"vm0","status":"running","node":"pve1","template":0}]}`) + case "/api2/json/nodes/pve1/qemu/100/config": + _, _ = io.WriteString(w, `{"data":{"cores":4,"sockets":1,"memory":"4096"}}`) default: // config / resize / agent-network-get / etc. — benign success so // the update path completes without hitting an unhandled route. @@ -52,7 +52,7 @@ func TestApplyReturnsObservedVM(t *testing.T) { if got.Kind != kindVM || got.Metadata.Name != "vm0" { t.Errorf("Apply returned %s/%s, want %s/vm0", got.Kind, got.Metadata.Name, kindVM) } - if got.APIVersion == "" { - t.Error("Apply result APIVersion is empty; providers must stamp it") + if got.Spec["cpu"].(map[string]any)["cores"] != 4 || got.Spec["memory"].(map[string]any)["size"] != 4096 { + t.Errorf("Apply returned incomplete or invented hardware: %v", got.Spec) } } diff --git a/internal/controller/providers/proxmox/conformance_test.go b/internal/controller/providers/proxmox/conformance_test.go index 0fafc39..538d593 100644 --- a/internal/controller/providers/proxmox/conformance_test.go +++ b/internal/controller/providers/proxmox/conformance_test.go @@ -59,6 +59,10 @@ func (f *fakeProxmox) newServer(t *testing.T) *httptest.Server { newid, _ := strconv.Atoi(r.Form.Get("newid")) f.vms[r.Form.Get("name")] = newid _ = json.NewEncoder(w).Encode(map[string]any{"data": ""}) + case strings.HasSuffix(path, "/config") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "cores": 1, "sockets": 1, "memory": "512", + }}) case r.Method == http.MethodDelete && strings.Contains(path, "/qemu/"): vmid, _ := strconv.Atoi(path[strings.LastIndex(path, "/")+1:]) for name, id := range f.vms { diff --git a/internal/controller/providers/proxmox/multicontext_test.go b/internal/controller/providers/proxmox/multicontext_test.go index 42d1a01..6a35eec 100644 --- a/internal/controller/providers/proxmox/multicontext_test.go +++ b/internal/controller/providers/proxmox/multicontext_test.go @@ -78,13 +78,15 @@ func TestApplyUnknownContextErrors(t *testing.T) { // configured endpoint, merged into one result set. func TestListMergesAcrossEndpoints(t *testing.T) { srvA := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pveA","status":"online"}]}`, - "/api2/json/nodes/pveA/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pveA","template":0}]}`, + "/api2/json/nodes": `{"data":[{"node":"pveA","status":"online"}]}`, + "/api2/json/nodes/pveA/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pveA","template":0}]}`, + "/api2/json/nodes/pveA/qemu/100/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srvA.Close() srvB := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, - "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, + "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes/pveB/qemu/200/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srvB.Close() @@ -115,8 +117,9 @@ func TestGetRoutesToOwningEndpoint(t *testing.T) { }) defer srvA.Close() srvB := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, - "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, + "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes/pveB/qemu/200/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srvB.Close() diff --git a/internal/controller/providers/proxmox/provider.go b/internal/controller/providers/proxmox/provider.go index 60d6932..f679d51 100644 --- a/internal/controller/providers/proxmox/provider.go +++ b/internal/controller/providers/proxmox/provider.go @@ -17,6 +17,7 @@ import ( "github.com/openctl/openctl/internal/controller/providers" "github.com/openctl/openctl/pkg/protocol" pmhandler "github.com/openctl/openctl/pkg/proxmox/handler" + pmresources "github.com/openctl/openctl/pkg/proxmox/resources" ) const ( @@ -147,6 +148,14 @@ func (p *Provider) Kinds() []string { return []string{kindVM, kindNode} } // API rather than provisioning them — they can never be in applied_manifests. func (p *Provider) ObservedOnlyKinds() []string { return []string{kindNode} } +// DriftSpecs compares observable VM settings, excluding creation-only inputs. +func (p *Provider) DriftSpecs(kind string, desired, observed map[string]any) (map[string]any, map[string]any) { + if kind == kindVM { + return pmresources.ComparableVMSpecs(desired, observed) + } + return desired, observed +} + // Actions implements providers.Actioner. VirtualMachine supports the // standard power-lifecycle set plus "console" (opens Proxmox noVNC in // a new tab); ProxmoxNode has no runtime actions. @@ -216,9 +225,8 @@ func (p *Provider) DoAction(ctx context.Context, kind, name, action string) (*pr } } -// Apply creates a VM if missing; otherwise returns the observed state -// without mutating (per the no-op-on-existing rule). ProxmoxNode is -// observed-only and rejects Apply. +// Apply creates a missing VM or resizes an existing VM's CPU, memory, and disks, +// then returns observed state. ProxmoxNode is observed-only and rejects Apply. func (p *Provider) Apply(ctx context.Context, manifest *protocol.Resource) (*protocol.Resource, error) { if manifest.Kind == kindNode { return nil, fmt.Errorf("%s is observed-only; cannot be applied", kindNode) diff --git a/internal/controller/providers/proxmox/provider_test.go b/internal/controller/providers/proxmox/provider_test.go index 9d09c83..202352b 100644 --- a/internal/controller/providers/proxmox/provider_test.go +++ b/internal/controller/providers/proxmox/provider_test.go @@ -130,6 +130,9 @@ func TestProviderChildrenOfNodeReturnsVMs(t *testing.T) { "/api2/json/nodes/pve2/qemu": `{"data":[ {"vmid":200,"name":"cache-01","status":"running","node":"pve2","template":0} ]}`, + "/api2/json/nodes/pve1/qemu/100/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, + "/api2/json/nodes/pve1/qemu/101/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, + "/api2/json/nodes/pve2/qemu/200/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srv.Close() @@ -161,8 +164,9 @@ func TestProviderChildrenOfNodeReturnsVMs(t *testing.T) { func TestProviderListReturnsVMs(t *testing.T) { srv := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pve1","status":"online"}]}`, - "/api2/json/nodes/pve1/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pve1","template":0},{"vmid":200,"name":"tpl-x","status":"stopped","node":"pve1","template":1}]}`, + "/api2/json/nodes": `{"data":[{"node":"pve1","status":"online"}]}`, + "/api2/json/nodes/pve1/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pve1","template":0},{"vmid":200,"name":"tpl-x","status":"stopped","node":"pve1","template":1}]}`, + "/api2/json/nodes/pve1/qemu/100/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srv.Close() diff --git a/internal/controller/reconciler/reconciler.go b/internal/controller/reconciler/reconciler.go index d6e2295..dff8d03 100644 --- a/internal/controller/reconciler/reconciler.go +++ b/internal/controller/reconciler/reconciler.go @@ -226,7 +226,8 @@ func (r *Reconciler) reconcileOne(ctx context.Context, ref manifests.Ref) { return } - drifted := !specsEqual(desired.Spec, observed.Spec) + desiredSpec, observedSpec := providers.DriftSpecs(p, ref.Kind, desired.Spec, observed.Spec) + drifted := !specsEqual(desiredSpec, observedSpec) reason := "" if drifted { reason = "spec drift" diff --git a/internal/controller/server/proxmox_drift_test.go b/internal/controller/server/proxmox_drift_test.go new file mode 100644 index 0000000..379eba7 --- /dev/null +++ b/internal/controller/server/proxmox_drift_test.go @@ -0,0 +1,175 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + "time" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/openctl/openctl/internal/controller/providers" + pmprovider "github.com/openctl/openctl/internal/controller/providers/proxmox" + "github.com/openctl/openctl/internal/controller/reconciler" + apiv1 "github.com/openctl/openctl/pkg/api/v1" + "github.com/openctl/openctl/pkg/protocol" +) + +// Exercise real Proxmox decoding and observation through the controller's +// consumers. Inventory deliberately differs from configured hardware, as it +// does after a running VM's configuration changes without a reboot. +func TestProxmoxDriftUsesLiveConfiguration(t *testing.T) { + var mu sync.Mutex + config := map[string]any{ + "name": "dev-system", "cores": 8, "sockets": "1", "memory": "10240", + "scsi0": "local-lvm:vm-101-disk-0,size=300G", + "ide2": "local-lvm:vm-101-cloudinit,media=cdrom,size=4M", + "net0": "virtio=BC:24:11:CC:5D:77,bridge=vmbr0", + "agent": "1", "ciuser": "ubuntu", "ipconfig0": "ip=dhcp", + "sshkeys": url.PathEscape("ssh-ed25519 AAAA development-key"), + } + configUnavailable := false + pve := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method != http.MethodGet { + t.Errorf("unexpected mutation: %s %s", req.Method, req.URL.Path) + http.Error(w, "read-only fixture", http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", "application/json") + switch req.URL.Path { + case "/api2/json/nodes": + _, _ = w.Write([]byte(`{"data":[{"node":"pve1"}]}`)) + case "/api2/json/nodes/pve1/qemu": + _, _ = w.Write([]byte(`{"data":[{"vmid":101,"name":"dev-system","status":"stopped","cpus":2,"maxmem":2147483648}]}`)) + case "/api2/json/nodes/pve1/qemu/101/config": + mu.Lock() + defer mu.Unlock() + if configUnavailable { + http.Error(w, "configuration unavailable", http.StatusServiceUnavailable) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": config}) + default: + t.Errorf("unexpected read: %s", req.URL.Path) + http.NotFound(w, req) + } + })) + defer pve.Close() + + reg := providers.NewRegistry() + reg.Register(pmprovider.New(&pmprovider.Config{Endpoint: pve.URL, TokenID: "test", TokenSecret: "test"})) + addr, mat, store := startTestServerWithManifests(t, reg) + conn := dialTestServer(t, addr, mat.CACertPath) + defer func() { _ = conn.Close() }() + desired := &protocol.Resource{ + APIVersion: "proxmox.openctl.io/v1", Kind: "VirtualMachine", + Metadata: protocol.ResourceMetadata{ + Name: "dev-system", Annotations: map[string]string{"openctl.io/autoReconcile": "true"}, + }, + Spec: map[string]any{ + "context": "homelab", "node": "pve1", "template": map[string]any{"name": "ubuntu"}, + "startOnCreate": true, + "cpu": map[string]any{"cores": 8, "sockets": 1}, "memory": map[string]any{"size": 10240}, + "disks": []any{map[string]any{"name": "scsi0", "storage": "local-lvm", "size": "307200M"}}, + "networks": []any{map[string]any{"name": "net0", "bridge": "vmbr0", "model": "virtio", "firewall": false}}, + "agent": map[string]any{"enabled": true}, + "cloudInit": map[string]any{ + "user": "ubuntu", "sshKeys": []any{"ssh-ed25519 AAAA development-key"}, + "ipConfig": map[string]any{"net0": map[string]any{"ip": "dhcp"}}, + "password": map[string]any{"$secret": "initial-password"}, + "packages": []any{"build-essential"}, "runcmd": []any{"true"}, + }, + }, + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := store.Save(ctx, desired); err != nil { + t.Fatal(err) + } + client := apiv1.NewResourceServiceClient(conn) + get := func() *apiv1.Resource { + t.Helper() + response, err := client.Get(ctx, &apiv1.GetRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind, Name: desired.Metadata.Name}) + if err != nil { + t.Fatal(err) + } + return response.GetResource() + } + list := func() *apiv1.Resource { + t.Helper() + response, err := client.List(ctx, &apiv1.ListRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind}) + if err != nil { + t.Fatal(err) + } + if len(response.GetResources()) != 1 { + t.Fatalf("managed inventory = %v, want dev-system only", response.GetResources()) + } + return response.GetResources()[0] + } + for _, read := range []func() *apiv1.Resource{get, list} { + if resource := read(); len(resource.GetDrift()) != 0 { + t.Fatalf("configured VM reported false drift: %v", resource.GetDrift()) + } + } + + applies := 0 + rec := reconciler.New(reg, store, 0).WithAutoApply(func(context.Context, *protocol.Resource) error { + applies++ + return nil + }) + rec.ReconcileOnce(ctx) + if applies != 0 { + t.Fatal("matching live config must not trigger automatic apply") + } + + stream, err := client.Watch(ctx, &apiv1.WatchRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind, Name: desired.Metadata.Name}) + if err != nil { + t.Fatal(err) + } + first, err := stream.Recv() + if err != nil || first.GetType() != apiv1.WatchEvent_ADDED || len(first.GetResource().GetDrift()) != 0 { + t.Fatalf("initial watch snapshot = %v, %v; want clean ADDED", first, err) + } + mu.Lock() + config["scsi0"] = "local-lvm:vm-101-disk-0,size=320G" + mu.Unlock() + changed, err := stream.Recv() + if err != nil || changed.GetType() != apiv1.WatchEvent_MODIFIED { + t.Fatalf("live disk change = %v, %v; want MODIFIED", changed, err) + } + for _, resource := range []*apiv1.Resource{changed.GetResource(), get(), list()} { + drift := resource.GetDrift() + if len(drift) != 1 || drift[0].GetPath() != "spec.disks[0].size" { + t.Fatalf("live disk change must remain visible: %v", drift) + } + } + rec.ReconcileOnce(ctx) + if applies != 1 { + t.Fatalf("actual live config drift triggered %d automatic applies, want 1", applies) + } + + // A failed configuration read is an outage, not missing hardware or a + // deleted VM. Get/List fail, reconciliation does not enqueue mutations, + // and a name-scoped Watch terminates as unavailable instead of DELETED. + mu.Lock() + configUnavailable = true + mu.Unlock() + if _, err := client.Get(ctx, &apiv1.GetRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind, Name: desired.Metadata.Name}); status.Code(err) != codes.Internal { + t.Fatalf("configuration outage on Get: %v", err) + } + if _, err := client.List(ctx, &apiv1.ListRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind}); status.Code(err) != codes.Internal { + t.Fatalf("configuration outage on List: %v", err) + } + rec.ReconcileOnce(ctx) + if applies != 1 { + t.Fatal("unavailable observations must not trigger automatic apply") + } + if event, err := stream.Recv(); status.Code(err) != codes.Unavailable { + t.Fatalf("configuration outage on Watch: event=%v error=%v; want Unavailable, never DELETED", event, err) + } +} diff --git a/internal/controller/server/resource.go b/internal/controller/server/resource.go index 2e28544..edc0aba 100644 --- a/internal/controller/server/resource.go +++ b/internal/controller/server/resource.go @@ -191,7 +191,8 @@ func (h *resourceHandler) Get(ctx context.Context, req *apiv1.GetRequest) (*apiv return nil, status.Errorf(codes.Internal, "load applied manifest: %v", lerr) } if desired != nil { - out.Drift = computeDrift(desired.Spec, r.Spec) + desiredSpec, observedSpec := providers.DriftSpecs(p, r.Kind, desired.Spec, r.Spec) + out.Drift = computeDrift(desiredSpec, observedSpec) applied, perr := resourceToProto(desired) if perr != nil { return nil, status.Errorf(codes.Internal, "encode applied: %v", perr) @@ -824,7 +825,12 @@ func (h *resourceHandler) attachDrift(ctx context.Context, out *apiv1.Resource, if desired == nil { return nil } - out.Drift = computeDrift(desired.Spec, observed.Spec) + p, err := h.registry.For(observed.APIVersion) + if err != nil { + return err + } + desiredSpec, observedSpec := providers.DriftSpecs(p, observed.Kind, desired.Spec, observed.Spec) + out.Drift = computeDrift(desiredSpec, observedSpec) return nil } diff --git a/internal/controller/server/watch.go b/internal/controller/server/watch.go index 461dfcc..8bf1311 100644 --- a/internal/controller/server/watch.go +++ b/internal/controller/server/watch.go @@ -11,6 +11,7 @@ import ( "github.com/openctl/openctl/internal/controller/manifests" "github.com/openctl/openctl/internal/controller/operations" + "github.com/openctl/openctl/internal/controller/providers" apiv1 "github.com/openctl/openctl/pkg/api/v1" "github.com/openctl/openctl/pkg/protocol" ) @@ -168,8 +169,11 @@ func (h *resourceHandler) listForWatch(ctx context.Context, apiVersion, kind, na if name != "" { r, err := p.Get(ctx, kind, name) if err != nil { - // NotFound is normal during watch — return empty, let DELETED fire. - return nil, nil // #nosec — intentional: surface as DELETED + var notFound *providers.NotFoundError + if errors.As(err, ¬Found) { + return nil, nil + } + return nil, err } if r == nil { return nil, nil diff --git a/pkg/proxmox/client/client.go b/pkg/proxmox/client/client.go index ba852cd..4919de1 100644 --- a/pkg/proxmox/client/client.go +++ b/pkg/proxmox/client/client.go @@ -15,6 +15,7 @@ import ( "net/url" "os" "sort" + "strconv" "strings" "time" @@ -232,18 +233,66 @@ type VM struct { // VMConfig represents VM configuration type VMConfig struct { - Name string `json:"name"` - Cores int `json:"cores"` - Sockets int `json:"sockets"` - Memory int `json:"memory"` - Boot string `json:"boot"` - OSType string `json:"ostype"` - SCSI0 string `json:"scsi0"` - Net0 string `json:"net0"` - IDE2 string `json:"ide2"` - IPConfig0 string `json:"ipconfig0"` - CIUser string `json:"ciuser"` - SSHKeys string `json:"sshkeys"` + Name string `json:"name"` + Cores int `json:"cores"` + Sockets int `json:"sockets"` + Memory int `json:"memory"` + Boot string `json:"boot"` + OSType string `json:"ostype"` + SCSI0 string `json:"scsi0"` + Net0 string `json:"net0"` + IDE2 string `json:"ide2"` + IPConfig0 string `json:"ipconfig0"` + CIUser string `json:"ciuser"` + SSHKeys string `json:"sshkeys"` + Raw map[string]any `json:"-"` +} + +// UnmarshalJSON preserves the full configuration while accepting Proxmox's +// integer fields as either JSON integers or decimal integer strings. +func (config *VMConfig) UnmarshalJSON(data []byte) error { + type plainVMConfig VMConfig + var decoded struct { + plainVMConfig + Cores json.RawMessage `json:"cores"` + Sockets json.RawMessage `json:"sockets"` + Memory json.RawMessage `json:"memory"` + } + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + if err := json.Unmarshal(data, &decoded.Raw); err != nil { + return err + } + if decoded.Raw == nil { + return errors.New("VM config must be a JSON object") + } + for _, field := range []struct { + name string + value json.RawMessage + dest *int + }{ + {"cores", decoded.Cores, &decoded.plainVMConfig.Cores}, + {"sockets", decoded.Sockets, &decoded.plainVMConfig.Sockets}, + {"memory", decoded.Memory, &decoded.plainVMConfig.Memory}, + } { + if len(field.value) == 0 { + continue + } + text := string(field.value) + if field.value[0] == '"' { + if err := json.Unmarshal(field.value, &text); err != nil { + return fmt.Errorf("invalid VM config %s: %w", field.name, err) + } + } + value, err := strconv.ParseInt(text, 10, strconv.IntSize) + if err != nil { + return fmt.Errorf("invalid VM config %s: %w", field.name, err) + } + *field.dest = int(value) + } + *config = VMConfig(decoded.plainVMConfig) + return nil } // Template represents a VM template @@ -312,13 +361,16 @@ func (c *Client) GetVMConfig(ctx context.Context, node string, vmid int) (*VMCon } var result struct { - Data VMConfig `json:"data"` + Data *VMConfig `json:"data"` } if err := json.Unmarshal(resp, &result); err != nil { return nil, fmt.Errorf("failed to parse VM config: %w", err) } - return &result.Data, nil + if result.Data == nil { + return nil, errors.New("missing VM config data") + } + return result.Data, nil } // CreateVM creates a new VM with the given parameters @@ -475,10 +527,8 @@ func (c *Client) ConfigureVM(ctx context.Context, node string, vmid int, params return err } -// GetVMConfigRaw returns the full VM config as a generic map. Unlike -// GetVMConfig (which uses a struct that only models a fixed subset of -// keys), this surfaces all disk and network slots that the VM actually -// has — necessary when merging flags into an arbitrary disk entry. +// GetVMConfigRaw returns the full VM config as a generic map without decoding +// typed fields, for callers merging flags into arbitrary disk entries. func (c *Client) GetVMConfigRaw(ctx context.Context, node string, vmid int) (map[string]any, error) { path := fmt.Sprintf("/api2/json/nodes/%s/qemu/%d/config", node, vmid) resp, err := c.get(ctx, path) diff --git a/pkg/proxmox/client/client_test.go b/pkg/proxmox/client/client_test.go index 6043561..c74d612 100644 --- a/pkg/proxmox/client/client_test.go +++ b/pkg/proxmox/client/client_test.go @@ -267,6 +267,99 @@ func TestGetVMConfig(t *testing.T) { } } +func TestGetVMConfigNumericCompatibility(t *testing.T) { + for _, numbers := range []string{ + `"cores":8,"sockets":1,"memory":10240`, + `"cores":"8","sockets":"1","memory":"10240"`, + `"cores":8,"sockets":1,"memory":"10240"`, + } { + t.Run(numbers, func(t *testing.T) { + server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api2/json/nodes/pve1/qemu/101/config" { + t.Errorf("unexpected request: %s", r.URL.Path) + http.NotFound(w, r) + return + } + _, _ = w.Write([]byte(`{"data":{` + numbers + `,"name":"dev-system","scsi1":"local-lvm:vm-101-disk-1,size=300G","net2":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","ipconfig2":"ip=dhcp","hostpci3":"0000:01:00.0","agent":"1"}}`)) + }) + defer server.Close() + c := New(server.URL, "test", "test") + c.httpClient = server.Client() + config, err := c.GetVMConfig(context.Background(), "pve1", 101) + if err != nil { + t.Fatalf("GetVMConfig: %v", err) + } + if config.Cores != 8 || config.Sockets != 1 || config.Memory != 10240 || config.Name != "dev-system" { + t.Fatalf("decoded config = %+v", config) + } + for key, want := range map[string]any{ + "scsi1": "local-lvm:vm-101-disk-1,size=300G", + "net2": "virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0", + "ipconfig2": "ip=dhcp", + "hostpci3": "0000:01:00.0", + "agent": "1", + } { + if got := config.Raw[key]; got != want { + t.Errorf("Raw[%q] = %v, want %v", key, got, want) + } + } + if len(config.Raw) != 9 { + t.Errorf("Raw has %d fields, want 9", len(config.Raw)) + } + encoded, err := json.Marshal(config) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(encoded), `"Raw"`) || strings.Contains(string(encoded), `"hostpci3"`) { + t.Errorf("Raw leaked into encoded typed config: %s", encoded) + } + }) + } +} + +func TestGetVMConfigRejectsInvalidNumbers(t *testing.T) { + for _, field := range []string{"cores", "sockets", "memory"} { + for _, value := range []string{ + `"bad"`, `""`, `"8.5"`, `8.5`, `8.0`, `1e3`, + `true`, `null`, `{}`, `[]`, `" 8"`, `"8 "`, + `9223372036854775808`, `"9223372036854775808"`, + `-9223372036854775809`, `"-9223372036854775809"`, + } { + t.Run(field+"/"+value, func(t *testing.T) { + server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":{"` + field + `":` + value + `}}`)) + }) + defer server.Close() + c := New(server.URL, "test", "test") + c.httpClient = server.Client() + config, err := c.GetVMConfig(context.Background(), "pve1", 101) + if err == nil || config != nil { + t.Fatalf("expected decoding failure, got config=%+v, err=%v", config, err) + } + if !strings.Contains(err.Error(), field) { + t.Errorf("error does not identify %s: %v", field, err) + } + }) + } + } +} + +func TestGetVMConfigRejectsMissingData(t *testing.T) { + for _, response := range []string{`{}`, `{"data":null}`, `{"data":[]}`, `{"data":""}`} { + t.Run(response, func(t *testing.T) { + server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(response)) + }) + defer server.Close() + c := New(server.URL, "test", "test") + c.httpClient = server.Client() + if config, err := c.GetVMConfig(context.Background(), "pve1", 101); err == nil || config != nil { + t.Fatalf("expected failure, got config=%+v, err=%v", config, err) + } + }) + } +} + func TestListTemplates(t *testing.T) { server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { diff --git a/pkg/proxmox/handler/handler.go b/pkg/proxmox/handler/handler.go index c531a60..80c46c5 100644 --- a/pkg/proxmox/handler/handler.go +++ b/pkg/proxmox/handler/handler.go @@ -208,7 +208,11 @@ func (h *Handler) listVMs(ctx context.Context) (*protocol.Response, error) { if vm.Template == 1 { continue } - result = append(result, resources.VMToResource(vm, nil)) + config, err := h.client.GetVMConfig(ctx, vm.Node, vm.VMID) + if err != nil { + return nil, fmt.Errorf("read VM %q config: %w", vm.Name, err) + } + result = append(result, resources.VMToResourceWithIP(vm, config, "")) } return &protocol.Response{ @@ -228,7 +232,10 @@ func (h *Handler) getVM(ctx context.Context, name string) (*protocol.Response, e return nil, err } - config, _ := h.client.GetVMConfig(ctx, vm.Node, vm.VMID) + config, err := h.client.GetVMConfig(ctx, vm.Node, vm.VMID) + if err != nil { + return nil, fmt.Errorf("read VM %q config: %w", vm.Name, err) + } // Try to get IP if VM is running (non-blocking) var ip string diff --git a/pkg/proxmox/handler/handler_test.go b/pkg/proxmox/handler/handler_test.go index 6f757d6..b197fec 100644 --- a/pkg/proxmox/handler/handler_test.go +++ b/pkg/proxmox/handler/handler_test.go @@ -767,3 +767,120 @@ func TestCreateVMFromImage_MissingFile(t *testing.T) { t.Errorf("expected error about missing file, got: %s", resp.Error.Message) } } + +func TestVMObservationUsesConfiguredHardware(t *testing.T) { + for _, action := range []string{protocol.ActionGet, protocol.ActionList} { + t.Run(action, func(t *testing.T) { + configReads, guestReads := 0, 0 + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api2/json/nodes": + _, _ = io.WriteString(w, `{"data":[{"node":"pve1"}]}`) + case "/api2/json/nodes/pve1/qemu": + // Runtime usage and vCPU count are deliberately different from config. + _, _ = io.WriteString(w, `{"data":[{"vmid":101,"name":"dev-system","status":"running","cpus":16,"maxmem":2147483648,"mem":1048576},{"vmid":9000,"name":"template","template":1}]}`) + case "/api2/json/nodes/pve1/qemu/101/config": + configReads++ + _, _ = io.WriteString(w, `{"data":{"cores":8,"sockets":"1","memory":"10240","scsi0":"local-lvm:vm-101-disk-0,size=300G","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","agent":"1","ciuser":"ubuntu","sshkeys":"ssh-ed25519%20AAAA%20test","ipconfig0":"ip=dhcp"}}`) + case "/api2/json/nodes/pve1/qemu/101/agent/network-get-interfaces": + guestReads++ + http.Error(w, "guest agent unavailable", http.StatusInternalServerError) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.NotFound(w, r) + } + })) + defer server.Close() + h := New(&protocol.ProviderConfig{Endpoint: server.URL, TokenID: "t", TokenSecret: "s"}) + resp, err := h.Handle(context.Background(), &protocol.Request{ + Action: action, ResourceType: "VirtualMachine", ResourceName: "dev-system", + }) + if err != nil { + t.Fatalf("Handle: %v", err) + } + if resp.Status != protocol.StatusSuccess { + t.Fatalf("expected success, got %+v", resp) + } + resource := resp.Resource + if action == protocol.ActionList { + if len(resp.Resources) != 1 { + t.Fatalf("listed %d resources, want one non-template VM", len(resp.Resources)) + } + resource = resp.Resources[0] + } + if resource == nil { + t.Fatal("missing observation") + } + cpu, ok := resource.Spec["cpu"].(map[string]any) + if !ok || cpu["cores"] != 8 || cpu["sockets"] != 1 { + t.Errorf("observed CPU = %v, want configured cores=8 sockets=1", resource.Spec["cpu"]) + } + memory, ok := resource.Spec["memory"].(map[string]any) + if !ok || memory["size"] != 10240 { + t.Errorf("observed memory = %v, want configured size=10240", resource.Spec["memory"]) + } + if configReads != 1 { + t.Errorf("config reads = %d, want exactly one", configReads) + } + if action == protocol.ActionList && guestReads != 0 { + t.Errorf("List made %d guest-agent calls, want zero", guestReads) + } + if _, ok := resource.Status["ip"]; ok { + t.Errorf("unexpected guest IP: %v", resource.Status["ip"]) + } + }) + } +} + +func TestVMObservationPropagatesConfigFailure(t *testing.T) { + for _, action := range []string{protocol.ActionGet, protocol.ActionList} { + for _, failure := range []string{"api", "decode"} { + t.Run(action+"/"+failure, func(t *testing.T) { + configReads, guestReads := 0, 0 + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api2/json/nodes": + _, _ = io.WriteString(w, `{"data":[{"node":"pve1"}]}`) + case "/api2/json/nodes/pve1/qemu": + _, _ = io.WriteString(w, `{"data":[{"vmid":100,"name":"healthy","status":"stopped"},{"vmid":101,"name":"dev-system","status":"running"}]}`) + case "/api2/json/nodes/pve1/qemu/100/config": + configReads++ + _, _ = io.WriteString(w, `{"data":{"cores":2,"sockets":1,"memory":2048}}`) + case "/api2/json/nodes/pve1/qemu/101/config": + configReads++ + if failure == "api" { + http.Error(w, "config unavailable", http.StatusInternalServerError) + } else { + _, _ = io.WriteString(w, `{"data":{"cores":8,"memory":"invalid"}}`) + } + default: + guestReads++ + t.Errorf("unexpected request after config failure: %s", r.URL.Path) + http.NotFound(w, r) + } + })) + defer server.Close() + h := New(&protocol.ProviderConfig{Endpoint: server.URL, TokenID: "t", TokenSecret: "s"}) + resp, err := h.Handle(context.Background(), &protocol.Request{ + Action: action, ResourceType: "VirtualMachine", ResourceName: "dev-system", + }) + if err == nil || resp != nil { + t.Fatalf("expected error and no partial observation, got resp=%+v, err=%v", resp, err) + } + if !strings.Contains(err.Error(), "dev-system") { + t.Errorf("error does not identify VM: %v", err) + } + wantReads := 1 + if action == protocol.ActionList { + wantReads = 2 + } + if configReads != wantReads { + t.Errorf("config reads = %d, want %d", configReads, wantReads) + } + if guestReads != 0 { + t.Errorf("guest reads after config failure = %d, want zero", guestReads) + } + }) + } + } +} diff --git a/pkg/proxmox/handler/resize.go b/pkg/proxmox/handler/resize.go index 1827e1a..24b7381 100644 --- a/pkg/proxmox/handler/resize.go +++ b/pkg/proxmox/handler/resize.go @@ -12,8 +12,9 @@ import ( // resizeVM applies in-place changes to an existing VM for the fields Proxmox // can update live: memory, CPU (cores/sockets), and disk growth. Other spec -// differences (template, networks, cloud-init) are NOT touched here — changing -// them still requires delete + re-apply, and they surface as drift via Get. +// differences in networks and native cloud-init settings are not applied here +// and remain visible as drift. Creation-only inputs are not live-state +// comparison targets; changing their effects requires replacement. // // This realizes the CONTROLLER.md decision to update atomic resources in place // for resizable fields rather than no-op. Idempotent: re-applying an unchanged diff --git a/pkg/proxmox/resources/vm.go b/pkg/proxmox/resources/vm.go index c86f099..a106f4d 100644 --- a/pkg/proxmox/resources/vm.go +++ b/pkg/proxmox/resources/vm.go @@ -21,13 +21,7 @@ func VMToResourceWithIP(vm *client.VM, config *client.VMConfig, ip string) *prot } if config != nil { - spec["cpu"] = map[string]any{ - "cores": config.Cores, - "sockets": config.Sockets, - } - spec["memory"] = map[string]any{ - "size": config.Memory, - } + observeVMConfig(spec, config) } else { spec["cpu"] = map[string]any{ "cores": vm.CPUs, diff --git a/pkg/proxmox/resources/vm_compare.go b/pkg/proxmox/resources/vm_compare.go new file mode 100644 index 0000000..3ff699a --- /dev/null +++ b/pkg/proxmox/resources/vm_compare.go @@ -0,0 +1,194 @@ +package resources + +import ( + "encoding/json" + "math/big" + "strconv" + "strings" +) + +// ComparableVMSpecs projects live configuration onto explicitly managed fields. +// Only creation/routing and unobservable cloud-init execution inputs are +// excluded. Unknown desired fields remain managed, so missing observations drift. +// Neither input is modified. +func ComparableVMSpecs(desired, observed map[string]any) (map[string]any, map[string]any) { + d, o := compareVMMaps(desired, observed, "") + return d, o +} + +func compareVMMaps(desired, observed map[string]any, path string) (map[string]any, map[string]any) { + d, o := make(map[string]any, len(desired)), make(map[string]any, len(desired)) + for key, value := range desired { + if path == "" { + switch key { + case "context", "template", "image", "cloudImage", "startOnCreate": + continue + } + } + if path == "cloudInit" { + switch key { + case "password", "packages", "runcmd": + continue + } + } + child := key + if path != "" { + child = path + "." + key + } + live, exists := observed[key] + if m, ok := value.(map[string]any); ok { + liveMap, _ := live.(map[string]any) + dm, om := compareVMMaps(m, liveMap, child) + // An execution-only cloud-init declaration manages no native state. + if child == "cloudInit" && len(dm) == 0 { + continue + } + d[key] = dm + if exists { + o[key] = om + } + continue + } + if list, ok := value.([]any); ok && (child == "disks" || child == "networks" || child == "hostPCI") { + liveList, _ := live.([]any) + dl, ol := compareVMLists(list, liveList, child) + d[key] = dl + if exists { + o[key] = ol + } + continue + } + if text, ok := value.(string); ok && live == text { + d[key], o[key] = value, live + continue + } + d[key] = normalizedVMValue(value, child) + if exists { + o[key] = normalizedVMValue(live, child) + } + } + return d, o +} + +func compareVMLists(desired, observed []any, path string) ([]any, []any) { + d, o := make([]any, 0, len(desired)), make([]any, 0, len(desired)) + for i, value := range desired { + m, ok := value.(map[string]any) + if !ok { + d = append(d, normalizedVMValue(value, path)) + if i < len(observed) { + o = append(o, normalizedVMValue(observed[i], path)) + } + continue + } + var live map[string]any + if path == "hostPCI" { + if i < len(observed) { + live, _ = observed[i].(map[string]any) + } + } else if name, ok := m["name"].(string); ok { + for _, candidate := range observed { + if candidateMap, ok := candidate.(map[string]any); ok && candidateMap["name"] == name { + live = candidateMap + break + } + } + } + dm, om := compareVMMaps(m, live, path) + d = append(d, dm) + if live != nil { + o = append(o, om) + } else { + o = append(o, nil) + } + } + return d, o +} + +func normalizedVMValue(value any, path string) any { + switch v := value.(type) { + case map[string]any: + m := make(map[string]any, len(v)) + for key, value := range v { + m[key] = normalizedVMValue(value, path+"."+key) + } + return m + case []any: + list := make([]any, len(v)) + for i, value := range v { + list[i] = normalizedVMValue(value, path) + } + return list + case []string: + list := make([]any, len(v)) + for i, value := range v { + list[i] = value + } + return list + case string: + if path == "disks.size" { + return binaryDiskSize(v) + } + if path == "networks.macAddress" { + return strings.ToLower(v) + } + return v + case int: + return float64(v) + case int64: + return float64(v) + case int32: + return float64(v) + case uint: + return float64(v) + case uint64: + return float64(v) + case float32: + return float64(v) + case json.Number: + if n, err := strconv.ParseFloat(string(v), 64); err == nil { + return n + } + } + return value +} + +// Proxmox disk sizes use binary units even for the G/T spellings. Rational +// arithmetic preserves fractional sizes without float rounding or overflow. +func binaryDiskSize(value string) string { + s := strings.ToUpper(strings.TrimSpace(value)) + i := 0 + for i < len(s) && ((s[i] >= '0' && s[i] <= '9') || s[i] == '.') { + i++ + } + if i == 0 { + return value + } + unit := s[i:] + power := 0 + switch unit { + case "", "B": + case "K", "KB", "KIB": + power = 1 + case "M", "MB", "MIB": + power = 2 + case "G", "GB", "GIB": + power = 3 + case "T", "TB", "TIB": + power = 4 + case "P", "PB", "PIB": + power = 5 + default: + return value + } + if whole, err := strconv.ParseUint(s[:i], 10, 64); err == nil && whole <= ^uint64(0)>>uint(10*power) { + return strconv.FormatUint(whole< 0 && key[i-1] >= '0' && key[i-1] <= '9' { + i-- + } + n, _ := strconv.Atoi(key[i:]) + return key[:i], n +} + +func numberedSlot(key string, prefixes ...string) bool { + prefix, _ := slotIndex(key) + if prefix == key { + return false + } + return slices.Contains(prefixes, prefix) +} diff --git a/pkg/proxmox/resources/vm_observation_test.go b/pkg/proxmox/resources/vm_observation_test.go new file mode 100644 index 0000000..b766ad3 --- /dev/null +++ b/pkg/proxmox/resources/vm_observation_test.go @@ -0,0 +1,144 @@ +package resources + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/openctl/openctl/pkg/proxmox/client" +) + +func observedConfig(t *testing.T, raw string) map[string]any { + t.Helper() + var config client.VMConfig + if err := json.Unmarshal([]byte(raw), &config); err != nil { + t.Fatal(err) + } + return VMToResourceWithIP(&client.VM{Name: "dev-system", Node: "pve", CPUs: 99, MaxMem: 1}, &config, "192.0.2.1").Spec +} + +func TestNativeVMConfigurationComparesWithoutCreationDrift(t *testing.T) { + observed := observedConfig(t, `{"cores":8,"sockets":1,"memory":"10240","scsi0":"local-lvm:vm-101-disk-0,size=300G","ide2":"local-lvm:vm-101-cloudinit,media=cdrom","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","agent":"1","ciuser":"ubuntu","sshkeys":"ssh-ed25519%20AAA+BBB%20user%40host%0A","ipconfig0":"ip=dhcp","cipassword":"SECRET"}`) + desired := map[string]any{ + "context": "lab", "template": map[string]any{"name": "ubuntu"}, "image": map[string]any{"file": "ubuntu.img"}, "cloudImage": map[string]any{"url": "https://example.test/image"}, "startOnCreate": true, + "node": "pve", "cpu": map[string]any{"cores": float64(8), "sockets": float64(1)}, "memory": map[string]any{"size": float64(10240)}, + "disks": []any{map[string]any{"name": "scsi0", "storage": "local-lvm", "size": "300GiB"}}, + "networks": []any{map[string]any{"name": "net0", "bridge": "vmbr0", "model": "virtio", "macAddress": "aa:bb:cc:dd:ee:ff", "firewall": false}}, + "agent": map[string]any{"enabled": true}, + "cloudInit": map[string]any{"user": "ubuntu", "sshKeys": []any{"ssh-ed25519 AAA+BBB user@host"}, "ipConfig": map[string]any{"net0": map[string]any{"ip": "dhcp"}}, "password": "SECRET", "packages": []any{"curl"}, "runcmd": []any{"echo hello"}}, + } + d, o := ComparableVMSpecs(desired, observed) + if !reflect.DeepEqual(d, o) { + t.Fatalf("unexpected drift: desired=%#v observed=%#v", d, o) + } + ci := observed["cloudInit"].(map[string]any) + if _, ok := ci["password"]; ok { + t.Fatal("write-only password exposed") + } + if len(observed["disks"].([]any)) != 1 { + t.Fatal("cloud-init CD-ROM exposed as data disk") + } + if _, ok := observed["template"]; ok { + t.Fatal("creation state exposed") + } + if desired["cloudInit"].(map[string]any)["password"] != "SECRET" { + t.Fatal("comparison mutated desired state") + } + if observed["cpu"].(map[string]any)["cores"] != 8 { + t.Fatal("comparison mutated observation") + } +} + +func TestNativeVMDriftRemainsVisible(t *testing.T) { + observed := observedConfig(t, `{"cores":"8","sockets":"1","memory":"10240","scsi0":"local-lvm:vm-101-disk-0,size=300G,backup=0,ssd=1,discard=on,iothread=1","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0,tag=20,firewall=1","agent":"enabled=1,fstrim_cloned_disks=1","ciuser":"ubuntu","ciupgrade":1,"ipconfig0":"ip=dhcp","sshkeys":"ssh-ed25519%20AAA+BBB"}`) + cases := []map[string]any{ + {"cpu": map[string]any{"cores": 4}}, {"cpu": map[string]any{"sockets": 2}}, {"cpu": map[string]any{"type": "host"}}, + {"memory": map[string]any{"size": 2048}}, {"agent": map[string]any{"enabled": false}}, + {"disks": []any{map[string]any{"name": "scsi0", "size": "301G"}}}, + {"disks": []any{map[string]any{"name": "scsi1", "size": "300G"}}}, + {"disks": []any{map[string]any{"name": "scsi0", "backup": true}}}, + {"disks": []any{map[string]any{"name": "scsi0", "ssd": false, "discard": false, "iothread": false}}}, + {"networks": []any{map[string]any{"name": "net0", "firewall": false}}}, + {"networks": []any{map[string]any{"name": "net0", "bridge": "vmbr1"}}}, + {"networks": []any{map[string]any{"name": "net0", "vlan": 21}}}, + {"networks": []any{map[string]any{"name": "net1", "bridge": "vmbr0"}}}, + {"cloudInit": map[string]any{"user": "root"}}, {"cloudInit": map[string]any{"packageUpgrade": false}}, + {"cloudInit": map[string]any{"sshKeys": []any{"ssh-ed25519 CHANGED"}}}, + {"cloudInit": map[string]any{"ipConfig": map[string]any{"net0": map[string]any{"ip": "192.0.2.5/24"}}}}, + {"cloudInit": map[string]any{"ipConfig": map[string]any{"net1": map[string]any{"ip": "dhcp"}}}}, + {"futureManagedOption": true}, {"cpu": map[string]any{"futureManagedOption": true}}, + } + for i, desired := range cases { + d, o := ComparableVMSpecs(desired, observed) + if reflect.DeepEqual(d, o) { + t.Errorf("case %d lost drift for %#v", i, desired) + } + } +} + +func TestVMNamedSlotsAndFractionalDiskSizes(t *testing.T) { + observed := observedConfig(t, `{"scsi12":"fast:vm-101-disk-12,size=1.5T","scsi2":"slow:vm-101-disk-2,size=1G","net12":"virtio=AA:BB:CC:DD:EE:12,bridge=vmbr12","net2":"virtio=AA:BB:CC:DD:EE:02,bridge=vmbr2","ipconfig12":"ip=192.0.2.12/24,gw=192.0.2.1","hostpci0":"mapping=gpu,pcie=1,x-vga=1,rombar=0,mdev=nvidia-123,romfile=gpu.rom","efidisk0":"fast:vm-101-disk-0,efitype=4m,pre-enrolled-keys=1","bios":"ovmf","machine":"q35","ostype":"l26","cpu":"host,flags=+aes","nameserver":"1.1.1.1 8.8.8.8","searchdomain":"example.test"}`) + desired := map[string]any{ + "disks": []any{map[string]any{"name": "scsi12", "size": "1536GiB"}, map[string]any{"name": "scsi2", "size": "1024M"}}, + "networks": []any{map[string]any{"name": "net12", "bridge": "vmbr12"}}, + "hostPCI": []any{map[string]any{"mapping": "gpu", "pcie": true, "primaryGPU": true, "rombar": false, "mdev": "nvidia-123", "romfile": "gpu.rom"}}, + "efiDisk": map[string]any{"storage": "fast", "type": "4m", "preEnrolledKeys": true}, + "bios": "ovmf", "machine": "q35", "osType": "l26", "cpu": map[string]any{"type": "host"}, + "cloudInit": map[string]any{"ipConfig": map[string]any{"net12": map[string]any{"ip": "192.0.2.12/24", "gateway": "192.0.2.1"}}, "searchDomain": "example.test", "nameservers": []any{"1.1.1.1", "8.8.8.8"}}, + } + d, o := ComparableVMSpecs(desired, observed) + if !reflect.DeepEqual(d, o) { + t.Fatalf("slot ordering or native observation caused drift: %#v != %#v", d, o) + } + for _, field := range []string{"bios", "machine", "osType", "hostPCI", "efiDisk"} { + missing := make(map[string]any, len(observed)) + for key, value := range observed { + if key != field { + missing[key] = value + } + } + d, o := ComparableVMSpecs(desired, missing) + if reflect.DeepEqual(d, o) { + t.Errorf("missing %s lost drift", field) + } + } +} + +func TestVMSSHKeyDecodingDoesNotEraseMalformedOrLiteralPlus(t *testing.T) { + for _, key := range []string{"ssh-ed25519 AAA+BBB", "ssh-ed25519 AAA%invalid"} { + raw, err := json.Marshal(map[string]any{"sshkeys": key}) + if err != nil { + t.Fatal(err) + } + observed := observedConfig(t, string(raw)) + keys := observed["cloudInit"].(map[string]any)["sshKeys"].([]any) + if len(keys) != 1 || keys[0] != key { + t.Fatalf("public key changed: %v", keys) + } + } +} + +func TestVMUnmanagedDefaultsAndExplicitFalseOptions(t *testing.T) { + observed := observedConfig(t, `{"scsi0":"local-lvm:vm-101-disk-0,size=8G","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","hostpci2":"host=0000:01:00.0"}`) + desired := map[string]any{ + "disks": []any{map[string]any{"name": "scsi0", "ssd": false, "discard": false, "iothread": false, "backup": true}}, + "networks": []any{map[string]any{"name": "net0", "firewall": false, "vlan": 0}}, + "agent": map[string]any{"enabled": false}, + } + d, o := ComparableVMSpecs(desired, observed) + if !reflect.DeepEqual(d, o) { + t.Fatalf("native default flags caused drift: %#v != %#v", d, o) + } + d, o = ComparableVMSpecs(map[string]any{"disks": []any{}, "networks": []any{}}, observed) + if !reflect.DeepEqual(d, o) { + t.Fatal("undeclared live members became managed") + } + d, o = ComparableVMSpecs(map[string]any{"hostPCI": []any{map[string]any{"device": "0000:01:00.0"}}}, observed) + if reflect.DeepEqual(d, o) { + t.Fatal("hostpci2 incorrectly satisfied desired hostpci0") + } + pci := observed["hostPCI"].([]any) + if len(pci) != 3 || pci[2].(map[string]any)["device"] != "0000:01:00.0" { + t.Fatal("numbered PCI slot was lost") + } +}