[pve-devel] [PATCH docs/proxmox-firewall 0/3] migrate proxmox-firewall to proxmox-log + introduce subcommands

[pve-devel] [PATCH docs/proxmox-firewall 0/3] migrate proxmox-firewall to proxmox-log + introduce subcommands

[Stefan Hanreich]

Since we now have proxmox-log as the standard crate for logging purposes,
migrate proxmox-firewall to the new logging crate.

The old logging setup was also tied with the debugging mechanisms described in
the documentation. I used that opportunity to implement specific subcommands for
debugging proxmox-firewall, instead of just relying solely on the log output.

The patch for changing to proxmox-log can be applied independently, but this
breaks the commands included in the documentation. That's why I decided to send
them as part of one patch series, because the change to proxmox-log prompted me
to implement the subcommands for debugging in the first place.

proxmox-firewall:

Stefan Hanreich (2):
  firewall: use proxmox_log
  firewall: add subcommands to proxmox-firewall binary

 debian/control                               |   4 +-
 debian/proxmox-firewall.service              |   4 +-
 proxmox-firewall/Cargo.toml                  |   5 +-
 proxmox-firewall/src/bin/proxmox-firewall.rs | 124 ++++++++++++++-----
 proxmox-firewall/src/config.rs               |   2 +
 proxmox-firewall/src/firewall.rs             |   2 +
 proxmox-firewall/src/object.rs               |   2 +
 proxmox-firewall/src/rule.rs                 |   2 +
 8 files changed, 110 insertions(+), 35 deletions(-)


pve-docs:

Stefan Hanreich (1):
  firewall: update 'useful commands' section with new subcommands

 pve-firewall.adoc | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)


Summary over all repositories:
  9 files changed, 134 insertions(+), 49 deletions(-)

-- 
Generated by git-murpp 0.8.0

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

[pve-devel] [PATCH proxmox-firewall 1/2] firewall: use proxmox_log

[Stefan Hanreich]

The firewall did not yet use proxmox-log as its logging provider.
Adapt the existing code to setup logging via proxmox-log over the
current custom solution and import proxmox-log in all files as an
alias to the log cate.

This commit removes the ability to debug proxmox-firewall via running
it in the foreground and dumping the log output, since it now always
gets written to the journal. This will be fixed in a future commit,
that introduces dedicated subcommands for debugging.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 debian/control                               |  3 +-
 debian/proxmox-firewall.service              |  2 --
 proxmox-firewall/Cargo.toml                  |  3 +-
 proxmox-firewall/src/bin/proxmox-firewall.rs | 33 ++++++--------------
 proxmox-firewall/src/config.rs               |  2 ++
 proxmox-firewall/src/firewall.rs             |  2 ++
 proxmox-firewall/src/object.rs               |  2 ++
 proxmox-firewall/src/rule.rs                 |  2 ++
 8 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/debian/control b/debian/control
index 6323a4b..be6e584 100644
--- a/debian/control
+++ b/debian/control
@@ -5,10 +5,9 @@ Maintainer: Proxmox Support Team <support@proxmox.com>
 Build-Depends: cargo:native,
                debhelper-compat (= 13),
                librust-anyhow-1+default-dev,
-               librust-env-logger-0.11+default-dev,
                librust-insta-1+default-dev (>= 1.21-~~),
                librust-insta-1+json-dev (>= 1.21-~~),
-               librust-log-0.4+default-dev,
+               librust-proxmox-log-0.2+default-dev (>= 0.2.9-~~),
                librust-proxmox-sys-0.6+default-dev,
                librust-proxmox-ve-config-dev (>= 0.2.3-~~),
                librust-serde-1+default-dev,
diff --git a/debian/proxmox-firewall.service b/debian/proxmox-firewall.service
index c2dc903..ececa75 100644
--- a/debian/proxmox-firewall.service
+++ b/debian/proxmox-firewall.service
@@ -6,8 +6,6 @@ After=pvefw-logger.service pve-cluster.service network.target systemd-modules-lo
 [Service]
 ExecStart=/usr/libexec/proxmox/proxmox-firewall
 Type=simple
-Environment="RUST_LOG_STYLE=SYSTEMD"
-Environment="RUST_LOG=warn"
 
 [Install]
 WantedBy=multi-user.target
