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.

Sign up for GitHub

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

Jump to bottom

nmcli: avoid changed status for most cases with VPN connections #5126

Merged
merged 4 commits into from Sep 3, 2022
Merged

nmcli: avoid changed status for most cases with VPN connections #5126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0a104b
nmcli: avoid changed status for most cases with VPN connections
Aug 11, 2022
1e591fa
Add changelog
Aug 18, 2022
3fccc6b
Some suggested changes
Aug 19, 2022
378670c
nmcli: allow any string for vpn service-type
Aug 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/5126-nmcli-remove-diffs.yml
View file
Open in desktop
@@ -0,0 +1,2 @@
bugfixes:
- "nmcli - avoid changed status for most cases with VPN connections (https://github.com/ansible-collections/community.general/pull/5126)."
11 changes: 5 additions & 6 deletions plugins/modules/net_tools/nmcli.py
View file
Open in desktop
Expand Up @@ -923,7 +923,6 @@
description: This defines the service type of connection.
type: str
required: true
choices: [ pptp, l2tp ]
gateway:
description: The gateway to connection. It can be an IP address (for example C(192.0.2.1))
or a FQDN address (for example C(vpn.example.com)).
Expand All @@ -949,7 +948,7 @@
ipsec-enabled:
description:
- Enable or disable IPSec tunnel to L2TP host.
- This option is need when C(service-type) is C(l2tp).
- This option is need when C(service-type) is C(org.freedesktop.NetworkManager.l2tp).
type: bool
choices: [ yes, no ]
ipsec-psk:
Expand Down Expand Up @@ -1350,7 +1349,7 @@
conn_name: my-vpn-connection
vpn:
permissions: "{{ ansible_user }}"
service-type: l2tp
service-type: org.freedesktop.NetworkManager.l2tp
gateway: vpn.example.com
password-flags: 2
user: brittany
Expand Down Expand Up @@ -1670,7 +1669,7 @@ def connection_options(self, detect_change=False):
for name, value in self.vpn.items():
if name == 'service-type':
options.update({
'vpn-type': value,
'vpn.service-type': value,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are vpn-type and vpn.service-type always exchangeable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like vpn-type is an alias of vpn.service-type since NetworkManager 1.4 [1] (released in Aug 2016). How old NetworkManager should this collection support?

[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/commit/c3422e917d75b48bcfde9036caec61bf97d6c312

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's a very good question, and one I think next to impossible to answer. It seems that the module does not say which versions of nmcli it supports, and never mentioned that in the past. So the answer would be: "every version that the module supported in the past and that it didn't explicitly drop support for (which is probably none)".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently current nmcli.py does not work with very old NetworkManager, anyway. With NetworkManager 1.2.6 on Ubuntu 16.04, I got:

$ ansible some_old_machine -m nmcli -a "conn_name=foo state=present type=ethernet ifname=foo" --become
some_old_machine | FAILED! => {
    "ansible_facts": {
        "discovered_interpreter_python": "/usr/bin/python3"
    },
    "changed": false,
    "msg": "Error: mandatory 'ifname' not seen before 'ipv6.ignore-auto-dns'.\n",
    "name": "foo",
    "rc": 2
}

The used command is:

/usr/bin/nmcli con add type ethernet con-name foo ipv6.ignore-auto-dns no ipv4.ignore-auto-routes no connection.interface-name foo connection.autoconnect yes ipv4.never-default no ipv4.ignore-auto-dns no ipv6.ignore-auto-routes no ipv4.may-fail yes

ifname is specified via connection.interface-name, while older nmcli insists using ifname. On the other hand, the same command works fine with NetworkManager 1.22.10 on Ubuntu 20.04.

As a side note, ifname is required before NetworkManager 1.22 [1]. I don't have a machine with NetworkManager between 1.4 and 1.22 for testing, though.

[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/commit/02e5a8d10a39f0f401b72f3a0a39619770fe51de

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess let's try this out :)

})
elif name == 'permissions':
options.update({
Expand Down Expand Up @@ -2100,8 +2099,8 @@ def _compare_conn_params(self, conn_info, options):
if key == self.mtu_setting and self.mtu is None:
self.mtu = 0
if key == 'vpn.data':
current_value = list(map(str.strip, current_value.split(',')))
value = list(map(str.strip, value.split(',')))
current_value = sorted(re.sub(r'\s*=\s*', '=', part.strip(), count=1) for part in current_value.split(','))
value = sorted(part.strip() for part in value.split(','))
else:
# parameter does not exist
current_value = None
Expand Down
14 changes: 6 additions & 8 deletions tests/unit/plugins/modules/net_tools/test_nmcli.py
View file
Open in desktop
Expand Up @@ -1201,7 +1201,7 @@
'conn_name': 'vpn_l2tp',
'vpn': {
'permissions': 'brittany',
'service-type': 'l2tp',
'service-type': 'org.freedesktop.NetworkManager.l2tp',
'gateway': 'vpn.example.com',
'password-flags': '2',
'user': 'brittany',
Expand All @@ -1221,9 +1221,8 @@
connection.permissions: brittany
ipv4.method: auto
ipv6.method: auto
vpn-type: l2tp
vpn.service-type: org.freedesktop.NetworkManager.l2tp
vpn.data: gateway=vpn.example.com, password-flags=2, user=brittany, ipsec-enabled=true, ipsec-psk=QnJpdHRhbnkxMjM=
vpn.data: gateway = vpn.example.com, ipsec-enabled = true, ipsec-psk = QnJpdHRhbnkxMjM=, password-flags = 2, user = brittany
felixfontein marked this conversation as resolved.
Show resolved Hide resolved
vpn.secrets: ipsec-psk = QnJpdHRhbnkxMjM=
vpn.persistent: no
vpn.timeout: 0
Expand All @@ -1235,7 +1234,7 @@
'conn_name': 'vpn_pptp',
'vpn': {
'permissions': 'brittany',
'service-type': 'pptp',
'service-type': 'org.freedesktop.NetworkManager.pptp',
'gateway': 'vpn.example.com',
'password-flags': '2',
'user': 'brittany',
Expand All @@ -1253,9 +1252,8 @@
connection.permissions: brittany
ipv4.method: auto
ipv6.method: auto
vpn-type: pptp
vpn.service-type: org.freedesktop.NetworkManager.pptp
vpn.data: password-flags=2, gateway=vpn.example.com, user=brittany
vpn.data: gateway=vpn.example.com, password-flags=2, user=brittany
"""


Expand Down Expand Up @@ -3630,7 +3628,7 @@ def test_create_vpn_l2tp(mocked_generic_connection_create, capfd):

for param in ['connection.autoconnect', 'no',
'connection.permissions', 'brittany',
'vpn.data', 'vpn-type', 'l2tp',
'vpn.data', 'vpn.service-type', 'org.freedesktop.NetworkManager.l2tp',
]:
assert param in add_args_text

Expand Down Expand Up @@ -3670,7 +3668,7 @@ def test_create_vpn_pptp(mocked_generic_connection_create, capfd):

for param in ['connection.autoconnect', 'no',
'connection.permissions', 'brittany',
'vpn.data', 'vpn-type', 'pptp',
'vpn.data', 'vpn.service-type', 'org.freedesktop.NetworkManager.pptp',
]:
assert param in add_args_text

Expand Down