[FINE] Lazy load StorageManagers upon volume creation #4600
Conversation
When creating a new volume user has to pick what StorageManager is volume supposed to be created with, hence we perform API request to fetch those. Problem is we share angular controller with other volume operations (attach, detach) that don't need this list of all managers and fetching those anyway results in permission denied error due to fine-grained permission setting. For example if we give user only permission to attach/detach volume, but not to view StorageManagers, then we must not perform the API call as it results in 403 with red popup. With this commit we refactor controller so that it now only performs the API calls that are really needed. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1571224 Signed-off-by: Miha Pleško <[email protected]>
| @@ -57,4 +57,5 @@ | |||
| :javascript | |||
| ManageIQ.angular.app.value('cloudVolumeFormId', '#{@volume.id}'); | |||
| ManageIQ.angular.app.value('storageManagerId', #{@volume.ext_management_system.id}); | |||
There was a problem hiding this comment.
@himdel in Fine the IDs seem to be integers so we don't need to change to string as on the original PR.
There was a problem hiding this comment.
We definitely do, the same issue with too big integers is also present in fine (but more places where it's not fixed)
(Furthermore, you may need to do the from_cid / to_cid trasnformation - you need to use a region other than 0 in your database in order to make sure this works.)
There was a problem hiding this comment.
(IIRC in regions other than 0, the ids will come in the 10r256 format from the API too)
|
@miq-bot add_label blocker,storage |
| vm.storageManagerChanged(vm.cloudVolumeModel.storage_manager_id); | ||
| } | ||
| // Can handle list of all managers or a single manager. | ||
| vm.storageManagers = data.resources ? data.resources : [data]; |
There was a problem hiding this comment.
@himdel this is where we store value "as is" - and it's integer. If I convert storageManagerId into a string (in the haml) then nothing gets selected in the drop-down. Is it possible that API returns integers? Or you think controller somehow parses it wrong?
I say we do separate PR for this as it's not related to this BZ. Will check it a bit later when I checkout fine branch again.
There was a problem hiding this comment.
The API will most likely return compressed ids, in the format like "1r23". You can use miqUncompressedId to convert to '1000000000023' in JS, or you can use to_cid ruby-side to convert to that format.
(again, you'll need to be in a db region other than 0 to be able to tell)
|
@himdel sorry it took me so long to respond, but eventually I resolved the ID issue by grabbing the ID from Looking forward to your opinion. |
It's unsafe to interpret IDs as integers as they may be too large. But since API returns them as integers, we rather do a workaround and override them with string equivalents obtained by parsing the href string: ``` "href":"http://172.16.117.189:3000/api/providers/123000000000001" |--- the ID ---| ``` Signed-off-by: Miha Pleško <[email protected]>
miha-plesko
force-pushed
the
fine-lazy-fetch-storage-managers
branch
from
328d169 to
a25207a
Compare
| // Parse ID from href, e.g. http://172.16.117.189:3000/api/providers/123000000000001. | ||
| return href.toString().replace(/\/$/, '').split('/').slice(-1)[0]; | ||
| }; | ||
|
|
There was a problem hiding this comment.
@himdel I added some more checks to prevent crash on strange inputs like undefined or integer value. It's better if we don't crash and return invalid value because we're just filling the drop-down after all...
There was a problem hiding this comment.
I've just tested cloud volume creation (the only operation where user can actually modify drop-down value for manager) and it passes on correct value with valid payload like:
name=miha-volume&aws_encryption=false&incremental=false&force=false&storage_manager_id=123000000000003&emstype=...
In other words, it seems to work.
There was a problem hiding this comment.
Yeah, looks about right :)
slice with negative indexes seems to be supported everywhere, not seeing any issues 👍
|
Checked commits miha-plesko/manageiq-ui-classic@12f5a03~...a25207a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Same as #4549 but with merge conflicts resolved.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1625250
@miq-bot add_label bug,fine/yes,gaprindashvili/no
@miq-bot assign @himdel
/cc @AparnaKarve @simaishi