add sshkey support to OpenStack Cloud #4388

sseago · 2018-07-31T16:16:36Z

Add sshkey support to OpenStack Cloud needed for v2v.

sseago · 2018-08-10T04:18:50Z

app/views/shared/views/ems_common/angular/_form.html.haml

@@ -261,7 +261,7 @@
                            :change_stored_password => _("Change stored client key"),
                            :cancel_password_change => _("Cancel client key change"),
                            :verify_title_off       => _("Tenant ID, Client ID and matching Client Key fields are needed to perform verification of credentials")}
-    %div{"ng-if" => "emsCommonModel.emstype != '' && emsCommonModel.emstype != 'azure'"}
+    %div{"ng-show" => "emsCommonModel.emstype != '' && emsCommonModel.emstype != 'azure'"}


This may be the wrong solution here, but "ng-show" is what is used for the ems_infra version of this form. Before I changed from ng-if, I was getting a javascript error. It looks like what was happening was that with the partial not rendering at all with ng-if (vs. rendering and being hidden with ng-show), default_userid is left undefined when $scope.canValidateBasicInfo is called. As a result of this, the javascript on the SSH Keypair endpoint tab does not run. I don't know if there are any downsides to using ng-show here, but this partially reverses an earlier change to the validation (I'll link to the commit in another comment).

The downside to ng-show is that all the fields are there, always.

You can't have multiple ng-show statemens with different conditions including the same partial because then, the partial will always exist multiple times.

(Unless that partial supports some kind of prefix to regulate this.)

((If you really need the partial to exist and not be visible, use ng-if with a .hidden class, but ... better fix the underlying problem really.))

In this specific case... this looks like isBasicInfoValid is already doing logic to determine which fields should be there.

So I'm almost sure any fix for your issue should be to isBasicInfoValid itself, not the partial.

sseago · 2018-08-10T04:20:37Z

The javascript error referenced in my comment on the commit above when I don't change the directive to "ng-show":
TypeError: $scope.angularForm.default_userid is undefined Stack trace: $scope.isBasicInfoValid@http://192.168.1.83:3000/assets/controllers/ems_common/ems_common_form_controller.self-03b2803e616dd53db1122e9cdac7bac3318a68357689230c51338c66eef9132a.js?body=1:318:1 $scope.canValidateBasicInfo@http://192.168.1.83:3000/assets/controllers/ems_common/ems_common_form_controller.self-03b2803e616dd53db1122e9cdac7bac3318a68357689230c51338c66eef9132a.js?body=1:304:12

sseago · 2018-08-10T04:22:23Z

This is the commit that is partially undone by the ng-show change above:
ManageIQ/manageiq@07e9be9#diff-ca482ed9159124886ce2c48c3e67ecc2R201

aufi · 2018-08-14T13:13:07Z

@sseago OpenStack part was merged, can the WIP be removed?

sseago · 2018-08-14T13:28:30Z

@aufi I can remove WIP now. I kept it around in case further work was needed around the potential issue I commented on above, but it's not really "in progress" anymore. It's done unless a better suggestion comes down as to how to handle the prior javascript error.

miq-bot · 2018-08-22T06:33:17Z

This pull request is not mergeable. Please rebase and repush.

blomquisg · 2018-08-23T13:50:45Z

Ping @himdel can you give this another look.

@sseago looks like miq-bot decided this was unmergeable ... take a look at the codeclimate and/or scrutinizer failures

sseago · 2018-08-23T18:21:01Z

@blomquisg The merge issue has already been resolved -- miq-bot removed the unmergeable label. Looking at the codeclimate and scrutinizer errors, those don't seem particularly relevant to my PR. For scrutinizer, it hit an npm version error, and for codeclimate, it's mostly flagging "complexity" issues that reflect the existing design of the code I'm making minor tweaks to rather than the design of the changes.

himdel · 2018-08-27T14:06:13Z

app/controllers/mixins/ems_common_angular.rb

