Skip to content

[Bugfix][Router] Authenticate static backend health checks - #1043

Open
keyuchen21 wants to merge 2 commits into
vllm-project:mainfrom
keyuchen21:fix/authenticated-static-health-checks
Open

[Bugfix][Router] Authenticate static backend health checks#1043
keyuchen21 wants to merge 2 commits into
vllm-project:mainfrom
keyuchen21:fix/authenticated-static-health-checks

Conversation

@keyuchen21

Copy link
Copy Markdown
Contributor

Summary

  • reuse the router's existing VLLM_API_KEY environment variable for static backend health checks
  • send the key as a Bearer token while preserving JSON and multipart request-specific behavior
  • keep unauthenticated health checks unchanged when the variable is unset or empty
  • document the behavior and add mocked unit coverage for JSON and transcription requests

The optional generic /ping health-check mode discussed in the issue remains out of scope because authenticated model-specific checks resolve the reported failure without introducing a new endpoint or configuration contract.

Fixes #631

Testing

  • uv run --frozen --group test pytest src/tests/test_utils.py src/tests/test_static_service_discovery.py (26 passed)
  • uv run --frozen --group test pytest src/tests (226 passed)
  • uv run --frozen pre-commit run --files src/vllm_router/utils.py src/tests/test_utils.py src/vllm_router/README.md

  • Make sure the code changes pass the pre-commit checks.
  • Sign-off your commit by using -s when doing git commit
  • Classify the PR with [Bugfix][Router].

Signed-off-by: keyuchen21 <keyuchen2020@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for including the VLLM_API_KEY as a Bearer token in the Authorization header during static backend health checks. It updates the is_model_healthy utility function, adds comprehensive unit tests, and updates the README documentation. The feedback suggests simplifying the transcription request construction in utils.py by passing the auth_headers dictionary directly to requests.post instead of conditionally building the request arguments.

Comment thread src/vllm_router/utils.py Outdated
Signed-off-by: keyuchen21 <keyuchen2020@gmail.com>
@sethforprivacy

Copy link
Copy Markdown

+1 on this — we hit the exact 401 failure this fixes, in front of three key-protected vLLM backends, and have had --static-backend-health-checks disabled since deployment because of it.

One note for the scope decision on the generic health endpoint: the stock probe is a real completion, so it queues behind in-flight chunked prefill on a busy engine. We measure 23.7 s mean / 34.0 s max TTFT under an 8×~6K-token burst, against a 10 s default probe timeout — so even with the Bearer header a saturated-but-healthy backend can be evicted. Details and the keyless GET /health alternative we run: #631 (comment)

Not a blocker for this PR — the auth fix is the right first step. Happy to follow up with the /health variant as an opt-in mode on top of it if that is wanted.

🤖 Generated with Claude Code

@keyuchen21

Copy link
Copy Markdown
Contributor Author

+1 on this — we hit the exact 401 failure this fixes, in front of three key-protected vLLM backends, and have had --static-backend-health-checks disabled since deployment because of it.

One note for the scope decision on the generic health endpoint: the stock probe is a real completion, so it queues behind in-flight chunked prefill on a busy engine. We measure 23.7 s mean / 34.0 s max TTFT under an 8×~6K-token burst, against a 10 s default probe timeout — so even with the Bearer header a saturated-but-healthy backend can be evicted. Details and the keyless GET /health alternative we run: #631 (comment)

Not a blocker for this PR — the auth fix is the right first step. Happy to follow up with the /health variant as an opt-in mode on top of it if that is wanted.

🤖 Generated with Claude Code

Thanks for the detailed production data — the TTFT numbers under load make the timeout-eviction problem very concrete.

Agree that GET /health as an opt-in mode is the right follow-up. Plan is to add a --static-backend-health-check-mode={completion,health} flag on top of this PR once it merges, defaulting to completion (current behavior with auth fix). Transcription would stay on the completion probe since /health can't exercise the audio path.

Happy to coordinate on this if you'd like to co-author, or we can open separate PRs — either works.

@ruizhang0101 : any preference on the flag name or approach before we start?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: Health check fails on vLLM instances with authentication – add API key handling

2 participants