keyfile parser: add support for all tunnel types (LP: #2016473)#360
keyfile parser: add support for all tunnel types (LP: #2016473)#360daniloegea merged 9 commits intocanonical:mainfrom
Conversation
Add the minimum support necessary to generate a YAML that will be accepted by the parser. The rest of the tunnel settings will be stored in the passthrough section.
slyon
changed the title
There was a problem hiding this comment.
Thank you Danilo, this is another big step in our NM backend support!
It's looking mostly good to me, but I had a few remarks about the handling of NetplanTunnelMode, the termination of validation after a warning and the NM tunnels support in general.
I left a bunch of (mostly simple) inline remarks, which we might want to fix before merging.
PS: I assume you already ran the new NM autopkgtests (from https://code.launchpad.net/~danilogondolfo/network-manager/+git/network-manager/+merge/443011) against this branch and those PASSed?
| NETPLAN_TUNNEL_MODE_IP6GRE = 8, | ||
| NETPLAN_TUNNEL_MODE_VTI6 = 9, | ||
| NETPLAN_TUNNEL_MODE_VXLAN = 10, | ||
| NETPLAN_TUNNEL_MODE_GRETAP = 10, |
There was a problem hiding this comment.
thought: This change has the potential to break ABI. But AFAICT it is only being used internal to libnetplan.so and thus should not break anything in reality. Our abi-compat checker didn't catch any issue either.
Keeping the enum in sync with NM is a good thing and was the original intent, according to the comment above.
Netplan doesn't allow partial Wireguard configuration while Network Manager does. The original Netplan's behavior will not allow NM to build a WG connection in separate steps.
Sync the enum NetplanTunnelMode with the tunnel modes supported by Network Manager.
And add few more tests.
They are conflicting with functions with the same name from parse.c when these files are include in ctests.
The keyfile is already being automatically released.
The second parameter of g_datalist_set_data_full() is not stored in the list and must be free'd.
daniloegea
force-pushed
the
tunnel_wg_keyfile_reader
branch
from
7949d36 to
14c54a9
Compare
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
+1
Thanks for resolving my concerns.
2 participants
Description
Support for Wireguard keyfiles parsing is required to get the new Network Manager autopkgtests passing [1]. The original plan was to add support only for WG, but I ended up including all tunnel types.
There is a not-so-nice function renaming in src/parse-nm.c that was required to get new ctests compiling.
[1] - https://code.launchpad.net/~danilogondolfo/network-manager/+git/network-manager/+merge/443011
Checklist
make checksuccessfully.make check-coverage).