Add cloud creds support for terraform runner

[putmanoj]

• Add cloud credentials classes for embedded-terraform with DDF configuration
• Add cloud credentails classes with authentication connection parameters for terraform-runner

[agrare]

Why call a property security_token if we're just going to store it in auth_key, could you use "auth_key" instead up here ☝️ https://github.com/ManageIQ/manageiq-providers-embedded_terraform/pull/16/files#diff-10eac92f3aa8adb511c059784672e62020e8e6172d8154c295c6a5678c75007fR29-R30

[jrafanie]

Good question. We also have an alias for security_token from auth_key so hopefully we can use auth_key everywhere to keep it consistent.

[agrare]

Yeah I understand having the :label be something specific but wasn't sure what having the internal key being different from the column bought us

[putmanoj]

Just followed the pattern of what is in ansible implemetation - just a copy the same
https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/embedded_ansible/automation_manager/amazon_credential.rb#L52

[jrafanie]

Ok, even better. Let's just use auth_key. I don't think it makes sense to translate one field to another unless we have a good reason.

[agrare]

This can be done in a follow-up

[jrafanie]

yup, I'll do this in a follow up. no need to delay merging on this.

[agrare]

Basically the same question here as above (and looks like for a number of these). Maybe there is a good reason? IDK

[putmanoj]

Again same pattern as in ansible implementation ...

https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/embedded_ansible/automation_manager/google_credential.rb#L54

[agrare]

Right, I don't think we should blindly copy&paste from other code without thinking about it first :)

[jrafanie]

I'll get this for followup.

[jrafanie]

I think something like this does what you're trying to do:

Suggested change

	if %i[host domain project region cacert clientcert clientkey insecure].any? { |opt| attrs.has_key?(opt) }
	attrs[:options] ||= {}
	attrs[:options][:host] = attrs.delete(:host) if attrs.key?(:host)
	attrs[:options][:domain] = attrs.delete(:domain) if attrs.key?(:domain)
	attrs[:options][:project] = attrs.delete(:project) if attrs.key?(:project)
	attrs[:options][:region] = attrs.delete(:region) if attrs.key?(:region)
	attrs[:options][:cacert] = attrs.delete(:cacert) if attrs.key?(:cacert)
	attrs[:options][:clientcert] = attrs.delete(:clientcert) if attrs.key?(:clientcert)
	attrs[:options][:clientkey] = attrs.delete(:clientkey) if attrs.key?(:clientkey)
	attrs[:options][:insecure] = attrs.delete(:insecure) if attrs.key?(:insecure)
	end
	attrs
	unless (attrs.keys & %i[host domain project region cacert clientcert clientkey insecure]).empty?
	attrs.slice!(:host, :domain, :project, :region, :cacert, :clientkey, :insecure)
	end
	{:options => attrs}

[jrafanie]

Are there other values you're concerned about retaining? The slice option will only remove all key/values that aren't in the list so if you have others that aren't in that list, they'll be lost.

[agrare]

I think attrs will have more than just these keys like userid / password so we can't replace the top level hash with just {:options =>
What about something like

options = super.dup
attrs = options.slice!(:host, :domain, :project, :region, :cacert, :clientkey, :insecure)
attrs.merge!(:options => options) unless options.blank?
attrs

attrs will now have the rest of the keys that aren't in the slice! which should be the rest of the top level keys.

[jrafanie]

Note, we have this pattern in params_to_attributes to pull out specific keys from attrs and store them in the :options subkey instead. Perhaps extract a method move_keys_to_options from above.

[jrafanie]

Or not... this one is the most complicated one.

[agrare]

Note, we have this pattern in params_to_attributes to pull out specific keys from attrs and store them in the :options subkey instead. Perhaps extract a method move_keys_to_options from above.

Yeah I wonder if maybe defining accessors would help clean this up? Like

def clientkey
  options && options[:clientkey]
end

def clientkey=(val)
  options ||= {}
  options[:clientkey] = val
end

Then we probably don't need to override params_to_attributes at all. This would be a lot of change maybe something for a followup

[jrafanie]

yup, followup is good

[jrafanie]

Is this the same thing as VERIFY_NONE (when insecure true) vs. VERIFY_PEER (when insecure false)? We call that Verify SSL, SSL Verification or SSL Verify Mode elsewhere.

[putmanoj]

Yes, we can improve the label text ,

[putmanoj]

Though by default insecure is false by default, so is OFF or 'unchecked'.
https://registry.terraform.io/providers/terraform-provider-openstack/openstack/latest/docs#insecure

If we use Verify SSL as label, then meaning changes, ...
the checkbox needs to be 'ON' or 'checked' by default. Just don't know enough DDF yet, to know how to make checkbox 'checked' by default.

[putmanoj]

@jrafanie , any other label suggestion ?

[jrafanie]

I agree, we should use positive terms "Verify SSL" with it checked/enabled by default. We can with check @GilbertCherrie if we're not seeing it "on" by default.

[jrafanie]

We can merge as is and I'll work with @GilbertCherrie if I can't figure out how to correctly switch the logic. I like "verify ssl" true/false as insecure "false" is a double negative and hard to understand.

[jrafanie]

With #3 and ManageIQ/manageiq-ui-classic#9117 (rebased) locally and this PR... here are some screenshots of the credentials we're defining here:

amazon:

azure:

google:

ibm cloud (quite wordy)

openstack 1:

openstack 2:

scm:

vsphere:

[jrafanie]

@putmanoj let ms know if there's any suggested changes you'd like me to work on... Some of these can be things we do in followup PRs. Anything resolved, we can mark resolved and any still remaining, we can choose to not resolve at all or leave them for followup work.

[jrafanie]

I agree, we should use positive terms "Verify SSL" with it checked/enabled by default. We can with check @GilbertCherrie if we're not seeing it "on" by default.

[miq-bot]

Checked commits putmanoj/manageiq-providers-embedded_terraform@816f9c8~...101cd17 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
32 files checked, 0 offenses detected
Everything looks fine. 👍