-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: avoid changed status for most cases with VPN connections #5126
nmcli: avoid changed status for most cases with VPN connections #5126
Conversation
Follow-up #4746 * `nmcli connection show` includes vpn.service-type but not vpn-type. Switching to vpn.service-type removes unneeded diffs while keeping the same functionality, as vpn-type is an alias of vpn.service-type per nm-settings-nmcli(1). NetworkManager also adds `org.freedesktop.NetworkManager.` prefix for known VPN types [1]. The logic is non-trivial so I didn't implement it in this commit. If a user specifies `service-type: l2tp`, changed will be always be True: - "vpn.service-type": "org.freedesktop.NetworkManager.l2tp" + "vpn.service-type": "l2tp" * The vpn.data field from `nmcli connection show` is sorted by keys and there are spaces around equal signs. I added codes for parsing such data. Tests are also updated to match outputs of nmcli commands. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.38.4/src/libnm-core-impl/nm-vpn-plugin-info.c#L619
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @jremerich who implemented this originally.
@@ -1670,7 +1670,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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
* Make space stripping more flexible - works for cases without equal signs. * Keep vpn.data in a test case with no spaces
Thank you very much for detailed review and kind suggestions! I fixed some, while others need more time for investigation. |
Hi! I took a look at the changes and didn't see anything I can contribute. Thanks for your contribution, @yan12125 ! |
Backport to stable-5: 💚 backport PR created✅ Backport PR branch: Backported as #5220 🤖 @patchback |
* nmcli: avoid changed status for most cases with VPN connections Follow-up #4746 * `nmcli connection show` includes vpn.service-type but not vpn-type. Switching to vpn.service-type removes unneeded diffs while keeping the same functionality, as vpn-type is an alias of vpn.service-type per nm-settings-nmcli(1). NetworkManager also adds `org.freedesktop.NetworkManager.` prefix for known VPN types [1]. The logic is non-trivial so I didn't implement it in this commit. If a user specifies `service-type: l2tp`, changed will be always be True: - "vpn.service-type": "org.freedesktop.NetworkManager.l2tp" + "vpn.service-type": "l2tp" * The vpn.data field from `nmcli connection show` is sorted by keys and there are spaces around equal signs. I added codes for parsing such data. Tests are also updated to match outputs of nmcli commands. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.38.4/src/libnm-core-impl/nm-vpn-plugin-info.c#L619 * Add changelog * Some suggested changes * Make space stripping more flexible - works for cases without equal signs. * Keep vpn.data in a test case with no spaces * nmcli: allow any string for vpn service-type (cherry picked from commit 6ff594b)
@yan12125 thanks for improving this module! |
… (#5220) * nmcli: avoid changed status for most cases with VPN connections Follow-up #4746 * `nmcli connection show` includes vpn.service-type but not vpn-type. Switching to vpn.service-type removes unneeded diffs while keeping the same functionality, as vpn-type is an alias of vpn.service-type per nm-settings-nmcli(1). NetworkManager also adds `org.freedesktop.NetworkManager.` prefix for known VPN types [1]. The logic is non-trivial so I didn't implement it in this commit. If a user specifies `service-type: l2tp`, changed will be always be True: - "vpn.service-type": "org.freedesktop.NetworkManager.l2tp" + "vpn.service-type": "l2tp" * The vpn.data field from `nmcli connection show` is sorted by keys and there are spaces around equal signs. I added codes for parsing such data. Tests are also updated to match outputs of nmcli commands. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.38.4/src/libnm-core-impl/nm-vpn-plugin-info.c#L619 * Add changelog * Some suggested changes * Make space stripping more flexible - works for cases without equal signs. * Keep vpn.data in a test case with no spaces * nmcli: allow any string for vpn service-type (cherry picked from commit 6ff594b) Co-authored-by: Chih-Hsuan Yen <[email protected]>
…ble-collections#5126) * nmcli: avoid changed status for most cases with VPN connections Follow-up ansible-collections#4746 * `nmcli connection show` includes vpn.service-type but not vpn-type. Switching to vpn.service-type removes unneeded diffs while keeping the same functionality, as vpn-type is an alias of vpn.service-type per nm-settings-nmcli(1). NetworkManager also adds `org.freedesktop.NetworkManager.` prefix for known VPN types [1]. The logic is non-trivial so I didn't implement it in this commit. If a user specifies `service-type: l2tp`, changed will be always be True: - "vpn.service-type": "org.freedesktop.NetworkManager.l2tp" + "vpn.service-type": "l2tp" * The vpn.data field from `nmcli connection show` is sorted by keys and there are spaces around equal signs. I added codes for parsing such data. Tests are also updated to match outputs of nmcli commands. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.38.4/src/libnm-core-impl/nm-vpn-plugin-info.c#L619 * Add changelog * Some suggested changes * Make space stripping more flexible - works for cases without equal signs. * Keep vpn.data in a test case with no spaces * nmcli: allow any string for vpn service-type
…ble-collections#5126) * nmcli: avoid changed status for most cases with VPN connections Follow-up ansible-collections#4746 * `nmcli connection show` includes vpn.service-type but not vpn-type. Switching to vpn.service-type removes unneeded diffs while keeping the same functionality, as vpn-type is an alias of vpn.service-type per nm-settings-nmcli(1). NetworkManager also adds `org.freedesktop.NetworkManager.` prefix for known VPN types [1]. The logic is non-trivial so I didn't implement it in this commit. If a user specifies `service-type: l2tp`, changed will be always be True: - "vpn.service-type": "org.freedesktop.NetworkManager.l2tp" + "vpn.service-type": "l2tp" * The vpn.data field from `nmcli connection show` is sorted by keys and there are spaces around equal signs. I added codes for parsing such data. Tests are also updated to match outputs of nmcli commands. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.38.4/src/libnm-core-impl/nm-vpn-plugin-info.c#L619 * Add changelog * Some suggested changes * Make space stripping more flexible - works for cases without equal signs. * Keep vpn.data in a test case with no spaces * nmcli: allow any string for vpn service-type
SUMMARY
Follow-up #4746
nmcli connection show
includes vpn.service-type but not vpn-type.Switching to vpn.service-type removes unneeded diffs while keeping
the same functionality, as vpn-type is an alias of vpn.service-type
per nm-settings-nmcli(1).
NetworkManager also adds
org.freedesktop.NetworkManager.
prefix forknown VPN types [1]. The logic is non-trivial so I didn't implement it
in this commit. If a user specifies
service-type: l2tp
, changed willbe always be True:
nmcli connection show
is sorted by keys andthere are spaces around equal signs. I added codes for parsing such
data.
Tests are also updated to match outputs of nmcli commands.
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.38.4/src/libnm-core-impl/nm-vpn-plugin-info.c#L619
ISSUE TYPE
COMPONENT NAME
nmcli
ADDITIONAL INFORMATION
N/A