From b901a7f812ec6a06baacd01542ad38ae63333cc2 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 22:46:47 -0400 Subject: [PATCH 1/5] =?UTF-8?q?fix(do-plugin):=20firewall=20must=20declare?= =?UTF-8?q?=20targets=20=E2=80=94=20fail=20plan=20when=20none=20(F7)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plumb canonical droplet_ids and tags into godo.FirewallRequest and reject firewall specs declaring neither at plan time. The error string mirrors plan P-2.F7 step 3 verbatim, including the App Platform clause that explains why DO firewalls do not protect App Platform services. Files: - internal/drivers/firewall.go: dropletIDsFromConfig, tagsFromConfig, validateFirewallTargets; Create/Update call validate before any API call. Doc-comment on FirewallDriver explains the App Platform exception. - internal/drivers/firewall_test.go: 7 new tests (pass-through, both, Update pass-through, Create empty, Update empty, mixed numeric). Existing tests gain droplet_ids so they continue to exercise their intended paths. - internal/drivers/firewall_stateheal_test.go: state-heal fixtures gain droplet_ids so they exercise UUID/heal paths, not the new validation. - plugin.json: canonicalSchema for infra.firewall documents droplet_ids, tags, and includes a tag-based example. - CHANGELOG.md: Unreleased entry covers Added (droplet_ids/tags) and Changed (no-targets is now a hard error). Co-authored-by: Claude --- CHANGELOG.md | 22 ++ internal/drivers/firewall.go | 79 ++++++- internal/drivers/firewall_stateheal_test.go | 14 +- internal/drivers/firewall_test.go | 239 +++++++++++++++++++- plugin.json | 41 +++- 5 files changed, 378 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d93d0b..74176a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,28 @@ All notable changes to workflow-plugin-digitalocean are documented here. +## [Unreleased] + +### Added + +- **Firewall `droplet_ids` + `tags` target keys (P-2.F7)** — `infra.firewall` + specs now plumb the canonical `droplet_ids` (list of Droplet IDs) and + `tags` (list of Droplet/DOKS-pool tag strings) keys into + `godo.FirewallRequest.DropletIDs` / `Tags`. Tag-based attachment lets + future Droplets and DOKS pools auto-join the firewall when they receive + the matching tag. + +### Changed + +- **Firewall specs without targets now fail at plan time (P-2.F7)** — + `FirewallDriver.Create` and `FirewallDriver.Update` now reject specs that + declare neither `droplet_ids` nor `tags` with the error: + `firewall %q has no targets (specify droplet_ids or tags) — App Platform + services cannot be firewall-protected; use expose: internal or + trusted_sources`. DO firewalls do **not** attach to App Platform apps; + for App-Platform-only deployments, omit `infra.firewall` and use + `expose: internal` services plus `trusted_sources` on managed databases. + ## [v0.7.9] - 2026-04-24 ### Added diff --git a/internal/drivers/firewall.go b/internal/drivers/firewall.go index cd30848..ce91ecf 100644 --- a/internal/drivers/firewall.go +++ b/internal/drivers/firewall.go @@ -19,6 +19,16 @@ type FirewallClient interface { } // FirewallDriver manages DigitalOcean Firewalls (infra.firewall). +// +// Targets are required: every firewall must declare at least one of +// `droplet_ids` (a list of Droplet integer IDs) or `tags` (a list of +// Droplet/DOKS-pool tag strings, which auto-attach future resources). Both +// Create and Update reject specs with neither field set. +// +// Note: DO firewalls do NOT attach to App Platform apps. For +// App-Platform-only deployments, omit `infra.firewall` and instead use +// `expose: internal` on the service plus `trusted_sources` on managed +// databases. type FirewallDriver struct { client FirewallClient } @@ -35,6 +45,9 @@ func NewFirewallDriverWithClient(c FirewallClient) *FirewallDriver { func (d *FirewallDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { req := firewallRequest(spec) + if err := validateFirewallTargets(spec.Name, req); err != nil { + return nil, err + } fw, _, err := d.client.Create(ctx, req) if err != nil { return nil, fmt.Errorf("firewall create %q: %w", spec.Name, WrapGodoError(err)) @@ -103,11 +116,14 @@ func (d *FirewallDriver) resolveProviderID(ctx context.Context, ref interfaces.R } func (d *FirewallDriver) Update(ctx context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { + req := firewallRequest(spec) + if err := validateFirewallTargets(spec.Name, req); err != nil { + return nil, err + } providerID, err := d.resolveProviderID(ctx, ref) if err != nil { return nil, err } - req := firewallRequest(spec) fw, _, err := d.client.Update(ctx, providerID, req) if err != nil { return nil, fmt.Errorf("firewall update %q: %w", ref.Name, WrapGodoError(err)) @@ -157,10 +173,19 @@ func (d *FirewallDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ in // firewallRequest builds a godo FirewallRequest from a ResourceSpec. // Config keys: // +// droplet_ids []int — Droplet IDs to attach the firewall to. +// tags []string — Droplet tags (auto-attaches future Droplets / DOKS pools). // inbound_rules []map[string]any — each: {protocol, ports, sources} // outbound_rules []map[string]any — each: {protocol, ports, destinations} +// +// At least one of `droplet_ids` or `tags` must be set; this is enforced by +// validateFirewallTargets, which Create and Update both call. func firewallRequest(spec interfaces.ResourceSpec) *godo.FirewallRequest { - req := &godo.FirewallRequest{Name: spec.Name} + req := &godo.FirewallRequest{ + Name: spec.Name, + DropletIDs: dropletIDsFromConfig(spec.Config), + Tags: tagsFromConfig(spec.Config), + } if rules, ok := spec.Config["inbound_rules"].([]any); ok { for _, r := range rules { @@ -224,3 +249,53 @@ func fwOutput(fw *godo.Firewall) *interfaces.ResourceOutput { func (d *FirewallDriver) SensitiveKeys() []string { return nil } func (d *FirewallDriver) ProviderIDFormat() interfaces.ProviderIDFormat { return interfaces.IDFormatUUID } + +// dropletIDsFromConfig extracts the canonical "droplet_ids" list. Accepts the +// numeric variants the modular YAML loader can emit (int, int64, float64). +// Non-numeric entries are silently dropped, matching how other helpers in +// this package degrade malformed input. +func dropletIDsFromConfig(cfg map[string]any) []int { + raw, ok := cfg["droplet_ids"].([]any) + if !ok || len(raw) == 0 { + return nil + } + out := make([]int, 0, len(raw)) + for _, v := range raw { + switch t := v.(type) { + case int: + out = append(out, t) + case int64: + out = append(out, int(t)) + case float64: + out = append(out, int(t)) + } + } + return out +} + +// tagsFromConfig extracts the canonical "tags" list of Droplet/DOKS-pool tag +// strings. Non-string entries are dropped. +func tagsFromConfig(cfg map[string]any) []string { + raw, ok := cfg["tags"].([]any) + if !ok || len(raw) == 0 { + return nil + } + out := make([]string, 0, len(raw)) + for _, v := range raw { + if s, ok := v.(string); ok { + out = append(out, s) + } + } + return out +} + +// validateFirewallTargets returns the spec-mandated error when the firewall +// request has no DropletIDs and no Tags. The error string is verbatim from +// plan P-2.F7 step 3 — including the em dash and the App Platform clause — +// because operators search for it and reviewers grep for it. +func validateFirewallTargets(name string, req *godo.FirewallRequest) error { + if len(req.DropletIDs) == 0 && len(req.Tags) == 0 { + return fmt.Errorf("firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or trusted_sources", name) + } + return nil +} diff --git a/internal/drivers/firewall_stateheal_test.go b/internal/drivers/firewall_stateheal_test.go index 1121ad4..c469594 100644 --- a/internal/drivers/firewall_stateheal_test.go +++ b/internal/drivers/firewall_stateheal_test.go @@ -61,8 +61,10 @@ func TestFirewallDriver_Create_PersistsUUIDInState(t *testing.T) { } d := NewFirewallDriverWithClient(m) out, err := d.Create(context.Background(), interfaces.ResourceSpec{ - Name: "my-fw", - Config: map[string]any{}, + Name: "my-fw", + // droplet_ids satisfies the targets-required validation; this test + // exercises UUID-persistence, not the targets path. + Config: map[string]any{"droplet_ids": []any{123}}, }) if err != nil { t.Fatalf("Create: %v", err) @@ -85,7 +87,7 @@ func TestFirewallDriver_Update_UsesExistingUUID(t *testing.T) { d := NewFirewallDriverWithClient(m) _, err := d.Update(context.Background(), interfaces.ResourceRef{Name: "my-fw", ProviderID: uuid}, - interfaces.ResourceSpec{Name: "my-fw", Config: map[string]any{}}, + interfaces.ResourceSpec{Name: "my-fw", Config: map[string]any{"droplet_ids": []any{123}}}, ) if err != nil { t.Fatalf("Update: %v", err) @@ -107,7 +109,7 @@ func TestFirewallDriver_Update_HealsStaleName(t *testing.T) { d := NewFirewallDriverWithClient(m) out, err := d.Update(context.Background(), interfaces.ResourceRef{Name: "my-fw", ProviderID: "my-fw"}, // stale name - interfaces.ResourceSpec{Name: "my-fw", Config: map[string]any{}}, + interfaces.ResourceSpec{Name: "my-fw", Config: map[string]any{"droplet_ids": []any{123}}}, ) if err != nil { t.Fatalf("Update with stale name: %v", err) @@ -128,7 +130,9 @@ func TestFirewallDriver_Update_HealFails_WhenListFails(t *testing.T) { d := NewFirewallDriverWithClient(m) _, err := d.Update(context.Background(), interfaces.ResourceRef{Name: "my-fw", ProviderID: "my-fw"}, - interfaces.ResourceSpec{Name: "my-fw", Config: map[string]any{}}, + // droplet_ids satisfies the targets-required validation; this test + // exercises the heal-list-failure path, not the targets path. + interfaces.ResourceSpec{Name: "my-fw", Config: map[string]any{"droplet_ids": []any{123}}}, ) if err == nil { t.Fatal("expected error when heal lookup fails, got nil") diff --git a/internal/drivers/firewall_test.go b/internal/drivers/firewall_test.go index 4d3753c..0bfb404 100644 --- a/internal/drivers/firewall_test.go +++ b/internal/drivers/firewall_test.go @@ -15,9 +15,14 @@ type mockFirewallClient struct { fw *godo.Firewall err error listErr error + // lastReq captures the most recent FirewallRequest sent to Create or + // Update, allowing tests to assert that DropletIDs / Tags pass through + // to godo. + lastReq *godo.FirewallRequest } -func (m *mockFirewallClient) Create(_ context.Context, _ *godo.FirewallRequest) (*godo.Firewall, *godo.Response, error) { +func (m *mockFirewallClient) Create(_ context.Context, req *godo.FirewallRequest) (*godo.Firewall, *godo.Response, error) { + m.lastReq = req return m.fw, nil, m.err } func (m *mockFirewallClient) Get(_ context.Context, _ string) (*godo.Firewall, *godo.Response, error) { @@ -32,7 +37,8 @@ func (m *mockFirewallClient) List(_ context.Context, _ *godo.ListOptions) ([]god } return []godo.Firewall{*m.fw}, nil, nil } -func (m *mockFirewallClient) Update(_ context.Context, _ string, _ *godo.FirewallRequest) (*godo.Firewall, *godo.Response, error) { +func (m *mockFirewallClient) Update(_ context.Context, _ string, req *godo.FirewallRequest) (*godo.Firewall, *godo.Response, error) { + m.lastReq = req return m.fw, nil, m.err } func (m *mockFirewallClient) Delete(_ context.Context, _ string) (*godo.Response, error) { @@ -54,6 +60,7 @@ func TestFirewallDriver_Create(t *testing.T) { out, err := d.Create(context.Background(), interfaces.ResourceSpec{ Name: "my-fw", Config: map[string]any{ + "droplet_ids": []any{123}, "inbound_rules": []any{ map[string]any{"protocol": "tcp", "ports": "80", "sources": []any{"0.0.0.0/0"}}, }, @@ -72,8 +79,10 @@ func TestFirewallDriver_Create_Error(t *testing.T) { d := drivers.NewFirewallDriverWithClient(mock) _, err := d.Create(context.Background(), interfaces.ResourceSpec{ - Name: "my-fw", - Config: map[string]any{}, + Name: "my-fw", + // droplet_ids satisfies the targets-required validation so the + // test exercises the API-error propagation path. + Config: map[string]any{"droplet_ids": []any{123}}, }) if err == nil { t.Fatal("expected error, got nil") @@ -103,7 +112,7 @@ func TestFirewallDriver_Update_Success(t *testing.T) { Name: "my-fw", ProviderID: "fw-123", }, interfaces.ResourceSpec{ Name: "my-fw", - Config: map[string]any{}, + Config: map[string]any{"droplet_ids": []any{123}}, }) if err != nil { t.Fatalf("Update: %v", err) @@ -121,7 +130,7 @@ func TestFirewallDriver_Update_Error(t *testing.T) { Name: "my-fw", ProviderID: "fw-123", }, interfaces.ResourceSpec{ Name: "my-fw", - Config: map[string]any{}, + Config: map[string]any{"droplet_ids": []any{123}}, }) if err == nil { t.Fatal("expected error, got nil") @@ -294,8 +303,10 @@ func TestFirewallDriver_Create_EmptyIDFromAPI(t *testing.T) { d := drivers.NewFirewallDriverWithClient(mock) _, err := d.Create(context.Background(), interfaces.ResourceSpec{ - Name: "my-fw", - Config: map[string]any{}, + Name: "my-fw", + // droplet_ids satisfies the targets-required validation so this + // test exercises the empty-ID guard path, not the targets path. + Config: map[string]any{"droplet_ids": []any{123}}, }) if err == nil { t.Fatal("expected error for empty ProviderID, got nil") @@ -309,7 +320,7 @@ func TestFirewallDriver_Create_ProviderIDIsAPIAssigned(t *testing.T) { out, err := d.Create(context.Background(), interfaces.ResourceSpec{ Name: "my-fw", - Config: map[string]any{}, + Config: map[string]any{"droplet_ids": []any{123}}, }) if err != nil { t.Fatalf("Create: %v", err) @@ -321,3 +332,213 @@ func TestFirewallDriver_Create_ProviderIDIsAPIAssigned(t *testing.T) { t.Errorf("ProviderID must not be empty") } } + +// ── F7: Firewall target enforcement ────────────────────────────────────────── + +// noTargetsErrFmt is the verbatim error string the spec requires when a +// firewall declares no targets. Character-for-character, including the em +// dash (U+2014) and the App Platform clause. Format-arg %q quotes the +// firewall name. See plan P-2.F7 step 3. +const noTargetsErrFmt = `firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or trusted_sources` + +// TestFirewallDriver_Create_DropletIDs_PassThrough verifies droplet_ids reach +// godo.FirewallRequest.DropletIDs. +func TestFirewallDriver_Create_DropletIDs_PassThrough(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{ + "droplet_ids": []any{123, 456}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if mock.lastReq == nil { + t.Fatal("Create did not capture a FirewallRequest") + } + want := []int{123, 456} + if got := mock.lastReq.DropletIDs; !equalIntSlices(got, want) { + t.Errorf("DropletIDs = %v, want %v", got, want) + } +} + +// TestFirewallDriver_Create_Tags_PassThrough verifies tags reach +// godo.FirewallRequest.Tags. +func TestFirewallDriver_Create_Tags_PassThrough(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{ + "tags": []any{"bmw-prod"}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if mock.lastReq == nil { + t.Fatal("Create did not capture a FirewallRequest") + } + want := []string{"bmw-prod"} + if got := mock.lastReq.Tags; !equalStringSlices(got, want) { + t.Errorf("Tags = %v, want %v", got, want) + } +} + +// TestFirewallDriver_Create_BothTargets verifies droplet_ids AND tags both +// flow through when set together. +func TestFirewallDriver_Create_BothTargets(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{ + "droplet_ids": []any{123}, + "tags": []any{"bmw-prod", "edge"}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if mock.lastReq == nil { + t.Fatal("Create did not capture a FirewallRequest") + } + if got, want := mock.lastReq.DropletIDs, []int{123}; !equalIntSlices(got, want) { + t.Errorf("DropletIDs = %v, want %v", got, want) + } + if got, want := mock.lastReq.Tags, []string{"bmw-prod", "edge"}; !equalStringSlices(got, want) { + t.Errorf("Tags = %v, want %v", got, want) + } +} + +// TestFirewallDriver_Update_Targets_PassThrough verifies droplet_ids and tags +// flow through Update. +func TestFirewallDriver_Update_Targets_PassThrough(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Update(context.Background(), interfaces.ResourceRef{ + Name: "my-fw", ProviderID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + }, interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{ + "droplet_ids": []any{789}, + "tags": []any{"bmw-prod"}, + }, + }) + if err != nil { + t.Fatalf("Update: %v", err) + } + if mock.lastReq == nil { + t.Fatal("Update did not capture a FirewallRequest") + } + if got, want := mock.lastReq.DropletIDs, []int{789}; !equalIntSlices(got, want) { + t.Errorf("DropletIDs = %v, want %v", got, want) + } + if got, want := mock.lastReq.Tags, []string{"bmw-prod"}; !equalStringSlices(got, want) { + t.Errorf("Tags = %v, want %v", got, want) + } +} + +// TestFirewallDriver_Create_NoTargets_Errors verifies that a firewall spec +// with neither droplet_ids nor tags fails Create with the exact spec error. +func TestFirewallDriver_Create_NoTargets_Errors(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + // inbound_rules without targets is an App Platform footgun: there + // is nothing for the rule to apply to. + Config: map[string]any{ + "inbound_rules": []any{ + map[string]any{"protocol": "tcp", "ports": "80"}, + }, + }, + }) + if err == nil { + t.Fatal("expected error for empty targets, got nil") + } + want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + if got := err.Error(); got != want { + t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) + } + // Validation must short-circuit BEFORE the API call. + if mock.lastReq != nil { + t.Error("FirewallRequest reached godo client despite empty-targets validation") + } +} + +// TestFirewallDriver_Update_NoTargets_Errors verifies the same exact-string +// validation also fires on Update. +func TestFirewallDriver_Update_NoTargets_Errors(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Update(context.Background(), interfaces.ResourceRef{ + Name: "my-fw", ProviderID: "f8b6200c-3bba-48a7-8bf1-7a3e3a885eb5", + }, interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{}, + }) + if err == nil { + t.Fatal("expected error for empty targets, got nil") + } + want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + if got := err.Error(); got != want { + t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) + } + if mock.lastReq != nil { + t.Error("FirewallRequest reached godo client despite empty-targets validation") + } +} + +// TestFirewallDriver_Create_DropletIDs_AcceptsMixedNumeric verifies the +// helper accepts the YAML-decoded numeric variants (int, int64, float64) the +// modular YAML loader can produce. +func TestFirewallDriver_Create_DropletIDs_AcceptsMixedNumeric(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{ + "droplet_ids": []any{int(1), int64(2), float64(3)}, + }, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got, want := mock.lastReq.DropletIDs, []int{1, 2, 3}; !equalIntSlices(got, want) { + t.Errorf("DropletIDs = %v, want %v", got, want) + } +} + +func equalIntSlices(a, b []int) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +func equalStringSlices(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/plugin.json b/plugin.json index 8edf27e..fe4ae3f 100644 --- a/plugin.json +++ b/plugin.json @@ -32,7 +32,46 @@ "infra.droplet", "infra.iam_role", "infra.api_gateway" - ] + ], + "canonicalSchema": { + "infra.firewall": { + "description": "DigitalOcean cloud firewall. Attaches to Droplets by ID or by tag (which auto-attaches future Droplets / DOKS pools that receive the tag). Either `droplet_ids` or `tags` is REQUIRED; `wfctl infra plan` rejects firewalls with no targets. NOTE: DO firewalls do not attach to App Platform apps — for App-Platform-only deployments, use `expose: internal` services plus `trusted_sources` on managed databases.", + "fields": { + "droplet_ids": { + "type": "array", + "required": false, + "description": "Droplet IDs the firewall attaches to. At least one of `droplet_ids` or `tags` must be set." + }, + "tags": { + "type": "array", + "required": false, + "description": "Droplet/DOKS-pool tag strings. Resources receiving any listed tag auto-join the firewall. Example: [\"bmw-prod\"]." + }, + "inbound_rules": { + "type": "array", + "required": false, + "description": "Each rule: {protocol: tcp|udp|icmp, ports: \"||all\", sources: [...]}." + }, + "outbound_rules": { + "type": "array", + "required": false, + "description": "Each rule: {protocol: tcp|udp|icmp, ports: \"||all\", destinations: [...]}." + } + }, + "examples": [ + { + "name": "tag-based-firewall", + "comment": "Tag-based attachment so future Droplets / DOKS pools auto-join.", + "spec": { + "tags": ["bmw-prod"], + "inbound_rules": [ + {"protocol": "tcp", "ports": "443", "sources": ["0.0.0.0/0"]} + ] + } + } + ] + } + } } } } From 2e2337f91c1767fbe59b1a2a5b8c9e663f4f90cd Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 22:47:12 -0400 Subject: [PATCH 2/5] chore(do-plugin): rename plugin.json schema key to configSchema Align the firewall canonical-schema key with workflow SDK terminology (see workflow@v0.19.0 schema/schema.go:637). Co-authored-by: Claude --- plugin.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin.json b/plugin.json index fe4ae3f..4714956 100644 --- a/plugin.json +++ b/plugin.json @@ -33,7 +33,7 @@ "infra.iam_role", "infra.api_gateway" ], - "canonicalSchema": { + "configSchema": { "infra.firewall": { "description": "DigitalOcean cloud firewall. Attaches to Droplets by ID or by tag (which auto-attaches future Droplets / DOKS pools that receive the tag). Either `droplet_ids` or `tags` is REQUIRED; `wfctl infra plan` rejects firewalls with no targets. NOTE: DO firewalls do not attach to App Platform apps — for App-Platform-only deployments, use `expose: internal` services plus `trusted_sources` on managed databases.", "fields": { From f6c02ab51a12ab2e88ef48c53022e668458f04da Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 23:09:09 -0400 Subject: [PATCH 3/5] fix(do-plugin): firewall Diff cascade + fail-fast filters (F7 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 #36. Finding 1 — Diff cascade (Important): in-place toggles of droplet_ids, tags, inbound_rules, or outbound_rules now produce a Plan action. fwOutput records the four canonical fields on Outputs (godo shape); Diff compares desired vs. current and emits FieldChange entries when any diverges. Set semantics for droplet_ids/tags (reorder is not a change); order-sensitive deep-equal for rules. Pre-F7 state without recorded keys is treated as empty — first plan post-upgrade safely over-detects. Finding 2 — Fail-fast: tagsFromConfig now filters empty strings, so `tags: [""]` fails the targets-required validation instead of slipping through to a runtime DO API rejection. Finding 3 — Fail-fast: dropletIDsFromConfig now filters IDs ≤ 0, by symmetry with the empty-string tag filter. Refactored the type switch to a single id var with a single id <= 0 guard. Refactor: extracted inboundRulesFromConfig / outboundRulesFromConfig helpers from firewallRequest so Diff can reuse them. No behavior change in firewallRequest. Tests: TestFirewallDriver_FwOutput_RecordsTargetsAndRules, TestFirewallDriver_Diff_DetectsTargetsChange (8 sub-cases — change detection for each field, no-change, reorder set semantics for IDs + tags, pre-F7 state migration), TestFirewallDriver_Create_*Tags*Rejected + ZeroOrNegativeDropletIDsFiltered (sub-cases for all-empty fail and mixed-slice filter). All RED first; GREEN after impl. Regression invariant verified by stash/restore of firewall.go — round-1 baseline produces the exact RED failures. Co-authored-by: Claude --- CHANGELOG.md | 19 ++ internal/drivers/firewall.go | 264 +++++++++++++++++++----- internal/drivers/firewall_test.go | 324 ++++++++++++++++++++++++++++++ 3 files changed, 558 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74176a3..34d8847 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,25 @@ All notable changes to workflow-plugin-digitalocean are documented here. for App-Platform-only deployments, omit `infra.firewall` and use `expose: internal` services plus `trusted_sources` on managed databases. +### Fixed + +- **`FirewallDriver.Diff` detects in-place target/rule changes (P-2.F7)** — + pre-F7 `Diff` was a stub that returned `NeedsUpdate=false` for every + non-nil current state, silently suppressing in-place toggles of + `droplet_ids`, `tags`, `inbound_rules`, or `outbound_rules` from `wfctl + infra plan` output. F7 round 2 extends `Diff` to compare those four + canonical fields against state recorded by `fwOutput`, surfacing a Plan + action when any diverges. Set semantics for `droplet_ids` and `tags` + (reorder is not a change); order-sensitive deep-equal for rules. Pre-F7 + state without recorded fields is treated as having empty fields, so the + first plan post-upgrade safely over-detects and re-asserts current + configuration. +- **Empty / non-positive target entries now fail at plan time (P-2.F7)** — + `tagsFromConfig` filters empty strings and `dropletIDsFromConfig` filters + IDs ≤ 0. Without these filters, a spec like `tags: [""]` or + `droplet_ids: [0]` would slip past `validateFirewallTargets` (slice is + non-empty after parsing) and fail only at the DO API call. + ## [v0.7.9] - 2026-04-24 ### Added diff --git a/internal/drivers/firewall.go b/internal/drivers/firewall.go index ce91ecf..69f6679 100644 --- a/internal/drivers/firewall.go +++ b/internal/drivers/firewall.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "log" + "reflect" + "sort" "github.com/GoCodeAlone/workflow/interfaces" "github.com/digitalocean/godo" @@ -143,11 +145,135 @@ func (d *FirewallDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) return nil } +// Diff compares the desired spec against the live firewall recorded on +// `current` to detect in-place reconfiguration. Pre-F7 the body was a stub +// that always returned NeedsUpdate=false — meaning every droplet_ids/tags +// toggle silently no-op'd at plan time. F7 round 2 extends it to compare the +// four canonical fields (`droplet_ids`, `tags`, `inbound_rules`, +// `outbound_rules`). +// +// `droplet_ids` and `tags` use SET semantics: reorder is not a change, since +// DO normalizes membership server-side. Rules use ORDER-SENSITIVE deep-equal +// because rule order is preserved in the API response and may carry user +// intent. +// +// Pre-F7 state without the recorded keys (legacy ResourceOutputs from older +// plugin versions) is treated as having empty fields, which surfaces a Plan +// action on first Diff post-upgrade — the safe over-detect direction. +// (Code-review Finding 1, F7 round 2.) func (d *FirewallDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { if current == nil { return &interfaces.DiffResult{NeedsUpdate: true}, nil } - return &interfaces.DiffResult{NeedsUpdate: false}, nil + desiredReq := firewallRequest(desired) + var changes []interfaces.FieldChange + + curIDs := outputsAsIntSlice(current.Outputs["droplet_ids"]) + if !equalIntSet(desiredReq.DropletIDs, curIDs) { + changes = append(changes, interfaces.FieldChange{ + Path: "droplet_ids", Old: curIDs, New: desiredReq.DropletIDs, + }) + } + + curTags := outputsAsStringSlice(current.Outputs["tags"]) + if !equalStringSet(desiredReq.Tags, curTags) { + changes = append(changes, interfaces.FieldChange{ + Path: "tags", Old: curTags, New: desiredReq.Tags, + }) + } + + curIn, _ := current.Outputs["inbound_rules"].([]godo.InboundRule) + if !reflect.DeepEqual(curIn, desiredReq.InboundRules) { + changes = append(changes, interfaces.FieldChange{ + Path: "inbound_rules", Old: curIn, New: desiredReq.InboundRules, + }) + } + + curOut, _ := current.Outputs["outbound_rules"].([]godo.OutboundRule) + if !reflect.DeepEqual(curOut, desiredReq.OutboundRules) { + changes = append(changes, interfaces.FieldChange{ + Path: "outbound_rules", Old: curOut, New: desiredReq.OutboundRules, + }) + } + + return &interfaces.DiffResult{NeedsUpdate: len(changes) > 0, Changes: changes}, nil +} + +// outputsAsIntSlice tolerantly coerces a stored Outputs value to []int, +// accepting both the in-memory []int that fwOutput writes and the []any of +// numerics that JSON/YAML state round-trips can produce. +func outputsAsIntSlice(v any) []int { + switch t := v.(type) { + case []int: + return append([]int(nil), t...) + case []any: + out := make([]int, 0, len(t)) + for _, x := range t { + switch n := x.(type) { + case int: + out = append(out, n) + case int64: + out = append(out, int(n)) + case float64: + out = append(out, int(n)) + } + } + return out + } + return nil +} + +// outputsAsStringSlice is the analogous coercer for []string Outputs. +func outputsAsStringSlice(v any) []string { + switch t := v.(type) { + case []string: + return append([]string(nil), t...) + case []any: + out := make([]string, 0, len(t)) + for _, x := range t { + if s, ok := x.(string); ok { + out = append(out, s) + } + } + return out + } + return nil +} + +// equalIntSet returns true iff a and b contain the same multiset of ints, +// ignoring order. DO normalizes droplet_ids server-side; reorders should +// not produce Plan actions. +func equalIntSet(a, b []int) bool { + if len(a) != len(b) { + return false + } + sa := append([]int(nil), a...) + sb := append([]int(nil), b...) + sort.Ints(sa) + sort.Ints(sb) + for i := range sa { + if sa[i] != sb[i] { + return false + } + } + return true +} + +// equalStringSet is the string analogue of equalIntSet. +func equalStringSet(a, b []string) bool { + if len(a) != len(b) { + return false + } + sa := append([]string(nil), a...) + sb := append([]string(nil), b...) + sort.Strings(sa) + sort.Strings(sb) + for i := range sa { + if sa[i] != sb[i] { + return false + } + } + return true } func (d *FirewallDriver) HealthCheck(ctx context.Context, ref interfaces.ResourceRef) (*interfaces.HealthResult, error) { @@ -181,66 +307,94 @@ func (d *FirewallDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ in // At least one of `droplet_ids` or `tags` must be set; this is enforced by // validateFirewallTargets, which Create and Update both call. func firewallRequest(spec interfaces.ResourceSpec) *godo.FirewallRequest { - req := &godo.FirewallRequest{ - Name: spec.Name, - DropletIDs: dropletIDsFromConfig(spec.Config), - Tags: tagsFromConfig(spec.Config), + return &godo.FirewallRequest{ + Name: spec.Name, + DropletIDs: dropletIDsFromConfig(spec.Config), + Tags: tagsFromConfig(spec.Config), + InboundRules: inboundRulesFromConfig(spec.Config), + OutboundRules: outboundRulesFromConfig(spec.Config), } +} - if rules, ok := spec.Config["inbound_rules"].([]any); ok { - for _, r := range rules { - m, _ := r.(map[string]any) - if m == nil { - continue - } - rule := godo.InboundRule{ - Protocol: strFromConfig(m, "protocol", "tcp"), - PortRange: strFromConfig(m, "ports", "all"), - Sources: &godo.Sources{}, - } - if srcs, ok := m["sources"].([]any); ok { - for _, s := range srcs { - if addr, ok := s.(string); ok { - rule.Sources.Addresses = append(rule.Sources.Addresses, addr) - } +// inboundRulesFromConfig extracts canonical "inbound_rules" into godo shape. +// Each rule: {protocol, ports, sources: [...]}. Defaults: tcp / all / +// no sources. The Sources struct is always allocated (matching DO API +// convention) so equality comparisons in Diff don't fight nil-vs-empty +// pointer differences. +func inboundRulesFromConfig(cfg map[string]any) []godo.InboundRule { + raw, ok := cfg["inbound_rules"].([]any) + if !ok || len(raw) == 0 { + return nil + } + out := make([]godo.InboundRule, 0, len(raw)) + for _, r := range raw { + m, _ := r.(map[string]any) + if m == nil { + continue + } + rule := godo.InboundRule{ + Protocol: strFromConfig(m, "protocol", "tcp"), + PortRange: strFromConfig(m, "ports", "all"), + Sources: &godo.Sources{}, + } + if srcs, ok := m["sources"].([]any); ok { + for _, s := range srcs { + if addr, ok := s.(string); ok { + rule.Sources.Addresses = append(rule.Sources.Addresses, addr) } } - req.InboundRules = append(req.InboundRules, rule) } + out = append(out, rule) } + return out +} - if rules, ok := spec.Config["outbound_rules"].([]any); ok { - for _, r := range rules { - m, _ := r.(map[string]any) - if m == nil { - continue - } - rule := godo.OutboundRule{ - Protocol: strFromConfig(m, "protocol", "tcp"), - PortRange: strFromConfig(m, "ports", "all"), - Destinations: &godo.Destinations{}, - } - if dsts, ok := m["destinations"].([]any); ok { - for _, s := range dsts { - if addr, ok := s.(string); ok { - rule.Destinations.Addresses = append(rule.Destinations.Addresses, addr) - } +// outboundRulesFromConfig extracts canonical "outbound_rules" into godo +// shape. Mirror of inboundRulesFromConfig but uses Destinations. +func outboundRulesFromConfig(cfg map[string]any) []godo.OutboundRule { + raw, ok := cfg["outbound_rules"].([]any) + if !ok || len(raw) == 0 { + return nil + } + out := make([]godo.OutboundRule, 0, len(raw)) + for _, r := range raw { + m, _ := r.(map[string]any) + if m == nil { + continue + } + rule := godo.OutboundRule{ + Protocol: strFromConfig(m, "protocol", "tcp"), + PortRange: strFromConfig(m, "ports", "all"), + Destinations: &godo.Destinations{}, + } + if dsts, ok := m["destinations"].([]any); ok { + for _, s := range dsts { + if addr, ok := s.(string); ok { + rule.Destinations.Addresses = append(rule.Destinations.Addresses, addr) } } - req.OutboundRules = append(req.OutboundRules, rule) } + out = append(out, rule) } - - return req + return out } +// fwOutput records the firewall's targets (droplet_ids, tags) and rules +// (inbound_rules, outbound_rules) on Outputs so Diff can detect in-place +// reconfiguration. Storing the godo-shape directly keeps the comparison +// symmetric with what `firewallRequest` builds from the desired cfg. +// (F7 round 2 — Diff cascade fix.) func fwOutput(fw *godo.Firewall) *interfaces.ResourceOutput { return &interfaces.ResourceOutput{ Name: fw.Name, Type: "infra.firewall", ProviderID: fw.ID, Outputs: map[string]any{ - "status": fw.Status, + "status": fw.Status, + "droplet_ids": append([]int(nil), fw.DropletIDs...), + "tags": append([]string(nil), fw.Tags...), + "inbound_rules": append([]godo.InboundRule(nil), fw.InboundRules...), + "outbound_rules": append([]godo.OutboundRule(nil), fw.OutboundRules...), }, Status: fw.Status, } @@ -252,8 +406,10 @@ func (d *FirewallDriver) ProviderIDFormat() interfaces.ProviderIDFormat { return // dropletIDsFromConfig extracts the canonical "droplet_ids" list. Accepts the // numeric variants the modular YAML loader can emit (int, int64, float64). -// Non-numeric entries are silently dropped, matching how other helpers in -// this package degrade malformed input. +// Non-numeric entries and non-positive IDs are silently dropped: Droplet IDs +// are positive integers assigned by the DO API, so 0 / negatives are never +// valid and would only fail at apply time, defeating F7's plan-time-fail +// contract. (Code-review Finding 3, F7 round 2.) func dropletIDsFromConfig(cfg map[string]any) []int { raw, ok := cfg["droplet_ids"].([]any) if !ok || len(raw) == 0 { @@ -261,20 +417,30 @@ func dropletIDsFromConfig(cfg map[string]any) []int { } out := make([]int, 0, len(raw)) for _, v := range raw { + var id int switch t := v.(type) { case int: - out = append(out, t) + id = t case int64: - out = append(out, int(t)) + id = int(t) case float64: - out = append(out, int(t)) + id = int(t) + default: + continue + } + if id <= 0 { + continue } + out = append(out, id) } return out } // tagsFromConfig extracts the canonical "tags" list of Droplet/DOKS-pool tag -// strings. Non-string entries are dropped. +// strings. Non-string entries and empty strings are dropped: the DO API +// rejects empty tags, so a slice that contains only empty strings must fail +// the targets-required validation rather than being silently sent to the +// API. (Code-review Finding 2, F7 round 2.) func tagsFromConfig(cfg map[string]any) []string { raw, ok := cfg["tags"].([]any) if !ok || len(raw) == 0 { @@ -282,7 +448,7 @@ func tagsFromConfig(cfg map[string]any) []string { } out := make([]string, 0, len(raw)) for _, v := range raw { - if s, ok := v.(string); ok { + if s, ok := v.(string); ok && s != "" { out = append(out, s) } } diff --git a/internal/drivers/firewall_test.go b/internal/drivers/firewall_test.go index 0bfb404..9f9b68a 100644 --- a/internal/drivers/firewall_test.go +++ b/internal/drivers/firewall_test.go @@ -519,6 +519,330 @@ func TestFirewallDriver_Create_DropletIDs_AcceptsMixedNumeric(t *testing.T) { } } +// TestFirewallDriver_Create_EmptyStringTagsRejected verifies that an +// all-empty-string tags slice fails the targets-required validation, and +// that a mixed slice is filtered to only the non-empty entries. +// +// Without filtering, `tagsFromConfig` appends "" to its output, making +// `len(req.Tags) > 0` falsely succeed; the DO API then rejects the empty +// tag at apply time — defeating F7's plan-time-fail contract. Fix is in +// `tagsFromConfig` (filter `s != ""`). (Code-review Finding 2, F7 round 2.) +func TestFirewallDriver_Create_EmptyStringTagsRejected(t *testing.T) { + t.Run("all empty strings → no targets error", func(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"tags": []any{""}}, + }) + if err == nil { + t.Fatal("expected no-targets error for tags: [\"\"]; got nil") + } + want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + if got := err.Error(); got != want { + t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) + } + if mock.lastReq != nil { + t.Error("FirewallRequest reached godo client despite all-empty tags") + } + }) + + t.Run("mixed slice → empty entries filtered, non-empty kept", func(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"tags": []any{"", "bmw-prod", ""}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got, want := mock.lastReq.Tags, []string{"bmw-prod"}; !equalStringSlices(got, want) { + t.Errorf("Tags = %v, want %v (empty entries should be filtered)", got, want) + } + }) +} + +// TestFirewallDriver_Create_ZeroOrNegativeDropletIDsFiltered verifies that +// non-positive Droplet IDs are filtered out, by symmetry with the +// empty-string tag filter (Finding 2). Droplet IDs are positive integers +// assigned by the DO API; 0 and negatives are never valid and would be +// rejected at apply time. +// +// Without filtering, `dropletIDsFromConfig` appends every numeric to its +// output, so `droplet_ids: [0]` slips past validation and the DO API rejects +// it at runtime. (Code-review Finding 3, F7 round 2.) +func TestFirewallDriver_Create_ZeroOrNegativeDropletIDsFiltered(t *testing.T) { + t.Run("only non-positives → no targets error", func(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{0, -1, int64(-2), float64(0)}}, + }) + if err == nil { + t.Fatal("expected no-targets error for non-positive droplet IDs; got nil") + } + want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + if got := err.Error(); got != want { + t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) + } + if mock.lastReq != nil { + t.Error("FirewallRequest reached godo client despite all-non-positive droplet IDs") + } + }) + + t.Run("mixed slice → non-positives filtered, positives kept", func(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{0, 123, int64(-1), float64(456)}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got, want := mock.lastReq.DropletIDs, []int{123, 456}; !equalIntSlices(got, want) { + t.Errorf("DropletIDs = %v, want %v (non-positives should be filtered)", got, want) + } + }) +} + +// ── F7 Finding 1 — Diff cascade ────────────────────────────────────────────── +// +// Pre-F7, FirewallDriver.Diff was a stub: it returned NeedsUpdate=true for nil +// current and NeedsUpdate=false otherwise, which made every in-place toggle of +// targets, tags, or rules a silent no-op at plan time. F7 makes target +// reconfiguration the most common firewall lifecycle action, so Diff must +// detect changes to droplet_ids, tags, inbound_rules, and outbound_rules. +// (Code-review Finding 1, F7 round 2.) + +// TestFirewallDriver_FwOutput_RecordsTargetsAndRules verifies that Create +// returns a ResourceOutput whose Outputs map carries the four target/rule +// fields recovered from the godo.Firewall API response. Without these, Diff +// has nothing to compare against. +func TestFirewallDriver_FwOutput_RecordsTargetsAndRules(t *testing.T) { + fw := &godo.Firewall{ + ID: "fw-uuid", + Name: "my-fw", + Status: "succeeded", + DropletIDs: []int{123, 456}, + Tags: []string{"bmw-prod"}, + InboundRules: []godo.InboundRule{ + {Protocol: "tcp", PortRange: "443", Sources: &godo.Sources{Addresses: []string{"0.0.0.0/0"}}}, + }, + OutboundRules: []godo.OutboundRule{ + {Protocol: "tcp", PortRange: "1-65535", Destinations: &godo.Destinations{Addresses: []string{"0.0.0.0/0"}}}, + }, + } + mock := &mockFirewallClient{fw: fw} + d := drivers.NewFirewallDriverWithClient(mock) + + out, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{123, 456}, "tags": []any{"bmw-prod"}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got := out.Outputs["droplet_ids"]; !equalIntSlices(toInts(got), []int{123, 456}) { + t.Errorf("Outputs[droplet_ids] = %v, want [123 456]", got) + } + if got := out.Outputs["tags"]; !equalStringSlices(toStrings(got), []string{"bmw-prod"}) { + t.Errorf("Outputs[tags] = %v, want [bmw-prod]", got) + } + if _, ok := out.Outputs["inbound_rules"]; !ok { + t.Error("Outputs[inbound_rules] missing") + } + if _, ok := out.Outputs["outbound_rules"]; !ok { + t.Error("Outputs[outbound_rules] missing") + } +} + +// TestFirewallDriver_Diff_DetectsTargetsChange parametrically verifies that +// changes to each of the four canonical firewall fields produce a Plan +// action, and that no-op cases (and reorder for set fields) don't. +func TestFirewallDriver_Diff_DetectsTargetsChange(t *testing.T) { + d := drivers.NewFirewallDriverWithClient(&mockFirewallClient{fw: testFirewall()}) + ctx := context.Background() + + // Helper: build a current ResourceOutput whose Outputs reflects a live + // firewall (the way fwOutput will populate it after F7 round 2). + makeCurrent := func(ids []int, tags []string, in []godo.InboundRule, out []godo.OutboundRule) *interfaces.ResourceOutput { + return &interfaces.ResourceOutput{ + ProviderID: "fw-uuid", + Outputs: map[string]any{ + "status": "succeeded", + "droplet_ids": ids, + "tags": tags, + "inbound_rules": in, + "outbound_rules": out, + }, + } + } + + cases := []struct { + name string + desiredCfg map[string]any + current *interfaces.ResourceOutput + wantUpdate bool + wantPath string // expected FieldChange.Path on first change; "" if no change + }{ + { + name: "droplet_ids change", + desiredCfg: map[string]any{ + "droplet_ids": []any{789}, + }, + current: makeCurrent([]int{123}, nil, nil, nil), + wantUpdate: true, + wantPath: "droplet_ids", + }, + { + name: "tags change", + desiredCfg: map[string]any{ + "tags": []any{"new-tag"}, + }, + current: makeCurrent(nil, []string{"old-tag"}, nil, nil), + wantUpdate: true, + wantPath: "tags", + }, + { + name: "inbound_rules change", + desiredCfg: map[string]any{ + "droplet_ids": []any{123}, + "inbound_rules": []any{ + map[string]any{"protocol": "tcp", "ports": "443", "sources": []any{"0.0.0.0/0"}}, + }, + }, + current: makeCurrent( + []int{123}, nil, + []godo.InboundRule{{Protocol: "tcp", PortRange: "80", Sources: &godo.Sources{Addresses: []string{"0.0.0.0/0"}}}}, + nil, + ), + wantUpdate: true, + wantPath: "inbound_rules", + }, + { + name: "outbound_rules change", + desiredCfg: map[string]any{ + "droplet_ids": []any{123}, + "outbound_rules": []any{ + map[string]any{"protocol": "tcp", "ports": "1-65535", "destinations": []any{"0.0.0.0/0"}}, + }, + }, + current: makeCurrent( + []int{123}, nil, nil, + []godo.OutboundRule{{Protocol: "tcp", PortRange: "53", Destinations: &godo.Destinations{Addresses: []string{"0.0.0.0/0"}}}}, + ), + wantUpdate: true, + wantPath: "outbound_rules", + }, + { + name: "no change — droplet_ids and tags identical", + desiredCfg: map[string]any{ + "droplet_ids": []any{123, 456}, + "tags": []any{"bmw-prod"}, + }, + current: makeCurrent([]int{123, 456}, []string{"bmw-prod"}, nil, nil), + wantUpdate: false, + }, + { + name: "droplet_ids reorder is NOT a change (set semantics)", + desiredCfg: map[string]any{ + "droplet_ids": []any{456, 123}, + }, + current: makeCurrent([]int{123, 456}, nil, nil, nil), + wantUpdate: false, + }, + { + name: "tags reorder is NOT a change (set semantics)", + desiredCfg: map[string]any{ + "tags": []any{"b", "a"}, + }, + current: makeCurrent(nil, []string{"a", "b"}, nil, nil), + wantUpdate: false, + }, + { + name: "pre-F7 state — Outputs lacks target keys, desired has targets", + desiredCfg: map[string]any{ + "droplet_ids": []any{123}, + }, + current: &interfaces.ResourceOutput{ProviderID: "fw-uuid", Outputs: map[string]any{"status": "succeeded"}}, + wantUpdate: true, + wantPath: "droplet_ids", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := d.Diff(ctx, interfaces.ResourceSpec{Name: "my-fw", Config: tc.desiredCfg}, tc.current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if got.NeedsUpdate != tc.wantUpdate { + t.Errorf("NeedsUpdate = %v, want %v (changes=%v)", got.NeedsUpdate, tc.wantUpdate, got.Changes) + } + if tc.wantPath != "" { + found := false + for _, c := range got.Changes { + if c.Path == tc.wantPath { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange with Path=%q in Changes=%+v", tc.wantPath, got.Changes) + } + } + }) + } +} + +// toInts is a tolerant Outputs-coercer for tests that may receive []int or +// []any-of-numerics depending on whether state was round-tripped through +// JSON/YAML state encoding. +func toInts(v any) []int { + switch t := v.(type) { + case []int: + return append([]int(nil), t...) + case []any: + out := make([]int, 0, len(t)) + for _, x := range t { + switch n := x.(type) { + case int: + out = append(out, n) + case int64: + out = append(out, int(n)) + case float64: + out = append(out, int(n)) + } + } + return out + } + return nil +} + +// toStrings is the analogous coercer for []string Outputs. +func toStrings(v any) []string { + switch t := v.(type) { + case []string: + return append([]string(nil), t...) + case []any: + out := make([]string, 0, len(t)) + for _, x := range t { + if s, ok := x.(string); ok { + out = append(out, s) + } + } + return out + } + return nil +} + func equalIntSlices(a, b []int) bool { if len(a) != len(b) { return false From 54ba721a60d0d367fb32b9a0962dc97216aeceff Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 23:38:58 -0400 Subject: [PATCH 4/5] fix(do-plugin): firewall structpb-compatible Outputs + fractional ID + docs (F7 r3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: round-2 stored typed slices ([]int, []string, []godo.InboundRule, []godo.OutboundRule) on Outputs. The wfctl→plugin gRPC dispatch path encodes through structpb.NewStruct, which rejects native typed slices with "proto: invalid type". Round 2's Diff cascade fix was effectively a no-op in production gRPC mode — every reconcile would either fail at structpb encoding or surface spurious FieldChange because the post-roundtrip type assertions returned ok=false. This round normalizes Outputs to the structpb-compatible canonical shape: - droplet_ids: []any of float64 (structpb numeric demotion) - tags: []any of string - inbound_rules: []any of map[string]any{protocol, ports, sources: []any} - outbound_rules: []any of map[string]any{protocol, ports, destinations: []any} Diff comparisons are normalized symmetrically: desired-side rules are converted to canonical via inboundRulesCanonical / outboundRulesCanonical before DeepEqual against current.Outputs[*].([]any). Important fixes: - dropletIDsFromConfig now returns ([]int, error) and rejects fractional float64 values (e.g. YAML 123.9). Pre-fix, structpb-roundtripped 123.9 silently became 123 — the wrong Droplet attached. The error propagates through firewallRequest → Create / Update / Diff. - plugin.json + CHANGELOG no longer claim "wfctl infra plan rejects" the no-targets case. Validation runs in Create / Update (apply-time) only; DOProvider.Plan does not call them. Honest docs > overpromise. Tests (RED first; regression-invariant verified by stash of firewall.go): - TestFirewallDriver_StructpbBoundary_FwOutputAcceptedByStructpb pins the invariant that Outputs survives structpb.NewStruct. - TestFirewallDriver_StructpbBoundary_DiffSurvivesRoundTrip is the canonical regression: live firewall → fwOutput → structpb round-trip → Diff against matching desired must report NeedsUpdate=false. - TestFirewallDriver_DropletIDs_FractionalFloat_Rejected (sub-tests for fractional 123.9 rejected and integer-valued 123.0 accepted). - TestFirewallDriver_Diff_DetectsTargetsChange's makeCurrent helper now builds canonical-shape Outputs (matching what fwOutput produces) so the test fixtures are honest about the post-round-trip representation. Co-authored-by: Claude --- CHANGELOG.md | 29 +++- internal/drivers/firewall.go | 180 +++++++++++++++++++---- internal/drivers/firewall_test.go | 233 +++++++++++++++++++++++++++++- plugin.json | 2 +- 4 files changed, 402 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34d8847..5d19c54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,14 +15,17 @@ All notable changes to workflow-plugin-digitalocean are documented here. ### Changed -- **Firewall specs without targets now fail at plan time (P-2.F7)** — - `FirewallDriver.Create` and `FirewallDriver.Update` now reject specs that - declare neither `droplet_ids` nor `tags` with the error: +- **Firewall specs without targets now fail at apply time (P-2.F7)** — + `FirewallDriver.Create` and `FirewallDriver.Update` reject specs that + declare neither `droplet_ids` nor `tags` BEFORE any DO API call, with + the error: `firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or - trusted_sources`. DO firewalls do **not** attach to App Platform apps; - for App-Platform-only deployments, omit `infra.firewall` and use - `expose: internal` services plus `trusted_sources` on managed databases. + trusted_sources`. The validation runs at the start of every Apply + reconcile (Create + Update). DO firewalls do **not** attach to App + Platform apps; for App-Platform-only deployments, omit `infra.firewall` + and use `expose: internal` services plus `trusted_sources` on managed + databases. ### Fixed @@ -37,11 +40,23 @@ All notable changes to workflow-plugin-digitalocean are documented here. state without recorded fields is treated as having empty fields, so the first plan post-upgrade safely over-detects and re-asserts current configuration. -- **Empty / non-positive target entries now fail at plan time (P-2.F7)** — +- **Empty / non-positive target entries now fail at apply time (P-2.F7)** — `tagsFromConfig` filters empty strings and `dropletIDsFromConfig` filters IDs ≤ 0. Without these filters, a spec like `tags: [""]` or `droplet_ids: [0]` would slip past `validateFirewallTargets` (slice is non-empty after parsing) and fail only at the DO API call. +- **Fractional `droplet_ids` rejected, not truncated (P-2.F7)** — + `dropletIDsFromConfig` now returns an error when a numeric value is not + integer-valued. Pre-fix, a YAML `droplet_ids: [123.9]` silently truncated + to `123`, attaching the wrong Droplet. +- **Outputs are gRPC structpb-compatible (P-2.F7)** — `fwOutput` stores + `droplet_ids` / `tags` / `inbound_rules` / `outbound_rules` in the + canonical `[]any` shape (numerics as `float64`, rules as + `[]any`-of-`map[string]any`). The wfctl→plugin gRPC dispatch path + encodes Outputs through `structpb.NewStruct`, which rejects native typed + slices (`[]int`, `[]string`, struct slices). Storing canonical-from-the- + start ensures `Diff` reads symmetric values whether `current.Outputs` + is consumed in-process or after a structpb round-trip. ## [v0.7.9] - 2026-04-24 diff --git a/internal/drivers/firewall.go b/internal/drivers/firewall.go index 69f6679..c39e024 100644 --- a/internal/drivers/firewall.go +++ b/internal/drivers/firewall.go @@ -46,7 +46,10 @@ func NewFirewallDriverWithClient(c FirewallClient) *FirewallDriver { } func (d *FirewallDriver) Create(ctx context.Context, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { - req := firewallRequest(spec) + req, err := firewallRequest(spec) + if err != nil { + return nil, fmt.Errorf("firewall create %q: %w", spec.Name, err) + } if err := validateFirewallTargets(spec.Name, req); err != nil { return nil, err } @@ -118,7 +121,10 @@ func (d *FirewallDriver) resolveProviderID(ctx context.Context, ref interfaces.R } func (d *FirewallDriver) Update(ctx context.Context, ref interfaces.ResourceRef, spec interfaces.ResourceSpec) (*interfaces.ResourceOutput, error) { - req := firewallRequest(spec) + req, err := firewallRequest(spec) + if err != nil { + return nil, fmt.Errorf("firewall update %q: %w", ref.Name, err) + } if err := validateFirewallTargets(spec.Name, req); err != nil { return nil, err } @@ -157,15 +163,22 @@ func (d *FirewallDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) // because rule order is preserved in the API response and may carry user // intent. // +// Both sides of the rule comparison are normalized to the structpb- +// compatible canonical map shape so the comparison is symmetric whether +// `current.Outputs` is read in-process or after a wfctl→plugin gRPC round- +// trip. (F7 round 3 — gRPC boundary fix.) +// // Pre-F7 state without the recorded keys (legacy ResourceOutputs from older // plugin versions) is treated as having empty fields, which surfaces a Plan // action on first Diff post-upgrade — the safe over-detect direction. -// (Code-review Finding 1, F7 round 2.) func (d *FirewallDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { if current == nil { return &interfaces.DiffResult{NeedsUpdate: true}, nil } - desiredReq := firewallRequest(desired) + desiredReq, err := firewallRequest(desired) + if err != nil { + return nil, fmt.Errorf("firewall diff %q: %w", desired.Name, err) + } var changes []interfaces.FieldChange curIDs := outputsAsIntSlice(current.Outputs["droplet_ids"]) @@ -182,17 +195,19 @@ func (d *FirewallDriver) Diff(_ context.Context, desired interfaces.ResourceSpec }) } - curIn, _ := current.Outputs["inbound_rules"].([]godo.InboundRule) - if !reflect.DeepEqual(curIn, desiredReq.InboundRules) { + desiredIn := inboundRulesCanonical(desiredReq.InboundRules) + curIn, _ := current.Outputs["inbound_rules"].([]any) + if !reflect.DeepEqual(curIn, desiredIn) { changes = append(changes, interfaces.FieldChange{ - Path: "inbound_rules", Old: curIn, New: desiredReq.InboundRules, + Path: "inbound_rules", Old: curIn, New: desiredIn, }) } - curOut, _ := current.Outputs["outbound_rules"].([]godo.OutboundRule) - if !reflect.DeepEqual(curOut, desiredReq.OutboundRules) { + desiredOut := outboundRulesCanonical(desiredReq.OutboundRules) + curOut, _ := current.Outputs["outbound_rules"].([]any) + if !reflect.DeepEqual(curOut, desiredOut) { changes = append(changes, interfaces.FieldChange{ - Path: "outbound_rules", Old: curOut, New: desiredReq.OutboundRules, + Path: "outbound_rules", Old: curOut, New: desiredOut, }) } @@ -306,14 +321,22 @@ func (d *FirewallDriver) Scale(_ context.Context, _ interfaces.ResourceRef, _ in // // At least one of `droplet_ids` or `tags` must be set; this is enforced by // validateFirewallTargets, which Create and Update both call. -func firewallRequest(spec interfaces.ResourceSpec) *godo.FirewallRequest { +// +// Returns an error when `droplet_ids` contains a fractional float — YAML +// `123.9` would otherwise silently truncate to Droplet ID 123, attaching +// the wrong Droplet. +func firewallRequest(spec interfaces.ResourceSpec) (*godo.FirewallRequest, error) { + ids, err := dropletIDsFromConfig(spec.Config) + if err != nil { + return nil, err + } return &godo.FirewallRequest{ Name: spec.Name, - DropletIDs: dropletIDsFromConfig(spec.Config), + DropletIDs: ids, Tags: tagsFromConfig(spec.Config), InboundRules: inboundRulesFromConfig(spec.Config), OutboundRules: outboundRulesFromConfig(spec.Config), - } + }, nil } // inboundRulesFromConfig extracts canonical "inbound_rules" into godo shape. @@ -381,9 +404,14 @@ func outboundRulesFromConfig(cfg map[string]any) []godo.OutboundRule { // fwOutput records the firewall's targets (droplet_ids, tags) and rules // (inbound_rules, outbound_rules) on Outputs so Diff can detect in-place -// reconfiguration. Storing the godo-shape directly keeps the comparison -// symmetric with what `firewallRequest` builds from the desired cfg. -// (F7 round 2 — Diff cascade fix.) +// reconfiguration. Values are stored in structpb-compatible canonical +// shapes (`[]any` of `float64` / `string` / `map[string]any`) — the wfctl→ +// plugin gRPC dispatch path encodes Outputs through `structpb.NewStruct`, +// which rejects native typed slices ([]int, []string, []godo.InboundRule) +// with "proto: invalid type", and demotes all numerics to float64 on the +// way out. Storing canonical-from-the-start ensures the comparison +// performed in Diff is symmetric whether Outputs is consumed in-process +// or after a structpb round-trip. (F7 round 3 — gRPC boundary fix.) func fwOutput(fw *godo.Firewall) *interfaces.ResourceOutput { return &interfaces.ResourceOutput{ Name: fw.Name, @@ -391,29 +419,117 @@ func fwOutput(fw *godo.Firewall) *interfaces.ResourceOutput { ProviderID: fw.ID, Outputs: map[string]any{ "status": fw.Status, - "droplet_ids": append([]int(nil), fw.DropletIDs...), - "tags": append([]string(nil), fw.Tags...), - "inbound_rules": append([]godo.InboundRule(nil), fw.InboundRules...), - "outbound_rules": append([]godo.OutboundRule(nil), fw.OutboundRules...), + "droplet_ids": intsToAnySlice(fw.DropletIDs), + "tags": stringsToAnySlice(fw.Tags), + "inbound_rules": inboundRulesCanonical(fw.InboundRules), + "outbound_rules": outboundRulesCanonical(fw.OutboundRules), }, Status: fw.Status, } } +// intsToAnySlice converts a typed []int to the structpb-compatible +// []any-of-float64 shape. structpb represents all numerics as float64 on +// the wire, so storing Outputs in this shape is symmetric with what +// arrives back after a round-trip. +func intsToAnySlice(ids []int) []any { + if len(ids) == 0 { + return nil + } + out := make([]any, 0, len(ids)) + for _, n := range ids { + out = append(out, float64(n)) + } + return out +} + +// stringsToAnySlice converts a typed []string to []any of strings — +// structpb's required shape for list values. +func stringsToAnySlice(ss []string) []any { + if len(ss) == 0 { + return nil + } + out := make([]any, 0, len(ss)) + for _, s := range ss { + out = append(out, s) + } + return out +} + +// inboundRuleCanonical flattens a godo.InboundRule into the canonical map +// representation used for both Outputs storage and Diff comparison. The +// shape mirrors how the cfg expresses rules: {protocol, ports, sources} +// where sources is `[]any` of address strings. This drops Sources fields +// the cfg cannot express (Tags, DropletIDs, LoadBalancerUIDs, +// KubernetesIDs) — by design: those are outside the canonical schema, so +// changes to them must not surface in Diff. +func inboundRuleCanonical(r godo.InboundRule) map[string]any { + var addrs []any + if r.Sources != nil { + addrs = stringsToAnySlice(r.Sources.Addresses) + } + return map[string]any{ + "protocol": r.Protocol, + "ports": r.PortRange, + "sources": addrs, + } +} + +// inboundRulesCanonical converts a slice of godo.InboundRule into a +// structpb-compatible []any-of-map. +func inboundRulesCanonical(rules []godo.InboundRule) []any { + if len(rules) == 0 { + return nil + } + out := make([]any, 0, len(rules)) + for _, r := range rules { + out = append(out, inboundRuleCanonical(r)) + } + return out +} + +// outboundRuleCanonical mirrors inboundRuleCanonical for outbound rules +// (uses Destinations.Addresses). +func outboundRuleCanonical(r godo.OutboundRule) map[string]any { + var addrs []any + if r.Destinations != nil { + addrs = stringsToAnySlice(r.Destinations.Addresses) + } + return map[string]any{ + "protocol": r.Protocol, + "ports": r.PortRange, + "destinations": addrs, + } +} + +// outboundRulesCanonical converts a slice of godo.OutboundRule into a +// structpb-compatible []any-of-map. +func outboundRulesCanonical(rules []godo.OutboundRule) []any { + if len(rules) == 0 { + return nil + } + out := make([]any, 0, len(rules)) + for _, r := range rules { + out = append(out, outboundRuleCanonical(r)) + } + return out +} + func (d *FirewallDriver) SensitiveKeys() []string { return nil } func (d *FirewallDriver) ProviderIDFormat() interfaces.ProviderIDFormat { return interfaces.IDFormatUUID } -// dropletIDsFromConfig extracts the canonical "droplet_ids" list. Accepts the -// numeric variants the modular YAML loader can emit (int, int64, float64). -// Non-numeric entries and non-positive IDs are silently dropped: Droplet IDs -// are positive integers assigned by the DO API, so 0 / negatives are never -// valid and would only fail at apply time, defeating F7's plan-time-fail -// contract. (Code-review Finding 3, F7 round 2.) -func dropletIDsFromConfig(cfg map[string]any) []int { +// dropletIDsFromConfig extracts the canonical "droplet_ids" list. Accepts +// the numeric variants the modular YAML loader can emit (int, int64, +// float64) — and, post-structpb-roundtrip, all of these collapse to +// float64. Non-numeric entries and non-positive IDs are silently dropped +// (so the all-non-positive case is caught by validateFirewallTargets); +// fractional floats return an error rather than truncating, since YAML +// `123.9` silently becoming Droplet ID 123 would attach the wrong Droplet. +func dropletIDsFromConfig(cfg map[string]any) ([]int, error) { raw, ok := cfg["droplet_ids"].([]any) if !ok || len(raw) == 0 { - return nil + return nil, nil } out := make([]int, 0, len(raw)) for _, v := range raw { @@ -424,6 +540,12 @@ func dropletIDsFromConfig(cfg map[string]any) []int { case int64: id = int(t) case float64: + // structpb represents all numerics as float64. Reject + // fractional values explicitly so silent truncation can't + // substitute the wrong Droplet ID. + if t != float64(int64(t)) { + return nil, fmt.Errorf("droplet_ids: %v is not an integer", t) + } id = int(t) default: continue @@ -433,7 +555,7 @@ func dropletIDsFromConfig(cfg map[string]any) []int { } out = append(out, id) } - return out + return out, nil } // tagsFromConfig extracts the canonical "tags" list of Droplet/DOKS-pool tag diff --git a/internal/drivers/firewall_test.go b/internal/drivers/firewall_test.go index 9f9b68a..843b816 100644 --- a/internal/drivers/firewall_test.go +++ b/internal/drivers/firewall_test.go @@ -9,6 +9,7 @@ import ( "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" "github.com/GoCodeAlone/workflow/interfaces" "github.com/digitalocean/godo" + "google.golang.org/protobuf/types/known/structpb" ) type mockFirewallClient struct { @@ -671,16 +672,78 @@ func TestFirewallDriver_Diff_DetectsTargetsChange(t *testing.T) { ctx := context.Background() // Helper: build a current ResourceOutput whose Outputs reflects a live - // firewall (the way fwOutput will populate it after F7 round 2). + // firewall in the structpb-compatible canonical shape that fwOutput + // produces in round 3 (no typed slices on Outputs — see the + // StructpbBoundary tests). + canonicalInbound := func(rules []godo.InboundRule) []any { + if len(rules) == 0 { + return nil + } + out := make([]any, 0, len(rules)) + for _, r := range rules { + var addrs []any + if r.Sources != nil { + for _, a := range r.Sources.Addresses { + addrs = append(addrs, a) + } + } + out = append(out, map[string]any{ + "protocol": r.Protocol, + "ports": r.PortRange, + "sources": addrs, + }) + } + return out + } + canonicalOutbound := func(rules []godo.OutboundRule) []any { + if len(rules) == 0 { + return nil + } + out := make([]any, 0, len(rules)) + for _, r := range rules { + var addrs []any + if r.Destinations != nil { + for _, a := range r.Destinations.Addresses { + addrs = append(addrs, a) + } + } + out = append(out, map[string]any{ + "protocol": r.Protocol, + "ports": r.PortRange, + "destinations": addrs, + }) + } + return out + } + idsToAny := func(ids []int) []any { + if len(ids) == 0 { + return nil + } + out := make([]any, 0, len(ids)) + for _, n := range ids { + out = append(out, float64(n)) + } + return out + } + tagsToAny := func(tags []string) []any { + if len(tags) == 0 { + return nil + } + out := make([]any, 0, len(tags)) + for _, t := range tags { + out = append(out, t) + } + return out + } makeCurrent := func(ids []int, tags []string, in []godo.InboundRule, out []godo.OutboundRule) *interfaces.ResourceOutput { return &interfaces.ResourceOutput{ ProviderID: "fw-uuid", Outputs: map[string]any{ "status": "succeeded", - "droplet_ids": ids, - "tags": tags, - "inbound_rules": in, - "outbound_rules": out, + "droplet_ids": idsToAny(ids), + "tags": tagsToAny(tags), + "inbound_rules": canonicalInbound(in), + "outbound_rules": canonicalOutbound(out), }, } } @@ -866,3 +929,163 @@ func equalStringSlices(a, b []string) bool { } return true } + +// ── F7 Round 3 — gRPC structpb boundary regression ────────────────────────── +// +// The wfctl→plugin gRPC dispatch path encodes outputs through +// `structpb.NewStruct` then decodes via `.AsMap()`. structpb rejects native +// typed slices ([]string, []int, []godo.InboundRule, …) with "proto: invalid +// type"; numerics survive only as float64; structs lose their type identity +// and become map[string]any. Round-2 stored typed slices on Outputs, which +// meant the entire Diff cascade fix was a no-op in production gRPC mode — +// every reconcile would either fail at structpb encoding or surface spurious +// FieldChange because the post-roundtrip type-assertions returned ok=false. +// +// These tests pin the structpb-compatible Outputs contract. + +// firewallOutputsRoundTrip simulates the wfctl→plugin gRPC dispatch boundary +// by encoding Outputs through structpb.NewStruct then decoding via AsMap(). +// Mirrors `internal.grpcRoundTrip` (kept local because that helper lives in +// a different package). +func firewallOutputsRoundTrip(t *testing.T, outputs map[string]any) map[string]any { + t.Helper() + if outputs == nil { + return nil + } + s, err := structpb.NewStruct(outputs) + if err != nil { + t.Fatalf("structpb.NewStruct rejected Outputs (typed slices on Outputs are a bug): %v", err) + } + return s.AsMap() +} + +// TestFirewallDriver_StructpbBoundary_FwOutputAcceptedByStructpb pins the +// invariant that Outputs values returned by Create / Read are structpb- +// compatible. Without this, the wfctl→plugin gRPC encoding fails before the +// outputs even reach the wire. +func TestFirewallDriver_StructpbBoundary_FwOutputAcceptedByStructpb(t *testing.T) { + fw := &godo.Firewall{ + ID: "fw-uuid", Name: "my-fw", Status: "succeeded", + DropletIDs: []int{123, 456}, + Tags: []string{"bmw-prod", "edge"}, + InboundRules: []godo.InboundRule{ + {Protocol: "tcp", PortRange: "443", Sources: &godo.Sources{Addresses: []string{"0.0.0.0/0"}}}, + }, + OutboundRules: []godo.OutboundRule{ + {Protocol: "tcp", PortRange: "1-65535", Destinations: &godo.Destinations{Addresses: []string{"0.0.0.0/0"}}}, + }, + } + mock := &mockFirewallClient{fw: fw} + d := drivers.NewFirewallDriverWithClient(mock) + + out, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{123, 456}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + + if _, err := structpb.NewStruct(out.Outputs); err != nil { + t.Fatalf("structpb.NewStruct rejected Outputs (Outputs must be structpb-compatible — no typed slices, no struct values): %v", err) + } +} + +// TestFirewallDriver_StructpbBoundary_DiffSurvivesRoundTrip is the canonical +// regression for round-2's bug. It records the live firewall via fwOutput, +// round-trips Outputs through structpb, then asserts Diff against the +// matching desired spec produces NeedsUpdate=false. A failure here means the +// Diff cascade fix is a no-op in production gRPC mode. +func TestFirewallDriver_StructpbBoundary_DiffSurvivesRoundTrip(t *testing.T) { + fw := &godo.Firewall{ + ID: "fw-uuid", Name: "my-fw", Status: "succeeded", + DropletIDs: []int{123, 456}, + Tags: []string{"bmw-prod"}, + InboundRules: []godo.InboundRule{ + {Protocol: "tcp", PortRange: "443", Sources: &godo.Sources{Addresses: []string{"0.0.0.0/0"}}}, + }, + OutboundRules: []godo.OutboundRule{ + {Protocol: "tcp", PortRange: "1-65535", Destinations: &godo.Destinations{Addresses: []string{"0.0.0.0/0"}}}, + }, + } + mock := &mockFirewallClient{fw: fw} + d := drivers.NewFirewallDriverWithClient(mock) + + // Record the firewall via Create (which calls fwOutput internally). + out, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{123, 456}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + + // Round-trip Outputs through the structpb gRPC boundary. + rtOutputs := firewallOutputsRoundTrip(t, out.Outputs) + + // Build a current state with the round-tripped Outputs. + current := &interfaces.ResourceOutput{ + ProviderID: out.ProviderID, + Outputs: rtOutputs, + } + + // Same desired spec — Diff must NOT report a change. + desired := interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{ + "droplet_ids": []any{123, 456}, + "tags": []any{"bmw-prod"}, + "inbound_rules": []any{ + map[string]any{"protocol": "tcp", "ports": "443", "sources": []any{"0.0.0.0/0"}}, + }, + "outbound_rules": []any{ + map[string]any{"protocol": "tcp", "ports": "1-65535", "destinations": []any{"0.0.0.0/0"}}, + }, + }, + } + got, err := d.Diff(context.Background(), desired, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if got.NeedsUpdate { + t.Errorf("NeedsUpdate = true after structpb round-trip, want false. Spurious changes: %+v", got.Changes) + } +} + +// TestFirewallDriver_DropletIDs_FractionalFloat_Rejected verifies that +// fractional float values are rejected rather than silently truncated. +// YAML's `123.9` would otherwise become Droplet ID 123 — wrong droplet +// attached. (Code-review round-2 follow-up: float64 truncation.) +func TestFirewallDriver_DropletIDs_FractionalFloat_Rejected(t *testing.T) { + t.Run("fractional float rejected", func(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{123.9}}, + }) + if err == nil { + t.Fatal("expected error for fractional droplet_ids, got nil") + } + if mock.lastReq != nil { + t.Error("FirewallRequest reached godo client despite fractional droplet_ids") + } + }) + + t.Run("integer-valued float accepted", func(t *testing.T) { + mock := &mockFirewallClient{fw: testFirewall()} + d := drivers.NewFirewallDriverWithClient(mock) + + _, err := d.Create(context.Background(), interfaces.ResourceSpec{ + Name: "my-fw", + Config: map[string]any{"droplet_ids": []any{123.0}}, + }) + if err != nil { + t.Fatalf("Create: %v", err) + } + if got, want := mock.lastReq.DropletIDs, []int{123}; !equalIntSlices(got, want) { + t.Errorf("DropletIDs = %v, want %v", got, want) + } + }) +} diff --git a/plugin.json b/plugin.json index 4714956..4175238 100644 --- a/plugin.json +++ b/plugin.json @@ -35,7 +35,7 @@ ], "configSchema": { "infra.firewall": { - "description": "DigitalOcean cloud firewall. Attaches to Droplets by ID or by tag (which auto-attaches future Droplets / DOKS pools that receive the tag). Either `droplet_ids` or `tags` is REQUIRED; `wfctl infra plan` rejects firewalls with no targets. NOTE: DO firewalls do not attach to App Platform apps — for App-Platform-only deployments, use `expose: internal` services plus `trusted_sources` on managed databases.", + "description": "DigitalOcean cloud firewall. Attaches to Droplets by ID or by tag (which auto-attaches future Droplets / DOKS pools that receive the tag). Either `droplet_ids` or `tags` is REQUIRED; `wfctl infra apply` rejects firewalls with no targets before any DO API call. NOTE: DO firewalls do not attach to App Platform apps — for App-Platform-only deployments, use `expose: internal` services plus `trusted_sources` on managed databases.", "fields": { "droplet_ids": { "type": "array", From 8d4e86e8eab9af28e144c37cd7fdf70307db3c8f Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 27 Apr 2026 23:56:18 -0400 Subject: [PATCH 5/5] =?UTF-8?q?chore(do-plugin):=20firewall=20polish=20?= =?UTF-8?q?=E2=80=94=20sanitize=20comments,=20NoTargetsErrFmt=20const,=20g?= =?UTF-8?q?ofmt=20(F7=20r4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Sanitize internal-process language from doc comments per public-repo team conventions: tagsFromConfig (firewall.go), Diff doc, fwOutput doc, EmptyStringTagsRejected / ZeroOrNegativeDropletIDsFiltered / FractionalFloat_Rejected test docstrings, and the Diff cascade section header. Each comment now describes the underlying behavior contract rather than which round/finding produced it. - Extract the no-targets error format string to an exported constant drivers.NoTargetsErrFmt so the validator and tests share a single source of truth — drift between `firewall.go` and `firewall_test.go` the next time the wording is tightened. Tests reference fmt.Sprintf(drivers.NoTargetsErrFmt, "...") instead of redefining the literal. plugin.json + CHANGELOG remain documentation copies. - gofmt -w internal/drivers/firewall.go: ProviderIDFormat single-line → multi-line, matching the file's standard formatting. No behavior change. Tests still green. Co-authored-by: Claude --- internal/drivers/firewall.go | 38 +++++++++++------- internal/drivers/firewall_test.go | 65 +++++++++++++++---------------- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/internal/drivers/firewall.go b/internal/drivers/firewall.go index c39e024..8cadb91 100644 --- a/internal/drivers/firewall.go +++ b/internal/drivers/firewall.go @@ -152,11 +152,10 @@ func (d *FirewallDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) } // Diff compares the desired spec against the live firewall recorded on -// `current` to detect in-place reconfiguration. Pre-F7 the body was a stub -// that always returned NeedsUpdate=false — meaning every droplet_ids/tags -// toggle silently no-op'd at plan time. F7 round 2 extends it to compare the -// four canonical fields (`droplet_ids`, `tags`, `inbound_rules`, -// `outbound_rules`). +// `current` to detect in-place reconfiguration. It compares the four +// canonical fields (`droplet_ids`, `tags`, `inbound_rules`, +// `outbound_rules`) so that toggling any of them between deploys produces +// a Plan action rather than silently no-op'ing. // // `droplet_ids` and `tags` use SET semantics: reorder is not a change, since // DO normalizes membership server-side. Rules use ORDER-SENSITIVE deep-equal @@ -166,9 +165,9 @@ func (d *FirewallDriver) Delete(ctx context.Context, ref interfaces.ResourceRef) // Both sides of the rule comparison are normalized to the structpb- // compatible canonical map shape so the comparison is symmetric whether // `current.Outputs` is read in-process or after a wfctl→plugin gRPC round- -// trip. (F7 round 3 — gRPC boundary fix.) +// trip. // -// Pre-F7 state without the recorded keys (legacy ResourceOutputs from older +// Legacy state without the recorded keys (`ResourceOutput` written by older // plugin versions) is treated as having empty fields, which surfaces a Plan // action on first Diff post-upgrade — the safe over-detect direction. func (d *FirewallDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { @@ -411,7 +410,7 @@ func outboundRulesFromConfig(cfg map[string]any) []godo.OutboundRule { // with "proto: invalid type", and demotes all numerics to float64 on the // way out. Storing canonical-from-the-start ensures the comparison // performed in Diff is symmetric whether Outputs is consumed in-process -// or after a structpb round-trip. (F7 round 3 — gRPC boundary fix.) +// or after a structpb round-trip. func fwOutput(fw *godo.Firewall) *interfaces.ResourceOutput { return &interfaces.ResourceOutput{ Name: fw.Name, @@ -517,7 +516,9 @@ func outboundRulesCanonical(rules []godo.OutboundRule) []any { func (d *FirewallDriver) SensitiveKeys() []string { return nil } -func (d *FirewallDriver) ProviderIDFormat() interfaces.ProviderIDFormat { return interfaces.IDFormatUUID } +func (d *FirewallDriver) ProviderIDFormat() interfaces.ProviderIDFormat { + return interfaces.IDFormatUUID +} // dropletIDsFromConfig extracts the canonical "droplet_ids" list. Accepts // the numeric variants the modular YAML loader can emit (int, int64, @@ -562,7 +563,7 @@ func dropletIDsFromConfig(cfg map[string]any) ([]int, error) { // strings. Non-string entries and empty strings are dropped: the DO API // rejects empty tags, so a slice that contains only empty strings must fail // the targets-required validation rather than being silently sent to the -// API. (Code-review Finding 2, F7 round 2.) +// API. func tagsFromConfig(cfg map[string]any) []string { raw, ok := cfg["tags"].([]any) if !ok || len(raw) == 0 { @@ -577,13 +578,20 @@ func tagsFromConfig(cfg map[string]any) []string { return out } -// validateFirewallTargets returns the spec-mandated error when the firewall -// request has no DropletIDs and no Tags. The error string is verbatim from -// plan P-2.F7 step 3 — including the em dash and the App Platform clause — -// because operators search for it and reviewers grep for it. +// NoTargetsErrFmt is the format string for the error returned by +// validateFirewallTargets when a firewall spec has neither droplet_ids nor +// tags. The string is exported and stable so tests can assert exact-match +// equality without redefining the literal, and so plan-output greps and +// runbooks can rely on a fixed phrase. The em dash (U+2014) and the App +// Platform clause are part of the contract — DO firewalls do not protect +// App Platform apps, and the surfaced error must say so. +const NoTargetsErrFmt = `firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or trusted_sources` + +// validateFirewallTargets returns NoTargetsErrFmt-formatted error when the +// firewall request has no DropletIDs and no Tags. func validateFirewallTargets(name string, req *godo.FirewallRequest) error { if len(req.DropletIDs) == 0 && len(req.Tags) == 0 { - return fmt.Errorf("firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or trusted_sources", name) + return fmt.Errorf(NoTargetsErrFmt, name) } return nil } diff --git a/internal/drivers/firewall_test.go b/internal/drivers/firewall_test.go index 843b816..cbdc5d2 100644 --- a/internal/drivers/firewall_test.go +++ b/internal/drivers/firewall_test.go @@ -334,13 +334,12 @@ func TestFirewallDriver_Create_ProviderIDIsAPIAssigned(t *testing.T) { } } -// ── F7: Firewall target enforcement ────────────────────────────────────────── +// ── Firewall target enforcement ────────────────────────────────────────────── -// noTargetsErrFmt is the verbatim error string the spec requires when a -// firewall declares no targets. Character-for-character, including the em -// dash (U+2014) and the App Platform clause. Format-arg %q quotes the -// firewall name. See plan P-2.F7 step 3. -const noTargetsErrFmt = `firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or trusted_sources` +// Tests reference the no-targets error format string via +// `drivers.NoTargetsErrFmt`, the exported source-of-truth in firewall.go. +// Re-defining the literal here would risk drift the next time the +// validator's wording is tightened. // TestFirewallDriver_Create_DropletIDs_PassThrough verifies droplet_ids reach // godo.FirewallRequest.DropletIDs. @@ -465,7 +464,7 @@ func TestFirewallDriver_Create_NoTargets_Errors(t *testing.T) { if err == nil { t.Fatal("expected error for empty targets, got nil") } - want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + want := fmt.Sprintf(drivers.NoTargetsErrFmt, "my-fw") if got := err.Error(); got != want { t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) } @@ -490,7 +489,7 @@ func TestFirewallDriver_Update_NoTargets_Errors(t *testing.T) { if err == nil { t.Fatal("expected error for empty targets, got nil") } - want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + want := fmt.Sprintf(drivers.NoTargetsErrFmt, "my-fw") if got := err.Error(); got != want { t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) } @@ -522,12 +521,10 @@ func TestFirewallDriver_Create_DropletIDs_AcceptsMixedNumeric(t *testing.T) { // TestFirewallDriver_Create_EmptyStringTagsRejected verifies that an // all-empty-string tags slice fails the targets-required validation, and -// that a mixed slice is filtered to only the non-empty entries. -// -// Without filtering, `tagsFromConfig` appends "" to its output, making -// `len(req.Tags) > 0` falsely succeed; the DO API then rejects the empty -// tag at apply time — defeating F7's plan-time-fail contract. Fix is in -// `tagsFromConfig` (filter `s != ""`). (Code-review Finding 2, F7 round 2.) +// that a mixed slice is filtered to only the non-empty entries. This is a +// regression: empty-string tags must be filtered so target validation +// catches the no-targets condition at validation time rather than letting +// the DO API reject them at apply time — defeating F7's fail-fast contract. func TestFirewallDriver_Create_EmptyStringTagsRejected(t *testing.T) { t.Run("all empty strings → no targets error", func(t *testing.T) { mock := &mockFirewallClient{fw: testFirewall()} @@ -540,7 +537,7 @@ func TestFirewallDriver_Create_EmptyStringTagsRejected(t *testing.T) { if err == nil { t.Fatal("expected no-targets error for tags: [\"\"]; got nil") } - want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + want := fmt.Sprintf(drivers.NoTargetsErrFmt, "my-fw") if got := err.Error(); got != want { t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) } @@ -568,13 +565,11 @@ func TestFirewallDriver_Create_EmptyStringTagsRejected(t *testing.T) { // TestFirewallDriver_Create_ZeroOrNegativeDropletIDsFiltered verifies that // non-positive Droplet IDs are filtered out, by symmetry with the -// empty-string tag filter (Finding 2). Droplet IDs are positive integers -// assigned by the DO API; 0 and negatives are never valid and would be -// rejected at apply time. -// -// Without filtering, `dropletIDsFromConfig` appends every numeric to its -// output, so `droplet_ids: [0]` slips past validation and the DO API rejects -// it at runtime. (Code-review Finding 3, F7 round 2.) +// empty-string tag filter. Droplet IDs are positive integers assigned by +// the DO API; 0 and negatives are never valid and would be rejected at +// apply time. This is a regression: non-positive droplet IDs must be +// filtered so target validation catches no-targets at validation time +// rather than letting the DO API reject them at apply time. func TestFirewallDriver_Create_ZeroOrNegativeDropletIDsFiltered(t *testing.T) { t.Run("only non-positives → no targets error", func(t *testing.T) { mock := &mockFirewallClient{fw: testFirewall()} @@ -587,7 +582,7 @@ func TestFirewallDriver_Create_ZeroOrNegativeDropletIDsFiltered(t *testing.T) { if err == nil { t.Fatal("expected no-targets error for non-positive droplet IDs; got nil") } - want := fmt.Sprintf(noTargetsErrFmt, "my-fw") + want := fmt.Sprintf(drivers.NoTargetsErrFmt, "my-fw") if got := err.Error(); got != want { t.Errorf("error mismatch:\n got: %q\nwant: %q", got, want) } @@ -613,14 +608,14 @@ func TestFirewallDriver_Create_ZeroOrNegativeDropletIDsFiltered(t *testing.T) { }) } -// ── F7 Finding 1 — Diff cascade ────────────────────────────────────────────── +// ── Diff cascade ───────────────────────────────────────────────────────────── // -// Pre-F7, FirewallDriver.Diff was a stub: it returned NeedsUpdate=true for nil -// current and NeedsUpdate=false otherwise, which made every in-place toggle of -// targets, tags, or rules a silent no-op at plan time. F7 makes target -// reconfiguration the most common firewall lifecycle action, so Diff must -// detect changes to droplet_ids, tags, inbound_rules, and outbound_rules. -// (Code-review Finding 1, F7 round 2.) +// FirewallDriver.Diff must detect target/rule changes so that toggling +// droplet_ids, tags, inbound_rules, or outbound_rules between deploys +// produces a Plan action. A stub Diff that always reported NeedsUpdate=false +// for non-nil current would silently no-op the most common firewall lifecycle +// action — target reconfiguration — so the comparison covers all four +// canonical fields against state recorded by `fwOutput`. // TestFirewallDriver_FwOutput_RecordsTargetsAndRules verifies that Create // returns a ResourceOutput whose Outputs map carries the four target/rule @@ -673,8 +668,8 @@ func TestFirewallDriver_Diff_DetectsTargetsChange(t *testing.T) { // Helper: build a current ResourceOutput whose Outputs reflects a live // firewall in the structpb-compatible canonical shape that fwOutput - // produces in round 3 (no typed slices on Outputs — see the - // StructpbBoundary tests). + // produces (no typed slices on Outputs — see the StructpbBoundary + // tests for why). canonicalInbound := func(rules []godo.InboundRule) []any { if len(rules) == 0 { return nil @@ -1054,8 +1049,10 @@ func TestFirewallDriver_StructpbBoundary_DiffSurvivesRoundTrip(t *testing.T) { // TestFirewallDriver_DropletIDs_FractionalFloat_Rejected verifies that // fractional float values are rejected rather than silently truncated. -// YAML's `123.9` would otherwise become Droplet ID 123 — wrong droplet -// attached. (Code-review round-2 follow-up: float64 truncation.) +// YAML's `123.9` would otherwise become Droplet ID 123 — the wrong +// Droplet attached. structpb represents all numerics as float64, so a +// fractional input survives the gRPC boundary and the int conversion +// must reject it explicitly. func TestFirewallDriver_DropletIDs_FractionalFloat_Rejected(t *testing.T) { t.Run("fractional float rejected", func(t *testing.T) { mock := &mockFirewallClient{fw: testFirewall()}