Add cloud types to authentication options #13951

jntullo · 2017-02-16T18:49:43Z

The UI needs a way to populate a dropdown menu with the different ansible cloud credential type names and know the subclass to filter on for those credentials:

Sample output: (not all included - because it's long)

"data":{  
   "credential_types":{  
      "external_ansible_credentials":{  
         "ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential":{  
            "type":"cloud",
            "label":"Amazon",
            "attributes":{  
               "userid":{  
                  "label":"Access Key",
                  "help_text":"AWS Access Key for this credential"
               },
               "password":{  
                  "type":"password",
                  "label":"Secret Key",
                  "help_text":"AWS Secret Key for this credential"
               },
               "security_token":{  
                  "type":"password",
                  "label":"STS Token",
                  "help_text":"Security Token Service(STS) Token for this credential",
                  "max_length":1024
               }
            }
         }
      },
      "embedded_ansible_credentials":{  
         "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential":{  
            "label":"Scm",
            "type":"scm",
            "attributes":{  
               "userid":{  
                  "label":"Access Key",
                  "help_text":"AWS Access Key for this credential"
               },
               "password":{  
                  "type":"password",
                  "label":"Secret Key",
                  "help_text":"AWS Secret Key for this credential"
               },
               "ssh_key_unlock":{  
                  "type":"password",
                  "label":"Private key passphrase",
                  "help_text":"Passphrase to unlock SSH private key if encrypted",
                  "max_length":1024
               },
               "ssh_key_data":{  
                  "type":"password",
                  "label":"Private key",
                  "help_text":"RSA or DSA private key to be used instead of password"
               }
            }
         },
         "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::AmazonCredential":{  
            "type":"cloud",
            "label":"Amazon",
            "attributes":{  
               "userid":{  
                  "label":"Access Key",
                  "help_text":"AWS Access Key for this credential"
               },
               "password":{  
                  "type":"password",
                  "label":"Secret Key",
                  "help_text":"AWS Secret Key for this credential"
               },
               "security_token":{  
                  "type":"password",
                  "label":"STS Token",
                  "help_text":"Security Token Service(STS) Token for this credential",
                  "max_length":1024
               }
            }
         },
         "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::VmwareCredential":{  
            "label":"VMware",
            "type":"cloud",
            "attributes":{  
               "userid":{  
                  "label":"Username",
                  "help_text":"Username for this credential"
               },
               "password":{  
                  "type":"password",
                  "label":"Password",
                  "help_text":"Password for this credential"
               },
               "host":{  
                  "type":"string",
                  "label":"vCenter Host",
                  "help_text":"The hostname or IP address of the vCenter Host",
                  "max_length":1024
               }
            }
         }
      }
   }

@miq-bot add_label api, enhancement, euwe/no
@miq-bot assign @abellotti

cc: @h-kataria @bdunne

abellotti · 2017-02-16T18:58:59Z

app/controllers/api/authentications_controller.rb

+
+    def build_additional_fields
+      {
+        :cloud_types => ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential.credential_types


they looked sorted, but that may not always be guaranteed. can you add a sort here or in the credential_types method itself (otherwise, you'd need the sort in the rspec too).

👍 looks like the underlying method now does the sort, so we're good.

abellotti · 2017-02-16T19:03:36Z

spec/requests/api/authentications_spec.rb

+      attributes = (Authentication.attribute_names - Authentication.virtual_attribute_names).sort.as_json
+      attributes.delete('password')
+      reflections = (Authentication.reflections.keys | Authentication.virtual_reflections.keys.collect(&:to_s)).sort
+      subcollections = Array(Api::ApiConfig.collections[:authentications].subcollections).collect(&:to_s).sort


the above four are now repeated (short of class name) in arbitration_rule_spec.rb, querying_spec.rb, and not authentications. Can you create a helper that returns the four (attributes, virtual_attributes, relationships, subcollections) in a hash for a given class and use that helper ? Thanks.

bdunne · 2017-02-16T19:41:51Z

app/models/manageiq/providers/ansible_tower/automation_manager/cloud_credential.rb

@@ -1,2 +1,10 @@
 class ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential < ManageIQ::Providers::AutomationManager::Authentication
+  def self.credential_types
+    credential_types = {}
+    subclasses.each do |subclass|


You could use subclasses.each_with_object({}) do |subclass, credential_types|

bdunne · 2017-02-16T19:42:10Z

app/models/manageiq/providers/ansible_tower/automation_manager/cloud_credential.rb

+      provider_name = subclass.name.demodulize.split('Credential').first
+      credential_types[provider_name] = subclass.name
+    end
+    credential_types.sort.to_h


Why does this need to be sorted?

If sorting is important, it would be more efficient to sort before creating the hash.

subclasses.sort_by(&:name).each_with_object.....

bdunne · 2017-02-16T19:45:22Z

app/models/manageiq/providers/ansible_tower/automation_manager/cloud_credential.rb

@@ -1,2 +1,10 @@
 class ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential < ManageIQ::Providers::AutomationManager::Authentication
+  def self.credential_types


Shouldn't this be defined on the base class?

@bdunne we're only worried about cloud credentials here

miq-bot · 2017-02-16T19:56:36Z

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

abellotti · 2017-02-16T20:40:39Z

spec/support/api_helper.rb

+          'virtual_attributes' => Authentication.virtual_attribute_names.sort.as_json,
+          'relationships'      => reflections,
+          'subcollections'     => subcollections,
+          'data'               => {}.merge!(data)


can this just be 'data' => data ?

abellotti · 2017-02-16T20:57:58Z

@jntullo I'm good with this PR. there are 3 specs, clusters_spec.rb, hosts_spec.rb and querying_spec.rb that could use the new expect_options_results and remove some duplication, we can do this here or in another PR if you prefer. up to you. Thanks.

jntullo · 2017-02-16T20:59:39Z

@abellotti I'll create another PR for that 👍

abellotti · 2017-02-16T21:47:59Z

@bdunne ok by you ?

Fryguy · 2017-02-16T22:01:50Z

Does this mean a "toplevel" credential will be named "Amazon", or is that only the display name.

Can you show here what the result of the OPTIONS call is (might help me understand better)

jntullo · 2017-02-16T22:13:35Z

@Fryguy only the display name. The response from OPTIONS is:

{
"attributes" => [<authentication attributes>],
"virtual_attributes" => [...],
"relationships" => [...],
"subcollections" => []
"data"=>
  {"cloud_types"=>
    {"Amazon"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential",
     "Azure"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::AzureCredential",
     "Google"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::GoogleCredential",
     "Openstack"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::OpenstackCredential",
     "Rackspace"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::RackspaceCredential",
     "Satellite6"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::Satellite6Credential",
     "Vmware"=>"ManageIQ::Providers::AnsibleTower::AutomationManager::VmwareCredential"}}
}

bdunne · 2017-02-16T22:38:04Z

app/models/manageiq/providers/ansible_tower/automation_manager/cloud_credential.rb

@@ -1,2 +1,8 @@
 class ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential < ManageIQ::Providers::AnsibleTower::AutomationManager::Credential
+  def self.credential_types
+    subclasses.sort_by(&:name).each_with_object({}) do |subclass, credential_types|
+      provider_name = subclass.name.demodulize.split('Credential').first


Should this be a display_name class method on each subclass?

We have the same concept on other models already...
https://github.com/ManageIQ/manageiq/blob/master/app/models/ext_management_system.rb#L20-L26
https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/redhat/infra_manager.rb#L32-L38

Fryguy · 2017-02-16T22:45:17Z

Ah, now I see...so, to me, this feels off a bit, and also may cause issues with when we need to have the extra attributes in the OPTIONS (see #13826).

cloud_types is too high level, and precludes including credentials for Cloud Managers in the future (say, Amazon Key Pair types), because the "cloud" name is taken. I'm thinking this should be related to the class nesting somehow, My initial thought is something like:

{
  "automation_manager/cloud" => { # Not sold on this naming or nesting with strings versus proper Hash nesting, but just illustrating
    "ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential" => {
      "display_name" => "Amazon",
      "extra_attributes" => [
        {
          "type" => "password",
          "display_name" => "STS Token",
          "description" => "For the tooltip"
        }
      ]
    }
  }
}

Fryguy · 2017-02-16T22:47:31Z

Re: "automation_manager/cloud" @bdunne @blomquisg @jntullo Thoughts on better naming? I was trying to go with the class nesting, but it's kind of nasty.

Fryguy · 2017-02-16T22:49:38Z

Alternately, we don't nest by cloud/machine whatever and that just becomes a property....

{
  "automation_manager" => { # Not sold on this naming or nesting with strings versus proper Hash nesting, but just illustrating
    "ManageIQ::Providers::AnsibleTower::AutomationManager::AmazonCredential" => {
      "display_name" => "Amazon",
      "type" => "cloud",  # or group or something
      "extra_attributes" => [
        {
          "type" => "password",
          "display_name" => "STS Token",
          "description" => "For the tooltip"
        }
      ]
    }
  }
}

jntullo · 2017-02-16T23:33:08Z

@Fryguy I like the automation_manager suggestion. All of that info then, display_name, type, extra_attributes can be returned from a class method - so #13826 can include that.

Fryguy · 2017-02-17T20:42:16Z

Agreed. cc @blomquisg re more data we might need on the OPTIONS call.

Fryguy · 2017-02-22T17:03:41Z

I'm thinking we may remove the concept of "extra_attributes" here and just have it be "attributes" or something, and would include both the "extra" stuff and the regular columns. See #13826 (comment)

jameswnl · 2017-03-02T21:17:49Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/amazon_credential.rb

@@ -23,4 +23,10 @@ module ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::AmazonCrede
  }.freeze

  API_ATTRIBUTES = COMMON_ATTRIBUTES.merge(EXTRA_ATTRIBUTES).freeze
+
+  API_OPTIONS = {
+    'display_name' => 'Amazon',


We use label for the individual fields in the credential as here. It will be of same usage as display_name. Do we want to unify them?

jntullo · 2017-03-13T13:09:57Z

ping @abellotti

abellotti · 2017-03-13T18:44:34Z

Hi @jntullo nice detailed options returned.

One concern here is that we lost one level under data which precludes us from adding option data for a different aspect of the collection.

"data" : {
  "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential": {
    "label": "Machine",
    ...
  },
  "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential": {
    "label": "Scm",
   ...
  },
  ...
}

maybe add something like credential_types as follows:

"data" : {
  "credential_types" : {
    "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential": {
      "label": "Machine",
      ...
    },
    "ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential": {
      "label": "Scm",
     ... 
    },
  ...
  }
}

mzazrivec · 2017-03-14T15:27:12Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/amazon_credential.rb

+
+  API_OPTIONS = {
+    :type       => 'cloud',
+    :label      => 'Amazon',


I think the 'Amazon' here (and the other labels too) needs to be inside N_():

N_('Amazon')

so that it gets into the translation catalog and can then be localized on the UI side (or whatever would
consume this info via API).

mzazrivec · 2017-03-14T15:46:25Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/amazon_credential.rb

@@ -23,4 +23,10 @@ module ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::AmazonCrede
  }.freeze

  API_ATTRIBUTES = COMMON_ATTRIBUTES.merge(EXTRA_ATTRIBUTES).freeze
+
+  API_OPTIONS = {
+    :type       => N_('cloud'),


I think :type here doesn't need to be inside N_(). It's not going to be presented in the UI as a nice string, correct? Just the label should be enough here.

abellotti · 2017-03-15T19:55:30Z

Thanks @jntullo for the added credential_types section. Why the additional sections in it ? ansible_tower_credentials, embedded_ansible_credentials, etc. it will be more work to just search by class name, plus it will add churn to this API options logic as more types are added.

jntullo · 2017-03-15T19:58:57Z

@abellotti this was required by @h-kataria - she may be able to provide more information.

h-kataria · 2017-03-15T20:24:01Z

@abellotti i need to show Cloud types only for Embedded Ansible class, without ansible_tower_credentials, embedded_ansible_credentials sections, i am not able to get only specific type to populate the drop down.

abellotti · 2017-03-15T20:34:48Z

ok. @jntullo so the remaining concern is the possible future churn on that API code.

I see the code for build_ansible_tower_creds and build_embedded_ansible_creds similar other than the starting class. would it be possible to have the model provide us the following Hash:

{
  :ansible_tower_credential_types => ManangeIQ::Providers::AnsibleTower::AutomationManager::Credential,
  :embedded_ansible_credential_types => ManageIQ::Providers::EmbeddedAnsible::AutomationManager::Credential
}

or some variant, then the API simply go throw that Hash in build_additional_fields to generate dynamically. In future we could simply update that Hash in model and no API changes needed.

miq-bot · 2017-03-16T14:56:37Z

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

adding sort

stay consistent with label

update to include credential_types translations

miq-bot · 2017-03-16T15:08:56Z

Checked commits jntullo/manageiq@f45eea5~...2a74dc1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 0 offenses detected
Everything looks good. ⭐

abellotti · 2017-03-16T15:50:20Z

Thanks @jntullo for the update. LGTM!! 👍

miq-bot added api enhancement euwe/no labels Feb 16, 2017

miq-bot assigned abellotti Feb 16, 2017

abellotti reviewed Feb 16, 2017

View reviewed changes

bdunne reviewed Feb 16, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Feb 16, 2017

abellotti reviewed Feb 16, 2017

View reviewed changes

bdunne reviewed Feb 16, 2017

View reviewed changes

jntullo mentioned this pull request Feb 23, 2017

Extensible attributes for Ansible Tower Credentials #13826

Merged

miq-bot added the providers/ansible_tower label Feb 28, 2017

jameswnl reviewed Mar 2, 2017

View reviewed changes

h-kataria mentioned this pull request Mar 3, 2017

Use API call to populate cloud_types drop down ManageIQ/manageiq-ui-classic#570

Merged

jntullo closed this Mar 6, 2017

jntullo reopened this Mar 6, 2017

jntullo closed this Mar 9, 2017

jntullo reopened this Mar 9, 2017

mzazrivec reviewed Mar 14, 2017

View reviewed changes

miq-bot added the unmergeable label Mar 16, 2017

Jillian Tullo added 5 commits March 16, 2017 11:01

add ansible cloud types to authentication options

f45eea5

adding sort

utilize new API options

0946492

stay consistent with label

select only unencrypted

875fcde

update to include credential_types translations

update format to separate embedded and tower

c44afb3

move creation of options to the model

2a74dc1

miq-bot removed the unmergeable label Mar 16, 2017

abellotti merged commit 2cbb57b into ManageIQ:master Mar 16, 2017

abellotti added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 16, 2017

jntullo mentioned this pull request Mar 24, 2017

update options tests to use expect_options_results #14489

Merged

chrisarcand deleted the cloud_credential_options branch November 28, 2017 19:26

Fryguy added the providers/automation label Jan 24, 2020

		@@ -1,2 +1,10 @@
		class ManageIQ::Providers::AnsibleTower::AutomationManager::CloudCredential < ManageIQ::Providers::AutomationManager::Authentication
		def self.credential_types

Add cloud types to authentication options #13951

Add cloud types to authentication options #13951

Conversation

jntullo commented Feb 16, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Feb 16, 2017

jntullo commented Feb 16, 2017

abellotti commented Feb 16, 2017

Fryguy commented Feb 16, 2017

jntullo commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Feb 16, 2017 • edited Loading

Fryguy commented Feb 16, 2017

Fryguy commented Feb 16, 2017 • edited Loading

jntullo commented Feb 16, 2017

Fryguy commented Feb 17, 2017

Fryguy commented Feb 22, 2017

Choose a reason for hiding this comment

jntullo commented Mar 13, 2017

abellotti commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Mar 15, 2017

jntullo commented Mar 15, 2017

h-kataria commented Mar 15, 2017

abellotti commented Mar 15, 2017

miq-bot commented Mar 16, 2017

miq-bot commented Mar 16, 2017

abellotti commented Mar 16, 2017

jntullo commented Feb 16, 2017 •

edited

Loading

Fryguy commented Feb 16, 2017 •

edited

Loading

Fryguy commented Feb 16, 2017 •

edited

Loading