test: Add test to verify model registry access for different users created from the run#283
test: Add test to verify model registry access for different users created from the run#283fege merged 36 commits intoopendatahub-io:mainfrom
Conversation
WalkthroughThe changes introduce a comprehensive RBAC (Role-Based Access Control) testing framework for the Model Registry component. New utilities manage ephemeral OpenShift test users and groups via HTPasswd IDPs, enable dynamic context switching, and provide helper functions for Model Registry client setup. Multiple test cases verify user and group permissions through direct and group-based role bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as RBAC Test
participant UserUtils as User Utils
participant OpenShift as OpenShift Cluster
participant MR as Model Registry
Test->>UserUtils: create_test_idp()
UserUtils->>OpenShift: Create htpasswd secret, add IDP
UserUtils->>OpenShift: Login as test user, create context
UserUtils-->>Test: Return UserTestSession
Test->>OpenShift: Create group and add user
Test->>OpenShift: Bind group/user to MR access role
Test->>MR: Attempt Model Registry access with user token
MR-->>Test: Allow or deny based on RBAC
Test->>UserUtils: cleanup_test_idp()
UserUtils->>OpenShift: Remove IDP, delete secret, cleanup context
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/wip |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/wip', '/lgtm'} |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (6)
tests/model_registry/utils.py (2)
161-184: Consider handling multiple services case consistentlyThe function correctly retrieves the model registry service, but it doesn't handle the case of multiple services in the same way as the existing
get_mr_service_by_labelfunction. For consistency, consider raisingTooManyServicesErrorif multiple services are found.services = list( Service.get( dyn_client=admin_client, namespace=namespace, label_selector="app=model-registry,component=model-registry" ) ) if not services: raise RuntimeError(f"Model Registry service not found in namespace {namespace}") + if len(services) > 1: + raise TooManyServicesError(services) return services[0]
161-184: Use more specific exception typeConsider using a more specific exception type like
ResourceNotFoundErrorinstead ofRuntimeErrorfor consistency with the rest of the codebase.services = list( Service.get( dyn_client=admin_client, namespace=namespace, label_selector="app=model-registry,component=model-registry" ) ) if not services: - raise RuntimeError(f"Model Registry service not found in namespace {namespace}") + raise ResourceNotFoundError(f"Model Registry service not found in namespace {namespace}") return services[0]tests/model_registry/test_user_permission.py (4)
67-68: Extract test users into constantsConsider extracting the hardcoded user names into constants at the module level for better maintainability and readability.
+# Define user constants at the module level +CLUSTER_ADMIN_USER = "htpasswd-cluster-admin-user" +NON_ADMIN_USER = "ldap-user1" + @pytest.mark.parametrize( "user_name, password, context_manager", [ - ("htpasswd-cluster-admin-user", os.environ.get("ADMIN_PASSWORD"), nullcontext()), - ("ldap-user1", os.environ.get("NON_ADMIN_PASSWORD"), pytest.raises(ForbiddenException)), + (CLUSTER_ADMIN_USER, os.environ.get("ADMIN_PASSWORD"), nullcontext()), + (NON_ADMIN_USER, os.environ.get("NON_ADMIN_PASSWORD"), pytest.raises(ForbiddenException)), ], )
84-84: Use better assertion formatFor more informative error messages, consider using a more descriptive assertion format with a message explaining what's being checked.
- assert model_registry_instance.name == MR_INSTANCE_NAME + assert model_registry_instance.name == MR_INSTANCE_NAME, f"Model registry instance name should be {MR_INSTANCE_NAME}, but got {model_registry_instance.name}"
90-96: Consider capturing and verifying the client instanceThe test only checks if client creation succeeds or fails, but doesn't perform any operations with the client to further verify proper access. Consider storing the created client and performing a basic operation to verify functionality.
with context_manager: - ModelRegistryClient( + client = ModelRegistryClient( server_address=f"https://{server}", port=int(port), author="opendatahub-test", user_token=user_token, is_secure=False, ) + if user_name == "htpasswd-cluster-admin-user": + # Verify admin can perform operations by making a basic API call + client.get_registered_models()
1-97: Add logging for test progress and diagnosticsThe test could benefit from additional logging to help diagnose issues during test runs, especially when working with external services and authentication.
Add logging at key points in the test:
def test_user_permission( self: Self, updated_dsc_component_state_scope_class, model_registry_instance, admin_client: DynamicClient, user_name, password, context_manager, ): + LOGGER.info(f"Testing model registry access for user: {user_name}") assert model_registry_instance.name == MR_INSTANCE_NAME user_token = get_token(user_name=user_name, password=password, admin_client=admin_client) + LOGGER.info(f"Successfully obtained token for user: {user_name}") svc = get_model_registry_service(admin_client=admin_client, namespace=TEST_NAMESPACE) server, port = get_endpoint_from_mr_service(svc, Protocols.REST).split(":") + LOGGER.info(f"Connecting to model registry at: https://{server}:{port}") with context_manager: ModelRegistryClient( server_address=f"https://{server}", port=int(port), author="opendatahub-test", user_token=user_token, is_secure=False, ) + LOGGER.info(f"User {user_name} access test completed successfully")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/test_user_permission.py(1 hunks)tests/model_registry/utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/model_registry/utils.py (1)
tests/conftest.py (1)
admin_client(50-51)
tests/model_registry/test_user_permission.py (4)
tests/model_registry/utils.py (2)
get_endpoint_from_mr_service(43-47)get_model_registry_service(161-184)utilities/constants.py (3)
DscComponents(146-164)Protocols(87-94)ManagementState(151-153)tests/conftest.py (1)
admin_client(50-51)tests/model_registry/conftest.py (2)
updated_dsc_component_state_scope_class(223-250)model_registry_instance(142-172)
|
@coderabbitat review |
|
Tip For best results, initiate chat on the files or code changes. @fege I noticed that you may have misspelled my username. The correct format is "@coderabbitai review" (without the "at" at the end). I see that you've already successfully triggered a review in a previous comment, so your PR should already be in the review queue. If you're trying to trigger another review, please use "@coderabbitai review" or "@coderabbitai full review" for a complete review from scratch. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_registry/test_user_permission.py (1)
24-26: Document and restrict TLS bypass to test environmentsWe're deliberately using
--insecure-skip-tls-verify=truewhich bypasses TLS verification.
🧹 Nitpick comments (4)
tests/model_registry/utils.py (1)
166-190: Consider consistent error handling for multiple servicesThe implementation correctly retrieves Model Registry services, but doesn't handle multiple matching services in the same way as
get_mr_service_by_label. If multiple services match the label selector, this function silently returns the first one without warning about the ambiguity.Consider applying a similar approach as in
get_mr_service_by_labelfor consistency:def get_model_registry_service(admin_client: DynamicClient, namespace: str) -> Service: """ Get the Model Registry service. Retrieves the Model Registry service from the specified namespace. Args: admin_client (DynamicClient): OCP Client to use. namespace (str): Namespace where the Model Registry service is located. Returns: Service: The Model Registry service. Raises: RuntimeError: if no Model Registry service is found. + TooManyServicesError: if more than one Model Registry service is found. """ services = list( Service.get( dyn_client=admin_client, namespace=namespace, label_selector="app=model-registry,component=model-registry" ) ) if not services: raise RuntimeError(f"Model Registry service not found in namespace {namespace}") + if len(services) > 1: + raise TooManyServicesError(services) return services[0]tests/model_registry/test_user_permission.py (3)
36-51: Consider more robust endpoint parsingThe current implementation assumes the endpoint format is always "server:port", which might not be robust in all environments.
Consider using a more resilient parsing approach:
def assert_mr_client(user_token, admin_client, context): """ Initiate MR client """ svc = get_model_registry_service(admin_client=admin_client, namespace=TEST_NAMESPACE) server, port = get_endpoint_from_mr_service(svc, Protocols.REST).split(":") + # Ensure endpoint is properly parsed even if it contains multiple colons (e.g., IPv6) + # Example format: "hostname:port" or "192.168.0.1:8080" + parts = get_endpoint_from_mr_service(svc, Protocols.REST).rsplit(":", 1) + if len(parts) == 2: + server, port = parts + else: + raise ValueError(f"Invalid endpoint format: {get_endpoint_from_mr_service(svc, Protocols.REST)}") with context: ModelRegistryClient( server_address=f"https://{server}", port=int(port), author="opendatahub-test", user_token=user_token, is_secure=False, )
128-131: Fix typo in log messageThere's a typo in the log message.
- LOGGER.info("User has no acces to MR") + LOGGER.info("User has no access to MR") - LOGGER.info("Add user hthe model registry users group") + LOGGER.info("Add user to the model registry users group")
136-137: Fix typo in log messageThere's a typo in the log message.
- LOGGER.info("User has acces to MR") + LOGGER.info("User has access to MR")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/test_user_permission.py(1 hunks)tests/model_registry/utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/model_registry/utils.py (1)
tests/conftest.py (1)
admin_client(50-51)
tests/model_registry/test_user_permission.py (4)
tests/model_registry/utils.py (2)
get_endpoint_from_mr_service(48-52)get_model_registry_service(166-189)utilities/constants.py (3)
DscComponents(146-164)Protocols(87-94)ManagementState(151-153)tests/conftest.py (1)
admin_client(50-51)tests/model_registry/conftest.py (2)
updated_dsc_component_state_scope_class(224-255)model_registry_instance(143-173)
opendatahub-tests-bot
added
commented-by-fege
and removed
commented-by-lugi0
commented-by-fege
labels
lugi0
left a comment
lugi0
left a comment
There was a problem hiding this comment.
few suggestions but looks good in general
dbasunag
left a comment
dbasunag
left a comment
There was a problem hiding this comment.
Couple of comments. Looks good otherwise. Thank you for addressing all the open comments.
rhods-ci-bot
removed
Verified
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_registry/rbac/group_utils.py (1)
36-44: LGTM: Clean implementation with proper resource lifecycle managementThe implementation correctly uses the Group resource as a context manager, handling both creation and cleanup automatically. The logging statements provide valuable visibility into the group lifecycle during test execution.
Consider adding basic error handling to log any exceptions that might occur during group creation, which could help with debugging test failures:
- with Group( - client=admin_client, - name=group_name, - users=users or [], - wait_for_resource=wait_for_resource, - ) as _: - LOGGER.info(f"Group {group_name} created successfully.") - yield group_name - LOGGER.info(f"Group {group_name} deletion initiated by context manager.") + try: + with Group( + client=admin_client, + name=group_name, + users=users or [], + wait_for_resource=wait_for_resource, + ) as _: + LOGGER.info(f"Group {group_name} created successfully.") + yield group_name + LOGGER.info(f"Group {group_name} deletion initiated by context manager.") + except Exception as e: + LOGGER.error(f"Error during group {group_name} lifecycle: {e}") + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/rbac/conftest.py(3 hunks)tests/model_registry/rbac/group_utils.py(1 hunks)tests/model_registry/rbac/test_mr_rbac.py(1 hunks)tests/model_registry/rbac/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_registry/rbac/utils.py
- tests/model_registry/rbac/conftest.py
- tests/model_registry/rbac/test_mr_rbac.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/rbac/group_utils.py (1)
tests/conftest.py (1)
admin_client(52-53)
🔇 Additional comments (5)
tests/model_registry/rbac/group_utils.py (5)
1-7: LGTM: Appropriate imports and logger setupThe imports are well-organized and include all necessary dependencies. Good practice to use a dedicated logger instance with the module name.
10-16: LGTM: Well-defined function signature with proper type annotationsThe context manager is correctly defined with appropriate type hints. The function signature clearly indicates what parameters are required and which are optional, with sensible defaults.
17-35: LGTM: Comprehensive and well-structured docstringThe docstring clearly explains the purpose, parameters, return value, and includes a practical usage example. This follows best practices for Python documentation.
39-39: LGTM: Defensive handling of optional users parameterGood use of
users or []to handle the case when no users are provided. This ensures a valid empty list is passed to the Group constructor.
14-14:Details
✅ Verification successful
Verify Python version compatibility for union type syntax
The use of
list[str] | Nonefor type annotation uses the union type operator (|) which is only supported in Python 3.10+. Ensure your project's minimum Python version requirement is compatible with this syntax, or consider usingOptional[List[str]]from thetypingmodule for broader compatibility.
🏁 Script executed:
#!/bin/bash # Check the project's Python version requirements grep -r "python_requires" --include="*.py" --include="setup.py" --include="pyproject.toml" . # Check if there are any Python version constraints in requirements files grep -r "^python" --include="requirements*.txt" .Length of output: 140
I didn’t find any
python_requiresinsetup.py/setup.cfgor arequires-python/Poetry entry inpyproject.toml. Let’s check a few more common places for Python version constraints:
🏁 Script executed:
#!/bin/bash # Check setup.cfg for python_requires grep -R "python_requires" --include=setup.cfg . # Check pyproject.toml for PEP 621 requires-python grep -R "requires-python" --include=pyproject.toml . # Check Poetry-style Python constraint in pyproject.toml grep -R "^\s*python\s*=" --include=pyproject.toml . # Check Pipfile for a Python version requirement grep -R "python_version" --include=Pipfile .Length of output: 245
No action needed—project targets Python 3.12 so
list[str] | Noneis supportedI verified that
pyproject.tomlincludes:requires-python = "==3.12.*"Since Python 3.12 ≥ 3.10, the use of PEP 604 union syntax is fully compatible.
|
Status of building tag latest: success. |
| mr_client = ModelRegistryClient(**client_args) | ||
| assert mr_client is not None, "Client initialization failed after granting permissions" |
There was a problem hiding this comment.
shouldn't be safer to check a return code or some specific error msg saying the initialization failed because of permissions?
6 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Tests