Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM pools are displayed for non permitted users during manual/auto refresh #1343

Open
sgratch opened this issue Dec 8, 2020 · 12 comments
Open

Comments

@sgratch
Copy link
Member

sgratch commented Dec 8, 2020

After creating a new pool with "Maximum number of VMs per user" set to more than 3 vms, every logged non admin user in web-ui will view this new pool's VMs as part of his VMs list dashboard, even though they are not assigned to him.
Only after hard refresh (ctrl+F5) those new pool VMs are dismissed from user's VMs list dashboard

For reproducing:

  1. Logged in to web-ui as a specific user with only UserRole permission on global level.
  2. Create a new pool via webadmin with "Maximum number of VMs per user" set to 3.
  3. on web-ui logged as a specific user: manually refresh or wait for auto refresh and check the VMs list dashboard.

Actual result:
The new pool and one one pool VM will be displayed on VMs list dashboard. For example in case new pool name is eee-pool5, the following will be displayed on dashboard till hard refresh:
image

Expected result:
Only new pool card should be displayed without displaying any of the new pool VMs, since those pool VMs are not assigned to this logged user.

@sgratch
Copy link
Member Author

sgratch commented Dec 8, 2020

Note that if Pool's "Maximum number of VMs per user" value is set to 4, then 2 pool VMs are displayed for non admin users on web-ui dashboard etc..

@michalskrivanek
Copy link
Member

1. Logged in to web-ui as a specific user with only UserRole permission on global level.

that means you have operational permissions to every single VM in the system.
It shouldn't be assigned like that
If you do that then the bug is the other way around:) you should always see all the VMs.

@sgratch
Copy link
Member Author

sgratch commented Dec 9, 2020

that means you have operational permissions to every single VM in the system.
It shouldn't be assigned like that
If you do that then the bug is the other way around:) you should always see all the VMs.

This issue is reproduced when UserRole is assigned on cluster level as well.
In addition, UserRole on system level means that you have operational permissions to every single VM in the system except pool vms (checking the permissions on webadmin shows that inheritance is not done for pool vms until they are assigned)

@michalskrivanek
Copy link
Member

This issue is reproduced when UserRole is assigned on cluster level as well.

yes, because then you granted operational permission to every single VM in that Cluster. Again, it's not what you typically want.
Inheritance works as per https://www.ovirt.org/develop/developer-guide/action-permissions-overview.html

@hstastna
Copy link
Member

hstastna commented Dec 9, 2020

Folks, if you still think this is an issue, then it should be fixed in the API. The API returns the VMs which shouldn't and the UI displays them. The query we use in the UI to fetch the VMs or just one single VM is correct.

If this is not the issue, we should to make some changes in the webadmin regarding assigning permissions, to make sure (or at least better to inform) the admin assigns permissions the correct way to the correct entities.

@sgratch
Copy link
Member Author

sgratch commented Dec 9, 2020

@hstastna partially correct since hard refresh does return a different result (without those vms) so we are not consistent.
In addition, the "allocated vms per user" counter should reflect those vms that are shown on web-ui and it's currently not.

@sjd78
Copy link
Member

sjd78 commented Dec 11, 2020

I ran some test on a 4.4.3 rpm install engine with master branch ovirt-web-ui and direct calls with curl. Pools created in 2 different clusters. A non-admin user has "UserRole" on a Pool in the first cluster and has "UserRole" on the second cluster in general.

So when the user has "UserRole" only on the Pool, things work properly. Pools VMs that are not assigned/taken by a user return a 404 when queried directly. Stopping a taken VM automatically disappears from the card list and responds 404 with direct query.

When the user has "UserRole" on the Cluster containing the Pool, things kinda work, but not really. The main VM/Pool fetching grabs the Pool but excludes all of the Pool VMs, even when the query is run directly. However, ALL Pool VMs in the pool can be directly queried, since they inherit UserRole from the cluster. That is what causes the VM Portal to hold on to a Stopped/returned/newly unassigned VMs. On background refresh in ovirt-web-ui, if a VM is known in the redux store, and is not updated with VM fetchByPage, it will be refreshed directly. Since the VM fetch does not return a 404, ovirt-web-ui considers the VM ok and just happily refreshes and displays it.

Only the VMs fetch that hits the ovirt-web-ui case on the REST API side works in the "UserRole" on cluster scenario. Since PR #1335 is not in 4.4.3, all the Pool VMs eventually show up after a round of background refreshing!

What can we do to handle the "UserRole" on the Cluster issue?

  • Have docs updated so permissions are assigned correctly.

  • We maybe could fudge it a bit. When stopping a Pool VM, preemptively remove it from redux and push the user directly back to the Card list. Then if the VM stops quickly it won't get reloaded at the next refresh. If the VM takes longer than the next refresh, the card will come back and be "stuck" there again. Really it may not (probably won't) work at all.

  • If we have access to who the VM is assigned to, then we could manually filter out Pool VMs not assigned to the current user. I don't know if that data is available on the standard VMs or Pools fetch.

  • Raise a BZ on the rest api such that Pool VM direct fetches return a 404 if the "Filter: true" header is defined and the VM is not assigned to the requesting user. There must be logic like that somewhere since the /ovirt-engine/api/vms;max=10?search=SORTBY NAME ASC page 1 fetch does not return those VMs.

@isaranova
Copy link

I'm joining Scott on the issues he raised. I'm in the middle of automating VM Pools basic sanity tests and cannot proceed, because on the latest 4.4.4 user cannot allocate a VM from VM Pool if the pool has only 1 VM (it's free, no other user has it).

Steps:

  1. In AdminPortal create VM Pool with 1 VM, 0 prestarted VMs and 1 VM per user.
  2. Add user with system UserRole and PowerUserRole.
  3. Login as the user and go to VM Portal.
  4. Try to allocate 1 VM from the VM Pool.

Results:
VM Pool is not present and user cannot allocate the VM. However, user sees the VM from the VM Pool in Off state, even though he didn't allocate it. User can run the VM. Refreshing (both hard and VM Portal) does not fix the issue.

Reproduced 100% on d/s: ovirt-engine-4.4.4.5-0.10.el8ev

@sgratch
Copy link
Member Author

sgratch commented Dec 23, 2020

I'm joining Scott on the issues he raised. I'm in the middle of automating VM Pools basic sanity tests and cannot proceed, because on the latest 4.4.4 user cannot allocate a VM from VM Pool if the pool has only 1 VM (it's free, no other user has it).

@isaranova the specific scenario you described here (pool with one VM pool) is not reproduced to me. can you please attach screenshots/videos? Thanks

@isaranova
Copy link

Seems like it was a broken hosted-engine environment - I retested on 3 different new rebuilt environments and cannot reproduce the issue anymore. Sorry for false report.

@sgratch
Copy link
Member Author

sgratch commented Jan 17, 2021

This was reproduced to me on oVirt version 4.3.11 as well (web-ui version 1.6.0-1) and therefore it's not a new regression.

@sgratch
Copy link
Member Author

sgratch commented Jan 18, 2021

Since this issue happens on a very specific timing when a new pool is still created and refresh is done on web-ui and since it is not a new regression then I suggest to avoid fixing it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants