[FEATURE] Allow getting static IP address(es) for saved connections

[joshwd36]

Is your feature request related to a problem? Please describe.
SavedConnection contains only a subset of the possible properties available on a saved connection. Specifically, I am looking to read the ipv4.addresses entry, both for getting the static IP address for a connection without necessarily being connected to it, and for getting the IP address of a connection in AP mode

Describe the solution you'd like
An ipv4 section on SavedConnection containing at least an addresses field.

For Developers- what would your solution be?
Add any potential solutions or plans of action for this feature. Make sure you have a good understanding of the code base.

Additional context
Add any other context or screenshots about the feature request here.

[joshwd36]

I've been thinking about what would be the best way to implement this. The API surface for a connection is huge and I don't think adding every entry to SavedConnection is the right solution. One thing I had wondered about is having structs for each section (e.g. ipv4, or even a subset of that), and having a way to construct that from the raw properties hashmap, which I think would be good to include in SavedConnection. I also wonder whether these could be shared with the builders module, as they are essentially holding the same data. Any thoughts on this @cachebag?

[cachebag]

I also wonder whether these could be shared with the builders module

@joshwd36
I don't think so. The builders emit HashMap<&'static str, HashMap<&'static str, Value<'static>>> containing only what the caller sets. And then reads yield HashMap<String, HashMap<String, OwnedValue>> that is fully populated with NM's defaults. This means one struct serving both is either fully optional or defaulted. Either path seems crude to me.

I am also trying to think of how to do this without breaking the API. Unfortunately I do understand there's alot about the API surface I did not think clearly about early on in this project. I am trying to pay for my sins and thus while I am okay with refactoring/redoing some of the surface, I would like to see how to address things without breaking user space.

Anyways...what if we did something like this:

#[non_exhaustive] pub struct IpSettings {
    pub method: IpMethod,          // Auto | Manual | Disabled | LinkLocal | Shared | Ignore | Other(String)
    pub addresses: Vec<IpAddress>, // { address, prefix }
    pub gateway: Option<String>,
    pub dns: Vec<String>,
    pub dns_search: Vec<String>,
    pub routes: Vec<IpRoute>,
    pub never_default: bool,
    pub ignore_auto_dns: bool,
}

pub struct SavedConnection {
    // ...existing
    pub ipv4: Option<IpSettings>,
    pub ipv6: Option<IpSettings>,
}

Idk if that covers the fields but you get the gist. Basically we can do a minor bump since SavedConnection is non exhaustive. and since decode_saved already holds the full map, this would be pure decoding with no extra Dbus stuff to mess with.

Maybe for interop we do a bridge. Something like impl From<&IpAddress> for builders::IpConfig.

Idk. These thoughts are kind of all over the place since the surface area of the codebase has slowly been slipping my mind lately but let me know what you think.

[joshwd36]

I also wonder whether these could be shared with the builders module

@joshwd36 I don't think so. The builders emit HashMap<&'static str, HashMap<&'static str, Value<'static>>> containing only what the caller sets. And then reads yield HashMap<String, HashMap<String, OwnedValue>> that is fully populated with NM's defaults. This means one struct serving both is either fully optional or defaulted. Either path seems crude to me.

Ah, that makes sense. No worries then!

Anyways...what if we did something like this:

#[non_exhaustive] pub struct IpSettings {
pub method: IpMethod, // Auto | Manual | Disabled | LinkLocal | Shared | Ignore | Other(String)
pub addresses: Vec, // { address, prefix }
pub gateway: Option,
pub dns: Vec,
pub dns_search: Vec,
pub routes: Vec,
pub never_default: bool,
pub ignore_auto_dns: bool,
}

pub struct SavedConnection {
// ...existing
pub ipv4: Option,
pub ipv6: Option,
}

That would definitely work, but my worry is that if every property available on a connection was exposed under SavedConnection then from a user perspective it could get very unwieldy, as well as having potentially quite a large size in bytes and non-negligible decoding time. However, if not every property is directly available, then I think there needs to be a way of exposing the remaining properties, which is why I was thinking about having separate structs for each section, so users only pay for what they need. I don't know if that's the best way to do it. There also needs to be some thought put into where the line is drawn, I think the current set of properties is quite nice as it essentially maps to the general connection properties section, but I think adding some (but not all) other sections to the struct could get confusing.

[joshwd36]

