diff --git a/.github/workflows/codemod-report.yml b/.github/workflows/codemod-report.yml new file mode 100644 index 0000000..96a44c8 --- /dev/null +++ b/.github/workflows/codemod-report.yml @@ -0,0 +1,85 @@ +name: codemod-report +on: + pull_request: + branches: [main] +permissions: + # Copilot review #14 (round 4): scope to least privilege — + # contents:read for checkout, issues:write for the sticky PR + # comment (PR comments are issues at the GitHub API layer). The + # prior pull-requests:write grant was unused (no PR-state + # mutations) and inconsistent with the other workflows in this + # repo (ci.yml uses contents:read only). + contents: read + issues: write +env: + GOPRIVATE: github.com/GoCodeAlone/* +jobs: + codemod-report: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: Configure Git for private repos + env: + RELEASES_TOKEN: ${{ secrets.RELEASES_TOKEN }} + run: | + if [ -n "${RELEASES_TOKEN}" ]; then + git config --global url."https://x-access-token:${RELEASES_TOKEN}@github.com/".insteadOf "https://github.com/" + fi + - name: Run iac-codemod refactor-apply (dry-run) + run: | + set -euo pipefail + WFCTL_VERSION="$(go list -m github.com/GoCodeAlone/workflow | awk '{print $2}')" + go run "github.com/GoCodeAlone/workflow/cmd/iac-codemod@${WFCTL_VERSION}" \ + refactor-apply -dry-run . > /tmp/codemod-report.md + # Short summary for the sticky PR comment. + head -30 /tmp/codemod-report.md > /tmp/codemod-summary.md + echo "" >> /tmp/codemod-summary.md + echo "_Full report (90-day retention) attached as workflow artifact._" >> /tmp/codemod-summary.md + - uses: actions/upload-artifact@v4 + with: + name: codemod-report-${{ github.event.pull_request.number }} + path: /tmp/codemod-report.md + retention-days: 90 + - name: Comment summary on PR (first-party action SHA-pinned) + # rev5 — pin to commit SHA even though first-party. Tag mutability is a + # known risk; Renovate config tracks upstream releases via .github/renovate.json. + # + # Copilot review #5: PRs from forks run with a read-only GITHUB_TOKEN + # (per GitHub's pull_request workflow security model), so issues:create- + # comment + issues:update-comment fail 403. Skip the comment step on + # fork PRs — the artifact upload above still runs unconditionally so + # the report is reachable from the Actions tab without a comment. + if: github.event.pull_request.head.repo.fork == false + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const fs = require('fs'); + const summary = fs.readFileSync('/tmp/codemod-summary.md', 'utf8'); + const marker = ''; + const body = `${marker}\n${summary}`; + // Paginate to handle PRs with >30 comments (default page size). + const allComments = await github.paginate(github.rest.issues.listComments, { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + per_page: 100, + }); + const existing = allComments.find(c => c.body && c.body.startsWith(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body, + }); + } diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bc5f65..fb2a132 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,84 @@ All notable changes to workflow-plugin-digitalocean are documented here. ## [Unreleased] +## [v0.10.0] + +### Added + +- **`iacProvider.computePlanVersion: v2` opt-in** (PR P-DO TP4) — wfctl's + runtime dispatcher now routes Apply through `wfctlhelpers.ApplyPlan` + instead of the legacy in-provider switch. The new dispatch path adds: + Replace decomposition + `ReplaceIDMap` propagation, JIT + `${MODULE.id}` / `${VAR}` substitution, the input-drift postcondition, + per-action context cancellation between iterations, and the + `interfaces.UpsertSupporter` recovery contract. + + Backward compat: wfctl < v0.21.0 ignores the new field; the legacy + v1 dispatch (`provider.Apply` switch, now wrapping + `wfctlhelpers.ApplyPlan`) continues to work for all existing callers. + +- **`ValidatePlan` (`interfaces.ProviderValidator`)** (PR P-DO TP3) — + read-only, no-remote-call cross-resource constraint check that runs + at `wfctl infra align` time before any cloud API call. First pass + covers three constraint families: + 1. App Platform `infra.container_service` requires a region GROUP + slug (`nyc`, `ams`, `fra`, `sfo`, `sgp`, `syd`, `tor`, `blr`, + `lon`); zone slugs (`nyc1`, `sfo3`, …) rejected with + `PlanDiagnosticError`. + 2. Zone-bound resources (`infra.vpc`, `infra.droplet`, `infra.volume`) + require a zone slug; bare group slugs rejected with + `PlanDiagnosticError`. + 3. Cross-resource: App Platform `vpc_ref` must reference a VPC whose + region zone belongs to the App Platform's region group; database + `vpc_ref` must resolve to an in-plan VPC. Locks the recurring + "App Platform in nyc cannot reach VPC in sfo3" production bug + class (root-cause issue D from the conformance design). + + Severity mapping: Error always fails align; Warning fails only under + `--strict`; Info never affects exit. ValidatePlan is read-only and + makes no remote calls per the W-4 contract. + +- **Conformance test (`provider_conformance_test.go`)** (PR P-DO TP5) + — invokes `iac/conformance.Run` against a freshly-constructed + `DOProvider`. Behind the `conformance` build tag; opt in with + `go test -tags=conformance ./internal/...`. Six non-cloud scenarios + run by default; `CONFORMANCE_LIVE_CLOUD=1` (with + `DIGITALOCEAN_ACCESS_TOKEN`) opts into the cloud-touching probes. + +- **`.github/workflows/codemod-report.yml`** (PR P-DO TP1) — per-PR + workflow runs `iac-codemod refactor-apply -dry-run` against the + plugin source, uploads the full Markdown report as a 90-day retention + GitHub Actions artifact, and posts/updates a sticky PR comment with + the top-30-line summary so drive-by reviewers see findings without + downloading the artifact. + +### Changed + +- **`DOProvider.Apply` collapsed to wrap `wfctlhelpers.ApplyPlan`** + (PR P-DO TP2) — the in-Apply per-action switch + (create/update/replace/delete + upsert recovery + nil-out diagnostic) + is replaced with a single dispatch to the helper. The DO-plugin-specific + deferred-update flush (DatabaseDriver `type=app` `trusted_sources` + referencing apps created later in the plan; regression-gated by + `provider_deferred_test.go`) is preserved by wrapping `ApplyPlan` with + the second-pass loop. The wrapper deviates from the codemod's canonical + single-statement shape; the deviation is documented and marked with + `// wfctl:skip-iac-codemod` so `AssertApplyDelegatesToHelper` + recognises it as intentional. + +- **DO drivers `AppPlatformDriver`, `VPCDriver`, `FirewallDriver`, + `DatabaseDriver`** structurally satisfy the canonical + `interfaces.UpsertSupporter` (their existing `SupportsUpsert() bool` + method is bit-identical to the new interface; no driver-side changes + needed). + +- **`DOProvider.Apply` `delete` action with `Current == nil`** — under + v2 dispatch the contract is "driver is the authority on what an empty + ProviderID means" (per `wfctlhelpers/apply.go::doUpdate`'s analogous + comment). The v1-era pre-flight precondition error is retired; + `provider_apply_test.go::TestDOProvider_Apply_DeleteAction_MissingCurrent` + was rewritten to lock the new contract. + ### Fixed - **`infra.vpc` exposes `id` output** — wfctl `infra_output: .id` @@ -14,6 +92,11 @@ All notable changes to workflow-plugin-digitalocean are documented here. `Outputs["id"]` so the standard `.id` reference works. Surfaced by core-dump deploy run 25278900082. +### Bumped + +- `github.com/GoCodeAlone/workflow` → `e2c582bece90` (workflow main HEAD + with W-7 conformance suite + W-8 codemod merged). + ## [v0.9.1] ### Fixed diff --git a/go.mod b/go.mod index 0b4867e..9ed48da 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/GoCodeAlone/workflow-plugin-digitalocean go 1.26.0 require ( - github.com/GoCodeAlone/workflow v0.20.5 + github.com/GoCodeAlone/workflow v0.20.6-0.20260505011403-e2c582bece90 github.com/aws/aws-sdk-go-v2 v1.41.5 github.com/aws/aws-sdk-go-v2/credentials v1.19.12 github.com/aws/aws-sdk-go-v2/service/s3 v1.97.2 diff --git a/go.sum b/go.sum index 921b757..1ef1a78 100644 --- a/go.sum +++ b/go.sum @@ -56,8 +56,8 @@ github.com/GoCodeAlone/modular/modules/jsonschema v1.15.0 h1:xb1mI4NZkzvNKQ2F6nk github.com/GoCodeAlone/modular/modules/jsonschema v1.15.0/go.mod h1:hhGouwAVsonmJ4Lain4jINZ9nZCoc9l9eF3BHbmR8eE= github.com/GoCodeAlone/modular/modules/reverseproxy/v2 v2.8.0 h1:cvdLHbM/vzvygQTcAWSJsy+dAPzzwWyjzKMmTBFcFIo= github.com/GoCodeAlone/modular/modules/reverseproxy/v2 v2.8.0/go.mod h1:/9ipMG4qM2CHQ14BfXKdVlYRJelef6M8MFI5TbZv67M= -github.com/GoCodeAlone/workflow v0.20.5 h1:CtsQ7vGiF0xlJQ7w/bhWgW06ojw6E45kxjqgRB6jAJQ= -github.com/GoCodeAlone/workflow v0.20.5/go.mod h1:ypkCqXTwnIPqNjS8h38KZfwzdVsgwgkS1d6Dq0lXyQQ= +github.com/GoCodeAlone/workflow v0.20.6-0.20260505011403-e2c582bece90 h1:A4efhC8phY+M8KOB9ccga8COdjYMw+OKHhUuWcKgltA= +github.com/GoCodeAlone/workflow v0.20.6-0.20260505011403-e2c582bece90/go.mod h1:IaonttCuyJcjT+XryXUuh3ATvTI7jx0t8e3n+ASCOOA= github.com/GoCodeAlone/yaegi v0.17.2 h1:WK6Y6e0t1a6U7r+S2dN3CGWW1PizYD3zO0zneToZPxM= github.com/GoCodeAlone/yaegi v0.17.2/go.mod h1:z5Pr6Wse6QJcQvpgxTxzMAevFarH0N37TG88Y9dprx0= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 h1:rIkQfkCOVKc1OiRCNcSDD8ml5RJlZbH/Xsq7lbpynwc= diff --git a/internal/provider.go b/internal/provider.go index 151fb29..026ac29 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -10,6 +10,7 @@ import ( "time" "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" + "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" "github.com/GoCodeAlone/workflow/interfaces" "github.com/digitalocean/godo" "golang.org/x/oauth2" @@ -217,134 +218,68 @@ func resourceOutputFromState(state interfaces.ResourceState) *interfaces.Resourc } } -// upsertSupporter is an optional interface for ResourceDrivers that support -// locating a resource by name alone (empty ProviderID) in their Read method. -// Only drivers that implement name-based discovery should implement this -// interface. Apply gates the ErrResourceAlreadyExists → upsert path on it to -// prevent calling Read with an empty ProviderID on drivers that require one. -type upsertSupporter interface { - SupportsUpsert() bool -} - // deferredUpdater is an optional interface for ResourceDrivers that accumulate // resource-level updates that cannot be applied until all plan creates complete. // The canonical case is DatabaseDriver deferring type=app trusted_sources entries // that reference apps created later in the same plan. Apply calls -// FlushDeferredUpdates once after the main action loop; errors are appended to +// FlushDeferredUpdates once after the main dispatch loop; errors are appended to // ApplyResult.Errors so the failure is visible to the operator. +// +// This is a DO-plugin-specific extension point not yet hoisted into +// wfctlhelpers.ApplyPlan; the second-pass flush below preserves the regression +// gate while the v2 dispatch handles the per-action loop. type deferredUpdater interface { HasDeferredUpdates() bool FlushDeferredUpdates(ctx context.Context) error } -// Apply executes the plan. +// Apply executes the plan via wfctlhelpers.ApplyPlan, then runs the +// DO-specific deferred-update second pass. +// +// PR P-DO TP2: under iacProvider.computePlanVersion: v2 wfctl dispatches +// directly through wfctlhelpers.ApplyPlan and does not call this method. +// The implementation here remains for legacy v1 callers (wfctl < v0.21.0 +// or any in-process embedder of the gRPC plugin) and for the deferred-flush +// behavior that wfctlhelpers does not yet hoist. +// +// Per-action upsert recovery, JIT substitution, the Replace cascade, and +// the input-drift postcondition all live in wfctlhelpers.ApplyPlan now — +// drivers that opt into the upsert recovery path implement +// interfaces.UpsertSupporter (DO drivers AppPlatform, VPC, Firewall, +// Database all do; signature: SupportsUpsert() bool). The local +// upsertSupporter interface previously declared here is no longer needed: +// its SupportsUpsert() bool method is structurally identical to +// interfaces.UpsertSupporter, so the existing driver implementations +// satisfy the canonical interface without code change. +// +// wfctl:skip-iac-codemod +// +// The body intentionally wraps wfctlhelpers.ApplyPlan rather than +// matching the codemod's canonical single-statement +// `return wfctlhelpers.ApplyPlan(ctx, p, plan)` shape: the +// post-helper deferred-update flush below is a DO-plugin-specific +// regression gate (see provider_deferred_test.go and CHANGELOG entry +// for staging-deploy-blockers Blocker 2) that wfctlhelpers does not +// hoist. The skip marker tells the codemod's +// AssertApplyDelegatesToHelper analyzer this deviation is intentional. +// When wfctlhelpers grows a deferred-update lifecycle hook, the +// wrapper can collapse and the marker can drop. func (p *DOProvider) Apply(ctx context.Context, plan *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { - result := &interfaces.ApplyResult{PlanID: plan.ID} - for _, action := range plan.Actions { - d, err := p.ResourceDriver(action.Resource.Type) - if err != nil { - result.Errors = append(result.Errors, interfaces.ActionError{ - Resource: action.Resource.Name, Action: action.Action, Error: err.Error(), - }) - continue - } - var out *interfaces.ResourceOutput - switch action.Action { - case "create": - out, err = d.Create(ctx, action.Resource) - if errors.Is(err, interfaces.ErrResourceAlreadyExists) { - // Upsert: resource exists in the provider but is absent from - // local state. Only attempt upsert if the driver supports - // name-based discovery (SupportsUpsert returns true). - // Drivers that pass ProviderID directly to their API client - // (VPC, database, firewall, etc.) do not support this path. - us, ok := d.(upsertSupporter) - if !ok || !us.SupportsUpsert() { - // Propagate original error; upsert not available for this type. - break - } - createErr := err - ref := interfaces.ResourceRef{ - Name: action.Resource.Name, - Type: action.Resource.Type, - } - existing, readErr := d.Read(ctx, ref) - if readErr != nil { - // Preserve createErr so ErrResourceAlreadyExists remains - // in the error chain for callers using errors.Is. - err = fmt.Errorf("upsert: read after conflict: %w", errors.Join(createErr, readErr)) - break - } - if existing.ProviderID == "" { - err = fmt.Errorf("upsert: resource %q found by name but ProviderID is empty; cannot update: %w", ref.Name, createErr) - break - } - ref.ProviderID = existing.ProviderID - out, err = d.Update(ctx, ref, action.Resource) - } - case "update": - ref := interfaces.ResourceRef{ - Name: action.Resource.Name, - Type: action.Resource.Type, - ProviderID: action.Current.ProviderID, - } - out, err = d.Update(ctx, ref, action.Resource) - case "replace": - if action.Current == nil { - err = fmt.Errorf("replace action missing current resource state") - break - } - ref := interfaces.ResourceRef{ - Name: action.Resource.Name, - Type: action.Resource.Type, - ProviderID: action.Current.ProviderID, - } - if err = d.Delete(ctx, ref); err != nil { - break - } - out, err = d.Create(ctx, action.Resource) - case "delete": - // Delete uses Current for the ref (ProviderID). Resource.Config is - // empty for deletes — the desired state is "absent" — but - // Resource.Type and Name are still set (required for driver lookup above). - if action.Current == nil { - err = fmt.Errorf("delete action for %q missing current resource state", action.Resource.Name) - break - } - ref := interfaces.ResourceRef{ - Name: action.Current.Name, - Type: action.Current.Type, - ProviderID: action.Current.ProviderID, - } - err = d.Delete(ctx, ref) - // out remains nil — deleted resources have no post-apply output. - default: - err = fmt.Errorf("unknown action %q", action.Action) - } - if err != nil { - result.Errors = append(result.Errors, interfaces.ActionError{ - Resource: action.Resource.Name, Action: action.Action, Error: err.Error(), - }) - continue - } - if out != nil { - result.Resources = append(result.Resources, *out) - } else if action.Action != "delete" { - // A nil output without an error from create/update/replace is a driver - // bug — surface it explicitly so callers aren't silently missing state - // entries. "delete" legitimately returns nil out, so it's excluded. - result.Errors = append(result.Errors, interfaces.ActionError{ - Resource: action.Resource.Name, Action: action.Action, - Error: "driver returned nil output without error", - }) - } + result, err := wfctlhelpers.ApplyPlan(ctx, p, plan) + if err != nil { + // ApplyPlan only returns a top-level error on context cancellation + // — per-action failures land on result.Errors. Surface the + // cancellation alongside whatever partial result is in hand so + // callers can still inspect any actions that completed. + return result, err } - // Second pass: flush deferred updates accumulated by drivers during the main - // action loop. These arise when a resource's config (e.g. DB trusted_sources - // with type=app) references another resource provisioned later in the same - // plan. By this point all plan creates have completed and the referenced - // resources exist. Each driver type is flushed at most once. + // Second pass: flush deferred updates accumulated by drivers during the + // main action loop. These arise when a resource's config (e.g. DB + // trusted_sources with type=app) references another resource provisioned + // later in the same plan. By this point all plan creates have completed + // and the referenced resources exist. Each driver type is flushed at + // most once. seen := make(map[string]struct{}, len(plan.Actions)) for _, action := range plan.Actions { if _, done := seen[action.Resource.Type]; done { diff --git a/internal/provider_apply_test.go b/internal/provider_apply_test.go index f13859f..c640ed3 100644 --- a/internal/provider_apply_test.go +++ b/internal/provider_apply_test.go @@ -84,8 +84,22 @@ func TestDOProvider_Apply_DeleteAction(t *testing.T) { } } -// TestDOProvider_Apply_DeleteAction_MissingCurrent verifies that a delete -// action with no Current state is collected as an error (not a panic/crash). +// TestDOProvider_Apply_DeleteAction_MissingCurrent verifies the v2-dispatch +// contract for a delete action with action.Current == nil: +// wfctlhelpers.ApplyPlan dispatches Delete with an empty-ProviderID +// ResourceRef (the driver is the authority on what an empty ProviderID +// means — see wfctlhelpers/apply.go::doUpdate's analogous comment). The +// v1-era pre-flight precondition error has been retired with the v2 +// migration (PR P-DO TP2): the response when ProviderID is empty is now +// driver-dependent rather than centrally synthesised. +// +// Copilot review #11 (round 3): driver behavior on an empty-ProviderID +// delete varies — FirewallDriver.Delete, for example, resolves by name +// when ProviderID is empty (uses ListByTag/ListByDroplet to locate the +// firewall) and may succeed; other drivers (DatabaseDriver, +// VolumeDriver) require ProviderID and surface a typed error. The v2 +// contract is "the driver knows what an empty ProviderID means for its +// resource shape", not "all drivers reject empty ProviderID". func TestDOProvider_Apply_DeleteAction_MissingCurrent(t *testing.T) { fake := &deleteFakeDriver{} p := &DOProvider{drivers: map[string]interfaces.ResourceDriver{"infra.firewall": fake}} @@ -93,21 +107,30 @@ func TestDOProvider_Apply_DeleteAction_MissingCurrent(t *testing.T) { plan := &interfaces.IaCPlan{ ID: "plan-delete-no-current", Actions: []interfaces.PlanAction{{ - Action: "delete", + Action: "delete", Resource: interfaces.ResourceSpec{Name: "orphan-firewall", Type: "infra.firewall"}, - Current: nil, // missing — defensive handling required + Current: nil, // v2 contract: dispatched with empty ProviderID }}, } result, err := p.Apply(t.Context(), plan) if err != nil { - t.Fatalf("Apply returned top-level error (want nil + error in result.Errors): %v", err) + t.Fatalf("Apply returned top-level error: %v", err) } - if len(result.Errors) == 0 { - t.Fatal("expected error in result.Errors for delete with missing Current, got none") + // v2 contract: the dispatch IS made with an empty ProviderID; this + // stub fake accepts it and returns nil. Real drivers handle the + // empty case driver-by-driver (some resolve-by-name, some error). + if fake.deleteCalls != 1 { + t.Errorf("Delete dispatched %d times, want 1 (v2 dispatch contract)", fake.deleteCalls) + } + if fake.deletedRef.ProviderID != "" { + t.Errorf("Delete called with ProviderID %q, want empty (Current was nil)", fake.deletedRef.ProviderID) } - if fake.deleteCalls != 0 { - t.Errorf("Delete should not be called when Current is nil, but called %d times", fake.deleteCalls) + // No top-level error and no per-action error because the fake driver + // accepted the empty-ProviderID delete. (Real drivers like FirewallDriver + // would emit an error here; the fake's job is to verify dispatch only.) + if len(result.Errors) != 0 { + t.Errorf("expected no per-action errors for fake driver dispatch, got %v", result.Errors) } } diff --git a/internal/provider_conformance_test.go b/internal/provider_conformance_test.go new file mode 100644 index 0000000..3412a58 --- /dev/null +++ b/internal/provider_conformance_test.go @@ -0,0 +1,103 @@ +//go:build conformance + +// Package internal — DO provider conformance test. +// +// This file lives behind the `conformance` build tag so it is excluded +// from the default `go test ./...` (which runs the existing per-package +// unit tests). Opt in with: +// +// GOWORK=off go test -tags=conformance ./internal/... -run TestConformance +// +// The conformance suite (github.com/GoCodeAlone/workflow/iac/conformance) +// dispatches a fixed set of provider-portable scenarios; each scenario +// independently decides whether it can run against a fresh provider +// (some skip when an opt-in driver type like infra.compute is absent — +// DO does not expose infra.compute, so the upsert-on-already-exists +// scenario will skip on DO). +// +// Two opt-in env vars gate the heavier scenarios: +// +// - CONFORMANCE_LIVE_CLOUD=1 — runs RequiresCloud=true scenarios +// against the real DO API. Requires DIGITALOCEAN_ACCESS_TOKEN. +// - testing.Short()=true — limits to Smoke=true scenarios only +// (the per-PR smoke gate's narrow contract). +// +// Without either env var, the suite runs only the non-cloud, +// non-smoke-only scenarios (the default for unit-style invocation). +// +// PR P-DO TP5. + +package internal + +import ( + "os" + "testing" + + "github.com/GoCodeAlone/workflow/iac/conformance" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestConformance dispatches the workflow conformance suite against a +// freshly-constructed DOProvider. Each scenario receives a new provider +// via the Provider closure; this guarantees per-scenario isolation +// (state from one scenario does not bleed into the next). +// +// The provider is initialized with the DIGITALOCEAN_ACCESS_TOKEN env var +// when LiveCloud is enabled. Without the token the LiveCloud scenarios +// would fail at the first cloud call; with it absent + LiveCloud=true +// the suite returns a clear error instead of hanging on a malformed +// network request. +func TestConformance(t *testing.T) { + liveCloud := os.Getenv("CONFORMANCE_LIVE_CLOUD") == "1" + cfg := conformance.Config{ + Provider: func() interfaces.IaCProvider { + p := NewDOProvider() + // Always initialize the provider exactly once with a + // single token, picked at runtime: the real + // DIGITALOCEAN_ACCESS_TOKEN when LiveCloud is enabled, + // or a stub placeholder otherwise. The non-cloud + // scenarios (cross-resource constraint rejection, + // plan-stale diagnostic, structpb-roundtrip, + // infra-output cross-module resolution, + // protected-replace with/without override) need the + // driver registry populated to dispatch their probes + // but do NOT require a real DO token because the + // driver methods they exercise are read-only or + // pure-data transformations. + // + // Copilot review #4: the prior comment described a + // "stub-then-real swap" with two Initialize calls; + // the implementation has always made one call with + // the right token chosen up-front, so the comment was + // incorrect. + token := "stub-token-for-non-cloud-conformance" + if liveCloud { + token = os.Getenv("DIGITALOCEAN_ACCESS_TOKEN") + if token == "" { + t.Fatal("CONFORMANCE_LIVE_CLOUD=1 but DIGITALOCEAN_ACCESS_TOKEN is not set") + } + } + // Copilot review #13 (round 4) → #15 (round 5): + // pass t.Context() through Initialize so callers that + // honor context (and any future rev of Initialize that + // does) get the test-scoped cancellation/deadline path. + // Note: today's DOProvider.Initialize implementation + // constructs its godo client with its own + // context.Background() and ignores the passed-in ctx, + // so passing t.Context() is forward-prep rather than an + // immediate behavior change. Tracked as a follow-up to + // thread the context through the godo construction so + // LiveCloud cancellation reaches the HTTP transport. + if err := p.Initialize(t.Context(), map[string]any{ + "token": token, + "region": "nyc3", + }); err != nil { + t.Fatalf("provider Initialize: %v", err) + } + return p + }, + SmokeOnly: testing.Short(), + LiveCloud: liveCloud, + } + conformance.Run(t, cfg) +} diff --git a/internal/validate_plan.go b/internal/validate_plan.go new file mode 100644 index 0000000..c34649a --- /dev/null +++ b/internal/validate_plan.go @@ -0,0 +1,455 @@ +package internal + +import ( + "fmt" + "regexp" + "sort" + "strings" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// uuidPattern matches a canonical RFC-4122 UUID (8-4-4-4-12 hex with +// hyphens). The match is case-insensitive (Copilot review #12 round 4 +// — UUIDs are case-insensitive in practice; a VPC UUID emitted in +// upper-case by an operator's clipboard or a templating engine must +// also be classified as a UUID, not as a plain resource name). DO +// VPC IDs are UUIDs; vpc_ref values that match this shape are +// external-or-resolved-VPC-IDs, NOT in-plan resource names — the +// validator MUST NOT flag them as dangling references. +var uuidPattern = regexp.MustCompile(`^(?i)[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) + +// jitTemplatePattern matches the wfctl JIT substitution syntax +// (${VAR} / ${MODULE.field} / $(...)). vpc_ref values that contain a +// template are resolved at apply time by wfctlhelpers.ApplyPlan's +// jitsubst.ResolveSpec; ValidatePlan runs at PLAN time (before +// substitution) and MUST treat templated values as deferred. +var jitTemplatePattern = regexp.MustCompile(`\$[\{\(]`) + +// looksLikeResourceName reports whether s is plausibly an in-plan +// resource Name reference rather than a UUID or a JIT template. Used +// by ValidatePlan to decide whether to apply name-based byName lookup +// for vpc_ref. The contract: only the name-shape branch may emit +// dangling-reference diagnostics — UUIDs and templates are deferred to +// apply-time validation in the wfctlhelpers dispatch path. +// +// Copilot review #8/#9 (round 3): the prior validator unconditionally +// did name-based lookup, which incorrectly rejected production configs +// that pass vpc_ref as a UUID (the shape DO's Apps/Databases API +// expects) or as ${vpc.id} (the canonical wfctl form, resolved by JIT +// at apply time). +func looksLikeResourceName(s string) bool { + if s == "" { + return false + } + if uuidPattern.MatchString(s) { + return false + } + if jitTemplatePattern.MatchString(s) { + return false + } + return true +} + +// ValidatePlan implements interfaces.ProviderValidator (W-4): a read-only, +// no-remote-call cross-resource constraint check that runs at `wfctl infra +// align` time before any cloud API call. Diagnostics surface as +// PlanDiagnosticError|Warning|Info with severity-driven exit-code mapping +// (Error always fails align; Warning fails only under --strict; Info never +// affects exit). +// +// PR P-DO TP3 — first DO region-constraint pass: +// +// 1. App Platform `infra.container_service` MUST use one of DO App +// Platform's supported region GROUPS (nyc, ams, fra, sfo, sgp, syd, +// tor, blr, lon). If `region` is set to a Droplet/VPC zone slug +// (nyc1, nyc3, sfo2, sfo3 …) it is rejected — App Platform configures +// by group, not zone. +// +// 2. Droplet/VPC/Volume zone-bound resources MUST use a zone slug +// (nyc1, nyc2, nyc3, sfo2, sfo3, ams2, ams3, fra1, sgp1, lon1, tor1, +// blr1, syd1) — bare group slugs (nyc, sfo) are rejected for these +// types because the DO API for these resources requires a specific +// zone. +// +// 3. App Platform with `vpc_ref` (referencing a VPC by name in the same +// plan) — the referenced VPC's region zone MUST belong to the App +// Platform's region group (e.g., AppPlatform region nyc → VPC in +// nyc1 OR nyc3; AppPlatform region sfo → VPC in sfo2 OR sfo3). +// Mismatch is the recurring "App Platform in nyc cannot reach VPC in +// sfo3" production bug (root-cause issue D from the conformance +// design). +// +// Future extensions (deferred follow-ups): database/cache zone slugs, +// load balancer zone matching against attached droplets, registry +// regional restrictions. +func (p *DOProvider) ValidatePlan(plan *interfaces.IaCPlan) []interfaces.PlanDiagnostic { + if plan == nil || len(plan.Actions) == 0 { + return nil + } + + // Index plan resources by Name for cross-resource lookup. Both desired + // (from action.Resource) and existing (from action.Current) entries + // land here so an App Platform that references an existing VPC by + // name (no plan action of its own) still resolves. + // + // Delete-action resources are EXCLUDED from the index — a vpc_ref + // pointing to a VPC scheduled for deletion in the same plan must not + // "resolve" as if it were live; otherwise the database/App Platform + // checks would silently treat a soon-to-be-gone VPC as valid. + // (Copilot review #1 — "vpc_ref can resolve to a VPC that is being + // deleted in the same plan".) Cross-resource references to + // delete-targets surface as dangling-reference Errors via the same + // path as references to names not in the plan at all. + byName := make(map[string]planResource, len(plan.Actions)) + for _, a := range plan.Actions { + if a.Action == "delete" { + continue + } + byName[a.Resource.Name] = planResource{spec: a.Resource, current: a.Current} + } + + var diags []interfaces.PlanDiagnostic + for _, a := range plan.Actions { + // Skip delete actions — they target the existing resource, not + // the desired spec, so region constraints on the (empty) spec + // would produce false positives. + if a.Action == "delete" { + continue + } + region, _ := a.Resource.Config["region"].(string) + switch a.Resource.Type { + case "infra.container_service": + diags = appendAppPlatformDiagnostics(diags, a.Resource, region, byName) + case "infra.vpc", "infra.droplet", "infra.volume": + if region != "" && !isZoneSlug(region) { + // Copilot review #16 (round 5): a region that is + // neither a known zone nor a known App Platform + // group falls into two buckets: + // (a) a documented misconfig — operator typed an + // App Platform group ("nyc") where a zone is + // required. This is the recurring bug and + // stays Severity=Error. + // (b) an unknown slug — possibly a brand-new DO + // region the plugin's hardcoded allowlist + // hasn't caught up to. This was previously + // Error too, blocking apply until the plugin + // was bumped. Now downgraded to Warning so + // operators on bleeding-edge DO regions can + // proceed (under non-strict align) while + // --strict still catches the case for + // cautious operators. + severity := interfaces.PlanDiagnosticWarning + if isAppPlatformRegionGroup(region) { + severity = interfaces.PlanDiagnosticError + } + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: severity, + Resource: a.Resource.Name, + Field: "region", + Message: fmt.Sprintf( + "DO %s requires a zone slug (e.g. nyc1, sfo3); got %q which is %s. Use a specific zone.", + a.Resource.Type, region, classifyRegion(region), + ), + }) + } + case "infra.database": + diags = appendDatabaseDiagnostics(diags, a.Resource, byName) + } + } + return diags +} + +// appendDatabaseDiagnostics emits cross-resource diagnostics for an +// infra.database action. Currently: +// +// - vpc_ref MUST resolve to a non-delete VPC action in the same plan. +// A vpc_ref pointing to a name that is absent from the plan +// entirely OR that is scheduled for deletion in the plan emits a +// Severity=Error diagnostic. This is the dangling-cross-resource- +// reference case the conformance suite locks via +// Scenario_CrossResourceConstraintRejection — both the W-4 design +// and the conformance scenario require Error severity, not +// Warning, so the conformance assertion ("at least one +// PlanDiagnosticError") matches. +func appendDatabaseDiagnostics( + diags []interfaces.PlanDiagnostic, + spec interfaces.ResourceSpec, + byName map[string]planResource, +) []interfaces.PlanDiagnostic { + vpcRef, _ := spec.Config["vpc_ref"].(string) + if vpcRef == "" { + return diags + } + // Copilot review #9 (round 3): vpc_ref's accepted shapes per the + // DO Databases API are a VPC UUID OR a wfctl JIT template like + // ${vpc.id} that resolves to a UUID at apply time. Only flag the + // in-plan-name-reference branch — UUID and template values are + // deferred to apply-time validation by wfctlhelpers.ApplyPlan + // after JIT substitution. + if !looksLikeResourceName(vpcRef) { + return diags + } + target, ok := byName[vpcRef] + if !ok { + // vpc_ref names a resource not in the plan. Surface as Error so + // the conformance scenario's "at least one Severity=Error" + // assertion matches; this is the dangling-cross-resource- + // reference case the W-4 design explicitly calls out as a + // plan-time validator gap. + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "DO database %q references vpc_ref %q (a plain resource name) which is not declared in the same plan; either add the VPC resource, switch to a UUID or ${vpc.id} template, or remove the vpc_ref", + spec.Name, vpcRef, + ), + }) + return diags + } + // Copilot review #6 (round 2): the resolved-name check above + // is necessary but not sufficient — the target must also be an + // infra.vpc resource. A vpc_ref pointing to a Droplet, App, or + // other non-VPC resource silently passed prior validation; + // surface it as a typed Error so the operator catches the + // type-mismatch at plan time rather than getting a confusing + // 4xx from the DO database API. + if target.spec.Type != "infra.vpc" { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "DO database %q references vpc_ref %q whose type is %q; vpc_ref must point to an infra.vpc resource", + spec.Name, vpcRef, target.spec.Type, + ), + }) + } + return diags +} + +// planResource is the cross-resource lookup record used by ValidatePlan. +// Carries both the desired spec and the optional current state so a plan +// where an App Platform references an existing-and-unchanged VPC still +// resolves the VPC's region. +type planResource struct { + spec interfaces.ResourceSpec + current *interfaces.ResourceState +} + +// appendAppPlatformDiagnostics emits region-constraint diagnostics for an +// infra.container_service action's region/vpc_ref pair. +func appendAppPlatformDiagnostics( + diags []interfaces.PlanDiagnostic, + spec interfaces.ResourceSpec, + region string, + byName map[string]planResource, +) []interfaces.PlanDiagnostic { + if region != "" && !isAppPlatformRegionGroup(region) { + // Copilot review #16 (round 5): two-bucket severity + // matching the VPC/Droplet/Volume case above: + // (a) operator typed a known zone slug ("nyc3") where a + // group ("nyc") is required — documented misconfig, + // stays Error. + // (b) unknown slug — possibly a brand-new App Platform + // group the plugin's allowlist hasn't caught up to. + // Downgraded to Warning so operators can proceed + // (under non-strict align) without a plugin bump; + // --strict still escalates. + severity := interfaces.PlanDiagnosticWarning + if isZoneSlug(region) { + severity = interfaces.PlanDiagnosticError + } + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: severity, + Resource: spec.Name, + Field: "region", + Message: fmt.Sprintf( + "DO App Platform configures by region GROUP (nyc, ams, fra, sfo, sgp, syd, tor, blr, lon); got %q which is %s. Replace with the parent group slug.", + region, classifyRegion(region), + ), + }) + } + + vpcRef, _ := spec.Config["vpc_ref"].(string) + if vpcRef == "" || region == "" || !isAppPlatformRegionGroup(region) { + return diags + } + // Copilot review #8 (round 3): App Platform vpc_ref maps to + // godo.AppVpcSpec{ID: vpcID} — the DO Apps API expects a VPC UUID + // at apply time. ValidatePlan runs at PLAN time before JIT + // substitution, so vpc_ref values shaped like a UUID or a wfctl + // template (${vpc.id}) cannot be cross-resolved against byName + // without false positives. Only the plain-name branch is checked + // here; UUID/template values are deferred to apply-time + // validation. + if !looksLikeResourceName(vpcRef) { + return diags + } + target, ok := byName[vpcRef] + if !ok { + // vpc_ref points to a name not in the plan; cannot validate + // region match without state. Emit a Warning so an operator + // configuring a fresh deployment sees the gap; non-strict + // runs continue. + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticWarning, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "App Platform %q references vpc_ref %q (a plain resource name) which is not declared in the same plan; region-match check skipped", + spec.Name, vpcRef, + ), + }) + return diags + } + // Copilot review #7 (round 2): the resolved-name check above is + // necessary but not sufficient — the target must also be an + // infra.vpc resource. A vpc_ref pointing to a Droplet, App, or + // other non-VPC resource would silently bypass the region-match + // check (because target.spec.Config["region"] would be a + // zone slug for a Droplet, not a VPC region) and the operator + // would never see a clear diagnostic. Surface as Error so the + // type-mismatch is caught at plan time. + if target.spec.Type != "infra.vpc" { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "App Platform %q references vpc_ref %q whose type is %q; vpc_ref must point to an infra.vpc resource", + spec.Name, vpcRef, target.spec.Type, + ), + }) + return diags + } + // Resolve the referenced VPC's region. Prefer the desired spec + // (config the user is actively declaring) and fall back to the + // current state's outputs (existing VPC adoption). + vpcRegion, _ := target.spec.Config["region"].(string) + if vpcRegion == "" && target.current != nil { + if v, ok := target.current.Outputs["region"].(string); ok { + vpcRegion = v + } + } + if vpcRegion == "" { + // Cannot determine VPC region — silent, not a finding (planning + // pipeline upstream may not have populated yet). + return diags + } + if appPlatformRegionGroupOf(vpcRegion) != region { + diags = append(diags, interfaces.PlanDiagnostic{ + Severity: interfaces.PlanDiagnosticError, + Resource: spec.Name, + Field: "vpc_ref", + Message: fmt.Sprintf( + "App Platform region %q is incompatible with VPC %q region %q. App Platform region group %q only routes to VPCs in zones %s.", + region, vpcRef, vpcRegion, region, strings.Join(zonesInGroup(region), ", "), + ), + }) + } + return diags +} + +// isAppPlatformRegionGroup reports whether s is a DO App Platform region +// group slug (the parent of one or more zone slugs). +func isAppPlatformRegionGroup(s string) bool { + _, ok := appPlatformGroupZones[s] + return ok +} + +// isZoneSlug reports whether s is a DO zone slug (e.g., nyc1, sfo3). +// Zone slugs are the value DO's API expects for VPC, Droplet, Volume, +// Database, Spaces. +func isZoneSlug(s string) bool { + _, ok := zoneToGroup[s] + return ok +} + +// appPlatformRegionGroupOf returns the App Platform region group that +// contains a zone slug. Returns "" for unknown zones. +func appPlatformRegionGroupOf(zone string) string { + return zoneToGroup[zone] +} + +// zonesInGroup returns a sorted (lexicographic) copy of the zone slugs +// inside a group, for use in diagnostic messages where deterministic +// ordering matters (test assertions, copy-paste reproducibility). +// Returns nil for unknown groups. The returned slice is owned by the +// caller and safe to mutate. +// +// Copilot review #3: the prior implementation returned the underlying +// slice without sorting; the docstring promised sorted output but did +// not deliver it. +func zonesInGroup(group string) []string { + src := appPlatformGroupZones[group] + if len(src) == 0 { + return nil + } + out := make([]string, len(src)) + copy(out, src) + sort.Strings(out) + return out +} + +// classifyRegion returns a short human-readable label for a region slug +// to help diagnostic messages distinguish bare-group from zone-slug +// confusion. +// +// Copilot review #10 (round 3): zoneToGroup intentionally maps some +// legacy zones (nyc2, ams2) to an empty group string so they pass +// is-a-zone-slug validation without claiming App Platform routing. +// The prior implementation surfaced these as 'a zone slug in group ""' +// which renders as a confusing empty-name label. The empty-group case +// is now special-cased. +func classifyRegion(s string) string { + if isAppPlatformRegionGroup(s) { + return fmt.Sprintf("a region GROUP (zones: %s)", strings.Join(zonesInGroup(s), ", ")) + } + if isZoneSlug(s) { + group := appPlatformRegionGroupOf(s) + if group == "" { + return "a zone slug not in any App Platform region group" + } + return fmt.Sprintf("a zone slug in group %q", group) + } + return "not a recognized DO region slug" +} + +// appPlatformGroupZones maps each App Platform region group to the +// zone slugs it routes to. Source: DO docs (Apps API Region object) + +// Apps API region listing as of 2026-05. +var appPlatformGroupZones = map[string][]string{ + "nyc": {"nyc1", "nyc3"}, + "ams": {"ams3"}, + "fra": {"fra1"}, + "sfo": {"sfo2", "sfo3"}, + "sgp": {"sgp1"}, + "syd": {"syd1"}, + "tor": {"tor1"}, + "blr": {"blr1"}, + "lon": {"lon1"}, +} + +// zoneToGroup is the inverse mapping computed at init() so lookups are +// O(1). Kept distinct from appPlatformGroupZones so the source-of-truth +// (group → zones) stays single-direction. +var zoneToGroup = func() map[string]string { + m := make(map[string]string, 16) + for group, zones := range appPlatformGroupZones { + for _, z := range zones { + m[z] = group + } + } + // Additional zones DO supports that don't currently route to App + // Platform (legacy nyc2, ams2). Including them lets the zone-slug + // validation accept these without flagging valid VPC/Droplet/Volume + // configs — they simply have no App Platform routing. + m["nyc2"] = "" + m["ams2"] = "" + return m +}() + +// Compile-time assertion: DOProvider implements ProviderValidator. +var _ interfaces.ProviderValidator = (*DOProvider)(nil) diff --git a/internal/validate_plan_test.go b/internal/validate_plan_test.go new file mode 100644 index 0000000..6dd3156 --- /dev/null +++ b/internal/validate_plan_test.go @@ -0,0 +1,550 @@ +package internal + +import ( + "strings" + "testing" + + "github.com/GoCodeAlone/workflow/interfaces" +) + +// TestDOProvider_ValidatePlan_NilAndEmpty asserts the contract that +// ValidatePlan tolerates a nil plan and an empty actions slice without +// panicking; both return zero diagnostics. R-A10 invokes ValidatePlan +// before any align-time enrichment so the early-return cases must hold. +func TestDOProvider_ValidatePlan_NilAndEmpty(t *testing.T) { + p := NewDOProvider() + if d := p.ValidatePlan(nil); len(d) != 0 { + t.Errorf("nil plan: expected 0 diagnostics, got %d: %+v", len(d), d) + } + if d := p.ValidatePlan(&interfaces.IaCPlan{}); len(d) != 0 { + t.Errorf("empty plan: expected 0 diagnostics, got %d: %+v", len(d), d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformGroupSlugAccepted asserts that +// an App Platform action with a valid region GROUP slug ("nyc") emits no +// diagnostics. This is the canonical happy-path for AP — region groups +// are the App Platform API's required region shape. +func TestDOProvider_ValidatePlan_AppPlatformGroupSlugAccepted(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for AP region=nyc; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformZoneSlugRejected asserts that +// an App Platform action with a zone slug ("nyc3") gets a Severity=Error +// diagnostic explaining the AP-region-group requirement. This is the +// concrete root-cause-issue-D defense from the conformance design: an +// operator who copy-pasted nyc3 from a Droplet config gets a clear +// explanation pre-apply, instead of a cryptic DO API rejection at +// `wfctl infra apply`. +func TestDOProvider_ValidatePlan_AppPlatformZoneSlugRejected(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc3"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 { + t.Fatalf("expected 1 diagnostic for AP region=nyc3; got %d: %+v", len(d), d) + } + got := d[0] + if got.Severity != interfaces.PlanDiagnosticError { + t.Errorf("severity = %d, want Error (%d)", got.Severity, interfaces.PlanDiagnosticError) + } + if got.Resource != "core-app" || got.Field != "region" { + t.Errorf("diagnostic resource/field = %q/%q, want core-app/region", got.Resource, got.Field) + } + if !strings.Contains(got.Message, "region GROUP") { + t.Errorf("message should explain the group requirement; got %q", got.Message) + } + if !strings.Contains(got.Message, "nyc3") { + t.Errorf("message should cite the offending value nyc3; got %q", got.Message) + } +} + +// TestDOProvider_ValidatePlan_VPCRequiresZone asserts that a VPC with a +// bare group slug ("nyc") gets a Severity=Error diagnostic — the DO VPC +// API requires a specific zone, the inverse of the App Platform +// constraint above. +func TestDOProvider_ValidatePlan_VPCRequiresZone(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 || d[0].Severity != interfaces.PlanDiagnosticError { + t.Fatalf("expected 1 Error diagnostic for VPC region=nyc; got %+v", d) + } + if !strings.Contains(d[0].Message, "zone slug") { + t.Errorf("message should explain the zone-slug requirement; got %q", d[0].Message) + } +} + +// TestDOProvider_ValidatePlan_DropletAndVolumeAcceptZone asserts the +// happy path for the inverse direction — Droplet/Volume with valid zone +// slugs emit no diagnostics. +func TestDOProvider_ValidatePlan_DropletAndVolumeAcceptZone(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-droplet", Type: "infra.droplet", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vol", Type: "infra.volume", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc1"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for zone-slug-armed plan; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMismatch is the +// flagship test: App Platform group "nyc" with a vpc_ref pointing to a +// VPC declared in zone "sfo3" must produce an Error diagnostic with the +// vpc_ref field path. This locks the recurring "App Platform in nyc +// cannot reach VPC in sfo3" production bug class (root-cause issue D). +func TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMismatch(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "sfo3"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "core-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 { + t.Fatalf("expected 1 diagnostic for AP/VPC region mismatch; got %d: %+v", len(d), d) + } + if d[0].Severity != interfaces.PlanDiagnosticError { + t.Errorf("severity = %d, want Error", d[0].Severity) + } + if d[0].Resource != "core-app" || d[0].Field != "vpc_ref" { + t.Errorf("resource/field = %q/%q, want core-app/vpc_ref", d[0].Resource, d[0].Field) + } + for _, want := range []string{"nyc", "sfo3", "core-vpc"} { + if !strings.Contains(d[0].Message, want) { + t.Errorf("message should cite %q; got %q", want, d[0].Message) + } + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMatchHappyPath +// covers the inverse: AP group "nyc" + VPC zone "nyc1" → no diagnostic. +// Locks the contract that the cross-resource check is restricted to +// genuine mismatches. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefRegionMatchHappyPath(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "core-vpc"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for AP nyc + VPC nyc1; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefUnknownEmitsWarning +// asserts that vpc_ref pointing to a name not in the plan emits a +// Severity=Warning (not Error) — the operator may have an existing VPC +// whose region is not yet resolved, so the strict mode escalates while +// non-strict tolerates. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefUnknownEmitsWarning(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "missing-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 || d[0].Severity != interfaces.PlanDiagnosticWarning { + t.Fatalf("expected 1 Warning diagnostic for unknown vpc_ref; got %+v", d) + } + if d[0].Field != "vpc_ref" || !strings.Contains(d[0].Message, "missing-vpc") { + t.Errorf("message should cite the unknown vpc_ref name; got %+v", d[0]) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefResolvesCurrentState +// asserts that the cross-resource region check looks at action.Current's +// outputs when no desired region is set on the VPC's spec — the typical +// shape when an existing VPC is unchanged but referenced by a new +// AppPlatform action. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefResolvesCurrentState(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + { + Action: "update", + Resource: interfaces.ResourceSpec{ + Name: "existing-vpc", Type: "infra.vpc", + // No region in the desired spec — must fall back to current state. + Config: map[string]any{}, + }, + Current: &interfaces.ResourceState{ + Name: "existing-vpc", Type: "infra.vpc", + Outputs: map[string]any{"region": "sfo3"}, + }, + }, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "existing-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 || d[0].Severity != interfaces.PlanDiagnosticError { + t.Fatalf("expected 1 Error for existing-VPC region mismatch via current state; got %+v", d) + } + if !strings.Contains(d[0].Message, "sfo3") { + t.Errorf("message should cite the current-state region sfo3; got %q", d[0].Message) + } +} + +// TestDOProvider_ValidatePlan_DeleteActionsSkipped asserts that delete +// actions with empty Config don't trigger false positives. A delete plan +// targets the existing resource for removal — the "desired" spec carries +// no region (user is asking for absence), so region constraints would +// always fire on every delete if not skipped. +func TestDOProvider_ValidatePlan_DeleteActionsSkipped(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "delete", Resource: interfaces.ResourceSpec{ + Name: "old-app", Type: "infra.container_service", Config: map[string]any{}, + }, Current: &interfaces.ResourceState{Name: "old-app", Type: "infra.container_service", ProviderID: "app-1"}}, + {Action: "delete", Resource: interfaces.ResourceSpec{ + Name: "old-vpc", Type: "infra.vpc", Config: map[string]any{}, + }, Current: &interfaces.ResourceState{Name: "old-vpc", Type: "infra.vpc", ProviderID: "vpc-1"}}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for delete-only plan; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_DatabaseDanglingVPCRef asserts the +// regression-pin case the conformance suite locks +// (Scenario_CrossResourceConstraintRejection): a database with a +// vpc_ref pointing to a name not present in the plan emits at least +// one Severity=Error diagnostic. This is the in-tree mirror of the +// portable conformance assertion. +func TestDOProvider_ValidatePlan_DatabaseDanglingVPCRef(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "missing-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) == 0 { + t.Fatal("expected at least one diagnostic for dangling vpc_ref") + } + var hasError bool + for _, x := range d { + if x.Message == "" { + t.Errorf("diagnostic message must be non-empty: %+v", x) + } + if x.Severity == interfaces.PlanDiagnosticError { + hasError = true + } + } + if !hasError { + t.Errorf("expected at least one Severity=Error diagnostic; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_DatabaseVPCRefToDeleteTargetIsDangling +// pins Copilot review #1: a vpc_ref pointing to a VPC that is being +// deleted in the same plan must be treated as a dangling reference +// (Severity=Error), NOT silently accepted as if the VPC were live. +// The byName index must exclude delete-action resources for +// cross-resource resolution to work correctly. +func TestDOProvider_ValidatePlan_DatabaseVPCRefToDeleteTargetIsDangling(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + // VPC scheduled for deletion in this plan. + { + Action: "delete", + Resource: interfaces.ResourceSpec{Name: "old-vpc", Type: "infra.vpc", Config: map[string]any{}}, + Current: &interfaces.ResourceState{ + Name: "old-vpc", Type: "infra.vpc", ProviderID: "vpc-old", + Outputs: map[string]any{"region": "nyc1"}, + }, + }, + // Database referencing the soon-to-be-deleted VPC. + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "old-vpc"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) == 0 { + t.Fatal("expected at least one diagnostic for vpc_ref to delete-target") + } + var hasDanglingError bool + for _, x := range d { + if x.Severity == interfaces.PlanDiagnosticError && x.Field == "vpc_ref" && x.Resource == "db" { + hasDanglingError = true + } + } + if !hasDanglingError { + t.Errorf("expected Severity=Error vpc_ref diagnostic on db resource; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_DatabaseVPCRefInPlanNoFinding asserts +// the inverse — when a database vpc_ref resolves to an in-plan VPC, +// no diagnostic is emitted (well-formed plan, happy path). +func TestDOProvider_ValidatePlan_DatabaseVPCRefInPlanNoFinding(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "core-vpc"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for in-plan vpc_ref; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_DatabaseVPCRefToNonVPCType pins +// Copilot review #6 (round 2): a database vpc_ref pointing to a name +// that resolves in the plan but to a NON-VPC type (e.g., a droplet) +// must Error. Type-checking at plan time avoids confusing 4xx errors +// from the DO database API at apply time. +func TestDOProvider_ValidatePlan_DatabaseVPCRefToNonVPCType(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-droplet", Type: "infra.droplet", + Config: map[string]any{"region": "nyc1"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "core-droplet"}, + }}, + }} + d := p.ValidatePlan(plan) + var hasTypeError bool + for _, x := range d { + if x.Severity == interfaces.PlanDiagnosticError && + x.Resource == "db" && x.Field == "vpc_ref" && + strings.Contains(x.Message, "infra.droplet") { + hasTypeError = true + } + } + if !hasTypeError { + t.Errorf("expected Severity=Error with infra.droplet in message; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType pins +// Copilot review #7 (round 2): same type-checking for App Platform +// vpc_ref pointing to a non-VPC resource (e.g., another App Platform +// app). Without this check the region-match logic would silently skip +// because target.spec.Config["region"] would be a region GROUP for an +// App Platform target, not a zone. +func TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "other-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "other-app"}, + }}, + }} + d := p.ValidatePlan(plan) + var hasTypeError bool + for _, x := range d { + if x.Severity == interfaces.PlanDiagnosticError && + x.Resource == "core-app" && x.Field == "vpc_ref" && + strings.Contains(x.Message, "infra.container_service") { + hasTypeError = true + } + } + if !hasTypeError { + t.Errorf("expected Severity=Error with infra.container_service in message; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_VPCRefAsUUIDIsDeferred pins Copilot +// review #8/#9 (round 3): vpc_ref values that look like a VPC UUID +// (the canonical DO Apps/Databases API shape at apply time) MUST NOT +// trigger the in-plan-name-resolution branch — they are deferred to +// apply-time validation by wfctlhelpers.ApplyPlan after JIT +// substitution. The test exercises both the database and the App +// Platform paths with a fixed UUID literal. +func TestDOProvider_ValidatePlan_VPCRefAsUUIDIsDeferred(t *testing.T) { + const vpcUUID = "a1b2c3d4-e5f6-7890-abcd-ef0123456789" + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": vpcUUID}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": vpcUUID}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for UUID vpc_ref values; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_VPCRefAsUpperCaseUUIDIsDeferred pins +// Copilot review #12 (round 4): UUIDs are case-insensitive; an +// upper-case or mixed-case VPC UUID must also be classified as a +// UUID literal (not as a plain resource name) so it does not trigger +// false dangling-reference diagnostics. Verifies the (?i) flag on +// uuidPattern. +func TestDOProvider_ValidatePlan_VPCRefAsUpperCaseUUIDIsDeferred(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "A1B2C3D4-E5F6-7890-ABCD-EF0123456789"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "A1b2C3d4-E5f6-7890-AbCd-Ef0123456789"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for upper/mixed-case UUID vpc_ref; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_VPCRefAsJITTemplateIsDeferred pins +// Copilot review #8/#9 (round 3): vpc_ref values that contain the +// wfctl JIT substitution syntax (${MODULE.field} or $(...)) MUST NOT +// trigger name-based resolution at plan time — they are resolved by +// wfctlhelpers.ApplyPlan's jitsubst.ResolveSpec at apply time. The +// test exercises both the database and the App Platform paths with +// canonical ${vpc.id} templates. +func TestDOProvider_ValidatePlan_VPCRefAsJITTemplateIsDeferred(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "db", Type: "infra.database", + Config: map[string]any{"vpc_ref": "${some-vpc.id}"}, + }}, + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + Config: map[string]any{"region": "nyc", "vpc_ref": "$(other-vpc.id)"}, + }}, + }} + if d := p.ValidatePlan(plan); len(d) != 0 { + t.Errorf("expected 0 diagnostics for JIT-template vpc_ref values; got %+v", d) + } +} + +// TestDOProvider_ValidatePlan_ClassifyRegionEmptyGroup pins Copilot +// review #10 (round 3): a Droplet/VPC region set to a zone whose +// zoneToGroup mapping is the empty string (e.g., legacy nyc2/ams2) +// must produce a clear human-readable diagnostic, not 'a zone slug in +// group ""'. This test indirectly verifies via the message content of +// the bare-group rejection on a Droplet using nyc2. +// +// Note: nyc2 IS a valid zone slug (passes isZoneSlug), so a Droplet +// with region=nyc2 alone would pass validation. We exercise the +// empty-group branch via the App Platform group-vs-zone mismatch path +// where classifyRegion is called on a non-AP-routed zone. +func TestDOProvider_ValidatePlan_ClassifyRegionEmptyGroup(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "core-app", Type: "infra.container_service", + // nyc2 is a valid zone slug but not in any AP group. + Config: map[string]any{"region": "nyc2"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 1 { + t.Fatalf("expected 1 diagnostic for AP region=nyc2; got %d: %+v", len(d), d) + } + if strings.Contains(d[0].Message, `group ""`) { + t.Errorf("diagnostic still emits empty-group label; got %q", d[0].Message) + } + if !strings.Contains(d[0].Message, "not in any App Platform region group") { + t.Errorf("expected 'not in any App Platform region group' phrasing; got %q", d[0].Message) + } +} + +// TestDOProvider_ValidatePlan_UnknownRegionSlugWarnsNotErrors pins +// Copilot review #16 (round 5): a region slug that is neither a known +// App Platform group nor a known zone (e.g., a brand-new DO region +// 'atl1' the plugin's hardcoded allowlist hasn't caught up to) MUST +// downgrade to Severity=Warning so non-strict align lets operators +// proceed without a plugin bump. The documented misconfig cases +// (group-where-zone-required, zone-where-group-required) keep +// Severity=Error. +func TestDOProvider_ValidatePlan_UnknownRegionSlugWarnsNotErrors(t *testing.T) { + p := NewDOProvider() + plan := &interfaces.IaCPlan{Actions: []interfaces.PlanAction{ + // VPC with unknown region — should Warning (forward-compat). + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "fwd-vpc", Type: "infra.vpc", + Config: map[string]any{"region": "atl1"}, + }}, + // AP with unknown region — should Warning (forward-compat). + {Action: "create", Resource: interfaces.ResourceSpec{ + Name: "fwd-app", Type: "infra.container_service", + Config: map[string]any{"region": "atl"}, + }}, + }} + d := p.ValidatePlan(plan) + if len(d) != 2 { + t.Fatalf("expected 2 diagnostics for unknown slugs; got %d: %+v", len(d), d) + } + for _, x := range d { + if x.Severity != interfaces.PlanDiagnosticWarning { + t.Errorf("expected Severity=Warning for unknown slug; got %v in %+v", x.Severity, x) + } + } +} + +// TestDOProvider_ValidatePlan_CompileTimeAssertion documents that the +// compile-time interface assertion in validate_plan.go locks +// DOProvider's ProviderValidator implementation. If the interface +// signature changes upstream, the compile-time check catches the drift +// before any test runs. This test exists to make the assertion visible +// in test output and to fail the build if ValidatePlan goes missing. +func TestDOProvider_ValidatePlan_CompileTimeAssertion(t *testing.T) { + var _ interfaces.ProviderValidator = NewDOProvider() +} diff --git a/plugin.json b/plugin.json index bd6b7e4..ea67eae 100644 --- a/plugin.json +++ b/plugin.json @@ -1,6 +1,6 @@ { "name": "workflow-plugin-digitalocean", - "version": "0.9.0", + "version": "0.10.0", "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", @@ -14,24 +14,27 @@ { "os": "linux", "arch": "amd64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-linux-amd64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-linux-amd64.tar.gz" }, { "os": "linux", "arch": "arm64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-linux-arm64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-linux-arm64.tar.gz" }, { "os": "darwin", "arch": "amd64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-darwin-amd64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-darwin-amd64.tar.gz" }, { "os": "darwin", "arch": "arm64", - "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.9.0/workflow-plugin-digitalocean-darwin-arm64.tar.gz" + "url": "https://github.com/GoCodeAlone/workflow-plugin-digitalocean/releases/download/v0.10.0/workflow-plugin-digitalocean-darwin-arm64.tar.gz" } ], + "iacProvider": { + "computePlanVersion": "v2" + }, "capabilities": { "configProvider": false, "moduleTypes": ["iac.provider"],