foreman inventory script does not properly detect IP addresses when satellite6_want_ssh_ansible_host=True

[kdelee]

ISSUE TYPE

• Bug Report

COMPONENT NAME

• API

SUMMARY

We updated the foreman.py script and got https://github.com/ansible/ansible/pull/34169 According to docs, I get to override vars in foreman.ini by providing them in extra vars on source with satellite6_ prepended to front of var: https://docs.ansible.com/ansible-tower/latest/html/userguide/inventories.html#red-hat-satellite-6

If I provide satellite6_want_ssh_ansible_host: True I see NO effect and DO NOT see the expected:

"ansible_ssh_host": "192.168.0.101",

in the vars related to host in inventory

ENVIRONMENT

• AWX version: devel @ 271bd10b475312d586951d5ccd4a3a61b0d26824
• AWX install method: docker on linux
• Ansible version: X.Y.Z
• Operating System:
• Web Browser:

STEPS TO REPRODUCE

Run sync with satellite source with satellite6_want_ssh_ansible_host: True

EXPECTED RESULTS

Get the following value in my vars for host:

"ansible_ssh_host": "192.168.0.101",

ACTUAL RESULTS

No effect, same as not passing it.

ADDITIONAL INFORMATION

Contact me for reproducer satellite

[wenottingham]

It would need special handling, see how other vars are handled (it goes in the other section)

[AlanCoding]

This is now ready to be implemented.

[rmkraus]

This seems to be stale since March. What is the status on this one? Can I help with this in any way?

For anyone that stumbles upon this trying to find out the issue: All of the relevant configuration for the dynamic inventory plugin goes into the [ansible] section. When Tower creates the configuration file when running the inventory script, it puts everything into the [foreman] section. Most dynamic inventory scripts read their config out of a section with the same name as the plugin, so this behavior is usually correct, but not for the Foreman plugin.

[AlanCoding]

Some information goes into [ansible] and some goes into [foreman]. You have to go by the example config file.

Let's identify the exact syntax expected for this issue. The example config is:

https://github.com/keithresar/ansible/blob/devel/contrib/inventory/foreman.ini

this contains

[ansible]
# Whether to populate the ansible_ssh_host variable to explicitly specify the
# connection target.  Only tested with Katello (Red Hat Satellite).
# If the foreman 'ip' fact exists then the ansible_ssh_host varibale is populated
# to permit connections where DNS resolution fails.
want_ansible_ssh_host = False

But the parameter needs to get passed through the inventory logic.

This new parameter is identical in nature to satellite6_want_hostcollections

https://github.com/ansible/awx/blob/b5fa1606bd14bc96d6f884dfee15bd38d06d8441/awx/main/models/inventory.py#L2594-L2616

I don't want to do any refactoring here. We have one new parameter to add, which should follow the same patterns as existing parameters.

[rmkraus]

I see, thanks for pointing me in that direction. That works.

What do you think is the best approach for documenting this?

[rmkraus]

Also, what are your thoughts on enabling control of the rich_params variable as well?

[AlanCoding]

it's in foreman.ini, so 👍 to adding that in this pattern too.

[elyezer]

I was trying to verify this and need some guidance. The original issue states that AWX should support the new variables added to the foreman.ini and I tried to run an inventory sync with the following vars:

satellite6_want_ansible_ssh_host: true
satellite6_group_prefix: sat6_
satellite6_want_facts: false

And I got proper prefix and didn't get the facts as expected but the satellite6_want_ansible_ssh_host seems not to be working. This is what the vars of a given host look like after the sync:

foreman:
  host_group: null
  id: 2
  ipv4: <ip redcted>
  location: Default Location
  name: <name redacted>
  organization: Default Organization
  smart_proxies:
    - <redacted proxy>
  subnet: null
foreman_params: {}

Am I doing something wrong?

[elyezer]

