test(e2e): migrate the seed-tail tests off the Admin API onto etcd/status - #792
Conversation
…atus Step 2 of retiring the Admin API (after the admin.enabled switch, #791): move the e2e tests that used the Admin API only for setup or readiness — not to test the Admin API itself — onto the direct-etcd SeedClient and the metrics/status listener, and run them with admin: false. Seed-only (4): cache-ttl (admin:false only); datadog (AdminClient → SeedClient for the exporter + routing seed); guardrail-disabled-bypass (Gate-A readiness moves from GET /admin/v1/guardrails to GET /status/config applied resource_counts — stronger, reads the applied snapshot not just the etcd write); provider-key-rotation (the revision oracle becomes an etcd read-back asserting the rotated secret — a more direct no-op guard, no shared-harness change). Mixed (5): background-health, runtime-mixed-filtering, cooldown-contract (7 spawn sites), retry-on-429-vs-background-ignore, runtime-status — seeding was already on SeedClient; the only surviving admin-shaped call is listModelStatuses, which reads GET /status/models on the metrics listener and survives admin:false. Tests that exercise the Admin API surface itself (characterization, file-mode 409, status-equivalence, health, rotate, deprecation-header, admin-auth) stay admin-on until the Admin API is removed. No harness or Rust changes; assertions unchanged (two readiness/no-op guards strengthened).
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe E2E tests now disable the application admin listener, use metrics endpoints for status and readiness checks, seed Datadog resources through E2E harness and listener setup
Datadog exporter provisioning
Direct readiness and rotation verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/src/cases/guardrail-disabled-bypass-e2e.test.ts`:
- Around line 129-133: Update the fetch call in the config/status probe to
retain the Response, validate response.ok before invoking json(), and handle a
non-OK response appropriately before accessing cfg. Preserve the existing
configuration parsing for successful responses.
In `@tests/e2e/src/cases/provider-key-rotation-e2e.test.ts`:
- Line 120: Update the prerequisite guard in the E2E test so only !etcdReachable
uses the existing skip behavior. Add a separate validation for app, upstream,
etcd, and pkId that throws an explicit error when any setup value is missing,
ensuring beforeAll provisioning failures fail the test rather than being
skipped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0428f7b9-4657-4266-84f6-d951422e85a8
📒 Files selected for processing (9)
tests/e2e/src/cases/background-health-e2e.test.tstests/e2e/src/cases/cache-ttl-eviction-e2e.test.tstests/e2e/src/cases/cooldown-contract-e2e.test.tstests/e2e/src/cases/datadog-exporter-e2e.test.tstests/e2e/src/cases/guardrail-disabled-bypass-e2e.test.tstests/e2e/src/cases/provider-key-rotation-e2e.test.tstests/e2e/src/cases/retry-on-429-vs-background-ignore-e2e.test.tstests/e2e/src/cases/runtime-mixed-filtering-e2e.test.tstests/e2e/src/cases/runtime-status-e2e.test.ts
|
|
||
| test("an in-place secret rotation under sustained load keeps dispatch serving and bumps revision", async (ctx) => { | ||
| if (!etcdReachable || !app || !upstream || !admin || !pkId) { | ||
| if (!etcdReachable || !app || !upstream || !etcd || !pkId) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fail the test on missing prerequisites instead of gracefully skipping.
As per learnings, graceful skips in E2E tests should exclusively apply to etcd unavailability (!etcdReachable). If variables like app, upstream, etcd, or pkId are falsy, it indicates a genuine failure during the beforeAll test setup (such as a provisioning failure). Masking these as a skip hides real regressions; they must surface as explicit test failures.
🛠️ Proposed fix to isolate etcd unavailability
Replace the condition with the following structure to ensure actual setup failures correctly fail the test block:
if (!etcdReachable) {
// Keep the existing skip logic here (e.g., return ctx.skip())
}
if (!app || !upstream || !etcd || !pkId) {
throw new Error("Test prerequisites not met (check beforeAll for failures)");
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/src/cases/provider-key-rotation-e2e.test.ts` at line 120, Update
the prerequisite guard in the E2E test so only !etcdReachable uses the existing
skip behavior. Add a separate validation for app, upstream, etcd, and pkId that
throws an explicit error when any setup value is missing, ensuring beforeAll
provisioning failures fail the test rather than being skipped.
Source: Learnings
…rotation Audit LOW: the header/scope/reference comments still described the rotation as `PUT /admin/v1/provider_keys/:id` with a revision-bump check, but the test now rotates via a declarative etcd update and verifies the rotated secret by reading the key back. Comment-only.
|
Independent cold audit (3 diverse lenses — coverage-preservation / admin-off-safety / classification-boundary). All three PASS, zero HIGH/MEDIUM.
LOWs: provider-key-rotation's stale |
…l gate CodeRabbit: fetch does not throw on HTTP error status; guard the readiness poll with res.ok before .json() so a transient non-200 retries cleanly instead of parsing an error body.
|
CI + CodeRabbit triage CI e2e failure = pre-existing flake, not this PR. The failing test was CodeRabbit comments:
|
1 participant
What
Migrate the e2e tests that still depended on the Admin API for setup or readiness onto the direct-etcd
SeedClientand the metrics/status listener, and run them withadmin: false. This is step 2 of retiring the Admin API (after #791 landed theadmin.enabledswitch): it moves the tests that don't test the Admin API itself out of the admin-on world, shrinking the Admin API's remaining e2e footprint to only the tests whose subject is the admin surface.9 files migrated (all now spawn
admin: false):Seed-only (4) — the Admin API was used purely to set up resources or as a removable oracle:
cache-ttl-eviction— already seeded viaSeedClient; only neededadmin: false.datadog-exporter—AdminClient→SeedClientfor the exporter + routing seed (the exporter delivery is the subject, not how it was seeded; the admin-write-path coverage lives in the held-back characterization test).guardrail-disabled-bypass— the Gate-A readiness probe (GET /admin/v1/guardrails, proving the rule loaded) becomes aGET /status/configcheck onapplied.resource_counts.guardrails, which reflects the applied snapshot on the metrics listener — stronger than reading etcd back (which would only prove the write landed), preserving the anti-vacuous intent.provider-key-rotation— the revision oracle (GET /admin/v1/provider_keys/:id→.revisionbumped) becomes an etcd read-back asserting the key now holds the rotated secret — a more direct no-op guard than a revision bump, using the existingEtcdClient.get(no shared-harness change).Mixed (5) — seeding was already on
SeedClient; the only remaining admin-shaped call islistModelStatuses, which readsGET /status/modelson the metrics listener (not the admin API) and survivesadmin: false:background-health,runtime-mixed-filtering,cooldown-contract(7 spawn sites),retry-on-429-vs-background-ignore,runtime-status.Out of scope (deliberately unchanged)
Tests whose subject is the Admin API surface stay admin-on until the Admin API is removed, then get deleted or split — tracked on the removal checklist (AISIX-Cloud#1007 Phase 4):
seed-vs-admin-characterization(seed≡admin equivalence),file-resource-source(file-mode write-409),status-models(/status/modelsvs/admin/v1/models/statusequivalence),health-minimal(/admin/v1/health),apikey-lifecycle/apikey-budget(rotate/budget admin ops),auth-baseline,openai-sdk-compat(RFC 9745 Deprecation-header contract),prometheus-metrics.status-configonly uses/admin/v1/healthas a probe (it doesn't seed via admin) — left for the readiness-default step.Method
Classification (seed-only / mixed / held-back / probe-only) was done by fanning out one reader per candidate file, then synthesized into this migration plan — so each file's admin usage was read and classified individually rather than pattern-matched.
Verification
npx tsc --noEmitadds no new type errors in the changed files. Each migrated file passes in isolation; full e2e suite green locally. No harness or Rust changes — the direct-etcd seed infra (SeedClient/EtcdClient) already existed, and/status/models//status/configon the metrics listener already serve with the admin listener off (#791). The migrated tests keep their assertions; only the seed/readiness transport moved off the Admin API (two readiness/no-op guards were strengthened, noted above).Summary by CodeRabbit