[V2V] Add default credentials to ansible_playbook method #18724

djberg96 · 2019-05-03T18:58:54Z

In the ansible_playbook method it's possible that an authentication isn't yet directly associated with the conversion host, e.g. a rhev host without a provided ssh key. This modifies the method so that it calls find_credentials as a fallback which will (ultimately) use the authentication of the associated resource.

Without this a undefined method 'userid' for nil:NilClass error could result.

This wasn't caught in the specs because it's a private method. I think our own testing didn't catch it because we were either using openstack or were explicitly providing an ssh key. I personally was just stubbing that method out without checking it. Oops!

This fix also has the added advantage of raising a proper error if it still can't be found for some reason.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1622728

Edit: also fixed an error message bug in the find_credentials method while we were here.

…h conversion host in ansible_playbook method.

djberg96 · 2019-05-03T19:02:18Z

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

jerryk55 · 2019-05-03T19:08:12Z

@djberg96 since you're adding a call to find_credentials() it might be appropriate to fix up a line of redundant code in there assigning to the "msg" variable. Line #255 before this change.

djberg96 · 2019-05-03T19:08:25Z

@miq-bot add_reviewer @agrare

djberg96 · 2019-05-03T19:11:30Z

@jerryk55 oh, good catch, i spaced out on that one. Fixed that while we were here.

jerryk55 · 2019-05-03T19:19:46Z

@djberg96 thanks!

app/models/conversion_host.rb

…he ansible_playbook method to use it.

djberg96 · 2019-05-06T12:25:31Z

@agrare @jerryk55 Since nothing was using the msg argument, I altered it to accept an auth_type instead, which defaults to "v2v", and updated the ansible_playbook method to use it.

Look ok?

jerryk55 · 2019-05-06T13:01:29Z

@djberg96 the only issue I see now is that the ansible_playbook method now has an unused auth_type argument. It can probably be removed. Also I just want to confirm that you no longer wish to have that auth_type = nil argument be the default.

djberg96 · 2019-05-06T13:18:40Z

@jerryk55 Ok, I've updated it so that that auth_type is passed explicitly to find_credentials, which will try to use that first, and fall back to "v2v" as the first default.

jerryk55 · 2019-05-06T13:34:45Z

Ok @djberg96 cool. Looks good!

Fix find credentials for resources where the authentication is associated with the EMS.

djberg96 · 2019-05-06T17:06:14Z

@jerryk55 @agrare I've added another fix that handles cases where it must go through the associated resource. For OSP, it should default to resource -> ems -> authentication instead of resource -> authentication.

app/models/conversion_host.rb

ghost · 2019-05-07T17:28:22Z

I don't think there's any other credentials for conversion host. Le mar. 7 mai 2019 à 13:02, Daniel Berger <[email protected]> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In app/models/conversion_host.rb <#18724 (comment)>: > # - def find_credentials(msg = nil) - authentication = authentication_type('v2v') || - resource.authentication_type('ssh_keypair') || - resource.authentication_type('default') + def find_credentials(auth_type = nil) + authentication = authentication_type(auth_type) || authentication_type('v2v') || authentications.first @fdupont-redhat <https://github.com/fdupont-redhat> can it be anything other than 'v2v', 'ssh_keypair' or 'default'? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18724 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGFAA2E34ZC677FHQFIA5BLPUGYZBANCNFSM4HKWHZUQ> .

agrare · 2019-05-07T17:35:29Z

Okay so we can drop the || authentications.first

djberg96 · 2019-05-07T17:52:23Z

@agrare Updated the current logic. See if that's ok, or if you think those "or" checks should be dropped completely.

app/models/conversion_host.rb

miq-bot · 2019-05-07T18:03:15Z

Checked commits https://github.com/djberg96/manageiq/compare/1840f6cbb873168eadcad3c367b637783a8e10da~...802a13b818a9e78ffcea9f696c91d1f316db6f84 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

ghost · 2019-05-07T18:03:18Z

Nope. We can default to the 'ssh_keypair' and 'default' as they were used in previous version. We will probably drop them in Ivanchuk. Le mar. 7 mai 2019 à 13:53, Adam Grare <[email protected]> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In app/models/conversion_host.rb <#18724 (comment)>: > # - def find_credentials(msg = nil) - authentication = authentication_type('v2v') || - resource.authentication_type('ssh_keypair') || - resource.authentication_type('default') + def find_credentials(auth_type = 'v2v') + authentication = authentication_type(auth_type) || + authentication_type('ssh_keypair') || + authentication_type('default') Wait I thought we just said v2v was the only auth type that could be added? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18724 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGFAA2DFSJM7JRUYG54OUUTPUG6YLANCNFSM4HKWHZUQ> .

agrare · 2019-05-07T18:13:54Z

@fdupont-redhat okay now you've confused me, is v2v auth type the only one supported or not?

ghost · 2019-05-07T18:17:05Z

No, it's not. We support 'v2v' (new, common), as well as 'ssh_keypair' (legacy, OSP) and 'default' (legacy, RHV). Le mar. 7 mai 2019 à 14:14, Adam Grare <[email protected]> a écrit :

…

@fdupont-redhat <https://github.com/fdupont-redhat> okay now you've confused me, is v2v auth type the only one supported or not? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18724 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGFAA2DGRIIWLHIXIYEAUBTPUHBGNANCNFSM4HKWHZUQ> .

agrare · 2019-05-07T18:21:25Z

Okay but on the conversion host v2v was the only option, ssh_keypair and default we're only for the underlying resource

ghost · 2019-05-07T23:32:11Z

You're right. It's a new converged authentication, that makes the graphical configuration easier, and the others, associated to the resource, will disappear in Ivanchuk.

…ook_default_credentials [V2V] Add default credentials to ansible_playbook method (cherry picked from commit a68811e) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1694229

simaishi · 2019-05-08T01:20:11Z

Hammer backport details:

$ git log -1
commit e9beef70e978a6ec75feb376c5cc691e81fb20b1
Author: Adam Grare <[email protected]>
Date:   Tue May 7 19:58:11 2019 -0400

    Merge pull request #18724 from djberg96/conversion_host_ansible_playbook_default_credentials
    
    [V2V] Add default credentials to ansible_playbook method
    
    (cherry picked from commit a68811eee56fc4a7bc632c4b74f8c64c0e5b6ef0)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1694229

…le_playbook_default_credentials" This reverts commit e9beef7. https://bugzilla.redhat.com/show_bug.cgi?id=1694229

simaishi · 2019-05-08T16:48:06Z

Reverted the hammer backport:

commit 282f74ed07d2f851110d9b3d9a5428f1fa7a79ef
Author: Satoe Imaishi <[email protected]>
Date:   Wed May 8 12:46:48 2019 -0400

    Revert "Merge pull request #18724 from djberg96/conversion_host_ansible_playbook_default_credentials"
    
    This reverts commit e9beef70e978a6ec75feb376c5cc691e81fb20b1.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1694229

djberg96 · 2019-05-16T19:54:37Z

@miq-bot remove_label hammer/no

djberg96 · 2019-05-16T19:54:45Z

@miq-bot add_label hammer/yes

…ook_default_credentials [V2V] Add default credentials to ansible_playbook method (cherry picked from commit a68811e) https://bugzilla.redhat.com/show_bug.cgi?id=1712135

simaishi · 2019-05-20T19:01:06Z

Re-backported to hammer branch:

commit 2a0f93eb01710880b45b651317c28693584b378a
Author: Adam Grare <[email protected]>
Date:   Tue May 7 19:58:11 2019 -0400

    Merge pull request #18724 from djberg96/conversion_host_ansible_playbook_default_credentials
    
    [V2V] Add default credentials to ansible_playbook method
    
    (cherry picked from commit a68811eee56fc4a7bc632c4b74f8c64c0e5b6ef0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1712135

Add default credentials if authentication not directly associated wit…

1840f6c

…h conversion host in ansible_playbook method.

miq-bot added bug hammer/yes v2v labels May 3, 2019

miq-bot requested a review from agrare May 3, 2019 19:09

Fixed error message handler in find_credentials.

cee75fb

agrare reviewed May 3, 2019

View reviewed changes

app/models/conversion_host.rb Show resolved Hide resolved

agrare reviewed May 3, 2019

View reviewed changes

app/models/conversion_host.rb Outdated Show resolved Hide resolved

Modify find_credentials so that it accepts an auth_type, and update t…

6b22c60

…he ansible_playbook method to use it.

Explicitly pass auth_type argument to find_credentials.

f4607f2

Fix find credentials for resources where the authentication is associated with the EMS.

Daniel Berger and others added 2 commits May 6, 2019 13:09

Fix stray pipe char.

aa3cde8

Remove useless ||

24b2b45

agrare self-assigned this May 7, 2019

agrare reviewed May 7, 2019

View reviewed changes

app/models/conversion_host.rb Outdated Show resolved Hide resolved

agrare reviewed May 7, 2019

View reviewed changes

app/models/conversion_host.rb Outdated Show resolved Hide resolved

agrare reviewed May 7, 2019

View reviewed changes

app/models/conversion_host.rb Outdated Show resolved Hide resolved

Daniel Berger added 2 commits May 7, 2019 12:52

Simplify the find_credentials method a bit.

b7aa651

Default to v2v auth_type for find_credentials.

4e89c38

agrare reviewed May 7, 2019

View reviewed changes

app/models/conversion_host.rb Outdated Show resolved Hide resolved

Daniel Berger added 2 commits May 7, 2019 13:55

Don't default on first auth type check.

d2cc525

Update comments for find_credentials.

802a13b

agrare approved these changes May 7, 2019

View reviewed changes

agrare merged commit a68811e into ManageIQ:master May 7, 2019

agrare added this to the Sprint 111 Ending May 13, 2019 milestone May 7, 2019

simaishi added hammer/backported and removed hammer/yes labels May 8, 2019

simaishi added a commit that referenced this pull request May 8, 2019

Revert "Merge pull request #18724 from djberg96/conversion_host_ansib…

282f74e

…le_playbook_default_credentials" This reverts commit e9beef7. https://bugzilla.redhat.com/show_bug.cgi?id=1694229

simaishi added hammer/no and removed hammer/backported labels May 8, 2019

djberg96 mentioned this pull request May 16, 2019

[V2V] Have ConversionHost explicitly search authentications for auth type #18748

Merged

miq-bot added hammer/yes and removed hammer/no labels May 16, 2019

simaishi added hammer/backported and removed hammer/yes labels May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2V] Add default credentials to ansible_playbook method #18724

[V2V] Add default credentials to ansible_playbook method #18724

djberg96 commented May 3, 2019 •

edited by simaishi

Loading

djberg96 commented May 3, 2019

jerryk55 commented May 3, 2019

djberg96 commented May 3, 2019

djberg96 commented May 3, 2019

jerryk55 commented May 3, 2019

djberg96 commented May 6, 2019

jerryk55 commented May 6, 2019

djberg96 commented May 6, 2019

jerryk55 commented May 6, 2019

djberg96 commented May 6, 2019

ghost commented May 7, 2019 via email

agrare commented May 7, 2019

djberg96 commented May 7, 2019

miq-bot commented May 7, 2019

ghost commented May 7, 2019 via email

agrare commented May 7, 2019

ghost commented May 7, 2019 via email

agrare commented May 7, 2019

ghost commented May 7, 2019

simaishi commented May 8, 2019

simaishi commented May 8, 2019

djberg96 commented May 16, 2019

djberg96 commented May 16, 2019

simaishi commented May 20, 2019

[V2V] Add default credentials to ansible_playbook method #18724

[V2V] Add default credentials to ansible_playbook method #18724

Conversation

djberg96 commented May 3, 2019 • edited by simaishi Loading

djberg96 commented May 3, 2019

jerryk55 commented May 3, 2019

djberg96 commented May 3, 2019

djberg96 commented May 3, 2019

jerryk55 commented May 3, 2019

djberg96 commented May 6, 2019

jerryk55 commented May 6, 2019

djberg96 commented May 6, 2019

jerryk55 commented May 6, 2019

djberg96 commented May 6, 2019

ghost commented May 7, 2019 via email

agrare commented May 7, 2019

djberg96 commented May 7, 2019

miq-bot commented May 7, 2019

ghost commented May 7, 2019 via email

agrare commented May 7, 2019

ghost commented May 7, 2019 via email

agrare commented May 7, 2019

ghost commented May 7, 2019

simaishi commented May 8, 2019

simaishi commented May 8, 2019

djberg96 commented May 16, 2019

djberg96 commented May 16, 2019

simaishi commented May 20, 2019

djberg96 commented May 3, 2019 •

edited by simaishi

Loading