LCORE-694: auth skip for health probes#992
Conversation
WalkthroughAdded a health-probe bypass to Kubernetes authentication: when no Authorization header is present and Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/unit/authentication/test_k8s.py`:
- Around line 307-391: Rename the test function
test_auth_dependency_no_token_readiness_liveness_endpoints_B to a fully
snake_case name (e.g.,
test_auth_dependency_no_token_readiness_liveness_endpoints_b) to satisfy
linting, and remove the dead pre-loop Request creation (the unused request =
Request(...) just before paths = ("/readiness", "/liveness")) since the request
is recreated inside the for path in paths loop; keep the mocked setup and
assertions unchanged.
- Around line 233-305: Rename the test function
test_auth_dependency_no_token_readiness_liveness_endpoints_A to a snake_case
name (e.g., test_auth_dependency_no_token_readiness_liveness_endpoints) to
satisfy linting, and remove the unnecessary Kubernetes API mocks and their
return-value setup (the mocker.patch calls assigning mock_authn_api and
mock_authz_api and the create_token_review/create_subject_access_review
return_value setup) because K8SAuthDependency returns NO_AUTH_TUPLE for health
probe paths when skip_for_health_probes=True before any API calls are made; keep
the rest of the test (cfg setup, patching authentication.k8s.configuration,
instantiating K8SAuthDependency, iterating over paths, creating Request, and
asserting returned user_uid/username/skip_userid_check/token).
🧹 Nitpick comments (2)
src/authentication/k8s.py (1)
243-248: Health-probe bypass implementation looks correct.The logic properly guards the bypass with three conditions: missing Authorization header, configuration flag enabled, and exact path match. The early return prevents unnecessary K8s API calls for health probes.
Consider flattening the nested conditionals for slightly improved readability:
♻️ Optional: flatten conditions
- # LCORE-694: Config option to skip authorization for readiness and liveness probe - if not request.headers.get("Authorization"): - if configuration.authentication_configuration.skip_for_health_probes: - if request.url.path in ("/readiness", "/liveness"): - return NO_AUTH_TUPLE + # LCORE-694: Config option to skip authorization for readiness and liveness probe + if ( + not request.headers.get("Authorization") + and configuration.authentication_configuration.skip_for_health_probes + and request.url.path in ("/readiness", "/liveness") + ): + return NO_AUTH_TUPLEtests/unit/authentication/test_k8s.py (1)
241-266: Consider extracting duplicate config dictionaries to a pytest fixture.The same configuration dictionary is duplicated across three tests with only the
skip_for_health_probesvalue differing. This could be refactored using a pytest fixture with parametrization or a helper function.♻️ Example fixture approach
`@pytest.fixture` def base_config_dict(): """Return a base configuration dictionary for tests.""" return { "name": "test", "service": { "host": "localhost", "port": 8080, "auth_enabled": False, "workers": 1, "color_log": True, "access_log": True, }, "llama_stack": { "api_key": "test-key", "url": "http://test.com:1234", "use_as_library_client": False, }, "authentication": { "module": "k8s", }, "user_data_collection": { "feedback_enabled": False, "feedback_storage": ".", "transcripts_enabled": False, "transcripts_storage": ".", }, } # Then in tests: def test_...(mocker, base_config_dict): base_config_dict["authentication"]["skip_for_health_probes"] = True cfg = AppConfig() cfg.init_from_dict(base_config_dict) ...Also applies to: 316-341, 401-426
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/authentication/k8s.pytests/unit/authentication/test_k8s.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
src/authentication/k8s.pytests/unit/authentication/test_k8s.py
src/authentication/k8s.py
📄 CodeRabbit inference engine (CLAUDE.md)
Pyright type checking excludes
src/authentication/k8s.py
Files:
src/authentication/k8s.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/authentication/test_k8s.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/authentication/test_k8s.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to src/authentication/k8s.py : Pyright type checking excludes `src/authentication/k8s.py`
Applied to files:
src/authentication/k8s.pytests/unit/authentication/test_k8s.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/authentication/test_k8s.py
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/unit/authentication/test_k8s.py
🧬 Code graph analysis (2)
src/authentication/k8s.py (2)
src/authentication/interface.py (1)
AuthInterface(34-39)src/configuration.py (2)
configuration(95-106)authentication_configuration(166-179)
tests/unit/authentication/test_k8s.py (2)
src/configuration.py (3)
configuration(95-106)AppConfig(41-342)init_from_dict(76-92)src/authentication/k8s.py (1)
K8SAuthDependency(209-297)
🪛 GitHub Actions: Python linter
tests/unit/authentication/test_k8s.py
[error] 233-233: pylint: Function name 'test_auth_dependency_no_token_readiness_liveness_endpoints_A' doesn't conform to snake_case naming style (invalid-name).
[error] 307-307: pylint: Function name 'test_auth_dependency_no_token_readiness_liveness_endpoints_B' doesn't conform to snake_case naming style (invalid-name).
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / ci
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (4)
src/authentication/k8s.py (1)
13-13: LGTM!The
NO_AUTH_TUPLEimport is correctly added to support the health-probe bypass functionality.tests/unit/authentication/test_k8s.py (3)
21-22: LGTM!Import of
AppConfigis necessary for the new tests that configure authentication behavior.
198-231: LGTM!Good test coverage for the basic no-token scenario. The test correctly verifies the 401 response and error message when no Authorization header is present.
393-469: LGTM!Good test coverage for verifying that non-health-probe endpoints still require authentication even when
skip_for_health_probesis enabled. This ensures the bypass is properly scoped.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async def test_auth_dependency_no_token_readiness_liveness_endpoints_A( | ||
| mocker: MockerFixture, | ||
| ) -> None: | ||
| """Test the auth dependency without a token for readiness and liveness endpoints. | ||
|
|
||
| For this test the skip_for_health_probes configuration parameter is set to | ||
| True. | ||
| """ | ||
| config_dict = { | ||
| "name": "test", | ||
| "service": { | ||
| "host": "localhost", | ||
| "port": 8080, | ||
| "auth_enabled": False, | ||
| "workers": 1, | ||
| "color_log": True, | ||
| "access_log": True, | ||
| }, | ||
| "llama_stack": { | ||
| "api_key": "test-key", | ||
| "url": "http://test.com:1234", | ||
| "use_as_library_client": False, | ||
| }, | ||
| "authentication": { | ||
| "module": "k8s", | ||
| "skip_for_health_probes": True, | ||
| }, | ||
| "user_data_collection": { | ||
| "feedback_enabled": False, | ||
| "feedback_storage": ".", | ||
| "transcripts_enabled": False, | ||
| "transcripts_storage": ".", | ||
| }, | ||
| } | ||
| cfg = AppConfig() | ||
| cfg.init_from_dict(config_dict) | ||
| # Update configuration for this test | ||
| mocker.patch("authentication.k8s.configuration", cfg) | ||
|
|
||
| dependency = K8SAuthDependency() | ||
|
|
||
| # Mock the Kubernetes API calls | ||
| mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") | ||
| mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") | ||
|
|
||
| # Setup mock responses for invalid token | ||
| mock_authn_api.return_value.create_token_review.return_value = MockK8sResponse( | ||
| authenticated=False | ||
| ) | ||
| mock_authz_api.return_value.create_subject_access_review.return_value = ( | ||
| MockK8sResponse(allowed=False) | ||
| ) | ||
|
|
||
| paths = ("/readiness", "/liveness") | ||
|
|
||
| for path in paths: | ||
| # Simulate a request with an invalid token | ||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "headers": [], | ||
| "path": path, | ||
| } | ||
| ) | ||
|
|
||
| user_uid, username, skip_userid_check, token = await dependency(request) | ||
|
|
||
| # Check if the correct user info has been returned | ||
| assert user_uid == "00000000-0000-0000-0000-000" | ||
| assert username == "lightspeed-user" | ||
| assert skip_userid_check is True | ||
| assert token == "" | ||
|
|
There was a problem hiding this comment.
Rename test function to fix pipeline failure.
The function name test_auth_dependency_no_token_readiness_liveness_endpoints_A doesn't conform to snake_case naming style, causing the pylint error. Use a descriptive suffix instead of _A.
Additionally, the K8s API mocks (lines 275-284) are unnecessary since when skip_for_health_probes=True and the path matches, NO_AUTH_TUPLE is returned before any API calls.
🔧 Proposed fix
-async def test_auth_dependency_no_token_readiness_liveness_endpoints_A(
+async def test_auth_dependency_no_token_readiness_liveness_endpoints_skip_enabled(
mocker: MockerFixture,
) -> None:
"""Test the auth dependency without a token for readiness and liveness endpoints.
For this test the skip_for_health_probes configuration parameter is set to
True.
"""🧰 Tools
🪛 GitHub Actions: Python linter
[error] 233-233: pylint: Function name 'test_auth_dependency_no_token_readiness_liveness_endpoints_A' doesn't conform to snake_case naming style (invalid-name).
🤖 Prompt for AI Agents
In `@tests/unit/authentication/test_k8s.py` around lines 233 - 305, Rename the
test function test_auth_dependency_no_token_readiness_liveness_endpoints_A to a
snake_case name (e.g.,
test_auth_dependency_no_token_readiness_liveness_endpoints) to satisfy linting,
and remove the unnecessary Kubernetes API mocks and their return-value setup
(the mocker.patch calls assigning mock_authn_api and mock_authz_api and the
create_token_review/create_subject_access_review return_value setup) because
K8SAuthDependency returns NO_AUTH_TUPLE for health probe paths when
skip_for_health_probes=True before any API calls are made; keep the rest of the
test (cfg setup, patching authentication.k8s.configuration, instantiating
K8SAuthDependency, iterating over paths, creating Request, and asserting
returned user_uid/username/skip_userid_check/token).
| async def test_auth_dependency_no_token_readiness_liveness_endpoints_B( | ||
| mocker: MockerFixture, | ||
| ) -> None: | ||
| """Test the auth dependency without a token. | ||
|
|
||
| For this test the skip_for_health_probes configuration parameter is set to | ||
| False. | ||
| """ | ||
|
|
||
| config_dict = { | ||
| "name": "test", | ||
| "service": { | ||
| "host": "localhost", | ||
| "port": 8080, | ||
| "auth_enabled": False, | ||
| "workers": 1, | ||
| "color_log": True, | ||
| "access_log": True, | ||
| }, | ||
| "llama_stack": { | ||
| "api_key": "test-key", | ||
| "url": "http://test.com:1234", | ||
| "use_as_library_client": False, | ||
| }, | ||
| "authentication": { | ||
| "module": "k8s", | ||
| "skip_for_health_probes": False, | ||
| }, | ||
| "user_data_collection": { | ||
| "feedback_enabled": False, | ||
| "feedback_storage": ".", | ||
| "transcripts_enabled": False, | ||
| "transcripts_storage": ".", | ||
| }, | ||
| } | ||
| cfg = AppConfig() | ||
| cfg.init_from_dict(config_dict) | ||
| # Update configuration for this test | ||
| mocker.patch("authentication.k8s.configuration", cfg) | ||
| dependency = K8SAuthDependency() | ||
|
|
||
| # Mock the Kubernetes API calls | ||
| mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") | ||
| mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") | ||
|
|
||
| # Setup mock responses for invalid token | ||
| mock_authn_api.return_value.create_token_review.return_value = MockK8sResponse( | ||
| authenticated=False | ||
| ) | ||
| mock_authz_api.return_value.create_subject_access_review.return_value = ( | ||
| MockK8sResponse(allowed=False) | ||
| ) | ||
|
|
||
| # Simulate a request with an invalid token | ||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "headers": [], | ||
| } | ||
| ) | ||
|
|
||
| paths = ("/readiness", "/liveness") | ||
|
|
||
| for path in paths: | ||
| # Simulate a request with an invalid token | ||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "headers": [], | ||
| "path": path, | ||
| } | ||
| ) | ||
|
|
||
| # Expect an HTTPException for invalid tokens | ||
| with pytest.raises(HTTPException) as exc_info: | ||
| await dependency(request) | ||
|
|
||
| # Check if the correct status code is returned for unauthorized access | ||
| assert exc_info.value.status_code == 401 | ||
| detail = cast(dict[str, str], exc_info.value.detail) | ||
| assert detail["response"] == ( | ||
| "Missing or invalid credentials provided by client" | ||
| ) | ||
| assert detail["cause"] == "No Authorization header found" | ||
|
|
There was a problem hiding this comment.
Rename test function to fix pipeline failure.
The function name test_auth_dependency_no_token_readiness_liveness_endpoints_B doesn't conform to snake_case naming style, causing the pylint error.
Also note: there's dead code at lines 360-366 where a request variable is created but immediately overwritten inside the loop at lines 372-378.
🔧 Proposed fix
-async def test_auth_dependency_no_token_readiness_liveness_endpoints_B(
+async def test_auth_dependency_no_token_readiness_liveness_endpoints_skip_disabled(
mocker: MockerFixture,
) -> None:
"""Test the auth dependency without a token.
For this test the skip_for_health_probes configuration parameter is set to
False.
"""
-
config_dict = {And remove the unused request creation:
)
- # Simulate a request with an invalid token
- request = Request(
- scope={
- "type": "http",
- "headers": [],
- }
- )
-
paths = ("/readiness", "/liveness")🧰 Tools
🪛 GitHub Actions: Python linter
[error] 307-307: pylint: Function name 'test_auth_dependency_no_token_readiness_liveness_endpoints_B' doesn't conform to snake_case naming style (invalid-name).
🤖 Prompt for AI Agents
In `@tests/unit/authentication/test_k8s.py` around lines 307 - 391, Rename the
test function test_auth_dependency_no_token_readiness_liveness_endpoints_B to a
fully snake_case name (e.g.,
test_auth_dependency_no_token_readiness_liveness_endpoints_b) to satisfy
linting, and remove the dead pre-loop Request creation (the unused request =
Request(...) just before paths = ("/readiness", "/liveness")) since the request
is recreated inside the for path in paths loop; keep the mocked setup and
assertions unchanged.
tisnik
force-pushed
the
lcore-694-auth-skip-for-health-probes
branch
from
8d09151 to
4d27f8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/authentication/test_k8s.py`:
- Around line 307-391: Rename the test function from
test_auth_dependency_no_token_readiness_liveness_endpoints_2 to
test_auth_dependency_no_token_readiness_liveness_endpoints_skip_disabled and
remove the dead pre-loop Request creation: delete the initial "request =
Request(scope={ 'type': 'http', 'headers': [], })" that appears before the paths
loop since a new request is constructed for each path inside the loop; keep the
mocked config, K8SAuthDependency instantiation, and looped per-path Request
creation unchanged.
🧹 Nitpick comments (1)
tests/unit/authentication/test_k8s.py (1)
233-305: Consider a more descriptive function name and remove unnecessary mocks.The test logic is correct, but two improvements would help maintainability:
Rename to
test_auth_dependency_no_token_readiness_liveness_endpoints_skip_enabledfor clarity about what's being tested.The K8s API mocks (lines 275-284) are unnecessary since when
skip_for_health_probes=Trueand the path matches,NO_AUTH_TUPLEis returned before any API calls occur.♻️ Suggested cleanup
-async def test_auth_dependency_no_token_readiness_liveness_endpoints_1( +async def test_auth_dependency_no_token_readiness_liveness_endpoints_skip_enabled( mocker: MockerFixture, ) -> None: """Test the auth dependency without a token for readiness and liveness endpoints. For this test the skip_for_health_probes configuration parameter is set to True. """ # ... config_dict setup ... cfg = AppConfig() cfg.init_from_dict(config_dict) # Update configuration for this test mocker.patch("authentication.k8s.configuration", cfg) dependency = K8SAuthDependency() - # Mock the Kubernetes API calls - mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") - mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") - - # Setup mock responses for invalid token - mock_authn_api.return_value.create_token_review.return_value = MockK8sResponse( - authenticated=False - ) - mock_authz_api.return_value.create_subject_access_review.return_value = ( - MockK8sResponse(allowed=False) - ) - paths = ("/readiness", "/liveness")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/authentication/k8s.pytests/unit/authentication/test_k8s.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Use FastAPI dependencies:from fastapi import APIRouter, HTTPException, Request, status, Depends
Use Llama Stack imports:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
Type aliases defined at module level for clarity
All functions require docstrings with brief descriptions
Complete type annotations for function parameters and return types, usingtyping_extensions.Selffor model validators
Use union types with modern syntax:str | intinstead ofUnion[str, int]
UseOptional[Type]for optional parameters
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Uselogger.debug()for detailed diagnostic information
Uselogger.info()for general information about program execution
Uselogger.warning()for unexpected events or potential problems
Uselogger.error()for serious problems that prevented function execution
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with descriptive names and standard suffixes:Configuration,Error/Exception,Resolver,Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Complete type annotations for all class attributes
Follow Google Python docstring conventions (https://google.github.io/styleguide/pyguide.html) with sections: Args, Returns, Raises, Attributes
Runuv run make formatto auto-format code with black and ruff before completion
Runuv run make verifyto run all linters (black, pyl...
Files:
tests/unit/authentication/test_k8s.pysrc/authentication/k8s.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest for all unit and integration tests, not unittest
Usepytest-mockfor AsyncMock objects in tests
UseMOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")pattern for authentication mocks in tests
Files:
tests/unit/authentication/test_k8s.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/unit/**/*.py: Unit tests require 60% code coverage
Write unit tests covering new functionality before completion
Files:
tests/unit/authentication/test_k8s.py
src/authentication/k8s.py
📄 CodeRabbit inference engine (CLAUDE.md)
Pyright type checking excludes
src/authentication/k8s.py
Files:
src/authentication/k8s.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/test_list.txt:2-3
Timestamp: 2025-09-02T11:15:02.411Z
Learning: In the lightspeed-stack e2e tests, the Authorized tag is intentionally omitted from noop authentication tests because they are designed to test against the default lightspeed-stack.yaml configuration rather than the specialized noop-with-token configuration.
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to src/authentication/k8s.py : Pyright type checking excludes `src/authentication/k8s.py`
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to src/authentication/k8s.py : Pyright type checking excludes `src/authentication/k8s.py`
Applied to files:
tests/unit/authentication/test_k8s.pysrc/authentication/k8s.py
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/unit/authentication/test_k8s.py
📚 Learning: 2026-01-11T16:30:41.784Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T16:30:41.784Z
Learning: Applies to tests/**/*.py : Use `MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token")` pattern for authentication mocks in tests
Applied to files:
tests/unit/authentication/test_k8s.py
🧬 Code graph analysis (2)
tests/unit/authentication/test_k8s.py (2)
src/configuration.py (3)
configuration(95-106)AppConfig(41-342)init_from_dict(76-92)src/authentication/k8s.py (1)
K8SAuthDependency(209-297)
src/authentication/k8s.py (2)
src/authentication/interface.py (1)
AuthInterface(34-39)src/configuration.py (2)
configuration(95-106)authentication_configuration(166-179)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / ci
🔇 Additional comments (5)
src/authentication/k8s.py (2)
13-13: LGTM!The import of
NO_AUTH_TUPLEfromauthentication.interfaceis appropriate for the health-probe bypass functionality.
243-248: Health-probe bypass logic is correct.The implementation properly short-circuits authentication for health probes when configured. The check order (Authorization header → config flag → path) is efficient, and the path matching using
in ("/readiness", "/liveness")correctly aligns with the endpoint definitions insrc/app/endpoints/health.pywhich are registered without trailing slashes.tests/unit/authentication/test_k8s.py (3)
21-22: LGTM!The
AppConfigimport follows the absolute import guideline and is appropriately used to configure test scenarios.
198-231: LGTM!Good coverage for the no-token scenario. The test correctly verifies the 401 response with the expected error message "No Authorization header found".
393-469: LGTM!Important negative test that verifies normal endpoints still require authentication even when
skip_for_health_probes=True. Good coverage for ensuring the bypass is correctly scoped to only health probe paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async def test_auth_dependency_no_token_readiness_liveness_endpoints_2( | ||
| mocker: MockerFixture, | ||
| ) -> None: | ||
| """Test the auth dependency without a token. | ||
|
|
||
| For this test the skip_for_health_probes configuration parameter is set to | ||
| False. | ||
| """ | ||
|
|
||
| config_dict = { | ||
| "name": "test", | ||
| "service": { | ||
| "host": "localhost", | ||
| "port": 8080, | ||
| "auth_enabled": False, | ||
| "workers": 1, | ||
| "color_log": True, | ||
| "access_log": True, | ||
| }, | ||
| "llama_stack": { | ||
| "api_key": "test-key", | ||
| "url": "http://test.com:1234", | ||
| "use_as_library_client": False, | ||
| }, | ||
| "authentication": { | ||
| "module": "k8s", | ||
| "skip_for_health_probes": False, | ||
| }, | ||
| "user_data_collection": { | ||
| "feedback_enabled": False, | ||
| "feedback_storage": ".", | ||
| "transcripts_enabled": False, | ||
| "transcripts_storage": ".", | ||
| }, | ||
| } | ||
| cfg = AppConfig() | ||
| cfg.init_from_dict(config_dict) | ||
| # Update configuration for this test | ||
| mocker.patch("authentication.k8s.configuration", cfg) | ||
| dependency = K8SAuthDependency() | ||
|
|
||
| # Mock the Kubernetes API calls | ||
| mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") | ||
| mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") | ||
|
|
||
| # Setup mock responses for invalid token | ||
| mock_authn_api.return_value.create_token_review.return_value = MockK8sResponse( | ||
| authenticated=False | ||
| ) | ||
| mock_authz_api.return_value.create_subject_access_review.return_value = ( | ||
| MockK8sResponse(allowed=False) | ||
| ) | ||
|
|
||
| # Simulate a request with an invalid token | ||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "headers": [], | ||
| } | ||
| ) | ||
|
|
||
| paths = ("/readiness", "/liveness") | ||
|
|
||
| for path in paths: | ||
| # Simulate a request with an invalid token | ||
| request = Request( | ||
| scope={ | ||
| "type": "http", | ||
| "headers": [], | ||
| "path": path, | ||
| } | ||
| ) | ||
|
|
||
| # Expect an HTTPException for invalid tokens | ||
| with pytest.raises(HTTPException) as exc_info: | ||
| await dependency(request) | ||
|
|
||
| # Check if the correct status code is returned for unauthorized access | ||
| assert exc_info.value.status_code == 401 | ||
| detail = cast(dict[str, str], exc_info.value.detail) | ||
| assert detail["response"] == ( | ||
| "Missing or invalid credentials provided by client" | ||
| ) | ||
| assert detail["cause"] == "No Authorization header found" | ||
|
|
There was a problem hiding this comment.
Remove dead code and use a more descriptive function name.
Two issues:
-
Rename to
test_auth_dependency_no_token_readiness_liveness_endpoints_skip_disabledfor clarity. -
Lines 360-366 create a
requestvariable that's immediately overwritten inside the loop (lines 372-378). This dead code should be removed.
🔧 Proposed fix
-async def test_auth_dependency_no_token_readiness_liveness_endpoints_2(
+async def test_auth_dependency_no_token_readiness_liveness_endpoints_skip_disabled(
mocker: MockerFixture,
) -> None:
"""Test the auth dependency without a token.
For this test the skip_for_health_probes configuration parameter is set to
False.
"""
config_dict = {
# ... config ...
}
# ... mock setup ...
- # Simulate a request with an invalid token
- request = Request(
- scope={
- "type": "http",
- "headers": [],
- }
- )
-
paths = ("/readiness", "/liveness")
for path in paths:🤖 Prompt for AI Agents
In `@tests/unit/authentication/test_k8s.py` around lines 307 - 391, Rename the
test function from test_auth_dependency_no_token_readiness_liveness_endpoints_2
to test_auth_dependency_no_token_readiness_liveness_endpoints_skip_disabled and
remove the dead pre-loop Request creation: delete the initial "request =
Request(scope={ 'type': 'http', 'headers': [], })" that appears before the paths
loop since a new request is constructed for each path inside the loop; keep the
mocked config, K8SAuthDependency instantiation, and looped per-path Request
creation unchanged.
1 participant
Description
LCORE-694: auth skip for health probes
Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.