Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nmcli] Fix issue with argument conversion for run_command #58115

Open

Conversation

@marcin-sucharski
Copy link

commented Jun 20, 2019

SUMMARY

Changes in this PR are fixing the issue where nmcli module would throw an error when executing self.module.run_command. Arguments passed to run_command are expected to be a list of strings (or single string). In the case of nmcli module, sometimes integer value was present in the list which caused run_command to fail.

I've also noticed that there are references to undefined variable self.vxlanid. I've replaced them with self.vxlan_id.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

nmcli

ADDITIONAL INFORMATION

I've noticed this bug when running following task:

- name: Add bridge connection for virtual machines.
  nmcli:
    type: bridge
    autoconnect: no
    conn_name: '{{ qemu_network_manager_bridge_name }}'
    ifname: '{{ qemu_network_manager_bridge_name }}'
    ip4: '{{ qemu_network_manager_bridge_ip4 }}'
    stp: no
    state: present

And it gave me following error:

TASK [qemu : Add bridge connection for virtual machines.] *********************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: TypeError: expected str, bytes or os.PathLike object, not int
fatal: [host]: FAILED! => changed=false 
  module_stderr: |-
    Traceback (most recent call last):
      File "<stdin>", line 114, in <module>
      File "<stdin>", line 106, in _ansiballz_main
      File "<stdin>", line 49, in invoke_module
      File "/usr/lib64/python3.6/imp.py", line 235, in load_module
        return load_source(name, filename, file)
      File "/usr/lib64/python3.6/imp.py", line 170, in load_source
        module = _exec(spec, sys.modules[name])
      File "<frozen importlib._bootstrap>", line 618, in _exec
      File "<frozen importlib._bootstrap_external>", line 678, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/tmp/ansible_nmcli_payload_oi1b0db1/__main__.py", line 1549, in <module>
      File "/tmp/ansible_nmcli_payload_oi1b0db1/__main__.py", line 1527, in main
      File "/tmp/ansible_nmcli_payload_oi1b0db1/__main__.py", line 1425, in modify_connection
      File "/tmp/ansible_nmcli_payload_oi1b0db1/__main__.py", line 682, in execute_command
      File "/tmp/ansible_nmcli_payload_oi1b0db1/ansible_nmcli_payload.zip/ansible/module_utils/basic.py", line 2477, in run_command
      File "/tmp/ansible_nmcli_payload_oi1b0db1/ansible_nmcli_payload.zip/ansible/module_utils/basic.py", line 2477, in <listcomp>
      File "/usr/lib/python-exec/python3.6/../../../lib64/python3.6/posixpath.py", line 281, in expandvars
        path = os.fspath(path)
    TypeError: expected str, bytes or os.PathLike object, not int
  module_stdout: ''
  msg: |-
    MODULE FAILURE
    See stdout/stderr for the exact error
  rc: 1

I'm using Ansible 2.8.0.

@marcin-sucharski marcin-sucharski changed the title Fix issue with argument conversion for run_command [nmcli] Fix issue with argument conversion for run_command Jun 20, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

@bcoca
Copy link
Member

left a comment

first, never use str use to_ functions we provide (in this case to_text is the appropriate one), 2nd i would argue that you should not do this on each value, but ensure that all elements in the list in run_command are strings.

@ansibot ansibot added needs_revision and removed core_review labels Jun 20, 2019

@marcin-sucharski marcin-sucharski force-pushed the marcin-sucharski:fix/module-nmcli-argument-conversion branch 5 times, most recently from 9b31d63 to a7b0a9a Jun 22, 2019

@marcin-sucharski

This comment has been minimized.

Copy link
Author

commented Jun 23, 2019

@bcoca Thanks for your feedback!