diff --git a/proxmox-firewall/Cargo.toml b/proxmox-firewall/Cargo.toml
index a9abf93..a7031a3 100644
--- a/proxmox-firewall/Cargo.toml
+++ b/proxmox-firewall/Cargo.toml
@@ -11,8 +11,6 @@ description = "Proxmox VE nftables firewall implementation"
 license = "AGPL-3"
 
 [dependencies]
-log = "0.4"
-env_logger = "0.11"
 anyhow = "1"
 
 serde = { version = "1", features = [ "derive" ] }
@@ -20,6 +18,7 @@ serde_json = "1"
 
 signal-hook = "0.3"
 
+proxmox-log = "0.2.9"
 proxmox-nftables = { path = "../proxmox-nftables", features = ["config-ext"] }
 proxmox-ve-config = { workspace = true }
 
diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index 4732e51..70dca73 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -1,4 +1,3 @@
-use std::io::Write;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
 use std::time::{Duration, Instant};
@@ -7,6 +6,8 @@ use anyhow::{Context, Error};
 
 use proxmox_firewall::config::{FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader};
 use proxmox_firewall::firewall::Firewall;
+use proxmox_log as log;
+use proxmox_log::{LevelFilter, Logger};
 use proxmox_nftables::{client::NftError, NftClient};
 
 const RULE_BASE: &str = include_str!("../../resources/proxmox-firewall.nft");
@@ -54,31 +55,14 @@ fn handle_firewall() -> Result<(), Error> {
     Ok(())
 }
 
