ManageIQ / manageiq-providers-vmware

ManageIQ plugin for the VMware vSphere and vCloud providers.
Apache License 2.0
22 stars 70 forks source link

Fix Host default_authentication_type #895

Closed agrare closed 6 months ago

agrare commented 7 months ago

As part of the Host Edit Form React Conversion (https://github.com/ManageIQ/manageiq-ui-classic/pull/8608) we changed how host credentials were saved.

Previously we were using https://github.com/ManageIQ/manageiq-ui-classic/pull/8608/files#diff-2f9b6ee0650a1df116a5ec2bc289814be20808a8107e27fd15cffc2394ad416aL199 which set the :default authentication type.

Now as of https://github.com/ManageIQ/manageiq-providers-vmware/pull/851 and https://github.com/ManageIQ/manageiq-providers-vmware/pull/859 the authentication type being set is "ws".

This causes issues with host.authentication_status_ok? (which is used by SSA https://github.com/ManageIQ/manageiq/blob/master/app/models/vm_or_template.rb#L991) because it is looking for a :default authentication.

NOTE we will have upgrade scenarios where existing hosts will have "default" authentication records, we'll need to resolve this likely with a data migration.

kbrock commented 7 months ago

Personal notes:

In https://github.com/ManageIQ/manageiq/pull/22336 we changed the authentication types so we did not have to copy/paste the same code in all child providers. I was concerned that this introduced the bug.

In that code, we leveraged default authentication type. Before the change, the code defaulted the authentication to :ws - which was strange since only vmware implemented :ws. I(?) later added the default concept so we could remove more providers overwriting the code.

But @agrare is suggesting that it may be only loosely related.

I agree with this direction:

agrare commented 7 months ago

I think even though this will require a data migration to truly fix, we should probably backport this because otherwise there is no way to add host credentials for host-based SSA scanning. We could doc that the user has to re-enter the credentials after upgrading to this version in a patch release

agrare commented 7 months ago

I'm going to mark as WIP pending some more testing with the latest commit

miq-bot commented 7 months ago

Checked commits https://github.com/agrare/manageiq-providers-vmware/compare/879a964e873c31de5b5af14e3171a155cd119314~...db0f71478c11147cca926bab5bd5b6c4380f59df with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 2 files checked, 0 offenses detected Everything looks fine. :star:

agrare commented 6 months ago

Okay I've tested adding creds with the "default" type and checked that doing a host ssa scan will use this authentication when it connects.

agrare commented 6 months ago

Taking out of WIP