Add a Network service by imobachgs · Pull Request #571 · agama-project/agama

imobachgs · 2023-05-10T05:51:23Z

Problem

Agama does not offer a service to set up the network. The web UI talks directly to NetworkManager and the CLI/autoinstallation does not support configuring the network.

Solution

Trello card: https://trello.com/c/1kiDu6yN/ and https://trello.com/c/TCJqEQ2z/

This PR introduces a new service that allows configuring the network. The code is organized into these modules:

model contains the types that represent the network concepts. They are supposed to be agnostic from the actual network service (e.g., NetworkManager).
adapter offers an abstraction to support other backends in the future (if needed).
dbus includes the D-Bus service implementation.
nm implements support for interacting with NetworkManager.

Example of a network D-Bus tree

$ sudo busctl --address unix:path=/run/agama/bus tree org.opensuse.Agama.Network1
└─/org
  └─/org/opensuse
    └─/org/opensuse/Agama
      └─/org/opensuse/Agama/Network1
        ├─/org/opensuse/Agama/Network1/connections
        │ ├─/org/opensuse/Agama/Network1/connections/0
        │ ├─/org/opensuse/Agama/Network1/connections/1
        │ ├─/org/opensuse/Agama/Network1/connections/10
        │ ├─/org/opensuse/Agama/Network1/connections/2
        │ ├─/org/opensuse/Agama/Network1/connections/3
        │ ├─/org/opensuse/Agama/Network1/connections/4
        │ ├─/org/opensuse/Agama/Network1/connections/5
        │ ├─/org/opensuse/Agama/Network1/connections/6
        │ ├─/org/opensuse/Agama/Network1/connections/7
        │ ├─/org/opensuse/Agama/Network1/connections/8
        │ └─/org/opensuse/Agama/Network1/connections/9
        └─/org/opensuse/Agama/Network1/devices
          ├─/org/opensuse/Agama/Network1/devices/0
          ├─/org/opensuse/Agama/Network1/devices/1
          ├─/org/opensuse/Agama/Network1/devices/2
          ├─/org/opensuse/Agama/Network1/devices/3
          ├─/org/opensuse/Agama/Network1/devices/4
          └─/org/opensuse/Agama/Network1/devices/5

Exposing a wireless network

$ sudo busctl --address unix:path=/run/agama/bus introspect org.opensuse.Agama.Network1 /org/opensuse/Agama/Network1/connections/4
NAME                                            TYPE      SIGNATURE RESULT/VALUE                                                   FLAGS
org.freedesktop.DBus.Introspectable             interface -         -                                                              -
.Introspect                                     method    -         s                                                              -
org.freedesktop.DBus.Peer                       interface -         -                                                              -
.GetMachineId                                   method    -         s                                                              -
.Ping                                           method    -         -                                                              -
org.freedesktop.DBus.Properties                 interface -         -                                                              -
.Get                                            method    ss        v                                                              -
.GetAll                                         method    s         a{sv}                                                          -
.Set                                            method    ssv       -                                                              -
.PropertiesChanged                              signal    sa{sv}as  -                                                              -
org.opensuse.Agama.Network1.Connection          interface -         -                                                              -
.Id                                             property  s         "Vodafone_4960_7DRS"                                           emits-change
.UUID                                           property  s         "889e7b38-82ed-42d4-bee2-5d10ba9c706b"                         emits-change
org.opensuse.Agama.Network1.Connection.IPv4     interface -         -                                                              -
.Addresses                                      property  a(su)     1 "192.168.0.99" 24                                            emits-change writable
.Gateway                                        property  s         "192.168.0.1"                                                  emits-change writable
.Method                                         property  y         1                                                              emits-change writable
.Nameservers                                    property  as        2 "6.6.6.6" "8.8.8.8"                                          emits-change writable
org.opensuse.Agama.Network1.Connection.Wireless interface -         -                                                              -
.Mode                                           property  y         0                                                              emits-change writable
.Password                                       property  s         ""                                                             emits-change writable
.SSID                                           property  ay        18 86 111 100 97 102 111 110 101 95 52 57 54 48 95 55 68 82 83 emits-change writable
.Security                                       property  y         3                                                              emits-change

Documentation

