Use API to get block storage manager for volumes #861

gberginc · 2017-03-30T21:05:17Z

Instead of using instance variables to pass the list of available block storage managers to the form we are exploiting the capabilities of the API to return the list of storage managers supporting block_storage.

This patch removes the logic from the ruby controller and adds the required API calls to the JS controller. In order to deal with the asynchronous nature of data retrieval, this patch also changes how afterGet is set to ensure that all the required information is in place.

A video showing the form for creating new cloud volumes: http://x.k00.fr/ha2qj.

It first shows how the form behaves when cloud volumes are created for a specific block storage manager and then also how it behaves when creating a volume from the list of volumes from all block storage managers.

@miq-bot add_label storage,refactoring
@miq-bot assign @AparnaKarve

/cc @miha-plesko Could be useful with regards to #651.

AparnaKarve · 2017-03-30T22:28:06Z

app/controllers/cloud_volume_controller.rb

@@ -269,13 +269,8 @@ def new
    assert_privileges("cloud_volume_new")
    @volume = CloudVolume.new
    @in_a_form = true
-    @storage_manager_choices = {}
    if params[:storage_manager_id]
      @storage_manager_id = params[:storage_manager_id]


@gberginc This line seems to be causing the Hakiri Security issue.
Is there any other way we could retrieve the Storage Manager id here without directly assigning params[:storage_manager_id] ?

I guess an alternate approach (somewhat roundabout route) would be to loop through all EMSes that have supports_block_storage? = true and look for the one that matches params[:storage_manager_id]

gberginc · 2017-03-31T04:24:08Z

@AparnaKarve I have added two commits, one passing Hakiri, the other surprisingly not...

First will query the DB and look for the storage manager with that specific ID. It will then store the whole storage manager into an instance variable. I am not completely sure what you meant by looping through all EMSes, but I guess this is something similar.

Second commit fails. I am more in favour of this one because I think it is safe and we only ever use the ID. But Hakiri doesn't agree. It basically ensures the given storage_manager_id is an Integer and passes it to the form - if it's not an integer, it will be set to nil due to exception in Integer(params[:storage_manager]). For example, http://localhost:3000/cloud_volume/new?storage_manager_id=abrakadabra will result in storage_manager_id = nil. The only consequence of this is that the select will not be pre-populated (check screenshot).

However, since I am nowhere near being an expert in XSS, I have no idea if someone can exploit something like this.

Can you please check if any of the two commits works for you and I'll then remove the other one?

gberginc · 2017-03-31T04:33:58Z

Nah, even rubocop doesn't like my proposal 😄.

miha-plesko · 2017-03-31T06:17:02Z

app/assets/javascripts/controllers/cloud_volume/cloud_volume_form_controller.js

@@ -180,5 +177,19 @@ ManageIQ.angular.app.controller('cloudVolumeFormController', ['$scope', 'miqServ
    }
  };

+  var formOptions = function() {
+    API.get("/api/providers?expand=resources&attributes=id,name&filter[]=supports_block_storage?=true").then(function(data) {


Does every StorageManager that supports :block_storage feature also supports creation of new cloud volumes? At the moment this seems to be true (since only OpenStack and Amazon support object storage), but will it always be true? When someone implements inventory (e.g. for Azure) it will appear on this list even if creating new volumes is not implemented yet.

Yes. This code is currently only replacing the existing logic which filtered with supports_block_storage?. In EBS, supports_create? is only available in the CloudVolume so I am not sure how to check for that or even if it makes sense at this point.

gberginc · 2017-03-31T07:55:40Z

@AparnaKarve @miha-plesko has suggested the use of ERB::Util.escape which I did in c87754f, but Hakiri still complains. Pure logic tells me that the second commit should be sufficient, but I am no longer sure.

AparnaKarve · 2017-03-31T17:24:49Z

@gberginc Probably all you need is this:

@storage_manager = find_record_with_rbac(ExtManagementSystem, params[:storage_manager_id])

and this:

ManageIQ.angular.app.value('storageManagerId', '#{@storage_manager.try(:id)}');

Can you give that a try?

gberginc · 2017-03-31T18:04:20Z

@AparnaKarve that was similar to the first proposed solution. If it's ok with you, it is also ok with me. If an illegal storage_manager_id, it will (check screenshot):

AparnaKarve · 2017-03-31T18:08:09Z

@gberginc Yes, very similar to your first try.

Why would storage_manager_id ever be invalid?

gberginc · 2017-03-31T18:15:21Z

@AparnaKarve only is the user would alter the ID :-D

Instead of using instance variables to pass the list of available block storage managers to the form we are exploiting the capabilities of the API to return the list of storage managers supporting `block_storage`. This patch removes the logic from the ruby controller and adds the required API calls to the JS controller. In order to deal with the asynchronous nature of data retrieval, this patch also changes how `afterGet` is set to ensure that all the required information is in place.

miq-bot · 2017-03-31T18:53:28Z

Checked commit gberginc@93dd8e0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

AparnaKarve · 2017-03-31T19:06:18Z

only is the user would alter the ID :-D

Ok, in that case, nothing to worry about :)

AparnaKarve · 2017-03-31T22:12:13Z

app/assets/javascripts/controllers/cloud_volume/cloud_volume_form_controller.js

+      miqService.sparkleOff();
+    });
+  };
+


