fix(droplet): detect monitoring/ipv6 drift via droplet.Features

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,50 @@ 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 {
+		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 {
+		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,
+				})
+			}
+		}
+	}
+
 	// 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 +250,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 +352,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 +382,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

internal/drivers/droplet_test.go

@@ -976,3 +976,227 @@ 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)
+	}
+}
+
+// --- 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)
+	}
+}