I tried couple more times and I still don't see the ansible_ssh_host being set when satellite6_want_ansible_ssh_host is set to true on the inventory source:

Then after a successful inventory source sync the ansible_ssh_host is not present on the host's variables.

[ryanpetrello]

@AlanCoding @jladdjr @jainnikhil30 any of you have any feedback or thoughts on this? Sounds like this one maybe isn't squashed just yet?

[AlanCoding]

Looking back at:

https://github.com/ansible/awx/issues/2916#issuecomment-584834026

Made me think, cross-referencing with:

https://github.com/ansible/ansible/blob/devel/contrib/inventory/foreman.py#L642

                if self.want_ansible_ssh_host and 'ip' in self.cache[hostname]:

Indeed, it is true that those hostvars given do not have "ip" in them. It has ipv4 instead. So that speculates some possibilities... does this server have newer stuff that the script needs to be updated for? Was the param want_IPv4 also set, and does that have an incompatibility with this?

[ryanpetrello]

That's what it is @AlanCoding, it's just a bug in the script:

Honestly, there's probably not going to be much we can do about this outside of our upcoming move to the plugin.

[ryanpetrello]

This (hypothetical) diff to the script fixes it for me:

diff --git a/awx/plugins/inventory/foreman.py b/awx/plugins/inventory/foreman.py
index c3f97710d2..d65dcecfed 100755
--- a/awx/plugins/inventory/foreman.py
+++ b/awx/plugins/inventory/foreman.py
@@ -644,8 +644,10 @@ class ForemanInventory(object):
                     'foreman': self.cache[hostname],
                     'foreman_params': self.params[hostname],
                 }
-                if self.want_ansible_ssh_host and 'ip' in self.cache[hostname]:
-                    self.inventory['_meta']['hostvars'][hostname]['ansible_ssh_host'] = self.cache[hostname]['ip']
+                for key in ('ip', 'ipv4', 'ipv6'):
+                    if self.want_ansible_ssh_host and key in self.cache[hostname]:
+                        self.inventory['_meta']['hostvars'][hostname]['ansible_ssh_host'] = self.cache[hostname][key]
+                        break
                 if self.want_facts:
                     self.inventory['_meta']['hostvars'][hostname]['foreman_facts'] = self.facts[hostname]

[ryanpetrello]

@ares @sjha4 either of you have any thoughts on this?

[jladdjr]

If that change makes sense to others, sounds good to me. I confirmed that @elyezer's instance doesn't have any instances with the ip field set. When I changed the snippet @ryanpetrello mentioned to ipv4, the hostvars were populated.

Echoing @ryanpetrello 's comment, with the script about to be replaced with the plugin, we should be thinking about that, too. If @ryanpetrello's change looks good, we should apply it to the plugin as well (which also only keys off of the ip field)

[sjha4]

@ryanpetrello : Although I don't have much context around this, the change in the snippet above looks good to me. :+1:

[ryanpetrello]

@jladdjr if you want to turn this into a PR for the plugin, that would awesome. Personally, I wouldn't bother with the script at this point given our plans to move away soon.

[AlanCoding]

If @ryanpetrello's change looks good, we should apply it to the plugin as well (which also only keys off of the ip field)

Good find there, I agree with Ryan's comment, that should be patched in the plugin where you linked.

[jladdjr]

@AlanCoding @ryanpetrello - patched ^

[jladdjr]

The foreman plugin has been patched. When awx switches to using the plugin, it should pick up the correct behavior.

@kdelee - seems like we can close this now.

[elyezer]

@jladdjr is there a separate issue to track the switch to the plugin? I think it would be good to link here.

[ryanpetrello]

@elyezer @jladdjr probably this one?

https://github.com/ansible/awx/issues/3509

[jladdjr]

Yep, that's it 👍

[kdelee]

Closing this as a duplicate, because fixing this is equivalent to fixing https://github.com/ansible/awx/issues/3509. When the plugin lands, we should test this