This PR updates the network_support_design.md document, as we decided not to rely on third-party tools (that's the reason the branch is called network-service-2 😅).

In addition to that, we tried to document the code to make it easier to contribute.

Library documentation

Testing

As with most of our Rust code, it contains a few unit tests. I will try to add more before merging.

Why is it still in draft mode?

Reload after saving (to get the correct UUIDs and clean-up removed devices)
Extend the documentation
Adapt to agama-dbus-server package (see Rust locale #533).
(optional) Report which connections failed to add/update/remove (some Report struct).
(optional) Better error handling (using anyhow).

What's next?

Add support to the CLI to configure the network: https://trello.com/c/MowTUSWi/

* It only exports the ids.

coveralls · 2023-05-10T05:55:48Z

Changes unknown
when pulling 0787522 on network-service-2
into ** on master**.

mvidner

I'm being extra pedantic about API naming consistency here...

The aspects to observe are

pluralization
capitalization

For pluralization, it makes sense to use the plural name whenever there is a collection: /.../things and /.../things/1, /.../things/33 (as opposed to the singular /.../thing/33)

For capitalization,

the Naming Conventions want to use CamelCase even for objects
here we are following the systemd convention of naming the objects in lowercase
other parts of Agama use CamelCase for object names

IMHO we should use CamelCase
(but before reaching this conclusion I have adjusted the docs to document the code reality)

doc/network_support_design.md

rust/agama-network/src/lib.rs

mvidner · 2023-05-29T12:24:12Z

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

+            .connections_paths()
+            .iter()
+            .filter_map(|c| ObjectPath::try_from(c.clone()).ok())
+            .collect()


NP: I've found that the resulting array is not sorted, at least not by ObjectPath

mvidner · 2023-05-29T12:35:31Z

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

+    /// Adds a new network connection.
+    ///
+    /// * `name`: connection name.
+    /// * `ty`: connection type (see [crate::model::DeviceType]).


FWIW, the argument name is exposed in D-Feet and my first thought was: "Ty... pe? Why the truncation?"

Using type_ is slightly awkward, but https://docs.rs/zbus/3.12.0/zbus/attr.dbus_interface.html doesn't let us name parameters freely so it is IMHO the best option.

mvidner · 2023-05-29T12:42:37Z

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

+    /// Adds a new network connection.
+    ///
+    /// * `name`: connection name.
+    /// * `ty`: connection type (see [crate::model::DeviceType]).


About the value of the type argument:

It should be documented where the D-Bus API user can easily reference it.

So I'm going to add /doc/dbus/*Network1*.doc.xml files and document it there.

rust/agama-network/src/nm/client.rs

mvidner

Strings vs ObjectPaths:

mvidner · 2023-05-29T13:17:21Z

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

+        objects
+            .devices_paths()
+            .iter()
+            .filter_map(|c| ObjectPath::try_from(c.clone()).ok())


In a strongly typed language I would expect the object paths to be always valid, unless coming as user input or as arguments to a constructor, which is not the case here.

mvidner · 2023-05-29T13:27:33Z

rust/agama-network/src/dbus/tree.rs

+    }
+
+    /// Returns all devices paths.
+    pub fn devices_paths(&self) -> Vec<String> {


We should be using specialized strings that are guaranteed to be valid object paths.
The zbus_names crate exists exactly for this purpose and... it does not include object paths... because they appear on the wire so the type lives in zvariant.

So,

use zvariant::ObjectPath; pub fn devices_paths(&self) -> Vec<OwnedObjectPath> { or pub fn devices_paths(&self) -> Vec<ObjectPath> {

here and in many other places.
With respect to the D-Bus API it is an implementation detail, so we can leave it for later.

mvidner · 2023-05-29T13:42:23Z

For pluralization, it makes sense to use the plural name whenever there is a collection: /.../things and /.../things/1, /.../things/33 (as opposed to the singular /.../thing/33)

BTW I don't have a source for this, it is "original research" 😄 and the reasoning is that the tree is more compact when we use the same word forms for both the "manager" and the "items"

so much for interoperability :-/

jreidinger

OK, so now after updates only important stuff for me is DBus naming ( consistency and that ty as property ). And in general I would like to see type_ instead of ty as mvidner agrees with me :) Others can wait for next PR

Co-authored-by: Martin Vidner <mvidner@suse.cz>

imobachgs · 2023-05-31T05:44:19Z

IMHO we should use CamelCase (but before reaching this conclusion I have adjusted the docs to document the code reality)

I would say we are using systemd capitalization style in new code (basically in storage). We could re-evaluate that decision but, at least, we should document why we went that road (and plan for adopting it in the rest of the APIs).

imobachgs · 2023-05-31T05:58:34Z

OK, so now after updates only important stuff for me is DBus naming ( consistency and that ty as property ). And in general I would like to see type_ instead of ty as mvidner agrees with me :) Others can wait for next PR

OK, I still do not like type_. To be clear, the property is exposed as Type over D-Bus, so it remains as an internal detail.

I have found more usages of ty in the Rust world:

The ty module: representing types in the Compiler Development Guide.
Many fields in the syn crate, which is widely used to develop derive macros (e.g., https://docs.rs/syn/latest/syn/struct.ItemType.html#structfield.ty).
Other crates (you can find a few on this list) although I do not know how relevant they are.

However, I can understand that it looks weird, so I have renamed it by now to unblock this PR.

rust/agama-network/src/dbus/tree.rs

rust/agama-network/src/action.rs

Co-authored-by: Martin Vidner <mvidner@suse.cz>

mvidner

Thank you!

Just a .changes entry is missing I think

imobachgs · 2023-06-01T08:02:35Z

Thank you!

Just a .changes entry is missing I think

Oops, good point. I will add it.

mvidner

Yay!

imobachgs and others added 27 commits May 9, 2023 16:51

Add o.o.Agama.Network1 to the D-Bus configuration

abc520b

Add an agama-network package

5712b2d

Add agama-network dependencies

a032669

Add a basic NetworkManager client

c82bf0b

Export network devices on D-Bus

fcbd519

Add a missing error.rs file

86f6a29

Export network connections on D-Bus

c71717d

* It only exports the ids.

Read and expose IPv4 addresses

d9ce0b3

Export nameservers on network D-Bus

b79f2d9

Export gateways on network D-Bus interface

9cb8d95

Export wireless settings on D-Bus

d9f7b91

Update the network support design document

13015b4

Remove unused 'use' statements

1b3a707

Remove the unused 'applied_connections' method

1c6b074

Test the conversion from D-Bus to the NetworkManager model

7aaf53f

Use an index for the published device interfaces

922dd54

Publish devices interface

160808b

Publish connections interface

b04bf43

Simplify the network service building

6ebf188

Refactor object paths handling in the network service

47c47ea

GetDevices and GetConnections returns ao

5b43a94

Remove 'network' from interfaces::Connection

b8d9969

Avoid unneeded copies of strings

fdf8434

Extend agama-network configuration

0e0463f

Move the logic to read the network to NetworkState

2958086

Add some documentation to agama-network

2196935

Minor documentation changes

0787522

imobachgs added 2 commits May 10, 2023 10:11

Use o.o.A.N.Connection.* for D-Bus connection interfaces

155aca8

Extend the agama-network documentation

f226893

mvidner reviewed May 29, 2023

View reviewed changes

doc/network_support_design.md Outdated Show resolved Hide resolved

doc/network_support_design.md Outdated Show resolved Hide resolved

rust/agama-network/src/lib.rs Outdated Show resolved Hide resolved

mvidner reviewed May 29, 2023

View reviewed changes

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

mvidner reviewed May 29, 2023

View reviewed changes

mvidner added 3 commits May 29, 2023 16:17

doc/dbus: seeded introspection XML for Network1.*

edcf0e3

doc/dbus: copied to .doc.xml and kept only relevant interface

73ca602

doc/dbus: added headings so that gdbus-codegen sees the docs

82a3b8d

so much for interoperability :-/

jreidinger requested changes May 30, 2023

View reviewed changes

imobachgs and others added 3 commits May 31, 2023 06:33

Fix objects paths

649b16a

Co-authored-by: Martin Vidner <mvidner@suse.cz>

Log the device name for unsupported types

36c4837

Improve documentation

e23a865

Replace ty with type_

55e0324

jreidinger approved these changes May 31, 2023

View reviewed changes

mvidner reviewed May 31, 2023

View reviewed changes

rust/agama-network/src/dbus/tree.rs Show resolved Hide resolved

rust/agama-network/src/dbus/tree.rs Show resolved Hide resolved

rust/agama-network/src/dbus/tree.rs Show resolved Hide resolved

mvidner reviewed May 31, 2023

View reviewed changes

rust/agama-network/src/action.rs Show resolved Hide resolved

imobachgs and others added 3 commits May 31, 2023 14:42

Do not send the 'addresses' key

fd6f821

Apply suggestions from code review

de0dad2

Co-authored-by: Martin Vidner <mvidner@suse.cz>

Add a TODO about using OwnedObjectPath

c19bfee

jreidinger approved these changes May 31, 2023

View reviewed changes

mvidner requested changes Jun 1, 2023

View reviewed changes

Update the changes file

5141cb9

mvidner approved these changes Jun 1, 2023

View reviewed changes

imobachgs merged commit 2c34acd into master Jun 1, 2023

imobachgs deleted the network-service-2 branch June 1, 2023 09:27

teclator mentioned this pull request Jun 26, 2023

Added support for showing the network configuration through the CLI #622

Merged

imobachgs mentioned this pull request Aug 2, 2023

Release version 3 #683

Merged

Conversation

imobachgs commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Documentation

Testing

Why is it still in draft mode?

What's next?

Uh oh!

coveralls commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvidner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvidner May 29, 2023

Choose a reason for hiding this comment

Uh oh!

mvidner May 29, 2023

Choose a reason for hiding this comment

Uh oh!

mvidner May 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mvidner left a comment

Choose a reason for hiding this comment

Uh oh!

mvidner May 29, 2023

Choose a reason for hiding this comment

Uh oh!

mvidner May 29, 2023

Choose a reason for hiding this comment

Uh oh!

imobachgs May 31, 2023

Choose a reason for hiding this comment

Uh oh!

mvidner commented May 29, 2023

Uh oh!

jreidinger left a comment

Choose a reason for hiding this comment

Uh oh!

imobachgs commented May 31, 2023

Uh oh!

imobachgs commented May 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvidner left a comment

Choose a reason for hiding this comment

Uh oh!

imobachgs commented Jun 1, 2023

Uh oh!

mvidner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

imobachgs commented May 10, 2023 •

edited

Loading

coveralls commented May 10, 2023 •

edited

Loading