Vlan keyfile parser support #370
Conversation
|
I just thought of an unfortunate side effect of this placeholder type approach. After parsing the vlan and creating the fake type, if netplan ingests a file defining a device with the same name as the one linked in the vlan, it will fail with This approach will work only if we leave it to be done after loading the entire netplan state and then the vlan as the last one. But this would require some changes in the way we handle the missing netdefs. Currently we handle the missing netdefs based on the loaded parser state + the file being parsed. We don't wait for the next YAMLs to assume an interface wasn't defined, our approach is "if it wasn't defined so far or in this file we are parsing, we error out". |
|
I handled the problem by repurposing the place holder netdef and using it as it were a new one. |
There was a problem hiding this comment.
Thanks Danilo,
this is looking mostly good! I like the approach of relaxing Netplan's validation just for the NetworkManager special cases, where we have the architectural discrepancy of NM's programatical vs Netplan's declarative way of defining the connections.
I put two questions/suggestions inline, as I think we can improve the patching of parent and child devices a bit, taking into account that NM's [vlan].parent could be either an interface name or an UUID of a nmconnection profile.
| * It's required because Network Manager allows the creation of | ||
| * VLAN connections with non-existing parent interfaces. | ||
| */ | ||
| nd->vlan_link = netplan_netdef_new(npp, parent, NETPLAN_DEF_TYPE_NM_PLACEHOLDER_, NETPLAN_BACKEND_NM); |
There was a problem hiding this comment.
According to [1] NetworkManager's [vlan].parent can be either an interface name or a NM UUID of the corresponding connection profile. I think we should be able to detect the UUID case by string length (len(UUID) > IF_NAMESIZE) and check if a configuration with netdef ID NM-<UUID> already exists in the parser context. If not, the placeholder should be created with a NM-<UUID> NetdefID, so when that other keyfile is parsed, it can override the placeholder.
Maybe we need some extra logic in nm.c to cut the NM- prefix, when writing back the [vlan].parent= value into a keyfile.
[1] https://developer-old.gnome.org/NetworkManager/stable/settings-vlan.html
There was a problem hiding this comment.
I didn't realize it could be the connection UUID as well hmm...
And it seems to be a bit more complicated than that. For example, if eth0 was configured via Netplan, it's netdef ID will be eth0 and it will not have a UUID in the generated key file. Although you can create a VLAN pointing the parent to the NM generated UUID. In this case, we will never find the parent in the netplan state. The same problem will happen if eth0 was configured using match and its netdef ID is not its name, we will not find it by the name eth0 either.
And there is also the case the parent wasn't parsed yet by Netplan because it's in a different file.
As I see, it's OK to just use whatever parent name was used, even if we already have the parent parsed with a different netdef ID. Network Manager will still be able to create the interface.
What do you think?
There was a problem hiding this comment.
ACK. Maybe it is a premature optimization to try matching the Netdefs already at this stage and we should just take whatever NM provides, so it will be able to match it back after the keyfile is re-generated.
We have some check like this if (strnlen(netdef->id, IF_NAMESIZE) == IF_NAMESIZE) in our codebase, and I'm wondering if this could get us into trouble if NM decides to put a UUID as the parent value. But AFAICT this shouldn't be the case as we're bailing out earlier if we detect it to be a PLACEHOLDER interface.
suggestion: Let's add a test case where we use a [vlan].parent = UUID keyfile setting, just to be on the safe side.
There was a problem hiding this comment.
I added a test.
Regarding the interface name length, for virtual interfaces we don't write the connection.interface-name if it's longer than IF_NAMESIZE. But as you said we don't render the placeholder interface. We also have a validation step checking if the name is longer thanbut it's only a warning at the moment. Maybe we should look at this validation again...
This change is intended to be used only by Network Manager, more specifically when used through the integration with Netplan. Network Manager doesn't enforce the existence of interfaces in order to create a connection. For example, when creating a VLAN, it doesn't care if the parent interface doesn't exist. The nmconnection will be created anyway. When creating a VLAN directly in Netplan, the "link" property is required and the interface used as "link", the parent interface, must exist in Netplan. It will throw an error otherwise. Since the integration with Network Manager, some of these restrictions need to be relaxed or NM will error out when creating these types of connections. The NETPLAN_DEF_TYPE_NM_PLACEHOLDER_ is intended to fill the gaps of interfaces that must exist for the connection to work but don't really exist. Practical example: when creating a VLAN, the "link" property is required and the interface pointed by the link also must exist somewhere in the YAML files. Although, when creating it through Network Manager, it will successfully create the connection, it will just not do anything. Since the integration with Netplan, this scenario will start to fail and might be surprising for users. Also, when creating an ethernet connection through NM, the interface ID in Netplan will end up being something like NM-030c95e3-b025-4a1e-905f-d3d92acfd8b9. The VLAN link is supposed to point to that interface name in the Netplan configuration. But as translation from keyfile to YAML are handled isolated from the rest of the netplan state, we don't know what netdef ID no point to when doing the translation. Doing it properly would require loading and analyzing all the Netplan state before emitting YAML for keyfiles.
It relies on the new placeholder netdef type.
daniloegea
force-pushed
the
vlan_keyfile_parser
branch
from
b0e9897
to
9f87878
Compare
Also add a few more tests.
If we find an interface with the same name of a place holder created earlier, we just change the place holder type to the type of the new interface and use it to store the configuration. Without this logic the parser would fail if it finds an interface with the same name of the place holder but of a different type.
daniloegea
force-pushed
the
vlan_keyfile_parser
branch
from
9f87878
to
04681a8
Compare
Description
Support for reading VLAN keyfiles.
One problem found during the implementation is that Netplan requires the existence of the
linkinterface in the YAML configuration. Although Network Manager can create a vlan connection even if the parent interface doesn't exist.The approach used here basically makes netplan to behave the same way: when the renderer is NetworkManager, it will ignore the fact that the parent doesn't exist and point to whatever interface was used by NM. In order to continue passing the VLAN validation rules, the netdef type
NETPLAN_DEF_TYPE_NM_PLACEHOLDER_was introduced. It will be used in cases where a required dependency can't be satisfied but it's still required by Netplan but not by Network Manager. So its use is basically to fill this gap and allow the backend renderer to retrieve the VLAN link ID from the place holder netdef.For example, the command
nmcli con add con-name vlan-123 type vlan dev enp5s0 id 123will generate the following keyfile:The keyfile parser will produce the following YAML. The value in the key
linkwill point to a name that might not be an existing netplan interface ID (netdef).This is unfortunately required due to the way we import keyfiles at the moment.
Checklist
make checksuccessfully.make check-coverage).