[EmbeddedAnsible::Credential] use id for manager_ref

[NickLaMuro]

The manager_ref column is used by the authentications table to reference a external resource id for a given provider. Not required for all uses, but generally used by provider authentications so we have a mapping to a unique id on the provider side when doing a refresh so we aren't clobbering existing records, allowing add/update/delete operations to function properly when doing a refresh.

With EmbeddedAnsible now not using ansible tower, this is no longer needed by default, but there are certain places where it is still being called, like ServiceTemplateAnsiblePlaybook.build_parameter_list via the native_ref method.

With the change to EmbeddedAnsible::AutomationManager::Credential's native_ref method to include a Integer type cast that was ported over from the manageiq-provider-ansible_tower repo (not part of the original POC by Jason), this caused errors with nil manager_ref ids when trying to create a new ServiceTemplateAnsiblePlaybook and using the default credential. The purpose of this type casting originally was to enforce a validation that was expected on the Ansible Tower side that required Integer values when communicating through it's API and referencing primary keys:

https://github.com/ManageIQ/manageiq-providers-ansible_tower/blob/487b4aef/spec/support/ansible_shared/automation_manager/credential.rb#L8

Since manager_ref no longer has value to EmbeddedAnsible, as it isn't tied to a tower instance, we can just assign the id value to manager_ref on create to give it a unique value that can be used in places where searching by native_ref or manager_ref is required.

Also, the uses of native_ref in our codebase overall are limited:

https://github.com/search?q=org%3AManageIQ+native_ref&type=Code

(10 code locations across the ManageIQ org at the time of writing)

Links

• Merged code adding the bug: Use ansible-runner in EmbeddedAnsible #18687

Steps for Testing/QA

• Enable EmbeddedAnsible on an appliance
• Add a repository
• Create a catalog and try adding an Ansible Playbook catalog item

On master, this will fail with:

Could not create Service Template - can't convert nil into Integer

With this fix, it will create the Service Template normally, and display:

Catalog Item <<name>> was added

[NickLaMuro]

@miq-bot add_label embedded ansible, bug, seeding

cc @carbonin @Fryguy

[miq-bot]

@NickLaMuro Cannot apply the following label because they are not recognized: seeding

[carbonin]

@NickLaMuro What's the plan for when we create other credentials in #18870. Are we also setting it to the manager_id there?

Also, what is is being used for? It is meaningless now, so maybe we would have a better time removing the users rather than fudging it to something that looks just close enough to cause a bug that's insanely difficult to find. Plus it's easy to find the users of this if we blow up.

[carbonin]

Lol just saw your comment #18870 (comment)

[NickLaMuro]

Also, what is is being used for?

Honestly, I don't even know the specifics of where it is all being used, and if those places are possibly shared between EmbeddedAnsible and the Ansible Tower provider. Instead of trying to find that out, I went with this option for now, since even the original functionality of the POC is busted at the moment.

[carbonin]

if those places are possibly shared between EmbeddedAnsible and the Ansible Tower provider

Ah, right, forgot about that. I guess this is fine then

Changed my mind

[Fryguy]

Yeah, so I'm starting to question the purpose of the Integer casting. That seems like something that is unnecessary, as any provider of the AutomationManager variety could bring, say, GUIDs as their ref. If we can drop the Integer casting, that may also just solve the issue.

On that note, if we don't care about the actual content of manager_ref and can't leave nil in, then I would just use SecureRandom.uuid.

[NickLaMuro]

On that note, if we don't care about the actual content of manager_ref and can't leave nil in, then I would just use SecureRandom.uuid.

SecureRandom.uuid doesn't even work with Integer()...

$ irb
irb(main):001:0> require 'securerandom'
=> true
irb(main):002:0> SecureRandom.uuid
=> "ec00c825-1e75-436b-a615-be14de273c02"
irb(main):003:0> Integer(SecureRandom.uuid)
ArgumentError: invalid value for Integer(): "36dd4dfa-dea8-45de-af5a-6d183dbc5b25"
        from (irb):3:in `Integer'
        from (irb):3
        from /Users/nicklamuro/.irbrc:126:in `evaluate'
        from /Users/nicklamuro/.rubies/ruby-2.4.2/bin/irb:11:in `<main>'
irb(main):004:0>

If we can drop the Integer casting, that may also just solve the issue...

@Fryguy @carbonin I am trying to make this the most atomic change possible, and removing the Interger() call will then cause some specs to fail, because I know I brought some specs over that tested just that. Not trying to defend those specs, but I have been in "hot fix mode" all week, and I am really trying to make this a surgical fix, and what is being suggested is not that.

See below.

[NickLaMuro]

For the record, this is the second time I have been made aware of this issue, and have only just been able to get around to diagnosing and fixing it.

https://gitter.im/ManageIQ/api?at=5d0a0fe4bb789747fb3e2ad2

There are people currently trying to make use of this feature, and currently can't. I am all for figuring this out in a follow up, but seems like we are delaying a simple fix for "trying to get it right the first time", and by my bug fix track record this week (many probably self induced), we are already not doing that

See below.

[NickLaMuro]

@Fryguy @carbonin IGNORE ME!!1!

After finally eating some food (undoing the "hangry") and doing a short bout of greping, it turns out I am a dirty liar, and didn't port the specs I thought I did:

https://github.com/ManageIQ/manageiq-providers-ansible_tower/blob/487b4aef6/spec/support/ansible_shared/automation_manager/credential.rb#L10

And after looking doing a quick search on the org:

https://github.com/search?q=org%3AManageIQ+native_ref&type=Code

The usage of native_ref is super minimal. So, I think native_ref can just be safely deleted. Rebasing in master and checking a few things but will push that change shortly.

Sorry for the freak out. 😞

[NickLaMuro]

Dammit... I knew there were some specs I needed to fix... bah, give me a minute...

[NickLaMuro]

Okay, re-worked this again based off the suggestion from @carbonin . The code base uses native_ref/manager_ref as an id to find credentials on when creating templates, so we don't want to loose that reference if it has any values, but that also does mean this needs to be a unique value.

Changed the code to be a after_create to "double save" the record create and update the manager_ref to the value of id on create only. id should never change, so this should work out, assuming no-one circumvents model callbacks. Also fixed the specs to work off this new change.

[miq-bot]

Checked commit NickLaMuro@73fa537 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb

• ❗ - Line 30, Col 24 - Style/RedundantSelf - Redundant self detected.

[carbonin]

Looks good to me.

This leaves us open to be able to remove the uses of manager ref if it's somewhere embedded ansible specific in a followup.

[carbonin]

So I assume we should write up a migration to change the manager ref to the id after moving the credential fields over from the AWX database, right? Otherwise we could have duplicates.

[NickLaMuro]

@carbonin yeah, I think that makes sense. Not sure we would have duplicates, but I think after migrating, the credentials won't get added to the template as expected... I think...

[carbonin]

What I mean by duplicates is that we could have a migrated credential with manager_ref "10", but also have a new credential with id 10 and therefore manager_ref "10". Right? I'm not sure what type of havoc this would cause, but I want to avoid it 😅