feat: network settings #51

ronny-standtke · 2025-07-18T11:55:57Z

This pull request introduces several updates across multiple files, focusing on adding network configuration management, enhancing rollback capabilities, and updating dependencies. The changes include new functionality for managing network settings, improvements to error handling, and updates to configuration files and dependencies.

Network Configuration Management:

src/api.rs: Added support for network configuration management, including functions to set network settings, handle server restarts with new certificates, and manage rollbacks for network changes. Introduced new types like NetworkSetting and PendingNetworkRollback, and added macros for file paths. [1] [2] [3]
src/main.rs: Added global server restart signal and rollback timer management to facilitate network rollback and server restart workflows.

Rollback Enhancements:

src/api.rs: Implemented rollback mechanisms for network settings, including saving, loading, and executing pending rollbacks, as well as canceling rollback timers.
src/main.rs: Added utility functions for managing rollback timers, such as set_rollback_timer and cancel_rollback_timer.

Dependency Updates:

Cargo.toml: Added new dependencies like rust-ini for INI file handling and actix-cors for cross-origin resource sharing. Updated existing dependency versions. [1] [2]

Configuration Updates:

biome.json: Upgraded schema version and added new linter rules for stricter code style enforcement. Updated file inclusion patterns. [1] [2] [3]

Miscellaneous Improvements:

src/certificate.rs: Modified create_module_certificate to accept an optional IP address for certificate generation, enabling dynamic updates based on network changes. [1] [2]
build-and-run-image.sh: Added volume mapping for the network directory to support network configuration changes in Docker containers.

- upgraded vuetify from ^3.8.12 to ^3.9.0 - updated @biomejs/biome from 2.0.6 to 2.1.1 - updated @types/bun from ^1.2.17 to ^1.2.18 - upgraded unocss from ^66.3.2 to ^66.3.3 - updated vite from ^7.0.0 to ^7.0.3 - upgraded vue-tsc from ^3.0.0 to ^3.0.1

…ubmission

…ality

…ix URL reference in healthcheck

…e handling

… file handling

empwilli

LGTM overall minor nits

empwilli · 2025-07-21T06:34:05Z

src/api.rs

+    previous_ip: String,
+    ip: Option<String>,
+    netmask: Option<u8>,
+    gateway: Option<Vec<String>>,
+    dns: Option<Vec<String>>,


Preferably use strong types instead of "stringly types". The standard has your back here: std::net::IpAddr (+ std::net::Ipv4Addr, std::net::Ipv6Addr). serde conveniently already provides Serialize/Deserialize implementations for these types.

The benefit is, that you gain some addtional validity checks + convenience methods for working with these types.

empwilli · 2025-07-21T06:53:23Z

src/api.rs

    }

