[Creator] [design-spec] gcp-apigee-traffic-health

[rw-codebundle-agent]

Automated CodeBundle from runwhen-contrib/codecollection-registry#158.

Source: runwhen-contrib/codecollection-registry#158
Author: rw-codebundle-agent

[Rohit-Ekbote]

Verified this against the design spec — the org anchoring, qualifier fix, auth gate, discovery-in-Suite-Initialization move and runbook-only changes all check out.

Evidence from a local run at 9973fd5:

Check	Result
.test/offline/run.sh	111 passed, 0 failed
.test/render/run.sh	19 passed, 0 failed
shellcheck -S style *.sh .test/**/*.sh	clean
Mutation pass (14 mutations, scratch copies)	14/14 turned the expected tier red

The mutation pass includes the ones specific to this PR: reverting qualifiers to ["project", "resource"], re-adding type: sli and type: slo to outputItems, and moving discovery back out of Suite Initialization into a task — each caught.

I also reproduced the original defect before the fix. Rendering the pre-fix template through runwhen-local's jinja2 config (SandboxedEnvironment, trim_blocks, lstrip_blocks, CustomUndefined) gave:

APIGEE_ORG -> 'missing_workspaceInfo_custom_variable'
alias      -> 'GCP Apigee Traffic and Performance Health for Organization
                missing_workspaceInfo_custom_variable'

since match_resource.resource_name is not an attribute runwhen-local builds and CustomUndefined.__str__ returns a placeholder rather than "". It now resolves correctly, and the render tier catches a revert in seconds — which the template grep alone would not.

Fixture provenance re-checked against the Apigee v1 discovery document rather than taken on trust: apis.list returns GoogleCloudApigeeV1ListApiProxiesResponse with the proxies field (matches the fixture), and environments/targetservers have no list method at all and return bare string arrays (also matches).

One non-blocking finding inline.

[Rohit-Ekbote]

Second pass, this time on the .test harness rather than the bundle code — the earlier review covered org anchoring, the qualifier fix, discovery-in-Suite-Initialization and config wiring, and all of that still checks out at a15e946.

Now that this branch is rebased onto main (fb50021), the tree carries gcp-apigee-environment-health, gcp-apigee-product-governance and gcp-apigee-proxy-health alongside this bundle. The conventions below are all sitting in sibling directories in this same checkout, so aligning means copying a file that is already here.

Re-verified at a15e946: .test/offline/run.sh 130 passed / 0 failed, .test/render/run.sh skips loudly without jinja2/pyyaml. The check logic is in good shape, and the fixture split is well reasoned — offline/fixtures/ for Apigee Management API shapes, mock/ for Cloud Monitoring aggregates, with the rationale recorded at the top of run.sh. All four mock scenarios (healthy, high_error, latency_target, multi_offender) are exercised and match the README table. Everything below is harness, not checks.

---

High

1. build-infra reports success when it did nothing

.test/Taskfile.yaml:34

else
  echo "tf.secret not found. Skipping Terraform apply (mock tests do not require real infra)."
fi

Exit 0. The parenthetical is true for the mock tier but the task is still telling its caller that infrastructure was provisioned when none was. The merged siblings treat this as a class of bug rather than a convenience — gcp-apigee-environment-health/.test/Taskfile.yaml:134:

Fixture provisioning must exit non-zero when it cannot run. Printing "Skipping" and returning 0 tells the caller the fixtures exist when they do not, and every check that follows then validates nothing.

Given the Terraform here is explicitly optional scaffold (see item 4), the cleanest resolution may be to drop build-infra from the harness entirely rather than to make it fail.

2. validate-generation-rules cannot fail

.test/Taskfile.yaml:87

ajv validate ... && echo "$yaml_file is valid." || echo "$yaml_file is invalid."

The || echo swallows the failure and the block's exit status is the trailing rm -rf. I ran the task's logic with a stub ajv that exits 1:

Validating rules/rule.yaml
stub-ajv: INVALID
rules/rule.yaml is invalid.
EXIT CODE = 0

It prints "is invalid" and passes. Second defect on .test/Taskfile.yaml:80: curl -s ... || echo "Warning: could not fetch schema" is strictly worse than no fallback — a 404 writes GitHub's error page into the schema file and the run continues, so every rule then "fails" against garbage while the task still exits 0. An outage becomes indistinguishable from broken rules and from success.

gcp-apigee-environment-health/.test/Taskfile.yaml:372 is the model: curl -fsS, a jq -e . sanity check on the download, a failure counter, and a guard that errors when the glob matches no rules.

This matters more here than elsewhere: validate-generation-rules is in this bundle's default chain, so it is currently the second of three steps that a green task run proves nothing about.

3. default never reaches discovery, and its description says otherwise

.test/Taskfile.yaml:9

  default:
    desc: "Run mock tests, validation, and (if creds present) terraform infra"
    cmds:
      - task: check-unpushed-commits
      - task: validate-generation-rules
      - task: run-mock-tests

build-infra is not in the list, so the "(if creds present) terraform infra" half of the description never happens. More importantly there is no generate-rwl-config / run-rwl-discovery, so this branch has no path to an end-to-end run at all — task cannot tell you whether the generation rule produces an SLX against a real indexed Apigee org.

All three merged siblings and #733 reach discovery in their default. Given this bundle's rule gates on gcp_apigee_organizations and the earlier review specifically fixed the qualifiers and the org anchoring, an actual discovery run is the thing that would confirm those fixes end to end.

4. No shared credential contract

.test here sources tf.secret directly. The three siblings all use load-credentials.sh, which resolves both accepted spellings of the org (APIGEE_ORG bare / TF_VAR_org_id prefixed) and the project, strips the organizations/ prefix before it reaches a REST path, and warns when an ambient env var disagrees with the file — the drift that a shared, one-per-project Apigee org makes easy. Three copies already exist in this tree; adopting it lets one tf.secret serve all four Apigee bundles in the checkout.

---

Medium

5. The Terraform provisions a GCS bucket nobody reads

.test/terraform/main.tf:15 creates google_storage_bucket.apigee_mock_data, and the header comment is candid that it is "inert by default and is NOT required to run the mock tests" — a scaffold for possible future work. But build-infra would really create it, and clean would really destroy it, so the harness has a live resource whose only purpose is to be a placeholder.

The sibling stubs handle this differently and better. proxy-health/.test/terraform/main.tf also cannot provision its fixtures, but instead of an inert resource it emits ground truth — discovery_expected_proxies and discovery_expected_findings — so a discovery run has something machine-readable to be checked against. If the Terraform here stays, that is a more useful thing for it to do than owning a bucket.

6. Discovery image is unpinned

.test/Taskfile.yaml:167 uses runwhen-local:latest. This bundle's rule gates on gcp_apigee_organizations, the resource type that caused proxy-health to pin 0.11.11 and add a pre-flight registry probe — on an older image discovery exits 0 and produces zero SLXs, which reads as "the rule matched nothing" when it is really an image problem.

Not unique to this PR — environment-health and product-governance also use :latest — and as of today upstream's newest release is v0.11.12 with gcp_apigee_organizations present in the registry, so latest has probably moved past it. "Probably" is the issue; proxy-health's pin-plus-probe is the pattern worth copying, and it pairs naturally with item 3.

---

Non-blocking

7. MOCK_DATA_FILE is a test-only branch in five production scripts

check_error_rates.sh, check_latency.sh, check_throughput.sh, check_target_performance.sh and discover_metrics_scope.sh each carry if [ -n "${MOCK_DATA_FILE:-}" ] && [ -f ... ]. To be clear, this is exercised — mock_scenario() drives all four checks through it across four scenarios, so it is not dead code, and the rationale in run.sh ("there is no API shape to get wrong in these") is reasonable.

The design question is whether it needs to be in shipped code at all. The Cloud Monitoring calls go through curl like everything else, so they could be stubbed at the offline/bin/curl boundary the Apigee calls already use — which is how all three merged bundles do it, and it leaves no test seam in production scripts. The fixtures themselves would carry over largely unchanged.

8. Access tokens are written to the xtrace stream — collection-wide, not introduced here

All five scripts run set -x and expand the token into the trace. Confirmed with the offline stub on check_latency.sh:

+ access_token=offline-fake-token
+ '[' -z offline-fake-token ']'
+++ curl -s -H 'Authorization: Bearer offline-fake-token' 'https://monitoring.googleapis.com/v3/...'

