From f2a509e77a84f06dc5c0be91ac462519a63f578a Mon Sep 17 00:00:00 2001 From: Griffen Fargo <3642037+gfargo@users.noreply.github.com> Date: Tue, 25 Aug 2026 09:24:21 -0400 Subject: [PATCH] docs: add GCP Cloud Armor update note to the provider checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The provider-adding checklist had an "Update note" section for Fastly (#186) but nothing for GCP (#187), even though GCP's build turned up more new information than Fastly's did — a real BaseFirewallClient deviation (async auth headers, #232), a structural fact no prior provider has (priority as the only id/order/addressing-key, no separate server-assigned id), and confirmation that #184/#185 both genuinely don't apply here. The main addition is the real-e2e-verification gap this checklist didn't previously have a category for: GCP shipped, passed a holistic epic-wide review, and was still broken on the first real command against any real policy, because neither unit tests nor the mock server's fixtures ever happened to include the mandatory default rule every real Cloud Armor policy carries. Two more bugs (a shared-code id-remapping gap, a dead --ci flag on download) surfaced the same way once real infrastructure was finally involved. Added as both a new bullet in the existing "Verification methodology" section (so it's an actionable checklist item for AWS, not just GCP history) and as the headline finding in the new GCP update note. --- .kiro/steering/adding-a-provider.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.kiro/steering/adding-a-provider.md b/.kiro/steering/adding-a-provider.md index dc28737..77d173e 100644 --- a/.kiro/steering/adding-a-provider.md +++ b/.kiro/steering/adding-a-provider.md @@ -36,6 +36,7 @@ Worth carrying forward for a new provider's tests, not just repeating by habit: - **Mutation-verify new regression tests before trusting them.** Don't just add a test and watch it pass — locally revert the fix (or invert the specific behavior the test claims to guard), confirm the test _fails_ with a clear message, then restore the fix and confirm it passes again. Several real gaps this pass found (the Cloudflare credential-resolution bug fixed in #193, the non-atomic config write fixed in [PR #194](https://github.com/gfargo/doorman/pull/194)) were caught specifically because this step was done, not skipped. - **Verify end-to-end against `demos/mock-server.mjs`, not just unit tests**, for anything that changes what actually gets sent over the wire (`DOORMAN_VERCEL_API_BASE_URL` points the real CLI at it). Unit-level mocks can pass while the real command-level flow is still broken. - **Check whether a "generalize this per-provider test" issue's suggested invariants actually hold for both existing providers before writing them into a shared suite.** #197's initial invariant list included "partial `syncRules` failure surfaces via `errors[]`" — checking it against real code first found Vercel's write model is per-rule (independently failable) while Cloudflare's is a single atomic ruleset replace with no comparable partial-failure shape. Forcing a uniform assertion would have either silently skipped Cloudflare or mischaracterized its write model; better to land what's honestly true today and grow the suite as more providers clarify what's actually universal. +- **Check up front whether the new provider's SDK/auth can be exercised in a fully offline mock-server e2e at all — don't assume it can just because the first three could.** GCP Cloud Armor (#187) couldn't: `google-auth-library`'s `GoogleAuth` has to reach Google's real infrastructure to mint an OAuth2 access token even when the actual API traffic is routed to a local mock server, so "thorough unit tests + a working mock server" left a real bug unreachable for months — every real Cloud Armor policy carries a mandatory default rule in a shape no test fixture happened to include, crashing the provider on literally its first real command (see the GCP update note below). If a new provider's SDK has the same property, budget a real-account verification pass as part of calling the provider done, not an optional follow-up — see #187's real-e2e pass (PR #264) for the pattern (a small, disposable, promptly-torn-down cloud resource, not a permanent test fixture). ## Update note: Fastly (#186) — a third provider, checklist held up, two new traps found @@ -48,3 +49,13 @@ Fastly Next-Gen WAF landed as the third provider. The checklist above was writte **New checklist item, missing above until now — a shared-schema trap that isn't provider-specific code:** `src/lib/schemas/commonSchemas.ts`'s `providerTypeSchema` was a **hardcoded** `z.enum(['vercel', 'cloudflare'])`, and `providersConfigSchema` only declared `vercel`/`cloudflare` keys. Neither is mentioned anywhere in this checklist's "what a new provider touches" list, and neither shows up from `PROVIDER_TYPES`/`CREDENTIAL_DESCRIPTORS`/`initProviders.ts` being correct — the compiler didn't catch it because `ProvidersConfig`'s new optional key still satisfied the zod schema's inferred type, and nothing in the existing test suite constructed a config with a third provider's `provider` field. The practical effect: every config with `provider: 'fastly'` would have failed `unifiedConfigSchema.safeParse` — silently rejected, with a confusing "Invalid enum value" error pointing nowhere near the real cause. Fixed by deriving `providerTypeSchema` from `PROVIDER_TYPES` instead of a literal list, so this can't recur for GCP/AWS. **Check `providerTypeSchema`/`providersConfigSchema` in `commonSchemas.ts` for the next provider even though this checklist's main list doesn't mention them** — this doc's list will be updated once GCP/AWS confirm there's nothing else in the same category. **Diffing gotcha worth carrying forward:** a translator function that unconditionally sets an optional field to `undefined` (e.g. `negated: someBoolean || undefined`) is _not_ the same object shape as omitting the key — `isDeepEqual`'s `Object.keys().length` check treats them as different. A local config loaded from disk (JSON strips `undefined` values on write) never has the key; a freshly-translated remote rule built this way always does — so `getChanges` would report a phantom "update" for every ordinary (non-negated) rule, on every single sync, forever, and it would never self-correct. Caught here by a rigorous "no changes when configs match" test using a _non-trivial_ rule (Vercel's own equivalent test only exercises empty rule arrays on both sides, so it doesn't catch this — see [#203](https://github.com/gfargo/doorman/issues/203), filed to check whether Vercel's `vercelToUnified` has the same latent bug). Fixed in Fastly's translator via conditional spread (`...(negated ? { negated: true } : {})`) instead. Worth the same scrutiny in GCP/AWS's translators — and in any future change to Vercel's/Cloudflare's. + +## Update note: GCP Cloud Armor (#187) — a fourth provider, one real fit deviation, and the deepest real-e2e gap yet + +GCP Cloud Armor landed as the fourth provider (#232/#234/#238/#240, seven epic-review findings fixed in #253-259, then a real end-to-end verification pass that found three more bugs — PR #264). It confirmed some of this checklist's open questions, broke one assumption every prior provider shared, and forced the new verification-methodology item above onto this list. In rough order of how much they should change how the *next* provider (AWS) gets built: + +- **The real-e2e gap above is the single most important finding here — read it as the headline, not a footnote.** #238 shipped, passed a holistic epic-wide review (#253-259), and was *still* broken on the very first real command against any real Cloud Armor policy: every real policy carries a mandatory, server-injected default rule using a match shape (`versionedExpr`/`config`, not CEL) that neither the unit tests nor the mock server's fixtures ever happened to include, so `translatePolicy` crashed immediately, unconditionally. Two more real bugs surfaced the same way, once the CLI was finally pointed at a real project: `applySyncResultToConfig` (shared, cross-provider code) only ever wrote a sync's id remapping back onto `config.rules[]`, never `config.ips[]` — invisible until a provider actually populated an IP-rule id remapping, which GCP was the first to do; and `download`'s `--ci` flag never actually gated its confirmation prompt, so the command couldn't run non-interactively at all despite advertising that it could. None of these three were reachable by unit tests or the mock server, no matter how thorough — only by running the actual CLI against actual infrastructure. +- **`BaseFirewallClient` needed one real, targeted change to fit — not a full escape-hatch bypass.** GCP's OAuth2 access token (via `google-auth-library`'s `GoogleAuth`) is short-lived (~1h) and must be minted fresh per request, unlike the other three providers' static tokens, so `getAuthHeaders()` had to become `async` across the shared base class (#232). The composable-utility escape hatch (#198) wasn't needed for this — a signature change to the shared base was the right call instead. AWS WAFv2's SigV4 signing remains the real test of whether the escape hatch is ever actually needed. +- **New structural fact, not seen in any prior provider: no separate server-assigned rule id at all.** A Cloud Armor rule's `priority` is simultaneously its id, its evaluation order, and its addressing key for update/delete — `CloudArmorFirewallService` has to assign priorities to brand-new rules itself (spaced 1000 apart, checked against a freshly-fetched remote policy to avoid collisions) rather than getting one back from a create call, and relocating a rule has no in-place API operation — it's remove-then-add-at-the-new-priority (Cloud Armor rejects a priority collision outright). **Check whether AWS WAFv2 shares this shape before assuming either way** — a `Priority` integer existing there too doesn't necessarily mean it plays all three roles the way Cloud Armor's does. +- **No dedicated IP-blocking resource — an IP rule is just a specially-shaped ordinary rule, and shape alone doesn't reliably distinguish them.** A hand-authored single-IP custom rule and a real IP-list entry produce byte-identical CEL — #248 found this ambiguity causes a self-perpetuating misclassify-and-recreate on every sync once it happens once. The actual fix needed the caller to already know which ids are local custom rules (so an existing rule's priority wins the classification tie over CEL shape), not something shape-matching alone can resolve — worth remembering if AWS also lacks a dedicated IP resource. +- **#184 (flat condition trees) and #185 (single-resource targeting) both confirmed genuinely not needed for GCP** — verified directly against Cloud Armor's real API/CEL docs *before* implementation, not assumed: a `google_compute_security_policy`'s rule content has no backend-service field at all (attachment is a one-directional pointer living on the backend-service side instead), and real-world Cloud Armor CEL usage is flat, explicitly capped at "up to five subexpressions" by Google's own docs. Both gaps remain real and expected to actually bind for AWS — GCP's clean fit here is provider-specific, not evidence either concern is broadly obsolete.