Skip to content

Fix authentication tests for InferenceService#1175

Merged
threcc merged 5 commits intoopendatahub-io:mainfrom
threcc:fix-auth-test
Mar 9, 2026
Merged

Fix authentication tests for InferenceService#1175
threcc merged 5 commits intoopendatahub-io:mainfrom
threcc:fix-auth-test

Conversation

@threcc
Copy link
Copy Markdown
Contributor

@threcc threcc commented Mar 5, 2026

Description

  • Remove workarounds for resolved RHOAIENG-19645 (cross-model auth returned 302 instead of 403 under old OAuth proxy — fixed by migration to kube-rbac-proxy, closed as WONTDO)
  • Update pod rollout workaround in patched_remove_raw_authentication_isvc from closed RHOAIENG-19275 to current RHOAIENG-52129
  • Add 403 Forbidden handling to verify_inference_response, deriving the RBAC resource name from the object's kind

Tests

  • Locally
  • Jenkins

@threcc threcc force-pushed the fix-auth-test branch 2 times, most recently from f497a34 to 35fca5d Compare March 6, 2026 08:49
@threcc threcc force-pushed the fix-auth-test branch 2 times, most recently from 5fa682b to 2a618dc Compare March 9, 2026 11:05
@threcc threcc marked this pull request as ready for review March 9, 2026 11:05
mwaykole
mwaykole previously approved these changes Mar 9, 2026
@threcc threcc changed the title update authentication tests Fix authentication tests for InferenceService Mar 9, 2026
@threcc threcc enabled auto-merge (squash) March 9, 2026 11:11
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 9, 2026

📝 Walkthrough

Walkthrough

Added logger and updated Jira ID in a test fixture, simplified authentication tests by removing cross-model branches and an admin_client parameter, and extended verify_inference_response to assert HTTP 403 Forbidden responses for unauthorized users.

Changes

Cohort / File(s) Summary
Test fixture updates
tests/model_serving/model_server/kserve/authentication/conftest.py
Imported and initialized logger (get_logger), changed Jira check to jira_id="RHOAIENG-52129", and added an info log when the Jira condition is true.
Authentication test simplification
tests/model_serving/model_server/kserve/authentication/test_kserve_token_authentication_raw.py
Removed UserInference and is_jira_open imports; updated Jira marker to RHOAIENG-52129; simplified test_cross_model_authentication_raw by dropping admin_client parameter and replacing multi-branch Jira-dependent logic with a single verify_inference_response(..., authorized_user=False) call.
Authorization error handling
tests/model_serving/model_server/utils.py
Added branch in verify_inference_response for authorized_user=False to validate HTTP 403 responses. Computes resource string (lowercased/pluralized kind) and asserts a Forbidden message pattern. Note: verify_inference_response assertions could reveal resource/user details — review for information exposure (CWE-200).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing authentication tests for InferenceService, which aligns with the PR's objectives of removing workarounds, simplifying test logic, and extending authentication handling.
Description check ✅ Passed PR description provides comprehensive context including motivation, detailed file-level changes, and test status with specific Jira references and technical details.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/model_serving/model_server/kserve/authentication/conftest.py`:
- Line 93: The fixture currently hard-wires a pod restart by calling
predictor_pod.wait_deleted(), which breaks tests that expect "no pod rollout";
instead, make the fixture rollout-agnostic by removing the
predictor_pod.wait_deleted() call and replace it with a polling loop that waits
for the desired auth annotation/setting to be observed on the predictor resource
(or on any current pod/deployment annotations) using the same client used
elsewhere in this file (e.g., check predictor.get_annotations() or query the
deployment/pod annotations) with a timeout, and do not assert a restart—leave
any restart-specific assertions to the rollout test that explicitly checks pod
recreation.

In `@tests/model_serving/model_server/utils.py`:
- Around line 95-96: The helper verify_inference_response currently hardcodes
"resource=inferenceservices" for the 403 check which breaks when called with an
InferenceGraph; update verify_inference_response to derive the resource name
from the passed inference_service object's kind (or type) instead of hardcoding
— e.g., compute resource = inference_service.kind.lower() + "s" (or map kinds to
expected RBAC resource names) and use that resource in the regex assertion (the
function name to edit is verify_inference_response and the parameter is
inference_service).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 8c5970f0-e9fe-4739-907b-702893e75223

📥 Commits

Reviewing files that changed from the base of the PR and between d476993 and 2a618dc.

📒 Files selected for processing (3)
  • tests/model_serving/model_server/kserve/authentication/conftest.py
  • tests/model_serving/model_server/kserve/authentication/test_kserve_token_authentication_raw.py
  • tests/model_serving/model_server/utils.py

Comment thread tests/model_serving/model_server/kserve/authentication/conftest.py Outdated
Comment thread tests/model_serving/model_server/utils.py Outdated
@threcc threcc disabled auto-merge March 9, 2026 11:22
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/model_serving/model_server/utils.py`:
- Around line 95-97: The current fallback regex in the 403 check only matches
the resource kind and can falsely accept denials for the wrong object; update
the assertion that builds the regex (where resource =
f"{inference_service.kind.lower()}s" and the assert on res["output"]) to also
require the specific inference_service.name in the forbidden message (include
inference_service.name in the regex pattern alongside resource and verb=get) so
the assertion matches the denied object name, not just the kind.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: f5509dc8-7409-458e-affc-756b971150a8

📥 Commits

Reviewing files that changed from the base of the PR and between 2a618dc and 648d0cb.

📒 Files selected for processing (3)
  • tests/model_serving/model_server/kserve/authentication/conftest.py
  • tests/model_serving/model_server/kserve/authentication/test_kserve_token_authentication_raw.py
  • tests/model_serving/model_server/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/model_serving/model_server/kserve/authentication/conftest.py

Comment thread tests/model_serving/model_server/utils.py
@threcc threcc enabled auto-merge (squash) March 9, 2026 15:07
Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

@threcc threcc merged commit 41a3ab1 into opendatahub-io:main Mar 9, 2026
8 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

@threcc threcc deleted the fix-auth-test branch March 9, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants