Fix Key Pairs from refresh being returned with provider AuthPrivateKeys #18633
Conversation
To prevent cloud_manager auth_key_pairs from being returned with other AuthPrivateKeys change the parent class to Authentication
|
👍 |
app/models/manageiq/providers/cloud_manager/provision/options_helper.rb
Outdated
Show resolved
Hide resolved
|
If we are sure we are not using anything from the base class, then this is a good change, conceptually...though I have one issue with the implementation. |
|
Oh, additionally, you are going to need a data migration for upgrading users. |
|
@agrare Can you also verify the UI side of this (I think it's in the Cloud EMS summary page)? I know that keypairs are shown in the UI, and I think they do some dance to only show the "right" ones. |
If we're updating the parent class? What would we change, not the |
|
Checked commits agrare/manageiq@321d9d3~...e79e13a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/manageiq/providers/cloud_manager.rb
|
| @@ -4,7 +4,7 @@ def dest_availability_zone | |||
| end | |||
|
|
|||
| def guest_access_key_pair | |||
| @guest_access_key_pair ||= AuthPrivateKey.find_by(:id => get_option(:guest_access_key_pair)) | |||
| @guest_access_key_pair ||= ManageIQ::Providers::CloudManager::AuthKeyPair.find_by(:id => get_option(:guest_access_key_pair)) | |||
There was a problem hiding this comment.
I'm good with this for now, but I don't understand why this doesn't do a find_by_id off of the EMS (in this entire file). @gmcculloug ? Doesn't this allow a provision run to get at a different EMS's records?
There was a problem hiding this comment.
The pattern setup in the options_helper.rb file has always been to lookup the record by the ID on the model, but we it could definitely be scoped down where possible. Might be outside of the scope of this PR.
For this value, if the wrong instance is pulled we are only sending the name to the provider.
https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/cloud_manager/provision/cloning.rb#L21
|
I was thinking we would have to change the polymorphic relationships where the base class is stored in cols like resource_type. However, I see you only changed a "middle" class in the hierarchy not the leaf nor the base, so yeah, no DB migration needed. |
|
👍 yeah now I see what you meant |
@Fryguy I didn't see keypairs on the summary page anymore (checked on master as well) but there is a key pairs page under cloud_manager and that still works great |
|
Confirmed that this fixes the issue with the GCE provider |
AuthPrivateKeys are used to connect to providers,ManageIQ::Provider::CloudManager::AuthKeyPairs were being returned with these because they inherited from that class without actually using anything from it.Update the CloudManager::AuthKeyPairs to inherit from the base Authentications class