From 47bdbeecf2729e3a02f4a9abaef2b8493f5586fa Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 22:37:26 -0400 Subject: [PATCH 1/5] feat(do-plugin): honor expose: internal for App Platform services (F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds canonical `expose: internal` opt-in for infra.container_service. When set, buildAppSpec zeroes HTTPPort, folds http_port into InternalPorts, and drops Routes — making the service reachable only from sibling components via DO App Platform's internal DNS (`.internal:`). Misconfiguration guard: declaring both http_port and internal_ports with conflicting values returns an explicit error at plan time rather than silently dropping the http port. Default behaviour (`expose: public` / unset) is unchanged — existing HTTPPort + Routes auto-creation continues to work. P-2.F4 from docs/plans/2026-04-27-iac-do-staging-implementation.md. Co-authored-by: Claude --- CHANGELOG.md | 20 ++++ internal/drivers/app_platform_buildspec.go | 57 ++++++++- .../drivers/app_platform_buildspec_test.go | 113 ++++++++++++++++++ plugin.json | 12 +- 4 files changed, 200 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d93d0b..5ddb965 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ All notable changes to workflow-plugin-digitalocean are documented here. +## [Unreleased] + +### Added + +- **`expose: internal` on `infra.container_service`** (P-2.F4) — App Platform + services may now declare `expose: internal` to opt out of the public edge + route. When set, `buildAppSpec` zeroes `HTTPPort`, folds `http_port` into + `InternalPorts` (so siblings can dial it), and drops `Routes` entirely. The + service becomes reachable only from sibling components in the same app via + DO App Platform's internal DNS (`.internal:`). Default + remains `expose: public`. + + Misconfiguration guard: declaring both `http_port` and `internal_ports` with + conflicting values under `expose: internal` returns the explicit error + `internal_ports must include http_port when both are set; use one or the + other` at plan time, rather than silently dropping the http port. + + This unblocks core-dump P-1's NATS sidecar and any other backing-service + component that must not face the open internet. + ## [v0.7.9] - 2026-04-24 ### Added diff --git a/internal/drivers/app_platform_buildspec.go b/internal/drivers/app_platform_buildspec.go index 4bbc497..94c7d6a 100644 --- a/internal/drivers/app_platform_buildspec.go +++ b/internal/drivers/app_platform_buildspec.go @@ -24,7 +24,7 @@ func buildAppSpec(name string, cfg map[string]any, region string) (*godo.AppSpec return nil, fmt.Errorf("app platform image config: %w", err) } - httpPort, _ := intFromConfig(cfg, "http_port", 8080) + httpPort, httpPortSet := intFromConfig(cfg, "http_port", 8080) instanceCount, _ := intFromConfig(cfg, "instance_count", 1) svc := &godo.AppServiceSpec{ @@ -50,6 +50,13 @@ func buildAppSpec(name string, cfg map[string]any, region string) (*godo.AppSpec Alerts: componentAlertsFromConfig(cfg), } + // Apply expose: internal semantics — see exposeFromConfig comment. + // Sibling services reach an internal component via `.internal:` + // (DO App Platform runtime contract). + if err := applyExposeInternal(svc, cfg, httpPort, httpPortSet); err != nil { + return nil, fmt.Errorf("app platform expose config: %w", err) + } + // Extract provider_specific.digitalocean overrides for top-level AppSpec fields. var ( disableEdgeCache bool @@ -152,6 +159,54 @@ func internalPortsFromConfig(cfg map[string]any) []int64 { return out } +// exposeFromConfig returns the canonical exposure mode for a container service: +// "public" (default — service has an edge HTTP port and may carry routes) or +// "internal" (no edge port; reachable only from sibling services via App +// Platform's internal DNS at `.internal:`). An empty string means +// the field was omitted; treat as "public". +func exposeFromConfig(cfg map[string]any) string { + v, ok := cfg["expose"].(string) + if !ok { + return "" + } + return strings.ToLower(strings.TrimSpace(v)) +} + +// applyExposeInternal mutates svc when `expose: internal` is set: +// - HTTPPort is zeroed so DO does not provision a public edge route. +// - http_port (if any) is folded into InternalPorts so siblings can dial it. +// - Routes is dropped — `internal` promises no public surface, even if the +// caller supplied a routes list. +// +// When the user declared both http_port and internal_ports with conflicting +// values, return an error so misconfiguration fails fast at plan time rather +// than silently dropping the http_port. +func applyExposeInternal(svc *godo.AppServiceSpec, cfg map[string]any, httpPort int, httpPortSet bool) error { + if exposeFromConfig(cfg) != "internal" { + return nil + } + // Detect mismatched http_port vs internal_ports before mutating anything. + if httpPortSet { + hp := int64(httpPort) + hasMatch := false + for _, p := range svc.InternalPorts { + if p == hp { + hasMatch = true + break + } + } + if len(svc.InternalPorts) > 0 && !hasMatch { + return fmt.Errorf("internal_ports must include http_port when both are set; use one or the other") + } + if !hasMatch { + svc.InternalPorts = append(svc.InternalPorts, hp) + } + } + svc.HTTPPort = 0 + svc.Routes = nil + return nil +} + // routesFromConfig converts the canonical "routes" list to []*godo.AppRouteSpec. func routesFromConfig(cfg map[string]any) []*godo.AppRouteSpec { raw, ok := cfg["routes"].([]any) diff --git a/internal/drivers/app_platform_buildspec_test.go b/internal/drivers/app_platform_buildspec_test.go index ba2062d..f45d924 100644 --- a/internal/drivers/app_platform_buildspec_test.go +++ b/internal/drivers/app_platform_buildspec_test.go @@ -1,6 +1,7 @@ package drivers_test import ( + "strings" "testing" "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" @@ -198,6 +199,118 @@ func TestBuildAppSpec_InternalPorts(t *testing.T) { } } +// ── Expose: internal ───────────────────────────────────────────────────────── +// +// `expose: internal` makes a service reachable only via App Platform's internal +// service-DNS (`.internal:` from sibling components) — no public +// route, no edge HTTP port. This is the contract relied on by core-dump P-1's +// NATS sidecar (and any backing-service component that must not face the open +// internet). Implementation contract: +// +// - HTTPPort is zeroed so DO does not provision an edge route. +// - InternalPorts contains the (single) port siblings dial. +// - Routes is nil — DO would otherwise auto-create a default `/` route. +// +// Cross-component reach: with `expose: internal`, sibling services in the same +// app reach this component at `.internal:` (DO App Platform +// runtime semantics). There is no code under test for the DNS path; this is a +// platform contract documented here for downstream readers. + +func TestBuildAppSpec_Expose_Internal_HTTPPort(t *testing.T) { + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 4222, + "expose": "internal", + } + spec := buildSpecViaCreate(t, cfg) + svc := spec.Services[0] + if svc.HTTPPort != 0 { + t.Errorf("HTTPPort = %d, want 0 (internal services must not expose an edge port)", svc.HTTPPort) + } + if len(svc.InternalPorts) != 1 || svc.InternalPorts[0] != 4222 { + t.Errorf("InternalPorts = %v, want [4222] (http_port copied to internal_ports)", svc.InternalPorts) + } + if len(svc.Routes) != 0 { + t.Errorf("Routes = %+v, want nil (no public routes for internal services)", svc.Routes) + } +} + +func TestBuildAppSpec_Expose_Internal_InternalPortsOnly(t *testing.T) { + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "internal_ports": []any{4222}, + "expose": "internal", + } + spec := buildSpecViaCreate(t, cfg) + svc := spec.Services[0] + if svc.HTTPPort != 0 { + t.Errorf("HTTPPort = %d, want 0", svc.HTTPPort) + } + if len(svc.InternalPorts) != 1 || svc.InternalPorts[0] != 4222 { + t.Errorf("InternalPorts = %v, want [4222]", svc.InternalPorts) + } + if len(svc.Routes) != 0 { + t.Errorf("Routes = %+v, want nil", svc.Routes) + } +} + +func TestBuildAppSpec_Expose_Internal_ExplicitRoutesDropped(t *testing.T) { + // Even if the caller passes routes, expose: internal must drop them — the + // promise of `internal` is "no public surface". + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 4222, + "expose": "internal", + "routes": []any{map[string]any{"path": "/"}}, + } + spec := buildSpecViaCreate(t, cfg) + if len(spec.Services[0].Routes) != 0 { + t.Errorf("Routes = %+v, want nil (expose: internal must drop user-supplied routes)", spec.Services[0].Routes) + } +} + +func TestBuildAppSpec_Expose_Internal_PortMismatchFails(t *testing.T) { + // Catch the user error of declaring both http_port and internal_ports + // where they disagree — the user almost certainly meant to use one or + // the other, not both with different ports. + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 4222, + "internal_ports": []any{9999}, + "expose": "internal", + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatal("expected error for mismatched http_port vs internal_ports under expose: internal, got nil") + } + wantSubstr := "internal_ports must include http_port when both are set; use one or the other" + if !strings.Contains(err.Error(), wantSubstr) { + t.Errorf("error message = %q, want it to contain %q", err.Error(), wantSubstr) + } +} + +func TestBuildAppSpec_Expose_Public_Default(t *testing.T) { + // expose unset (or "public") preserves the prior behaviour: HTTPPort set, + // no InternalPorts unless explicitly configured, default route possible. + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 8080, + } + spec := buildSpecViaCreate(t, cfg) + svc := spec.Services[0] + if svc.HTTPPort != 8080 { + t.Errorf("HTTPPort = %d, want 8080 (default expose=public should leave HTTPPort unchanged)", svc.HTTPPort) + } + if len(svc.InternalPorts) != 0 { + t.Errorf("InternalPorts = %v, want empty (default expose=public should not auto-populate)", svc.InternalPorts) + } +} + // ── Routes ─────────────────────────────────────────────────────────────────── func TestBuildAppSpec_Routes(t *testing.T) { diff --git a/plugin.json b/plugin.json index 8edf27e..c7a357a 100644 --- a/plugin.json +++ b/plugin.json @@ -32,7 +32,17 @@ "infra.droplet", "infra.iam_role", "infra.api_gateway" - ] + ], + "configSchema": { + "infra.container_service": { + "expose": { + "type": "string", + "enum": ["public", "internal"], + "default": "public", + "description": "Whether the service has a public edge route (public, default) or is reachable only from sibling components via DO App Platform internal DNS (.internal:) (internal)." + } + } + } } } } From 370a98b8a61bf2ba269ad611e27d96c73d48e50b Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 22:57:53 -0400 Subject: [PATCH 2/5] fix(do-plugin): expose validation + Diff cascade (F4 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code-reviewer Findings 1, 2, 3 on PR #35. Finding 1 — Diff cascade (Important): in-place public↔internal toggles now produce a Plan action. appOutput records Outputs["expose"] derived from the live AppSpec (HTTPPort==0 with InternalPorts populated → "internal"; otherwise "public"); Diff compares it so security-relevant toggles surface in plan output rather than silently no-op'ing. Pre-F4 state without recorded expose is treated as "public" for the comparison. Finding 2 — Reachability fail-fast: expose: internal with neither http_port nor internal_ports set would produce a service with no listening port. Now rejected with `expose: internal requires http_port or internal_ports to be set`. Finding 3 — Enum validation: typo'd values like `intenral` or unsupported values like `private` no longer silently fall through to public. Returns `expose: %q invalid; must be one of [public, internal]`. Tests: 5 new test cases (3 buildspec, 2 Diff, 1 appOutput sub-table) — all RED first, GREEN after impl, regression-invariant verified by stash/restore of app_platform.go + app_platform_buildspec.go. Co-authored-by: Claude --- CHANGELOG.md | 25 +++- internal/drivers/app_platform.go | 56 +++++++++ internal/drivers/app_platform_buildspec.go | 32 ++++- .../drivers/app_platform_buildspec_test.go | 50 ++++++++ internal/drivers/app_platform_test.go | 110 ++++++++++++++++++ 5 files changed, 264 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ddb965..b6307dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,10 +14,27 @@ All notable changes to workflow-plugin-digitalocean are documented here. DO App Platform's internal DNS (`.internal:`). Default remains `expose: public`. - Misconfiguration guard: declaring both `http_port` and `internal_ports` with - conflicting values under `expose: internal` returns the explicit error - `internal_ports must include http_port when both are set; use one or the - other` at plan time, rather than silently dropping the http port. + Misconfiguration guards (all reject at plan time before any DO API call): + - `expose` must be one of `[public, internal]`. Typos like `intenral` or + unsupported values like `private` return the error + `expose: %q invalid; must be one of [public, internal]` rather than + silently defaulting to public. + - `expose: internal` requires at least one of `http_port` or + `internal_ports` to be set. Setting `expose: internal` with no ports + would produce a service with no listening port — silently unreachable. + Returns `expose: internal requires http_port or internal_ports to be + set`. + - `http_port` and `internal_ports` set with disjoint values returns + `internal_ports must include http_port when both are set; use one or + the other`. + + Plan/Diff: `appOutput` now records `Outputs["expose"]` derived from the + live `AppSpec` (HTTPPort==0 with InternalPorts populated → "internal"; + otherwise "public"). `Diff` compares the canonical `expose` field so + in-place public↔internal toggles produce a Plan action with a + `FieldChange{Path: "expose"}`, rather than silently no-op'ing as the + pre-F4 image-only Diff would have done. Pre-F4 state without recorded + expose is treated as `public` for the comparison. This unblocks core-dump P-1's NATS sidecar and any other backing-service component that must not face the open internet. diff --git a/internal/drivers/app_platform.go b/internal/drivers/app_platform.go index 9c170a2..eb0a679 100644 --- a/internal/drivers/app_platform.go +++ b/internal/drivers/app_platform.go @@ -173,9 +173,40 @@ func (d *AppPlatformDriver) Diff(_ context.Context, desired interfaces.ResourceS changes = append(changes, interfaces.FieldChange{Path: "image", Old: curImg, New: img}) } } + // Compare canonical `expose` so in-place public↔internal toggles produce a + // Plan action rather than silently no-op'ing — quality-review F4 Finding 1. + // Desired side is the canonical string from cfg (default "public" when + // unset/empty). Current side is the value `appOutput` derived from the + // live AppSpec at last Read. + desiredExpose := canonicalExpose(desired.Config) + curExpose, _ := current.Outputs["expose"].(string) + if curExpose == "" { + // Pre-F4 state has no `expose` recorded; treat absence as public so a + // transition to `internal` is detected. + curExpose = "public" + } + if desiredExpose != curExpose { + changes = append(changes, interfaces.FieldChange{Path: "expose", Old: curExpose, New: desiredExpose}) + } return &interfaces.DiffResult{NeedsUpdate: len(changes) > 0, Changes: changes}, nil } +// canonicalExpose returns the canonical `expose` value for a desired-spec +// config: "public" (default when unset/empty), "internal", or whatever +// non-empty string the user supplied. Validation of the enum is performed +// later in `applyExposeInternal` during buildAppSpec. +func canonicalExpose(cfg map[string]any) string { + v, ok := cfg["expose"].(string) + if !ok { + return "public" + } + v = strings.ToLower(strings.TrimSpace(v)) + if v == "" { + return "public" + } + return v +} + func (d *AppPlatformDriver) HealthCheck(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.HealthResult, error) { providerID, err := d.resolveProviderID(ctx, ref) if err != nil { @@ -408,6 +439,11 @@ func appOutput(app *godo.App) *interfaces.ResourceOutput { ProviderID: app.ID, Outputs: map[string]any{ "live_url": app.LiveURL, + // `expose` is derived from the first service component: + // HTTPPort==0 with InternalPorts populated → "internal"; + // otherwise "public". Stored on Outputs so Diff can detect + // in-place public↔internal toggles — F4 Finding 1. + "expose": deriveExposeFromAppSpec(app.Spec), }, Status: "running", } @@ -417,6 +453,26 @@ func appOutput(app *godo.App) *interfaces.ResourceOutput { return out } +// deriveExposeFromAppSpec inspects the first service component of an AppSpec +// to determine whether the deployed app is public or internal. App Platform +// allows multiple services per app; the canonical `expose` key applies to the +// primary service (matching the pattern used in buildAppSpec where `svc` is +// services[0]). Apps with no services default to "public" (cannot be +// internal-only by definition). +func deriveExposeFromAppSpec(spec *godo.AppSpec) string { + if spec == nil || len(spec.Services) == 0 { + return "public" + } + svc := spec.Services[0] + if svc == nil { + return "public" + } + if svc.HTTPPort == 0 && len(svc.InternalPorts) > 0 { + return "internal" + } + return "public" +} + // ParseImageRef parses a flat image reference string into a DO App Platform // ImageSourceSpec. Supports: // diff --git a/internal/drivers/app_platform_buildspec.go b/internal/drivers/app_platform_buildspec.go index 94c7d6a..7e9a0d3 100644 --- a/internal/drivers/app_platform_buildspec.go +++ b/internal/drivers/app_platform_buildspec.go @@ -172,19 +172,41 @@ func exposeFromConfig(cfg map[string]any) string { return strings.ToLower(strings.TrimSpace(v)) } -// applyExposeInternal mutates svc when `expose: internal` is set: +// applyExposeInternal validates the canonical `expose` value and, when set to +// "internal", mutates svc: // - HTTPPort is zeroed so DO does not provision a public edge route. // - http_port (if any) is folded into InternalPorts so siblings can dial it. // - Routes is dropped — `internal` promises no public surface, even if the // caller supplied a routes list. // -// When the user declared both http_port and internal_ports with conflicting -// values, return an error so misconfiguration fails fast at plan time rather -// than silently dropping the http_port. +// Validation: `expose` must be one of {"", "public", "internal"}. Any other +// value (typo'd "intenral", unsupported "private", etc.) returns an enum +// error so misconfigurations fail at plan time rather than silently +// defaulting to public — quality-review F4 Finding 3. +// +// Reachability: when `expose: internal` is set with neither http_port nor +// internal_ports, the result would be a service with no listening port — +// silently unreachable. Reject this combination — quality-review F4 Finding 2. +// +// Port-mismatch: when both http_port and internal_ports are set under +// `expose: internal` with disjoint values, return an explicit error so the +// caller picks one. (Original F4 behaviour.) func applyExposeInternal(svc *godo.AppServiceSpec, cfg map[string]any, httpPort int, httpPortSet bool) error { - if exposeFromConfig(cfg) != "internal" { + exp := exposeFromConfig(cfg) + switch exp { + case "", "public": return nil + case "internal": + // fall through to the internal-mode mutation below + default: + return fmt.Errorf("expose: %q invalid; must be one of [public, internal]", exp) } + + // Internal-mode requires at least one port to be reachable from siblings. + if !httpPortSet && len(svc.InternalPorts) == 0 { + return fmt.Errorf("expose: internal requires http_port or internal_ports to be set") + } + // Detect mismatched http_port vs internal_ports before mutating anything. if httpPortSet { hp := int64(httpPort) diff --git a/internal/drivers/app_platform_buildspec_test.go b/internal/drivers/app_platform_buildspec_test.go index f45d924..aee44bb 100644 --- a/internal/drivers/app_platform_buildspec_test.go +++ b/internal/drivers/app_platform_buildspec_test.go @@ -294,6 +294,56 @@ func TestBuildAppSpec_Expose_Internal_PortMismatchFails(t *testing.T) { } } +func TestBuildAppSpec_Expose_Internal_NoPorts_Fails(t *testing.T) { + // expose: internal with neither http_port nor internal_ports would produce + // a service with no listening port — silently unreachable. Fail fast at + // plan time per the F4 fail-fast contract; quality-review Finding 2. + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "expose": "internal", + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatal("expected error for expose: internal with no ports, got nil") + } + wantSubstr := "expose: internal requires http_port or internal_ports to be set" + if !strings.Contains(err.Error(), wantSubstr) { + t.Errorf("error message = %q, want it to contain %q", err.Error(), wantSubstr) + } +} + +func TestBuildAppSpec_Expose_Invalid_Fails(t *testing.T) { + // Typo'd values like "intenral" or "private" must not silently fall + // through to public. Reject with an enum-listing error; quality-review + // Finding 3. + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 8080, + "expose": "intenral", // typo + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatal("expected error for invalid expose value, got nil") + } + // Error must mention the bad value and the allowed enum. + if !strings.Contains(err.Error(), `"intenral"`) { + t.Errorf("error %q should quote the bad value 'intenral'", err.Error()) + } + if !strings.Contains(err.Error(), "[public, internal]") { + t.Errorf("error %q should list the allowed enum [public, internal]", err.Error()) + } +} + func TestBuildAppSpec_Expose_Public_Default(t *testing.T) { // expose unset (or "public") preserves the prior behaviour: HTTPPort set, // no InternalPorts unless explicitly configured, default route possible. diff --git a/internal/drivers/app_platform_test.go b/internal/drivers/app_platform_test.go index 2be464c..1763512 100644 --- a/internal/drivers/app_platform_test.go +++ b/internal/drivers/app_platform_test.go @@ -350,6 +350,116 @@ func TestAppPlatformDriver_Diff_NoChanges(t *testing.T) { } } +// 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 +// pre-F4 image-only Diff did. +// +// Today appOutput populates Outputs["expose"] from the live AppSpec +// (HTTPPort==0 && len(InternalPorts)>0 → "internal" else "public"), and +// Diff compares it against the desired config. +func TestAppPlatformDriver_Diff_DetectsExposeChange_PublicToInternal(t *testing.T) { + mock := &mockAppClient{} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "expose": "public", + }, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 4222, + "expose": "internal", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsUpdate { + t.Fatal("expected NeedsUpdate=true when expose toggles public→internal") + } + // At least one change must reference "expose" so the user sees what changed. + found := false + for _, c := range result.Changes { + if c.Path == "expose" { + found = true + break + } + } + if !found { + t.Errorf("expected a FieldChange with Path=\"expose\"; got %+v", result.Changes) + } +} + +func TestAppPlatformDriver_Diff_DetectsExposeChange_InternalToPublic(t *testing.T) { + mock := &mockAppClient{} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "expose": "internal", + }, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 8080, + "expose": "public", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsUpdate { + t.Fatal("expected NeedsUpdate=true when expose toggles internal→public") + } +} + +// TestAppPlatformDriver_AppOutput_ExposeDerivedFromAppSpec verifies that the +// live-state derivation from godo.AppSpec is correct: HTTPPort==0 with +// InternalPorts populated → "internal"; everything else → "public". Without +// this, Diff comparing against current state can't tell whether a previously +// applied service is internal or public, so the toggle detection (above) +// would silently no-op on the round-trip. +func TestAppPlatformDriver_AppOutput_ExposeDerivedFromAppSpec(t *testing.T) { + internalApp := &godo.App{ + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + Spec: &godo.AppSpec{Name: "internal-app", Services: []*godo.AppServiceSpec{{Name: "svc", HTTPPort: 0, InternalPorts: []int64{4222}}}}, + ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, + } + publicApp := &godo.App{ + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb6", + Spec: &godo.AppSpec{Name: "public-app", Services: []*godo.AppServiceSpec{{Name: "svc", HTTPPort: 8080}}}, + ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, + } + + for _, tc := range []struct { + name string + app *godo.App + want string + }{ + {"internal_when_http_port_zero_and_internal_ports_set", internalApp, "internal"}, + {"public_when_http_port_set", publicApp, "public"}, + } { + t.Run(tc.name, func(t *testing.T) { + mock := &mockAppClient{app: tc.app} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + out, err := d.Read(context.Background(), interfaces.ResourceRef{ProviderID: tc.app.ID, Name: tc.app.Spec.Name}) + if err != nil { + t.Fatalf("Read: %v", err) + } + got, _ := out.Outputs["expose"].(string) + if got != tc.want { + t.Errorf("Outputs[\"expose\"] = %q, want %q", got, tc.want) + } + }) + } +} + func TestAppPlatformDriver_Create_EnvVars(t *testing.T) { mock := &mockAppClient{app: testApp()} d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") From bebe73599728d99668885130c3e5561a3d829629 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 23:22:35 -0400 Subject: [PATCH 3/5] fix(do-plugin): image output cascade + non-string expose (F4 round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot round-2 findings on PR #35. Finding A — image output cascade gap: Diff at app_platform.go:170-173 was reading current.Outputs["image"] but appOutput never populated it, so every reconcile of an unchanged service emitted a spurious image FieldChange forever. Now appOutput populates Outputs["image"] via new deriveImageFromAppSpec → formatImageSpec (the reverse of ParseImageRef). DOCR's lossy Registry round-trip is handled by Diff's new imageRefsEqual helper which parses both sides and compares RegistryType+Repository+Tag structurally, falling back to literal string compare when either side fails to parse. Finding B — non-string expose silently defaults to public: previous exposeFromConfig used `cfg["expose"].(string)` and returned "" when the value was a non-string (bool, int, map, slice). That silently treated the misconfiguration as "public" on a security-relevant toggle. Signature changed to return (string, error); applyExposeInternal propagates. New error: `expose: must be a string (one of [public, internal]), got `. Tests: - TestAppPlatformDriver_AppOutput_ImageDerivedFromAppSpec (5 sub-cases: DOCR-empty-registry, DOCR-with-registry, GHCR, DockerHub, nil-image). - TestAppPlatformDriver_Diff_NoSpurious_ImageChange_DOCR (round-trip no-op reconcile must not flag an image change). - TestBuildAppSpec_Expose_NonStringValue_Fails (5 sub-cases: bool true, bool false, int, map, slice). All RED first, GREEN after impl, regression-invariant verified by stash/restore of app_platform.go + app_platform_buildspec.go. Co-authored-by: Claude --- CHANGELOG.md | 23 ++- internal/drivers/app_platform.go | 98 +++++++++++- internal/drivers/app_platform_buildspec.go | 27 +++- .../drivers/app_platform_buildspec_test.go | 42 +++++ internal/drivers/app_platform_test.go | 151 ++++++++++++++++++ 5 files changed, 325 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6307dd..e4a09bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,10 +15,13 @@ All notable changes to workflow-plugin-digitalocean are documented here. remains `expose: public`. Misconfiguration guards (all reject at plan time before any DO API call): + - `expose` must be a string. Non-string values (accidental YAML bool + `true`, numbers, maps, etc.) return `expose: must be a string (one of + [public, internal]), got ` rather than silently defaulting to + public. - `expose` must be one of `[public, internal]`. Typos like `intenral` or - unsupported values like `private` return the error - `expose: %q invalid; must be one of [public, internal]` rather than - silently defaulting to public. + unsupported values like `private` return + `expose: %q invalid; must be one of [public, internal]`. - `expose: internal` requires at least one of `http_port` or `internal_ports` to be set. Setting `expose: internal` with no ports would produce a service with no listening port — silently unreachable. @@ -30,11 +33,15 @@ All notable changes to workflow-plugin-digitalocean are documented here. Plan/Diff: `appOutput` now records `Outputs["expose"]` derived from the live `AppSpec` (HTTPPort==0 with InternalPorts populated → "internal"; - otherwise "public"). `Diff` compares the canonical `expose` field so - in-place public↔internal toggles produce a Plan action with a - `FieldChange{Path: "expose"}`, rather than silently no-op'ing as the - pre-F4 image-only Diff would have done. Pre-F4 state without recorded - expose is treated as `public` for the comparison. + otherwise "public") AND `Outputs["image"]` formatted from the first + service's `ImageSourceSpec` via the new `formatImageSpec` reverse of + `ParseImageRef`. `Diff` compares `expose` against the canonical desired + value AND compares `image` structurally (RegistryType+Repository+Tag, + with parse-then-compare via `imageRefsEqual`) — so in-place + public↔internal toggles produce a Plan action with a + `FieldChange{Path: "expose"}`, and unchanged image refs no longer emit + spurious `image` `FieldChange`s on every reconcile. Pre-F4 state + without recorded `expose` is treated as `public` for the comparison. This unblocks core-dump P-1's NATS sidecar and any other backing-service component that must not face the open internet. diff --git a/internal/drivers/app_platform.go b/internal/drivers/app_platform.go index eb0a679..1e5320b 100644 --- a/internal/drivers/app_platform.go +++ b/internal/drivers/app_platform.go @@ -169,7 +169,13 @@ func (d *AppPlatformDriver) Diff(_ context.Context, desired interfaces.ResourceS var changes []interfaces.FieldChange if img, _ := desired.Config["image"].(string); img != "" { curImg, _ := current.Outputs["image"].(string) - if img != curImg { + // Compare structurally: ParseImageRef into godo.ImageSourceSpec on + // both sides and compare RegistryType+Repository+Tag. Falls back to + // raw string equality when either side fails to parse, so unparseable + // hand-written state still surfaces a change rather than silently + // matching. Round-3 Finding A — fixes spurious image diffs caused by + // the lossy DOCR Registry round-trip. + if !imageRefsEqual(img, curImg) { changes = append(changes, interfaces.FieldChange{Path: "image", Old: curImg, New: img}) } } @@ -444,6 +450,12 @@ func appOutput(app *godo.App) *interfaces.ResourceOutput { // otherwise "public". Stored on Outputs so Diff can detect // in-place public↔internal toggles — F4 Finding 1. "expose": deriveExposeFromAppSpec(app.Spec), + // `image` is derived from the first service's ImageSourceSpec + // and formatted as a canonical user-facing ref. Stored on + // Outputs so Diff can structurally compare against the user's + // desired `cfg["image"]` and avoid emitting spurious image + // FieldChanges on no-op reconciles — F4 round-3 Finding A. + "image": deriveImageFromAppSpec(app.Spec), }, Status: "running", } @@ -453,6 +465,90 @@ func appOutput(app *godo.App) *interfaces.ResourceOutput { return out } +// deriveImageFromAppSpec returns a canonical user-facing image ref string for +// the first service component of an AppSpec, suitable for use as +// Outputs["image"] in Diff comparisons. Returns "" when the AppSpec has no +// services or the first service lacks an Image. Round-3 Finding A. +func deriveImageFromAppSpec(spec *godo.AppSpec) string { + if spec == nil || len(spec.Services) == 0 { + return "" + } + svc := spec.Services[0] + if svc == nil { + return "" + } + return formatImageSpec(svc.Image) +} + +// formatImageSpec reverses ParseImageRef: it formats a godo.ImageSourceSpec +// back into a canonical user-facing image ref string. The format is chosen so +// that ParseImageRef(formatImageSpec(spec)) returns a struct with the same +// RegistryType, Repository, and Tag — even though Registry may be lossy for +// DOCR (DO API leaves it empty on the wire). Used by deriveImageFromAppSpec +// and as the basis for imageRefsEqual's structural compare. +func formatImageSpec(img *godo.ImageSourceSpec) string { + if img == nil || img.Repository == "" { + return "" + } + tag := img.Tag + if tag == "" { + tag = "latest" + } + switch img.RegistryType { + case godo.ImageSourceSpecRegistryType_DOCR: + // DOCR requires 3 path segments to parse (registry.digitalocean.com / + // / ). When Registry is empty (the DO API + // convention), substitute Repository as the placeholder so the round + // trip yields a parseable ref with the same Repository+Tag — DOCR + // drops the middle segment during parse, so a structural compare + // (RegistryType+Repository+Tag) still matches the user's input. + reg := img.Registry + if reg == "" { + reg = img.Repository + } + return fmt.Sprintf("registry.digitalocean.com/%s/%s:%s", reg, img.Repository, tag) + case godo.ImageSourceSpecRegistryType_Ghcr: + if img.Registry == "" { + return fmt.Sprintf("ghcr.io/%s:%s", img.Repository, tag) + } + return fmt.Sprintf("ghcr.io/%s/%s:%s", img.Registry, img.Repository, tag) + case godo.ImageSourceSpecRegistryType_DockerHub: + if img.Registry == "" { + return fmt.Sprintf("docker.io/%s:%s", img.Repository, tag) + } + return fmt.Sprintf("docker.io/%s/%s:%s", img.Registry, img.Repository, tag) + default: + // Unknown registry type — emit a best-effort ref. ParseImageRef will + // likely fail on these, which means imageRefsEqual will fall back to + // raw string compare — still safer than emitting "". + if img.Registry == "" { + return fmt.Sprintf("%s:%s", img.Repository, tag) + } + return fmt.Sprintf("%s/%s:%s", img.Registry, img.Repository, tag) + } +} + +// imageRefsEqual compares two image-ref strings structurally: parses both via +// ParseImageRef and compares RegistryType+Repository+Tag. Falls back to raw +// string equality when either side fails to parse. Two empty strings are +// equal; an empty paired with a non-empty is unequal. Round-3 Finding A. +func imageRefsEqual(a, b string) bool { + if a == b { + return true + } + if a == "" || b == "" { + return false + } + aSpec, aErr := ParseImageRef(a) + bSpec, bErr := ParseImageRef(b) + if aErr != nil || bErr != nil { + return false + } + return aSpec.RegistryType == bSpec.RegistryType && + aSpec.Repository == bSpec.Repository && + aSpec.Tag == bSpec.Tag +} + // deriveExposeFromAppSpec inspects the first service component of an AppSpec // to determine whether the deployed app is public or internal. App Platform // allows multiple services per app; the canonical `expose` key applies to the diff --git a/internal/drivers/app_platform_buildspec.go b/internal/drivers/app_platform_buildspec.go index 7e9a0d3..64fe965 100644 --- a/internal/drivers/app_platform_buildspec.go +++ b/internal/drivers/app_platform_buildspec.go @@ -162,14 +162,24 @@ func internalPortsFromConfig(cfg map[string]any) []int64 { // exposeFromConfig returns the canonical exposure mode for a container service: // "public" (default — service has an edge HTTP port and may carry routes) or // "internal" (no edge port; reachable only from sibling services via App -// Platform's internal DNS at `.internal:`). An empty string means -// the field was omitted; treat as "public". -func exposeFromConfig(cfg map[string]any) string { - v, ok := cfg["expose"].(string) +// Platform's internal DNS at `.internal:`). +// +// Returns ("", nil) when the `expose` key is absent (treat as "public" by the +// caller). Returns ("", error) when `expose` is present but not a string — +// e.g. an accidental YAML bool `true`, a number, or a structured value. +// Distinguishing key-absent from key-present-but-wrong-type is required so +// non-string values fail loudly rather than silently defaulting to public — +// quality-review F4 round-3 Finding B (security-relevant). +func exposeFromConfig(cfg map[string]any) (string, error) { + v, exists := cfg["expose"] + if !exists { + return "", nil + } + s, ok := v.(string) if !ok { - return "" + return "", fmt.Errorf("expose: must be a string (one of [public, internal]), got %T", v) } - return strings.ToLower(strings.TrimSpace(v)) + return strings.ToLower(strings.TrimSpace(s)), nil } // applyExposeInternal validates the canonical `expose` value and, when set to @@ -192,7 +202,10 @@ func exposeFromConfig(cfg map[string]any) string { // `expose: internal` with disjoint values, return an explicit error so the // caller picks one. (Original F4 behaviour.) func applyExposeInternal(svc *godo.AppServiceSpec, cfg map[string]any, httpPort int, httpPortSet bool) error { - exp := exposeFromConfig(cfg) + exp, err := exposeFromConfig(cfg) + if err != nil { + return err + } switch exp { case "", "public": return nil diff --git a/internal/drivers/app_platform_buildspec_test.go b/internal/drivers/app_platform_buildspec_test.go index aee44bb..b53ef13 100644 --- a/internal/drivers/app_platform_buildspec_test.go +++ b/internal/drivers/app_platform_buildspec_test.go @@ -317,6 +317,48 @@ func TestBuildAppSpec_Expose_Internal_NoPorts_Fails(t *testing.T) { } } +func TestBuildAppSpec_Expose_NonStringValue_Fails(t *testing.T) { + // Round-3 Finding B (security-relevant): if `expose` is not a string — + // e.g. accidental YAML bool `true` or a number — the type-assertion + // should reject it loudly rather than silently treat it as absent (which + // would default to public). Sibling to Finding 3 (typo'd string values). + for _, tc := range []struct { + name string + val any + }{ + {"bool_true", true}, + {"bool_false", false}, + {"int", 1}, + {"map", map[string]any{"public": true}}, + {"slice", []any{"public"}}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 8080, + "expose": tc.val, + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatalf("expected error for non-string expose value (%T), got nil", tc.val) + } + if !strings.Contains(err.Error(), "expose:") { + t.Errorf("error %q should mention the `expose:` config key", err.Error()) + } + // Error should hint that the value must be a string and what was + // actually received, so the user can locate the misformatted YAML. + if !strings.Contains(err.Error(), "must be a string") { + t.Errorf("error %q should explain `must be a string`", err.Error()) + } + }) + } +} + func TestBuildAppSpec_Expose_Invalid_Fails(t *testing.T) { // Typo'd values like "intenral" or "private" must not silently fall // through to public. Reject with an enum-listing error; quality-review diff --git a/internal/drivers/app_platform_test.go b/internal/drivers/app_platform_test.go index 1763512..df11834 100644 --- a/internal/drivers/app_platform_test.go +++ b/internal/drivers/app_platform_test.go @@ -419,6 +419,157 @@ func TestAppPlatformDriver_Diff_DetectsExposeChange_InternalToPublic(t *testing. } } +// TestAppPlatformDriver_AppOutput_ImageDerivedFromAppSpec covers code-reviewer +// round-3 Finding A: `Diff` reads `current.Outputs["image"]` but pre-round-3 +// `appOutput` never populated it, so every reconcile of an unchanged service +// emitted a spurious `image` FieldChange. Now `appOutput` derives the image +// string from the live AppSpec so a no-op reconcile produces no Plan action. +// +// Round-trip rule: the derived string must parse back via ParseImageRef to a +// godo.ImageSourceSpec that is structurally equal (RegistryType + Repository + +// Tag) to the user's original `cfg["image"]`. DOCR's Registry field is +// dropped during parse, so for DOCR with empty Registry we substitute the +// Repository as the registry-path placeholder — Diff's structural compare +// (see Diff_NoSpurious_ImageChange below) still finds the round-trip +// equivalent. +func TestAppPlatformDriver_AppOutput_ImageDerivedFromAppSpec(t *testing.T) { + for _, tc := range []struct { + name string + image *godo.ImageSourceSpec + wantNonEmpty bool + wantContains []string // substrings every well-formed output must include + }{ + { + name: "docr_with_empty_registry", + image: &godo.ImageSourceSpec{ + RegistryType: godo.ImageSourceSpecRegistryType_DOCR, + Registry: "", + Repository: "myapp", + Tag: "v1", + }, + wantNonEmpty: true, + wantContains: []string{"registry.digitalocean.com/", "myapp", "v1"}, + }, + { + name: "docr_with_registry", + image: &godo.ImageSourceSpec{ + RegistryType: godo.ImageSourceSpecRegistryType_DOCR, + Registry: "myrepo", + Repository: "myapp", + Tag: "v2", + }, + wantNonEmpty: true, + wantContains: []string{"registry.digitalocean.com/myrepo/myapp:v2"}, + }, + { + name: "ghcr", + image: &godo.ImageSourceSpec{ + RegistryType: godo.ImageSourceSpecRegistryType_Ghcr, + Registry: "myorg", + Repository: "myapp", + Tag: "latest", + }, + wantNonEmpty: true, + wantContains: []string{"ghcr.io/myorg/myapp:latest"}, + }, + { + name: "docker_hub", + image: &godo.ImageSourceSpec{ + RegistryType: godo.ImageSourceSpecRegistryType_DockerHub, + Registry: "library", + Repository: "nginx", + Tag: "alpine", + }, + wantNonEmpty: true, + wantContains: []string{"docker.io/library/nginx:alpine"}, + }, + { + name: "nil_image", + image: nil, + wantNonEmpty: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + app := &godo.App{ + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + Spec: &godo.AppSpec{Name: "img-app", Services: []*godo.AppServiceSpec{{Name: "svc", Image: tc.image, HTTPPort: 8080}}}, + ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, + } + mock := &mockAppClient{app: app} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + out, err := d.Read(context.Background(), interfaces.ResourceRef{ProviderID: app.ID, Name: app.Spec.Name}) + if err != nil { + t.Fatalf("Read: %v", err) + } + got, _ := out.Outputs["image"].(string) + if !tc.wantNonEmpty { + if got != "" { + t.Errorf("Outputs[\"image\"] = %q, want empty for nil/missing image", got) + } + return + } + if got == "" { + t.Fatal("Outputs[\"image\"] empty; want a populated canonical ref") + } + for _, want := range tc.wantContains { + if !strings.Contains(got, want) { + t.Errorf("Outputs[\"image\"] = %q, want it to contain %q", got, want) + } + } + }) + } +} + +// TestAppPlatformDriver_Diff_NoSpurious_ImageChange covers the practical +// outcome of Finding A: a Read whose Outputs["image"] was derived from the +// AppSpec via appOutput must NOT trigger a spurious FieldChange when the +// user's desired cfg["image"] resolves to the same Repository+Tag. This is +// the production failure mode Copilot flagged (every reconcile emitted a +// noisy image change forever). +func TestAppPlatformDriver_Diff_NoSpurious_ImageChange_DOCR(t *testing.T) { + const userImage = "registry.digitalocean.com/myrepo/myapp:v1" + app := &godo.App{ + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + Spec: &godo.AppSpec{ + Name: "round-trip-app", + Services: []*godo.AppServiceSpec{{ + Name: "svc", + // godo.ImageSourceSpec mirrors what ParseImageRef(userImage) + // produces — DOCR drops Registry on parse. + Image: &godo.ImageSourceSpec{RegistryType: godo.ImageSourceSpecRegistryType_DOCR, Registry: "", Repository: "myapp", Tag: "v1"}, + HTTPPort: 8080, + }}, + }, + ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, + } + mock := &mockAppClient{app: app} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + out, err := d.Read(context.Background(), interfaces.ResourceRef{ProviderID: app.ID, Name: app.Spec.Name}) + if err != nil { + t.Fatalf("Read: %v", err) + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"image": userImage, "http_port": 8080}, + }, out) + if err != nil { + t.Fatalf("Diff: %v", err) + } + for _, c := range result.Changes { + if c.Path == "image" { + t.Errorf("spurious image FieldChange emitted on no-op reconcile: %+v (Outputs[image]=%q vs cfg[image]=%q)", + c, out.Outputs["image"], userImage) + } + } + if result.NeedsUpdate { + // Allow other fields to drive an update only if they materially differ; + // this assertion is a guard against false-positive image changes only. + // If a future change adds another silent diff, this t.Error will pin it. + for _, c := range result.Changes { + t.Errorf("unexpected FieldChange on no-op reconcile: %+v", c) + } + } +} + // TestAppPlatformDriver_AppOutput_ExposeDerivedFromAppSpec verifies that the // live-state derivation from godo.AppSpec is correct: HTTPPort==0 with // InternalPorts populated → "internal"; everything else → "public". Without From d34efe01d91eb46e93ca5d82e1e4846abd11b152 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 23:46:56 -0400 Subject: [PATCH 4/5] fix(do-plugin): imageRefsEqual compares Registry + gofmt (F4 round 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot round-3 finding on PR #35 head bebe7359. Real finding: imageRefsEqual ignored the Registry field in its structural comparison, so GHCR/DockerHub registry-org changes were silently missed (e.g. `ghcr.io/orgA/app:tag` → `ghcr.io/orgB/app:tag` produced no Plan action). Fix adds `aSpec.Registry == bSpec.Registry` to the comparison. DOCR remains safe because ParseImageRef discards the middle path segment for DOCR refs, so both sides yield Registry="" regardless of the formatImageSpec placeholder used in the round-trip — the existing DOCR no-op behaviour is regression-pinned by a sub-case. Tests: TestAppPlatformDriver_Diff_DetectsRegistryChange covers 4 cases: GHCR org change, DockerHub registry change (both expect NeedsUpdate=true with FieldChange path "image"), DOCR placeholder no-op, GHCR unchanged no-op. RED on round-3 head bebe7359; GREEN after the Registry comparison addition; regression-invariant verified by stash/restore of app_platform.go. Polish: gofmt -w on app_platform_test.go (struct-field alignment normalized on the round-3 anonymous struct definitions). Co-authored-by: Claude --- internal/drivers/app_platform.go | 14 +++- internal/drivers/app_platform_test.go | 94 +++++++++++++++++++++++++-- 2 files changed, 99 insertions(+), 9 deletions(-) diff --git a/internal/drivers/app_platform.go b/internal/drivers/app_platform.go index 1e5320b..99a1282 100644 --- a/internal/drivers/app_platform.go +++ b/internal/drivers/app_platform.go @@ -529,9 +529,16 @@ func formatImageSpec(img *godo.ImageSourceSpec) string { } // imageRefsEqual compares two image-ref strings structurally: parses both via -// ParseImageRef and compares RegistryType+Repository+Tag. Falls back to raw -// string equality when either side fails to parse. Two empty strings are -// equal; an empty paired with a non-empty is unequal. Round-3 Finding A. +// ParseImageRef and compares RegistryType+Registry+Repository+Tag. Falls back +// to raw string equality when either side fails to parse. Two empty strings +// are equal; an empty paired with a non-empty is unequal. +// +// Registry is compared in the structural form, not the raw string. This +// matters for GHCR/DockerHub: `ghcr.io/orgA/app:v1` vs `ghcr.io/orgB/app:v1` +// is a real change Plan must surface (round-4 finding). For DOCR the +// comparison is still safe — ParseImageRef discards the middle segment for +// DOCR, so both sides yield Registry="" regardless of the +// formatImageSpec placeholder used during the round-trip. func imageRefsEqual(a, b string) bool { if a == b { return true @@ -545,6 +552,7 @@ func imageRefsEqual(a, b string) bool { return false } return aSpec.RegistryType == bSpec.RegistryType && + aSpec.Registry == bSpec.Registry && aSpec.Repository == bSpec.Repository && aSpec.Tag == bSpec.Tag } diff --git a/internal/drivers/app_platform_test.go b/internal/drivers/app_platform_test.go index df11834..0cd1b0f 100644 --- a/internal/drivers/app_platform_test.go +++ b/internal/drivers/app_platform_test.go @@ -434,8 +434,8 @@ func TestAppPlatformDriver_Diff_DetectsExposeChange_InternalToPublic(t *testing. // equivalent. func TestAppPlatformDriver_AppOutput_ImageDerivedFromAppSpec(t *testing.T) { for _, tc := range []struct { - name string - image *godo.ImageSourceSpec + name string + image *godo.ImageSourceSpec wantNonEmpty bool wantContains []string // substrings every well-formed output must include }{ @@ -520,6 +520,88 @@ func TestAppPlatformDriver_AppOutput_ImageDerivedFromAppSpec(t *testing.T) { } } +// TestAppPlatformDriver_Diff_DetectsRegistryChange covers F4 round-4 finding: +// imageRefsEqual must compare the Registry field too, otherwise GHCR/DockerHub +// org-changes (same repo+tag, different registry-org) silently slip past Plan. +// DOCR is regression-pinned to confirm the round-trip placeholder doesn't +// trigger a spurious change. +func TestAppPlatformDriver_Diff_DetectsRegistryChange(t *testing.T) { + for _, tc := range []struct { + name string + current *godo.ImageSourceSpec + desired string + wantDiff bool + }{ + { + // GHCR registry-org migration: orgA → orgB, same repo+tag. + // Real lifecycle event (e.g. ownership transfer, namespace rename). + name: "ghcr_org_change_detected", + current: &godo.ImageSourceSpec{RegistryType: godo.ImageSourceSpecRegistryType_Ghcr, Registry: "orgA", Repository: "app", Tag: "v1"}, + desired: "ghcr.io/orgB/app:v1", + wantDiff: true, + }, + { + // DockerHub registry change: library → myorg, same repo+tag. + // Real change: switching from official image to fork. + name: "dockerhub_registry_change_detected", + current: &godo.ImageSourceSpec{RegistryType: godo.ImageSourceSpecRegistryType_DockerHub, Registry: "library", Repository: "redis", Tag: "7"}, + desired: "docker.io/myorg/redis:7", + wantDiff: true, + }, + { + // DOCR no-op: live spec has Registry="" (DO API convention) and + // the desired ref's middle segment ("myrepo") is dropped on + // parse. Both sides should structurally compare equal — the + // regression-pin for round-3's DOCR fix. + name: "docr_placeholder_no_spurious_change", + current: &godo.ImageSourceSpec{RegistryType: godo.ImageSourceSpecRegistryType_DOCR, Registry: "", Repository: "myapp", Tag: "v1"}, + desired: "registry.digitalocean.com/myrepo/myapp:v1", + wantDiff: false, + }, + { + // GHCR no-op: same registry, repo, tag → no change. + name: "ghcr_unchanged_no_spurious_change", + current: &godo.ImageSourceSpec{RegistryType: godo.ImageSourceSpecRegistryType_Ghcr, Registry: "myorg", Repository: "app", Tag: "v1"}, + desired: "ghcr.io/myorg/app:v1", + wantDiff: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + app := &godo.App{ + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + Spec: &godo.AppSpec{ + Name: "registry-app", + Services: []*godo.AppServiceSpec{{Name: "svc", Image: tc.current, HTTPPort: 8080}}, + }, + ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, + } + mock := &mockAppClient{app: app} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + out, err := d.Read(context.Background(), interfaces.ResourceRef{ProviderID: app.ID, Name: app.Spec.Name}) + if err != nil { + t.Fatalf("Read: %v", err) + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"image": tc.desired, "http_port": 8080}, + }, out) + if err != nil { + t.Fatalf("Diff: %v", err) + } + gotImageChange := false + for _, c := range result.Changes { + if c.Path == "image" { + gotImageChange = true + break + } + } + if gotImageChange != tc.wantDiff { + t.Errorf("image FieldChange present = %v, want %v\n current.Outputs[image] = %q\n desired.cfg[image] = %q\n changes = %+v", + gotImageChange, tc.wantDiff, out.Outputs["image"], tc.desired, result.Changes) + } + }) + } +} + // TestAppPlatformDriver_Diff_NoSpurious_ImageChange covers the practical // outcome of Finding A: a Read whose Outputs["image"] was derived from the // AppSpec via appOutput must NOT trigger a spurious FieldChange when the @@ -578,13 +660,13 @@ func TestAppPlatformDriver_Diff_NoSpurious_ImageChange_DOCR(t *testing.T) { // would silently no-op on the round-trip. func TestAppPlatformDriver_AppOutput_ExposeDerivedFromAppSpec(t *testing.T) { internalApp := &godo.App{ - ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", - Spec: &godo.AppSpec{Name: "internal-app", Services: []*godo.AppServiceSpec{{Name: "svc", HTTPPort: 0, InternalPorts: []int64{4222}}}}, + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + Spec: &godo.AppSpec{Name: "internal-app", Services: []*godo.AppServiceSpec{{Name: "svc", HTTPPort: 0, InternalPorts: []int64{4222}}}}, ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, } publicApp := &godo.App{ - ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb6", - Spec: &godo.AppSpec{Name: "public-app", Services: []*godo.AppServiceSpec{{Name: "svc", HTTPPort: 8080}}}, + ID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb6", + Spec: &godo.AppSpec{Name: "public-app", Services: []*godo.AppServiceSpec{{Name: "svc", HTTPPort: 8080}}}, ActiveDeployment: &godo.Deployment{Phase: godo.DeploymentPhase_Active}, } From a5514e9f9df71956aa9842a2603f2da3eec8f537 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 28 Apr 2026 00:02:21 -0400 Subject: [PATCH 5/5] fix(do-plugin): TCP port range validation + apply-time wording (F4 round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot round-4 finding + 2 doc cleanups on PR #35 head d34efe0. Real bug: applyExposeInternal previously checked only whether the http_port KEY was set, not whether the value was a valid TCP port. `expose: internal` + `http_port: 0` passed validation and appended 0 to InternalPorts → unreachable service spec. Same gap existed for internal_ports entries (any out-of-range port flowed through unchecked). Fix: new validTCPPort(p) helper (p ∈ [1, 65535]). applyExposeInternal now rejects out-of-range http_port with `http_port: %d invalid (must be between 1 and 65535)` and out-of-range internal_ports with the parallel `internal_ports: %d invalid (must be between 1 and 65535)`. Both errors short-circuit at apply time, before any DO API call. Tests: - TestBuildAppSpec_Expose_Internal_HTTPPort_Zero_Fails (the team-lead-named case, asserting the exact "http_port: 0 invalid" substring). - TestBuildAppSpec_Expose_Internal_HTTPPort_OutOfRange_Fails (4 sub-cases: negative, zero, above_max_tcp 65536, way_above_max 999999). - TestBuildAppSpec_Expose_Internal_InternalPorts_OutOfRange_Fails (4 sub-cases: contains_zero, contains_negative, contains_above_max, valid_then_invalid — covers the case where one of several ports is bad). All 9 sub-cases RED first; GREEN after impl; regression-invariant verified by stash/restore of app_platform_buildspec.go. Doc cleanups: - applyExposeInternal docstring: "fail at apply time, before any DO API call" replaces the inaccurate "plan time" wording (the validation runs inside Create/Update, not Plan). - CHANGELOG `[Unreleased]` guards header: same plan→apply correction; new guard documented alongside existing four. Same correction F7 round 3 made to its CHANGELOG. Co-authored-by: Claude --- CHANGELOG.md | 9 +- internal/drivers/app_platform_buildspec.go | 33 ++++++- .../drivers/app_platform_buildspec_test.go | 97 +++++++++++++++++++ 3 files changed, 135 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4a09bd..e0e8454 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,8 @@ All notable changes to workflow-plugin-digitalocean are documented here. DO App Platform's internal DNS (`.internal:`). Default remains `expose: public`. - Misconfiguration guards (all reject at plan time before any DO API call): + Misconfiguration guards (all reject at apply time, before any DO API + call): - `expose` must be a string. Non-string values (accidental YAML bool `true`, numbers, maps, etc.) return `expose: must be a string (one of [public, internal]), got ` rather than silently defaulting to @@ -27,6 +28,12 @@ All notable changes to workflow-plugin-digitalocean are documented here. would produce a service with no listening port — silently unreachable. Returns `expose: internal requires http_port or internal_ports to be set`. + - Under `expose: internal`, every port (`http_port` and each entry in + `internal_ports`) must be in the valid TCP range [1, 65535]. Returns + `http_port: %d invalid (must be between 1 and 65535)` or + `internal_ports: %d invalid (must be between 1 and 65535)`. This + closes the `http_port: 0` landmine where a previous "is the key set" + check would silently append 0 to InternalPorts → unreachable spec. - `http_port` and `internal_ports` set with disjoint values returns `internal_ports must include http_port when both are set; use one or the other`. diff --git a/internal/drivers/app_platform_buildspec.go b/internal/drivers/app_platform_buildspec.go index 64fe965..557c601 100644 --- a/internal/drivers/app_platform_buildspec.go +++ b/internal/drivers/app_platform_buildspec.go @@ -189,14 +189,21 @@ func exposeFromConfig(cfg map[string]any) (string, error) { // - Routes is dropped — `internal` promises no public surface, even if the // caller supplied a routes list. // +// All errors below short-circuit the build at apply time, before any DO API +// call, so misconfiguration never reaches the wire. +// // Validation: `expose` must be one of {"", "public", "internal"}. Any other // value (typo'd "intenral", unsupported "private", etc.) returns an enum -// error so misconfigurations fail at plan time rather than silently -// defaulting to public — quality-review F4 Finding 3. +// error rather than silently defaulting to public — F4 Finding 3 / round 3. // // Reachability: when `expose: internal` is set with neither http_port nor // internal_ports, the result would be a service with no listening port — -// silently unreachable. Reject this combination — quality-review F4 Finding 2. +// silently unreachable. Reject this combination — F4 Finding 2. +// +// Port range: http_port and every internal_ports entry must be in the valid +// TCP range [1, 65535]. http_port: 0 used to slip through the "is the key +// set?" check and append 0 to InternalPorts → unreachable spec — F4 +// round-5 finding. // // Port-mismatch: when both http_port and internal_ports are set under // `expose: internal` with disjoint values, return an explicit error so the @@ -220,6 +227,19 @@ func applyExposeInternal(svc *godo.AppServiceSpec, cfg map[string]any, httpPort return fmt.Errorf("expose: internal requires http_port or internal_ports to be set") } + // Reject out-of-range http_port (including 0, the previous silent-default + // landmine that produced unreachable services). + if httpPortSet && !validTCPPort(httpPort) { + return fmt.Errorf("http_port: %d invalid (must be between 1 and 65535)", httpPort) + } + + // Reject any out-of-range internal_ports entry. + for _, p := range svc.InternalPorts { + if !validTCPPort(int(p)) { + return fmt.Errorf("internal_ports: %d invalid (must be between 1 and 65535)", p) + } + } + // Detect mismatched http_port vs internal_ports before mutating anything. if httpPortSet { hp := int64(httpPort) @@ -242,6 +262,13 @@ func applyExposeInternal(svc *godo.AppServiceSpec, cfg map[string]any, httpPort return nil } +// validTCPPort returns true when p is in the valid TCP port range [1, 65535]. +// Used by applyExposeInternal to reject http_port: 0 and any out-of-range +// internal_ports entry — F4 round-5 finding. +func validTCPPort(p int) bool { + return p >= 1 && p <= 65535 +} + // routesFromConfig converts the canonical "routes" list to []*godo.AppRouteSpec. func routesFromConfig(cfg map[string]any) []*godo.AppRouteSpec { raw, ok := cfg["routes"].([]any) diff --git a/internal/drivers/app_platform_buildspec_test.go b/internal/drivers/app_platform_buildspec_test.go index b53ef13..7eb1366 100644 --- a/internal/drivers/app_platform_buildspec_test.go +++ b/internal/drivers/app_platform_buildspec_test.go @@ -317,6 +317,103 @@ func TestBuildAppSpec_Expose_Internal_NoPorts_Fails(t *testing.T) { } } +// Round-5 finding: applyExposeInternal previously checked only whether +// http_port was SET (not its value), so `expose: internal` + `http_port: 0` +// silently appended 0 to InternalPorts → unreachable service spec. Same gap +// existed for internal_ports entries (any out-of-range port). Both now reject +// at apply time, before any DO API call. Tests cover the team-lead-named case +// and the parallel internal_ports case. + +func TestBuildAppSpec_Expose_Internal_HTTPPort_Zero_Fails(t *testing.T) { + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": 0, + "expose": "internal", + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatal("expected error for http_port: 0 under expose: internal, got nil") + } + if !strings.Contains(err.Error(), "http_port: 0 invalid") { + t.Errorf("error %q should contain 'http_port: 0 invalid'", err.Error()) + } +} + +func TestBuildAppSpec_Expose_Internal_HTTPPort_OutOfRange_Fails(t *testing.T) { + for _, tc := range []struct { + name string + port int + }{ + {"negative", -1}, + {"zero", 0}, + {"above_max_tcp", 65536}, + {"way_above_max", 999999}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "http_port": tc.port, + "expose": "internal", + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatalf("expected error for http_port: %d under expose: internal, got nil", tc.port) + } + if !strings.Contains(err.Error(), "http_port:") || !strings.Contains(err.Error(), "invalid") { + t.Errorf("error %q should mention `http_port:` and `invalid`", err.Error()) + } + if !strings.Contains(err.Error(), "1 and 65535") { + t.Errorf("error %q should hint at the valid range 1..65535", err.Error()) + } + }) + } +} + +func TestBuildAppSpec_Expose_Internal_InternalPorts_OutOfRange_Fails(t *testing.T) { + for _, tc := range []struct { + name string + ports []any + }{ + {"contains_zero", []any{0}}, + {"contains_negative", []any{-1}}, + {"contains_above_max", []any{65536}}, + {"valid_then_invalid", []any{4222, 0}}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "expose": "internal", + "internal_ports": tc.ports, + } + mock := &mockAppClient{app: testApp()} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + _, err := d.Create(t.Context(), interfaces.ResourceSpec{ + Name: "test-app", + Config: cfg, + }) + if err == nil { + t.Fatalf("expected error for internal_ports: %v under expose: internal, got nil", tc.ports) + } + if !strings.Contains(err.Error(), "internal_ports") { + t.Errorf("error %q should mention `internal_ports`", err.Error()) + } + if !strings.Contains(err.Error(), "1 and 65535") { + t.Errorf("error %q should hint at the valid range 1..65535", err.Error()) + } + }) + } +} + func TestBuildAppSpec_Expose_NonStringValue_Fails(t *testing.T) { // Round-3 Finding B (security-relevant): if `expose` is not a string — // e.g. accidental YAML bool `true` or a number — the type-assertion