@gberginc Can you add a .catch block here to catch failures?

We typically write API.get like this --

API.get(url) .then(getData) .catch(miqService.handleFailure);

The .catch did not work before (one of the reasons why I didn't suggest using it before), but it has been fixed now.

AparnaKarve · 2017-03-31T22:13:22Z

Other than the above comment, LGTM.

gberginc · 2017-04-06T19:55:08Z

@AparnaKarve Closing in favour of #950. It turned out that just the changes in this PR did not suffice for a more general approach to show the proper edit form.

miq-bot added refactoring storage labels Mar 30, 2017

miq-bot assigned AparnaKarve Mar 30, 2017

AparnaKarve reviewed Mar 30, 2017

View reviewed changes

gberginc force-pushed the use_api_in_cloud_volume_controller branch from 9c64c59 to b5c1027 Compare March 31, 2017 04:16

miha-plesko reviewed Mar 31, 2017

View reviewed changes

gberginc force-pushed the use_api_in_cloud_volume_controller branch 2 times, most recently from f9f058e to c87754f Compare March 31, 2017 07:50

gberginc force-pushed the use_api_in_cloud_volume_controller branch 2 times, most recently from 8d5325f to 8a5ab64 Compare March 31, 2017 18:01

gberginc force-pushed the use_api_in_cloud_volume_controller branch from 8a5ab64 to 93dd8e0 Compare March 31, 2017 18:50

AparnaKarve reviewed Mar 31, 2017

View reviewed changes

gberginc mentioned this pull request Apr 6, 2017

Fix and modernise cloud volume forms #950

Merged

gberginc closed this Apr 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use API to get block storage manager for volumes #861

Use API to get block storage manager for volumes #861

gberginc commented Mar 30, 2017

AparnaKarve Mar 30, 2017 •

edited

Loading

gberginc commented Mar 31, 2017

gberginc commented Mar 31, 2017

miha-plesko Mar 31, 2017

gberginc Mar 31, 2017

gberginc commented Mar 31, 2017

AparnaKarve commented Mar 31, 2017

gberginc commented Mar 31, 2017

AparnaKarve commented Mar 31, 2017

gberginc commented Mar 31, 2017

miq-bot commented Mar 31, 2017

AparnaKarve commented Mar 31, 2017

AparnaKarve Mar 31, 2017 •

edited

Loading

AparnaKarve commented Mar 31, 2017

gberginc commented Apr 6, 2017

Use API to get block storage manager for volumes #861

Use API to get block storage manager for volumes #861

Conversation

gberginc commented Mar 30, 2017

AparnaKarve Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

gberginc commented Mar 31, 2017

gberginc commented Mar 31, 2017

miha-plesko Mar 31, 2017

Choose a reason for hiding this comment

gberginc Mar 31, 2017

Choose a reason for hiding this comment

gberginc commented Mar 31, 2017

AparnaKarve commented Mar 31, 2017

gberginc commented Mar 31, 2017

AparnaKarve commented Mar 31, 2017

gberginc commented Mar 31, 2017

miq-bot commented Mar 31, 2017

AparnaKarve commented Mar 31, 2017

AparnaKarve Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Mar 31, 2017

gberginc commented Apr 6, 2017

AparnaKarve Mar 30, 2017 •

edited

Loading

AparnaKarve Mar 31, 2017 •

edited

Loading