nmcli should let interface-name unset when ifname = '*'

[giorgiga]

Summary

ansible.community.nmcli claims to treat an asterisk as magic value for ifname ("A special value of '*' can be used for interface-independent connections."), but there is no code to actually handle it and it ends up setting connection.interface-name=* in networkmanager.

The correct behaviour would be to leave connection.interface-name without a value ("If not set, then the connection can be attached to any interface", see https://developer-old.gnome.org/NetworkManager/stable/settings-connection.html).

Issue Type

Bug Report

Component Name

nmcli

Ansible Version

$ ansible --version
ansible [core 2.12.7]
  config file = /home/giorgio/Projects/infra/ansible.cfg
  configured module search path = ['/home/giorgio/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.10/site-packages/ansible
  ansible collection location = /home/giorgio/Projects/infra/galaxy/collections
  executable location = /usr/bin/ansible
  python version = 3.10.6 (main, Aug  2 2022, 00:00:00) [GCC 12.1.1 20220507 (Red Hat 12.1.1-1)]
  jinja version = 3.0.3
  libyaml = True

Community.general Version

$ ansible-galaxy collection list community.general

Collection        Version
----------------- -------
community.general 5.5.0  

Configuration

$ ansible-config dump --only-changed
ANSIBLE_NOCOWS([project root]/ansible.cfg) = True
COLLECTIONS_PATHS([project root]/ansible.cfg) = ['/home/giorgio/Projects/infra/galaxy/collections']
DEFAULT_GATHERING([project root]/ansible.cfg) = explicit
DEFAULT_HOST_LIST([project root]/ansible.cfg) = ['[project root]/inventory']
DEFAULT_ROLES_PATH([project root]/ansible.cfg) = ['[project root]/roles', '[project root]/galaxy/roles']
DISPLAY_SKIPPED_HOSTS([project root]/ansible.cfg) = False
INJECT_FACTS_AS_VARS([project root]/ansible.cfg) = False
INTERPRETER_PYTHON([project root]/ansible.cfg) = auto_silent

OS / Environment

Fedora f36

Steps to Reproduce

Sorry guys I can't provide a full script right now (this already took more time that I have today).

Just look at nmcli.py and see that there is no code to handle the special *.

Expected Results

ansible.community.nmcli should behave as documented

Actual Results

ansible.community.nmcli creates a non-functional connection

Code of Conduct

• [X] I agree to follow the Ansible Code of Conduct

[ansibullbot]

Files identified in the description:

• plugins/modules/net_tools/nmcli.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[ansibullbot]

cc @alcamie101 click here for bot help

[giorgiga]

Forgot to mention: adding

    # Handle the magic value '*'
    if self.ifname == '*':
        del options['connection.interface-name']

around line 1932 in nmcli.py seems to solve the issue (it's what I'm using as a quick fix).

I think I'll be able to submit a PR in the coming days, but do feel free to fix this yourself if that's your preference (by the looks of it, it won't be much more work than reviewing the PR)

[giorgiga]

It is interesting to note that the nmcli help does mention an asterisk as a value for ifname:

$ nmcli con add -h 2>&1 | grep -C 2 '*'
COMMON_OPTIONS:
                type <type>
                ifname <interface name> | "*"
                [con-name <connection name>]
                [autoconnect yes|no]

and that the asterisk seems to work as expected when used in nmcli (ie. one ends up with connection.interface-name not set).

However when using it in ansible.community.nmcli I ended up with connection.interface-name set to *... this is probably due to nmcli.py setting the option as connection.interface-name instad of ifname (which according the the nmcli doc shouldn't even work, as ifname is mandatory).

Swithiching to using ifname may be cleaner than adding the if I mentioned above.

[giorgiga]

ifname is not mandatory after all (see https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1089)

[giorgiga]

I looked into this a little bit more: hear me out :)

According to the community.general.nmcli documentation, the interface name defaults to the connection name:

This parameter defaults to conn_name when left unset for all connection types except vpn that removes it.

In the reality of code, however, this is true only on creation

    # Use connection name as default for interface name on creation.
    if nmcli_command == 'create' and self.ifname is None:
        ifname = self.conn_name
    else:
        ifname = self.ifname

and on modify a missing ifname parameter is interpreted as "leave connection.interface-name alone" rather than "connection.interface-name should be the connection name", as the doc suggests, or "connection.interface-name should have no value", which, I would argue, is what one would expect given how the other parameters work.

This means that, even with my naive "let's just add more ifs" fix in place, it is not possible to remove the connection.interface-name setting in an existing connection.

All this is probably due to the fact that ifname actually used to be mandatory in legacy versions of nmcli (see, again, https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1089)

[giorgiga]

Thomas Haller from freedesktop.org says ifname is not mandatory since NetworkManager 1.22 (thanks, Thomas!)

This means it's still mandatory in systems running Debian 10 buster (networkmanager 1.14), Ubuntu 18.04 LTS (networkmanager 1.10), RHEL/Centos 7 (networkmanager 1.18, but IIRC per default it used /etc/sysconfig/network-scripts), and distros of similar vintage.

[giorgiga]

I see two ways to solve this:

1. Getting rid of any special treatment of ifname (ie. adopling no ifname means no connection.interface-name). This would lead to cleaner/simpler code in community.general.nmcli and a behavior that is more consistent with nmcli and the other parameters in community.general.nmcli, at the price of a breaking change (people who didn't set a value for ifname will have their connection.interface-name cleared).
2. Keeping the current no ifname means don't touch connection.interface-name policy, but properly implement the special value *. This requires more complex code, allows to avoid breaking changes and allows to canonicalize the current (unintuitive) behaviour (IDK if this last one is desirable or not).

Which one do you devs think suits the project policy best? Is there a third approach you would recommend instead?

For what is worth, I'd say 1 since it has the greatest long-term benefits (simplicity and usability) and the unexpected "changed" while testing a playbook after upgrading the community.general collection will be hard to miss (yes: people who don't read changelogs and don't test after updating dependencies might have a break in produciton, but... they are kind of asking for it?)

[ansibullbot]

Files identified in the description:

• plugins/modules/nmcli.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[peterupton]

If there is consensus on the approach, I might be able to start a PR.

[c-erb]

I see two ways to solve this:

1. Getting rid of any special treatment of `ifname` (ie. adopling _no `ifname` means no  `connection.interface-name`_). This would lead to cleaner/simpler code in `community.general.nmcli` and a behavior that is more consistent with `nmcli` and the other parameters in `community.general.nmcli`, at the price of a breaking change (people who didn't set a value for `ifname` will have their `connection.interface-name` cleared).

2. Keeping the current _no `ifname` means don't touch `connection.interface-name`_ policy, but properly implement the special value *. This requires more complex code, allows to avoid breaking changes and allows to canonicalize the current (unintuitive) behaviour (IDK if this last one is desirable or not).

Which one do you devs think suits the project policy best? Is there a third approach you would recommend instead?

For what is worth, I'd say 1 since it has the greatest long-term benefits (simplicity and usability) and the unexpected "changed" while testing a playbook after upgrading the community.general collection will be hard to miss (yes: people who don't read changelogs and don't test after updating dependencies might have a break in produciton, but... they are kind of asking for it?)

For me number 1 sounds like the correct approach. I want to point out that this issue seems to be quite important for an production environment.

[SijmenHuizenga]

Agree that getting rid of any special treatments of ifname would be the best way forward.

This issue is blocking for us, as it prevents a creation of any nmcli connection that apply to all interfaces.