oVirt / ovirt-engine

The oVirt Engine virtualization manager
Other
492 stars 259 forks source link

engine: Check concrete disk permissions firstly on TransferImageStatus #946

Closed 0ffer closed 3 weeks ago

0ffer commented 1 month ago

There is some situations when storageDomainId is not come with parameters (image upload cancel and pause operations) for TransferImageStatusCommand. For this operations checked CREATE_DISK permission on SYSTEM_OBJECT (i.e. system-wide). Problem starts when we give permissions for user only on concrete storage domain object (not system-wide). Then permission check failed for operations without storage domain id info in parameters. Here I just add check permission for disk before other objects.

Are you the owner of the code you are sending in, or do you have permission of the owner?

y

sandrobonazzola commented 1 month ago

@dupondje @BrooklynDewolf can you please review?

BrooklynDewolf commented 1 month ago

Code looks good to me. Nice cleanup :)

sandrobonazzola commented 1 month ago

/OST

michalskrivanek commented 1 month ago

/ost

sandrobonazzola commented 1 month ago

/ost

sandrobonazzola commented 4 weeks ago

/ost

sandrobonazzola commented 4 weeks ago

OST is failing test_cold_incremental_backup_vm2 with engine error :

2024-06-05 07:55:49,863Z ERROR [org.ovirt.engine.core.bll.storage.backup.HybridBackupCommand] (default task-1) [full_cold_vm_backup] Command 'org.ovirt.engine.core.bll.storage.backup.HybridBackupCommand' failed: CallableStatementCallback; SQL [{call insertvmbackup(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)}]; ERROR: duplicate key value violates unique constraint "vm_backups_pkey"
  Detail: Key (backup_id)=(1733241d-4a0b-4205-9e52-5383eb7a82f2) already exists.

Not sure how much this failure is related to this PR.

BrooklynDewolf commented 3 weeks ago

@sandrobonazzola This was an issue related to an earlier commit that has been merged into the master branch. It has been fixed in https://github.com/oVirt/ovirt-engine/pull/948

sandrobonazzola commented 3 weeks ago

/ost

sandrobonazzola commented 3 weeks ago

/ost

sandrobonazzola commented 3 weeks ago

OST passed on run 113878 , merging based on previous reviews.