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

[pve-devel] [PATCH proxmox-firewall v2 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>
---

v2:
 - nothing changed

 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 v2 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>
---

v2, thanks @Stefan:
 - moved delete_unused_sets function and invocation into Firewall
   struct – this makes the bin files much cleaner

 proxmox-firewall/src/firewall.rs | 61 ++++++++++++++++++++++++++++++--
 proxmox-nftables/src/client.rs   |  9 +++--
 proxmox-nftables/src/types.rs    | 33 ++++++++++++++---
 3 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index bb5402393a1f..a457de7cd88d 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -1,9 +1,10 @@
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 use std::fs;
 
 use anyhow::{bail, Error};
 
-use proxmox_nftables::command::{Add, Commands, Delete, Flush};
+use proxmox_nftables::client::NftError;
+use proxmox_nftables::command::{Add, Commands, Delete, Flush, List, ListOutput};
 use proxmox_nftables::expression::{Meta, Payload};
 use proxmox_nftables::helper::NfVec;
 use proxmox_nftables::statement::{AnonymousLimit, Log, LogLevel, Match, Set, SetOperation};
@@ -11,7 +12,7 @@ use proxmox_nftables::types::{
     AddElement, AddRule, ChainPart, MapValue, RateTimescale, SetName, TableFamily, TableName,
     TablePart, Verdict,
 };
-use proxmox_nftables::{Expression, Statement};
+use proxmox_nftables::{Command, Expression, NftClient, Statement};
 
 use proxmox_ve_config::host::types::BridgeName;
 
@@ -201,6 +202,55 @@ impl Firewall {
         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(&self, commands: &Commands) -> Result<Commands, NftError> {
+        let mut delete_commands = Commands::default();
+
+        // 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),
+        }
+    }
+
     pub fn full_host_fw(&self) -> Result<Commands, Error> {
         let mut commands = Commands::default();
 
@@ -344,6 +394,11 @@ impl Firewall {
             self.create_bridge_chain(&mut commands, bridge_name, bridge_config)?;
         }
 
+        match self.delete_unused_sets(&commands) {
+            Ok(mut delete_commands) => commands.append(&mut delete_commands),
+            Err(err) => log::error!("error deleting unused sets {err:?}"),
+        }
+
         Ok(commands)
     }
 
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

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

[Fabian Grünbichler]

On October 31, 2024 11:10 am, 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>
> ---
> 
> v2:
>  - nothing changed
> 
>  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" }

where does this part come from? not on current master at least ;)
adapted it to the current Cargo.toml and applied, since the resulting
.deb files are identical anyhow..


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

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

[Gabriel Goller]

On 31.10.2024 14:04, Fabian Grünbichler wrote:
>On October 31, 2024 11:10 am, 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>
>> ---
>>
>> v2:
>>  - nothing changed
>>
>>  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" }
>
>where does this part come from? not on current master at least ;)
>adapted it to the current Cargo.toml and applied, since the resulting
>.deb files are identical anyhow..

Oops, forgot to mention it – this (the second patch) obviously depends
on the ipset series by Stefan.
(https://lore.proxmox.com/pve-devel/20241010155637.255451-1-s.hanreich@proxmox.com/.)


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