-fn init_logger() {
-    match std::env::var("RUST_LOG_STYLE") {
-        Ok(s) if s == "SYSTEMD" => env_logger::builder()
-            .format(|buf, record| {
-                writeln!(
-                    buf,
-                    "<{}>{}: {}",
-                    match record.level() {
-                        log::Level::Error => 3,
-                        log::Level::Warn => 4,
-                        log::Level::Info => 6,
-                        log::Level::Debug => 7,
-                        log::Level::Trace => 7,
-                    },
-                    record.target(),
-                    record.args()
-                )
-            })
-            .init(),
-        _ => env_logger::init(),
-    };
+fn init_logger() -> Result<(), Error> {
+    Logger::from_env("PVE_LOG", LevelFilter::WARN)
+        .journald()
+        .init()
 }
 
-fn main() -> Result<(), std::io::Error> {
-    init_logger();
+fn main() -> Result<(), Error> {
+    init_logger()?;
 
     let term = Arc::new(AtomicBool::new(false));
 
@@ -111,4 +95,5 @@ fn main() -> Result<(), std::io::Error> {
     }
 
     remove_firewall()
+        .with_context(|| "Could not remove firewall rules")
 }
diff --git a/proxmox-firewall/src/config.rs b/proxmox-firewall/src/config.rs
index ec9849e..ebfbabb 100644
--- a/proxmox-firewall/src/config.rs
+++ b/proxmox-firewall/src/config.rs
@@ -5,6 +5,8 @@ use std::io::{self, BufReader};
 
 use anyhow::{bail, format_err, Context, Error};
 
+use proxmox_log as log;
+
 use proxmox_ve_config::firewall::bridge::Config as BridgeConfig;
 use proxmox_ve_config::firewall::cluster::Config as ClusterConfig;
 use proxmox_ve_config::firewall::guest::Config as GuestConfig;
diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index 086b96c..4448514 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -3,6 +3,8 @@ use std::fs;
 
 use anyhow::{bail, Error};
 
+use proxmox_log as log;
+
 use proxmox_nftables::command::{Add, Commands, Delete, Flush};
 use proxmox_nftables::expression::{Meta, Payload};
 use proxmox_nftables::helper::NfVec;
diff --git a/proxmox-firewall/src/object.rs b/proxmox-firewall/src/object.rs
index cf7e773..50ae7ba 100644
--- a/proxmox-firewall/src/object.rs
+++ b/proxmox-firewall/src/object.rs
@@ -1,4 +1,6 @@
 use anyhow::{format_err, Error};
+
+use proxmox_log as log;
 use proxmox_nftables::{
     command::{Add, Flush},
     expression::Prefix,
diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index 14ee544..a1db01a 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -1,6 +1,8 @@
 use std::ops::{Deref, DerefMut};
 
 use anyhow::{bail, format_err, Error};
+
+use proxmox_log as log;
 use proxmox_nftables::{
     expression::{Ct, IpFamily, Meta, Payload, Prefix},
     statement::{Log, LogLevel, Match, Operator},
-- 
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: add subcommands to proxmox-firewall binary

[Stefan Hanreich]

pve-firewall supports several subcommands for starting / stopping the
firewall, as well as debugging. proxmox-firewall currently only ran in
foreground when executed and had no real means of dumping the
generated ruleset other than running it with log level trace and
capturing the log output.

The commands introduced in this patch series are:
* start - runs proxmox-firewall in foreground
* skeleton - prints the rule skeleton, included in the binary
* compile - prints the commands generated from the firewall
  configuration files

For now, start retains the exact same behavior as the binary had
before introducing subcommands, so calling it with start as subcommand
is equivalent to invoking it without the start subcommand before.

The skeleton and compile subcommands can be used to dump the nftables
rules generated by proxmox-firewall. They print the ruleset in the
format expected by nft directly to STDOUT, so it can be piped to the
nft binary directly:

$ proxmox-firewall skeleton | nft -f -
$ proxmox-firewall compile | nft -j -f -

start always prints its logs to the journal, all other commands print
the logs to stderr. Since there isn't really a reason anymore to run
proxmox-firewall in the foreground via start (there are now specific
commands for debugging), this should be fine.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 debian/control                               |   1 +
 debian/proxmox-firewall.service              |   2 +-
 proxmox-firewall/Cargo.toml                  |   2 +
 proxmox-firewall/src/bin/proxmox-firewall.rs | 103 +++++++++++++++++--
 4 files changed, 97 insertions(+), 11 deletions(-)

diff --git a/debian/control b/debian/control
index be6e584..15d9cad 100644
--- a/debian/control
+++ b/debian/control
@@ -7,6 +7,7 @@ Build-Depends: cargo:native,
                librust-anyhow-1+default-dev,
                librust-insta-1+default-dev (>= 1.21-~~),
                librust-insta-1+json-dev (>= 1.21-~~),
+               librust-pico-args-0.5+default-dev,
                librust-proxmox-log-0.2+default-dev (>= 0.2.9-~~),
                librust-proxmox-sys-0.6+default-dev,
                librust-proxmox-ve-config-dev (>= 0.2.3-~~),
diff --git a/debian/proxmox-firewall.service b/debian/proxmox-firewall.service
index ececa75..398670f 100644
--- a/debian/proxmox-firewall.service
+++ b/debian/proxmox-firewall.service
@@ -4,7 +4,7 @@ Wants=pve-cluster.service pvefw-logger.service
 After=pvefw-logger.service pve-cluster.service network.target systemd-modules-load.service
 
 [Service]
-ExecStart=/usr/libexec/proxmox/proxmox-firewall
+ExecStart=/usr/libexec/proxmox/proxmox-firewall start
 Type=simple
 
 [Install]
diff --git a/proxmox-firewall/Cargo.toml b/proxmox-firewall/Cargo.toml
index a7031a3..3302060 100644
--- a/proxmox-firewall/Cargo.toml
+++ b/proxmox-firewall/Cargo.toml
@@ -13,6 +13,8 @@ license = "AGPL-3"
 [dependencies]
 anyhow = "1"
 
+pico-args = "0.5"
+
 serde = { version = "1", features = [ "derive" ] }
 serde_json = "1"
 
diff --git a/proxmox-firewall/src/bin/proxmox-firewall.rs b/proxmox-firewall/src/bin/proxmox-firewall.rs
index 70dca73..273daec 100644
--- a/proxmox-firewall/src/bin/proxmox-firewall.rs
+++ b/proxmox-firewall/src/bin/proxmox-firewall.rs
@@ -2,7 +2,8 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
 use std::time::{Duration, Instant};
 
-use anyhow::{Context, Error};
+use anyhow::{bail, format_err, Context, Error};
+use pico_args::Arguments;
 
 use proxmox_firewall::config::{FirewallConfig, PveFirewallConfigLoader, PveNftConfigLoader};
 use proxmox_firewall::firewall::Firewall;
@@ -10,6 +11,18 @@ use proxmox_log as log;
 use proxmox_log::{LevelFilter, Logger};
 use proxmox_nftables::{client::NftError, NftClient};
 
+const HELP: &str = r#"
+USAGE:
+  proxmox-firewall <COMMAND>
+
+COMMANDS:
+  help              Prints this help message.
+  skeleton          Prints the firewall rule skeleton as accepted by 'nft -f -'
+  compile           Compile and print firewall rules as accepted by 'nft -j -f -'
+  start             Execute proxmox-firewall service in foreground
+"#;
+
+
 const RULE_BASE: &str = include_str!("../../resources/proxmox-firewall.nft");
 
 const FORCE_DISABLE_FLAG_FILE: &str = "/run/proxmox-nftables-firewall-force-disable";
@@ -27,10 +40,13 @@ fn remove_firewall() -> Result<(), std::io::Error> {
     Ok(())
 }
 
-fn handle_firewall() -> Result<(), Error> {
+fn create_firewall_instance() -> Result<Firewall, Error> {
     let config = FirewallConfig::new(&PveFirewallConfigLoader::new(), &PveNftConfigLoader::new())?;
+    Ok(Firewall::new(config))
+}
 
-    let firewall = Firewall::new(config);
+fn handle_firewall() -> Result<(), Error> {
+    let firewall = create_firewall_instance()?;
 
     if !firewall.is_enabled() {
         return remove_firewall().with_context(|| "could not remove firewall tables".to_string());
@@ -55,15 +71,19 @@ fn handle_firewall() -> Result<(), Error> {
     Ok(())
 }
 
-fn init_logger() -> Result<(), Error> {
-    Logger::from_env("PVE_LOG", LevelFilter::WARN)
-        .journald()
-        .init()
-}
+fn init_logger(command: Command) -> Result<(), Error> {
+    let mut logger = Logger::from_env("PVE_LOG", LevelFilter::WARN);
 
-fn main() -> Result<(), Error> {
-    init_logger()?;
+    if command == Command::Start {
+        logger = logger.journald();
+    } else {
+        logger = logger.stderr_pve();
+    }
+
+    logger.init()
+}
 
+fn run_firewall() -> Result<(), Error> {
     let term = Arc::new(AtomicBool::new(false));
 
     signal_hook::flag::register(signal_hook::consts::SIGTERM, Arc::clone(&term))?;
@@ -97,3 +117,66 @@ fn main() -> Result<(), Error> {
     remove_firewall()
         .with_context(|| "Could not remove firewall rules")
 }
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum Command {
+    Compile,
+    Help,
+    Skeleton,
+    Start,
+}
+
+impl std::str::FromStr for Command {
+    type Err = Error;
+
+    fn from_str(value: &str) -> Result<Self, Self::Err> {
+        Ok(match value {
+            "help" => Command::Help,
+            "compile" => Command::Compile,
+            "skeleton" => Command::Skeleton,
+            "start" => Command::Start,
+            cmd => {
+                bail!("{cmd} is not a valid command")
+            },
+        })
+    }
+}
+
+fn run_command(command: Command) -> Result<(), Error> {
+    init_logger(command)?;
+
+    match command {
+        Command::Help => {
+            println!("{}", HELP);
+            Ok(())
+        },
+        Command::Compile => {
+            let commands = create_firewall_instance()?.full_host_fw()?;
+            let json = serde_json::to_string_pretty(&commands)?;
+
+            println!("{json}");
+            Ok(())
+        },
+        Command::Skeleton => {
+            println!("{}", RULE_BASE);
+            Ok(())
+        },
+        Command::Start => run_firewall(),
+    }
+}
+
+fn main() -> Result<(), Error> {
+    let mut args = Arguments::from_env();
+
+    let parsed_command = args.subcommand()?
+        .ok_or_else(|| format_err!("no subcommand specified"))?
+        .parse();
+
+    if let Ok(command) = parsed_command {
+        run_command(command)
+    } else {
+        eprintln!("Invalid command specified!\n{}", HELP);
+        std::process::exit(1);
+    }
+
+}
-- 
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 2/2] firewall: add subcommands to proxmox-firewall binary

[Gabriel Goller]

On 14.04.2025 17:44, Stefan Hanreich wrote:
> [snip]
>+fn main() -> Result<(), Error> {
>+    let mut args = Arguments::from_env();
>+
>+    let parsed_command = args.subcommand()?
>+        .ok_or_else(|| format_err!("no subcommand specified"))?

Maybe we could print HELP here again? So that you don't have to guess
the subcommands.

>+        .parse();
>+
>+    if let Ok(command) = parsed_command {
>+        run_command(command)
>+    } else {
>+        eprintln!("Invalid command specified!\n{}", HELP);
>+        std::process::exit(1);
>+    }
>+
>+}

Also this file needs a `cargo fmt` :)

Otherwise the whole series looks good, consider:
Reviewed-by: Gabriel Goller <g.goller@proxmox.com>
Tested-by: Gabriel Goller <g.goller@proxmox.com>


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

[pve-devel] [PATCH pve-docs 1/1] firewall: update 'useful commands' section with new subcommands

[Stefan Hanreich]

Migrating proxmox-firewall to the proxmox_log crate has removed the
RUST_LOG environment variable, among other things. Additionally, the
proxmox-firewall binary now has subcommands for dumping the generated
firewall ruleset. Update the documentation to provide information on
how to use the new subcommands for debugging proxmox-firewall.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 pve-firewall.adoc | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/pve-firewall.adoc b/pve-firewall.adoc
index 47ab3d3..ccf0cb3 100644
--- a/pve-firewall.adoc
+++ b/pve-firewall.adoc
@@ -825,38 +825,48 @@ You can check the generated ruleset via the following command:
 nft list ruleset
 ----
 
-If you want to debug `proxmox-firewall` you can simply run the daemon in
-foreground with the `RUST_LOG` environment variable set to `trace`. This should
-provide you with detailed debugging output:
+If you want to debug `proxmox-firewall` you can dump the commands generated by
+the firewall via the `compile` subcommand. Additionally, setting the PVE_LOG
+environment variable will print log output to STDERR, which can be useful for
+debugging issues during the generation of the nftables ruleset:
 
 ----
-RUST_LOG=trace /usr/libexec/proxmox/proxmox-firewall
+PVE_LOG=trace /usr/libexec/proxmox/proxmox-firewall compile > firewall.json
+----
+
+The nftables ruleset consists of the skeleton ruleset, that is included in the
+proxmox-firewall binary, as well as the rules generated from the firewall
+configuration. You can obtain the base ruleset via the `skeleton` subcommand:
+
+----
+/usr/libexec/proxmox/proxmox-firewall skeleton
+----
+
+The output of both commands can be piped directly to the `nft` executable. The
+following commands will re-create the whole nftables ruleset from scratch:
+
+----
+/usr/libexec/proxmox/proxmox-firewall skeleton | nft -f -
+/usr/libexec/proxmox/proxmox-firewall compile | nft -j -f -
 ----
 
 You can also edit the systemctl service if you want to have detailed output for
-your firewall daemon:
+your firewall daemon while it is running:
 
 ----
 systemctl edit proxmox-firewall
 ----
 
-Then you need to add the override for the `RUST_LOG` environment variable:
+Then you need to add the override for the `PVE_LOG` environment variable:
 
 ----
 [Service]
-Environment="RUST_LOG=trace"
+Environment="PVE_LOG=trace"
 ----
 
 This will generate a large amount of logs very quickly, so only use this for
 debugging purposes. Other, less verbose, log levels are `info` and `debug`.
 
-Running in foreground writes the log output to STDERR, so you can redirect it
-with the following command (e.g. for submitting logs to the community forum):
-
-----
-RUST_LOG=trace /usr/libexec/proxmox/proxmox-firewall 2> firewall_log_$(hostname).txt
-----
-
 It can be helpful to trace packet flow through the different chains in order to
 debug firewall rules. This can be achieved by setting `nftrace` to 1 for packets
 that you want to track. It is advisable that you do not set this flag for *all*
-- 
2.39.5


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