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).
Signed-off-by: Laine Stump <laine@redhat.com>
---
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.0On 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@redhat.com>
> ---
> 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 :|
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@redhat.com>
>> ---
>> 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
On Mon, Nov 25, 2024 at 11:56:31AM -0500, Laine Stump wrote: > 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"). Maybe we just shouldn't be pretending this is a firewall command at all ? Even with iptables, this really isn't anything to do with traffic filtering. iptables just happened to be a convenient place to put the logic in the kernel at the time. 'tc' is the new "convenient" place to put the logic today. How about putting a virNetDevFixDHCPChecksum() in virnetdev.h/c ? and just invoking this API after we've setup nftables rules ? 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 :|
On 11/25/24 12:15 PM, Daniel P. Berrangé wrote: > On Mon, Nov 25, 2024 at 11:56:31AM -0500, Laine Stump wrote: >> 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"). > > Maybe we just shouldn't be pretending this is a firewall command at > all ? > > Even with iptables, this really isn't anything to do with traffic > filtering. Well, if you're going to be pedantic and say that the only things that are a part of the "firewall" are those bits that control whether or not the traffic passes, and *not* the bits that modify packets on their way through, then none of the rules that setup NAT should be a part of the firewall either. > iptables just happened to be a convenient place to put > the logic in the kernel at the time. > > 'tc' is the new "convenient" place to put the logic today. How about > putting a virNetDevFixDHCPChecksum() in virnetdev.h/c ? > > and just invoking this API after we've setup nftables rules ? That's kind of where I started out with this (having the tc command run not even as a part of networkAddFirewallRules(), but at the same level), but this required adding a call to the "FixChecksum()' function at each and every place we called networkAddFirewallRules() (along with extra bits to recover from errors). Then I moved the call to FixChecksum() inside of networkAddFirewallRules, and that was obviously better, but made it more obvious that this was just one more external command that needed to be executed, just like all the nft commands, so handling it as a special case (rather than as just one more in the list of commands to run) complicated the code for no good reason. As to whether or not the tc command to fix the dhcp checksum belongs in the virFirewall object - a virFirewall is just a list of commands to be executed to setup (or tear down) packet filtering and packet modification for an interface. One type of packet modification is to implement NAT, and another type of modification is to fix incorrect checksums. Arguably the 2nd item shouldn't need to even exist, but here we are :-P. And if modifying packets for NAT can be considered a part of the firewall, then IMO modifying packet checksums is just as "firewally" (newly invented word of the day - I adjectived a noun!). Also I like having the tc command be a part of the virFirewall object because that means it automatically takes advantage of the "pseudo-atomic" nature of virFirewall, as well as its rollback functionality: 1) error cleanup is simpler - this is just one more rollback command in the virFirewall that gets execute if there is a failure at any point rather than extra code around a one-off virCommand execution that is really just duplicating the code that's already in virFirewallApply(), and 2) the command to remove the tc filter becomes a part of the fwRemoval object saved in the network status, making it much simpler to reliably remove the filter when virtnetworkd is restarted (and we remove/reinstall all the firewall rules) or the network is shutdown. Otherwise we could lose track of whether or not the tc filter was in place or exactly what the filter was and end up doing the wrong thing, e.g. if the backend setting changed from iptables to nftables (or vice versa) during a virtnetworkd restart, or if the tc command to add the filter changed from one release to the next leaving us attempting to remove the old filter with a command that wouldn't actually remove it, and/or attempting (and failing) to add a tc filter when one is already in place. This is actually something that happened when I tried adding the checksum filter by itself with no direct connection to the virFirewall, and part of what led me to make it another virFirewallCmd on the virFirewall's list.
On Mon, Nov 25, 2024 at 06:21:15PM -0500, Laine Stump wrote: > On 11/25/24 12:15 PM, Daniel P. Berrangé wrote: > > On Mon, Nov 25, 2024 at 11:56:31AM -0500, Laine Stump wrote: > > > 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"). > > > > Maybe we just shouldn't be pretending this is a firewall command at > > all ? > > > > Even with iptables, this really isn't anything to do with traffic > > filtering. > > Well, if you're going to be pedantic and say that the only things that are a > part of the "firewall" are those bits that control whether or not the > traffic passes, and *not* the bits that modify packets on their way through, > then none of the rules that setup NAT should be a part of the firewall > either. > > > iptables just happened to be a convenient place to put > > the logic in the kernel at the time. > > > 'tc' is the new "convenient" place to put the logic today. How about > > putting a virNetDevFixDHCPChecksum() in virnetdev.h/c ? > > > > and just invoking this API after we've setup nftables rules ? > > That's kind of where I started out with this (having the tc command run not > even as a part of networkAddFirewallRules(), but at the same level), but ...snip... Ok, since its all messy, I'll defer to your preference :-) 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 :|
© 2016 - 2024 Red Hat, Inc.