@cachebag I've been thinking about this some more, and I'm wondering whether there could be a type safe way to access properties. Something like this maybe? (Sketched out, I don't have proper access to my dev machine at the moment):

trait ParseProperty {
    fn parse_property(value: &OwnedValue) -> Option<Self> // Should this just be Self and panic on type error? Or Result<Self>?
    where
        Self: Sized;
}

impl ParseProperty for String {
    fn parse_property(value: &OwnedValue) -> Option<Self> {
        ...
    }
}

impl ParseProperty for i32 {
    fn parse_property(value: &OwnedValue) -> Option<Self> {
        ...
    }
}

impl ParseProperty for Vec<String> {
    fn parse_property(value: &OwnedValue) -> Option<Self> {
        ...
    }
}

struct Property<T> {
    section: &'static str,
    property: &'static str,
    _type: PhantomData<T>
}

struct SavedConnection {
    ...
    properties: HashMap<String, HashMap<String, OwnedValue>>,
}

impl SavedConnection {
    pub fn get_property<T>(&self, property: Property<T>) -> Option<T>
    where
        T: ParseProperty,
    {
        let value = self
            .properties
            .get(property.section)?
            .get(property.property)?;
        T::parse_property(value)
    }
}

mod ipv4 {
    pub const ADDRESSES: super::Property::<Vec<String>> = super::Property::<Vec<String>> {
        section: "ipv4",
        property: "addresses",
        _type: PhantomData,
    };
}

This would allow users to access any property without paying the cost to decode the whole thing, and it should be straightforward to generate the constants for each property.

[cachebag]

@joshwd36

I think we are discussing two different things here. But what you are suggesting works quite well I think. A couple things:

this won't decode:

pub const ADDRESSES: Property::<Vec<String>> = ... { property: "addresses" }

ipv4.addresses is the legacy aau format. The real data is in address-data (aa{sv} of {address, prefix}), so you probably want Vec<IpAddress> which just puts us back into what I suggested with IpSettings.

Also, OwnedValue here makes zvariant a public dep:

nvm this is irrelevant

We could do Option<IpSettings> for the common case + your get_property::<T> as the escape hatch for the rest. wdyt?

[joshwd36]

@cachebag

this won't decode:

pub const ADDRESSES: Property::<Vec> = ... { property: "addresses" }

ipv4.addresses is the legacy aau format. The real data is in address-data (aa{sv} of {address, prefix}), so you probably want Vec<IpAddress> which just puts us back into what I suggested with IpSettings.

Ah yes, I'd missed that, though the example was more to suggest the shape, rather than the exact property. My concern is just about continually adding to SavedConnection, which I think would would be a mistake beyond the most basic settings, but what counts as the most basic settings I think is more flexible

[cachebag]

My concern is just about continually adding to SavedConnection, which I think would would be a mistake beyond the most basic settings, but what counts as the most basic settings I think is more flexible

@joshwd36
That's exactly why I want get_property::<T>(). The whole point of the generic accessor is so we don't have to keep growing SavedConnection. Only genuinely common sections get promoted to typed fields; everything else stays reachable through the escape hatch. Being below "the line" still means the data is accessible, just untyped which is just fine IMO

[joshwd36]

@cachebag okay, I think we're on the same page! So just to confirm, the changes would be (each of which could be in a separate PR):

1. Add pub ipv4: Option<IpSettings> and pub ipv6: Option<IpSettings> to SavedConnection, along with the definition for IpSettings. (Sorry, just to add to this, but would it make more sense for it not to be optional, but to have each member of IpSettings as optional, except Vecs. I'm imagining a situation where e.g. never_default isn't set, should that be an option, or should we have a default that aligns with NetworkManager's default?)
2. Add pub properties: HashMap<String, HashMap<String, OwnedValue>> to SavedConnection
3. Add pub fn get_property<T>(&self, property: Property<T>) -> Option<T> to SavedConnection, along with definitions for Property, a way of converting from &OwnedValue to T given a specific Property<T>, and generate associated constants for each possible property on a NetworkManager connection

Does that sound right?

[cachebag]

@joshwd36

Yep this makes sense to me...please keep in mind the following though:

• Let's not make the IpSettings members Option. GetSettings omits unset keys, so we already fill NM's documented default everywhere else (autoconnect goes to true, etc.).IpSettings should match: never_default: bool defaults to false (NM's default), Vecs empty when absent. Keep Option only at the section level (ipv4/ipv6 can be wholly absent I guess). Per-field Option would just diverge from the rest of SavedConnection without buying us anything, since a decoded profile can't really distinguish "unset" from "default" anyway.
• Let's keep the map private and expose only get_property::<T>(). That way we don't freeze HashMap<String, HashMap<String, OwnedValue>> into the public API. The representation can stay ours to change.

Three PRs makes sense.

Maybe use that new stacked PRs feature GitHub introduced? (I have no idea if it's good or how to use it lol so it's your choice).

[joshwd36]

@cachebag sounds good! Only thing to clarify is that for addresses you have { address, prefix }. In other places you get this pattern (e.g. Network.ip4_address) this is just a string. This ties into another thing I've been thinking about that probably warrants its own issue, which is using stronger types for things, e.g. UUID for IDs, enums for some things that are currently strings, and this with IP addresses. Basically what I'm asking is should this be the place to introduce a struct made up of the parsed address, which other instances can then migrate to in a breaking change? Something I've noticed in this case is that it would mean that IpSettings may have to have different instances for ipv4 and ipv6, as I think it would be desirable for the ipv4 instance to contain an Ipv4Addr, and ipv6 to contain the V6 equivalent. I think it would be a shame to have to use the IpAddr enum, since we were know which variant it is in each case

[cachebag]

@joshwd36 Okay: yes, parameterizing is the right way to avoid IpAddr matching. Let's do:

   pub struct IpAddress<A> { pub address: A, pub prefix: u8 }
   pub struct IpSettings<A> { /* addresses: Vec<IpAddress<A>>, gateway: Option<A>, dns: Vec<A>, ... */ }

   pub struct SavedConnection {
       pub ipv4: Option<IpSettings<Ipv4Addr>>,
       pub ipv6: Option<IpSettings<Ipv6Addr>>,
   } 

Note that address-data values are strings, and NM occasionally stores something that won't parse. Let's drop malformed entries with a warn! rather than failing the whole decode. It's the same thing we already use for skipping malformed saved connections.

This ties into another thing I've been thinking about that probably warrants its own issue, which is using stronger types for things, e.g. UUID for IDs, enums for some things that are currently strings, and this with IP addresses.

I'm on board with the direction, but let's not anchor it to this issue. Feel free to open another issue for it as I think it's worth it. IpAddress/IpSettings here should be the first citizen of that effort without blocking the IP-address feature on a crate-wide migration.