[V2V] Refactor ConversionHost to use AuthenticationMixin

[djberg96]

Currently there's a lot of hand-crafted ssh handling using the MiqSshUtil library baked into the ConversionHost model that is tied to specific providers. I thought we should leverage the AuthenticationMixin to take advantage of its features.

Specifically, this would give use a direct association between conversion hosts and authentications. This would be beneficial for certain providers, e.g. openstack, where we are currently using credentials set at the provider level. This way, we can set it on a per-resource level for all provider types by setting the authentication on the conversion host instance itself.

This will also be necessary for the UI where users are allowed to upload their own ssh keys.

https://bugzilla.redhat.com/show_bug.cgi?id=1673729
https://bugzilla.redhat.com/show_bug.cgi?id=1695861

[djberg96]

@fdupont-redhat @agrare Just looking for some feedback here to see if it makes sense to leverage the AuthenticationMixin instead of the current code, while seems a bit clumsy to me.

[agrare]

Hm I think we were relying on the fact that the underlying resource had the authentication not the conversion host? And a Vm and a Host both include AuthenticationMixin afaik

[ghost]

We don't exactly rely on the resource authentications. In the case of OpenStack for example, the authentication is associated with the provider, forcing us to have a single key for all conversion hosts of the provider. It was implemented this way to simplify the UX, as it only required to expose the ssh_keypair authentication for the provider. It might be a good idea to revisit this topic with advanced UI for conversion host.

[miq-bot]

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

[miq-bot]

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

[miq-bot]

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

[djberg96]

@agrare Made a few changes. First, it will now use auth_type if present, using the mixed-in authentication_type method. To make that work properly however, I had to modify the AuthenticationMixin by adding a authentication_private_keys method (surprisingly not already present) and then modifying available_authentications to include them.

Second, I came around to the idea that iterating over every authentication was not necessary as there should only ever be one associated authentication. Plus, it simplified the code.

Third, an AuthPrivateKey is automatically associated with the conversion host if the conversion_host_ssh_private_key param is found (originally just ssh_key, but we're changing it).

I've updated the specs, too. Please let me know if there are any other issues and/or if you want me to squash these commits.

[mturley]

This one should also be hammer/yes and listed in https://bugzilla.redhat.com/show_bug.cgi?id=1622728

[ghost]

@miq-bot add_label transformation, enhancement, hammer/yes

[miq-bot]

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

[djberg96]

@agrare I think I've addressed your concerns. The only question mark is the available_authentications that strangely was limited to openstack infra authkeypairs. IMO my update is more sensical.

[djberg96]

@agrare, just realized there's a minor issue i need to work out for cases where there's no associated authentication.

[djberg96]

@agrare ok, I had to update the rescue handler because otherwise we could get a false positive for connect_ssh. I added some more specs to cover that.

[miq-bot]

Checked commits https://github.com/djberg96/manageiq/compare/5bc0dc517fdc68788d16e8d00a63f3dd6a7bf2eb~...002f7b4653511403f4eb6ca1be99b11ac2a73e74 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 4 offenses detected

app/models/conversion_host.rb

• ⚠️ - Line 78, Col 3 - Lint/RescueException - Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?
• ❗ - Line 11, Col 5 - Layout/AlignParameters - Align the parameters of a method call if they span more than one line.
• ❗ - Line 217, Col 7 - Layout/MultilineOperationIndentation - Align the operands of an expression in an assignment spanning multiple lines.
• ❗ - Line 218, Col 7 - Layout/MultilineOperationIndentation - Align the operands of an expression in an assignment spanning multiple lines.

[agrare]

LGTM

[agrare]

@djberg96 given the issues this causes on other providers I'm going to mark this refactoring hammer/no

[Fryguy]

If I recall, this was because in public clouds we also store the inventory-refreshed keypairs from the providers in the authentications table, and those happen to also be AuthKeyPair. Querying your key_pairs from the base class thus brings back both MIQ-owned and provider-owned, which is generally not wanted.

What really needs to happen is that we need to either a) move inventory-based key-pairs into a different table or b) create a separate intermediate class.

[agrare]

@Fryguy PR added here #18633