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.