I checked whether this is specific to this branch before raising it — it is not. The merged gcp-apigee-environment-health/discover_topology.sh does the same under its own stub, so this is a pre-existing convention across the Apigee bundles and should not block this PR. Raising it because the rebase puts both in one tree and it would be cheap to fix everywhere at once. Note the token leaks at the assignment, so wrapping only the request is insufficient:

{ set +x; } 2>/dev/null
access_token=$(gcloud auth print-access-token 2>/dev/null || echo "")
set -x

9. The gap the mock tier cannot close

The thresholds are well tested; the path from a real Apigee runtime → Cloud Monitoring time series → finding is not tested at all, and no harness in the collection generates traffic (proxy-health's README notes the same limitation for its Analytics checks). The blocker is structural rather than anything about this PR: an Apigee X runtime instance is reachable only on a private IP inside the peered VPC, so producing traffic needs a client inside that network.

For what it is worth, a small VM in the authorized network makes it tractable:

curl -sk --resolve "$ENVGROUP_HOST:443:$INSTANCE_IP" "https://$ENVGROUP_HOST/<basepath>"

against a proxy the proxy-health fixtures already deploy, then wait out the Cloud Monitoring ingestion lag. That would give this bundle its first genuine end-to-end assertion and would serve proxy-health at the same time — probably worth a shared .test helper rather than anything in this PR.

---

Items 1–4 are the ones I would want resolved before merge, with 3 the most consequential: without discovery in default, a green task run does not exercise the generation rule that the rest of this PR was about.

[Rohit-Ekbote]

All nine items checked against the tree before changing anything; items 1–6 addressed in c956d08, 7–9 answered below.

1. build-infra reported success when it did nothing — confirmed. Now sources load-credentials.sh, which exits non-zero. I kept the task rather than dropping it, since item 3 puts discovery in default and that needs credentials resolved somewhere.

2. validate-generation-rules could not fail — confirmed, including the curl -s half. Replaced with the environment-health model: checked curl -fsS, jq -e sanity check on the download, failure counter, empty-glob guard.

3. default never reached discovery — confirmed, and I agree it was the most consequential. The chain is now credential-free tiers → build-infra → generate-rwl-config → run-rwl-discovery → validate-generation-rules. generate-rwl-config also no longer defaults the org to mock-org, which would have produced a workspaceInfo naming an organization that does not exist — discovery would then index nothing and report zero SLXs, the same symptom as item 6 with a different cause.

4. No shared credential contract — adopted load-credentials.sh verbatim from the siblings.

5. GCS bucket nobody reads — removed. Took your suggestion for what should replace it: the stub now publishes discovery ground truth (discovery_expected_resource_type, discovery_expected_slx_count, discovery_expected_resource_path).

6. Unpinned image — pinned to 0.11.11 with the pre-flight registry probe.

8. Tokens in xtrace — fixed, despite being pre-existing and non-blocking, because it is a live bearer credential in captured output and the fix is contained. Your note that the assignment leaks too was right and incomplete in one way worth recording: there is a third site. [ -z "$token" ] is expanded by xtrace before the test runs, so restoring tracing before the emptiness check just moves the leak rather than closing it. Verified zero occurrences of the stub sentinel across all five scripts, and the offline tier now asserts it on the real trace so it cannot regress. I did not touch the merged siblings — out of scope for this PR, and worth doing in one pass across the collection as you suggest.

7. MOCK_DATA_FILE as a test seam in production scripts — I am deliberately not doing this one, and want to be explicit rather than silent. You are right that stubbing at the curl boundary is cleaner and matches the merged bundles. But the Cloud Monitoring responses would have to be rebuilt as time-series JSON at that boundary, all four checks rewritten to lose the branch, and the four mock scenarios re-derived — a large change to the code paths that carry the actual threshold logic, for a structural improvement with no behavioural gain, on a PR that is otherwise reviewed. It reads to me as its own change. Happy to be overruled.

9. The gap the mock tier cannot close — agreed, and I have recorded it as a known gap in .test/README.md rather than leaving it implicit. The --resolve recipe against a VM in the authorized network is the right shape; as you say, it belongs in a shared .test helper serving proxy-health too.

Verification at c956d08: offline 152 passed / 0 failed (was 130), render 19/0, shellcheck clean, 34 mutations all detected — each xtrace suppression point unwrapped independently, each harness fix reverted, the bucket re-added.

Not verified: the HCL. terraform is not installed in this environment, so .test/terraform was not syntax-checked or planned.