+    pub async fn set_network(
+        body: web::Json<NetworkSetting>,


nit: rather use "network_settings" than body. body communicates "this is the body of the request", whereas this already is the parsed body.

empwilli · 2025-07-21T06:58:37Z

src/api.rs

+        if body.name.is_empty() {
+            return HttpResponse::BadRequest().body("Network name cannot be empty");
+        }


Maybe it would be sensible to add in something like: https://crates.io/crates/serde_valid

empwilli · 2025-07-21T07:08:38Z

src/api.rs

+
+        if let Some(ip) = &body.ip {
+            if !ip.is_empty() && ip.parse::<std::net::IpAddr>().is_err() {
+                return HttpResponse::BadRequest().body("Invalid IP address format");
+            }
+        }
+
+        if let Some(netmask) = &body.netmask {
+            if *netmask > 32 {
+                return HttpResponse::BadRequest().body("Netmask must be between 0 and 32");
+            }
+        }
+
+        if let Some(gateway) = &body.gateway {
+            for gw in gateway {
+                if !gw.is_empty() && gw.parse::<std::net::IpAddr>().is_err() {
+                    return HttpResponse::BadRequest().body("Invalid gateway format");
+                }
+            }
+        }
+
+        if let Some(dns) = &body.dns {
+            for dns_entry in dns {
+                if !dns_entry.is_empty() && dns_entry.parse::<std::net::IpAddr>().is_err() {
+                    return HttpResponse::BadRequest().body("Invalid DNS format");
+                }
+            }
+        }


The same goes for these checks

empwilli · 2025-07-21T07:20:48Z

src/api.rs

+        if fs::exists(&config_file)? {
+            fs::remove_file(&config_file)?;
+        }
+
+        if fs::exists(&backup_file)? {
+            fs::rename(backup_file, config_file)?;
+        }


Couple of things (but maybe I don't get the logic fully here):

I'd copy the backup file instead of moving it, to keep it around, just in case.

I'd remove the existing config file only if there is a backup file or do we have another level of fallback?

Probably it is sensible to make the operation atomic, i.e. copy backup to config file directly?

Another level of fallback would be the default network config in /lib/systemd/network/

empwilli · 2025-07-21T07:22:08Z

src/api.rs

+    ) -> Result<()> {
+        let _ = self.restore_network_setting(network.clone());
+        let _ = self.ods_client.reload_network().await;
+        let _ = certificate::create_module_certificate(Some(network.previous_ip)).await;
+        crate::trigger_server_restart();
+
+        Ok(())


Why have this function return result if the calls don't propagate/handle the errors?

empwilli · 2025-07-21T07:40:06Z

src/api.rs

+        if Path::new(&backup_file).exists() {
+            fs::remove_file(&backup_file).context("Unable to remove backup file")?;
+        }
+
+        if Path::new(&config_file).exists() {
+            fs::rename(config_file, backup_file).context("Failed to back up file")?;
+        } else {


File operations should always be performed atomically, ideally: try to do the operation that you want to perform and handle the unexpected case. In this case I'd do sth. like the following:

if let Err(ref e) = fs::remove_file("/tmp/some/file/path.txt") && e.kind() != ErrorKind::NotFound { ... }

the same goes for renaming/copying the backup file.

I do copy now instead of moving. So if it would fail to write new config, old config will be still available.

empwilli · 2025-07-21T08:00:49Z

src/api.rs

+            .as_secs()
+            + 90; // 90 seconds from now


... + std::Duration::from_seconds(5) ... is longer but IMHO more expressive regarding the intend.

empwilli · 2025-07-21T08:05:36Z

src/api.rs

+#[derive(Serialize, Deserialize, Clone)]
+struct PendingNetworkRollback {
+    network_setting: NetworkSetting,
+    rollback_time: u64,


If I understand the code correctly, the rollback_time is only used by the omnect-ui to check since when there is a network rollback in progress? serde provides serialization/deserialization for std::time::Duration, i.e., I think it would be best to make rollback_time a std::time::Duration and let serde handle the rest. This would simplify the code downstream as you don't have to manually convert via seconds from u64 to duration and back.

empwilli · 2025-07-21T08:08:15Z

src/main.rs

+
+pub fn trigger_server_restart() {
+    if let Some(tx) = SERVER_RESTART_TX.get() {
+        let _ = tx.send(());


IMHO we should log any errors for later analysis.

alexomanie

frontend seems ok for me :)

empwilli · 2025-08-13T07:29:50Z

src/server_control.rs

As far as I understand the code there will never be a situation where we call trigger_server_restart or cancel_rollback_timer and they have not yet been initialized. Is there some test scenario that I overlook?

IMHO the if let Some(...) clauses are then complicated and confusing (as I have to wonder: what if they are not initialized? in what cases are they not initialized?). I'd probably go away from global variables and make a ServerControl struct that is part of API.

Both AbortHandle and broadcast::Sender implement Clone, i.e., their principle idea is, that instead of having a single instance that requires locking for synchornized access, they can be cloned cheaply and thus don't need synchronization (or, more correctly, take care of synchronization under the hood.).

empwilli · 2025-08-13T07:34:57Z

src/api.rs

+        HttpResponse::Ok().finish()
+    }
+
+    fn restore_network_setting(&self, network: NetworkSetting) -> Result<()> {


is there a reason not to pass network by reference?

empwilli

Minor nits, but looks good to me overall :)

empwilli · 2025-08-13T07:36:09Z

src/api.rs

+        Ok(())
+    }
+
+    async fn configure_network_interface(&self, network: NetworkSetting) -> Result<()> {


can we pass here network by reference?

empwilli · 2025-08-13T07:36:41Z

src/api.rs

+
+    async fn handle_server_restart_with_new_certificate(
+        &self,
+        network: NetworkSetting,


pass network by reference

src/api.rs

empwilli · 2025-08-13T07:39:52Z

src/api.rs

+    }
+
+    pub async fn check_and_execute_pending_rollback(&self) -> Result<()> {
+        if let Some(pending) = self.load_pending_rollback() {


IMHO its fine to return early here to reduce nesting, i.e., let Some(pending) = self.load_pending_rollback() else return Ok(());

I'm not sure what the difference to the current implementation is supposed to be.

…and service client

- Bumped versions for @vueuse/core, axios, centrifuge, vue, and vuetify in dependencies. - Updated devDependencies: @biomejs/biome, @types/bun, @vitejs/plugin-vue, @vue/tsconfig, typescript, unocss, vite, vite-plugin-vuetify, and vue-tsc.

…roved performance

Cargo.toml

JanZachmann · 2025-08-28T08:38:53Z

src/api.rs

+            error!("Failed to save pending rollback: {e:#}");
+        }
+
+        let network_clone = network.clone();


please rename to let network = network.clone();

src/api.rs

JanZachmann · 2025-08-28T08:43:03Z

src/api.rs

+    }
+
+    pub async fn check_and_execute_pending_rollback(&self) -> Result<()> {
+        if let Some(pending) = self.load_pending_rollback() {


src/api.rs

JanZachmann · 2025-08-28T09:11:31Z

src/api.rs

+        let config_file = network_path!(format!("10-{}.network", network.name));
+        let backup_file = network_path!(format!("10-{}.network.old", network.name));
+
+        if !fs::exists(&backup_file)? {


sure this is what you want? it might return with an error...

ronny-standtke added 11 commits July 2, 2025 16:29

feat: network settings

68b7495

feat: add network configuration API endpoint and related functionality

2efd8c8

feat: enhance network settings with restore functionality and async s…

956b7b5

…ubmission

feat: bump version to 0.16.0 for omnect-ui

4c0e768

feat: implement network settings overlay and new IP handling function…

c99ebb0

…ality

feat: integrate overlay spinner for network settings submission and f…

a4e64d9

…ix URL reference in healthcheck

feat: add actix-cors support for CORS handling and update dependencies

1c1ef27

feat: implement network rollback functionality and enhance certificat…

3e9c8fc

…e handling

feat: enhance network backup functionality to include current network…

3d5d7ef

… file handling

feat: update network file backup logic to use configured network path

8f468e0

ronny-standtke requested review from alexomanie, JoergZeidler, empwilli and JanZachmann July 18, 2025 11:56

empwilli requested changes Jul 21, 2025

View reviewed changes

alexomanie approved these changes Jul 22, 2025

View reviewed changes

ronny-standtke added 2 commits August 12, 2025 09:04

Merge branch 'main' into feature/network-settings

c3c7fd5

qa

bb393c8

ronny-standtke requested a review from empwilli August 12, 2025 16:02

empwilli reviewed Aug 13, 2025

View reviewed changes

empwilli approved these changes Aug 13, 2025

View reviewed changes

ronny-standtke added 3 commits August 18, 2025 13:11

refactor: enhance type constraints and improve async handling in API …

553a931

…and service client

refactor: update network setting methods to accept references for imp…

4953e6d

…roved performance

ronny-standtke requested a review from empwilli August 26, 2025 08:12

ronny-standtke added 2 commits August 26, 2025 15:43

Merge branch 'main' into feature/network-settings

bf91bb9

Merge branch 'main' into feature/network-settings

21f3ab0

empwilli approved these changes Aug 27, 2025

View reviewed changes

JanZachmann requested changes Aug 28, 2025

View reviewed changes

ronny-standtke added 2 commits August 28, 2025 15:33

resolve findings

7f6d4e9

refactor: simplify method calls by removing unnecessary self references

f9cfe79

ronny-standtke requested a review from JanZachmann August 28, 2025 13:57

chore: update dependencies in package.json

51a8279

feat: network settings #51

Are you sure you want to change the base?

feat: network settings #51

Uh oh!

Conversation

ronny-standtke commented Jul 18, 2025

Network Configuration Management:

Rollback Enhancements:

Dependency Updates:

Configuration Updates:

Miscellaneous Improvements:

Uh oh!

empwilli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexomanie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

empwilli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!