From 60519f0e474c00a586da2f2a81126294e6ea9f8a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 5 May 2026 07:52:22 +0000 Subject: [PATCH 1/3] Initial plan From 72a0063294d68a1e25096944c73f58262c29699b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 5 May 2026 08:12:07 +0000 Subject: [PATCH 2/3] fix: detect monitoring/ipv6 drift via droplet.Features in Droplet Diff Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-digitalocean/sessions/3cb12045-f989-4c4c-adf6-c1782e6322f6 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- internal/drivers/droplet.go | 71 +++++++++---- internal/drivers/droplet_test.go | 164 +++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 19 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 88cad46..3d8b0f5 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -115,12 +115,13 @@ func (d *DropletDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) // must replace the droplet, not patch it. // // Detected: size, vpc_uuid, enable_backups, tags, volumes (by NAME after -// dropletOutput's ID→name resolution). +// dropletOutput's ID→name resolution), monitoring, ipv6 (via the +// droplet.Features string slice returned by the DO API). // // 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. +// ssh_keys. 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 @@ -194,19 +195,39 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, } } + // monitoring / ipv6: the DO API surfaces these in droplet.Features + // (e.g. "monitoring", "ipv6"). dropletOutput stores them as booleans + // in Outputs so Diff can compare desired-vs-current reliably. + if _, hasMonitoring := desired.Config["monitoring"]; hasMonitoring { + desiredMonitoring := boolFromConfig(desired.Config, "monitoring", false) + curMonitoring, _ := current.Outputs["monitoring"].(bool) + if desiredMonitoring != curMonitoring { + changes = append(changes, interfaces.FieldChange{ + Path: "monitoring", Old: curMonitoring, New: desiredMonitoring, ForceNew: true, + }) + } + } + + if _, hasIPv6 := desired.Config["ipv6"]; hasIPv6 { + desiredIPv6 := boolFromConfig(desired.Config, "ipv6", false) + curIPv6, _ := current.Outputs["ipv6"].(bool) + if desiredIPv6 != curIPv6 { + changes = append(changes, interfaces.FieldChange{ + Path: "ipv6", Old: curIPv6, New: desiredIPv6, ForceNew: true, + }) + } + } + // 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) + // - user_data — write-only at create; not in Droplet.Get response + // - ssh_keys — no SSH-key list returned by Droplet.Get // // 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 + + // produce NO plan action — the Droplet keeps the original cloud-init + // and SSH key list. Operators must `taint` the Droplet (or delete + // re-apply) to roll a new value. // // TODO(workflow-plugin-digitalocean#56): once godo exposes these @@ -218,9 +239,7 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, // 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 + _ = strFromConfig(desired.Config, "user_data", "") // see TODO above // ssh_keys: parsed in Create; not re-parsed here to avoid wasted work. return &interfaces.DiffResult{ @@ -322,11 +341,12 @@ func (d *DropletDriver) dropletOutput(ctx context.Context, droplet *godo.Droplet "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. + // monitoring and ipv6 are surfaced via the Features string slice + // returned by Droplets.Get. The DO API includes "monitoring" / + // "ipv6" in that slice when the feature is enabled, so we can + // detect drift without any side-channel state file. + "monitoring": dropletHasFeature(droplet, "monitoring"), + "ipv6": dropletHasFeature(droplet, "ipv6"), } if len(resolutionFailures) > 0 { outputs["volumes_resolution"] = map[string]any{ @@ -351,6 +371,19 @@ func dropletBackupsEnabled(droplet *godo.Droplet) bool { return len(droplet.BackupIDs) > 0 } +// dropletHasFeature returns true when the named feature string is present in +// droplet.Features. The DO API populates this slice with entries like +// "monitoring", "ipv6", "private_networking", "backups" for each enabled +// feature, making it the canonical read-side source for those boolean flags. +func dropletHasFeature(droplet *godo.Droplet, feature string) bool { + for _, f := range droplet.Features { + if f == feature { + return true + } + } + return false +} + // 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 5ac4ddf..0cd5269 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -976,3 +976,167 @@ func contains(s, sub string) bool { } return false } + +// --- monitoring / ipv6 drift detection tests (issue #56) --- + +func TestDropletDriver_Read_MonitoringAndIPv6FromFeatures(t *testing.T) { + // dropletOutput must surface monitoring and ipv6 as booleans derived + // from droplet.Features, so Diff can compare desired-vs-current. + droplet := testDroplet() + droplet.Features = []string{"monitoring", "ipv6"} + mock := &mockDropletClient{droplet: droplet} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + out, err := d.Read(context.Background(), interfaces.ResourceRef{ + Name: "my-droplet", ProviderID: "42", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + if monitoring, _ := out.Outputs["monitoring"].(bool); !monitoring { + t.Errorf("Outputs[\"monitoring\"] = %v, want true", out.Outputs["monitoring"]) + } + if ipv6, _ := out.Outputs["ipv6"].(bool); !ipv6 { + t.Errorf("Outputs[\"ipv6\"] = %v, want true", out.Outputs["ipv6"]) + } +} + +func TestDropletDriver_Read_MonitoringAndIPv6FalseWhenAbsentFromFeatures(t *testing.T) { + // When Features is empty (or doesn't contain "monitoring"/"ipv6"), + // Outputs must report false for both flags. + droplet := testDroplet() + droplet.Features = nil + mock := &mockDropletClient{droplet: droplet} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + out, err := d.Read(context.Background(), interfaces.ResourceRef{ + Name: "my-droplet", ProviderID: "42", + }) + if err != nil { + t.Fatalf("Read: %v", err) + } + if monitoring, _ := out.Outputs["monitoring"].(bool); monitoring { + t.Errorf("Outputs[\"monitoring\"] = %v, want false", out.Outputs["monitoring"]) + } + if ipv6, _ := out.Outputs["ipv6"].(bool); ipv6 { + t.Errorf("Outputs[\"ipv6\"] = %v, want false", out.Outputs["ipv6"]) + } +} + +func TestDropletDriver_Diff_MonitoringToggleForcesReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + // Current: monitoring OFF; desired: ON — must flag ForceNew. + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "monitoring": false, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "monitoring": true, + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("monitoring toggle must force replace; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } + var found bool + for _, c := range r.Changes { + if c.Path == "monitoring" && c.ForceNew { + found = true + } + } + if !found { + t.Errorf("expected FieldChange{Path:\"monitoring\", ForceNew:true} in %+v", r.Changes) + } +} + +func TestDropletDriver_Diff_MonitoringAbsentSkipped(t *testing.T) { + // When "monitoring" is absent from desired config, no diff must be planned + // even if current has monitoring=true (backwards-compat for older YAML). + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "monitoring": true, + }, + } + 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 monitoring key must NOT trigger drift; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +} + +func TestDropletDriver_Diff_IPv6ToggleForcesReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + // Current: ipv6 OFF; desired: ON — must flag ForceNew. + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "ipv6": false, + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "ipv6": true, + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Errorf("ipv6 toggle must force replace; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } + var found bool + for _, c := range r.Changes { + if c.Path == "ipv6" && c.ForceNew { + found = true + } + } + if !found { + t.Errorf("expected FieldChange{Path:\"ipv6\", ForceNew:true} in %+v", r.Changes) + } +} + +func TestDropletDriver_Diff_IPv6AbsentSkipped(t *testing.T) { + // When "ipv6" is absent from desired config, no diff must be planned + // even if current has ipv6=true (backwards-compat for older YAML). + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "ipv6": true, + }, + } + 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 ipv6 key must NOT trigger drift; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +} From edbcc99889e01f79550783f3b541ce9e355e29c1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 5 May 2026 13:56:56 +0000 Subject: [PATCH 3/3] fix: guard monitoring/ipv6 Diff against legacy state missing keys Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-digitalocean/sessions/5f0057aa-dfcc-4a6d-a3c0-52fdf27fcbd3 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --- internal/drivers/droplet.go | 35 ++++++++++++------- internal/drivers/droplet_test.go | 60 ++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 3d8b0f5..6787380 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -198,23 +198,34 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, // monitoring / ipv6: the DO API surfaces these in droplet.Features // (e.g. "monitoring", "ipv6"). dropletOutput stores them as booleans // in Outputs so Diff can compare desired-vs-current reliably. + // + // Guard: only compare when the key is present in current.Outputs. If + // the key is absent the state was written by a version of the plugin + // that predates this PR (dropletOutput didn't emit the field). In that + // case, skip the comparison to avoid a spurious ForceNew replace — the + // key will be backfilled on the next state refresh (Read) and drift + // detection will work correctly from that point on. if _, hasMonitoring := desired.Config["monitoring"]; hasMonitoring { - desiredMonitoring := boolFromConfig(desired.Config, "monitoring", false) - curMonitoring, _ := current.Outputs["monitoring"].(bool) - if desiredMonitoring != curMonitoring { - changes = append(changes, interfaces.FieldChange{ - Path: "monitoring", Old: curMonitoring, New: desiredMonitoring, ForceNew: true, - }) + if _, curKeyExists := current.Outputs["monitoring"]; curKeyExists { + desiredMonitoring := boolFromConfig(desired.Config, "monitoring", false) + curMonitoring, _ := current.Outputs["monitoring"].(bool) + if desiredMonitoring != curMonitoring { + changes = append(changes, interfaces.FieldChange{ + Path: "monitoring", Old: curMonitoring, New: desiredMonitoring, ForceNew: true, + }) + } } } if _, hasIPv6 := desired.Config["ipv6"]; hasIPv6 { - desiredIPv6 := boolFromConfig(desired.Config, "ipv6", false) - curIPv6, _ := current.Outputs["ipv6"].(bool) - if desiredIPv6 != curIPv6 { - changes = append(changes, interfaces.FieldChange{ - Path: "ipv6", Old: curIPv6, New: desiredIPv6, ForceNew: true, - }) + if _, curKeyExists := current.Outputs["ipv6"]; curKeyExists { + desiredIPv6 := boolFromConfig(desired.Config, "ipv6", false) + curIPv6, _ := current.Outputs["ipv6"].(bool) + if desiredIPv6 != curIPv6 { + changes = append(changes, interfaces.FieldChange{ + Path: "ipv6", Old: curIPv6, New: desiredIPv6, ForceNew: true, + }) + } } } diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 0cd5269..99fa6d6 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -1140,3 +1140,63 @@ func TestDropletDriver_Diff_IPv6AbsentSkipped(t *testing.T) { r.NeedsReplace, r.Changes) } } + +// --- Legacy-state regression tests (issue #56 / PR upgrade guard) --- + +func TestDropletDriver_Diff_MonitoringLegacyStateNoSpuriousReplace(t *testing.T) { + // State written by a plugin version that predates the monitoring/ipv6 + // Outputs keys will NOT have "monitoring" in Outputs. Diff must NOT + // trigger a ForceNew replace just because the key is absent — that + // would destroy a live Droplet whose monitoring flag is already correct. + // The operator must run a Read (state refresh) to backfill the key, + // after which drift detection will work correctly. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + // no "monitoring" key — simulates legacy state + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "monitoring": true, // desired ON, but current has no key + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace || r.NeedsUpdate { + t.Errorf("legacy state (no monitoring key in Outputs) must NOT trigger spurious replace; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +} + +func TestDropletDriver_Diff_IPv6LegacyStateNoSpuriousReplace(t *testing.T) { + // Same as MonitoringLegacyState: absent "ipv6" key in current Outputs + // must not produce a spurious ForceNew replace. + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + // no "ipv6" key — simulates legacy state + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "ipv6": true, // desired ON, but current has no key + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace || r.NeedsUpdate { + t.Errorf("legacy state (no ipv6 key in Outputs) must NOT trigger spurious replace; NeedsReplace=%v changes=%+v", + r.NeedsReplace, r.Changes) + } +}