From dd3403ecba90b10faa775b24b0696aab4c9457fb Mon Sep 17 00:00:00 2001 From: Prathamesh Lohakare Date: Sun, 9 Aug 2026 17:55:27 +0530 Subject: [PATCH] skills: codify GCP authoring lessons from CodeBundle live-run reviews Adds transferable authoring guidance distilled from maintainer review of shipped GCP CodeBundles (bundles that passed the structural scorer but failed on the first live run). Kept general/cloud-agnostic; no bundle-specific recipes. auth-gcp.md - New "Cross-Project Quota Project" section: set CLOUDSDK_BILLING_QUOTA_PROJECT=${GCP_PROJECT_ID} in the suite env so a cross-project SA / Workload Identity doesn't fail SERVICE_DISABLED against the caller's project (no-op for in-project SAs). - Refined the "degrade gracefully" shell rule: only swallow errors from commands that exist; for calls that can fail otherwise, validate the response shape and fail loud instead of defaulting to []. sli-authoring.md - "Reading Metrics Safely": don't swallow command-not-found / API errors into empty data (a health check scores that as passing); prefer a first-class, tested call over a guessed subcommand/flag. - "Report Enrichment, Not Rollup Tasks": don't add a redundant summary task that re-queries what the individual checks already collect; the SLI aggregates natively -- enrich each check's report instead. test-infra-gcp.md - Loading test data after provisioning for data-dependent checks (null_resource + local-exec, batched/encoded payloads), with a note on provider metric lag/flooring. skill-template-authoring.md - Make regenerating SKILL-TEMPLATE.md with generate_skill_md.py a required PR step; note generator truncation / sourced-helper caveats. Co-Authored-By: Claude Opus 4.8 (1M context) --- skills/auth-gcp.md | 65 +++++++++++++++++++++++++++++- skills/skill-template-authoring.md | 25 ++++++++++++ skills/sli-authoring.md | 50 +++++++++++++++++++++++ skills/test-infra-gcp.md | 60 +++++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 2 deletions(-) diff --git a/skills/auth-gcp.md b/skills/auth-gcp.md index 4f0e876..de54b12 100644 --- a/skills/auth-gcp.md +++ b/skills/auth-gcp.md @@ -124,6 +124,48 @@ Apply this pattern to **both** `runbook.robot` and `sli.robot`. --- +## Cross-Project Quota Project (the SERVICE_DISABLED trap) + +When the credential's own project differs from the target project -- +i.e. a Workload-Identity or service-account whose home project is *not* +the `GCP_PROJECT_ID` you're monitoring -- gcloud derives the API +consumer / quota project from the **credential's** project, not from +`--project` / `CLOUDSDK_CORE_PROJECT`. Any API that checks its own +enablement (Service Usage, Cloud Monitoring, Cloud Spanner, ...) then +fails against the *caller's* project with: + +```text +PERMISSION_DENIED: Cloud Spanner API has not been used in project + before or it is disabled ... SERVICE_DISABLED +``` + +even though the API is perfectly enabled on the target. The check is +reading the wrong project. + +**Fix: pin the quota/billing project to the target in the suite env.** + +```robot +Set Suite Variable +... ${env} +... {"CLOUDSDK_CORE_PROJECT":"${GCP_PROJECT_ID}","CLOUDSDK_BILLING_QUOTA_PROJECT":"${GCP_PROJECT_ID}","GOOGLE_APPLICATION_CREDENTIALS":"./${gcp_credentials.key}","PATH":"$PATH:${OS_PATH}","GCP_PROJECT_ID":"${GCP_PROJECT_ID}"} +``` + +`CLOUDSDK_BILLING_QUOTA_PROJECT` (equivalently `gcloud config set +billing/quota_project`) forces gcloud and REST calls made through the +gcloud session to bill/consume against `${GCP_PROJECT_ID}`. + +- **Cross-project SAs / Workload Identity** need + `roles/serviceusage.serviceUsageConsumer` on the **target** project + for the pinned quota project to be usable. +- **In-project SAs** (credential home == target) are unaffected -- + setting it is a **no-op**, so it is always safe to include. + +Set it in **both** `runbook.robot` and `sli.robot` suite env. This bites +every GCP bundle used with WI or a shared/cross-project SA; add it by +default. + +--- + ## Runtime Environment (what the platform sets for you) `runrobot.py` prepares these before Robot starts. Do **not** override @@ -223,8 +265,16 @@ Rules: (`bigquery.tables.list`, `bigquery.routines.list`) that read-only service accounts often lack; `bq ls`/`bq show` work with basic viewer roles. -4. **Degrade gracefully**: `2>/dev/null || echo "[]"` on discovery - commands so a permission gap produces an empty result, not a crash. +4. **Degrade gracefully -- but only for commands that exist.** + `2>/dev/null || echo "[]"` is fine on a `gcloud`/`bq` *discovery* + command where the only expected failure is a **permission gap** (turn + `Access Denied` into an empty result, not a crash). **Never** wrap it + around a command whose existence or success isn't guaranteed -- a + typo'd subcommand, a REST call, an unsupported flag. That pattern + converts "command not found" / "API error" into "no data", which a + health check then scores as passing. For calls that can fail that way, + **validate the response shape** and **fail loud** instead of + defaulting to `[]`. 5. **Check all BigQuery access field variants** -- `bq show` returns public principals under `specialGroup`, `iamMember`, or `groupByEmail` depending on how they were granted: @@ -322,6 +372,17 @@ rather than hardcoding secret references: `RW.Core.Import Secret` / `RW.Core.Import User Variable` and use the `gcp-auth.yaml` include in templates. +10. **Omitting `CLOUDSDK_BILLING_QUOTA_PROJECT`** -- with a cross-project + SA / Workload Identity, gcloud bills API calls to the credential's + project and enablement checks fail `SERVICE_DISABLED` against the + wrong project. Pin it to `${GCP_PROJECT_ID}` in the suite env (no-op + for in-project SAs). Found in `gcp-cloudspanner-instance-health`. + +11. **Swallowing a nonexistent command / API error into `[]`** -- + `bad-cmd 2>/dev/null || echo "[]"` reports "no data" as healthy. + Only degrade real permission gaps; for calls whose existence or + success isn't guaranteed, validate the response shape and fail loud. + --- ## Reference Implementation diff --git a/skills/skill-template-authoring.md b/skills/skill-template-authoring.md index 7f1bfb1..92342e6 100644 --- a/skills/skill-template-authoring.md +++ b/skills/skill-template-authoring.md @@ -254,6 +254,12 @@ ro runbook.robot ## Regeneration +**Running the generator is a required PR step, not optional.** Every +CodeBundle PR must ship a `SKILL-TEMPLATE.md`, and it must be +regenerated whenever `runbook.robot`, `sli.robot`, or the scripts change +-- otherwise the manifest drifts from the source of truth (or is missing +entirely, as `gcp-cloudspanner-instance-health` shipped without one). + Use the bundled generator after editing robot files: ```bash @@ -263,10 +269,25 @@ python3 scripts/generate_skill_md.py /path/to/codecollection --bundle azure-aks- Writes `SKILL-TEMPLATE.md` and removes legacy `SKILL.md` if present. +**Review the generator output before committing.** It is known to need +hand-fixes for: + +- **Description truncation** -- long `[Documentation]` / frontmatter + `description` values get cut off; restore the full sentence. +- **Sourced helper scripts** -- a task that `source`s a shared helper + script may not have that helper attributed under `## Source files`; + add it by hand. + +Regenerate, then diff and fix these before the manifest goes in the PR. + --- ## Validation Checklist +- [ ] `SKILL-TEMPLATE.md` exists and was **regenerated** with + `generate_skill_md.py` after the latest robot/script edits +- [ ] Generator output reviewed for description truncation and missing + sourced-helper attributions - [ ] File is at `codebundles//SKILL-TEMPLATE.md` (not `SKILL.md`) - [ ] Frontmatter includes `kind: skill-template` - [ ] `name` matches directory name @@ -298,3 +319,7 @@ Writes `SKILL-TEMPLATE.md` and removes legacy `SKILL.md` if present. 7. **Using `runner: ro` in frontmatter.** `ro` is devcontainer-only. Production uses the platform runner + worker + `runrobot.sh` inside `rw-base-runtime`. + +8. **Shipping a PR with no `SKILL-TEMPLATE.md` (or a stale one).** Running + `generate_skill_md.py` is a required PR step; regenerate after every + robot/script change and hand-fix truncation before committing. diff --git a/skills/sli-authoring.md b/skills/sli-authoring.md index c7bd3dc..e99def8 100644 --- a/skills/sli-authoring.md +++ b/skills/sli-authoring.md @@ -48,6 +48,50 @@ run it periodically. --- +## Reading Metrics Safely (learned from live failures) + +A structurally-perfect SLI/runbook can still score healthy while reading +**nothing** if the underlying data call silently fails. These pitfalls +shipped past review and only surfaced on a live run -- guard against them +in every check that reads a metric or API. (The +`CLOUDSDK_BILLING_QUOTA_PROJECT` cross-project trap lives in `auth-gcp.md`; +the principles here are cloud-agnostic.) + +1. **Don't swallow "command not found" / API errors into empty data.** + `some-cmd 2>/dev/null || echo "[]"` is only safe when the command + *exists* and the sole expected failure is a permission gap. Wrapped + around a nonexistent subcommand, a bad flag, or a failed API call it + turns a hard error into "no data available" -- which the SLI then + scores as **healthy**. Validate the response shape and **fail loud**: + + ```bash + if ! echo "$resp" | jq -e '' >/dev/null 2>&1; then + echo "ERROR: read failed: $resp" >&2 + exit 1 + fi + ``` + +2. **Prefer a first-class, tested call over a guessed one.** If a + read-only call has no obvious CLI form, use a shared, tested helper + (e.g. a REST call through the already-authenticated session) rather + than inventing a subcommand or flag. A command that doesn't exist + fails the same way a permission error does -- and pitfall #1 then + hides it. + +## Report Enrichment, Not Rollup Tasks + +Do **not** add a separate "health summary" / rollup task that re-queries +the same data the individual checks already collected. The SLI already +computes its aggregate natively (mean of sub-scores in Robot); a rollup +task just duplicates state/CPU/storage calls, doubles API cost, and drifts +out of sync with the real checks. Instead, have **each** check enrich the +report with its own context via `RW.Core.Add to Report` -- full resource +config JSON, the active thresholds it applied, the inventory it scanned. +The reader gets richer, per-dimension detail and there's a single source +of truth per signal. + +--- + ## `RW_LOOKBACK_WINDOW` on SLIs (platform variable) `RW_LOOKBACK_WINDOW` controls how far back time-windowed SLI signals look @@ -458,3 +502,9 @@ Use this checklist when building or reviewing a CodeBundle: `Set Variable` argument and can turn dicts into lists; use `Evaluate ... json` 10. **Confusing Normalize `2` with “×2 interval”** -- second arg is output format; platform already supplies seconds sized to the scrape interval +11. **Silently swallowing a failed data read** -- `cmd 2>/dev/null || echo "[]"` + around a nonexistent command or failed API call scores the SLI healthy on + zero data; validate response shape and fail loud instead +12. **Redundant rollup/summary task** -- re-queries data the individual checks + already produce and the SLI already averages; enrich each check's report + via `Add to Report` instead diff --git a/skills/test-infra-gcp.md b/skills/test-infra-gcp.md index 3ffce69..74210b8 100644 --- a/skills/test-infra-gcp.md +++ b/skills/test-infra-gcp.md @@ -326,6 +326,55 @@ labels = { } ``` +### Loading test data after provisioning (metric/utilization checks) + +Some checks -- storage utilization, row/object counts, throughput -- only +have something to read once the resource holds **data**. Provisioning an +empty instance leaves those metrics at zero and the check untested. Load a +small amount of data after `apply` with a `null_resource` + +`local-exec`, and exercise the check via a **threshold override** (e.g. +`STORAGE_UTILIZATION_THRESHOLD=0`) rather than provisioning realistically +huge infra: + +```hcl +resource "null_resource" "load_test_data" { + depends_on = [google_spanner_database.overloaded_database] + + triggers = { + database = google_spanner_database.overloaded_database.name + } + + provisioner "local-exec" { + command = "${path.module}/load_test_data.sh" + interpreter = ["/bin/bash", "-c"] + environment = { + TF_VAR_project_id = var.project_id + INSTANCE = google_spanner_instance.overloaded_instance.name + DATABASE = google_spanner_database.overloaded_database.name + } + } +} +``` + +Keep the loader script robust to CLI arg limits: + +- **Hex-encode payloads** and pass them as bytes/`BYTES` columns (or + decode inside the query). This avoids gcloud's `--data`/DML string + parser choking on quotes/newlines and keeps you under the **~128 KB** + total command-line argument limit -- batch inserts rather than one + giant statement. +- Prefer many small `gcloud spanner databases execute-sql` batches (or + `bq load` from a temp file) over a single multi-megabyte argument. + +**⚠️ Metric lag / flooring caveat.** Provider metrics are not instant and +often **floor small or young data to 0**. Cloud Spanner's +`storage/used_bytes`, for instance, can take **hours** to report a nonzero +value for a freshly-loaded few MB. So a data-loader makes the check +*exercisable* (via threshold override) but does **not** guarantee a +nonzero live reading in a short test window. Document the expected lag in +the bundle's test notes and don't treat a still-zero metric immediately +after load as a bug. + --- ## Common Mistakes @@ -356,6 +405,17 @@ labels = { setting `GOOGLE_APPLICATION_CREDENTIALS` short-circuits the metadata-server path. +7. **Provisioning an empty resource for a data-dependent check** -- + storage/count/throughput checks read zero against an empty instance + and go untested. Load a little data via `null_resource` + `local-exec` + (hex-encoded, batched) and exercise the check with a threshold + override -- but account for provider metric lag/flooring (Spanner + `used_bytes` can take hours to report nonzero). + +8. **Passing large payloads as a single CLI arg** -- gcloud DML/`--data` + parsers choke on quotes/newlines and there's a ~128 KB arg limit. + Hex-encode and batch, or `bq load` from a temp file. + --- ## Reference Implementation