On Wed, May 03, 2023 at 04:21:28PM +0100, Daniel P. Berrangé wrote:
> On Sun, Apr 30, 2023 at 11:19:23PM -0400, Laine Stump wrote:
> > This is the only iptables-specific function in all of
> > virfirewall.c. By moving it to viriptables.c (with appropriate
> > renaming), and calling it indirectly through a similarly named wrapper
> > function in virnetfilter.c, we have made virfirewall.c backend
> > agnostic (the new wrapper function will soon be calling either
> > virIptablesApplyFirewallRule() or (to-be-created)
> > virNftablesApplyFirewallRule() depending on the backend chosen when
> > creating the virFirewall object).
> >
> > Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> > ---
> > src/libvirt_private.syms | 2 ++
> > src/util/virfirewall.c | 72 ++-----------------------------------
> > src/util/viriptables.c | 78 ++++++++++++++++++++++++++++++++++++++++
> > src/util/viriptables.h | 6 ++++
> > src/util/virnetfilter.c | 19 ++++++++++
> > src/util/virnetfilter.h | 3 ++
>
> I don't much like this split of responsibilities.
>
> With the current codebase
>
> * virfirewall.c is the low level transactional interface for
> interacting with firewalls.
>
> * viriptables.c is a medium level interface providing helpers
> needed by the network bridge driver
>
> The viriptables.c file probably ought not to even be located
> in the src/util directory. Its API is inherantly tied to the
> bridge driver, so ought to be moved to src/network/bridge_iptables.c
> I think.
>
> IOW, we have a clean flow from high level to low level of
>
> bridge_driver.c -> viriptables.c -> virfirewall.c
>
> and
>
> nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c
>
> After this change, AFAICT we have dependancy loops
>
> * virfirewall.c is the low level transactional interface for
> interacting with firwalls
>
> * viriptables.c is a medium level interface providing helpers
> needed by the netfilter APIs, and also helpers needed by
> virfirewall.c
>
> * virnetfilter.c is a slightly higher level inteface
> providing helpers needed by the bridge interface
>
> IOW, AFAICT we now have
>
> bridge-driver.c -> virnetfilter.c -> viriptables.c -> virfirewall.c
> ^ |
> | |
> \-----------------/
>
> and
>
> nwfilter_driver.c -> nwfilter_ebiptables_driver.c -> virfirewall.c -> viriptables.c
Looking at the overall end of this series, IMHO, we can just
drop this patch without any problem. The function that is being
moved here does not rely on any of the other code that exists
in the iptables.c file, and does rely on stuff in virfirewall.c
The only reason to move it appears to be because the logic is
related to iptables, and I don't think that's compellling when
the downside is creatin of a circular dependancy.
>
> > 6 files changed, 110 insertions(+), 70 deletions(-)
> >
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 11b84a866a..cf68e4c942 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -2550,6 +2550,7 @@ virInitctlSetRunLevel;
> > iptablesAddOutputFixUdpChecksum;
> > iptablesRemoveOutputFixUdpChecksum;
> > iptablesSetupPrivateChains;
> > +virIptablesApplyFirewallRule;
> >
> >
> > # util/viriscsi.h
> > @@ -2949,6 +2950,7 @@ virNetfilterAddTcpInput;
> > virNetfilterAddTcpOutput;
> > virNetfilterAddUdpInput;
> > virNetfilterAddUdpOutput;
> > +virNetfilterApplyFirewallRule;
> > virNetfilterRemoveDontMasquerade;
> > virNetfilterRemoveForwardAllowCross;
> > virNetfilterRemoveForwardAllowIn;
> > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > index e3ba8f7846..6603fd6341 100644
> > --- a/src/util/virfirewall.c
> > +++ b/src/util/virfirewall.c
> > @@ -24,6 +24,7 @@
> >
> > #include "virfirewall.h"
> > #include "virfirewalld.h"
> > +#include "virnetfilter.h"
> > #include "viralloc.h"
> > #include "virerror.h"
> > #include "vircommand.h"
> > @@ -37,14 +38,6 @@ VIR_LOG_INIT("util.firewall");
> >
> > typedef struct _virFirewallGroup virFirewallGroup;
> >
> > -VIR_ENUM_DECL(virFirewallLayerCommand);
> > -VIR_ENUM_IMPL(virFirewallLayerCommand,
> > - VIR_FIREWALL_LAYER_LAST,
> > - EBTABLES,
> > - IPTABLES,
> > - IP6TABLES,
> > -);
> > -
> > struct _virFirewallRule {
> > virFirewallLayer layer;
> >
> > @@ -500,67 +493,6 @@ virFirewallRuleToString(const char *cmd,
> > }
> >
> >
> > -static int
> > -virFirewallApplyRuleDirect(virFirewallRule *rule,
> > - char **output)
> > -{
> > - size_t i;
> > - const char *bin = virFirewallLayerCommandTypeToString(rule->layer);
> > - g_autoptr(virCommand) cmd = NULL;
> > - g_autofree char *cmdStr = NULL;
> > - int status;
> > - g_autofree char *error = NULL;
> > -
> > - if (!bin) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("Unknown firewall layer %1$d"),
> > - rule->layer);
> > - return -1;
> > - }
> > -
> > - cmd = virCommandNewArgList(bin, NULL);
> > -
> > - /* lock to assure nobody else is messing with the tables while we are */
> > - switch (rule->layer) {
> > - case VIR_FIREWALL_LAYER_ETHERNET:
> > - virCommandAddArg(cmd, "--concurrent");
> > - break;
> > - case VIR_FIREWALL_LAYER_IPV4:
> > - case VIR_FIREWALL_LAYER_IPV6:
> > - virCommandAddArg(cmd, "-w");
> > - break;
> > - case VIR_FIREWALL_LAYER_LAST:
> > - break;
> > - }
> > -
> > - for (i = 0; i < rule->argsLen; i++)
> > - virCommandAddArg(cmd, rule->args[i]);
> > -
> > - cmdStr = virCommandToString(cmd, false);
> > - VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
> > -
> > - virCommandSetOutputBuffer(cmd, output);
> > - virCommandSetErrorBuffer(cmd, &error);
> > -
> > - if (virCommandRun(cmd, &status) < 0)
> > - return -1;
> > -
> > - if (status != 0) {
> > - if (virFirewallRuleGetIgnoreErrors(rule)) {
> > - VIR_DEBUG("Ignoring error running command");
> > - } else {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("Failed to apply firewall rules %1$s: %2$s"),
> > - NULLSTR(cmdStr), NULLSTR(error));
> > - VIR_FREE(*output);
> > - return -1;
> > - }
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -
> > static int
> > virFirewallApplyRule(virFirewall *firewall,
> > virFirewallRule *rule)
> > @@ -568,7 +500,7 @@ virFirewallApplyRule(virFirewall *firewall,
> > g_autofree char *output = NULL;
> > g_auto(GStrv) lines = NULL;
> >
> > - if (virFirewallApplyRuleDirect(rule, &output) < 0)
> > + if (virNetfilterApplyFirewallRule(firewall, rule, &output) < 0)
> > return -1;
> >
> > if (rule->queryCB && output) {
> > diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> > index a0c35887c5..9c7f7790c4 100644
> > --- a/src/util/viriptables.c
> > +++ b/src/util/viriptables.c
> > @@ -31,6 +31,8 @@
> > #include "viriptables.h"
> > #include "virfirewalld.h"
> > #include "virerror.h"
> > +#include "viralloc.h"
> > +#include "vircommand.h"
> > #include "virlog.h"
> > #include "virhash.h"
> > #include "virenum.h"
> > @@ -40,6 +42,19 @@ VIR_LOG_INIT("util.iptables");
> > #define VIR_FROM_THIS VIR_FROM_NONE
> >
> >
> > +/* iptables backend uses a different program for each layer. This
> > + * gives us a convenient function for converting VIR_FIREWALL_LAYER_*
> > + * enum from a virFirewallRule into a binary name.
> > + */
> > +VIR_ENUM_DECL(virIptablesLayerCommand);
> > +VIR_ENUM_IMPL(virIptablesLayerCommand,
> > + VIR_FIREWALL_LAYER_LAST,
> > + EBTABLES,
> > + IPTABLES,
> > + IP6TABLES,
> > +);
> > +
> > +
> > VIR_ENUM_DECL(virIptablesAction);
> > VIR_ENUM_IMPL(virIptablesAction,
> > VIR_FIREWALL_ACTION_LAST,
> > @@ -49,6 +64,69 @@ VIR_ENUM_IMPL(virIptablesAction,
> > );
> >
> >
> > +int
> > +virIptablesApplyFirewallRule(virFirewall *firewall G_GNUC_UNUSED,
> > + virFirewallRule *rule,
> > + char **output)
> > +{
> > + virFirewallLayer layer = virFirewallRuleGetLayer(rule);
> > + const char *bin = virIptablesLayerCommandTypeToString(layer);
> > + g_autoptr(virCommand) cmd = NULL;
> > + g_autofree char *cmdStr = NULL;
> > + g_autofree char *error = NULL;
> > + size_t i, count;
> > + int status;
> > +
> > + if (!bin) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Unknown firewall layer %1$d"), layer);
> > + return -1;
> > + }
> > +
> > + cmd = virCommandNewArgList(bin, NULL);
> > +
> > + /* lock to assure nobody else is messing with the tables while we are */
> > + switch (layer) {
> > + case VIR_FIREWALL_LAYER_ETHERNET:
> > + virCommandAddArg(cmd, "--concurrent");
> > + break;
> > + case VIR_FIREWALL_LAYER_IPV4:
> > + case VIR_FIREWALL_LAYER_IPV6:
> > + virCommandAddArg(cmd, "-w");
> > + break;
> > + case VIR_FIREWALL_LAYER_LAST:
> > + break;
> > + }
> > +
> > + count = virFirewallRuleGetArgCount(rule);
> > + for (i = 0; i < count; i++)
> > + virCommandAddArg(cmd, virFirewallRuleGetArg(rule, i));
> > +
> > + cmdStr = virCommandToString(cmd, false);
> > + VIR_INFO("Applying rule '%s'", NULLSTR(cmdStr));
> > +
> > + virCommandSetOutputBuffer(cmd, output);
> > + virCommandSetErrorBuffer(cmd, &error);
> > +
> > + if (virCommandRun(cmd, &status) < 0)
> > + return -1;
> > +
> > + if (status != 0) {
> > + if (virFirewallRuleGetIgnoreErrors(rule)) {
> > + VIR_DEBUG("Ignoring error running command");
> > + } else {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to apply firewall rules %1$s: %2$s"),
> > + NULLSTR(cmdStr), NULLSTR(error));
> > + VIR_FREE(*output);
> > + return -1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > typedef struct {
> > const char *parent;
> > const char *child;
> > diff --git a/src/util/viriptables.h b/src/util/viriptables.h
> > index 17f43a8fa8..990cb2e25d 100644
> > --- a/src/util/viriptables.h
> > +++ b/src/util/viriptables.h
> > @@ -24,6 +24,12 @@
> > #include "virfirewall.h"
> > #include "virnetfilter.h"
> >
> > +/* virIptablesApplyFirewallRule should be called only from virnetfilter.c */
> > +int
> > +virIptablesApplyFirewallRule(virFirewall *firewall,
> > + virFirewallRule *rule,
> > + char **output);
> > +
> > /* These functions are (currently) called directly from the consumer
> > * (e.g. the network driver), and only when the iptables backend is
> > * selected. (Possibly/probably functions should be added to the
> > diff --git a/src/util/virnetfilter.c b/src/util/virnetfilter.c
> > index 10c1a54e26..ba0f292ea9 100644
> > --- a/src/util/virnetfilter.c
> > +++ b/src/util/virnetfilter.c
> > @@ -44,6 +44,25 @@ VIR_LOG_INIT("util.netfilter");
> > #define VIR_FROM_THIS VIR_FROM_NONE
> >
> >
> > +/**
> > + * virNetfilterApplyFirewallRule:
> > + * @fw: the virFirewall this rule is part of (currently unused)
> > + * @rule: this particular rule
> > + * @ignoreErrors: true if errors should be ignored
> > + * @output: everything that appears on stdout as a result of applying the rule
> > + *
> > + * Applies @rule to the host's network filtering. returns 0 on success
> > + * -1 on failure.
> > + */
> > +int
> > +virNetfilterApplyFirewallRule(virFirewall *fw,
> > + virFirewallRule *rule,
> > + char **output)
> > +{
> > + return virIptablesApplyFirewallRule(fw, rule, output);
> > +}
> > +
> > +
> > /**
> > * virNetfilterAddTcpInput:
> > * @ctx: pointer to the IP table context
> > diff --git a/src/util/virnetfilter.h b/src/util/virnetfilter.h
> > index b515512ad7..eff047cde0 100644
> > --- a/src/util/virnetfilter.h
> > +++ b/src/util/virnetfilter.h
> > @@ -30,6 +30,9 @@
> > #define VIR_NETFILTER_FWD_X_CHAIN "LIBVIRT_FWX"
> > #define VIR_NETFILTER_NAT_POSTROUTE_CHAIN "LIBVIRT_PRT"
> >
> > +int virNetfilterApplyFirewallRule (virFirewall *fw,
> > + virFirewallRule *rule,
> > + char **output);
> > void virNetfilterAddTcpInput (virFirewall *fw,
> > virFirewallLayer layer,
> > const char *iface,
> > --
> > 2.39.2
> >
>
> 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 :|
>
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 :|
