test(e2e): pin the proxy health endpoints under admin-off#802
Conversation
The Admin API removal roadmap listed '/readyz relocation' as a prerequisite, on the premise that /readyz only existed on the admin listener. The premise is wrong: the proxy listener already serves both /livez and /readyz (aisix-proxy build_router), through the same shared health functions, with the config-freshness block wired from the same supervisor watch_status the admin handler uses (etcd/managed), and an always-fresh probe in file mode. A sweep of every deployment artifact (this repo's Dockerfile/CI/docker, and the control-plane repo's deploy tree) found no health probe pointing at the admin listener — so nothing needs relocating or repointing. What was missing is a regression pin: nothing asserted the surviving health surface works with the admin listener off. Extend the admin-disabled e2e with a health leg in both source modes (etcd + file): wait for /readyz to report ready, then assert /livez 200 'ok' and /readyz?verbose lists '[+]shutdown ok' and '[+]config ok' — so the eventual admin-listener removal cannot regress /livez//readyz on the proxy.
|
Warning Review limit reached
Next review available in: 45 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)
📝 WalkthroughWalkthroughAdds end-to-end coverage for proxy health endpoints when the Admin listener is disabled, covering both etcd and file configuration sources. ChangesAdmin-disabled health checks
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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: 1
🤖 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/admin-disabled-e2e.test.ts`:
- Around line 148-151: Update the guard around the application setup result so
only !etcdReachable calls ctx.skip() and returns; when etcd is reachable but app
is absent, fail explicitly instead of skipping. Preserve the existing behavior
for successful app setup.
🪄 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: bdd39b96-7ef7-4440-bc89-ee442d81fef5
📒 Files selected for processing (1)
tests/e2e/src/cases/admin-disabled-e2e.test.ts
| if (!etcdReachable || !app) { | ||
| ctx.skip(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not skip an application setup failure.
!app currently turns any setup/provisioning failure into a skipped test even when etcd is reachable. Skip only for unavailable etcd; fail explicitly when app is absent.
Proposed fix
- if (!etcdReachable || !app) {
+ if (!etcdReachable) {
ctx.skip();
return;
}
+ if (!app) throw new Error("setup failed");Based on learnings, only unreachable etcd should cause a skip; setup failures must surface.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!etcdReachable || !app) { | |
| ctx.skip(); | |
| return; | |
| } | |
| if (!etcdReachable) { | |
| ctx.skip(); | |
| return; | |
| } | |
| if (!app) throw new Error("setup failed"); |
🤖 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/admin-disabled-e2e.test.ts` around lines 148 - 151,
Update the guard around the application setup result so only !etcdReachable
calls ctx.skip() and returns; when etcd is reachable but app is absent, fail
explicitly instead of skipping. Preserve the existing behavior for successful
app setup.
Source: Learnings
Audit MEDIUM on the health pin: dropping the with_config_apply_age wiring would leave readyz reporting '[+]config ok' unconditionally — byte-identical to wired-and-fresh, silently downgrading readiness to shutdown-only. Pin the plumbing at the router level: a wired probe returning None must 503 with '[-]config failed: not ready'. The wiring itself cannot be e2e'd leak-proof (the pre-first-apply 503 window is milliseconds); the structural fix — a fail-closed default — is filed as #803. Also correct the etcd health leg's comment: readyz reports ready after the initial load applies (an empty prefix counts), not only after the seed lands.
|
Independent cold audit: PASS — all four premise-correction claims verified against source and empirically (the auditor re-ran the sweeps, including the AISIX-Cloud helm chart the PR body's sweep description didn't mention: CP-only components, no DP probe — conclusion unchanged and now repo-wide). MEDIUM — the pin couldn't catch the config-freshness gate becoming unwired (field- LOW — comment overstated: the etcd health leg's comment now says readyz reports ready after the initial load applies (an empty prefix counts), matching the supervisor's record-apply-on-first-cycle behavior. Housekeeping per audit: branch is 1 commit behind main (release-notes CI only — no path overlap, no semantic conflict); CodeRabbit shows a rate-limit false-green with zero comments to triage. Verification at the new head: |
1 participant
What
Pin the proxy listener's health endpoints under admin-off — the health surface that survives the Admin API's removal. Two e2e legs added to
admin-disabled-e2e(etcd + file source): wait for/readyzto report ready, then assert/livez→ 200okand/readyz?verboselists[+]shutdown ok+[+]config ok, all with no admin listener bound.Why — a premise correction
The Admin-API-removal roadmap listed "
/readyzrelocation + deployment-probe repointing" as a prerequisite, on the premise that/readyzexisted only on the admin listener. Investigation shows the premise is wrong:/livezand/readyz(aisix-proxybuild_router), via the same sharedhealth::livez_response/readyz_responsethe admin handlers call./readyzcarries the full readiness semantics: its config-freshness block is wired from the samesupervisor.watch_status()the admin handler reads (etcd and managed modes), and an always-fresh probe in file mode (no watch to go stale) —crates/aisix-server/src/main.rswireswith_config_apply_agein every mode.Dockerfile(noHEALTHCHECK),docker/entrypoint.sh, CI docker smoke (already probes the proxy/livez), and the control-plane repo's entiredeploy/tree — found no health probe pointing at the admin listener. There is nothing to relocate and nothing to repoint.So the only real gap was a regression pin: nothing asserted that the surviving health surface works with the admin listener off. This PR adds that pin, so the eventual admin-listener removal PR cannot silently regress
/livez//readyzon the proxy.The operational readiness story after removal is unchanged and now fully covered: proxy
/livez(liveness) + proxy/readyz(readiness incl. config freshness/drain) + metrics listener/status/ready(config-only readiness) — the latter already pinned bystatus-config-e2e.Verification
admin-disabled-e2e9/9 green (7 existing + 2 new health legs); the new legs do their own readiness wait so they hold under test filtering.npx tsc --noEmitclean on the changed file;health-minimal(held-back admin health surface) untouched and green.Follow-up (not this PR)
The repo README still leads with "configure through the admin API on
:3001" and points at the rewritten docs quickstart — stale against the deprecation story; belongs with the coexistence-release communication pass. Docs-side, the proxy/livez//readyzcould be listed on the ports/proxy-api reference pages as the surviving health surface.Summary by CodeRabbit