netbox-community / ansible_modules

NetBox modules for Ansible using Ansible Collections
GNU General Public License v3.0
333 stars 216 forks source link

Bug Report: Compose not picking up inventory_hostname field #439

Open dmavrommatis opened 3 years ago

dmavrommatis commented 3 years ago
ISSUE TYPE
SOFTWARE VERSIONS
Ansible:

v2.9.12

Netbox:

v2.9.4

Collection:

v2.1.0

SUMMARY

When using the compose functionality to add a field the inventory_hostname variable is not accessible.

Usually this is needed in cases that you want to add a field based on the inventory_hostname mutations and this is supported by the builtin constructed plugin.

STEPS TO REPRODUCE

Add a compose field in the netbox configuration

compose:
  compose_field: inventory_hostname

Then run ansible-inventory to see if the field is added to the host variables.

ansible-inventory -vvvv --host name-of-host
EXPECTED RESULTS
{
    "ansible_host": "x.x.x.x",
    "compose_field": "name-of-host"
    "custom_fields": {},
    ...
}
ACTUAL RESULTS
{
    "ansible_host": "x.x.x.x",
    "custom_fields": {},
    ...
}
PROPOSED SOLUTION

https://github.com/dmavrommatis/ansible_modules/commit/90eaa238324a6221a0b7f8d4e66c0d0311924960

If you think it's the correct approach I can open a PR with the change.

FragmentedPacket commented 3 years ago

I'm curious as to what the use case is for this? The inventory hostname is available when running playbooks so curious as to what this functionality offers.

dmavrommatis commented 3 years ago

I'm curious as to what the use case is for this? The inventory hostname is available when running playbooks so curious as to what this functionality offers.

To create vars based on other fields or mutation of other fields.

I already use https://docs.ansible.com/ansible/devel/collections/ansible/builtin/constructed_inventory.html with a compose that based on the inventory_hostname it adds some fields. For example, you have delivery-service.digital-ocean.example.com so you can add:

 compose:
    server_type: inventory_hostname | regex_replace('[0-9]+\..*', '')
    facility: inventory_hostname | regex_replace('^[^.]+.|.example.com', '')

to get server_type: delivery-service and facility: digital-ocean.

Moving to the netbox_inventory plugin I would expect to get the same functionality which I don't as of right now.

FragmentedPacket commented 3 years ago

Ok awesome. I think your implementation should work.

My only ask at this time is to make sure to update one of the integration tests to test this compose functionality.

dmavrommatis commented 3 years ago

OK, I will add an integration test and open a PR. Thanks

dmavrommatis commented 3 years ago

For anyone interested, you can achieve the functionality by running the constructed plugin afterwards instead of using the compose functionality from this module.

Feel free to close this issue if that is a good enough solution and having this feature don't make sense.

mathieu-mp commented 3 years ago

I was expecting a similar behavior for any hostvar, and had to hack nb_inventory.py but @dmavrommatis made it in a much more elegant way ! I would really love to have this feature

For anyone interested, you can achieve the functionality by running the constructed plugin afterwards instead of using the compose functionality from this module.

Please, would you explain how to do so ? Using awx/tower, I fear that it would require to add an inventory source. As only one inventory source can auto-update, that would add a risk that the inventory remains in an "intermediate" state.

mathieu-mp commented 3 years ago

I tested @dmavrommatis code, and I would like to discuss it.

As-is, it erases all known variables built by the inventory plugin, such as id, detailed device_type, or primary_ip objects. I was using such detailed data that I cannot find in hostvars, as hostvars are reduced in details. So, it breaks my inventory source. It might break other people's use of compose, so it doesn't look right as-is.

On one hand, it looks great to retrieve the hostvars before getting to compose, for ease of use, so I would just suggest to get it to host['hostvars'] instead of host itself, and the users would just use hostvars in compose.

On the other hand, all hostvars data is available in the host object and users just need to know how to find it. I didn't. Thanks to @dmavrommatis code, I understood how to print the whole host object just before compose, I had a look at the full object built by the inventory source, and that helped my to identify the fields I needed to compose my variables. So we could just choose not to change the code, and instead inform the user about the variables that are available at the compose step. Should we do it through the docs ?

mathieu-mp commented 3 years ago

On the other hand, all hostvars data is available in the host object and users just need to know how to find it...

I realized @dmavrommatis retrieves host vars and group vars, so I am wrong assuming "all host vars data is available in the host object" because the host object doesn't contain group vars.
If we do not merge @dmavrommatis code, can we access group cars during the compose step ? And how ?

dmavrommatis commented 3 years ago

For anyone interested, you can achieve the functionality by running the constructed plugin afterwards instead of using the compose functionality from this module.

Please, would you explain how to do so ?

So playing around with all the inventory plugins I realized that they run in order based on their names. So what I did is to rename the inventories to 100-netbox.config and 200-constructed.config. Then, I only use netbox.netbox.nb_inventory to fetch the inventory and then do all the compose/groups/etc with the constructed plugin instead.

bluikko commented 3 years ago

The compose feature used to work perfectly long, long time ago. As it stands today it is quite useless. Sadly.

Now only a few fields from NetBox work - for example tags and last_updated. Even the examples for compose are broken now, only one out of three actually works! For example rack is unavailable so rack.display_name as listed in the example does not work.

Edit: I was looking at a virtual machine earlier and it does not have rack available for compose. tags seem to be available anywhere, as expected, but for example location doesn't work on either device or virtual machine. Am I just doing something else wrong, too?! I am sure it used to work easily long time before...

Edit: Indeed I was doing it wrong - VMs do not have locations. They only have region / site_group / site. It was a mistake to start use the location feature without checking this first. But for some reason site is not available for compose even though the API does return this information for virtual-machines?

bluikko commented 3 years ago

For anyone interested, you can achieve the functionality by running the constructed plugin afterwards instead of using the compose functionality from this module.

How so? The description of compose:

List of custom ansible host vars to create from the device object fetched from NetBox

It should be simple to create host vars with compose, from NetBox data of the relevant host. Another plugin would need to query NetBox for this data.

It does look like the compose feature is more or less broken and does not really give access to the NetBox data.