LCORE-694: add skip_for_health_probes support to jwk-token and api-key-token auth modules#1788
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)src/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (7)
WalkthroughTwo authentication dependency classes now support bypassing authentication for health probe ( ChangesHealth Probe and Metrics Authentication Bypass
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Jos-Jerus
changed the title
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
Please fix problems found by linters.
Jos-Jerus
force-pushed
the
fix/skip-health-probes-jwk-apikey
branch
from
dbb9a38 to
0bcbb15
Compare
![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 `@src/authentication/api_key_token.py`:
- Around line 73-78: Add a brief inline comment explaining the early-return
pattern: when request.headers lacks "Authorization" we check
_should_skip_auth(request) and return NO_AUTH_TUPLE if allowed, otherwise we
intentionally fall through to extract_user_token(request.headers) which will
raise the appropriate 401; add this comment near the Authorization header check
(around request.headers.get("Authorization"), _should_skip_auth, NO_AUTH_TUPLE,
and extract_user_token) so future readers understand why the header check
precedes the skip check.
- Around line 27-34: Extract the duplicated _should_skip_auth logic into a
single helper should_skip_auth in a new src/authentication/utils.py, changing
its signature to should_skip_auth(request: Request, auth_config:
AuthenticationConfiguration) and move the health/metrics checks there; update
api_key_token._should_skip_auth and jwk_token._should_skip_auth to import and
call should_skip_auth(request, configuration.authentication_configuration) (or
remove the duplicate functions and call the util directly), and replace the
docstring with a Google-style docstring including Parameters and Returns as
shown in the proposed refactor; ensure no behavior change and remove the
duplicated helper from other auth modules (k8s.py, rh_identity.py) to centralize
logic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8b0b421-ddae-4c91-9d12-589d6fb4ce76
📒 Files selected for processing (2)
src/authentication/api_key_token.pysrc/authentication/jwk_token.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 3
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/authentication/api_key_token.pysrc/authentication/jwk_token.py
🧬 Code graph analysis (2)
src/authentication/api_key_token.py (6)
src/configuration.py (1)
authentication_configuration(242-255)src/authentication/interface.py (1)
AuthInterface(34-48)src/authentication/utils.py (1)
extract_user_token(9-35)src/authentication/jwk_token.py (1)
_should_skip_auth(145-152)src/authentication/__init__.py (1)
get_auth_dependency(21-88)tests/unit/authentication/test_api_key_token.py (1)
test_api_key_with_token_auth_dependency_no_token(56-86)
src/authentication/jwk_token.py (4)
src/configuration.py (2)
configuration(104-115)authentication_configuration(242-255)src/authentication/interface.py (1)
AuthInterface(34-48)tests/unit/authentication/test_jwk_token.py (2)
test_no_auth_header(467-483)test_no_bearer(487-503)src/authentication/api_key_token.py (1)
_should_skip_auth(27-34)
🔇 Additional comments (4)
src/authentication/jwk_token.py (3)
145-152: Duplicate of_should_skip_authinapi_key_token.py.Same recommendation as for
api_key_token.py: consider extracting this to a shared utility inauthentication/utils.pyto centralize the logic.
19-21: LGTM!
203-207: LGTM!The insertion of the skip-auth check within the existing missing-header block cleanly preserves the original 401 behavior while enabling health probe bypass when configured.
src/authentication/api_key_token.py (1)
13-15: LGTM!
| def _should_skip_auth(request: Request) -> bool: | ||
| """Check if auth should be skipped for health probe or metrics endpoints.""" | ||
| auth_config = configuration.authentication_configuration | ||
| if auth_config.skip_for_health_probes and request.url.path in ("/readiness", "/liveness"): | ||
| return True | ||
| if auth_config.skip_for_metrics and request.url.path in ("/metrics",): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Duplicate helper function across auth modules.
This _should_skip_auth function is identical to the one in jwk_token.py (and likely mirrors logic in k8s.py/rh_identity.py). Consider extracting it to authentication/utils.py to centralize the skip-auth logic and avoid maintaining identical code in multiple modules.
Additionally, per coding guidelines, the docstring should follow Google conventions with Parameters and Returns sections.
♻️ Proposed refactor: extract to utils.py
Add to src/authentication/utils.py:
def should_skip_auth(request: Request, auth_config: AuthenticationConfiguration) -> bool:
"""Check if authentication should be skipped for health probe or metrics endpoints.
Parameters:
request: The incoming FastAPI request.
auth_config: The authentication configuration containing skip flags.
Returns:
True if authentication should be bypassed, False otherwise.
"""
if auth_config.skip_for_health_probes and request.url.path in ("/readiness", "/liveness"):
return True
if auth_config.skip_for_metrics and request.url.path in ("/metrics",):
return True
return FalseThen import and use in both auth modules:
-def _should_skip_auth(request: Request) -> bool:
- """Check if auth should be skipped for health probe or metrics endpoints."""
- auth_config = configuration.authentication_configuration
- if auth_config.skip_for_health_probes and request.url.path in ("/readiness", "/liveness"):
- return True
- if auth_config.skip_for_metrics and request.url.path in ("/metrics",):
- return True
- return False
+from authentication.utils import should_skip_auth🤖 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 `@src/authentication/api_key_token.py` around lines 27 - 34, Extract the
duplicated _should_skip_auth logic into a single helper should_skip_auth in a
new src/authentication/utils.py, changing its signature to
should_skip_auth(request: Request, auth_config: AuthenticationConfiguration) and
move the health/metrics checks there; update api_key_token._should_skip_auth and
jwk_token._should_skip_auth to import and call should_skip_auth(request,
configuration.authentication_configuration) (or remove the duplicate functions
and call the util directly), and replace the docstring with a Google-style
docstring including Parameters and Returns as shown in the proposed refactor;
ensure no behavior change and remove the duplicated helper from other auth
modules (k8s.py, rh_identity.py) to centralize logic.
There was a problem hiding this comment.
@Jos-Jerus this is actually a good point by code rabbit. It would be nice to extract the function to a shared module and reuse it.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@Jos-Jerus black still failing, please use |
…n auth modules The skip_for_health_probes and skip_for_metrics config options were only implemented in the k8s and rh_identity auth modules. The jwk_token and api_key_token modules immediately rejected unauthenticated requests without checking whether the request was for a health probe endpoint. Extract the skip logic into a _should_skip_auth helper to keep branch count within pylint limits. Fixes: lightspeed-core#1787 Signed-off-by: yjacobi <yjacobi@redhat.com>
Jos-Jerus
force-pushed
the
fix/skip-health-probes-jwk-apikey
branch
from
0bcbb15 to
8126f71
Compare
3 participants
Summary
The
skip_for_health_probesandskip_for_metricsconfig options were only implemented in thek8sandrh_identityauth modules. Thejwk_tokenandapi_key_tokenmodules immediately rejected unauthenticated requests without checking whether the request was for a health probe endpoint, forcing deployments to use TCP probes instead of HTTP health probes.Changes
src/authentication/jwk_token.py: Add health probe and metrics endpoint checks before raising 401 for missing Authorization headersrc/authentication/api_key_token.py: Same guard pattern added before token extractionBoth follow the existing pattern from
src/authentication/k8s.py(lines 440-447).Testing
authentication.module: jwk-tokenandskip_for_health_probes: trueGET /livenesswithout Authorization header → should return 200 (currently returns 401)GET /readinesswithout Authorization header → should return 200 (currently returns 401)POST /v1/querywithout Authorization header → should still return 401Fixes #1787
Summary by CodeRabbit