Open giorgiga opened 2 years ago
Files identified in the description:
If these files are incorrect, please update the component name
section of the description or use the !component
bot command.
cc @alcamie101 click here for bot help
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)
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.
ifname
is not mandatory after all (see https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1089)
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)
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.
I see two ways to solve this:
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).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?)
Files identified in the description:
If these files are incorrect, please update the component name
section of the description or use the !component
bot command.
If there is consensus on the approach, I might be able to start a PR.
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.
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.
interface-independent profile work for me with ifname: ''
Summary
ansible.community.nmcli
claims to treat an asterisk as magic value forifname
("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
Community.general Version
Configuration
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 documentedActual Results
ansible.community.nmcli
creates a non-functional connectionCode of Conduct