feat: Add privileged_client support to VirtualMachineInstance#2648
feat: Add privileged_client support to VirtualMachineInstance#2648
Conversation
Introduce explicit privileged_client parameter across VirtualMachineInstance methods to support proper RBAC-aware client usage. New public methods replacing deprecated properties: - get_virt_launcher_pod, get_virt_handler_pod, get_node, get_xml_dict Modified methods with privileged_client parameter: - pause, unpause, reset, get_xml, execute_virsh_command - get_dommemstat, get_domstate, wait_until_running, api_request New private helpers: - _get_pod_user_uid, _is_pod_root - _get_hypervisor_connection_uri, _build_virsh_cmd - _resolve_privileged_client, _get_subresource_api_url Warning strategy: - FutureWarning when privileged_client is omitted - DeprecationWarning on old property-based access
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactored Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ 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 |
…nto vm_priv_property
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
/verified |
![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
🤖 Fix all issues with AI agents
In `@ocp_resources/virtual_machine_instance.py`:
- Around line 574-596: _get_pod_user_uid is annotated as returning int but can
be None when spec.securityContext.runAsUser is unset, causing _is_pod_root to
misidentify pods as root; change _get_pod_user_uid's type hint to Optional[int]
(or at least allow None) and update _is_pod_root to explicitly check for UID ==
0 (return True only when _get_pod_user_uid(pod) == 0) and otherwise return
False, so absent runAsUser is treated as non-root; refer to
VirtualMachineInstance._get_pod_user_uid, VirtualMachineInstance._is_pod_root,
and Pod.instance.spec.securityContext.runAsUser when making the change.
- Around line 333-363: The current calls to api_request in pause/unpause/reset
always pass the resolved _client, causing api_request to always take the
privileged path; change those calls to pass the original privileged_client
parameter (not _client) so api_request can choose the non-privileged code path
when privileged_client is omitted (i.e., use privileged_client=privileged_client
in the api_request call inside pause, unpause, and reset). Optionally add a
one-line method comment on pause/unpause/reset documenting this deprecation
transition if you want to clarify intent.
🧹 Nitpick comments (1)
ocp_resources/virtual_machine_instance.py (1)
476-484: Considerself.logger.exception()for the error-path diagnostics (Ruff TRY400).Lines 477-479 use
self.logger.error()inside anexceptblock. While the exception is re-raised, usingself.logger.exception()(at least for the first call) would automatically attach the traceback to the log output, aiding debugging.
- Pass original privileged_client (not resolved _client) to api_request in pause/unpause/reset - Handle unset runAsUser in _get_pod_user_uid (return int | None) and _is_pod_root (explicit uid == 0 check) - Use self.logger.exception() for error-path diagnostics in wait_until_running
…nto vm_priv_property
|
/verifierd |
|
/verified |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ocp_resources/virtual_machine_instance.py (1)
470-484:⚠️ Potential issue | 🟡 Minor
wait_until_runningbypasses_resolve_privileged_client, breaking the deprecation contract.Every other method that accepts
privileged_clientroutes through_resolve_privileged_clientso callers get aFutureWarningwhen the argument is omitted. Here, the silentor self.clientfallback on line 476 skips that warning entirely, creating an inconsistency consumers may not notice until the breaking change lands.Suggested fix
def wait_until_running( self, timeout: int = TIMEOUT_4MINUTES, logs: bool = True, stop_status: str | None = None, privileged_client: DynamicClient | None = None, ) -> None: + _client = self._resolve_privileged_client( + privileged_client=privileged_client, + fallback_client=self.client, + method_name="wait_until_running", + ) ... try: self.wait_for_status(status=self.Status.RUNNING, timeout=timeout, stop_status=stop_status) except TimeoutExpiredError as sampler_ex: if not logs: raise try: - virt_pod = self.get_virt_launcher_pod(privileged_client=privileged_client or self.client) + virt_pod = self.get_virt_launcher_pod(privileged_client=_client)
🧹 Nitpick comments (3)
ocp_resources/virtual_machine_instance.py (3)
436-440:ResourceNotFoundErrorraised without a descriptive message.Line 394 provides a helpful message (
"VIRT launcher POD not found for ..."), but line 440 raises a bareResourceNotFoundError. Adding a similar message would help consumers debug issues.Suggested fix
- raise ResourceNotFoundError + raise ResourceNotFoundError(f"virt-handler pod not found on node {self.instance.status.nodeName}")
646-661: Redundant double-resolution ofprivileged_clientacross chained calls.
get_xmlresolves the client via_resolve_privileged_client(line 656) then passes_clienttoexecute_virsh_command(line 661), which internally calls_resolve_privileged_clientagain (line 823). The same pattern applies toget_dommemstat(line 747→752) andget_xml_dict(line 773→778→get_xml→execute_virsh_command).Because the resolved
_clientis always non-None, no double-warning is emitted, so this isn't a bug. However, the redundant resolution adds unnecessary overhead and cognitive noise. Consider having these "wrapper" methods pass_clientdirectly and haveexecute_virsh_commandskip re-resolution when the client is already provided, or document thatexecute_virsh_commandis also a standalone public entry point that needs its own resolution.
477-479: Ruff TRY400:logging.errorin an exception handler.Ruff flags lines 477-479 because they use
self.logger.error()inside anexceptblock. These are diagnostic messages (pod status, pod name, pod logs) rather than exception messages, sologging.exceptionisn't semantically ideal here either. You could suppress with a# noqa: TRY400if you want to keep the current style, or switch toself.logger.exception()for the first one (line 477) since it's in the context of a real error, and leave the rest as.error().
- Remove _resolve_privileged_client helper and FutureWarning mechanism - Use simple 'privileged_client or self.client' fallback pattern - Deprecated properties emit DeprecationWarning only - Fix _is_pod_root to treat unset runAsUser as root (Kubernetes default)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ocp_resources/virtual_machine_instance.py (2)
295-296: Inconsistent parameter naming:clientvsprivileged_client.
resetusesclientwhilepause(line 282) andunpause(line 289) useprivileged_client. For a consistent public API, consider renaming toprivileged_clienthere as well.♻️ Proposed fix
- def reset(self, client: DynamicClient | None = None) -> dict[str, Any]: - return self.api_request(method="PUT", action="reset", client=client or self.client) + def reset(self, privileged_client: DynamicClient | None = None) -> dict[str, Any]: + return self.api_request(method="PUT", action="reset", client=privileged_client or self.client)
509-520: Consider whetheruid is Noneshould truly be treated as root.The docstring explicitly documents this, but in Kubernetes, an unset
runAsUserdoesn't always mean root — it depends on the container image's default user and pod security policies. If this is intentional for the KubeVirt virt-launcher context, a brief inline comment explaining whyNone≈ root would help future readers.
rnetser
force-pushed
the
vm_priv_property
branch
from
0732212 to
ccffcc3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ocp_resources/virtual_machine_instance.py (3)
295-296: Inconsistent parameter name:clientvsprivileged_client.
resetusesclientwhilepause/unpauseand all other new methods useprivileged_client. This inconsistency in the public API could confuse callers about when to pass a privileged vs. regular client.Suggested fix
- def reset(self, client: DynamicClient | None = None) -> dict[str, Any]: - return self.api_request(method="PUT", action="reset", client=client or self.client) + def reset(self, privileged_client: DynamicClient | None = None) -> dict[str, Any]: + return self.api_request(method="PUT", action="reset", client=privileged_client or self.client)
686-701: Double pod lookup per virsh command execution.
execute_virsh_commandcallsget_virt_launcher_podon line 697 to get the pod, then callsvirsh_cmdon line 699, which internally callsget_virt_launcher_podagain (line 553). Each call results in a Kubernetes API list request, doubling the API overhead.Consider fetching the pod once and passing it through:
Suggested refactor
- def virsh_cmd(self, action: str, privileged_client: DynamicClient | None = None) -> list[str]: + def virsh_cmd(self, action: str, privileged_client: DynamicClient | None = None, pod: Pod | None = None) -> list[str]: ... - pod = self.get_virt_launcher_pod(privileged_client=privileged_client or self.client) + pod = pod or self.get_virt_launcher_pod(privileged_client=privileged_client or self.client) hypervisor_uri = self.get_hypervisor_connection_uri(pod=pod) return shlex.split(f"virsh {hypervisor_uri} {action} {self.namespace}_{self.name}") def execute_virsh_command(self, command: str, privileged_client: DynamicClient | None = None) -> str: ... _client = privileged_client or self.client pod = self.get_virt_launcher_pod(privileged_client=_client) return pod.execute( - command=self.virsh_cmd(action=command, privileged_client=_client), + command=self.virsh_cmd(action=command, privileged_client=_client, pod=pod), container="compute", )
365-365: Missing error message inResourceNotFoundError.
get_virt_launcher_pod(line 322) raises with a descriptive message, butget_virt_handler_podraises a bareResourceNotFoundError. This makes debugging harder when the error surfaces.Suggested fix
- raise ResourceNotFoundError + raise ResourceNotFoundError(f"VIRT handler POD not found for {self.kind}:{self.name}")
rnetser
force-pushed
the
vm_priv_property
branch
from
ccffcc3 to
85c659b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ocp_resources/virtual_machine_instance.py (3)
533-545:virsh_cmdandexecute_virsh_commandduplicate the command-construction logic.Both methods independently fetch the pod, resolve the hypervisor URI, and build the identical
shlex.split(f"virsh {hypervisor_uri} ... {self.namespace}_{self.name}")string.execute_virsh_commandcould delegate to a shared helper to avoid the duplication.A minimal refactor: extract a private
_build_virsh_contextthat returns the(pod, command_list)tuple, then have both public methods call it.Suggested refactor
+ def _build_virsh_context( + self, action: str, privileged_client: DynamicClient | None = None + ) -> tuple[Pod, list[str]]: + _client = privileged_client or self.client + pod = self.get_virt_launcher_pod(privileged_client=_client) + hypervisor_uri = self.get_hypervisor_connection_uri(pod=pod) + cmd = shlex.split(f"virsh {hypervisor_uri} {action} {self.namespace}_{self.name}") + return pod, cmd + def virsh_cmd(self, action: str, privileged_client: DynamicClient | None = None) -> list[str]: - pod = self.get_virt_launcher_pod(privileged_client=privileged_client or self.client) - hypervisor_uri = self.get_hypervisor_connection_uri(pod=pod) - return shlex.split(f"virsh {hypervisor_uri} {action} {self.namespace}_{self.name}") + _, cmd = self._build_virsh_context(action=action, privileged_client=privileged_client) + return cmd def execute_virsh_command(self, command: str, privileged_client: DynamicClient | None = None) -> str: - _client = privileged_client or self.client - pod = self.get_virt_launcher_pod(privileged_client=_client) - hypervisor_uri = self.get_hypervisor_connection_uri(pod=pod) - return pod.execute( - command=shlex.split(f"virsh {hypervisor_uri} {command} {self.namespace}_{self.name}"), - container="compute", - ) + pod, cmd = self._build_virsh_context(action=command, privileged_client=privileged_client) + return pod.execute(command=cmd, container="compute")Also applies to: 676-691
547-557: Redundantor self.clientfallback in wrapper methods.
get_xml,get_domstate,get_dommemstat, andget_xml_dicteach resolveprivileged_client or self.clientbefore passing the result toexecute_virsh_command/get_xml, which performs the same fallback internally. This means the inner fallback is never exercised when called through these wrappers.Consider passing
privileged_clientthrough as-is and letting the leaf method (execute_virsh_command) handle the single fallback, which is cleaner and avoids masking a genuinelyNoneintent:def get_xml(self, privileged_client: DynamicClient | None = None) -> str: - return self.execute_virsh_command(command="dumpxml", privileged_client=privileged_client or self.client) + return self.execute_virsh_command(command="dumpxml", privileged_client=privileged_client)Same pattern applies to
get_domstate,get_dommemstat, andget_xml_dict.Also applies to: 605-605, 618-618, 639-641
512-531:get_hypervisor_connection_uriexecutes a remotels— consider documenting the side effect.This static method runs
pod.execute(command=["ls", "/var/run/libvirt/"], ...)to probe for the socket type, which is a network call with latency and potential failure. Callers may not expect a "get URI" method to perform remote I/O. A brief docstring note about this would help.
rnetser
force-pushed
the
vm_priv_property
branch
from
85c659b to
cd9d51b
Compare
Replace all VirtualMachineForTests.privileged_vmi property usages with the new openshift-python-wrapper privileged_client API: - vm.privileged_vmi.xml_dict → vm.vmi.get_xml_dict(privileged_client=admin_client) - vm.privileged_vmi.virt_launcher_pod → vm.vmi.get_virt_launcher_pod(privileged_client=admin_client) - vm.privileged_vmi.node → vm.vmi.get_node(privileged_client=admin_client) - vm.privileged_vmi.virt_handler_pod → vm.vmi.get_virt_handler_pod(privileged_client=admin_client) - vm.privileged_vmi.execute_virsh_command() → vm.vmi.execute_virsh_command(privileged_client=admin_client) - vm.privileged_vmi.pause/unpause → vm.vmi.pause/unpause (no privileged_client needed) - vm.privileged_vmi.interfaces → vm.vmi.interfaces (no privileged_client needed) Remove privileged_vmi property and get_client import. Depends on: RedHatQE/openshift-python-wrapper#2648 Closes RedHatQE#3846
…nto vm_priv_property
…nto vm_priv_property
5 participants
Summary
Add explicit
privileged_clientparameter support acrossVirtualMachineInstancemethods to enable proper RBAC-aware client usage, replacing implicit reliance onself.clientfor privileged operations.New public methods (replacing deprecated properties)
get_virt_launcher_pod(privileged_client=...)— replacesvirt_launcher_podget_virt_handler_pod(privileged_client=...)— replacesvirt_handler_podget_node(privileged_client=...)— replacesnodeget_xml_dict(privileged_client=...)— replacesxml_dictModified methods with
privileged_clientparameterpause,unpause,resetget_xml,execute_virsh_commandget_dommemstat,get_domstatewait_until_runningapi_requestNew private helpers
_get_pod_user_uid— extract runAsUser UID from pod security context_is_pod_root— check if pod runs as root_get_hypervisor_connection_uri— build hypervisor socket URI_build_virsh_cmd— construct virsh command list_resolve_privileged_client— resolve client with FutureWarning fallback_get_subresource_api_url— build subresource API URL with optional clientDeprecation strategy
privileged_clientis omitted (falls back toself.client)virt_launcher_pod,virt_handler_pod,node,xml_dict, etc.)Consumer PR
Summary by CodeRabbit
New Features
Bug Fixes
Deprecations
get_*methods.