[pve-devel] [PATCH proxmox-firewall 1/2] firewall: cargo: use new cargo feature resolver

[pve-devel] [PATCH proxmox-firewall 1/2] firewall: cargo: use new cargo feature resolver

[Gabriel Goller]

Virtual cargo workspaces (workspaces without a [package] section)
default to the cargo feature resolver "1" – even though this outputs a
warning on every cargo invocation. To remove the warning, explicitly set
the resolver to version "2".

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 Cargo.toml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Cargo.toml b/Cargo.toml
index 8fba806ad2a5..3894d9f703a1 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -3,6 +3,7 @@ members = [
     "proxmox-nftables",
     "proxmox-firewall",
 ]
+resolver = "2"
 
 [workspace.dependencies]
 proxmox-ve-config = { version = "0.1.0" }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-firewall 2/2] firewall: delete unused sets

[Gabriel Goller]

Delete unused sets in nftables. We check if there are existing sets
which will not be recreated on the next iteration and delete them.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-firewall/src/bin/proxmox-firewall.rs | 60 +++++++++++++++++++-
 proxmox-nftables/src/client.rs               |  9 ++-
 proxmox-nftables/src/types.rs                | 33 +++++++++--
 3 files changed, 95 insertions(+), 7 deletions(-)

diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index 4732e51984b5..5871da677067 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -1,3 +1,4 @@
+use std::collections::HashSet;
 use std::io::Write;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
@@ -7,6 +8,9 @@ use anyhow::{Context, Error};
 
 use proxmox_firewall::config::{FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader};
 use proxmox_firewall::firewall::Firewall;
+use proxmox_nftables::command::{Add, Commands, Delete, List, ListOutput};
+use proxmox_nftables::types::SetName;
+use proxmox_nftables::Command;
 use proxmox_nftables::{client::NftError, NftClient};
 
 const RULE_BASE: &str = include_str!("../../resources/proxmox-firewall.nft");
@@ -26,6 +30,55 @@ fn remove_firewall() -> Result<(), std::io::Error> {
     Ok(())
 }
 
