fix: update MaaS tests for subscription binding and fix gateway 500 e…#1303
fix: update MaaS tests for subscription binding and fix gateway 500 e…#1303dbasunag merged 2 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/cherry-pick', '/wip', '/lgtm', '/verified', '/hold'} |
📝 WalkthroughWalkthroughPR refactors MaaS authentication tests to dynamically construct request headers from subscription-bound API keys rather than using pre-built header fixtures. Adds two new class-scoped fixtures that mint API keys explicitly bound to free and premium subscription tiers, with automatic cleanup via key revocation. Updates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ❌ 2❌ Failed checks (2 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/model_serving/maas_billing/maas_subscription/utils.py (1)
190-213:⚠️ Potential issue | 🟠 MajorAssert that the requested subscription was actually persisted.
Sending
payload["subscription"]is not enough to prove the binding path works. The new happy-path tests only assert a 200 after inference, so they will still pass if/v1/api-keyssilently ignores this field and falls back to normal subscription selection. Validate the stored subscription in the create response or with a follow-upGET /v1/api-keys/{id}before returning.As per coding guidelines, "Focus on security, test structure and coding style adherence in new code introduced."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/utils.py` around lines 190 - 213, The create API-key helper currently only sends payload["subscription"] to api_keys_url but does not verify it was persisted; update the function that builds payload (using api_key_name, expires_in, subscription) to validate the stored subscription by parsing the create response body (ensure it contains the created key id and stored subscription) or by performing a follow-up GET to /v1/api-keys/{id} and comparing the returned "subscription" field to the requested subscription; if raise_on_error is True assert the values match (or raise AssertionError) and if False return the response and parsed body so tests can inspect the mismatch.tests/model_serving/maas_billing/maas_subscription/conftest.py (1)
389-410:⚠️ Potential issue | 🟠 MajorThis shared fixture is now free-subscription-specific and leaves a live key behind.
maas_headers_for_actor_api_keyis still used by the premium-path consumer intests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.pyLines 75-97, so pinning every key here tomaas_subscription_tinyllama_free.namechanges the fixture contract from “actor-scoped” to “free-subscription-scoped”. Because the fixture returns instead of yielding, each class also leaves an active API key behind after teardown (CWE-459). Parameterize the target subscription per consumer and revoke it in fixture teardown.As per coding guidelines, "Focus on security, test structure and coding style adherence in new code introduced."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 389 - 410, The fixture maas_headers_for_actor_api_key currently hardcodes subscription=maas_subscription_tinyllama_free.name and returns a live API key, which changes its contract and leaks keys; change the fixture to accept a parameter (e.g., target_subscription) and pass that into the create_api_key call instead of maas_subscription_tinyllama_free.name, convert the fixture to yield so you can perform teardown, capture the created API key identifier from create_api_key, and revoke it in the teardown using the existing revoke API helper (or appropriate revoke function) so consumers like tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py can parametrize the fixture for free vs premium paths and no keys are left active after tests.
🤖 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/maas_billing/maas_subscription/conftest.py`:
- Around line 554-570: The loop waiting for Kuadrant AuthPolicy enforcement
(using extra_auth_policy, auth_policies, kuadrant_wait_timeout, LOGGER) must
fail the fixture on timeout instead of logging a warning and proceeding; replace
the else-case after the while with raising an AssertionError (or pytest.fail)
that includes the name and timeout so the test aborts when authPolicies are not
both accepted and enforced, ensuring downstream tests don't run on an invalid
precondition.
In
`@tests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.py`:
- Around line 125-133: The test currently treats both 401 and 403 as acceptable
results which lets an authentication failure mask an authorization check; in the
test function (test_wrong_group_sa_denied_on_premium_model) change the assertion
to require resp.status_code == 403 (and update any expected_statuses passed into
the request helper so it no longer includes 401) so the test only accepts an
authenticated-but-forbidden response; alternatively, if you need to validate
authentication failures separately, split into a second test that asserts
resp.status_code == 401 for authn-negative cases using the same request helper
and resp variable.
---
Outside diff comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 389-410: The fixture maas_headers_for_actor_api_key currently
hardcodes subscription=maas_subscription_tinyllama_free.name and returns a live
API key, which changes its contract and leaks keys; change the fixture to accept
a parameter (e.g., target_subscription) and pass that into the create_api_key
call instead of maas_subscription_tinyllama_free.name, convert the fixture to
yield so you can perform teardown, capture the created API key identifier from
create_api_key, and revoke it in the teardown using the existing revoke API
helper (or appropriate revoke function) so consumers like
tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py
can parametrize the fixture for free vs premium paths and no keys are left
active after tests.
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 190-213: The create API-key helper currently only sends
payload["subscription"] to api_keys_url but does not verify it was persisted;
update the function that builds payload (using api_key_name, expires_in,
subscription) to validate the stored subscription by parsing the create response
body (ensure it contains the created key id and stored subscription) or by
performing a follow-up GET to /v1/api-keys/{id} and comparing the returned
"subscription" field to the requested subscription; if raise_on_error is True
assert the values match (or raise AssertionError) and if False return the
response and parsed body so tests can inspect the mismatch.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 07d8592d-9573-40d5-a366-0ac364b3e638
📒 Files selected for processing (5)
tests/model_serving/maas_billing/conftest.pytests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.pytests/model_serving/maas_billing/maas_subscription/utils.py
tests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.py
Outdated
Show resolved
Hide resolved
SB159
force-pushed
the
test/run-maas-tests
branch
from
a95386b to
1ba2154
Compare
There was a problem hiding this comment.
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/maas_billing/maas_subscription/test_maas_sub_enforcement.py`:
- Line 42: Add a missing type annotation for the test fixture parameter: update
the test function signature to annotate maas_subscription_tinyllama_premium with
MaaSSubscription, and add the corresponding import for MaaSSubscription at the
top of the test file so the type is resolved; modify the function that
references maas_subscription_tinyllama_premium (e.g., the test in
test_maas_sub_enforcement.py) to use the annotated parameter.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d09cd165-946e-48d8-a75c-b5906fb73a3e
📒 Files selected for processing (4)
tests/model_serving/maas_billing/conftest.pytests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.pytests/model_serving/maas_billing/maas_subscription/utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_serving/maas_billing/conftest.py
- tests/model_serving/maas_billing/maas_subscription/utils.py
- tests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.py
tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py
Outdated
Show resolved
Hide resolved
…rror Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
test/run-maas-tests
branch
from
1ba2154 to
44f83de
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/conftest.py (1)
227-232: Teardown ignoresrevoke_api_keyfailures silently.The existing
three_active_api_key_idsfixture (lines 514-521) validates revoke responses and raises on unexpected status codes. These new fixtures discard the response entirely, masking cleanup failures (e.g., 500 from gateway, network timeouts) that could indicate environment instability.Add status checks consistent with the established pattern:
Proposed fix
yield body["key"] - revoke_api_key( + revoke_resp, _ = revoke_api_key( request_session_http=request_session_http, base_url=base_url, key_id=body["id"], ocp_user_token=ocp_token_for_actor, ) + if revoke_resp.status_code not in (200, 404): + LOGGER.warning( + f"Unexpected status {revoke_resp.status_code} revoking key id={body['id']}" + )Apply the same change to both
api_key_bound_to_free_subscription(lines 227-232) andapi_key_bound_to_premium_subscription(lines 256-261).Also applies to: 256-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/conftest.py` around lines 227 - 232, The teardown currently calls revoke_api_key and discards its result, hiding failures; update the fixtures api_key_bound_to_free_subscription and api_key_bound_to_premium_subscription to capture the response from revoke_api_key, verify the HTTP status (matching the pattern used in three_active_api_key_ids), and raise or assert if the status is not the expected success code so cleanup failures (500s, timeouts) surface; locate the revoke_api_key calls in those fixtures and add the same status check/raise logic used in three_active_api_key_ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 227-232: The teardown currently calls revoke_api_key and discards
its result, hiding failures; update the fixtures
api_key_bound_to_free_subscription and api_key_bound_to_premium_subscription to
capture the response from revoke_api_key, verify the HTTP status (matching the
pattern used in three_active_api_key_ids), and raise or assert if the status is
not the expected success code so cleanup failures (500s, timeouts) surface;
locate the revoke_api_key calls in those fixtures and add the same status
check/raise logic used in three_active_api_key_ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5115e710-8f66-4184-b614-9582a3c84fbe
📒 Files selected for processing (4)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.pytests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.pytests/model_serving/maas_billing/maas_subscription/utils.py
✅ Files skipped from review due to trivial changes (1)
- tests/model_serving/maas_billing/maas_subscription/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_serving/maas_billing/maas_subscription/test_maas_sub_enforcement.py
- tests/model_serving/maas_billing/maas_subscription/test_maas_auth_enforcement.py
|
Status of building tag latest: success. |
|
/cherry-pick 3.4ea2 |
|
Error cherry-picking. |
|
|
opendatahub-io#1303) * fix: update MaaS tests for subscription binding and fix gateway 500 error Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> * address review comment Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com> --------- Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
4 participants
…rror
Pull Request
Summary
update MaaS tests for subscription binding and fix gateway 500 error
Related Issues
Please review and indicate how it has been tested
Additional Requirements
Summary by CodeRabbit