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.
>
> 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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
