tests(maas-billing): add user creation for tests#843
tests(maas-billing): add user creation for tests#843mwaykole merged 12 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/lgtm', '/hold', '/cherry-pick', '/build-push-pr-image', '/verified'} |
SB159
force-pushed
the
feature/user-creation-tests
branch
from
4aa7802 to
50c9d40
Compare
SB159
requested review from
Raghul-M,
brettmthompson,
hdefazio and
threcc
as code owners
📝 WalkthroughWalkthroughAdds MaaS RBAC test scaffolding and e2e tests: new fixtures, constants, OAuth/htpasswd IDP lifecycle, per-user sessions and groups, token minting and header helpers, utilities to query /v1/models, and tests exercising /v1/models and /v1/chat/completions for admin/free/premium actors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
🧹 Nitpick comments (7)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py (1)
10-11: Solid E2E flow; just a couple of small polish opportunitiesThe overall actor-parametrized flow (mint token →
/v1/models→/v1/chat/completions) looks good and gives useful debug context via logs and sliced response bodies.Two minor nits:
CHAT_COMPLETIONS(Line 11) is unused in this module; consider dropping it to avoid linter noise.- For easier triage when one actor fails, consider including the actor kind in at least one assertion message or log (e.g., extend the mint or models failure message) so it’s obvious which role broke without digging into parametrization.
Also applies to: 31-91
tests/model_serving/model_server/maas_billing/utils.py (2)
142-179: Replace EN DASH in comment to satisfy Ruff and avoid odd charactersRuff is flagging the comment on Line 162 due to the
–EN DASH character:# Quick check first – if it's already Available=TrueSwitch that to a plain hyphen to keep the file ASCII-clean and silence RUF003:
# Quick check first - if it's already Available=True
181-203: Guard against emptyusersinput in htpasswd helper
make_bcrypt_htpasswd_file_with_usersassumesusersis non-empty and immediately doesusers[0]. If it’s ever called with an empty list, it will raiseIndexErrorin a non-obvious way.Consider an explicit guard to fail fast with a clearer error:
def make_bcrypt_htpasswd_file_with_users(users: list[tuple[str, str]]) -> Path: @@ - # First user: create (-c) - first_user, first_pass = users[0] + if not users: + raise ValueError("At least one (username, password) pair is required") + + # First user: create (-c) + first_user, first_pass = users[0]tests/model_serving/model_server/maas_billing/conftest.py (4)
109-176: API URL, original-user discovery, credentials, and htpasswd secret lifecycle look goodThe scaffolding to discover the API server URL, capture the original user, generate randomized FREE/PREMIUM credentials, and manage the htpasswd secret lifecycle is clear and well-contained. Deleting any existing secret first and cleaning up both the Secret and temp file in
finallyis exactly what we want for test hygiene.Only a minor optional tweak: you might want to pass
check=Trueto theoc delete secretcalls as well, to surface unexpected delete failures instead of silently continuing (the--ignore-not-found=trueflag already protects the common “not found” case).
178-225: Consider BYOIDC gating and silencing ARG001 lint inmaas_updated_oauth_configThe OAuth patch/restore flow with
ResourceEditorandwait_for_oauth_openshift_deployment()is solid and the “drop priormaas-*IDPs” step is a nice safety net.A few smaller points:
- On BYOIDC clusters, this fixture will still patch OAuth even though the user-session fixtures later call
pytest.skip. If you intend to fully avoid htpasswd-IDP tinkering on BYOIDC, consider injectingis_byoidchere and skipping early before any patching.admin_clientandmaas_created_htpasswd_secret_bothare intentionally unused but required to enforce fixture ordering. To keep Ruff happy (ARG001), you can either rename them to_admin_client/_maas_created_htpasswd_secret_bothor add# noqa: ARG001after their names.- Purely stylistic:
updated_providers = [combined_idp, *current_idps]is slightly cleaner than list concatenation and would address RUF005 if you care about that hint.
227-303: User-session fixtures are sound; a couple of flakiness and lint nitsThe FREE and PREMIUM
UserTestSessionfixtures are well-structured: they skip on BYOIDC, ensure the IDP is patched first, callwait_for_user_creation, restore the original login, and then clean up the user session infinally. Logging is informative and symmetric between free/premium.Two things to consider:
wait_for_user_creationperforms a single login attempt and raises on failure; depending on cluster behavior, that can be a bit brittle for newly created users. If you see flakiness around user readiness, you might want to introduce a small retry loop or reuselogin_with_retrysemantics here.maas_updated_oauth_configis included purely to enforce fixture ordering and is never referenced in the body, which triggers ARG001. Renaming the parameter to_maas_updated_oauth_config(and likewise_maas_updated_oauth_configin both fixtures) is a simple way to keep the dependency while silencing the lint.
339-375: Actor token fixture wiring looks correct; TRY003 suggestion is optional
ocp_token_for_actorcorrectly:
- Uses parametrization via
request.paramwith a sane default of"admin",- Short-circuits to the existing admin client/token,
- Switches logins for FREE/PREMIUM actors using
login_with_retry,- And reliably restores the original user afterwards.
The
ValueError(f"Unknown actor kind: {actor!r}")is fine in a test context; the TRY003 suggestion to move long messages into exception classes feels overkill here, so I’d keep it as-is unless your lint config enforces it strictly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_serving/model_server/maas_billing/conftest.py(2 hunks)tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py(1 hunks)tests/model_serving/model_server/maas_billing/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_serving/model_server/maas_billing/utils.py (3)
utilities/llmd_utils.py (1)
get_llm_inference_url(344-390)utilities/infra.py (1)
login_with_user_password(447-471)tests/conftest.py (1)
admin_client(76-77)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py (3)
utilities/plugins/constant.py (2)
RestHeader(10-11)OpenAIEnpoints(1-7)tests/model_serving/model_server/maas_billing/utils.py (1)
mint_token(81-98)tests/model_serving/model_server/maas_billing/conftest.py (4)
request_session_http(40-45)base_url(66-69)model_url(73-80)ocp_token_for_actor(340-375)
tests/model_serving/model_server/maas_billing/conftest.py (5)
utilities/general.py (1)
generate_random_name(319-344)utilities/user_utils.py (3)
UserTestSession(21-43)wait_for_user_creation(76-93)cleanup(39-43)utilities/infra.py (2)
login_with_user_password(447-471)get_openshift_token(603-617)tests/model_serving/model_server/maas_billing/utils.py (4)
create_maas_group(124-139)wait_for_oauth_openshift_deployment(142-178)make_bcrypt_htpasswd_file_with_users(181-202)login_with_retry(205-241)tests/conftest.py (2)
admin_client(76-77)is_byoidc(386-390)
🪛 Ruff (0.14.4)
tests/model_serving/model_server/maas_billing/utils.py
162-162: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
222-222: Consider moving this statement to an else block
(TRY300)
tests/model_serving/model_server/maas_billing/conftest.py
180-180: Unused function argument: admin_client
(ARG001)
182-182: Unused function argument: maas_created_htpasswd_secret_both
(ARG001)
202-202: Consider [combined_idp, *current_idps] instead of concatenation
Replace with [combined_idp, *current_idps]
(RUF005)
233-233: Unused function argument: maas_updated_oauth_config
(ARG001)
272-272: Unused function argument: maas_updated_oauth_config
(ARG001)
366-366: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
tests/model_serving/model_server/maas_billing/conftest.py (1)
305-337: Group fixtures usingcreate_maas_grouplook cleanThe
maas_free_groupandmaas_premium_groupfixtures nicely wrapcreate_maas_group, ensuring groups are created with the right users and cleaned up automatically at session end. Logging includes both group name and user, which will help during debugging.No changes needed here from my side.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_serving/model_server/maas_billing/utils.py (3)
utilities/llmd_utils.py (1)
get_llm_inference_url(344-390)utilities/infra.py (1)
login_with_user_password(447-471)tests/conftest.py (1)
admin_client(76-77)
🪛 Ruff (0.14.4)
tests/model_serving/model_server/maas_billing/utils.py
162-162: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
229-229: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (4)
tests/model_serving/model_server/maas_billing/utils.py (4)
1-24: LGTM!The new imports are appropriate and necessary for the OpenShift RBAC testing utilities introduced in this file.
123-140: LGTM!The context manager correctly creates and yields an OpenShift Group, ensuring cleanup on exit. The use of
wait_for_resource=Trueensures the group is fully created before the test proceeds.
142-179: Well-structured OAuth deployment wait logic.The function appropriately handles the common case (already Available) before entering a wait loop, and the 5-minute timeout is reasonable for OAuth rollouts.
221-229: Good fix: boolean return value is now properly checked.The implementation correctly captures the boolean return from
login_with_user_passwordand treatsFalseas a failure, addressing the previous review comment.Based on past review comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_serving/model_server/maas_billing/utils.py (1)
162-162: Fix EN DASH character in comment.The comment contains an EN DASH (–) instead of a HYPHEN-MINUS (-), which can cause encoding issues.
Apply this diff:
- # Quick check first – if it's already Available=True + # Quick check first - if it's already Available=TrueBased on coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_serving/model_server/maas_billing/utils.py (2)
utilities/infra.py (1)
login_with_user_password(447-471)tests/conftest.py (1)
admin_client(76-77)
🪛 Ruff (0.14.4)
tests/model_serving/model_server/maas_billing/utils.py
162-162: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
187-187: Avoid specifying long messages outside the exception class
(TRY003)
229-229: Consider moving this statement to an else block
(TRY300)
230-230: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
tests/model_serving/model_server/maas_billing/utils.py (3)
1-24: LGTM!The imports are well-organized and necessary for the new MaaS RBAC utilities. Logger initialization follows best practices.
123-139: LGTM!The context manager pattern is correctly implemented. Automatic cleanup on exit and logging provide good operational visibility for test scaffolding.
181-205: LGTM!The function correctly handles multiple users with proper bcrypt hashing. Empty list validation prevents runtime errors, and the docstring appropriately documents the caller's cleanup responsibility. The use of
shlex.splitensures command injection safety.
|
\lgtm |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/model_serving/model_server/maas_billing/conftest.py (4)
187-187: Consider iterable unpacking for better readability.Ruff suggests using iterable unpacking instead of list concatenation.
Apply this diff:
- updated_identity_providers = existing_identity_providers + [maas_identity_provider] + updated_identity_providers = [*existing_identity_providers, maas_identity_provider]
252-253: Redundant BYOIDC check (optional simplification).The
is_byoidccheck here is now redundant sincemaas_rbac_idp_env(a dependency) already skips whenis_byoidcis True. Pytest will never execute this fixture on BYOIDC clusters. However, this defensive check is harmless and may improve code clarity.
294-295: Redundant BYOIDC check (optional simplification).Same as
maas_free_user_session: thisis_byoidccheck is redundant sincemaas_rbac_idp_envalready handles it. The check is harmless and may be kept for clarity.
389-389: Optional: Extract exception message to constant (nitpick).Ruff suggests avoiding inline long messages in exceptions for better maintainability, though this is a very minor style issue.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/conftest.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_serving/model_server/maas_billing/conftest.py (6)
utilities/plugins/constant.py (2)
RestHeader(10-11)OpenAIEnpoints(1-7)utilities/general.py (2)
generate_random_name(320-345)wait_for_oauth_openshift_deployment(473-491)utilities/infra.py (2)
login_with_user_password(449-473)get_openshift_token(605-619)tests/model_serving/model_server/maas_billing/utils.py (1)
detect_scheme_via_llmisvc(53-66)tests/conftest.py (3)
admin_client(78-79)is_byoidc(387-391)original_user(788-791)tests/model_registry/conftest.py (1)
api_server_url(537-542)
🪛 Gitleaks (8.29.0)
tests/model_serving/model_server/maas_billing/conftest.py
[high] 163-163: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff (0.14.5)
tests/model_serving/model_server/maas_billing/conftest.py
187-187: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
389-389: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (13)
tests/model_serving/model_server/maas_billing/conftest.py (13)
3-18: LGTM!The imports are well-organized and all are used by the new MaaS RBAC fixtures.
34-35: LGTM!Good practice to define group names as constants for consistency across fixtures.
108-114: LGTM!Standard pattern for obtaining the cluster API server URL via the Infrastructure resource.
117-132: LGTM!Good use of randomized credentials to prevent conflicts across test runs, with clear naming conventions.
148-149: LGTM!The
is_byoidccheck at the fixture entry point correctly prevents any OAuth or Secret modifications on OIDC clusters, addressing the previous review feedback.
195-226: LGTM!Excellent use of context managers (
SecretandResourceEditor) to ensure proper resource lifecycle management. The teardown logic gracefully handles timeout scenarios and includes comprehensive verification that the temporary IDP is removed.
262-284: LGTM!Proper user creation flow with wait verification, session management, and cleanup in the finally block.
304-326: LGTM!Consistent implementation with
maas_free_user_session, including proper wait verification and cleanup.
329-342: LGTM!Clean use of the
create_maas_groupcontext manager for proper group lifecycle management.
344-357: LGTM!Consistent implementation with
maas_free_group, properly managing the premium-tier group lifecycle.
359-409: LGTM!The actor-switching logic is correct, properly handling admin/free/premium users with appropriate login/token retrieval and guaranteed restoration of the original user context in the finally block.
411-436: LGTM!Straightforward MaaS token minting with proper error checking and logging.
168-172: The htpasswd concatenation logic is correct; no changes needed.Based on verification:
Apache htpasswd format: The standard
htpasswdcommand outputs entries asusername:hashedpasswordwithout trailing newlines, which matches the assumption in the code.Concatenation is valid: Combining
free_bytes + b"\n" + premium_bytesproduces the correct multi-entry htpasswd format when both entries lack trailing newlines.Verified by downstream usage: The fixture
maas_rbac_idp_envfeeds this concatenated htpasswd to downstream fixtures (maas_free_user_session,maas_premium_user_session) that authenticate users viawait_for_user_creation(). If concatenation was malformed, these would fail. No evidence of such failures exists.No fragility observed: The implementation correctly handles the standard htpasswd output format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py (2)
31-37: Useocp_token_for_actorto satisfy Ruff and assert OCP token shape
ocp_token_for_actoris currently unused, triggering Ruff ARG002, even though the fixture is important. You can make the intent explicit and keep lint clean by asserting it is a non-empty string:def test_mint_token_for_actors( self, ocp_token_for_actor, maas_token_for_actor: str, ) -> None: - LOGGER.info(f"MaaS RBAC: using already minted MaaS token length={len(maas_token_for_actor)}") + assert isinstance(ocp_token_for_actor, str) and ocp_token_for_actor, "empty OCP token for actor" + LOGGER.info(f"MaaS RBAC: using already minted MaaS token length={len(maas_token_for_actor)}")This keeps the fixture meaningful at the test level and fixes the ARG002 warning.
67-80: Confirm payload format for MaaS/v1/chat/completions—promptvsmessagesThe test is sending
payload = {"model": model_id, "prompt": "Hello", "max_tokens": 16}to endpoint/v1/chat/completions.OpenAI-compatible
/v1/chat/completionsendpoints requiremessages(an array of message objects withroleandcontentfields). The payload usespromptinstead, which is the field for completions (non-chat) endpoints.This same pattern appears in both
test_maas_rbac_e2e.py(line 72) andtest_maas_endpoints.py(line 40), and the response handling expects standard OpenAI response format (choices[0].message).Action: Verify whether MaaS has a custom API contract that accepts
promptfor chat/completions, or if the payload should be corrected to usemessages=[{"role": "user", "content": "Hello"}].tests/model_serving/model_server/maas_billing/conftest.py (3)
132-166: Adjustmaas_htpasswd_filestype hints to reflectPathobjects
create_htpasswd_filereturns a filesystem path that you later call.unlink()on, so the first and third tuple elements arePath-like, notstr. Updating the annotation will make this clearer to readers and type-checkers:-from typing import Generator +from typing import Generator +from pathlib import Path ... -@pytest.fixture(scope="session") -def maas_htpasswd_files( - maas_user_credentials_both: dict[str, str], -) -> Generator[tuple[str, str, str, str], None, None]: +@pytest.fixture(scope="session") +def maas_htpasswd_files( + maas_user_credentials_both: dict[str, str], +) -> Generator[tuple[Path, str, Path, str], None, None]:The rest of the implementation (including
.unlink(missing_ok=True)) then matches the annotation.
249-255: Make side-effect dependency onmaas_htpasswd_oauth_idpexplicit
maas_rbac_idp_envintentionally doesn’t usemaas_htpasswd_oauth_idpdirectly, relying on its side effects (Secret + OAuth IDP lifecycle). This shows up as an unused-arg warning.You can make that intent explicit and silence ARG001 by binding it to
_:@pytest.fixture(scope="session") def maas_rbac_idp_env( maas_htpasswd_oauth_idp, maas_user_credentials_both: dict[str, str], ): - - return maas_user_credentials_both + # Ensure MaaS htpasswd Secret and OAuth IDP are set up before using credentials + _ = maas_htpasswd_oauth_idp + return maas_user_credentials_both
423-448: Alignmaas_token_for_actordocstring with its actual scope/behaviorThe docstring says:
“Mint a MaaS token once per actor … and reuse it across all tests in the class instance.”
But the fixture has default
functionscope and depends onocp_token_for_actor(also function-scoped), so a fresh token is minted per test per actor.Either:
- Update the docstring to something like “Mint a MaaS token for the current actor per test”, or
- If you really want per-actor reuse, you’d need to rework the fixture graph to use a broader scope (e.g., class/session) consistently for both
ocp_token_for_actorandmaas_token_for_actor.Given the complexity of widening scopes around login state, just fixing the docstring is likely the safest, low-risk change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_serving/model_server/maas_billing/conftest.py(3 hunks)tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py(1 hunks)tests/model_serving/model_server/maas_billing/utils.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_serving/model_server/maas_billing/utils.py (3)
utilities/plugins/constant.py (2)
RestHeader(10-11)OpenAIEnpoints(1-7)tests/conftest.py (1)
admin_client(78-79)tests/model_serving/model_server/maas_billing/conftest.py (1)
base_url(64-67)
🪛 Gitleaks (8.29.0)
tests/model_serving/model_server/maas_billing/conftest.py
[high] 151-151: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff (0.14.5)
tests/model_serving/model_server/maas_billing/conftest.py
214-214: Consider [*existing_idps, maas_idp] instead of concatenation
Replace with [*existing_idps, maas_idp]
(RUF005)
251-251: Unused function argument: maas_htpasswd_oauth_idp
(ARG001)
401-401: Avoid specifying long messages outside the exception class
(TRY003)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py
33-33: Unused method argument: ocp_token_for_actor
(ARG002)
🔇 Additional comments (1)
tests/model_serving/model_server/maas_billing/utils.py (1)
118-154: New MaaS helpers look correct and improve reuse
create_maas_group,build_maas_headers, andget_maas_models_responseencapsulate common MaaS RBAC plumbing cleanly (group lifecycle, standard headers,/v1/modelscalls) and are used consistently from the fixtures. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/model_serving/model_server/maas_billing/conftest.py (7)
132-165: Type hint mismatch for file paths.The return type annotation says
tuple[str, str, str, str], butcreate_htpasswd_filelikely returnsPathobjects (given the.unlink()call on lines 164-165). Consider updating the type hint:@pytest.fixture(scope="session") def maas_htpasswd_files( maas_user_credentials_both: dict[str, str], -) -> Generator[tuple[str, str, str, str], None, None]: +) -> Generator[tuple[Path, str, Path, str], None, None]:And add
from pathlib import Pathto the imports.
214-214: Prefer iterable unpacking over list concatenation.Per static analysis (RUF005), using iterable unpacking is more idiomatic Python:
- updated_idps = existing_idps + [maas_idp] + updated_idps = [*existing_idps, maas_idp]
265-266: Redundantis_byoidccheck.This skip is redundant since
maas_rbac_idp_env(a transitive dependency via fixture chain) already performs the BYOIDC skip. The code will never reach this point on BYOIDC clusters. Consider removing for clarity:- if is_byoidc: - pytest.skip("Working on OIDC support for tests that use htpasswd IDP for MaaS") - else: - username = maas_rbac_idp_env["free_user"] + username = maas_rbac_idp_env["free_user"]Then dedent the remaining code block accordingly.
303-345: Consider consolidating duplicate user session logic.
maas_free_user_sessionandmaas_premium_user_sessionshare ~90% identical code. A parameterized helper function could reduce duplication:def _create_user_session( user_type: str, credentials: dict[str, str], original_user: str, api_server_url: str, ) -> Generator[UserTestSession, None, None]: # Shared logic here ...However, keeping them separate may be intentional for clarity in test fixture dependencies.
406-407: Consider a more descriptive exception or accept the inline message.Static analysis (TRY003) flags the long message outside exception class, but for test code, this inline message is clear and acceptable. If you prefer consistency, you could define a custom exception:
The current implementation is fine for test code - this is a minor style preference.
429-454: Docstring/scope mismatch.The docstring says "reuse it across all tests in the class instance," but the fixture has no explicit scope (defaults to
function), meaning a new token is minted per test. Either:
- Add
scope="class"if reuse is intended:-@pytest.fixture +@pytest.fixture(scope="class") def maas_token_for_actor(
- Or update the docstring to reflect per-test behavior.
463-474: Consider adding return type hint.The
maas_models_response_for_actorfixture lacks a return type annotation. Consider adding it for clarity:@pytest.fixture def maas_models_response_for_actor( request_session_http: requests.Session, base_url: str, maas_headers_for_actor: dict, -): +) -> requests.Response:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/maas_billing/conftest.py(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.29.0)
tests/model_serving/model_server/maas_billing/conftest.py
[high] 151-151: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff (0.14.5)
tests/model_serving/model_server/maas_billing/conftest.py
214-214: Consider [*existing_idps, maas_idp] instead of concatenation
Replace with [*existing_idps, maas_idp]
(RUF005)
251-251: Unused function argument: maas_htpasswd_oauth_idp
(ARG001)
407-407: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
tests/model_serving/model_server/maas_billing/conftest.py (8)
1-35: Imports and constants look good.Module-level constants for group names and OpenAI endpoints are appropriately scoped. The imports are well-organized for the fixture dependencies.
37-43: LGTM - session management is correct.The fixture properly yields the session and closes it in teardown. Note that
verify=Falsedisables SSL verification, which is acceptable for internal test environments.
46-60: Token minting fixture is well-structured.Good validation of response status and token format. The 30-minute token lifetime is appropriate for test duration.
63-79: URL fixtures are correct.The URL construction logic is sound. Note the scope difference:
base_urlis module-scoped whilemodel_urlis session-scoped - ensure this is intentional based on test requirements.
81-102: Header and models fixtures are clean.Good use of utility functions for building headers and fetching models. Assertions provide clear failure messages.
105-130: API server URL and credential fixtures are well-designed.Good use of randomized suffixes to prevent test collisions. The credential dictionary structure is clear and comprehensive.
Note: The Gitleaks warning on line 151 is a false positive - these are dynamically generated test credentials, not exposed secrets.
249-256: Dependency injection pattern is correct.The
maas_htpasswd_oauth_idpparameter appears unused (ARG001), but this is intentional - it ensures the OAuth IDP is configured before returning credentials. This is a valid pytest pattern for fixture ordering.
348-375: Group fixtures are clean and well-structured.Good use of context managers for automatic teardown. The logging provides clear traceability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py (3)
1-15: MODELS_INFO/CHAT_COMPLETIONS constants are currently unused in this moduleRight now this file relies on
model_urland other fixtures for URLs, soMODELS_INFOandCHAT_COMPLETIONSdefined here are unused. You could either remove them or wire them into future assertions to keep the module free of dead code.
31-37: Remove or underscore the unusedocp_token_for_actorargumentIn
test_mint_token_for_actors,ocp_token_for_actoris not used directly; the test only logs the already‑minted MaaS token. Sincemaas_token_for_actoralready depends onocp_token_for_actor, you can safely drop this parameter (or rename it to_ocp_token_for_actorand reference it once) to avoid the Ruff ARG002 warning.For example:
- def test_mint_token_for_actors( - self, - ocp_token_for_actor, - maas_token_for_actor: str, - ) -> None: + def test_mint_token_for_actors( + self, + maas_token_for_actor: str, + ) -> None:
47-83: Chat completions test exercises key happy-path behaviorThe flow of reusing the models fixture, taking the first
id, POSTing to/v1/chat/completions, and assertingstatus_code == 200plus non‑emptychoicesis clear and matches the intended E2E coverage. If you want stronger guarantees later, you could also validate shape ofchoices[0](e.g., presence ofmessage/content), but it’s not required for this PR.tests/model_serving/model_server/maas_billing/conftest.py (3)
166-237: OAuth/Secret IDP fixture has solid gating and teardown; minor style nitThe
maas_htpasswd_oauth_idpfixture cleanly:
- Skips entirely on BYOIDC clusters.
- Combines htpasswd entries, creates the Secret, and patches
oauth/clusterviaResourceEditor.- Waits for
oauth-openshiftrollout both after patch and after teardown.That covers the main correctness and cleanup concerns for the temporary MaaS HTPasswd IDP. If you care about Ruff RUF005, you could optionally switch
updated_idps = existing_idps + [maas_idp]to
updated_idps = [*existing_idps, maas_idp]but that’s purely stylistic.
239-246: Clarify intentionally unusedmaas_htpasswd_oauth_idpdependency
maas_rbac_idp_envtakesmaas_htpasswd_oauth_idponly for its side effects and doesn’t use the value, which is fine, but triggers Ruff ARG001.You can make that intent explicit and silence the warning by underscoring the parameter:
- def maas_rbac_idp_env( - maas_htpasswd_oauth_idp, - maas_user_credentials_both: dict[str, str], -): + def maas_rbac_idp_env( + _maas_htpasswd_oauth_idp, + maas_user_credentials_both: dict[str, str], +):Behavior stays the same.
415-441: Alignmaas_token_for_actorfixture scope with its docstringThe docstring says:
Mint a MaaS token once per actor (admin / free / premium) and reuse it across all tests in the class instance.
But the fixture currently uses the default function scope, so it will mint a new token for each test invocation per actor. To match the documented behavior (and the way
minted_tokenworks), consider making it a class‑scope fixture:-@pytest.fixture +@pytest.fixture(scope="class") def maas_token_for_actor( request_session_http: requests.Session, base_url: str, ocp_token_for_actor: str, ) -> str:That will mint one token per actor per test class, and the existing usages will continue to work unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_serving/model_server/maas_billing/conftest.py(3 hunks)tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py(1 hunks)tests/model_serving/model_server/maas_billing/utils.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_serving/model_server/maas_billing/utils.py (2)
utilities/plugins/constant.py (2)
RestHeader(10-11)OpenAIEnpoints(1-7)tests/model_serving/model_server/maas_billing/conftest.py (1)
base_url(64-67)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py (2)
utilities/plugins/constant.py (1)
OpenAIEnpoints(1-7)tests/model_serving/model_server/maas_billing/conftest.py (6)
ocp_token_for_actor(369-412)maas_token_for_actor(416-440)maas_models_response_for_actor(450-460)request_session_http(38-43)model_url(71-78)maas_headers_for_actor(444-446)
tests/model_serving/model_server/maas_billing/conftest.py (6)
utilities/plugins/constant.py (1)
OpenAIEnpoints(1-7)utilities/general.py (1)
generate_random_name(320-345)utilities/user_utils.py (4)
UserTestSession(20-43)wait_for_user_creation(76-94)create_htpasswd_file(46-68)cleanup(39-43)utilities/infra.py (2)
login_with_user_password(449-473)get_openshift_token(605-619)tests/model_serving/model_server/maas_billing/utils.py (7)
detect_scheme_via_llmisvc(55-68)host_from_ingress_domain(22-27)mint_token(76-93)llmis_name(102-115)create_maas_group(119-134)build_maas_headers(137-142)get_maas_models_response(145-162)tests/conftest.py (3)
admin_client(78-79)is_byoidc(387-391)original_user(788-791)
🪛 Gitleaks (8.29.0)
tests/model_serving/model_server/maas_billing/conftest.py
[high] 149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff (0.14.5)
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py
33-33: Unused method argument: ocp_token_for_actor
(ARG002)
tests/model_serving/model_server/maas_billing/conftest.py
212-212: Consider [*existing_idps, maas_idp] instead of concatenation
Replace with [*existing_idps, maas_idp]
(RUF005)
241-241: Unused function argument: maas_htpasswd_oauth_idp
(ARG001)
393-393: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (14)
tests/model_serving/model_server/maas_billing/utils.py (3)
12-20: Logging, constants, and maas_auth_headers docstring look consistentModule‑level
LOGGER/MODELS_INFOand the clarifiedmaas_auth_headersdocstring are straightforward and match how the rest of the MaaS utilities are structured. No issues here.Also applies to: 71-73
118-135: create_maas_group context manager is a clean abstractionUsing the
Groupcontext manager withwait_for_resource=Trueand yielding the resource gives you automatic teardown and keeps the fixtures inconftest.pysimple. This is a good encapsulation for the RBAC test setup.
137-143: Header builder and /v1/models helper are well-factored
build_maas_headerscentralizes the MaaS Authorization + common headers, andget_maas_models_responsewraps/v1/modelswith logging and a strict200assert, which is appropriate for tests. This aligns with how the fixtures reuse them and avoids duplicated HTTP logic.Also applies to: 145-162
tests/model_serving/model_server/maas_billing/test_maas_rbac_e2e.py (2)
17-29: Class-level parametrization and group fixtures are wired correctlyUsing class‑level
parametrizeoverACTORSplusmaas_free_group/maas_premium_groupfixtures gives you a clear matrix over admin/free/premium behavior with minimal boilerplate in each test. The docstring accurately describes the flows covered.
38-46: /v1/models visibility test is minimal but sufficientReusing
maas_models_response_for_actorand asserting thatdatais a non‑empty list keeps this test focused on “can this actor see any models” without duplicating HTTP logic. Looks good as a first‑pass RBAC check.tests/model_serving/model_server/maas_billing/conftest.py (9)
46-61: Minted token fixture logging change is fineThe updated f-string logging in
minted_tokenkeeps behavior the same while improving readability. Status checks and token validation still look correct for reuse across a test class.
81-101: maas_headers and maas_models correctly reuse shared helpersSwitching
maas_headerstobuild_maas_headersandmaas_modelstoget_maas_models_responseremoves duplicated HTTP/header logic and centralizes the/v1/modelsstatus check. The additionalmodelsnon‑empty assert inmaas_modelsis appropriate for tests.
103-127: API server URL + randomized MaaS user/IDP naming look good
maas_api_server_urlusingInfrastructure(..., name="cluster")andstatus.apiServerURLis a reasonable way to derive the login URL. The randomized usernames/passwords/IDP/Secret inmaas_user_credentials_bothviagenerate_random_name()should avoid collisions across runs while still being readable.
130-164: htpasswd temp-file lifecycle is handled correctly
maas_htpasswd_filescreates per-user htpasswd files for FREE and PREMIUM users and ensures they are unlinked in thefinallyblock. Returning both paths and base64 contents gives flexibility to fixtures that either need the file or the encoded payload.
248-291: FREE user session fixture correctly waits, restores, and cleans up
maas_free_user_sessiondoes the right things: waits for the user to become usable, restores the original login, encapsulates state inUserTestSession, and callscleanup()in afinallyblock. The BYOIDC skip at the top also ensures this path isn’t exercised on incompatible clusters.
293-335: PREMIUM user session mirrors FREE session behavior
maas_premium_user_sessionmirrors the FREE path with the same wait/restore/cleanup pattern, which keeps the behavior consistent across tiers. No issues from a correctness or teardown perspective.
338-365: Group fixtures correctly encapsulate RBAC groups per tier
maas_free_groupandmaas_premium_groupusecreate_maas_groupwith the appropriate constants and users, and yield just the group name. This keeps group creation/cleanup centralized and avoids leaving extra groups behind.
368-413: Actor-specific OCP token fixture behavior is sound
ocp_token_for_actorcleanly:
- Falls back to
"admin"when no param is provided.- Uses the dynamic client for admin tokens.
- Logs in as free/premium via
login_with_user_password, asserts success, yields a token from the current context, and logs back in as the original user with an assertion.This provides a clear abstraction per actor while restoring the original identity afterwards.
443-460: Per-actor headers and models response fixtures are clean wrappers
maas_headers_for_actorandmaas_models_response_for_actornicely layer on top ofbuild_maas_headersandget_maas_models_response, keeping tests focused on behavior rather than plumbing. The docstring onmaas_models_response_for_actorclarifies that it returns a validated response, which matches its implementation.
rhods-ci-bot
added
lgtm-by-dbasunag
and removed
commented-by-dbasunag
changes-requested-by-dbasunag
labels
|
Status of building tag latest: success. |
* User creation for e2e test * Addressed review comments * Updated utils * Updated helpers * Address review comments for MaaS RBAC tests * maas-billing: updated user creation process * maas-billing: updated conftest * maas-billing: refactor RBAC fixtures * maas-billing: address review comment remaining) * maas-billing: address review comments --------- Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
* User creation for e2e test * Addressed review comments * Updated utils * Updated helpers * Address review comments for MaaS RBAC tests * maas-billing: updated user creation process * maas-billing: updated conftest * maas-billing: refactor RBAC fixtures * maas-billing: address review comment remaining) * maas-billing: address review comments --------- Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
4 participants
Description:
This PR adds temporary FREE and PREMIUM htpasswd users, groups, and the required OAuth update.
These will be used for the current end-to-end RBAC tests and future rate-limit, quota, and other MaaS Billing tests.
Changes:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.