Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to assign an interface or match settings to a connection #723

Merged
merged 12 commits into from
Sep 13, 2023

Conversation

teclator
Copy link
Contributor

@teclator teclator commented Aug 29, 2023

Problem

Agama allows to add/update network connections. However, it relies on NetworkManager to decide which interface to use. Alternatively, Agama should allow the user to assign a connection to an specific interface.

Instead of using just an interface name, Agama should offer some matching capabilities, just like NetworkManager. How the API should look like is up to who implements the feature but the interface-name, the path and the driver should be supported.

Solution

It has been added support to specify to which interfaces the connection should be bond to supporting it directly through the interface name attribute or through a set of match settings as them are supported by NetworkManager.

"network": {
    "connections": [
      {
        "id": "Ethernet network device 1",
        "method": "manual",
        "interface": "eth0",
        "addresses": [
          "192.168.122.100/24"
        ],
        "gateway": "192.168.122.1",
        "nameservers": [
          "192.168.122.1"
        ]
      }
    ]
  }

Testing

  • Added a new unit test
  • Tested manually

@coveralls
Copy link

coveralls commented Sep 11, 2023

Coverage Status

coverage: 72.814% (-0.05%) from 72.863% when pulling c3f1bd9 on matching_settings into 9c310af on master.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it looks good. Just a few suggestions.

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

match_config_dbus.insert("interface-name", interfaces);

match_config_dbus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do:

    HashMap::from([
        ("driver", drivers),
        ("kernel-command-line", kernels),
        ("path", paths),
        ("interface-name", interfaces),
    ])

It looks more readable to me and it is easy to find out what is included in the HashMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, originally did so, then added some logic to inserting only the key if it was not empty but as we are inserting it always now it makes sense to do so. Thanks

@teclator teclator force-pushed the matching_settings branch 2 times, most recently from 4b60739 to ed83dad Compare September 13, 2023 11:46
Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>
@teclator teclator merged commit f7fa3ad into master Sep 13, 2023
10 checks passed
@teclator teclator deleted the matching_settings branch September 13, 2023 12:10
@imobachgs imobachgs mentioned this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants