WIP: CNV-72446: use RBAC for VM bulk actions #3216
Conversation
|
@adamviktora: This pull request references CNV-72446 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adamviktora The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis PR introduces bulk RBAC access review functionality by adding helper utilities to aggregate resources and compute bulk access reviews, then applies these to multiple bulk virtual machine actions including delete, pause, restart, start, stop, and unpause operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
|
@adamviktora: This pull request references CNV-72446 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/virtualmachines/actions/BulkVirtualMachineActionFactory.tsx (1)
146-167: Update BulkVirtualMachineActionFactory pause action accessReview to use 'update' verb with 'pause' subresourceThe review comment is correct. The pause action in
BulkVirtualMachineActionFactory.tsx(line 151) uses an inconsistent RBAC configuration:
- Current (line 151):
asBulkAccessReview(VirtualMachineInstanceSubresourcesModel, vms, 'patch')- Expected:
asBulkAccessReview(VirtualMachineInstanceSubresourcesModel, vms, 'update', 'pause')The single VM pause action in
VirtualMachineActionFactory.tsx(line 305) already uses the correct pattern:asAccessReview(VirtualMachineInstanceSubresourcesModel, vm, 'update', 'pause'). The bulk pause action should mirror this, and it should be aligned with the unpause pattern in the same file (lines 221-226), which correctly uses'update'with'unpause'subresource.This mismatch will cause RBAC permission checks to fail or pass incorrectly since the pause operation will be checked against the wrong permission verb.
🧹 Nitpick comments (2)
src/utils/resources/shared.ts (1)
219-236: Clarify multi‑cluster / multi‑namespace semantics inasBulkAccessReviewThe overall shape of
asBulkAccessReviewlooks good and matches how bulk actions are used, but there is a subtle multi‑cluster edge case:
clusteris only set whenhaveSameCluster(resources)is true.namespaceis set wheneverhaveSameNamespace(resources)is true, even if the resources span different clusters but share the same namespace name.Depending on how
FleetAccessReviewResourceAttributesis interpreted, havingnamespaceset whileclusterisundefinedfor a multi‑cluster selection might be ambiguous or looser than intended. You may want to require both same cluster and same namespace before settingnamespace:return { - cluster: haveSameCluster(resources) ? getCluster(resources?.[0]) : undefined, + cluster: haveSameCluster(resources) ? getCluster(resources?.[0]) : undefined, group: model.apiGroup, - namespace: haveSameNamespace(resources) ? getNamespace(resources?.[0]) : undefined, + namespace: + haveSameCluster(resources) && haveSameNamespace(resources) + ? getNamespace(resources?.[0]) + : undefined, resource: model.plural, subresource, verb, };This keeps mixed‑cluster selections at pure cluster/collection scope and avoids coupling a single namespace to an unspecified cluster.
src/views/virtualmachines/actions/BulkVirtualMachineActionFactory.tsx (1)
10-16: Bulk RBAC wiring viaasBulkAccessReviewis a good consolidationUsing
asBulkAccessReviewacross the bulk VM actions (delete,editLabels,moveToFolder,pause,restart,start,stop,unpause) is a nice cleanup: it centralizes how cluster/namespace are derived and removes a lot of ad‑hocaccessReviewobjects. TheeditLabelsintegration withgetCommonLabelsandgetLabelsDiffPatchis also consistent with the labels patching logic inutils.ts.One follow‑up you might consider for consistency is updating
migrateStorageto use the same helper instead of a hand‑rolledaccessReview:- migrateStorage: ( - vms: V1VirtualMachine[], - createModal: (modal: ModalComponent) => void, - ): ActionDropdownItemType => ({ - accessReview: { - group: VirtualMachineModel.apiGroup, - namespace: getNamespace(vms?.[0]), - resource: VirtualMachineModel.plural, - verb: 'patch', - }, + migrateStorage: ( + vms: V1VirtualMachine[], + createModal: (modal: ModalComponent) => void, + ): ActionDropdownItemType => ({ + accessReview: asBulkAccessReview(VirtualMachineModel, vms, 'patch'),This keeps all bulk actions using the same access‑review construction path and automatically benefits from any future tweaks in
asBulkAccessReview.Also applies to: 26-27, 47-47, 66-67, 114-115, 151-152, 173-174, 191-192, 202-203, 221-226
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/resources/shared.ts(2 hunks)src/views/virtualmachines/actions/BulkVirtualMachineActionFactory.tsx(11 hunks)src/views/virtualmachines/actions/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/views/virtualmachines/actions/BulkVirtualMachineActionFactory.tsx (3)
src/utils/resources/shared.ts (2)
asBulkAccessReview(219-236)haveSameNamespace(518-519)src/utils/utils/utils.ts (1)
isEmpty(32-33)src/utils/models/index.ts (2)
VirtualMachineInstanceSubresourcesModel(79-87)VirtualMachineSubresourcesModel(69-77)
src/utils/resources/shared.ts (3)
src/utils/utils/utils.ts (1)
isEmpty(32-33)src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/views/cdi-upload-provider/utils/selectors.ts (1)
getNamespace(108-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-test
- GitHub Check: i18n
- GitHub Check: build
🔇 Additional comments (2)
src/views/virtualmachines/actions/utils.ts (1)
4-4: Shared selectors import looks correctImporting
getAnnotation,getLabels, andgetNamefrom the shared utilities matches their usages in this file and centralizes selector logic; no behavior change introduced here.src/utils/resources/shared.ts (1)
518-534: GenerichaveSame*helpers look solidThe
haveSamePropValueabstraction with the 0/1‑item fast path keeps the intent ofhaveSameNamespace/haveSameClusterclear and efficient. No functional issues spotted here.
|
/retest |
| return { | ||
| cluster: haveSameCluster(resources) ? getCluster(resources?.[0]) : undefined, | ||
| group: model.apiGroup, | ||
| namespace: haveSameNamespace(resources) ? getNamespace(resources?.[0]) : undefined, | ||
| resource: model.plural, | ||
| subresource, | ||
| verb, | ||
| }; |
There was a problem hiding this comment.
I don't think this is correct. For non-admin users that select multiple vms in different namespaces, this means that they have to have permissions to delete patch and update in all namespaces to perform the action. Non admin users can have permissions in the selected namespaces but not in all namespaces. So the bulk actions will not be available. This is why the access review was not implemented for the bulk actions. The only solution that i see is to notify the user with a toaster when some requests are not performed for some reasons
There was a problem hiding this comment.
You are right.
- is it possible to do accessReview on multiple specific namespaces?
If not, can we do this instead:
- if they select multiple namespaces, can we randomly pick one to perform the access check and block the action because of that?
But then if the user has access in 9 of 10 namespaces, it is not very good to block all because of just one, so the toast alerts seem like the best option.
There was a problem hiding this comment.
Yes we can run the checkAccess function for all the namespaces (@stolostron/multicluster-sdk have an internal function to do that in multicluster but i think we can use it). This means to fire a fetch request per namespace
There was a problem hiding this comment.
WE could adapt the useMultipleAccessReviews hook that we have in the repo
https://github.com/upalatucci/kubevirt-plugin/blob/5c9a4131711ee885c01e297fd91bef19caa4b772/src/views/cdi-upload-provider/hooks/useMultipleAccessReviews.ts
There was a problem hiding this comment.
But please make sure to fire that when the user open the action dropdown ( i don't know if that make sense ). We don't want to fire hundreds of accessreview requests when the user land on the vm list. Keep in mind that its per cluster per namespace so its N_CLUSTERS X N_NAMESPACES requests.
If we select vms in the same namespace but different clusters, those are separate requests.
There was a problem hiding this comment.
I think that toaster is the best approach
There was a problem hiding this comment.
@upalatucci @adamviktora
I will touch this area as part of bulk-actions-from-plugins story CNV-73282
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@adamviktora @upalatucci Pre-fetching the checks is already in place but we may need to limit the number of requests (or distribute them over time). Some bulk API on the server side would also help. |
adamviktora
changed the title
5 participants
📝 Description
accessReviewin actions ofBulkVirtualMachineActionFactoryisSameNamespaceto a general function to compare any prop on a list of objects🎥 Demo
TODO
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.