@@ -759,6 +761,11 @@ def build_credentials(ems, mode)
        ssh_keypair_password = params[:ssh_keypair_password] ? params[:ssh_keypair_password].gsub(/\r\n/, "\n") : ems.authentication_key(:ssh_keypair)
        creds[:ssh_keypair] = {:userid => params[:ssh_keypair_userid], :auth_key => ssh_keypair_password, :save => (mode != :validate)}
      end
+      if ems.kind_of?(ManageIQ::Providers::Openstack::CloudManager) &&
+         ems.supports_authentication?(:ssh_keypair) && params[:ssh_keypair_userid]
+        ssh_keypair_password = params[:ssh_keypair_password] ? params[:ssh_keypair_password].gsub(/\r\n/, "\n") : ems.authentication_key(:ssh_keypair)


why do we need a third copy of this code?

Especially since all the other places use a openstack or openstack_infra or rhevm condition instead of 3 different ones.

Please unify the three.

Updated version of the PR combines the three.

himdel · 2018-08-27T14:07:03Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

@@ -309,8 +309,8 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
      ($scope.emsCommonModel.emstype == "ec2" ||
       ['kubevirt', 'nuage_network', 'openstack', 'openstack_infra', 'rhevm', 'scvmm', 'vmwarews', 'vmware_cloud'].includes($scope.emsCommonModel.emstype) &&
       $scope.emsCommonModel.default_hostname) &&
