netbox-community / ansible_modules

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

[Bug]: int not of type integer if jinja2 expressions are used in custom attributes #985

Open agowa opened 1 year ago

agowa commented 1 year ago

Ansible NetBox Collection version

v3.12.0

Ansible version

ansible [core 2.14.3]
  config file = /workspaces/ansible.cfg
  configured module search path = ['/home/vscode/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /workspaces/.pyenv/versions/3.10.4/envs/ansible.venv/lib/python3.10/site-packages/ansible
  ansible collection location = /workspaces/collections
  executable location = /workspaces/.pyenv/versions/ansible.venv/bin/ansible
  python version = 3.10.4 (main, Mar  2 2023, 17:18:28) [GCC 10.2.1 20210110] (/workspaces/.pyenv/versions/3.10.4/envs/ansible.venv/bin/python3.10)
  jinja version = 3.1.2
  libyaml = True

NetBox version

v3.4.5

Python version

3.10

Steps to Reproduce

local_action:
  module: netbox.netbox.netbox_vm_interface
  state: present
  data:
...
    custom_fields:
      some_integer_field: "{{ some_ansible_variable|default(omit) }}"

with: some_ansible_variable: 3

Expected Behavior

correct serialization of custom fields using their correct datatype.

Observed Behavior

API Post request is formed incorrectly with "3" as string instead of integer. Doing a "type_debug" in the playbook outputs that it is of type int. But when I try to use it in the netbox.netbox.netbox_vm_interface module for example it gets serialized as string and an error message of "msg": "{\"__all__\":[\"Invalid value for custom field 'some_integer_field': Value must be an integer.\"]}" is printed

If I add an explicit int cast in _normalize_data, then the custom fields are also sent correctly. I don't know how to write a generic version that would query the custom field type and serialize it correctly though. Also I don't know if there is a possibility to use the same datatype ansible outputs for ansible.builtin.type_debug filter.

Edit: I found a workaround. To set [defaults] jinja2_native=True within the ansible.cfg, but as it is not required for other modules, there may be an attribute to set within the module.

sc68cal commented 1 year ago

Did you try casting it to int?

some_integer_field: "{{ some_ansible_variable | int or default(omit) }}"
lasse-jorgensen commented 1 year ago

Did you try casting it to int?

some_integer_field: "{{ some_ansible_variable | int or default(omit) }}"

I have encountered the same issue and I can confirm that casting a variable to int does not work. I have tested sending custom_fields using a variable which I casted to to_json which did not work.

"{{ customfields | to_json }}"

I also tried casting it to from_json again just to test it out, which also did not work.

"{{ customfields | to_json | from_json }}"
sc68cal commented 1 year ago

OK. I think at the very least, we should add the note about using jinja2_native to our documentation about custom types in the meantime.

We probably need to query the custom_field API and get the types, and use that to then cast each custom field passed in to the correct type.

ThomasADavis commented 1 year ago

Ansible jinja filter always return strings, unless you set jinja2_native=true in your ansible.cfg or do ANSIBLE_JINJA2_NATIVE=1 ansible-playbook This will break async however, until you get to v2.14.5..

I ran into this when using id's against netbox, it wants them to be ints not strings.

ie, id="1" is not the same as id=1

agowa commented 1 year ago

Hi @ThomasADavis that's just a workaround, the netbox module itself knows which datatype the netbox API expects (or could query the schema for custom objects GET /extras/custom-fields/ and GET /extras/custom-fields/{id}). So this module could (and like many in the ansible matrix channel pointed out even should) do type conversion. So if the field is int, it should always do a int(value_privided_by_ansible) before passing it off to the API.

Probably would also prevent injection attacks in some cases. But at a bare minimum, it would improve UX and also provide better error messages when users provide incorrect data.

(Btw, I just noticed the API calls I just mentioned also return any defined validation min/max/regex, so technically we could do the validation before even calling the actual API)

ThomasADavis commented 1 year ago

That is true, I ran into this when doing bulk updates via URL to netbox in ansible - it much faster to do bulk update of a large number of interfaces than do a loop with the netbox module. There is also other places in the modules that look/ask for a id, and I'm not sure if it's doing right conversion there either; ie, if it's a object_id, it's always going to be an int..

To get the bulk updates to work correctly, I had to turn on the jinja native type, which promptly broke ansible_async (another collection used this feature). So the modules need to be able to work no matter what the jinja_native setting is.

agowa commented 1 year ago

You can have a look at the API docs yourself https://demo.netbox.dev/api/docs

But speaking of it, I probably should have a look at what happened to that proposal from a dev over at Oracle Cloud from a few years ago that talked about writing a generic "swagger to ansible module" converter tool.

Edit: apparently nothing until now: https://groups.google.com/g/ansible-devel/c/eEx3lEGnQKQ

edvinaskairys commented 1 year ago

even when running python filter, which converts the data from string to int (confirmed with type_debug) - the module doesn't recognize it as a integer.

The only workaround is inja2_native=true in ansible.cfg. But the problem comes - how to do it if i'm using AWX ?

rodvand commented 1 year ago

The only workaround is inja2_native=true in ansible.cfg. But the problem comes - how to do it if i'm using AWX ?

Add ansible.cfg with the option to your project root.

cpund commented 1 year ago

The workaround I used when encountering this issue is setting my custom field to text, and then setting a regex validation field of ^\d+$, which essentially forces it to be an integer.