I've replaced str with to_text. I've decided to do this only on the values which are expected to be non-string and will correctly convert using to_text (boolean, for example, won't convert correctly).

For this reason (correctness) I think this should be done for each value, not for all of them. However, if it is preffered, I will change the implementation to one that will do this for all arguments.

Also I've added to_text with nonstring='strict' to execute_command in nmcli module to make sure that all arguments passed to self.module.run_command are strings (nonstring='strict' will protect from unexpected conversions). I tried to add this conversion with strict to module.run_command, but some tests started to fail and I do not know Ansible codebase good enough to fix this in reasonable time.

@ansibot ansibot added the stale_ci label Jul 1, 2019

@ansibot ansibot added the stale_review label Jul 9, 2019

@marcin-sucharski marcin-sucharski force-pushed the marcin-sucharski:fix/module-nmcli-argument-conversion branch from a7b0a9a to 275d7d9 Jul 11, 2019

@ansibot ansibot removed the stale_ci label Jul 11, 2019

@ansibot ansibot removed the stale_review label Jul 11, 2019

@@ -1002,7 +1002,7 @@ def modify_connection_ethernet(self, conn_type='ethernet'):
'autoconnect': self.bool_to_string(self.autoconnect),
'ipv4.dns-search': self.dns4_search,
'ipv6.dns-search': self.dns6_search,
'802-3-ethernet.mtu': self.mtu,
'802-3-ethernet.mtu': str(self.mtu),

This comment has been minimized.

Copy link
@Akasurde

Akasurde Jul 16, 2019

Member

How about adding to_text(value) in next loop where we are iterating on options ?

        for key, value in options.items():
            if value is not None:
                if key == '802-3-ethernet.mtu' and conn_type != 'ethernet':
                    continue
                cmd.extend([key, to_text(value)])

This comment has been minimized.

Copy link
@polatsinan

polatsinan Jul 16, 2019

Importing the to_text function

from ansible.module_utils._text import to_text

and replacing

        for key, value in options.items():
            if value is not None:
                cmd.extend([key, value])

with

        for key, value in options.items():
            if value is not None:
                cmd.extend([key, to_text(value)])

did solve issue #59095 for me. I however don't know what the consequences are.

This comment has been minimized.

Copy link
@marcin-sucharski

marcin-sucharski Jul 16, 2019

Author

@Akasurde I've made a mistake during previous rebase and didn't notice that change of str to to_text was lost. I've fixed that right now.

As mentioned in previous comment - the converion is done for every parameter explicitly on purpose.
I've decided to do this only on the values which are expected to be non-string and will correctly convert using to_text (boolean, for example, won't convert correctly).
For this reason (correctness) I think this should be done for each value, not for all of them.

However, if it is preffered, I will change the implementation to one that will do this for all arguments (in the loop). Just let me know.

@polatsinan Does the issue persist when applying current changes in the PR?

This comment has been minimized.

Copy link
@bcoca

bcoca Jul 16, 2019

Member

@marcin-sucharski check against current development, I've merged stringification at run_command so you might not need to do this anymore.

This comment has been minimized.

Copy link
@polatsinan

polatsinan Jul 17, 2019

@bcoca I have cloned the devel branche and the issue still exists:

ansible 2.9.0.dev0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/spolat/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/spolat/python/ansible/local/lib/python2.7/site-packages/ansible
  executable location = /home/spolat/python/ansible/bin/ansible
  python version = 2.7.15+ (default, Nov 27 2018, 23:36:35) [GCC 7.3.0]
TypeError: argument of type 'int' is not iterable

@marcin-sucharski: Applying current changes in the PR fixes the issue.

This comment has been minimized.

Copy link
@bcoca

bcoca Jul 17, 2019

Member

that is not same issue as original above, can you provide -vvv output?

Fix issue with argument conversion for run_command
In the nmcli module arguments in list passed to run_command sometimes
were of type int. This caused issue in the run_command which resulted
in exception being thrown.

@marcin-sucharski marcin-sucharski force-pushed the marcin-sucharski:fix/module-nmcli-argument-conversion branch from 275d7d9 to 7c10656 Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.