RSPEED-2429: Add health probe skip to RH Identity auth plugin #1144
Conversation
WalkthroughAdds an early-return health-probe bypass to RHIdentityAuthDependency: when the x-rh-identity header is absent and configuration.authentication_configuration.skip_for_health_probes is true, requests to /readiness or /liveness return Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Auth as RHIdentityAuthDependency
participant Config as Configuration
participant App as Application
Client->>Auth: Request (no x-rh-identity header)
Auth->>Config: read authentication_configuration.skip_for_health_probes
alt path is /readiness or /liveness and skip enabled
Auth-->>Client: return NO_AUTH_TUPLE (bypass)
Client->>App: proceed with NO_AUTH_TUPLE
else otherwise
Auth->>Auth: attempt header extraction/decoding
alt header absent or invalid
Auth-->>Client: raise authentication error
else valid header
Auth-->>App: return user tuple
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/authentication/test_rh_identity.py (1)
360-370:⚠️ Potential issue | 🟠 MajorThis existing test will break due to the new configuration access path.
As noted in the source file review, the new health-probe skip logic accesses
configuration.authentication_configurationbefore the 401 raise. This test doesn't mock configuration, so it will fail with aLogicError. If the source code is restructured per the suggestion, no change is needed here. Otherwise, add configuration mocking:Alternative: mock configuration in this test
`@pytest.mark.asyncio` - async def test_missing_header(self) -> None: + async def test_missing_header(self, mocker: MockerFixture) -> None: """Test authentication fails when header is missing.""" + mock_config = mocker.MagicMock() + mock_config.authentication_configuration.skip_for_health_probes = False + mocker.patch("authentication.rh_identity.configuration", mock_config) + auth_dep = RHIdentityAuthDependency() request = create_request_with_header(None)
🤖 Fix all issues with AI agents
In `@src/authentication/rh_identity.py`:
- Around line 216-223: Reorder the early-auth checks in the RH identity handler
so the request path is checked before accessing configuration: first test if
request.url.path is one of "/readiness" or "/liveness", then check
request.headers.get("x-rh-identity"), and only if header is missing consult
configuration.authentication_configuration.skip_for_health_probes to decide
returning NO_AUTH_TUPLE; also remove the duplicate
request.headers.get("x-rh-identity") call and keep the existing 401 raise
behavior (affecting the code around identity_header, NO_AUTH_TUPLE, and the
missing-header path used by test_missing_header).
🧹 Nitpick comments (1)
tests/unit/authentication/test_rh_identity.py (1)
477-484: Assert against the importedNO_AUTH_TUPLEconstant instead of hardcoded values.Lines 481–484 duplicate the contents of
NO_AUTH_TUPLE. If that constant changes, these assertions will silently become stale. The file already imports fromconstants; consider importingNO_AUTH_TUPLEfromauthentication.interfaceand asserting directly:Suggested change
+from authentication.interface import NO_AUTH_TUPLE ... - user_id, username, skip_userid_check, token = await auth_dep(request) - - assert user_id == "00000000-0000-0000-0000-000" - assert username == "lightspeed-user" - assert skip_userid_check is True - assert token == "" + result = await auth_dep(request) + assert result == NO_AUTH_TUPLE
The RH Identity auth plugin was missing the skip_for_health_probes check that exists in the K8s auth plugin (LCORE-694). When enabled, liveness and readiness probes would get 401'd because they don't carry the x-rh-identity header. Add the same probe-skip logic to RHIdentityAuthDependency.__call__ so that requests to /readiness and /liveness bypass auth when skip_for_health_probes is configured. Ref: RSPEED-2429 Signed-off-by: Major Hayden <[email protected]>
major
force-pushed
the
rspeed-2429/rh-identity-health-probe-skip
branch
from
99ef413 to
ddc3a82
Compare
2 participants
Description
RSPEED-2429: The RH Identity auth plugin was missing the
skip_for_health_probescheck that exists in the K8s auth plugin (LCORE-694). Whenskip_for_health_probesis enabled and therh_identityauth module is active, health probe requests to/readinessand/livenessreceive a 401 because they don't carry thex-rh-identityheader.This fix adds the same probe-skip logic to
RHIdentityAuthDependency.__call__so that requests to/readinessand/livenessbypass auth whenskip_for_health_probesis configured.Type of change
Tools used to create PR
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests