Add mac assigning to network base connection #893

jcronenberg · 2023-11-29T17:04:03Z

Problem

It's not possible to assign a mac address to network connections

Solution

Add a property mac_address to base connection and also to NetworkConnection inside settings to allow setting of custom mac addresses.

Note: Currently for simplicity sake it just stores it as String all around, but with this draft I also wanted to start a discussion on what agama should use to store the mac address inside BaseConnection. String is of course the simplest, but doesn't perform any validation before sending it to NetworkManager, so the result is a pretty non explanatory error that the connection couldn't be added, because NetworkManager rejects any invalid macs of course. Let me know what you would prefer 🙂 (adjustments to the type shouldn't be to much work so no worries if I should change it)

Testing

Modified existing unit tests
Tested manually

coveralls · 2023-11-29T17:08:00Z

coverage: 75.009% (+0.01%) from 74.998%
when pulling 4ec3314 on jcronenberg:general-mac-address
into 2c46c58 on openSUSE:master.

imobachgs · 2023-11-29T22:50:50Z

Hi @jcronenberg. First of all, thanks! And now, let's review the approach.

I prefer using some MAC address type. @teclator suggested using macaddr, which looks simple enough, but we are open to other options. However, using a String could be an acceptable temporary solution.

However, if the value is not mandatory, I would go with an Option (Option<MacAddr> or Option<String>). I would avoid using an empty string to represent that there is no address.

jcronenberg · 2023-11-30T09:50:29Z

I prefer using some MAC address type. @teclator suggested using macaddr, which looks simple enough, but we are open to other options. However, using a String could be an acceptable temporary solution.

Alright I tend to agree, so I'll look into using that crate for the mac address type.

However, if the value is not mandatory, I would go with an Option (Option<MacAddr> or Option<String>). I would avoid using an empty string to represent that there is no address.

When going the MacAddr type route I agree, wrapping it inside an Option probably makes the most sense. However for String I found it to be unnecessarily complex because in the end the None option would have to be mapped to an empty string anyway because NetworkManager requires this setting with an empty string to overwrite any already set mac address.

imobachgs · 2023-11-30T09:54:34Z

When going the MacAddr type route I agree, wrapping it inside an Option probably makes the most sense. However for String I found it to be unnecessarily complex because in the end the None option would have to be mapped to an empty string anyway because NetworkManager requires this setting with an empty string to overwrite any already set mac address.

IMHO the Option represents the reality better. I like to think about this model as backend-agnostic, so the fact that we need to map it to a string for NetworkManager should not drive our design.

jcronenberg · 2023-11-30T09:57:00Z

When going the MacAddr type route I agree, wrapping it inside an Option probably makes the most sense. However for String I found it to be unnecessarily complex because in the end the None option would have to be mapped to an empty string anyway because NetworkManager requires this setting with an empty string to overwrite any already set mac address.

IMHO the Option represents the reality better. I like to think about this model as backend-agnostic, so the fact that we need to map it to a string for NetworkManager should not drive our design.

I think for pretty much all backends this would be the case, but anyway it doesn't really matter.

jcronenberg · 2023-11-30T13:05:56Z

I have updated the PR with how I would envision it to work with a dedicated MAC type. ~~Though I am unsure if the model is now also too NM agnostic for your liking.~~

rust/agama-dbus-server/src/network/model.rs

cfconrad · 2023-11-30T14:03:49Z

rust/agama-dbus-server/src/network/nm/dbus.rs

+            Value::new(match &conn.base.mac_address {
+                Some(mac) => mac.to_string(),
+                None => "".to_string(),


because of this, maybe we could place mac_address_string() into BaseConnection

cfconrad

LGTM

imobachgs

When I proposed an Option I was not aware that it was not a simple address, but that you were defining your own enum. Sorry if I overlooked something.

In that case, the None variant could be part of your enum, so you would not need to cope with that case anymore and you could simplify the code.

And that _string method would not be needed at all.

imobachgs · 2023-11-30T21:53:50Z

rust/agama-dbus-server/src/network/dbus/interfaces.rs

+    /// Custom mac-address
+    #[dbus_interface(property)]
+    pub async fn mac_address(&self) -> String {
+        self.get_connection().await.base().mac_address_string()


For better encapsulation, please do not call base() from outside the struct. Actually, I think base() should be private.

I would implement mac_address() in Connection directly.

imobachgs · 2023-12-01T06:34:02Z

rust/agama-dbus-server/src/network/model.rs

 }

 #[derive(Debug, Default, Clone)]
 pub struct BaseConnection {
    pub id: String,
    pub uuid: Uuid,
+    pub mac_address: Option<MacAddress>,


When I proposed using an Option, I thought you would have the macaddr::MacAddr struct inside. But I see that it is more complex as you are defining an enum (MacAddress). To make things easier, why not just add a MacAddress::None (feel free to find a better name, please)? So you do not need to cope with the Option anymore.

Actually, you could convert that value to an empty string.

imobachgs · 2023-12-01T06:36:08Z

rust/agama-dbus-server/src/network/model.rs

+        }
+    }
+
+    pub fn set_mac_address(&mut self, mac_address: &str) -> Result<(), InvalidMacAddress> {


This one looks counter-intuitive to me. Why do I need to convert a MacAddress to a string? Not to mention the fact that an empty string means None. If you add the None variant to the MacAddress enum, well, problem solved.

If you continue with the Option approach, I would 1) pass an option or 2) have a set_mac_address and a unset_mac_address.

imobachgs · 2023-12-01T06:36:39Z

rust/agama-dbus-server/src/network/model.rs

+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "preserve" => Ok(Self::Preserve),
+            "permanent" => Ok(Self::Preserve),


Why is "permanent" converted to Preserve?

imobachgs · 2023-12-01T06:38:09Z

rust/agama-dbus-server/src/network/model.rs

+        match s {
+            "preserve" => Ok(Self::Preserve),
+            "permanent" => Ok(Self::Preserve),
+            "random" => Ok(Self::Permanent),


Why are "random" and "stable" converted to Permanent?

Oops must have been some lazy copy pasting 😄

teclator · 2023-12-05T10:33:32Z

rust/agama-dbus-server/src/network/model.rs

+        self.base().mac_address.to_string()
+    }
+
+    pub fn set_mac_address(&mut self, mac_address: &str) -> Result<(), InvalidMacAddress> {


Suggested change

pub fn set_mac_address(&mut self, mac_address: &str) -> Result<(), InvalidMacAddress> {

pub fn set_mac_address(&mut self, mac_address: MacAddress) {

teclator · 2023-12-05T10:33:58Z

rust/agama-dbus-server/src/network/model.rs

+    }
+
+    pub fn set_mac_address(&mut self, mac_address: &str) -> Result<(), InvalidMacAddress> {
+        self.base_mut().mac_address = MacAddress::from_str(mac_address)?;


Suggested change

self.base_mut().mac_address = MacAddress::from_str(mac_address)?;

self.base_mut().mac_address = mac_address;

teclator · 2023-12-05T10:34:09Z

rust/agama-dbus-server/src/network/model.rs

+
+    pub fn set_mac_address(&mut self, mac_address: &str) -> Result<(), InvalidMacAddress> {
+        self.base_mut().mac_address = MacAddress::from_str(mac_address)?;
+        Ok(())


Suggested change

Ok(())

rust/agama-dbus-server/src/network/nm/dbus.rs

jcronenberg changed the title ~~Add mac assigning to base connection~~ Add mac assigning to network base connection Nov 29, 2023

jcronenberg force-pushed the general-mac-address branch from 73d07af to c960eac Compare November 30, 2023 09:54

jcronenberg force-pushed the general-mac-address branch from c960eac to f7b86b5 Compare November 30, 2023 13:01

jcronenberg commented Nov 30, 2023

View reviewed changes

rust/agama-dbus-server/src/network/model.rs Outdated Show resolved Hide resolved

jcronenberg force-pushed the general-mac-address branch from f7b86b5 to b767cc7 Compare November 30, 2023 13:15

cfconrad reviewed Nov 30, 2023

View reviewed changes

jcronenberg force-pushed the general-mac-address branch from b767cc7 to e3b3305 Compare November 30, 2023 14:26

cfconrad approved these changes Nov 30, 2023

View reviewed changes

jcronenberg marked this pull request as ready for review November 30, 2023 16:25

imobachgs requested changes Dec 1, 2023

View reviewed changes

jcronenberg force-pushed the general-mac-address branch from 28dc327 to aa0d1b4 Compare December 1, 2023 09:14

teclator reviewed Dec 5, 2023

View reviewed changes

imobachgs reviewed Dec 5, 2023

View reviewed changes

rust/agama-dbus-server/src/network/nm/dbus.rs Outdated Show resolved Hide resolved

jcronenberg added 4 commits December 5, 2023 11:50

Add MAC assigning to base connection

bb7d466

Update agama-cli changes

0e09c4a

Rename MacAddress None to Unset

ecafa7a

Move dbus mac-address processing to separate function

db9a040

jcronenberg force-pushed the general-mac-address branch from b4c798b to 4fefbe6 Compare December 5, 2023 11:24

Move MacAddress string conversion to dbus interface

b27645f

jcronenberg force-pushed the general-mac-address branch from 4fefbe6 to b27645f Compare December 5, 2023 11:27

jcronenberg requested review from teclator and imobachgs December 5, 2023 11:33

Minor change to keep consistency

4ec3314

imobachgs approved these changes Dec 5, 2023

View reviewed changes

imobachgs merged commit 52dc7a3 into agama-project:master Dec 5, 2023
1 check passed

jcronenberg deleted the general-mac-address branch December 5, 2023 17:16

imobachgs mentioned this pull request Feb 12, 2024

Update to release 7 #947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mac assigning to network base connection #893

Add mac assigning to network base connection #893

jcronenberg commented Nov 29, 2023

coveralls commented Nov 29, 2023 •

edited

Loading

imobachgs commented Nov 29, 2023

jcronenberg commented Nov 30, 2023

imobachgs commented Nov 30, 2023

jcronenberg commented Nov 30, 2023

jcronenberg commented Nov 30, 2023 •

edited

Loading

cfconrad Nov 30, 2023

cfconrad left a comment

imobachgs left a comment

imobachgs Nov 30, 2023

imobachgs Dec 1, 2023

imobachgs Dec 1, 2023

imobachgs Dec 1, 2023

imobachgs Dec 1, 2023 •

edited

Loading

jcronenberg Dec 1, 2023

teclator Dec 5, 2023

teclator Dec 5, 2023

teclator Dec 5, 2023

	pub fn set_mac_address(&mut self, mac_address: &str) -> Result<(), InvalidMacAddress> {
	pub fn set_mac_address(&mut self, mac_address: MacAddress) {

	self.base_mut().mac_address = MacAddress::from_str(mac_address)?;
	self.base_mut().mac_address = mac_address;

Add mac assigning to network base connection #893

Add mac assigning to network base connection #893

Conversation

jcronenberg commented Nov 29, 2023

Problem

Solution

Testing

coveralls commented Nov 29, 2023 • edited Loading

imobachgs commented Nov 29, 2023

jcronenberg commented Nov 30, 2023

imobachgs commented Nov 30, 2023

jcronenberg commented Nov 30, 2023

jcronenberg commented Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

cfconrad left a comment

Choose a reason for hiding this comment

imobachgs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imobachgs Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 29, 2023 •

edited

Loading

jcronenberg commented Nov 30, 2023 •

edited

Loading

imobachgs Dec 1, 2023 •

edited

Loading