-      ($scope.emsCommonModel.default_userid != '' && $scope.angularForm.default_userid.$valid &&
-       $scope.emsCommonModel.default_password != '' && $scope.angularForm.default_password.$valid)) {
+      ($scope.emsCommonModel.default_userid != '' && $scope.angularForm.default_userid !== undefined && $scope.angularForm.default_userid.$valid &&


why do we need to check for ssh_keypair_userid === undefined for if the tab is correctly displayed?

(Either looks like a bug that needs to be fixed in the other places as well, or a random change that could potentially break other places, unless there is something special about this particular pair of fields that's not happening elsewhere..)

This was the fix that allowed me to remove the earlier change of "ng-if" to "ng-show". Without the 'undefined' check, this code fails with a javascript error. We didn't see the error appear until adding the sshkey tab, since the other case where the sshkey tab is used (ems_infra) uses 'ng-show' instead of 'ng-if' to display the tab. I followed the example of the existing code (see below in this same method) for handling fields which may be undefined. If the prior change using 'ng-show' instead of 'ng-if' is better, I could revert back to the prior version, but I was following your suggestion here to fix this method rather than using ng-show in the view. I think this version is probably a cleaner solution than what was done before. I suspect getting rid of the problem without checking for undefined would be a much larger change to this code which would be tricky to make sure it didn't break other things (across providers, etc.). This is a pretty safe change, since it basically can't break anything -- for any case where these are undefined, the code is broken without this change already, and when these are defined, checking for undefined is safe.

OK, makes sense, thanks :) I was just trying to make sure this is still intended :)

himdel · 2018-08-27T14:10:11Z

app/assets/javascripts/controllers/ems_keypair/ems_keypair_controller.js

@@ -63,9 +63,10 @@
  };

  EmsKeypairController.prototype.showValidate = function(tab) {
-    var openstackAndNew = (this.model.emstype === 'openstack_infra') && this.newRecord;
+    var openstackInfraAndNew = (this.model.emstype === 'openstack_infra') && this.newRecord;
+    var openstackCloud = this.model.emstype === 'openstack';


Is cloud supposed to be different from openstack infra and rhevm here?

Why is validation different for non-new openstack records than for non-new openstack infra? (Any chance that's a bug?)

Cloud is just like rhevm here -- no validation for either 'new' or 'update' forms. Cloud is not like openstack infra because for OpenStack infra, we only validate for 'update', not for 'new'. If you look at the conditional before, we treat rhevm and cloud the same. The distinction is made for OpenStack Infra -- where we validate for update form but not new form. It's not a bug -- it's the way it's supposed to work, since we don't necessarily have an instance running at this point in which we can validate the keypair credentials -- this is the case for both rhevm and openstack cloud.

Add sshkey support to OpenStack Cloud needed for v2v.

miq-bot · 2018-08-28T01:02:51Z

Checked commit sseago@cdc619f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

himdel · 2018-08-29T11:22:37Z

OK, LGTM , thanks for the details! :)
Verified seeing no breakage in the UI 👍

(ignoring hakiri - fixed by #4563, and cc because ems_common)

miq-bot added the wip label Jul 31, 2018

sseago force-pushed the v2v_sshkey branch from c75c689 to ead6acb Compare August 10, 2018 02:35

sseago commented Aug 10, 2018

View reviewed changes

sseago closed this Aug 10, 2018

sseago reopened this Aug 10, 2018

aufi mentioned this pull request Aug 13, 2018

add sshkey support to OpenStack Cloud ManageIQ/manageiq-providers-openstack#327

Merged

sseago changed the title ~~[WIP] add sshkey support to OpenStack Cloud~~ add sshkey support to OpenStack Cloud Aug 14, 2018

miq-bot removed the wip label Aug 14, 2018

mzazrivec added enhancement compute/cloud labels Aug 21, 2018

sseago force-pushed the v2v_sshkey branch from ead6acb to 4a89279 Compare August 22, 2018 01:40

miq-bot added the unmergeable label Aug 22, 2018

sseago force-pushed the v2v_sshkey branch from 4a89279 to 08d8ddd Compare August 22, 2018 12:53

miq-bot removed the unmergeable label Aug 22, 2018

himdel reviewed Aug 27, 2018

View reviewed changes

add sshkey support to OpenStack Cloud

cdc619f

Add sshkey support to OpenStack Cloud needed for v2v.

sseago force-pushed the v2v_sshkey branch from 08d8ddd to cdc619f Compare August 28, 2018 00:59

himdel merged commit 3f140ac into ManageIQ:master Aug 29, 2018

himdel added this to the Sprint 94 Ending Sept 10, 2018 milestone Aug 29, 2018

himdel self-assigned this Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add sshkey support to OpenStack Cloud #4388

add sshkey support to OpenStack Cloud #4388

sseago commented Jul 31, 2018 •

edited

Loading

sseago Aug 10, 2018

himdel Aug 21, 2018

himdel Aug 21, 2018

himdel Aug 21, 2018

sseago commented Aug 10, 2018

sseago commented Aug 10, 2018

aufi commented Aug 14, 2018

sseago commented Aug 14, 2018

miq-bot commented Aug 22, 2018

blomquisg commented Aug 23, 2018

sseago commented Aug 23, 2018

himdel Aug 27, 2018 •

edited

Loading

sseago Aug 28, 2018

himdel Aug 27, 2018

himdel Aug 27, 2018 •

edited

Loading

sseago Aug 27, 2018

himdel Aug 29, 2018

himdel Aug 27, 2018

sseago Aug 27, 2018

miq-bot commented Aug 28, 2018

himdel commented Aug 29, 2018 •

edited

Loading

add sshkey support to OpenStack Cloud #4388

add sshkey support to OpenStack Cloud #4388

Conversation

sseago commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sseago commented Aug 10, 2018

sseago commented Aug 10, 2018

aufi commented Aug 14, 2018

sseago commented Aug 14, 2018

miq-bot commented Aug 22, 2018

blomquisg commented Aug 23, 2018

sseago commented Aug 23, 2018

himdel Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 28, 2018

himdel commented Aug 29, 2018 • edited Loading

sseago commented Jul 31, 2018 •

edited

Loading

himdel Aug 27, 2018 •

edited

Loading

himdel Aug 27, 2018 •

edited

Loading

himdel commented Aug 29, 2018 •

edited

Loading