fix(do-plugin): firewall must declare targets — fail plan when none (F7)#36
fix(do-plugin): firewall must declare targets — fail plan when none (F7)#36
Conversation
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 <noreply@anthropic.com>
Align the firewall canonical-schema key with workflow SDK terminology (see workflow@v0.19.0 schema/schema.go:637). Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Enforces that infra.firewall resources declare at least one target (droplet_ids or tags) and plumbs those targets into the generated godo.FirewallRequest, with accompanying schema/docs and unit test updates.
Changes:
- Add
droplet_ids/tagsextraction + target validation toFirewallDriverrequest building and Create/Update paths. - Expand firewall driver tests (including pass-through and no-targets error cases) and update existing fixtures to satisfy the new validation.
- Document the new keys and behavior in
plugin.jsonschema and theCHANGELOG.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/drivers/firewall.go |
Adds target extraction helpers, plumbs targets into FirewallRequest, and validates targets before Create/Update. |
internal/drivers/firewall_test.go |
Updates existing tests to include targets; adds new tests for pass-through, both-targets, and empty-target failures. |
internal/drivers/firewall_stateheal_test.go |
Updates state-heal tests to include droplet_ids so they continue exercising heal paths. |
plugin.json |
Adds infra.firewall configSchema documenting droplet_ids, tags, and rule arrays with an example. |
CHANGELOG.md |
Adds Unreleased notes describing new keys and the new “no targets” failure behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| } |
There was a problem hiding this comment.
validateFirewallTargets is only called from Create/Update, but DOProvider.Plan (internal/provider.go) never calls those methods—plan builds create actions without consulting the driver, and uses Diff for existing resources. As a result, empty-target firewalls will fail during Apply, not at plan time as described. If plan-time rejection is required, add validation in the planning path (e.g., in DOProvider.Plan before appending actions, or a validation hook invoked by Plan).
| 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)) | ||
| } |
There was a problem hiding this comment.
dropletIDsFromConfig converts float64 values to int via truncation. If a user supplies a non-integer (e.g., 123.9) it will silently become 123, which can attach the firewall to the wrong droplet. Consider rejecting non-integer/negative values (return an error during validation) or at least skipping float64 values that aren’t exactly integral (like dnsOptionalIntField does in internal/drivers/dns.go:634-638).
| ], | ||
| "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.", |
There was a problem hiding this comment.
This description claims wfctl infra plan rejects firewalls with no targets, but the implementation validates only in FirewallDriver.Create/Update (Apply-time). Unless there is separate plan-time schema validation outside this repo, this is likely inaccurate; either implement plan-time validation or adjust the wording to avoid promising plan-time failure.
| - **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. |
There was a problem hiding this comment.
This entry says specs without targets “fail at plan time”, but the current code adds validation only in FirewallDriver.Create/Update, and DOProvider.Plan does not call those methods. Either implement validation in the Plan path or adjust the changelog text to reflect Apply-time failure.
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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") | ||
| } |
There was a problem hiding this comment.
Tests validate fwOutput/Diff behavior in-process, but they don't exercise the gRPC structpb boundary that the plugin uses (typed slices are rejected, and structs round-trip as map[string]any). Consider adding a test that round-trips a firewall ResourceOutput.Outputs map through structpb (or JSON marshal/unmarshal) and then calls Diff, to ensure the chosen Outputs representation survives transport and still compares correctly.
| 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...), | ||
| }, |
There was a problem hiding this comment.
fwOutput is storing typed slices (e.g., []int / []string) and slices of godo structs (InboundRules/OutboundRules) directly in ResourceOutput.Outputs. Those values are not structpb-compatible across the plugin gRPC boundary (see internal/grpc_dispatch_test.go:26-33: typed slices are rejected by structpb.NewStruct with "proto: invalid type"), which can break Create/Update responses and later Diff inputs. Convert these outputs to structpb-safe shapes (e.g., []any for droplet_ids/tags; rules as []any of map[string]any) and keep the representation stable for round-tripping.
| 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, | ||
| }) |
There was a problem hiding this comment.
Diff reads current inbound_rules/outbound_rules via type assertions to []godo.InboundRule / []godo.OutboundRule. After state is transported over gRPC (structpb) and/or persisted, these will typically round-trip as []any of map[string]any, so the assertions will fail and Diff will treat current rules as nil—leading to perpetual diffs or missed diffs. Decode rules from the structpb-compatible representation (or store them in a canonical, comparable shape in Outputs) before comparing.
| ], | ||
| "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.", |
There was a problem hiding this comment.
plugin.json claims that "wfctl infra plan rejects firewalls with no targets", but the current implementation enforces targets only inside FirewallDriver.Create/Update. DOProvider.Plan does not call Create/Update for create actions and FirewallDriver.Diff does not validate targets, so plan-time rejection likely won’t happen (it would fail at apply time instead). Either wire target validation into plan-time code paths (e.g., validate in Diff and/or Plan when current is nil) or soften this wording so it matches actual behavior.
…+ docs (F7 r3)
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| } |
There was a problem hiding this comment.
PR title/description mention rejecting no-target firewalls at plan time, but the new validation only runs in Create/Update (Apply). DOProvider.Plan doesn’t call the driver for create actions, and Diff doesn’t call validateFirewallTargets either, so plans can still succeed and only fail during Apply. Either adjust the PR messaging to “apply-time validation” or add a plan-time validation hook (e.g., validate in Plan for create actions and in Diff for update actions) so invalid specs are rejected during planning as stated.
| // 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) | ||
| } |
There was a problem hiding this comment.
The exact no-targets error string is duplicated across validateFirewallTargets, tests, plugin.json, and the changelog. To reduce drift risk while still meeting the exact-match requirement, consider defining a single constant/format string in the drivers package (potentially exported) and using it from validateFirewallTargets and tests (and optionally referencing it when generating docs).
| // 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.) |
There was a problem hiding this comment.
Comments like “Code-review Finding X / round Y” encode review-process history rather than the underlying rationale. Consider rewriting these to describe the behavioral invariant/regression being tested (e.g., “regression: empty-string tags must be filtered so validation catches no-targets”). This keeps tests and code comments evergreen and easier to understand later.
| // 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.) | |
| // Regression: `tagsFromConfig` must filter out empty strings before | |
| // target validation. Otherwise `len(req.Tags) > 0` falsely succeeds for | |
| // tags like []any{""}, and the DO API rejects the empty tag later at | |
| // apply time instead of surfacing the expected no-targets error during | |
| // planning. |
| // ── 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.) |
There was a problem hiding this comment.
The section header and trailing note reference “Code-review Finding 1 / F7 round 2”, which will become stale over time. Consider replacing with a neutral description of the regression/requirement (e.g., “Diff must detect target/rule changes so plan surfaces updates”).
| // ── 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.) | |
| // ── Diff surfaces firewall target/rule changes ─────────────────────────────── | |
| // | |
| // FirewallDriver.Diff must detect in-place changes to firewall targets, tags, | |
| // and rules. If droplet_ids, tags, inbound_rules, or outbound_rules are not | |
| // recorded and compared, plan/apply can miss required updates to an existing | |
| // firewall. |
| // 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.) |
There was a problem hiding this comment.
This comment includes “Code-review Finding 2 / F7 round 2”, which is review-process metadata and will get stale. Consider rephrasing to just capture the lasting rationale (e.g., “empty tags must be filtered so target validation fails before the API call”).
| // API. (Code-review Finding 2, F7 round 2.) | |
| // API. |
# Conflicts: # CHANGELOG.md
…t const, gofmt (F7 r4) - 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return nil, fmt.Errorf("firewall update %q: %w", ref.Name, err) | ||
| } | ||
| if err := validateFirewallTargets(spec.Name, req); err != nil { |
There was a problem hiding this comment.
Update validates targets using spec.Name, but other Update errors use ref.Name. If ref.Name and spec.Name ever diverge (or spec.Name is empty), the no-targets error will reference the wrong firewall. Consider using ref.Name consistently for validation/error formatting in Update (or enforce/normalize spec.Name to ref.Name earlier).
| if err := validateFirewallTargets(spec.Name, req); err != nil { | |
| if err := validateFirewallTargets(ref.Name, req); err != nil { |
…ll target enforcement (#38) P-2 staging IaC alignment. Bumps plugin.json: - version: 0.6.2 → 0.8.0 - minEngineVersion: 0.3.51 → 0.20.1 (matches workflow v0.20.1 fixtures used by alignment + align/security-check integration tests). Consolidates the [Unreleased] block under [v0.8.0] - 2026-04-28 with preamble describing the three F-track features: - F4 expose: internal (PR #35) — App Platform services may opt out of the public edge route. Five validation guards reject misconfiguration at apply time. Diff cascade derives expose + image from the live AppSpec so in-place toggles produce Plan actions. - F5 http_port_protocol + protocol: grpc alias (PR #34) — explicit canonical key for HTTP/2 plus gRPC shorthand. - F7 firewall droplet_ids + tags target enforcement (PR #36) — firewalls now require at least one target; Diff detects in-place target/rule changes; Outputs round-trip through structpb cleanly. Co-authored-by: Claude <noreply@anthropic.com>
Summary
Implements P-2.F7.
infra.firewallspecs must now declare at least one ofdroplet_idsortags; specs with neither are rejected at plan time before any DigitalOcean API call. The error string is verbatim from the plan and includes the App Platform clause that explains why DO firewalls cannot protect App Platform services.godo.FirewallRequest.DropletIDs []intandgodo.FirewallRequest.Tags []string(godo v1.178.0).validateFirewallTargetsruns at the very top ofCreateandUpdate, beforeresolveProviderIDand before any HTTP call.firewall %q has no targets (specify droplet_ids or tags) — App Platform services cannot be firewall-protected; use expose: internal or trusted_sources— em dash U+2014.Branch / SHAs
feat/firewall-targets2a26a08865af2fe83f0f315b4a8d9a5ea3c2993a(origin/main)2e2337f91c1767fbe59b1a2a5b8c9e663f4f90cdTwo commits:
b901a7f— F7 implementation, tests, doc-comment, CHANGELOG, plugin.json canonical-schema entry.2e2337f— Rename the plugin.json schema key fromcanonicalSchematoconfigSchemato match workflow SDK terminology (workflow@v0.19.0 schema/schema.go:637).Files
internal/drivers/firewall.go—dropletIDsFromConfig,tagsFromConfig,validateFirewallTargets; Create/Update validate before any API call.firewallRequestplumbs both fields. Doc-comment onFirewallDriverexplains the App Platform exception.internal/drivers/firewall_test.go— 7 new tests: droplet_ids pass-through, tags pass-through, both-targets, Update pass-through, Create-no-targets exact-string, Update-no-targets exact-string, mixed-numeric YAML decoding. Existing happy-path tests gaindroplet_idsso they continue to exercise their original paths.internal/drivers/firewall_stateheal_test.go— state-heal fixtures gaindroplet_idsso they exercise the UUID/heal paths, not the new validation.plugin.json—iacProvider.configSchema["infra.firewall"]documentsdroplet_ids,tags,inbound_rules,outbound_rules, plus a tag-based example.CHANGELOG.md— Unreleased entry covers Added (target keys) and Changed (no-targets is now a hard error).Test output
Full repo:
GOWORK=off go test -race ./...— all packages pass.Self-review checklist
validateFirewallTargets+ helpers GREEN → regression invariant verified (full driver + full repo green).err.Error() == fmt.Sprintf(noTargetsErrFmt, name).mock.lastReq != nilguard in both empty-targets tests).firewall.go,firewall_test.go,CHANGELOG.md,plugin.json. Thefirewall_stateheal_test.gochange is necessary regression maintenance — pre-existing tests used empty Config that now fails the new validation; they gaindroplet_idsto keep exercising their original paths.plugin.json.iacProvider.configSchema["infra.firewall"]documentsdroplet_idsandtagswith types + descriptions + a tag-based example.golangci-lintreports only pre-existing issues indatabase.go/app_platform_migration_repair_test.go— unrelated to F7.origin/main(2a26a08);git pull --ff-only origin mainbefore push; no cross-task pre-baking.🤖 Generated with Claude Code