diff --git a/CHANGELOG.md b/CHANGELOG.md index fb2a132..44b29d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to workflow-plugin-digitalocean are documented here. ## [Unreleased] +### Fixed + +- **VPCDriver / DropletDriver / AppPlatformDriver `Diff` now compares `region`** (#70) — previously only `VolumeDriver.Diff` checked region drift; the other three drivers silently no-op'd on region changes despite DO having no in-place region update on those resources. Region change now correctly emits `FieldChange{ForceNew: true}` and sets `NeedsReplace=true`. Surfaced by core-dump's TC2 cutover plan-shape assertion which expected a 5-resource cascade replace but got 1 (Volume only); without the assertion gate the partial cutover would have left VPC + Droplet in nyc3 while the App + Volume moved to nyc1, producing a half-migrated state requiring manual cleanup. Includes upgrade-safe guard: `vpcOutput` and `dropletOutput` already populated `region`, and this PR adds `region` to `appOutput.Outputs` so AppPlatformDriver.Diff has a current-side value to compare; the new check then skips when `current.Outputs["region"]` is empty so state from earlier plugin versions doesn't false-positive — the next Read populates it without spurious drift. Regression tests cover both the change-detection and empty-current-skip paths for all three drivers. + ## [v0.10.0] ### Added diff --git a/internal/drivers/app_platform.go b/internal/drivers/app_platform.go index 7643f00..a4d4dd9 100644 --- a/internal/drivers/app_platform.go +++ b/internal/drivers/app_platform.go @@ -209,7 +209,30 @@ func (d *AppPlatformDriver) Diff(_ context.Context, desired interfaces.ResourceS if desiredExpose != curExpose { changes = append(changes, interfaces.FieldChange{Path: "expose", Old: curExpose, New: desiredExpose}) } - return &interfaces.DiffResult{NeedsUpdate: len(changes) > 0, Changes: changes}, nil + + // region: App Platform's UpdateApp does not accept region changes — + // region is a Create-only field on godo.AppSpec, so any drift forces + // replace. Mirror VolumeDriver.Diff's region pattern. Guard + // curRegion != "" so apps in state from earlier plugin versions + // (when appOutput didn't include region) don't false-positive on + // the first plan after upgrade — they'll Read on next apply to + // populate the field. + var needsReplace bool + 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 + } + } + + return &interfaces.DiffResult{ + NeedsUpdate: len(changes) > 0, + NeedsReplace: needsReplace, + Changes: changes, + }, nil } // canonicalExpose returns the canonical `expose` value for a desired-spec @@ -768,6 +791,11 @@ func appOutput(app *godo.App) *interfaces.ResourceOutput { // desired `cfg["image"]` and avoid emitting spurious image // FieldChanges on no-op reconciles — F4 round-3 Finding A. "image": deriveImageFromAppSpec(app.Spec), + // `region` is the App Platform region from AppSpec.Region. + // Stored on Outputs so Diff can detect region drift and + // surface it as ForceNew (App Platform's UpdateApp does not + // accept region changes — region is a Create-only field). + "region": app.Spec.Region, }, Status: "running", } diff --git a/internal/drivers/app_platform_test.go b/internal/drivers/app_platform_test.go index 499ffb8..64fed6b 100644 --- a/internal/drivers/app_platform_test.go +++ b/internal/drivers/app_platform_test.go @@ -371,6 +371,77 @@ func TestAppPlatformDriver_Diff_NoChanges(t *testing.T) { } } +// TestAppPlatformDriver_Diff_RegionChangeForcesReplace covers issue #70: +// App Platform region is a Create-only field on godo.AppSpec — UpdateApp +// does not accept region changes. Region drift must surface as ForceNew so +// dependents (vpc_ref to a region-locked VPC) get correctly recreated too. +func TestAppPlatformDriver_Diff_RegionChangeForcesReplace(t *testing.T) { + mock := &mockAppClient{} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "region": "nyc3", + }, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsReplace { + t.Fatal("region change must force replace; NeedsReplace=false") + } + var found bool + for _, c := range result.Changes { + if c.Path == "region" && c.Old == "nyc3" && c.New == "nyc1" && c.ForceNew { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange{Path:region, Old:nyc3, New:nyc1, ForceNew:true}; got %+v", result.Changes) + } +} + +// TestAppPlatformDriver_Diff_RegionEmptyCurrentSkipped covers the +// upgrade-safe guard: Apps in state from earlier plugin versions (when +// appOutput didn't include region) must not false-positive on the first +// plan after upgrade — they'll Read on next apply to populate the field. +func TestAppPlatformDriver_Diff_RegionEmptyCurrentSkipped(t *testing.T) { + mock := &mockAppClient{} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + // no region — represents pre-region-output state + }, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if result.NeedsReplace { + t.Error("empty curRegion should not force replace; got NeedsReplace=true") + } + for _, c := range result.Changes { + if c.Path == "region" { + t.Errorf("empty curRegion should not emit a region change; got %+v", c) + } + } +} + // TestAppPlatformDriver_Diff_DetectsExposeChange covers quality-review Finding // 1: changing `expose` (a security-relevant toggle) on an existing service // must produce a Plan action — Diff cannot silently no-op the way the diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 88cad46..7cfe9fc 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -223,6 +223,21 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, _ = boolFromConfig(desired.Config, "ipv6", false) // see TODO above // ssh_keys: parsed in Create; not re-parsed here to avoid wasted work. + // region: Droplets are regional and DO does not support region change + // in Update. Mirror VolumeDriver.Diff's region pattern: any region + // drift forces replace. Guard curRegion != "" so Droplets in state + // from earlier plugin versions (when dropletOutput didn't include + // region) don't false-positive on the first plan after upgrade — + // they'll Read on next apply to populate the field. + 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, + }) + } + } + return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0, NeedsReplace: len(changes) > 0, diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 5ac4ddf..89dfc3c 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -965,6 +965,78 @@ func TestDropletDriver_Create_Volumes_EmptyStringRejected(t *testing.T) { } } +// TestDropletDriver_Diff_RegionChangeForcesReplace covers issue #70: +// Droplets are regional and DO does not support region change in Update +// (godo Droplet PUT only resizes). Region drift must surface as ForceNew +// so cascade dependents (Volume mount, Firewall tags) get correctly +// re-coordinated against the replaced Droplet. +func TestDropletDriver_Diff_RegionChangeForcesReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "region": "nyc3", + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Fatal("region change must force replace; NeedsReplace=false") + } + var found bool + for _, c := range r.Changes { + if c.Path == "region" && c.Old == "nyc3" && c.New == "nyc1" && c.ForceNew { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange{Path:region, Old:nyc3, New:nyc1, ForceNew:true}; got %+v", r.Changes) + } +} + +// TestDropletDriver_Diff_RegionEmptyCurrentSkipped covers the upgrade-safe +// guard: Droplets in state from earlier plugin versions (when dropletOutput +// didn't include region) must not false-positive on the first plan after +// upgrade — they'll Read on next apply to populate the field. +func TestDropletDriver_Diff_RegionEmptyCurrentSkipped(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + // no region — represents pre-region-output state + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Error("empty curRegion should not force replace; got NeedsReplace=true") + } + for _, c := range r.Changes { + if c.Path == "region" { + t.Errorf("empty curRegion should not emit a region change; got %+v", c) + } + } +} + func contains(s, sub string) bool { if len(sub) == 0 { return true diff --git a/internal/drivers/vpc.go b/internal/drivers/vpc.go index 53fb031..d20ecd7 100644 --- a/internal/drivers/vpc.go +++ b/internal/drivers/vpc.go @@ -151,6 +151,22 @@ func (d *VPCDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, cur }) } } + + // region: VPCs are regional and DO does not support region change in + // Update. Mirror VolumeDriver.Diff's region pattern: any region drift + // forces replace. Guard curRegion != "" so VPCs in state from earlier + // plugin versions (when vpcOutput didn't include region) don't false- + // positive on the first plan after upgrade — they'll Read on next + // apply to populate the field. + 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, + }) + } + } + return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0, NeedsReplace: len(changes) > 0, diff --git a/internal/drivers/vpc_test.go b/internal/drivers/vpc_test.go index b974896..2548ba3 100644 --- a/internal/drivers/vpc_test.go +++ b/internal/drivers/vpc_test.go @@ -217,6 +217,65 @@ func TestVPCDriver_Diff_IPRangeChange(t *testing.T) { } } +// TestVPCDriver_Diff_RegionChangeForcesReplace covers issue #70: VPCs are +// regional and DO does not support region change in Update. Region drift +// must surface as ForceNew so cascade dependents (Droplet vpc_uuid, App +// vpc_ref) are correctly replaced too. +func TestVPCDriver_Diff_RegionChangeForcesReplace(t *testing.T) { + mock := &mockVPCClient{vpc: testVPC()} + d := drivers.NewVPCDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{"ip_range": "10.20.0.0/16", "region": "nyc3"}, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"ip_range": "10.20.0.0/16", "region": "nyc1"}, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsReplace { + t.Fatal("expected NeedsReplace for region change") + } + var found bool + for _, c := range result.Changes { + if c.Path == "region" && c.Old == "nyc3" && c.New == "nyc1" && c.ForceNew { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange{Path:region, Old:nyc3, New:nyc1, ForceNew:true}; got %+v", result.Changes) + } +} + +// TestVPCDriver_Diff_RegionEmptyCurrentSkipped covers the upgrade-safe +// guard: VPCs in state from earlier plugin versions (when vpcOutput +// didn't include region) must not false-positive on the first plan +// after upgrade — they'll Read on next apply to populate the field. +func TestVPCDriver_Diff_RegionEmptyCurrentSkipped(t *testing.T) { + mock := &mockVPCClient{vpc: testVPC()} + d := drivers.NewVPCDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{"ip_range": "10.20.0.0/16"}, // no region + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"ip_range": "10.20.0.0/16", "region": "nyc1"}, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if result.NeedsReplace { + t.Error("empty curRegion should not force replace; got NeedsReplace=true") + } + for _, c := range result.Changes { + if c.Path == "region" { + t.Errorf("empty curRegion should not emit a region change; got %+v", c) + } + } +} + func TestVPCDriver_SupportsUpsert(t *testing.T) { d := drivers.NewVPCDriverWithClient(&mockVPCClient{}, "nyc3") if !d.SupportsUpsert() {