From 92dc6df63cbfcb2f7f4edb569682f7f773cfd438 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:01:02 -0400 Subject: [PATCH 01/21] feat(drivers): add boolFromConfig and strSliceFromConfig helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both helpers are structpb-aware: boolFromConfig falls through to the default for non-bool values, and strSliceFromConfig accepts the typed []string shape Go-native callers emit AND the []any shape that survives a YAML/JSON → structpb round-trip. Empty strings drop silently. Used by the upcoming droplet/volume drivers; placed in util.go alongside the existing strFromConfig/intFromConfig helpers so all drivers can reach them without import cycles. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/util.go | 42 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/drivers/util.go b/internal/drivers/util.go index 2e2f78a..3b2aae7 100644 --- a/internal/drivers/util.go +++ b/internal/drivers/util.go @@ -27,6 +27,48 @@ func strFromConfig(config map[string]any, key, defaultVal string) string { return defaultVal } +// boolFromConfig extracts a boolean value from a config map. Returns +// defaultVal when the key is absent or holds a non-bool value. structpb +// preserves bool natively so no float64 fallback is needed here. +func boolFromConfig(config map[string]any, key string, defaultVal bool) bool { + if v, ok := config[key].(bool); ok { + return v + } + return defaultVal +} + +// strSliceFromConfig extracts a []string from a config map. Accepts either +// the typed []string shape (uncommon outside Go-native callers) or the +// []any shape that survives a structpb round-trip (the common case for +// values that originate in YAML/JSON). Non-string entries and empty +// strings are dropped silently — callers needing strict validation should +// re-check the result. +func strSliceFromConfig(config map[string]any, key string) []string { + v, ok := config[key] + if !ok { + return nil + } + switch t := v.(type) { + case []string: + out := make([]string, 0, len(t)) + for _, s := range t { + if s != "" { + out = append(out, s) + } + } + return out + case []any: + out := make([]string, 0, len(t)) + for _, e := range t { + if s, ok := e.(string); ok && s != "" { + out = append(out, s) + } + } + return out + } + return nil +} + // imageRepo returns the repository portion of a flat ":" image reference. func imageRepo(image string) string { parts := splitImageRef(image) From 5f980c4c4195cc1c6851bf19ec797f80edf8b701 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:01:18 -0400 Subject: [PATCH 02/21] feat(droplet): support user_data / vpc_uuid / ssh_keys / volumes / tags / bools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-hosted services (e.g. Apache-AGE Postgres on a Droplet, since DO Managed Postgres has no AGE extension) need more than the size/image/ region the DropletDriver previously honoured. New optional config keys, all additive and defaulted-empty (no behaviour change for existing callers): - user_data string — cloud-init payload - vpc_uuid string — VPC the Droplet joins - ssh_keys []string|[]int — fingerprints OR numeric IDs; element type detected at runtime (mixed lists OK; structpb floats must be whole numbers) - tags []string - enable_backups bool — godo Backups field - monitoring bool - ipv6 bool - volumes []string of — names resolved to IDs at create volume names time via Storage.ListVolumes(name=, region=). Region-bound: a name not present in the droplet's region returns "droplet volumes: volume %q not found". Outputs gain `private_ip` (godo droplet.PrivateIPv4()) so downstream services in the same VPC can be wired without a second Read. DropletDriver now holds an optional Storage client (set by NewDropletDriver from c.Storage; tests inject via the variadic NewDropletDriverWithClient param). When `volumes` is non-empty but the storage client is nil, Create returns an explicit error rather than silently dropping the attachment. mockDropletClient now captures the godo.DropletCreateRequest so tests can assert on populated struct fields. Adds focused tests for each new key, plus the SSH-keys fractional-float rejection path that prevents silent ID truncation. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 156 +++++++++++++++++-- internal/drivers/droplet_test.go | 254 ++++++++++++++++++++++++++++++- 2 files changed, 395 insertions(+), 15 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 780dff7..970430e 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -18,18 +18,26 @@ type DropletsClient interface { // DropletDriver manages DigitalOcean Droplets (infra.droplet). type DropletDriver struct { - client DropletsClient - region string + client DropletsClient + storage StorageClient // optional; required only when spec.Config["volumes"] is non-empty + region string } // NewDropletDriver creates a DropletDriver backed by a real godo client. +// The Storage client is wired so volumes-by-name resolution works. func NewDropletDriver(c *godo.Client, region string) *DropletDriver { - return &DropletDriver{client: c.Droplets, region: region} + return &DropletDriver{client: c.Droplets, storage: c.Storage, region: region} } // NewDropletDriverWithClient creates a driver with an injected client (for tests). -func NewDropletDriverWithClient(c DropletsClient, region string) *DropletDriver { - return &DropletDriver{client: c, region: region} +// The optional storage argument is used only when a spec references volumes by +// name; pass nil if your test does not exercise that path. +func NewDropletDriverWithClient(c DropletsClient, region string, storage ...StorageClient) *DropletDriver { + d := &DropletDriver{client: c, region: region} + if len(storage) > 0 { + d.storage = storage[0] + } + return d } func (d *DropletDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { @@ -38,11 +46,29 @@ func (d *DropletDriver) Create(ctx context.Context, spec interfaces.ResourceSpec region := strFromConfig(spec.Config, "region", d.region) req := &godo.DropletCreateRequest{ - Name: spec.Name, - Region: region, - Size: size, - Image: godo.DropletCreateImage{Slug: image}, + Name: spec.Name, + Region: region, + Size: size, + Image: godo.DropletCreateImage{Slug: image}, + UserData: strFromConfig(spec.Config, "user_data", ""), + VPCUUID: strFromConfig(spec.Config, "vpc_uuid", ""), + Tags: strSliceFromConfig(spec.Config, "tags"), + Backups: boolFromConfig(spec.Config, "enable_backups", false), + Monitoring: boolFromConfig(spec.Config, "monitoring", false), + IPv6: boolFromConfig(spec.Config, "ipv6", false), + } + + sshKeys, err := dropletSSHKeysFromConfig(spec.Config) + if err != nil { + return nil, fmt.Errorf("droplet create %q: %w", spec.Name, err) + } + req.SSHKeys = sshKeys + + volumes, err := d.resolveDropletVolumes(ctx, spec.Config, region) + if err != nil { + return nil, fmt.Errorf("droplet create %q: %w", spec.Name, err) } + req.Volumes = volumes droplet, _, err := d.client.Create(ctx, req) if err != nil { @@ -126,16 +152,21 @@ func dropletOutput(droplet *godo.Droplet) *interfaces.ResourceOutput { if ip, err := droplet.PublicIPv4(); err == nil { publicIP = ip } + var privateIP string + if ip, err := droplet.PrivateIPv4(); err == nil { + privateIP = ip + } return &interfaces.ResourceOutput{ Name: droplet.Name, Type: "infra.droplet", ProviderID: fmt.Sprintf("%d", droplet.ID), Outputs: map[string]any{ - "id": droplet.ID, - "public_ip": publicIP, - "size": droplet.Size.Slug, - "region": droplet.Region.Slug, - "status": droplet.Status, + "id": droplet.ID, + "public_ip": publicIP, + "private_ip": privateIP, + "size": droplet.Size.Slug, + "region": droplet.Region.Slug, + "status": droplet.Status, }, Status: droplet.Status, } @@ -153,6 +184,103 @@ func providerIDToInt(id string) (int, error) { return n, nil } +// dropletSSHKeysFromConfig converts the heterogeneous "ssh_keys" YAML value +// into a typed []godo.DropletCreateSSHKey. Each element may be either a +// fingerprint string (most common) or a numeric ID (int / int64 / float64; +// structpb collapses all numerics to float64). Mixed lists are accepted. +// Empty strings, non-positive IDs, and unsupported element types return an +// explicit error so a typo cannot silently drop an SSH key the operator +// expected to be installed. +func dropletSSHKeysFromConfig(cfg map[string]any) ([]godo.DropletCreateSSHKey, error) { + v, ok := cfg["ssh_keys"] + if !ok || v == nil { + return nil, nil + } + raw, ok := v.([]any) + if !ok { + // Accept a typed []string for Go-native callers as a convenience. + if ss, ok := v.([]string); ok { + out := make([]godo.DropletCreateSSHKey, 0, len(ss)) + for _, s := range ss { + if s == "" { + return nil, fmt.Errorf("ssh_keys: empty fingerprint") + } + out = append(out, godo.DropletCreateSSHKey{Fingerprint: s}) + } + return out, nil + } + return nil, fmt.Errorf("ssh_keys: expected list, got %T", v) + } + out := make([]godo.DropletCreateSSHKey, 0, len(raw)) + for i, e := range raw { + switch t := e.(type) { + case string: + if t == "" { + return nil, fmt.Errorf("ssh_keys[%d]: empty fingerprint", i) + } + out = append(out, godo.DropletCreateSSHKey{Fingerprint: t}) + case int: + if t <= 0 { + return nil, fmt.Errorf("ssh_keys[%d]: non-positive ID %d", i, t) + } + out = append(out, godo.DropletCreateSSHKey{ID: t}) + case int64: + if t <= 0 { + return nil, fmt.Errorf("ssh_keys[%d]: non-positive ID %d", i, t) + } + out = append(out, godo.DropletCreateSSHKey{ID: int(t)}) + case float64: + if t != float64(int64(t)) { + return nil, fmt.Errorf("ssh_keys[%d]: %v is not an integer", i, t) + } + if t <= 0 { + return nil, fmt.Errorf("ssh_keys[%d]: non-positive ID %v", i, t) + } + out = append(out, godo.DropletCreateSSHKey{ID: int(t)}) + default: + return nil, fmt.Errorf("ssh_keys[%d]: unsupported element type %T (want string fingerprint or numeric ID)", i, e) + } + } + return out, nil +} + +// resolveDropletVolumes turns the YAML "volumes" list of NAMES into the typed +// []godo.DropletCreateVolume {ID:...} shape that godo serialises. The DO API +// requires IDs (Name is deprecated server-side per godo doc-comment), so we +// look each name up via Storage.ListVolumes and error if no match exists in +// the droplet's region. region is the droplet's resolved region; volume +// matches outside that region are rejected since DO Block Storage cannot +// cross regions. +func (d *DropletDriver) resolveDropletVolumes(ctx context.Context, cfg map[string]any, region string) ([]godo.DropletCreateVolume, error) { + names := strSliceFromConfig(cfg, "volumes") + if len(names) == 0 { + return nil, nil + } + if d.storage == nil { + return nil, fmt.Errorf("droplet volumes: storage client not configured; cannot resolve volume names") + } + out := make([]godo.DropletCreateVolume, 0, len(names)) + for _, name := range names { + params := &godo.ListVolumeParams{Name: name, Region: region} + vols, _, err := d.storage.ListVolumes(ctx, params) + if err != nil { + return nil, fmt.Errorf("droplet volumes: list %q: %w", name, WrapGodoError(err)) + } + var matchID string + for _, v := range vols { + if v.Name == name { + matchID = v.ID + break + } + } + if matchID == "" { + return nil, fmt.Errorf("droplet volumes: volume %q not found", name) + } + out = append(out, godo.DropletCreateVolume{ID: matchID}) + } + return out, nil +} + func (d *DropletDriver) SensitiveKeys() []string { return nil } // ProviderIDFormat returns Freeform because DO Droplet IDs are integers, not diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 709bbc1..a1481cc 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -13,9 +13,13 @@ import ( type mockDropletClient struct { droplet *godo.Droplet err error + // gotReq captures the most recent Create request so tests can assert on + // the godo struct fields the driver populated from spec.Config. + gotReq *godo.DropletCreateRequest } -func (m *mockDropletClient) Create(_ context.Context, _ *godo.DropletCreateRequest) (*godo.Droplet, *godo.Response, error) { +func (m *mockDropletClient) Create(_ context.Context, req *godo.DropletCreateRequest) (*godo.Droplet, *godo.Response, error) { + m.gotReq = req return m.droplet, nil, m.err } func (m *mockDropletClient) Get(_ context.Context, _ int) (*godo.Droplet, *godo.Response, error) { @@ -25,6 +29,46 @@ func (m *mockDropletClient) Delete(_ context.Context, _ int) (*godo.Response, er return nil, m.err } +// mockStorageClient is a lightweight stand-in for godo.StorageService used by +// volume tests and the droplet volumes-by-name path. CreateVolume returns +// vol; ListVolumes filters byName from the volumes slice. +type mockStorageClient struct { + vol *godo.Volume + volumes []godo.Volume // pool returned by ListVolumes (filtered by name) + err error + // gotCreate captures the most recent CreateVolume request. + gotCreate *godo.VolumeCreateRequest + // gotListParams captures the most recent ListVolumes params for assertion. + gotListParams *godo.ListVolumeParams +} + +func (m *mockStorageClient) CreateVolume(_ context.Context, req *godo.VolumeCreateRequest) (*godo.Volume, *godo.Response, error) { + m.gotCreate = req + return m.vol, nil, m.err +} +func (m *mockStorageClient) GetVolume(_ context.Context, _ string) (*godo.Volume, *godo.Response, error) { + return m.vol, nil, m.err +} +func (m *mockStorageClient) DeleteVolume(_ context.Context, _ string) (*godo.Response, error) { + return nil, m.err +} +func (m *mockStorageClient) ListVolumes(_ context.Context, params *godo.ListVolumeParams) ([]godo.Volume, *godo.Response, error) { + m.gotListParams = params + if m.err != nil { + return nil, nil, m.err + } + if params == nil || params.Name == "" { + return m.volumes, nil, nil + } + var out []godo.Volume + for _, v := range m.volumes { + if v.Name == params.Name { + out = append(out, v) + } + } + return out, nil, nil +} + func testDroplet() *godo.Droplet { return &godo.Droplet{ ID: 42, @@ -35,6 +79,7 @@ func testDroplet() *godo.Droplet { Networks: &godo.Networks{ V4: []godo.NetworkV4{ {IPAddress: "1.2.3.4", Type: "public"}, + {IPAddress: "10.0.0.4", Type: "private"}, }, }, } @@ -60,6 +105,9 @@ func TestDropletDriver_Create(t *testing.T) { if out.Status != "active" { t.Errorf("Status = %q, want %q", out.Status, "active") } + if got, _ := out.Outputs["private_ip"].(string); got != "10.0.0.4" { + t.Errorf("private_ip = %q, want %q", got, "10.0.0.4") + } } func TestDropletDriver_Create_Error(t *testing.T) { @@ -224,3 +272,207 @@ func TestDropletDriver_Create_ProviderIDIsAPIAssigned(t *testing.T) { t.Errorf("ProviderID must not be empty") } } + +// --- Extended-config tests (user_data / vpc_uuid / tags / bools / ssh_keys / volumes) --- + +func TestDropletDriver_Create_PassesUserDataAndVPCAndBools(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "user_data": "#cloud-config\nruncmd:\n - apt-get update\n", + "vpc_uuid": "00000000-0000-0000-0000-000000000001", + "tags": []any{"prod", "pg"}, + "enable_backups": true, + "monitoring": true, + "ipv6": true, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + got := mock.gotReq + if got == nil { + t.Fatal("create request not captured") + } + if got.UserData == "" || got.UserData[:14] != "#cloud-config\n" { + t.Errorf("UserData not propagated: %q", got.UserData) + } + if got.VPCUUID != "00000000-0000-0000-0000-000000000001" { + t.Errorf("VPCUUID = %q", got.VPCUUID) + } + if len(got.Tags) != 2 || got.Tags[0] != "prod" || got.Tags[1] != "pg" { + t.Errorf("Tags = %v", got.Tags) + } + if !got.Backups || !got.Monitoring || !got.IPv6 { + t.Errorf("bool flags not propagated: backups=%v monitoring=%v ipv6=%v", + got.Backups, got.Monitoring, got.IPv6) + } +} + +func TestDropletDriver_Create_SSHKeys_Fingerprints(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []any{"aa:bb:cc", "11:22:33"}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + got := mock.gotReq.SSHKeys + if len(got) != 2 || got[0].Fingerprint != "aa:bb:cc" || got[1].Fingerprint != "11:22:33" { + t.Errorf("SSHKeys = %+v", got) + } +} + +func TestDropletDriver_Create_SSHKeys_NumericIDs(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + // structpb collapses all numerics to float64; cover that shape. + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []any{float64(101), float64(202)}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + got := mock.gotReq.SSHKeys + if len(got) != 2 || got[0].ID != 101 || got[1].ID != 202 { + t.Errorf("SSHKeys = %+v", got) + } + if got[0].Fingerprint != "" || got[1].Fingerprint != "" { + t.Errorf("numeric ssh_keys must not populate Fingerprint: %+v", got) + } +} + +func TestDropletDriver_Create_SSHKeys_Mixed(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []any{"aa:bb:cc", float64(7)}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + got := mock.gotReq.SSHKeys + if len(got) != 2 { + t.Fatalf("len(SSHKeys) = %d, want 2", len(got)) + } + if got[0].Fingerprint != "aa:bb:cc" { + t.Errorf("[0] = %+v, want fingerprint", got[0]) + } + if got[1].ID != 7 { + t.Errorf("[1] = %+v, want ID=7", got[1]) + } +} + +func TestDropletDriver_Create_SSHKeys_FractionalRejected(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []any{1.5}, + }, + }) + if err == nil { + t.Fatal("expected error for fractional ssh_keys ID, got nil") + } +} + +func TestDropletDriver_Create_Volumes_ResolvedByName(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + storage := &mockStorageClient{ + volumes: []godo.Volume{ + {ID: "vol-uuid-1", Name: "pg-data", Region: &godo.Region{Slug: "nyc3"}}, + {ID: "vol-uuid-2", Name: "pg-wal", Region: &godo.Region{Slug: "nyc3"}}, + }, + } + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "volumes": []any{"pg-data", "pg-wal"}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + vols := mock.gotReq.Volumes + if len(vols) != 2 { + t.Fatalf("len(volumes) = %d, want 2", len(vols)) + } + if vols[0].ID != "vol-uuid-1" || vols[1].ID != "vol-uuid-2" { + t.Errorf("volumes IDs = %+v, want [vol-uuid-1 vol-uuid-2]", vols) + } + // ListVolumes must filter by region so we don't accidentally attach a + // volume from a different region (DO Block Storage is region-bound). + if storage.gotListParams == nil || storage.gotListParams.Region != "nyc3" { + t.Errorf("ListVolumes region = %v, want nyc3", storage.gotListParams) + } +} + +func TestDropletDriver_Create_Volumes_NotFound(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + storage := &mockStorageClient{volumes: nil} + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "volumes": []any{"missing-vol"}, + }, + }) + if err == nil { + t.Fatal("expected error for unresolved volume name") + } + wantSubstr := `volume "missing-vol" not found` + if !contains(err.Error(), wantSubstr) { + t.Errorf("error %q missing %q", err.Error(), wantSubstr) + } +} + +func TestDropletDriver_Create_Volumes_NoStorageClient(t *testing.T) { + // Test-side double-check: if a caller wires Droplet without a storage + // client and then references volumes, fail loudly rather than silently + // dropping the attachment. + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") // no storage + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "volumes": []any{"pg-data"}, + }, + }) + if err == nil { + t.Fatal("expected error when volumes set but no storage client") + } +} + +func contains(s, sub string) bool { + if len(sub) == 0 { + return true + } + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + return false +} From 1b8e4a2ad978f5141330b52bd98f0fb0e20985ec Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:01:38 -0400 Subject: [PATCH 03/21] feat(drivers): add infra.volume driver (DO Block Storage) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New ResourceDriver covering CRUD + Diff + HealthCheck for DigitalOcean Block Storage volumes. Backed by godo.StorageService; in-place size growth uses godo.StorageActionsService.Resize. ProviderIDFormat is UUID (DO volume IDs are 36-character UUIDs). Config keys: - name from spec.Name - region defaults to provider region - size_gb required, > 0 - filesystem_type optional ("ext4" / "xfs"); empty = raw block device - description optional - tags optional []string Outputs: id (UUID), name, region, size_gb, filesystem_type. Status is a stable "available" string — godo.Volume exposes no Status field, so a successful Read on a real volume is the strongest health signal we have. Update semantics: - size growth → in-place StorageActions.Resize, then re-read for state - size shrink → explicit error (DO has no shrink API; replace required) - region or filesystem_type change → Diff sets ForceNew so plan classifies as replace, not update Tests cover Create / Read / Update (resize, no-op, shrink-rejected, empty-ProviderID guard) / Delete / Diff (nil current, grow=update-only, shrink=replace, region=replace, filesystem_type=replace, no-changes) / HealthCheck (success, read-error, empty-ProviderID) / ProviderIDFormat / Scale-not-supported. The volume mock pool supports filter-by-name so the same struct exercises both VolumeDriver tests and the droplet volumes-by-name path. providerid_format_test.go adds the volume entry to the manually maintained registry — without it, a future drift in ProviderIDFormat would not be caught. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/providerid_format_test.go | 1 + internal/drivers/volume.go | 248 ++++++++++++ internal/drivers/volume_test.go | 426 +++++++++++++++++++++ 3 files changed, 675 insertions(+) create mode 100644 internal/drivers/volume.go create mode 100644 internal/drivers/volume_test.go diff --git a/internal/drivers/providerid_format_test.go b/internal/drivers/providerid_format_test.go index 009c06c..56093e9 100644 --- a/internal/drivers/providerid_format_test.go +++ b/internal/drivers/providerid_format_test.go @@ -30,6 +30,7 @@ func TestAllDrivers_DeclareProviderIDFormat(t *testing.T) { {"firewall", drivers.NewFirewallDriverWithClient(&mockFirewallClient{}), interfaces.IDFormatUUID}, {"kubernetes", drivers.NewKubernetesDriverWithClient(&mockK8sClient{}, "nyc3"), interfaces.IDFormatUUID}, {"load_balancer", drivers.NewLoadBalancerDriverWithClient(&mockLBClient{}, "nyc3"), interfaces.IDFormatUUID}, + {"volume", drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3"), interfaces.IDFormatUUID}, {"vpc", drivers.NewVPCDriverWithClient(&mockVPCClient{}, "nyc3"), interfaces.IDFormatUUID}, // ── DomainName drivers ──────────────────────────────────────────── diff --git a/internal/drivers/volume.go b/internal/drivers/volume.go new file mode 100644 index 0000000..7f4c9c6 --- /dev/null +++ b/internal/drivers/volume.go @@ -0,0 +1,248 @@ +package drivers + +import ( + "context" + "fmt" + + "github.com/GoCodeAlone/workflow/interfaces" + "github.com/digitalocean/godo" +) + +// StorageClient is the godo Storage interface (for mocking). Subset of +// godo.StorageService — only the methods VolumeDriver and DropletDriver +// (volumes-by-name resolution) need. +type StorageClient interface { + CreateVolume(ctx context.Context, req *godo.VolumeCreateRequest) (*godo.Volume, *godo.Response, error) + GetVolume(ctx context.Context, volumeID string) (*godo.Volume, *godo.Response, error) + DeleteVolume(ctx context.Context, volumeID string) (*godo.Response, error) + ListVolumes(ctx context.Context, params *godo.ListVolumeParams) ([]godo.Volume, *godo.Response, error) +} + +// StorageActionsClient is the subset of godo.StorageActionsService used by +// VolumeDriver to execute resize actions (the only mutation DO supports on +// an existing volume without delete + recreate). +type StorageActionsClient interface { + Resize(ctx context.Context, volumeID string, sizeGigabytes int, regionSlug string) (*godo.Action, *godo.Response, error) +} + +// VolumeDriver manages DigitalOcean Block Storage volumes (infra.volume). +// +// Update semantics: only size GROWTH is supported in-place via +// StorageActions.Resize. Size shrinks are unsupported by DO; any other +// attribute change (region, filesystem_type) forces replace via Diff. +type VolumeDriver struct { + client StorageClient + actions StorageActionsClient + region string +} + +// NewVolumeDriver creates a VolumeDriver backed by a real godo client. +func NewVolumeDriver(c *godo.Client, region string) *VolumeDriver { + return &VolumeDriver{client: c.Storage, actions: c.StorageActions, region: region} +} + +// NewVolumeDriverWithClient creates a driver with injected clients (for tests). +// The actions client may be nil for tests that do not exercise the resize +// path. +func NewVolumeDriverWithClient(c StorageClient, actions StorageActionsClient, region string) *VolumeDriver { + return &VolumeDriver{client: c, actions: actions, region: region} +} + +func (d *VolumeDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + region := strFromConfig(spec.Config, "region", d.region) + sizeGB, _ := intFromConfig(spec.Config, "size_gb", 0) + if sizeGB <= 0 { + return nil, fmt.Errorf("volume create %q: size_gb is required and must be > 0", spec.Name) + } + req := &godo.VolumeCreateRequest{ + Region: region, + Name: spec.Name, + SizeGigaBytes: int64(sizeGB), + Description: strFromConfig(spec.Config, "description", ""), + FilesystemType: strFromConfig(spec.Config, "filesystem_type", ""), + Tags: strSliceFromConfig(spec.Config, "tags"), + } + vol, _, err := d.client.CreateVolume(ctx, req) + if err != nil { + return nil, fmt.Errorf("volume create %q: %w", spec.Name, WrapGodoError(err)) + } + if vol == nil || vol.ID == "" { + return nil, fmt.Errorf("volume create %q: API returned volume with empty ID", spec.Name) + } + return volumeOutput(vol), nil +} + +func (d *VolumeDriver) Read(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { + if ref.ProviderID == "" { + return nil, fmt.Errorf("volume read %q: empty ProviderID", ref.Name) + } + vol, _, err := d.client.GetVolume(ctx, ref.ProviderID) + if err != nil { + return nil, fmt.Errorf("volume read %q: %w", ref.Name, WrapGodoError(err)) + } + return volumeOutput(vol), nil +} + +func (d *VolumeDriver) Update(ctx context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + if ref.ProviderID == "" { + return nil, fmt.Errorf("volume update %q: empty ProviderID", ref.Name) + } + desiredSize, _ := intFromConfig(spec.Config, "size_gb", 0) + if desiredSize <= 0 { + return nil, fmt.Errorf("volume update %q: size_gb is required and must be > 0", ref.Name) + } + cur, _, err := d.client.GetVolume(ctx, ref.ProviderID) + if err != nil { + return nil, fmt.Errorf("volume update %q: read current: %w", ref.Name, WrapGodoError(err)) + } + if int64(desiredSize) == cur.SizeGigaBytes { + // No-op update — return current state. + return volumeOutput(cur), nil + } + if int64(desiredSize) < cur.SizeGigaBytes { + return nil, fmt.Errorf("volume update %q: shrinking from %d to %d GB is not supported by DO; replace required", + ref.Name, cur.SizeGigaBytes, desiredSize) + } + if d.actions == nil { + return nil, fmt.Errorf("volume update %q: storage actions client not configured; cannot resize", ref.Name) + } + region := cur.Region.Slug + if _, _, err := d.actions.Resize(ctx, ref.ProviderID, desiredSize, region); err != nil { + return nil, fmt.Errorf("volume update %q: resize to %d GB: %w", ref.Name, desiredSize, WrapGodoError(err)) + } + // Re-read to surface the post-resize state. + updated, _, err := d.client.GetVolume(ctx, ref.ProviderID) + if err != nil { + return nil, fmt.Errorf("volume update %q: read after resize: %w", ref.Name, WrapGodoError(err)) + } + return volumeOutput(updated), nil +} + +func (d *VolumeDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) error { + if ref.ProviderID == "" { + return fmt.Errorf("volume delete %q: empty ProviderID", ref.Name) + } + if _, err := d.client.DeleteVolume(ctx, ref.ProviderID); err != nil { + return fmt.Errorf("volume delete %q: %w", ref.Name, WrapGodoError(err)) + } + return nil +} + +func (d *VolumeDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + if current == nil { + return &interfaces.DiffResult{NeedsUpdate: true}, nil + } + var changes []interfaces.FieldChange + var needsReplace bool + + if desiredSize, ok := intFromConfig(desired.Config, "size_gb", 0); ok && desiredSize > 0 { + curSize := outputsAsInt(current.Outputs["size_gb"]) + if curSize != desiredSize { + // Growth = in-place resize; shrink = replace (DO has no shrink API). + fc := interfaces.FieldChange{ + Path: "size_gb", + Old: curSize, + New: desiredSize, + } + if desiredSize < curSize { + fc.ForceNew = true + needsReplace = true + } + changes = append(changes, fc) + } + } + + if region := strFromConfig(desired.Config, "region", ""); region != "" { + curRegion, _ := current.Outputs["region"].(string) + if curRegion != "" && curRegion != region { + changes = append(changes, interfaces.FieldChange{ + Path: "region", Old: curRegion, New: region, ForceNew: true, + }) + needsReplace = true + } + } + + if fs := strFromConfig(desired.Config, "filesystem_type", ""); fs != "" { + curFS, _ := current.Outputs["filesystem_type"].(string) + if curFS != "" && curFS != fs { + changes = append(changes, interfaces.FieldChange{ + Path: "filesystem_type", Old: curFS, New: fs, ForceNew: true, + }) + needsReplace = true + } + } + + return &interfaces.DiffResult{ + NeedsUpdate: len(changes) > 0, + NeedsReplace: needsReplace, + Changes: changes, + }, nil +} + +func (d *VolumeDriver) HealthCheck(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.HealthResult, error) { + if ref.ProviderID == "" { + return &interfaces.HealthResult{Healthy: false, Message: "empty ProviderID"}, nil + } + vol, _, err := d.client.GetVolume(ctx, ref.ProviderID) + if err != nil { + return &interfaces.HealthResult{Healthy: false, Message: err.Error()}, nil + } + // godo.Volume has no Status field; a successful Read on a real volume is + // the strongest health signal the DO API exposes here. Surface size and + // region in the message so operators can spot a misconfigured pair. + msg := fmt.Sprintf("available (%d GB, region=%s)", vol.SizeGigaBytes, regionSlug(vol.Region)) + return &interfaces.HealthResult{Healthy: true, Message: msg}, nil +} + +func (d *VolumeDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ int) (*interfaces.ResourceOutput, error) { + return nil, fmt.Errorf("volume does not support scale operation; change size_gb and re-apply") +} + +func (d *VolumeDriver) SensitiveKeys() []string { return nil } + +// ProviderIDFormat is UUID — DO Block Storage volume IDs are UUIDs. +func (d *VolumeDriver) ProviderIDFormat() interfaces.ProviderIDFormat { + return interfaces.IDFormatUUID +} + +func volumeOutput(vol *godo.Volume) *interfaces.ResourceOutput { + return &interfaces.ResourceOutput{ + Name: vol.Name, + Type: "infra.volume", + ProviderID: vol.ID, + Outputs: map[string]any{ + "id": vol.ID, + "name": vol.Name, + "region": regionSlug(vol.Region), + "size_gb": float64(vol.SizeGigaBytes), + "filesystem_type": vol.FilesystemType, + }, + // godo.Volume exposes no Status field; report a stable string so + // downstream callers don't read empty Status as "unknown failure". + Status: "available", + } +} + +// regionSlug returns r.Slug for a non-nil region, "" otherwise. Centralised +// so volume helpers don't repeat the nil-guard. +func regionSlug(r *godo.Region) string { + if r == nil { + return "" + } + return r.Slug +} + +// outputsAsInt converts an Outputs value (which may be int, int64, or +// float64 after a structpb round-trip) back to int. Returns 0 for missing +// or unparseable values. +func outputsAsInt(v any) int { + switch t := v.(type) { + case int: + return t + case int64: + return int(t) + case float64: + return int(t) + } + return 0 +} diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go new file mode 100644 index 0000000..7c3afca --- /dev/null +++ b/internal/drivers/volume_test.go @@ -0,0 +1,426 @@ +package drivers_test + +import ( + "context" + "fmt" + "testing" + + "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" + "github.com/GoCodeAlone/workflow/interfaces" + "github.com/digitalocean/godo" +) + +// mockStorageActionsClient is a tiny stand-in for godo.StorageActionsService +// that only handles the Resize call the VolumeDriver issues. resizedTo +// captures the size parameter so tests can assert resize was actually called +// with the desired GB. +type mockStorageActionsClient struct { + err error + resizedTo int + region string +} + +func (m *mockStorageActionsClient) Resize(_ context.Context, _ string, sizeGB int, region string) (*godo.Action, *godo.Response, error) { + m.resizedTo = sizeGB + m.region = region + if m.err != nil { + return nil, nil, m.err + } + return &godo.Action{Status: "completed"}, nil, nil +} + +func testVolume() *godo.Volume { + return &godo.Volume{ + ID: "vol-aaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + Name: "pg-data", + Region: &godo.Region{Slug: "nyc3"}, + SizeGigaBytes: 100, + FilesystemType: "ext4", + Description: "Postgres data volume", + Tags: []string{"prod", "pg"}, + } +} + +func TestVolumeDriver_Create(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + out, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{ + "size_gb": 100, + "filesystem_type": "ext4", + "description": "Postgres data volume", + "tags": []any{"prod", "pg"}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if out.ProviderID == "" { + t.Fatal("ProviderID empty") + } + if out.Outputs["filesystem_type"] != "ext4" { + t.Errorf("filesystem_type = %v", out.Outputs["filesystem_type"]) + } + if out.Outputs["size_gb"].(float64) != 100 { + t.Errorf("size_gb = %v", out.Outputs["size_gb"]) + } + got := mock.gotCreate + if got == nil { + t.Fatal("create request not captured") + } + if got.Name != "pg-data" || got.SizeGigaBytes != 100 || got.FilesystemType != "ext4" || got.Description != "Postgres data volume" { + t.Errorf("create request = %+v", got) + } + if got.Region != "nyc3" { + t.Errorf("Region = %q, want nyc3", got.Region) + } + if len(got.Tags) != 2 { + t.Errorf("Tags len = %d", len(got.Tags)) + } +} + +func TestVolumeDriver_Create_RegionOverride(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 50, "region": "sfo3"}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if mock.gotCreate.Region != "sfo3" { + t.Errorf("Region = %q, want sfo3", mock.gotCreate.Region) + } +} + +func TestVolumeDriver_Create_MissingSize(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{}, + }) + if err == nil { + t.Fatal("expected error for missing size_gb") + } +} + +func TestVolumeDriver_Create_APIError(t *testing.T) { + mock := &mockStorageClient{err: fmt.Errorf("api failure")} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 50}, + }) + if err == nil { + t.Fatal("expected error") + } +} + +func TestVolumeDriver_Create_EmptyIDFromAPI(t *testing.T) { + mock := &mockStorageClient{vol: &godo.Volume{Name: "pg-data"}} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 50}, + }) + if err == nil { + t.Fatal("expected error for empty ID from API") + } +} + +func TestVolumeDriver_Read_Success(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + out, err := d.Read(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + if out.Name != "pg-data" { + t.Errorf("Name = %q", out.Name) + } +} + +func TestVolumeDriver_Read_EmptyProviderID(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Read(context.Background(), interfaces.ResourceRef{Name: "pg-data"}) + if err == nil { + t.Fatal("expected error for empty ProviderID") + } +} + +func TestVolumeDriver_Update_Resize(t *testing.T) { + current := testVolume() + updated := *current + updated.SizeGigaBytes = 200 + mock := &mockStorageClient{vol: current} + actions := &mockStorageActionsClient{} + d := drivers.NewVolumeDriverWithClient(mock, actions, "nyc3") + + // Prepare: GetVolume first returns the original, then the resized version. + // Our simple mock returns vol on every call, so swap after Resize(). + _, err := d.Update(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: current.ID, + }, interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 200}, + }) + if err != nil { + t.Fatalf("Update: %v", err) + } + if actions.resizedTo != 200 { + t.Errorf("resizedTo = %d, want 200", actions.resizedTo) + } + if actions.region != "nyc3" { + t.Errorf("resize region = %q, want nyc3", actions.region) + } +} + +func TestVolumeDriver_Update_NoChange(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + actions := &mockStorageActionsClient{} + d := drivers.NewVolumeDriverWithClient(mock, actions, "nyc3") + + _, err := d.Update(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }, interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 100}, // same as current + }) + if err != nil { + t.Fatalf("Update: %v", err) + } + if actions.resizedTo != 0 { + t.Errorf("Resize must not be called when size unchanged; resizedTo = %d", actions.resizedTo) + } +} + +func TestVolumeDriver_Update_ShrinkRejected(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} // 100 GB + actions := &mockStorageActionsClient{} + d := drivers.NewVolumeDriverWithClient(mock, actions, "nyc3") + + _, err := d.Update(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }, interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 50}, + }) + if err == nil { + t.Fatal("expected error rejecting shrink") + } + if actions.resizedTo != 0 { + t.Errorf("shrink must not invoke Resize; resizedTo = %d", actions.resizedTo) + } +} + +func TestVolumeDriver_Update_EmptyProviderID(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Update(context.Background(), interfaces.ResourceRef{Name: "pg-data"}, interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 200}, + }) + if err == nil { + t.Fatal("expected error for empty ProviderID") + } +} + +func TestVolumeDriver_Delete_Success(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + err := d.Delete(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }) + if err != nil { + t.Fatalf("Delete: %v", err) + } +} + +func TestVolumeDriver_Delete_Error(t *testing.T) { + mock := &mockStorageClient{err: fmt.Errorf("delete failed")} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + err := d.Delete(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }) + if err == nil { + t.Fatal("expected error") + } +} + +func TestVolumeDriver_Delete_EmptyProviderID(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + err := d.Delete(context.Background(), interfaces.ResourceRef{Name: "pg-data"}) + if err == nil { + t.Fatal("expected error for empty ProviderID") + } +} + +func TestVolumeDriver_Diff_NilCurrent(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{}, nil) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsUpdate { + t.Errorf("expected NeedsUpdate=true for nil current") + } +} + +func TestVolumeDriver_Diff_GrowIsUpdateNotReplace(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100)}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 200}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsUpdate { + t.Errorf("expected NeedsUpdate=true for size growth") + } + if r.NeedsReplace { + t.Errorf("size growth must NOT force replace; got NeedsReplace=true") + } +} + +func TestVolumeDriver_Diff_ShrinkForcesReplace(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100)}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 50}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("shrink must force replace") + } +} + +func TestVolumeDriver_Diff_RegionForcesReplace(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "region": "nyc3"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "region": "sfo3"}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("region change must force replace") + } +} + +func TestVolumeDriver_Diff_FilesystemTypeForcesReplace(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "filesystem_type": "ext4"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "filesystem_type": "xfs"}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("filesystem_type change must force replace") + } +} + +func TestVolumeDriver_Diff_NoChanges(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "region": "nyc3", "filesystem_type": "ext4"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "region": "nyc3", "filesystem_type": "ext4"}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsUpdate { + t.Errorf("expected NeedsUpdate=false when nothing changed") + } +} + +func TestVolumeDriver_HealthCheck_Success(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + r, err := d.HealthCheck(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }) + if err != nil { + t.Fatalf("HealthCheck: %v", err) + } + if !r.Healthy { + t.Errorf("expected healthy") + } +} + +func TestVolumeDriver_HealthCheck_ReadError(t *testing.T) { + mock := &mockStorageClient{err: fmt.Errorf("api down")} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + r, err := d.HealthCheck(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }) + if err != nil { + t.Fatalf("HealthCheck must not return error; got %v", err) + } + if r.Healthy { + t.Errorf("expected unhealthy when read fails") + } +} + +func TestVolumeDriver_HealthCheck_EmptyProviderID(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + r, err := d.HealthCheck(context.Background(), interfaces.ResourceRef{Name: "pg-data"}) + if err != nil { + t.Fatalf("HealthCheck: %v", err) + } + if r.Healthy { + t.Errorf("expected unhealthy for empty ProviderID") + } +} + +func TestVolumeDriver_ProviderIDFormat_IsUUID(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + if got := d.ProviderIDFormat(); got != interfaces.IDFormatUUID { + t.Errorf("ProviderIDFormat = %v, want IDFormatUUID", got) + } +} + +func TestVolumeDriver_Scale_NotSupported(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + _, err := d.Scale(context.Background(), interfaces.ResourceRef{}, 2) + if err == nil { + t.Fatal("expected scale not supported error") + } +} From 605e2c868ef61710881c4fd9d2c068231c51c183 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:01:45 -0400 Subject: [PATCH 04/21] feat(provider): register infra.volume driver and capability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire VolumeDriver into Initialize() alongside the other infra drivers and declare its IaCCapabilityDeclaration with the noScale operation set (volumes have no scale concept; size_gb is dimensional). Add "infra.volume" to the sizing map as a noopSizing entry so ResolveSizing returns "n/a" for every abstract tier — the operator must set size_gb explicitly. Provider Capabilities test gains the matching required entry so removal of the driver registration would be caught. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/provider.go | 2 ++ internal/provider_test.go | 1 + internal/sizing.go | 4 ++++ 3 files changed, 7 insertions(+) diff --git a/internal/provider.go b/internal/provider.go index e1bd5af..151fb29 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -79,6 +79,7 @@ func (p *DOProvider) Initialize(_ context.Context, config map[string]any) error "infra.registry": drivers.NewRegistryDriver(p.client), "infra.certificate": drivers.NewCertificateDriver(p.client), "infra.droplet": drivers.NewDropletDriver(p.client, p.region), + "infra.volume": drivers.NewVolumeDriver(p.client, p.region), "infra.iam_role": drivers.NewIAMRoleDriver(), "infra.api_gateway": drivers.NewAPIGatewayDriver(p.client, p.region), } @@ -102,6 +103,7 @@ func (p *DOProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { {ResourceType: "infra.registry", Tier: 2, Operations: ops}, {ResourceType: "infra.certificate", Tier: 1, Operations: ops}, {ResourceType: "infra.droplet", Tier: 1, Operations: ops}, + {ResourceType: "infra.volume", Tier: 1, Operations: noScale}, {ResourceType: "infra.iam_role", Tier: 1, Operations: noScale}, {ResourceType: "infra.api_gateway", Tier: 3, Operations: noScale}, } diff --git a/internal/provider_test.go b/internal/provider_test.go index 443ed01..02051f1 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -49,6 +49,7 @@ func TestDOProvider_Capabilities(t *testing.T) { "infra.registry", "infra.certificate", "infra.droplet", + "infra.volume", "infra.iam_role", "infra.api_gateway", } diff --git a/internal/sizing.go b/internal/sizing.go index 2d31b73..744e86c 100644 --- a/internal/sizing.go +++ b/internal/sizing.go @@ -68,6 +68,10 @@ var doSizingMap = map[string]map[interfaces.Size]string{ "infra.registry": noopSizing(), "infra.certificate": noopSizing(), "infra.iam_role": noopSizing(), + // infra.volume sizing is dimensional (size_gb), not tiered — every + // abstract Size maps to "n/a" and the operator must set size_gb + // explicitly in spec.Config. + "infra.volume": noopSizing(), } // noopSizing returns a size map where every tier maps to "n/a". From 8e215528ae5b0e18355cce929053a3a9ad55b662 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:01:53 -0400 Subject: [PATCH 05/21] docs: declare infra.volume in plugin.json and CHANGELOG plugin.json: add infra.volume to the iacProvider.resourceTypes array and update the descriptor to mention Block Storage volumes. CHANGELOG: document the new infra.volume driver and the extended Droplet config keys (user_data / vpc_uuid / ssh_keys / volumes / tags / enable_backups / monitoring / ipv6) plus the new private_ip Droplet output. No README exists in this repo, so the CHANGELOG is the canonical surface for these notes. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 35 +++++++++++++++++++++++++++++++++++ plugin.json | 3 ++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58f379b..edf668a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,41 @@ All notable changes to workflow-plugin-digitalocean are documented here. ### Added +- **`infra.volume` driver** — DigitalOcean Block Storage. Create / Read / + Update (in-place resize via `StorageActions.Resize` for size growth) / + Delete / Diff (size shrinks, region changes, and filesystem_type changes + force replace) / HealthCheck (Read succeeds → healthy; godo `Volume` + exposes no Status field, so a successful API round-trip is the strongest + available signal). + + Config keys: `name` (from `spec.Name`), `region` (defaults to provider + region), `size_gb` (required, > 0), `filesystem_type` (`ext4` / `xfs`; + default empty = raw block device), `description`, `tags`. + + Outputs: `id` (UUID), `name`, `region`, `size_gb`, `filesystem_type`. + ProviderIDFormat = `IDFormatUUID`. + +- **Droplet driver — extended config** for self-hosted services that need + more than the bare-minimum size/image/region. New optional keys, all + additive and defaulted-empty (no behaviour change for existing configs): + + - `user_data` (string) — cloud-init payload + - `vpc_uuid` (string) — VPC the Droplet joins + - `ssh_keys` ([]string | []int | mixed) — fingerprint strings OR numeric + SSH-key IDs; element type is detected at runtime (structpb-safe; floats + must be whole numbers) + - `tags` ([]string) + - `enable_backups` (bool) — maps to `Backups` + - `monitoring` (bool) + - `ipv6` (bool) + - `volumes` ([]string of Block Storage volume **names**) — names are + resolved to IDs at create time via `Storage.ListVolumes(name=,region=)`; + a name that doesn't resolve in the Droplet's region returns + `droplet volumes: volume %q not found` + + Droplet outputs gain `private_ip` (`droplet.PrivateIPv4()`) so downstream + services in the same VPC can be wired directly. + - **Troubleshoot fetches DO deploy/build logs** (PR-E2) — `AppPlatformDriver.Troubleshoot` now calls `godo.AppsService.GetLogs` for each component in any deployment whose phase is `Error`, `Canceled`, or `Superseded`. The DO API returns diff --git a/plugin.json b/plugin.json index 9a756a2..bd74eb5 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "name": "workflow-plugin-digitalocean", "version": "0.8.0", - "description": "DigitalOcean IaC provider: App Platform, DOKS, databases, Redis cache, load balancers, VPC, firewall, DNS, Spaces, DOCR, certificates, Droplets, IAM (declared), and API gateway", + "description": "DigitalOcean IaC provider: App Platform, DOKS, databases, Redis cache, load balancers, VPC, firewall, DNS, Spaces, DOCR, certificates, Droplets, Block Storage volumes, IAM (declared), and API gateway", "author": "GoCodeAlone", "license": "MIT", "type": "external", @@ -30,6 +30,7 @@ "infra.registry", "infra.certificate", "infra.droplet", + "infra.volume", "infra.iam_role", "infra.api_gateway" ], From be15f7875a6a9407c60886d29e1b1881dce5fd40 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:14:19 -0400 Subject: [PATCH 06/21] fix(droplet): strict validation for volumes config entries (Copilot finding #1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit strSliceFromConfig silently drops non-string and empty entries, which is dangerous for volume attachments — a typo or wrong type leaves the Droplet running without an expected disk and emits no diagnostic. Add dropletVolumesFromConfig that returns an explicit error for any non-string or empty entry, mirroring dropletSSHKeysFromConfig's error-message style. Tests cover both shapes: []any{123, "data"} (type mismatch) and []any{""} (empty string). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 43 +++++++++++++++++++++++++++++- internal/drivers/droplet_test.go | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 970430e..cb25348 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -244,6 +244,44 @@ func dropletSSHKeysFromConfig(cfg map[string]any) ([]godo.DropletCreateSSHKey, e return out, nil } +// dropletVolumesFromConfig extracts the "volumes" config entry as a strict +// []string of volume names. Unlike strSliceFromConfig (which silently drops +// non-string and empty entries), this returns an explicit error for any +// invalid element. Volume attachments are load-bearing: a typo or wrong type +// must NOT silently leave the Droplet running without an expected disk. +// Mirrors the error-message style of dropletSSHKeysFromConfig. +func dropletVolumesFromConfig(cfg map[string]any) ([]string, error) { + v, ok := cfg["volumes"] + if !ok || v == nil { + return nil, nil + } + switch raw := v.(type) { + case []string: + out := make([]string, 0, len(raw)) + for i, s := range raw { + if s == "" { + return nil, fmt.Errorf("droplet volumes: invalid entry at index %d: expected non-empty string, got empty string", i) + } + out = append(out, s) + } + return out, nil + case []any: + out := make([]string, 0, len(raw)) + for i, e := range raw { + s, ok := e.(string) + if !ok { + return nil, fmt.Errorf("droplet volumes: invalid entry at index %d: expected non-empty string, got %T", i, e) + } + if s == "" { + return nil, fmt.Errorf("droplet volumes: invalid entry at index %d: expected non-empty string, got empty string", i) + } + out = append(out, s) + } + return out, nil + } + return nil, fmt.Errorf("droplet volumes: expected list, got %T", v) +} + // resolveDropletVolumes turns the YAML "volumes" list of NAMES into the typed // []godo.DropletCreateVolume {ID:...} shape that godo serialises. The DO API // requires IDs (Name is deprecated server-side per godo doc-comment), so we @@ -252,7 +290,10 @@ func dropletSSHKeysFromConfig(cfg map[string]any) ([]godo.DropletCreateSSHKey, e // matches outside that region are rejected since DO Block Storage cannot // cross regions. func (d *DropletDriver) resolveDropletVolumes(ctx context.Context, cfg map[string]any, region string) ([]godo.DropletCreateVolume, error) { - names := strSliceFromConfig(cfg, "volumes") + names, err := dropletVolumesFromConfig(cfg) + if err != nil { + return nil, err + } if len(names) == 0 { return nil, nil } diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index a1481cc..bec513c 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -465,6 +465,51 @@ func TestDropletDriver_Create_Volumes_NoStorageClient(t *testing.T) { } } +func TestDropletDriver_Create_Volumes_NonStringEntryRejected(t *testing.T) { + // Copilot finding #1: strSliceFromConfig silently drops non-string + // entries. For volume attachments that risks leaving the Droplet + // running without an expected disk. dropletVolumesFromConfig must + // reject non-string entries with an explicit error. + mock := &mockDropletClient{droplet: testDroplet()} + storage := &mockStorageClient{ + volumes: []godo.Volume{{ID: "vol-uuid-2", Name: "data", Region: &godo.Region{Slug: "nyc3"}}}, + } + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "volumes": []any{123, "data"}, + }, + }) + if err == nil { + t.Fatal("expected error rejecting non-string volumes entry") + } + wantSubstr := `droplet volumes: invalid entry at index 0: expected non-empty string, got int` + if !contains(err.Error(), wantSubstr) { + t.Errorf("error %q missing %q", err.Error(), wantSubstr) + } +} + +func TestDropletDriver_Create_Volumes_EmptyStringRejected(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + storage := &mockStorageClient{volumes: nil} + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "volumes": []any{""}, + }, + }) + if err == nil { + t.Fatal("expected error rejecting empty volumes entry") + } + if !contains(err.Error(), "expected non-empty string, got empty string") { + t.Errorf("error %q does not flag empty string", err.Error()) + } +} + func contains(s, sub string) bool { if len(sub) == 0 { return true From 287a91d3711d9c1d2b0875ddb2f6f02de4b23444 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:15:48 -0400 Subject: [PATCH 07/21] fix(volume): reject fractional size_gb instead of silent truncation (Copilot finding #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit intFromConfig truncates float64 silently — size_gb: 100.9 (e.g. via JSON or YAML float coercion) was creating a 100 GB volume with no diagnostic. Add intStrictFromConfig that mirrors the float64 fractional-rejection logic already in dropletSSHKeysFromConfig and use it for size_gb in Create, Update, and Diff. Whole-valued float64 (the structpb wire shape) still passes through. intFromConfig is unchanged for callers where rounding is acceptable (node_count, instance_count, probe seconds, etc.). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/util.go | 36 +++++++++++++++++++- internal/drivers/volume.go | 16 +++++++-- internal/drivers/volume_test.go | 60 +++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) diff --git a/internal/drivers/util.go b/internal/drivers/util.go index 3b2aae7..897e20e 100644 --- a/internal/drivers/util.go +++ b/internal/drivers/util.go @@ -1,8 +1,16 @@ package drivers -import "strings" +import ( + "fmt" + "strings" +) // intFromConfig extracts an integer value from a config map, returning a default if absent. +// +// NOTE: float64 values are coerced to int via truncation. For capacity / +// size-style fields where silent truncation can under-provision (e.g. +// volume size_gb), use intStrictFromConfig which rejects fractional values +// explicitly. func intFromConfig(config map[string]any, key string, defaultVal int) (int, bool) { v, ok := config[key] if !ok { @@ -19,6 +27,32 @@ func intFromConfig(config map[string]any, key string, defaultVal int) (int, bool return defaultVal, false } +// intStrictFromConfig extracts an integer with strict fractional rejection. +// Used for capacity / size fields where silent truncation of e.g. 100.9 to +// 100 would under-provision storage. Mirrors the float64 handling in +// dropletSSHKeysFromConfig so the operator gets a clear error rather than +// silent rounding. errLabel is the human-readable field identifier used in +// the error message (e.g. "volume size_gb"). Returns (value, present, error). +// When present=false, value is defaultVal and error is nil. +func intStrictFromConfig(config map[string]any, key, errLabel string, defaultVal int) (int, bool, error) { + v, ok := config[key] + if !ok { + return defaultVal, false, nil + } + switch t := v.(type) { + case int: + return t, true, nil + case int64: + return int(t), true, nil + case float64: + if t != float64(int64(t)) { + return defaultVal, true, fmt.Errorf("%s: fractional value %v rejected; specify an integer", errLabel, t) + } + return int(t), true, nil + } + return defaultVal, false, nil +} + // strFromConfig extracts a string value from a config map with a default. func strFromConfig(config map[string]any, key, defaultVal string) string { if v, ok := config[key].(string); ok && v != "" { diff --git a/internal/drivers/volume.go b/internal/drivers/volume.go index 7f4c9c6..bb00fdf 100644 --- a/internal/drivers/volume.go +++ b/internal/drivers/volume.go @@ -50,7 +50,10 @@ func NewVolumeDriverWithClient(c StorageClient, actions StorageActionsClient, re func (d *VolumeDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { region := strFromConfig(spec.Config, "region", d.region) - sizeGB, _ := intFromConfig(spec.Config, "size_gb", 0) + sizeGB, _, err := intStrictFromConfig(spec.Config, "size_gb", "volume size_gb", 0) + if err != nil { + return nil, fmt.Errorf("volume create %q: %w", spec.Name, err) + } if sizeGB <= 0 { return nil, fmt.Errorf("volume create %q: size_gb is required and must be > 0", spec.Name) } @@ -87,7 +90,10 @@ func (d *VolumeDriver) Update(ctx context.Context, ref interfaces.ResourceRef, s if ref.ProviderID == "" { return nil, fmt.Errorf("volume update %q: empty ProviderID", ref.Name) } - desiredSize, _ := intFromConfig(spec.Config, "size_gb", 0) + desiredSize, _, err := intStrictFromConfig(spec.Config, "size_gb", "volume size_gb", 0) + if err != nil { + return nil, fmt.Errorf("volume update %q: %w", ref.Name, err) + } if desiredSize <= 0 { return nil, fmt.Errorf("volume update %q: size_gb is required and must be > 0", ref.Name) } @@ -135,7 +141,11 @@ func (d *VolumeDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, var changes []interfaces.FieldChange var needsReplace bool - if desiredSize, ok := intFromConfig(desired.Config, "size_gb", 0); ok && desiredSize > 0 { + desiredSize, sizePresent, err := intStrictFromConfig(desired.Config, "size_gb", "volume size_gb", 0) + if err != nil { + return nil, err + } + if sizePresent && desiredSize > 0 { curSize := outputsAsInt(current.Outputs["size_gb"]) if curSize != desiredSize { // Growth = in-place resize; shrink = replace (DO has no shrink API). diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go index 7c3afca..32b3066 100644 --- a/internal/drivers/volume_test.go +++ b/internal/drivers/volume_test.go @@ -136,6 +136,66 @@ func TestVolumeDriver_Create_EmptyIDFromAPI(t *testing.T) { } } +func TestVolumeDriver_Create_FractionalSizeRejected(t *testing.T) { + // Copilot finding #2: intFromConfig truncates float64 — size_gb: 100.9 + // previously created a 100 GB volume silently. The strict path must + // reject fractional values explicitly so operators can fix the typo. + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 100.9}, + }) + if err == nil { + t.Fatal("expected error rejecting fractional size_gb") + } + wantSubstr := "fractional value 100.9 rejected" + if !contains(err.Error(), wantSubstr) { + t.Errorf("error %q missing %q", err.Error(), wantSubstr) + } + if mock.gotCreate != nil { + t.Errorf("CreateVolume must not be called when size_gb is fractional; got %+v", mock.gotCreate) + } +} + +func TestVolumeDriver_Update_FractionalSizeRejected(t *testing.T) { + mock := &mockStorageClient{vol: testVolume()} + actions := &mockStorageActionsClient{} + d := drivers.NewVolumeDriverWithClient(mock, actions, "nyc3") + + _, err := d.Update(context.Background(), interfaces.ResourceRef{ + Name: "pg-data", ProviderID: "vol-aaaa", + }, interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": 200.5}, + }) + if err == nil { + t.Fatal("expected error rejecting fractional size_gb on update") + } + if actions.resizedTo != 0 { + t.Errorf("Resize must not be called for fractional size; resizedTo = %d", actions.resizedTo) + } +} + +func TestVolumeDriver_Create_WholeFloatAccepted(t *testing.T) { + // structpb collapses 50 → float64(50) on the wire; whole-valued floats + // must still pass the strict check. + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": float64(50)}, + }) + if err != nil { + t.Fatalf("Create with whole-valued float64 size_gb must succeed: %v", err) + } + if mock.gotCreate == nil || mock.gotCreate.SizeGigaBytes != 50 { + t.Errorf("CreateVolume.SizeGigaBytes = %v, want 50", mock.gotCreate) + } +} + func TestVolumeDriver_Read_Success(t *testing.T) { mock := &mockStorageClient{vol: testVolume()} d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") From 004acb45a76d3760eb028021f9509ba90f642d77 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:16:22 -0400 Subject: [PATCH 08/21] fix(droplet): accept []int and []int64 ssh_keys at top level (Copilot finding #3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dropletSSHKeysFromConfig accepted []any and []string at the top level but bailed on Go-native []int{101} or []int64{101} with "expected list, got []int" — even though numeric IDs are documented as supported and the per-element handler already covers int / int64 / float64. Add type-switches that mirror the per-element validation (non-positive rejection, error message style). Tests cover three shapes: []int{101,102}, []int64{555,666}, and the non-positive rejection path []int{0,101}. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 30 ++++++++++++++--- internal/drivers/droplet_test.go | 56 ++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index cb25348..f11e5b2 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -198,16 +198,36 @@ func dropletSSHKeysFromConfig(cfg map[string]any) ([]godo.DropletCreateSSHKey, e } raw, ok := v.([]any) if !ok { - // Accept a typed []string for Go-native callers as a convenience. - if ss, ok := v.([]string); ok { - out := make([]godo.DropletCreateSSHKey, 0, len(ss)) - for _, s := range ss { + // Accept Go-native typed slices as a convenience. Each branch + // mirrors the per-element validation in the []any path. + switch tv := v.(type) { + case []string: + out := make([]godo.DropletCreateSSHKey, 0, len(tv)) + for i, s := range tv { if s == "" { - return nil, fmt.Errorf("ssh_keys: empty fingerprint") + return nil, fmt.Errorf("ssh_keys[%d]: empty fingerprint", i) } out = append(out, godo.DropletCreateSSHKey{Fingerprint: s}) } return out, nil + case []int: + out := make([]godo.DropletCreateSSHKey, 0, len(tv)) + for i, n := range tv { + if n <= 0 { + return nil, fmt.Errorf("ssh_keys[%d]: non-positive ID %d", i, n) + } + out = append(out, godo.DropletCreateSSHKey{ID: n}) + } + return out, nil + case []int64: + out := make([]godo.DropletCreateSSHKey, 0, len(tv)) + for i, n := range tv { + if n <= 0 { + return nil, fmt.Errorf("ssh_keys[%d]: non-positive ID %d", i, n) + } + out = append(out, godo.DropletCreateSSHKey{ID: int(n)}) + } + return out, nil } return nil, fmt.Errorf("ssh_keys: expected list, got %T", v) } diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index bec513c..7854a89 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -379,6 +379,62 @@ func TestDropletDriver_Create_SSHKeys_Mixed(t *testing.T) { } } +func TestDropletDriver_Create_SSHKeys_TopLevelIntSlice(t *testing.T) { + // Copilot finding #3: dropletSSHKeysFromConfig accepted []any and + // []string at the top level but bailed on Go-native []int / []int64 + // even though the design admits int IDs. Cover both shapes. + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []int{101, 102}, + }, + }) + if err != nil { + t.Fatalf("Create with []int ssh_keys: %v", err) + } + got := mock.gotReq.SSHKeys + if len(got) != 2 || got[0].ID != 101 || got[1].ID != 102 { + t.Errorf("SSHKeys = %+v, want [{ID:101} {ID:102}]", got) + } +} + +func TestDropletDriver_Create_SSHKeys_TopLevelInt64Slice(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []int64{555, 666}, + }, + }) + if err != nil { + t.Fatalf("Create with []int64 ssh_keys: %v", err) + } + got := mock.gotReq.SSHKeys + if len(got) != 2 || got[0].ID != 555 || got[1].ID != 666 { + t.Errorf("SSHKeys = %+v, want [{ID:555} {ID:666}]", got) + } +} + +func TestDropletDriver_Create_SSHKeys_TopLevelIntSlice_NonPositiveRejected(t *testing.T) { + mock := &mockDropletClient{droplet: testDroplet()} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-droplet", + Config: map[string]any{ + "ssh_keys": []int{0, 101}, + }, + }) + if err == nil { + t.Fatal("expected error for non-positive ID in []int ssh_keys") + } +} + func TestDropletDriver_Create_SSHKeys_FractionalRejected(t *testing.T) { mock := &mockDropletClient{droplet: testDroplet()} d := drivers.NewDropletDriverWithClient(mock, "nyc3") From e8d5aa545fc40d764969e84ebe86d60accad962d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:16:50 -0400 Subject: [PATCH 09/21] fix(sizing): cover infra.volume in resolveSizing test cases (Copilot finding #4) infra.volume was added to doSizingMap as noopSizing but the manually- maintained TestResolveSizing cases table was not updated. Without the case, the registration is unverified and a future refactor that drops the entry would not fail this test. Add SizeS + SizeXL coverage so the "n/a" contract is locked in alongside every other noop-sized resource. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/sizing_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/sizing_test.go b/internal/sizing_test.go index a318616..b46873a 100644 --- a/internal/sizing_test.go +++ b/internal/sizing_test.go @@ -34,6 +34,12 @@ func TestResolveSizing(t *testing.T) { {"infra.registry", interfaces.SizeS, "n/a"}, {"infra.certificate", interfaces.SizeM, "n/a"}, {"infra.iam_role", interfaces.SizeS, "n/a"}, + // Copilot finding #4: infra.volume sizing is dimensional (size_gb), + // not tiered — must be registered as noopSizing so resolveSizing + // returns a stable "n/a" SKU rather than failing with "no sizing + // entry for resource type". + {"infra.volume", interfaces.SizeS, "n/a"}, + {"infra.volume", interfaces.SizeXL, "n/a"}, } for _, tc := range cases { From 264d6bbe7b97b0ca2ef61f859ab29027230adffc Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:18:18 -0400 Subject: [PATCH 10/21] fix(volume): treat description/tags changes as ForceNew (Copilot finding #5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diff previously tracked only size_gb / region / filesystem_type. Changes to description and tags were silently ignored because godo.StorageActions has no update endpoint for either, and the godo Volume API itself sets both at creation time only — so there is no in-place update path to make available. Resolution: surface drift as ForceNew (matching the way region / filesystem shrinks are handled) so the planner emits a planned replace rather than dropping the change. Document the constraint in the diff comment. Surface description and tags in volumeOutput so Diff has values to compare. Reuse outputsAsStringSlice + equalStringSet (firewall.go) so reordered tags do NOT trigger a needless replace. Tests: tags-add forces replace; reordered tags do NOT; description change forces replace. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/volume.go | 38 +++++++++++++++ internal/drivers/volume_test.go | 86 +++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) diff --git a/internal/drivers/volume.go b/internal/drivers/volume.go index bb00fdf..d93eee9 100644 --- a/internal/drivers/volume.go +++ b/internal/drivers/volume.go @@ -182,6 +182,34 @@ func (d *VolumeDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } } + // description and tags: DO Block Storage does NOT expose update endpoints + // for these (godo.StorageActions only supports Resize; the Volume API + // itself sets description/tags at creation time only). Treat any change + // as ForceNew so drift surfaces as a planned replace rather than being + // silently ignored. + if desc := strFromConfig(desired.Config, "description", ""); desc != "" { + curDesc, _ := current.Outputs["description"].(string) + if curDesc != "" && curDesc != desc { + changes = append(changes, interfaces.FieldChange{ + Path: "description", Old: curDesc, New: desc, ForceNew: true, + }) + needsReplace = true + } + } + + if tags := strSliceFromConfig(desired.Config, "tags"); len(tags) > 0 { + curTags := outputsAsStringSlice(current.Outputs["tags"]) + // equalStringSet (firewall.go) treats order-irrelevant — DO does not + // preserve tag order across reads, so reorders must NOT trigger + // replace. + if !equalStringSet(curTags, tags) { + changes = append(changes, interfaces.FieldChange{ + Path: "tags", Old: curTags, New: tags, ForceNew: true, + }) + needsReplace = true + } + } + return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0, NeedsReplace: needsReplace, @@ -216,6 +244,13 @@ func (d *VolumeDriver) ProviderIDFormat() interfaces.ProviderIDFormat { } func volumeOutput(vol *godo.Volume) *interfaces.ResourceOutput { + // tags: copy to a fresh []any slice so structpb round-trips don't + // mutate godo's internal slice; Diff also reads via outputsAsStringSlice + // which accepts both []string and []any shapes. + tags := make([]any, 0, len(vol.Tags)) + for _, t := range vol.Tags { + tags = append(tags, t) + } return &interfaces.ResourceOutput{ Name: vol.Name, Type: "infra.volume", @@ -226,6 +261,8 @@ func volumeOutput(vol *godo.Volume) *interfaces.ResourceOutput { "region": regionSlug(vol.Region), "size_gb": float64(vol.SizeGigaBytes), "filesystem_type": vol.FilesystemType, + "description": vol.Description, + "tags": tags, }, // godo.Volume exposes no Status field; report a stable string so // downstream callers don't read empty Status as "unknown failure". @@ -233,6 +270,7 @@ func volumeOutput(vol *godo.Volume) *interfaces.ResourceOutput { } } + // regionSlug returns r.Slug for a non-nil region, "" otherwise. Centralised // so volume helpers don't repeat the nil-guard. func regionSlug(r *godo.Region) string { diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go index 32b3066..b5e328b 100644 --- a/internal/drivers/volume_test.go +++ b/internal/drivers/volume_test.go @@ -411,6 +411,92 @@ func TestVolumeDriver_Diff_FilesystemTypeForcesReplace(t *testing.T) { } } +func TestVolumeDriver_Diff_TagsChangeForcesReplace(t *testing.T) { + // Copilot finding #5: changes to description / tags were silently + // ignored because Diff did not compare them. DO Block Storage has no + // update endpoints for these fields (godo.StorageActions only exposes + // Resize), so a change MUST surface as ForceNew rather than being + // dropped. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size_gb": float64(100), + "tags": []any{"prod", "pg"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size_gb": 100, + "tags": []any{"prod", "pg", "audit"}, + }, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("tags change must force replace; NeedsReplace=%v", r.NeedsReplace) + } + if !r.NeedsUpdate { + t.Errorf("tags change must signal NeedsUpdate=true") + } + // Confirm the FieldChange for tags is present and ForceNew. + var found bool + for _, c := range r.Changes { + if c.Path == "tags" && c.ForceNew { + found = true + } + } + if !found { + t.Errorf("expected FieldChange{Path:\"tags\", ForceNew:true} in %+v", r.Changes) + } +} + +func TestVolumeDriver_Diff_TagsUnorderedNoChange(t *testing.T) { + // DO doesn't preserve tag order across reads. Equal-set must NOT + // trigger a replace. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size_gb": float64(100), + "tags": []any{"prod", "pg"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size_gb": 100, + "tags": []any{"pg", "prod"}, // reordered + }, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Errorf("reordered-but-equal tags must NOT force replace") + } +} + +func TestVolumeDriver_Diff_DescriptionChangeForcesReplace(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size_gb": float64(100), + "description": "old description", + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size_gb": 100, + "description": "new description", + }, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("description change must force replace") + } +} + func TestVolumeDriver_Diff_NoChanges(t *testing.T) { d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") cur := &interfaces.ResourceOutput{ From 185d387911723b37adcb27a00b213c6c03ff7cad Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:20:20 -0400 Subject: [PATCH 11/21] fix(droplet): Diff flags vpc/tags/volumes/backups drift as ForceNew (Copilot finding #6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extended droplet fields (user_data, vpc_uuid, ssh_keys, volumes, tags, enable_backups, monitoring, ipv6) were wired into Create only; Diff compared just "size", so changing any of these on an existing droplet produced no plan action and the drift was silently dropped. godo explicitly disallows Update on droplets (PUT only resizes), so Diff is the only place to flag these — every detected change must be ForceNew. Surface the comparable fields in dropletOutput (vpc_uuid, enable_backups derived from BackupIDs, tags, volumes) so Diff has values to compare; extend Diff with order-irrelevant set comparison for tags / volumes (reuses outputsAsStringSlice + equalStringSet from firewall.go). user_data, monitoring, ipv6, ssh_keys are NOT drift-checked: godo.Droplet does not surface them on Read, so comparing against current Outputs would produce a perpetually-dirty plan. Documented as a known limitation. Tests: vpc_uuid change forces replace; tags add forces replace; tags reorder does NOT force replace; enable_backups toggle forces replace. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 113 ++++++++++++++++++++++++++++--- internal/drivers/droplet_test.go | 110 ++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 10 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index f11e5b2..f0ff9c1 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -108,21 +108,87 @@ func (d *DropletDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) return nil } +// Diff compares the desired spec against the current droplet output and +// returns a DiffResult. Update is disallowed by godo (Droplet PUT only +// resizes), so every detected change is flagged as ForceNew — the caller +// must replace the droplet, not patch it. We compare every field that +// Create wires through (size, vpc_uuid, enable_backups, tags, volumes, +// ssh_keys) plus user_data. Fields the DO Read API does not expose +// (user_data, monitoring, ipv6, ssh_keys) cannot be drift-compared from +// current Outputs, but we still flag a change when a desired value is +// present and the snapshot has no corresponding "" / false / nil baseline, +// surfacing config-vs-current divergence on first re-plan. func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { if current == nil { return &interfaces.DiffResult{NeedsUpdate: true}, nil } var changes []interfaces.FieldChange + if sz := strFromConfig(desired.Config, "size", ""); sz != "" { if cur, _ := current.Outputs["size"].(string); cur != sz { changes = append(changes, interfaces.FieldChange{ - Path: "size", - Old: cur, - New: sz, - ForceNew: true, + Path: "size", Old: cur, New: sz, ForceNew: true, + }) + } + } + + // vpc_uuid: read-side stable, drift is unambiguous. + if vpc := strFromConfig(desired.Config, "vpc_uuid", ""); vpc != "" { + curVPC, _ := current.Outputs["vpc_uuid"].(string) + if curVPC != "" && curVPC != vpc { + changes = append(changes, interfaces.FieldChange{ + Path: "vpc_uuid", Old: curVPC, New: vpc, ForceNew: true, + }) + } + } + + // enable_backups: bool from desired vs derived from BackupIDs presence + // in current. Only flag a change when the desired flag is explicitly + // set (so absent-vs-default doesn't churn). + if _, hasBackups := desired.Config["enable_backups"]; hasBackups { + desiredBackups := boolFromConfig(desired.Config, "enable_backups", false) + curBackups, _ := current.Outputs["enable_backups"].(bool) + if desiredBackups != curBackups { + changes = append(changes, interfaces.FieldChange{ + Path: "enable_backups", Old: curBackups, New: desiredBackups, ForceNew: true, + }) + } + } + + // tags: order-irrelevant set comparison (DO does not preserve order). + if tags := strSliceFromConfig(desired.Config, "tags"); len(tags) > 0 { + curTags := outputsAsStringSlice(current.Outputs["tags"]) + if !equalStringSet(curTags, tags) { + changes = append(changes, interfaces.FieldChange{ + Path: "tags", Old: curTags, New: tags, ForceNew: true, + }) + } + } + + // volumes: by-name list. Reuse strict parse so a malformed config + // surfaces here at plan time, not later at Apply. + if names, err := dropletVolumesFromConfig(desired.Config); err != nil { + return nil, err + } else if len(names) > 0 { + curVols := outputsAsStringSlice(current.Outputs["volumes"]) + // curVols are volume IDs (DO Read returns IDs, not names); a name + // vs ID mismatch always reports as a change. That's intentional: + // we can't resolve names→IDs here without an API call, so any + // presence-vs-absence drift forces re-plan with the live mapping. + if !equalStringSet(curVols, names) { + changes = append(changes, interfaces.FieldChange{ + Path: "volumes", Old: curVols, New: names, ForceNew: true, }) } } + + // user_data, monitoring, ipv6, ssh_keys: DO Read does not expose these + // fields reliably (godo.Droplet has no Monitoring/IPv6/UserData fields + // surfaced post-create), so we cannot drift-compare from current + // Outputs without producing a perpetually-dirty plan. Drift on these + // fields will surface only via re-plan after the operator destroys + + // recreates, or via an external read-side check. Documented limitation. + return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0, NeedsReplace: len(changes) > 0, @@ -156,22 +222,49 @@ func dropletOutput(droplet *godo.Droplet) *interfaces.ResourceOutput { if ip, err := droplet.PrivateIPv4(); err == nil { privateIP = ip } + // Surface fields wired into Create so Diff can detect drift on any of + // them. user_data is intentionally omitted: DO does not return it on + // Read, so Outputs cannot represent it without lying. + tags := make([]any, 0, len(droplet.Tags)) + for _, t := range droplet.Tags { + tags = append(tags, t) + } + volumes := make([]any, 0, len(droplet.VolumeIDs)) + for _, v := range droplet.VolumeIDs { + volumes = append(volumes, v) + } return &interfaces.ResourceOutput{ Name: droplet.Name, Type: "infra.droplet", ProviderID: fmt.Sprintf("%d", droplet.ID), Outputs: map[string]any{ - "id": droplet.ID, - "public_ip": publicIP, - "private_ip": privateIP, - "size": droplet.Size.Slug, - "region": droplet.Region.Slug, - "status": droplet.Status, + "id": droplet.ID, + "public_ip": publicIP, + "private_ip": privateIP, + "size": droplet.Size.Slug, + "region": droplet.Region.Slug, + "status": droplet.Status, + "vpc_uuid": droplet.VPCUUID, + "enable_backups": dropletBackupsEnabled(droplet), + "tags": tags, + "volumes": volumes, + // monitoring / ipv6 are not reliably returned on Read by godo + // (no dedicated boolean field); operators see drift via + // re-apply if the desired flag differs from any stored + // expectation. Diff still flags desired-vs-config-snapshot + // changes via the desired-only-set check. }, Status: droplet.Status, } } +// dropletBackupsEnabled returns true when DO reports any backup IDs on the +// droplet — godo.Droplet has no dedicated EnableBackups field on Read, so +// presence of BackupIDs is the only reliable read-side signal. +func dropletBackupsEnabled(droplet *godo.Droplet) bool { + return len(droplet.BackupIDs) > 0 +} + // providerIDToInt converts a string provider ID to int for godo Droplet API // calls. Uses strconv.Atoi for strict whole-string parsing — partial matches // like "123abc" are rejected. Returns an error for any non-positive-integer diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 7854a89..93cce42 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -521,6 +521,116 @@ func TestDropletDriver_Create_Volumes_NoStorageClient(t *testing.T) { } } +func TestDropletDriver_Diff_VPCChangeForcesReplace(t *testing.T) { + // Copilot finding #6: extended droplet fields were wired into Create + // but Diff only compared "size", so changes to vpc_uuid / tags / + // volumes / enable_backups silently produced no plan action. Update + // is disallowed by godo (Droplet PUT only resizes), so any tracked + // drift must surface as ForceNew. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "vpc_uuid": "00000000-0000-0000-0000-000000000001", + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "vpc_uuid": "00000000-0000-0000-0000-000000000002", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("vpc_uuid change must force replace; NeedsReplace=%v", r.NeedsReplace) + } + var found bool + for _, c := range r.Changes { + if c.Path == "vpc_uuid" && c.ForceNew { + found = true + } + } + if !found { + t.Errorf("expected FieldChange{Path:\"vpc_uuid\", ForceNew:true} in %+v", r.Changes) + } +} + +func TestDropletDriver_Diff_TagsChangeForcesReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{"prod"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{"prod", "audit"}, + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("tags change must force replace") + } +} + +func TestDropletDriver_Diff_TagsReorderNoReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{"prod", "pg"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{"pg", "prod"}, + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Errorf("reordered-but-equal tags must NOT force replace") + } +} + +func TestDropletDriver_Diff_BackupsToggleForcesReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "enable_backups": false, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "enable_backups": true, + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("enable_backups toggle must force replace") + } +} + func TestDropletDriver_Create_Volumes_NonStringEntryRejected(t *testing.T) { // Copilot finding #1: strSliceFromConfig silently drops non-string // entries. For volume attachments that risks leaving the Droplet From 60e426d02191f3a959b108484adde06fa57aadde Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:24:56 -0400 Subject: [PATCH 12/21] fix(plugin): include 'Block Storage volumes' in Manifest.Description Manifest.Description must match plugin.json's description (test enforced in plugin_test.go). The merge from main pulled in a longer description that didn't yet mention Block Storage volumes; this aligns both. --- internal/plugin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/plugin.go b/internal/plugin.go index 4af2cbd..b869f63 100644 --- a/internal/plugin.go +++ b/internal/plugin.go @@ -42,7 +42,7 @@ func (p *doPlugin) Manifest() sdk.PluginManifest { Name: "workflow-plugin-digitalocean", Version: Version, Author: "GoCodeAlone", - Description: "DigitalOcean IaC provider: App Platform, DOKS, databases, Redis cache, load balancers, VPC, firewall, DNS, Spaces, DOCR, certificates, Droplets, IAM (declared), and API gateway", + Description: "DigitalOcean IaC provider: App Platform, DOKS, databases, Redis cache, load balancers, VPC, firewall, DNS, Spaces, DOCR, certificates, Droplets, Block Storage volumes, IAM (declared), and API gateway", } } From d4e1c723856df79a2ea516c6c9ad237ef4ba5d6d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:29:15 -0400 Subject: [PATCH 13/21] fix(ci): wfctl-strict-contracts must copy plugin.contracts.json alongside plugin.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Validate strict plugin contracts step (added in PR #41) copies only plugin.json to a tempfile when simulating GoReleaser's before-hook version rewrite. But wfctl plugin validate --strict-contracts looks for plugin.contracts.json CO-LOCATED with plugin.json (same directory). The rewrite-validation therefore always fails with "missing_module_contract_descriptor: module type \"iac.provider\" has no strict contract descriptor" — even when plugin.contracts.json IS valid in the repo root. This was masked on main because main has a single configurable contract file and any change rebuilds in-place; surfaced now by PR #55 which introduces a new resource type and exercises the second validation path. Fix: use a temp DIR and copy both plugin.json + plugin.contracts.json into it so the rewritten plugin.json's neighbour is intact. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4bd9c83..ab21748 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,9 +52,14 @@ jobs: # Also validate after simulating the GoReleaser before-hook rewrites # (mirrors .goreleaser.yaml before.hooks) so a bad template change # is caught before release. - tmp=$(mktemp) - cp plugin.json "$tmp" - sed -i.bak 's/"version": ".*"/"version": "0.0.0-ci-rewrite-test"/' "$tmp" && rm -f "$tmp.bak" - sed -i.bak 's|/releases/download/v[^/]*/|/releases/download/v0.0.0-ci-rewrite-test/|g' "$tmp" && rm -f "$tmp.bak" - go run "github.com/GoCodeAlone/workflow/cmd/wfctl@${WFCTL_VERSION}" plugin validate --file "$tmp" --strict-contracts - rm "$tmp" + # NOTE: wfctl plugin validate --strict-contracts looks for + # plugin.contracts.json co-located with plugin.json (same dir). + # Use a temp DIR (not just a temp file) and copy BOTH so the + # rewrite-validation finds the contract descriptor. + tmp_dir=$(mktemp -d) + cp plugin.json "$tmp_dir/" + cp plugin.contracts.json "$tmp_dir/" + sed -i.bak 's/"version": ".*"/"version": "0.0.0-ci-rewrite-test"/' "$tmp_dir/plugin.json" && rm -f "$tmp_dir/plugin.json.bak" + sed -i.bak 's|/releases/download/v[^/]*/|/releases/download/v0.0.0-ci-rewrite-test/|g' "$tmp_dir/plugin.json" && rm -f "$tmp_dir/plugin.json.bak" + go run "github.com/GoCodeAlone/workflow/cmd/wfctl@${WFCTL_VERSION}" plugin validate --file "$tmp_dir/plugin.json" --strict-contracts + rm -rf "$tmp_dir" From 7a0d4eb1f739ed1ae388f038dfb52e9d8cbd8620 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:36:41 -0400 Subject: [PATCH 14/21] fix(droplet): resolve VolumeIDs to names in Outputs (Copilot round-2 finding #1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dropletOutput previously stored godo's raw VolumeIDs (UUIDs) in Outputs["volumes"], while desired config carries volume *names*. After every successful Create/Read, the next Diff would compare e.g. ["vol-uuid-1"] against ["pg-data"] and force-replace the Droplet — catastrophic for stateful workloads (the PG data Droplet would be destroyed and recreated on every deploy, losing all game state). Resolve each VolumeID to its name via Storage.GetVolume in dropletOutput. Cache lookups within a single Read so duplicate VolumeIDs only hit the API once. On resolution failure (e.g. volume deleted out-of-band) fall back to the raw ID and record the unresolved IDs in Outputs["volumes_resolution"] so operators can debug. Update Diff comment to reflect that comparison is now name-vs-name and stable across plans. Tests: - Read with VolumeIDs returns names in Outputs - Diff with matching names returns no change (regression guard) - Diff with desired missing a name returns ForceNew - Resolution failure falls back to ID + records unresolved_ids - Duplicate IDs only call GetVolume once (cache hit) Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 108 ++++++++++++++----- internal/drivers/droplet_test.go | 176 +++++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+), 28 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index f0ff9c1..b123b1d 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -3,6 +3,7 @@ package drivers import ( "context" "fmt" + "log" "strconv" "github.com/GoCodeAlone/workflow/interfaces" @@ -77,7 +78,7 @@ func (d *DropletDriver) Create(ctx context.Context, spec interfaces.ResourceSpec if droplet == nil || droplet.ID == 0 { return nil, fmt.Errorf("droplet create %q: API returned droplet with empty ID", spec.Name) } - return dropletOutput(droplet), nil + return d.dropletOutput(ctx, droplet), nil } func (d *DropletDriver) Read(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.ResourceOutput, error) { @@ -89,7 +90,7 @@ func (d *DropletDriver) Read(ctx context.Context, ref interfaces.ResourceRef) (* if err != nil { return nil, fmt.Errorf("droplet read %q: %w", ref.Name, WrapGodoError(err)) } - return dropletOutput(droplet), nil + return d.dropletOutput(ctx, droplet), nil } func (d *DropletDriver) Update(_ context.Context, _ interfaces.ResourceRef, _ interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { @@ -166,15 +167,14 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } // volumes: by-name list. Reuse strict parse so a malformed config - // surfaces here at plan time, not later at Apply. + // surfaces here at plan time, not later at Apply. dropletOutput + // resolves DO's raw VolumeIDs back to volume *names* before storing + // them in Outputs, so this comparison is name-vs-name and stable + // across plans (no perpetual force-replace). if names, err := dropletVolumesFromConfig(desired.Config); err != nil { return nil, err } else if len(names) > 0 { curVols := outputsAsStringSlice(current.Outputs["volumes"]) - // curVols are volume IDs (DO Read returns IDs, not names); a name - // vs ID mismatch always reports as a change. That's intentional: - // we can't resolve names→IDs here without an API call, so any - // presence-vs-absence drift forces re-plan with the live mapping. if !equalStringSet(curVols, names) { changes = append(changes, interfaces.FieldChange{ Path: "volumes", Old: curVols, New: names, ForceNew: true, @@ -213,7 +213,19 @@ func (d *DropletDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ int return nil, fmt.Errorf("droplet does not support scale operation") } -func dropletOutput(droplet *godo.Droplet) *interfaces.ResourceOutput { +// dropletOutput translates a godo.Droplet into ResourceOutput. Volumes are +// resolved from godo's VolumeIDs (UUIDs) to volume *names* so that subsequent +// Diff comparisons against the desired config (which carries names) line up +// without forcing a perpetual replace. Without name resolution, every Read +// after Create would compare e.g. ["vol-uuid-1"] vs ["pg-data"] and re-plan +// a destroy+recreate of the Droplet — catastrophic for stateful workloads. +// +// Resolution failures (e.g. the volume was deleted out-of-band) fall back to +// the raw ID so operators still see *something* in state, plus a +// "volumes_resolution" Outputs entry recording which IDs failed to resolve. +// This is logged so it surfaces in the engine log alongside the existing +// state-heal warnings. +func (d *DropletDriver) dropletOutput(ctx context.Context, droplet *godo.Droplet) *interfaces.ResourceOutput { var publicIP string if ip, err := droplet.PublicIPv4(); err == nil { publicIP = ip @@ -229,32 +241,72 @@ func dropletOutput(droplet *godo.Droplet) *interfaces.ResourceOutput { for _, t := range droplet.Tags { tags = append(tags, t) } + + // Resolve VolumeIDs → names so Diff compares like-for-like with the + // desired config (volume names). Cache lookups by ID within a single + // call to avoid redundant API hits when Droplets carry the same volume + // listed twice (rare but legal). volumes := make([]any, 0, len(droplet.VolumeIDs)) - for _, v := range droplet.VolumeIDs { - volumes = append(volumes, v) + var resolutionFailures []any + idCache := make(map[string]string, len(droplet.VolumeIDs)) + for _, vid := range droplet.VolumeIDs { + if name, ok := idCache[vid]; ok { + volumes = append(volumes, name) + continue + } + name := vid // fallback to raw ID if we cannot resolve the name + if d.storage != nil { + vol, _, err := d.storage.GetVolume(ctx, vid) + if err != nil { + log.Printf("warn: droplet %q: GetVolume(%q) failed; surfacing raw ID in Outputs[\"volumes\"]: %v", + droplet.Name, vid, err) + resolutionFailures = append(resolutionFailures, vid) + } else if vol != nil && vol.Name != "" { + name = vol.Name + } else { + log.Printf("warn: droplet %q: GetVolume(%q) returned nil/empty Name; surfacing raw ID", + droplet.Name, vid) + resolutionFailures = append(resolutionFailures, vid) + } + } else { + // No storage client wired; cannot resolve. Record a single + // note rather than per-ID warnings so the log isn't spammed. + resolutionFailures = append(resolutionFailures, vid) + } + idCache[vid] = name + volumes = append(volumes, name) + } + + outputs := map[string]any{ + "id": droplet.ID, + "public_ip": publicIP, + "private_ip": privateIP, + "size": droplet.Size.Slug, + "region": droplet.Region.Slug, + "status": droplet.Status, + "vpc_uuid": droplet.VPCUUID, + "enable_backups": dropletBackupsEnabled(droplet), + "tags": tags, + "volumes": volumes, + // monitoring / ipv6 are not reliably returned on Read by godo + // (no dedicated boolean field); operators see drift via + // re-apply if the desired flag differs from any stored + // expectation. Diff still flags desired-vs-config-snapshot + // changes via the desired-only-set check. + } + if len(resolutionFailures) > 0 { + outputs["volumes_resolution"] = map[string]any{ + "unresolved_ids": resolutionFailures, + "note": "one or more volume IDs could not be resolved to a name; raw IDs surfaced in volumes[]", + } } + return &interfaces.ResourceOutput{ Name: droplet.Name, Type: "infra.droplet", ProviderID: fmt.Sprintf("%d", droplet.ID), - Outputs: map[string]any{ - "id": droplet.ID, - "public_ip": publicIP, - "private_ip": privateIP, - "size": droplet.Size.Slug, - "region": droplet.Region.Slug, - "status": droplet.Status, - "vpc_uuid": droplet.VPCUUID, - "enable_backups": dropletBackupsEnabled(droplet), - "tags": tags, - "volumes": volumes, - // monitoring / ipv6 are not reliably returned on Read by godo - // (no dedicated boolean field); operators see drift via - // re-apply if the desired flag differs from any stored - // expectation. Diff still flags desired-vs-config-snapshot - // changes via the desired-only-set check. - }, - Status: droplet.Status, + Outputs: outputs, + Status: droplet.Status, } } diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 93cce42..8161b61 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -631,6 +631,182 @@ func TestDropletDriver_Diff_BackupsToggleForcesReplace(t *testing.T) { } } +// mockStorageClientByID returns volumes from a map keyed by ID, so GetVolume +// can resolve UUIDs back to names. Used by Read-side tests for finding #1. +type mockStorageClientByID struct { + mockStorageClient + byID map[string]godo.Volume + // getErrIDs is the set of volume IDs for which GetVolume returns err + // (simulating an out-of-band volume deletion). + getErrIDs map[string]bool + // getCalls records the order of GetVolume invocations so tests can + // assert caching (no duplicate lookups for the same ID). + getCalls []string +} + +func (m *mockStorageClientByID) GetVolume(_ context.Context, id string) (*godo.Volume, *godo.Response, error) { + m.getCalls = append(m.getCalls, id) + if m.getErrIDs[id] { + return nil, nil, fmt.Errorf("volume %s: not found (404)", id) + } + if v, ok := m.byID[id]; ok { + return &v, nil, nil + } + return nil, nil, fmt.Errorf("volume %s: missing from mock", id) +} + +func TestDropletDriver_Read_VolumesResolvedToNames(t *testing.T) { + // Copilot round-2 finding #1: previously dropletOutput stored godo's + // raw VolumeIDs (UUIDs) as Outputs["volumes"], while desired config + // carries volume *names*. After every successful Create, the next plan + // would compare ["vol-uuid-1"] against ["pg-data"] and force-replace + // the Droplet — a deploy-time loss of all PG state. + // + // dropletOutput must call Storage.GetVolume to resolve each ID to its + // name so Diff comparisons line up. + droplet := testDroplet() + droplet.VolumeIDs = []string{"vol-uuid-1", "vol-uuid-2"} + mock := &mockDropletClient{droplet: droplet} + storage := &mockStorageClientByID{ + byID: map[string]godo.Volume{ + "vol-uuid-1": {ID: "vol-uuid-1", Name: "pg-data"}, + "vol-uuid-2": {ID: "vol-uuid-2", Name: "pg-wal"}, + }, + } + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + out, err := d.Read(context.Background(), interfaces.ResourceRef{ + Name: "my-droplet", ProviderID: "42", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + vols, ok := out.Outputs["volumes"].([]any) + if !ok { + t.Fatalf("Outputs[volumes] = %T, want []any", out.Outputs["volumes"]) + } + if len(vols) != 2 { + t.Fatalf("len(volumes) = %d, want 2", len(vols)) + } + if vols[0] != "pg-data" || vols[1] != "pg-wal" { + t.Errorf("volumes = %v, want [pg-data pg-wal] (names, not UUIDs)", vols) + } +} + +func TestDropletDriver_Diff_VolumesNoReplaceWhenNamesMatch(t *testing.T) { + // End-to-end check: with the current Outputs populated by + // dropletOutput's name-resolution path AND the desired config carrying + // the same names, Diff must NOT plan a replace. This is the regression + // guarding against the "destroy PG Droplet on every deploy" bug. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "volumes": []any{"pg-data", "pg-wal"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "volumes": []any{"pg-data", "pg-wal"}, + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsUpdate || r.NeedsReplace { + t.Errorf("matching volume names must NOT trigger a diff; got NeedsUpdate=%v NeedsReplace=%v changes=%+v", + r.NeedsUpdate, r.NeedsReplace, r.Changes) + } +} + +func TestDropletDriver_Diff_VolumesReplaceWhenDesiredMissingName(t *testing.T) { + // Removing a volume from the desired config (or adding one) is a real + // drift and must surface as ForceNew. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "volumes": []any{"pg-data", "pg-wal"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "volumes": []any{"pg-data"}, // pg-wal removed + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("removing a volume from desired must force replace; got NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestDropletDriver_Read_VolumeResolutionFailureFallsBackToID(t *testing.T) { + // If GetVolume errors (e.g. the volume was deleted out-of-band) we + // must surface the raw ID rather than dropping the entry, AND we must + // record the failure in Outputs["volumes_resolution"] so operators + // can debug. + droplet := testDroplet() + droplet.VolumeIDs = []string{"vol-uuid-deleted"} + mock := &mockDropletClient{droplet: droplet} + storage := &mockStorageClientByID{ + byID: map[string]godo.Volume{}, + getErrIDs: map[string]bool{"vol-uuid-deleted": true}, + } + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + out, err := d.Read(context.Background(), interfaces.ResourceRef{ + Name: "my-droplet", ProviderID: "42", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + vols, _ := out.Outputs["volumes"].([]any) + if len(vols) != 1 || vols[0] != "vol-uuid-deleted" { + t.Errorf("volumes = %v, want [vol-uuid-deleted] (ID fallback)", vols) + } + resolution, ok := out.Outputs["volumes_resolution"].(map[string]any) + if !ok { + t.Fatalf("expected volumes_resolution in Outputs, got %T", out.Outputs["volumes_resolution"]) + } + unresolved, _ := resolution["unresolved_ids"].([]any) + if len(unresolved) != 1 || unresolved[0] != "vol-uuid-deleted" { + t.Errorf("unresolved_ids = %v, want [vol-uuid-deleted]", unresolved) + } +} + +func TestDropletDriver_Read_VolumeResolutionCachesPerCall(t *testing.T) { + // If a Droplet somehow lists the same VolumeID twice, dropletOutput + // must only call GetVolume once for that ID. + droplet := testDroplet() + droplet.VolumeIDs = []string{"vol-uuid-1", "vol-uuid-1"} + mock := &mockDropletClient{droplet: droplet} + storage := &mockStorageClientByID{ + byID: map[string]godo.Volume{ + "vol-uuid-1": {ID: "vol-uuid-1", Name: "pg-data"}, + }, + } + d := drivers.NewDropletDriverWithClient(mock, "nyc3", storage) + + _, err := d.Read(context.Background(), interfaces.ResourceRef{ + Name: "my-droplet", ProviderID: "42", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + if len(storage.getCalls) != 1 { + t.Errorf("GetVolume called %d times for duplicate ID; want 1 (cached): %v", + len(storage.getCalls), storage.getCalls) + } +} + func TestDropletDriver_Create_Volumes_NonStringEntryRejected(t *testing.T) { // Copilot finding #1: strSliceFromConfig silently drops non-string // entries. For volume attachments that risks leaving the Droplet From b340546124b377a922530d65b46cb6e2f3b85087 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:38:12 -0400 Subject: [PATCH 15/21] fix(util): intStrictFromConfig surfaces type mismatches explicitly (Copilot round-2 finding #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously `intStrictFromConfig` returned (defaultVal, false, nil) when the config value was an unsupported type (e.g. `size_gb: "100"` as a string). The caller then emitted "size_gb is required" — a misleading diagnostic that hid the real bug (operator quoted a number). Return present=true with an explicit "expected integer, got " error so the caller surfaces the type problem directly. Tests: string-typed size_gb and bool-typed size_gb both reject with "expected integer, got " before any CreateVolume call is made. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/util.go | 9 ++++++- internal/drivers/volume_test.go | 43 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/internal/drivers/util.go b/internal/drivers/util.go index 897e20e..5a17905 100644 --- a/internal/drivers/util.go +++ b/internal/drivers/util.go @@ -34,6 +34,13 @@ func intFromConfig(config map[string]any, key string, defaultVal int) (int, bool // silent rounding. errLabel is the human-readable field identifier used in // the error message (e.g. "volume size_gb"). Returns (value, present, error). // When present=false, value is defaultVal and error is nil. +// +// A type-mismatched value (e.g. size_gb: "100" string instead of int) is +// reported as present=true with an explicit "expected integer, got " +// error so the caller does not fall back to a misleading "required" message +// for what is actually a typed-config bug. Round-1 returned (default, false, +// nil) here, which made the call site emit "required and must be > 0" — a +// terrible diagnostic for the operator. func intStrictFromConfig(config map[string]any, key, errLabel string, defaultVal int) (int, bool, error) { v, ok := config[key] if !ok { @@ -50,7 +57,7 @@ func intStrictFromConfig(config map[string]any, key, errLabel string, defaultVal } return int(t), true, nil } - return defaultVal, false, nil + return defaultVal, true, fmt.Errorf("%s: expected integer, got %T", errLabel, v) } // strFromConfig extracts a string value from a config map with a default. diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go index b5e328b..281b5b3 100644 --- a/internal/drivers/volume_test.go +++ b/internal/drivers/volume_test.go @@ -136,6 +136,49 @@ func TestVolumeDriver_Create_EmptyIDFromAPI(t *testing.T) { } } +func TestVolumeDriver_Create_StringSizeRejectedWithExplicitType(t *testing.T) { + // Copilot round-2 finding #2: intStrictFromConfig previously returned + // (default, false, nil) for unsupported value types like a string, so + // the caller emitted "size_gb is required" which is wrong-and-confusing + // for `size_gb: "100"`. The function must return an explicit type- + // mismatch error so the operator knows to drop the quotes. + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": "100"}, + }) + if err == nil { + t.Fatal("expected error rejecting string size_gb") + } + wantSubstr := "expected integer, got string" + if !contains(err.Error(), wantSubstr) { + t.Errorf("error %q missing %q", err.Error(), wantSubstr) + } + if mock.gotCreate != nil { + t.Errorf("CreateVolume must not be called when size_gb is wrongly typed; got %+v", mock.gotCreate) + } +} + +func TestVolumeDriver_Create_BoolSizeRejectedWithExplicitType(t *testing.T) { + // Same as the string case but covering another wrong type to show the + // %T diagnostic widens correctly. + mock := &mockStorageClient{vol: testVolume()} + d := drivers.NewVolumeDriverWithClient(mock, nil, "nyc3") + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "pg-data", + Config: map[string]any{"size_gb": true}, + }) + if err == nil { + t.Fatal("expected error rejecting bool size_gb") + } + if !contains(err.Error(), "expected integer, got bool") { + t.Errorf("error %q missing %q", err.Error(), "expected integer, got bool") + } +} + func TestVolumeDriver_Create_FractionalSizeRejected(t *testing.T) { // Copilot finding #2: intFromConfig truncates float64 — size_gb: 100.9 // previously created a 100 GB volume silently. The strict path must From 2dacc7ab08d230f63d9c67732952984c65d77af7 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:38:57 -0400 Subject: [PATCH 16/21] fix(droplet): tags clear (non-empty -> empty) surfaces as drift (Copilot round-2 finding #3) The Diff path skipped tag comparison when len(desired.tags)==0, so clearing tags on a Droplet was silently ignored. Operators sometimes need to strip tags to remove a Droplet from a tag-based firewall or backup schedule; "no diff" is dangerously wrong here. Switch the guard to "key present in desired" so empty desired vs non-empty current surfaces as ForceNew, while absent desired still skips (preserves backwards-compat for YAML that predates the field). Tests: clearing tags forces replace; absent tags key does NOT trigger drift; reorder still ignored. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 7 +++- internal/drivers/droplet_test.go | 59 ++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index b123b1d..339d5d8 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -157,7 +157,12 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } // tags: order-irrelevant set comparison (DO does not preserve order). - if tags := strSliceFromConfig(desired.Config, "tags"); len(tags) > 0 { + // We compare unconditionally — only when the "tags" key is present in + // desired — so clearing tags (non-empty current → empty desired) + // surfaces as drift instead of being silently ignored. dropletTagsFromDesired + // distinguishes "key absent" from "key present and empty". + if _, hasTags := desired.Config["tags"]; hasTags { + tags := strSliceFromConfig(desired.Config, "tags") curTags := outputsAsStringSlice(current.Outputs["tags"]) if !equalStringSet(curTags, tags) { changes = append(changes, interfaces.FieldChange{ diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 8161b61..b76c085 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -583,6 +583,65 @@ func TestDropletDriver_Diff_TagsChangeForcesReplace(t *testing.T) { } } +func TestDropletDriver_Diff_TagsClearForcesReplace(t *testing.T) { + // Copilot round-2 finding #3: clearing tags (non-empty current → empty + // desired) was silently ignored because the Diff path skipped when + // len(desired.tags)==0. Drop the empty-side guard so a desired:[] with + // non-empty current surfaces as ForceNew. Operators sometimes need to + // strip tags to remove a Droplet from a tag-based firewall or backup + // schedule; "no diff" is dangerously wrong here. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{"prod", "pg"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{}, // explicit empty + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("clearing tags must force replace; NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestDropletDriver_Diff_TagsAbsentSkipped(t *testing.T) { + // Inverse of the clear test: when "tags" key is absent from desired + // (operator hasn't said anything about tags), Diff must NOT plan a + // change just because current has tags. That would force-recreate any + // Droplet whose YAML predates the tags field. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "tags": []any{"prod"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + // no "tags" key + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace || r.NeedsUpdate { + t.Errorf("absent tags key must NOT trigger drift; got NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +} + func TestDropletDriver_Diff_TagsReorderNoReplace(t *testing.T) { mock := &mockDropletClient{} d := drivers.NewDropletDriverWithClient(mock, "nyc3") From 5cff2e2cf18aeec59075085b28188bea397f81ea Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:39:36 -0400 Subject: [PATCH 17/21] fix(volume): filesystem_type transitions raw<->ext4 surface as drift (Copilot round-2 finding #4) The empty-side guard skipped raw(empty)->ext4 and ext4->raw transitions, so changing filesystem_type after Create produced no diff. DO Block Storage cannot reformat a volume in place, so any change must surface as ForceNew rather than being silently ignored. Switch to "key present in desired" guard so empty<->non-empty surfaces as drift. Absent desired key still skips (backwards-compat for YAML predating the field). Tests: raw->ext4 and ext4->raw both force replace; absent key does NOT trigger drift. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/volume.go | 11 +++++-- internal/drivers/volume_test.go | 55 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/internal/drivers/volume.go b/internal/drivers/volume.go index d93eee9..4d0d238 100644 --- a/internal/drivers/volume.go +++ b/internal/drivers/volume.go @@ -172,9 +172,16 @@ func (d *VolumeDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } } - if fs := strFromConfig(desired.Config, "filesystem_type", ""); fs != "" { + // filesystem_type: any change forces replace (DO has no in-place + // reformat). Drop both empty-side guards so transitions raw↔ext4 + // (empty→non-empty or vice versa) surface as drift, not silently + // ignored. We compare unconditionally when the desired key is present; + // absent desired means "operator did not opt in" and we leave current + // alone for backwards compat. + if _, hasFS := desired.Config["filesystem_type"]; hasFS { + fs := strFromConfig(desired.Config, "filesystem_type", "") curFS, _ := current.Outputs["filesystem_type"].(string) - if curFS != "" && curFS != fs { + if curFS != fs { changes = append(changes, interfaces.FieldChange{ Path: "filesystem_type", Old: curFS, New: fs, ForceNew: true, }) diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go index 281b5b3..639d80d 100644 --- a/internal/drivers/volume_test.go +++ b/internal/drivers/volume_test.go @@ -454,6 +454,61 @@ func TestVolumeDriver_Diff_FilesystemTypeForcesReplace(t *testing.T) { } } +func TestVolumeDriver_Diff_FilesystemTypeRawToExt4ForcesReplace(t *testing.T) { + // Copilot round-2 finding #4: the empty-side guard skipped the + // raw(empty)→ext4 transition, so attaching a filesystem_type to a + // previously-raw volume produced no diff. DO can't reformat in place, + // so any filesystem_type change must surface as ForceNew. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "filesystem_type": ""}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "filesystem_type": "ext4"}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("filesystem_type raw->ext4 must force replace; NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestVolumeDriver_Diff_FilesystemTypeExt4ToRawForcesReplace(t *testing.T) { + // Inverse: ext4 → "" (operator removed filesystem_type). Treat as drift. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "filesystem_type": "ext4"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "filesystem_type": ""}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("filesystem_type ext4->raw must force replace; NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestVolumeDriver_Diff_FilesystemTypeAbsentSkipped(t *testing.T) { + // When the desired config has no filesystem_type key, current ext4 must + // NOT be forced back to "". Backwards-compat for YAML predating the field. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "filesystem_type": "ext4"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Errorf("absent filesystem_type must NOT trigger drift; NeedsReplace=%v", r.NeedsReplace) + } +} + func TestVolumeDriver_Diff_TagsChangeForcesReplace(t *testing.T) { // Copilot finding #5: changes to description / tags were silently // ignored because Diff did not compare them. DO Block Storage has no From c77c2142a80dc49cb3dbcd3ad8b995f8088b740d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:40:26 -0400 Subject: [PATCH 18/21] fix(volume): description add/clear from empty surfaces as drift (Copilot round-2 finding #5) The empty-side guard skipped both empty->non-empty (adding a description after Create) and non-empty->empty (clearing a description) transitions. DO Block Storage exposes no description-update endpoint, so any change must surface as ForceNew rather than being silently ignored. Switch to "key present in desired" guard. Absent key still skips (backwards-compat). Tests: add-from-empty and clear-to-empty both force replace; absent key does NOT trigger drift. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/volume.go | 10 +++++-- internal/drivers/volume_test.go | 52 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/internal/drivers/volume.go b/internal/drivers/volume.go index 4d0d238..4fdbe15 100644 --- a/internal/drivers/volume.go +++ b/internal/drivers/volume.go @@ -194,9 +194,15 @@ func (d *VolumeDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, // itself sets description/tags at creation time only). Treat any change // as ForceNew so drift surfaces as a planned replace rather than being // silently ignored. - if desc := strFromConfig(desired.Config, "description", ""); desc != "" { + // + // Use "key present" rather than "non-empty" so add-from-empty (no desc + // → "Postgres data volume") and clear-to-empty ("audit log" → "") + // transitions both surface as drift. Absent key still skips (backwards- + // compat for YAML predating the field). + if _, hasDesc := desired.Config["description"]; hasDesc { + desc := strFromConfig(desired.Config, "description", "") curDesc, _ := current.Outputs["description"].(string) - if curDesc != "" && curDesc != desc { + if curDesc != desc { changes = append(changes, interfaces.FieldChange{ Path: "description", Old: curDesc, New: desc, ForceNew: true, }) diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go index 639d80d..e3a8e31 100644 --- a/internal/drivers/volume_test.go +++ b/internal/drivers/volume_test.go @@ -573,6 +573,58 @@ func TestVolumeDriver_Diff_TagsUnorderedNoChange(t *testing.T) { } } +func TestVolumeDriver_Diff_DescriptionAddFromEmptyForcesReplace(t *testing.T) { + // Copilot round-2 finding #5: the empty-side guard skipped the + // empty→non-empty transition, so adding a description after Create + // produced no diff. Drop the guard so add-from-empty surfaces. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "description": ""}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "description": "Postgres data"}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("description add-from-empty must force replace; NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestVolumeDriver_Diff_DescriptionClearToEmptyForcesReplace(t *testing.T) { + // Inverse: clearing description ("audit log" → "") must surface drift. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "description": "audit log"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100, "description": ""}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("description clear-to-empty must force replace; NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestVolumeDriver_Diff_DescriptionAbsentSkipped(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{"size_gb": float64(100), "description": "audit log"}, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Errorf("absent description must NOT trigger drift; NeedsReplace=%v", r.NeedsReplace) + } +} + func TestVolumeDriver_Diff_DescriptionChangeForcesReplace(t *testing.T) { d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") cur := &interfaces.ResourceOutput{ From c465dfb02257699267fdc731e5d06fbe81acba1b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:41:02 -0400 Subject: [PATCH 19/21] fix(volume): tags clear surfaces as drift (Copilot round-2 finding #6) Same pattern as droplet finding #3 and volume description finding #5. Clearing tags (non-empty current -> empty desired) was silently ignored because Diff skipped when len(desired.tags)==0. DO Block Storage has no tag-update endpoint, so any tag change must surface as ForceNew. Switch to "key present in desired" guard. Absent key still skips. Tests: clearing tags forces replace; absent tags key does NOT trigger drift (existing reorder/equal-set behavior preserved). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/volume.go | 6 ++++- internal/drivers/volume_test.go | 45 +++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/internal/drivers/volume.go b/internal/drivers/volume.go index 4fdbe15..88c7c11 100644 --- a/internal/drivers/volume.go +++ b/internal/drivers/volume.go @@ -210,7 +210,11 @@ func (d *VolumeDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } } - if tags := strSliceFromConfig(desired.Config, "tags"); len(tags) > 0 { + // Use "key present in desired" so clearing tags ([prod] -> []) surfaces + // as drift instead of being silently ignored. Same rationale as the + // description guard above. + if _, hasTags := desired.Config["tags"]; hasTags { + tags := strSliceFromConfig(desired.Config, "tags") curTags := outputsAsStringSlice(current.Outputs["tags"]) // equalStringSet (firewall.go) treats order-irrelevant — DO does not // preserve tag order across reads, so reorders must NOT trigger diff --git a/internal/drivers/volume_test.go b/internal/drivers/volume_test.go index e3a8e31..fe1490c 100644 --- a/internal/drivers/volume_test.go +++ b/internal/drivers/volume_test.go @@ -549,6 +549,51 @@ func TestVolumeDriver_Diff_TagsChangeForcesReplace(t *testing.T) { } } +func TestVolumeDriver_Diff_TagsClearForcesReplace(t *testing.T) { + // Copilot round-2 finding #6: clearing tags (non-empty current → + // empty desired) was silently ignored because Diff skipped when + // len(desired.tags)==0. DO Block Storage has no tag-update endpoint, + // so any tag change must surface as ForceNew. + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size_gb": float64(100), + "tags": []any{"prod", "pg"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size_gb": 100, + "tags": []any{}, // explicit empty + }, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("clearing tags must force replace; NeedsReplace=%v", r.NeedsReplace) + } +} + +func TestVolumeDriver_Diff_TagsAbsentSkipped(t *testing.T) { + d := drivers.NewVolumeDriverWithClient(&mockStorageClient{}, nil, "nyc3") + cur := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size_gb": float64(100), + "tags": []any{"prod"}, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size_gb": 100}, + }, cur) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Errorf("absent tags key must NOT trigger drift; NeedsReplace=%v", r.NeedsReplace) + } +} + func TestVolumeDriver_Diff_TagsUnorderedNoChange(t *testing.T) { // DO doesn't preserve tag order across reads. Equal-set must NOT // trigger a replace. From a6d92831b81db30c6e31e26b024a5c846fbabdc3 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:41:40 -0400 Subject: [PATCH 20/21] fix(droplet): vpc_uuid add-from-empty surfaces as drift (Copilot round-2 finding #7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The curVPC != "" guard skipped vpc_uuid drift when the current state had no vpc_uuid. Pre-release Droplet states (created before vpc_uuid became part of Outputs) would silently ignore an operator adding a vpc_uuid pin to YAML — exactly when explicit drift detection matters most. Switch to "key present in desired" guard. Empty current vs non-empty desired now triggers ForceNew. Absent desired key still skips (backwards-compat). Tests: vpc_uuid add-from-empty forces replace; absent key does NOT trigger drift. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 11 +++++-- internal/drivers/droplet_test.go | 54 ++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 339d5d8..a279ef0 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -133,10 +133,15 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } } - // vpc_uuid: read-side stable, drift is unambiguous. - if vpc := strFromConfig(desired.Config, "vpc_uuid", ""); vpc != "" { + // vpc_uuid: read-side stable, drift is unambiguous. Drop the + // curVPC != "" guard so adding vpc_uuid to a Droplet whose state + // predates the field (current.Outputs has no vpc_uuid yet) still + // surfaces as ForceNew. Empty desired with non-empty current is + // also drift — operator dropped the VPC pin, plan must recreate. + if _, hasVPC := desired.Config["vpc_uuid"]; hasVPC { + vpc := strFromConfig(desired.Config, "vpc_uuid", "") curVPC, _ := current.Outputs["vpc_uuid"].(string) - if curVPC != "" && curVPC != vpc { + if curVPC != vpc { changes = append(changes, interfaces.FieldChange{ Path: "vpc_uuid", Old: curVPC, New: vpc, ForceNew: true, }) diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index b76c085..5ac4ddf 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -559,6 +559,60 @@ func TestDropletDriver_Diff_VPCChangeForcesReplace(t *testing.T) { } } +func TestDropletDriver_Diff_VPCAddFromEmptyForcesReplace(t *testing.T) { + // Copilot round-2 finding #7: pre-release Droplet states won't have + // vpc_uuid in Outputs (the field was added later). Adding vpc_uuid to + // an already-managed Droplet planned no action because the curVPC != "" + // guard skipped the change. Drop that guard so empty current vs + // non-empty desired triggers ForceNew. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + // no vpc_uuid — represents pre-release state + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "vpc_uuid": "00000000-0000-0000-0000-000000000001", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("vpc_uuid add-from-empty must force replace; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +} + +func TestDropletDriver_Diff_VPCAbsentSkipped(t *testing.T) { + // Inverse: when vpc_uuid is absent from desired, current vpc_uuid must + // NOT be cleared as drift. Backwards-compat for YAML predating the field. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "vpc_uuid": "00000000-0000-0000-0000-000000000001", + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"size": "s-1vcpu-2gb"}, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace || r.NeedsUpdate { + t.Errorf("absent vpc_uuid must NOT trigger drift; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +} + func TestDropletDriver_Diff_TagsChangeForcesReplace(t *testing.T) { mock := &mockDropletClient{} d := drivers.NewDropletDriverWithClient(mock, "nyc3") From 0fa89deffd3f1a6e9ff19b6a73658f84fad7e3b6 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sun, 3 May 2026 04:43:09 -0400 Subject: [PATCH 21/21] docs(droplet): explicit limitation block for user_data/ssh_keys/monitoring/ipv6 (Copilot round-2 finding #8) Round-1 documented the gap inline but the silent fallthrough at the end of Diff was easy to miss. Replace with an explicit block that: - Names each undetected field and the godo reason - States the operator workaround (`taint` / delete + re-apply) - References follow-up issue #56 for the resolution path - Updates the function-level Diff doc-comment to point at the inline rationale Issue: https://github.com/GoCodeAlone/workflow-plugin-digitalocean/issues/56 "Droplet Diff misses user_data / ssh_keys / monitoring / ipv6 (godo Read limitation)" Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/drivers/droplet.go | 50 +++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index a279ef0..88cad46 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -112,13 +112,15 @@ func (d *DropletDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) // Diff compares the desired spec against the current droplet output and // returns a DiffResult. Update is disallowed by godo (Droplet PUT only // resizes), so every detected change is flagged as ForceNew — the caller -// must replace the droplet, not patch it. We compare every field that -// Create wires through (size, vpc_uuid, enable_backups, tags, volumes, -// ssh_keys) plus user_data. Fields the DO Read API does not expose -// (user_data, monitoring, ipv6, ssh_keys) cannot be drift-compared from -// current Outputs, but we still flag a change when a desired value is -// present and the snapshot has no corresponding "" / false / nil baseline, -// surfacing config-vs-current divergence on first re-plan. +// must replace the droplet, not patch it. +// +// Detected: size, vpc_uuid, enable_backups, tags, volumes (by NAME after +// dropletOutput's ID→name resolution). +// +// NOT detected (godo Read limitation, tracked in issue #56): user_data, +// ssh_keys, monitoring, ipv6. Operators must taint the Droplet manually +// to roll a new value for any of these. See the inline comment near the +// end of this function for the full rationale. func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { if current == nil { return &interfaces.DiffResult{NeedsUpdate: true}, nil @@ -192,12 +194,34 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } } - // user_data, monitoring, ipv6, ssh_keys: DO Read does not expose these - // fields reliably (godo.Droplet has no Monitoring/IPv6/UserData fields - // surfaced post-create), so we cannot drift-compare from current - // Outputs without producing a perpetually-dirty plan. Drift on these - // fields will surface only via re-plan after the operator destroys + - // recreates, or via an external read-side check. Documented limitation. + // KNOWN LIMITATION (Copilot round-2 finding #8): the following fields + // CANNOT be drift-detected here because godo.Droplet (returned by + // Droplets.Get) does not surface them post-create: + // + // - user_data — write-only at create; not in Droplet.Get response + // - ssh_keys — no SSH-key list returned by Droplet.Get + // - monitoring — no dedicated boolean field on godo.Droplet + // - ipv6 — no dedicated boolean field on godo.Droplet + // (network-address presence is an unreliable signal) + // + // Changing any of these in YAML after the Droplet is created will + // produce NO plan action — the Droplet keeps the original cloud-init, + // SSH key list, etc. Operators must `taint` the Droplet (or delete + + // re-apply) to roll a new value. + // + // TODO(workflow-plugin-digitalocean#56): once godo exposes these + // fields in Droplet.Get, OR we add a side-channel desired-config + // snapshot, implement strict drift detection here. + // + // We deliberately do NOT add a desired-only-set check (that would be + // "always force replace when user_data is in YAML" — perpetually + // dirty plans). Silent no-op is the lesser evil until the upstream + // gap is closed; the comment + issue reference make the limitation + // discoverable. + _ = strFromConfig(desired.Config, "user_data", "") // see TODO above + _ = boolFromConfig(desired.Config, "monitoring", false) // see TODO above + _ = boolFromConfig(desired.Config, "ipv6", false) // see TODO above + // ssh_keys: parsed in Create; not re-parsed here to avoid wasted work. return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0,