Re: [PATCH 4/5] util: add new "raw" layer for virFirewallCmd objects

Laine Stump <laine@xxxxxxxxx> · Mon, 25 Nov 2024 11:56:31 -0500

On 11/25/24 5:44 AM, Daniel P. Berrangé wrote:
On Fri, Nov 22, 2024 at 04:16:38PM -0500, Laine Stump wrote:
If the layer of a FirewallCmd is "raw", then the first arg is the name
of an arbitrary binary to exec, and the rest are the arguments to that
binary.

raw layer doesn't support auto-rollback command creation (any rollback
needs to be added manually with virFirewallAddRollbackCmd()), and also
raw layer isn't supported by the iptables backend (it would have been
straightforward to add, but the iptables backend doesn't need it, and
I didn't want to take the chance of causing a regression in that
code for no good reason).

I guess the obvious question to ask is why you chose to define
a "raw" layer, as opposed to defining a "tc" layer ? Being
more targetted about the anticipated usage feels better IMHO.

I thought about that, but while layer is used to figure out the binary 
name for the iptables backend (because the different layers use 
ebtables, iptables, or ip6tables), in the case of the nftables backend 
all of the different layers use "nft" as the binary, and the layer 
indicates changes in a few of the arguments to that command (and really 
both your suggestion and mine are technically messed up, since the layer 
in the case of this checksum-fix filter should really be "ipv4").

I can easily change it to have a "tc" layer rather than "raw" though; 
I'll do that in the next version and you can see if you prefer it to "raw".



Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
  src/network/network_nftables.c |  1 +
  src/util/virfirewall.c         | 74 +++++++++++++++++++++-------------
  src/util/virfirewall.h         |  1 +
  src/util/virfirewalld.c        |  1 +
  4 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/src/network/network_nftables.c b/src/network/network_nftables.c
index f8b5ab665d..e7ee3cd856 100644
--- a/src/network/network_nftables.c
+++ b/src/network/network_nftables.c
@@ -71,6 +71,7 @@ VIR_ENUM_DECL(nftablesLayer);
  VIR_ENUM_IMPL(nftablesLayer,
                VIR_FIREWALL_LAYER_LAST,
                "",
+              "",
                "ip",
                "ip6",
  );
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index 811b787ecc..48b83715d0 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
@@ -44,6 +44,7 @@ VIR_ENUM_IMPL(virFirewallBackend,
  VIR_ENUM_DECL(virFirewallLayer);
  VIR_ENUM_IMPL(virFirewallLayer,
                VIR_FIREWALL_LAYER_LAST,
+              "raw",
                "ethernet",
                "ipv4",
                "ipv6",
@@ -54,6 +55,7 @@ typedef struct _virFirewallGroup virFirewallGroup;
  VIR_ENUM_DECL(virFirewallLayerCommand);
  VIR_ENUM_IMPL(virFirewallLayerCommand,
                VIR_FIREWALL_LAYER_LAST,
+              "",
                EBTABLES,
                IPTABLES,
                IP6TABLES,
@@ -591,6 +593,7 @@ virFirewallCmdIptablesApply(virFirewall *firewall,
      case VIR_FIREWALL_LAYER_IPV6:
          virCommandAddArg(cmd, "-w");
          break;
+    case VIR_FIREWALL_LAYER_RAW:
      case VIR_FIREWALL_LAYER_LAST:
          break;
      }
@@ -672,43 +675,58 @@ virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED,
      size_t i;
      int status;
  
-    cmd = virCommandNew(NFT);
+    if (fwCmd->layer == VIR_FIREWALL_LAYER_RAW) {
  
-    if ((virFirewallTransactionGetFlags(firewall) & VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK) &&
-        fwCmd->argsLen > 1) {
-        /* skip any leading options to get to command verb */
-        for (i = 0; i < fwCmd->argsLen - 1; i++) {
-            if (fwCmd->args[i][0] != '-')
-                break;
-        }
+        /* for VIR_FIREWALL_LAYER_RAW, args[0] is the binary name
+         * and the rest are the args to that command
+         */
+        cmd = virCommandNew(fwCmd->args[0]);
  
-        if (i + 1 < fwCmd->argsLen &&
-            VIR_NFTABLES_ARG_IS_CREATE(fwCmd->args[i])) {
+        /* NB: RAW commands don't support auto-rollback command creation */
  
-            cmdIdx = i;
-            objectType = fwCmd->args[i + 1];
+        for (i = 1; i < fwCmd->argsLen; i++)
+            virCommandAddArg(cmd, fwCmd->args[i]);
  
-            /* we currently only handle auto-rollback for rules,
-             * chains, and tables, and those all can be "rolled
-             * back" by a delete command using the handle that is
-             * returned when "-ae" is added to the add/insert
-             * command.
-             */
-            if (STREQ_NULLABLE(objectType, "rule") ||
-                STREQ_NULLABLE(objectType, "chain") ||
-                STREQ_NULLABLE(objectType, "table")) {
+    } else {
+
+        cmd = virCommandNew(NFT);
+
+        if ((virFirewallTransactionGetFlags(firewall) & VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK) &&
+            fwCmd->argsLen > 1) {
+            /* skip any leading options to get to command verb */
+            for (i = 0; i < fwCmd->argsLen - 1; i++) {
+                if (fwCmd->args[i][0] != '-')
+                    break;
+            }
+
+            if (i + 1 < fwCmd->argsLen &&
+                VIR_NFTABLES_ARG_IS_CREATE(fwCmd->args[i])) {
  
-                needRollback = true;
-                /* this option to nft instructs it to add the
-                 * "handle" of the created object to stdout
+                cmdIdx = i;
+                objectType = fwCmd->args[i + 1];
+
+                /* we currently only handle auto-rollback for rules,
+                 * chains, and tables, and those all can be "rolled
+                 * back" by a delete command using the handle that is
+                 * returned when "-ae" is added to the add/insert
+                 * command.
                   */
-                virCommandAddArg(cmd, "-ae");
+                if (STREQ_NULLABLE(objectType, "rule") ||
+                    STREQ_NULLABLE(objectType, "chain") ||
+                    STREQ_NULLABLE(objectType, "table")) {
+
+                    needRollback = true;
+                    /* this option to nft instructs it to add the
+                     * "handle" of the created object to stdout
+                     */
+                    virCommandAddArg(cmd, "-ae");
+                }
              }
          }
-    }
  
-    for (i = 0; i < fwCmd->argsLen; i++)
-        virCommandAddArg(cmd, fwCmd->args[i]);
+        for (i = 0; i < fwCmd->argsLen; i++)
+            virCommandAddArg(cmd, fwCmd->args[i]);
+    }
  
      cmdStr = virCommandToString(cmd, false);
      VIR_INFO("Applying '%s'", NULLSTR(cmdStr));
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index bce51259d2..636337e13e 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -36,6 +36,7 @@ typedef struct _virFirewall virFirewall;
  typedef struct _virFirewallCmd virFirewallCmd;
  
  typedef enum {
+    VIR_FIREWALL_LAYER_RAW,
      VIR_FIREWALL_LAYER_ETHERNET,
      VIR_FIREWALL_LAYER_IPV4,
      VIR_FIREWALL_LAYER_IPV6,
diff --git a/src/util/virfirewalld.c b/src/util/virfirewalld.c
index 0a886780ad..21a9e02061 100644
--- a/src/util/virfirewalld.c
+++ b/src/util/virfirewalld.c
@@ -43,6 +43,7 @@ VIR_LOG_INIT("util.firewalld");
  VIR_ENUM_DECL(virFirewallLayerFirewallD);
  VIR_ENUM_IMPL(virFirewallLayerFirewallD,
                VIR_FIREWALL_LAYER_LAST,
+              "",
                "eb",
                "ipv4",
                "ipv6",
--
2.47.0


With regards,
Daniel







