Adding mtu configuration to nm

[fdizazzo]

Hi all,
this time i need to add mtu configuration to network manager 😄

The idea is using the already existing property net.interface.[interfaceName].config.mtu and propagate it in the right configurations for the various supported interface types. According to https://networkmanager.dev/docs/api/latest/ch01.html we'll have:
ethernet -> 802-3-ethernet.mtu
vlan -> 802-3-ethernet.mtu
modem -> gsm.mtu, ppp.mtu
loopback -> loopback.mtu
wifi -> 802-11-wireless.mtu

My main open point is that the above configuration only works for ipv4, while ipv6 has its own field in ipv6.mtu so I'm not sure if we prefer to use the same source property for both or it's better to have a specific .config.ip6.mtu.

Once we're settled on this I'll open a PR that allows configuration via snapshot upload which will be followed with the configuration fields on the UI.

Thanks,
Fabio

[mattdibi]

Hello there @fdizazzo 👋

🤔 I think the correct way to implement this is having a .config.ip6.mtu and .config.ip4.mtu in the snapshot. A user might want to set the MTU for ipv4 differently from what is set for ipv6. @pierantoniomerlino @MMaiero thoughts?

P.s. please note that we currently do not support the configuration of the loopback interface with NetworkManager. Therefore the loopback.mtu won't get used

[pierantoniomerlino]

Yes, I think that the properties should be net.interface.<interface>.config.ip4.mtu and net.interface.<interface>.config.ip6.mtu.

[fdizazzo]

Yes, I think that the properties should be net.interface..config.ip4.mtu and net.interface..config.ip6.mtu

Sounds good. What about the existing property net.interface.<interface>.config.mtu ? Should it be removed or just ignored in favor of the new ones?
My first idea was using it for ipv4 and create a new one for ipv6, reflecting networkmanager apis having one in the device configurations and the other specific for ipv6.

[fdizazzo]

Just noticed that the ipv6.mtu field is only supported in NetworkManager since 1.40 according to https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.44.0/NEWS
and trying to use it breaks the entire configuration attempt. 😞
Could use the version property of the installed NetworkManager to selectively populate the settings map be a solution?

[mattdibi]

Sounds good. What about the existing property net.interface..config.mtu ? Should it be removed or just ignored in favor of the new ones?

That is a not-documented, not-actually-enforced vestigial property that (please @MMaiero correct me on this) we can pretend doesn't exist and move on with our lives. Therefore proceed with the config.ipv4.mtu + config.ipv6.mtu combo.

Just noticed that the ipv6.mtu field is only supported in NetworkManager since 1.40 according to https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/1.44.0/NEWS and trying to use it breaks the entire configuration attempt.

This was bound to happen sooner or later 😩 . The right way to handle this, as you already mentioned, is to parse the NM version and act accordingly.

This shouldn't be too much work: upon connecting to the NM over DBus you'll just need to ask for the version and store it in the NetworkManagerDbusWrapper (we already have a method for retrieving the NM version). Upon creating the configuration you need to pass the version info and check for compatibility.

I suggest to model the version as an object so that you can add the proper parse and isGreaterThan() methods for checking compatibility.

[fdizazzo]

Ok on all your points, on it! 👍

[fdizazzo]

Hi, I've just opened PR #4877 for this.
It is still lacking test method not strictly needed for a successful build and usable descriptions on the UI but should be otherwise functional, so we can check if the implemented logic is sound.

In the meantime, I guess I need some pointers on this
https://github.com/eclipse/kura/blob/ec2d052dd3e698d60d8eddac24325f0f32980fd5/kura/org.eclipse.kura.web2/src/main/java/org/eclipse/kura/web/server/net2/configuration/NetworkConfigurationServicePropertiesBuilder.java#L249-L255
being invoked on every type of network interface and breaking any attempt at modification with null values. I've wrapped it in a if(this.gwtConfig instanceof GwtWifiNetInterfaceConfig) to quickly avoid the issue but i'm probably missing something.

[mattdibi]

🤔 @GregoryIvo do you have any insight about this?

[GregoryIvo]

This is an oversight in the PR, which will be fixed in #4872

[fdizazzo]

Thanks @GregoryIvo! :)
So I'll revert this to its original state when i'm done or merge your PR, whichever comes first 👍

[mattdibi]

Addressed in #4877

Please @fdizazzo, don't forget to update the documentation!

[mattdibi]

Documentation addressed in #4919

Closing the discussion since the feature was merged and the documentation updated.