+/// Fetch all the sets that currently exist, then get all the sets that will be added in the next
+/// interation. Make the difference between them and remove the unused ones.
+fn delete_unused_sets(commands: &Commands) -> Result<Commands, NftError> {
+    let mut delete_commands = Commands::new(vec![]);
+
+    // get sets that will be added
+    let new_sets = commands
+        .iter()
+        .filter_map(|w| match w {
+            Command::Add(Add::Set(x)) => Some(x.config.name()),
+            _ => None,
+        })
+        .collect::<HashSet<&SetName>>();
+
+    // get existing sets
+    let list_commands = Commands::new(vec![List::sets()]);
+    let existing_sets = NftClient::run_json_commands(&list_commands);
+    match existing_sets {
+        Ok(Some(existing_sets)) => {
+            let existing_sets = existing_sets
+                .nftables
+                .iter()
+                .filter_map(|item| {
+                    if let ListOutput::Set(e) = item {
+                        Some(e.name())
+                    } else {
+                        None
+                    }
+                })
+                .collect::<HashSet<&SetName>>();
+            let to_delete = existing_sets.difference(&new_sets);
+
+            log::debug!("existing sets: {:#?}", existing_sets);
+            log::debug!("new sets: {:#?}", new_sets);
+            log::debug!("these sets are going to be deleted: {:#?}", to_delete);
+
+            for set in to_delete.into_iter().cloned().cloned() {
+                delete_commands.push(Command::Delete(Delete::Set(set)));
+            }
+            Ok(delete_commands)
+        }
+        Ok(None) => {
+            log::debug!("no sets exist");
+            Ok(delete_commands)
+        }
+        Err(err) => Err(err),
+    }
+}
+
 fn handle_firewall() -> Result<(), Error> {
     let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
 
@@ -38,13 +91,18 @@ fn handle_firewall() -> Result<(), Error> {
     log::info!("creating the firewall skeleton");
     NftClient::run_commands(RULE_BASE)?;
 
-    let commands = firewall.full_host_fw()?;
+    let mut commands = firewall.full_host_fw()?;
 
     log::info!("Running proxmox-firewall commands");
     for (idx, c) in commands.iter().enumerate() {
         log::debug!("cmd #{idx} {}", serde_json::to_string(&c)?);
     }
 
+    match delete_unused_sets(&commands) {
+        Ok(mut delete_commands) => commands.append(&mut delete_commands),
+        Err(err) => log::error!("error deleting unused sets {err:?}"),
+    }
+
     let response = NftClient::run_json_commands(&commands)?;
 
     if let Some(output) = response {
diff --git a/proxmox-nftables/src/client.rs b/proxmox-nftables/src/client.rs
index eaa3dd2136ee..0fd950cdc1e1 100644
--- a/proxmox-nftables/src/client.rs
+++ b/proxmox-nftables/src/client.rs
@@ -52,8 +52,13 @@ impl NftClient {
         let output = Self::execute_nft_commands(true, &json)?;
 
         if !output.is_empty() {
-            let parsed_output: Option<CommandOutput> = serde_json::from_str(&output).ok();
-            return Ok(parsed_output);
+            return match serde_json::from_str::<CommandOutput>(&output) {
+                Ok(output) => Ok(Some(output)),
+                Err(err) => {
+                    log::error!("Error deserializing output: {err:#?}");
+                    Ok(None)
+                }
+            };
         }
 
         Ok(None)
diff --git a/proxmox-nftables/src/types.rs b/proxmox-nftables/src/types.rs
index 320c757c7cba..258bfc899734 100644
--- a/proxmox-nftables/src/types.rs
+++ b/proxmox-nftables/src/types.rs
@@ -19,7 +19,7 @@ use proxmox_ve_config::guest::types::Vmid;
 #[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd, Ord, Deserialize, Serialize)]
 pub struct Handle(i32);
 
-#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd, Ord, Deserialize, Serialize)]
+#[derive(Clone, Copy, Debug, Eq, PartialEq, PartialOrd, Ord, Deserialize, Serialize, Hash)]
 #[serde(rename_all = "lowercase")]
 pub enum TableFamily {
     Ip,
@@ -210,7 +210,7 @@ impl TableName {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct TablePart {
     family: TableFamily,
     table: String,
@@ -564,7 +564,7 @@ impl DerefMut for AddMap {
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct AddSet {
     #[serde(flatten)]
-    config: SetConfig,
+    pub config: SetConfig,
 
     #[serde(default, skip_serializing_if = "Vec::is_empty")]
     elem: NfVec<SetElem>,
@@ -602,7 +602,7 @@ impl AddSet {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq, Hash)]
 pub struct SetName {
     #[serde(flatten)]
     table: TablePart,
@@ -909,9 +909,34 @@ impl ListChain {
 }
 
 #[derive(Clone, Debug, Deserialize, Serialize)]
+#[serde(rename_all = "kebab-case")]
 pub struct ListSet {
     #[serde(flatten)]
     name: SetName,
+
+    #[serde(rename = "type", default, skip_serializing_if = "Vec::is_empty")]
+    ty: NfVec<ElementType>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    policy: Option<SetPolicy>,
+
+    #[serde(skip_serializing_if = "Vec::is_empty", default)]
+    flags: Vec<SetFlag>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    handle: Option<i64>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    elem: Option<NfVec<Expression>>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    timeout: Option<i64>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    gc_interval: Option<i64>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    size: Option<i64>,
 }
 
 impl ListSet {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-firewall 1/2] firewall: cargo: use new cargo feature resolver

[Gabriel Goller]

On 30.10.2024 15:12, Gabriel Goller wrote:
>Virtual cargo workspaces (workspaces without a [package] section)
>default to the cargo feature resolver "1" – even though this outputs a
>warning on every cargo invocation. To remove the warning, explicitly set
>the resolver to version "2".
>
>Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>---
> Cargo.toml | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/Cargo.toml b/Cargo.toml
>index 8fba806ad2a5..3894d9f703a1 100644
>--- a/Cargo.toml
>+++ b/Cargo.toml
>@@ -3,6 +3,7 @@ members = [
>     "proxmox-nftables",
>     "proxmox-firewall",
> ]
>+resolver = "2"
>
> [workspace.dependencies]
> proxmox-ve-config = { version = "0.1.0" }
>-- 
>2.39.5
>
>
>
>_______________________________________________
>pve-devel mailing list
>pve-devel@lists.proxmox.com
>https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

v2 for both patches on the list!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel