[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4] network: use firewalld instead of iptables, when available



On 08/21/2012 11:24 AM, Thomas Woerner wrote:
> Hello Laine,
>
> On 08/16/2012 08:18 AM, Laine Stump wrote:
>> From: Thomas Woerner <twoerner redhat com>
>>
>> (This is Thomas v3 version of 1/2 of the firewalld patches, modified
>> to check for firewall-cmd and firewalld state only once, rather than
>> every time an iptables rule is added or removed. It's not intended to
>> be pushed, because I'm still having issues with it, at least on my
>> machine. I'm mostly concerned with item (1) on the list below; the
>> others could be solved later or tolerated.)
>>
>> * configure.ac, spec file: firewalld defaults to enabled if dbus is
>>    available, otherwise is disabled. If --with_firewalld is explicitly
>>    requested and dbus is not available, configure will fail.
>>
>> * bridge_driver: add dbus filters to get the FirewallD1.Reloaded
>>    signal and DBus.NameOwnerChanged on org.fedoraproject.FirewallD1.
>>    When these are encountered, reload all the iptables reuls of all
>>    libvirt's virtual networks (similar to what happens when libvirtd is
>>    restarted).
>>
>> * iptables, ebtables: use firewall-cmd's direct passthrough interface
>>    when available, otherwise use iptables and ebtables commands. This
>>    decision is made once the first time libvirt calls
>>    iptables/ebtables, and that decision is maintained for the life of
>>    libvirtd.
>>
>> * Note that the nwfilter part of this patch was separated out into
>>    another patch by Stefan in V2, so that needs to be revised and
>>    re-reviewed as well.
>>
>> ================
>>
>> All the configure.ac and specfile changes are unchanged from Thomas'
>> V3.
>>
>> V3 re-ran "firewall-cmd --state" every time a new rule was added,
>> which was extremely inefficient.  V4 uses VIR_ONCE_GLOBAL_INIT to set
>> up a one-time initialization function.
>>
>> The VIR_ONCE_GLOBAL_INIT(x) macro references a static function called
>> vir(Ip|Eb)OnceInit(), which will then be called the first time that
>> the static function vir(Ip|Eb)TablesInitialize() is called (that
>> function is defined for you by the macro). This is
>> thread-safe, so there is no chance of any race.
>>
>> IMPORTANT NOTE: I've left the VIR_DEBUG messages in these two init
>> functions (one for iptables, on for ebtables) as VIR_WARN so that I
>> don't have to turn on all the other debug message just to see
>> these. Even if this patch doesn't need any other modification, those
>> messages need to be changed to VIR_DEBUG before pushing.
>>
>> This one-time initialization works well. However, I've encountered
>> problems with testing:
>>
>> 1) Whenever I have enabled the firewalld service, *all* attempts to
>> call firewall-cmd from within libvirtd end with firewall-cmd hanging
>> internally somewhere. This is *not* the case if firewall-cmd returns
>> non-0 in response to "firewall-cmd --state" (i.e. *that* command runs
>> and returns to libvirt successfully.)
>>
>> 2) If I start libvirtd while firewalld is stopped, then start
>> firewalld later, this triggers libvirtd to reload its iptables rules,
>> however it also spits out a *ton* of complaints about deletion failing
>> (I suppose because firewalld has nuked all of libvirt's rules). I
>> guess we need to suppress those messages (which is a more annoying
>> problem to fix than you might think, but that's another story).
>>
>> 3) I noticed a few times during this long line of errors that
>> firewalld made a complaint about "Resource Temporarily
>> unavailable. Having libvirtd access iptables commands directly at the
>> same time as firewalld is doing so is apparently problematic.
>>
>> 4) In general, I'm concerned about the "set it once and never change
>> it" method - if firewalld is disabled at libvirtd startup, causing
>> libvirtd to always use iptables/ebtables directly, this won't cause
>> *terrible* problems, but if libvirtd decides to use firewall-cmd and
>> firewalld is later disabled, libvirtd will not be able to recover.
>> ---
>>   AUTHORS                     |  2 +-
>>   configure.ac                | 17 +++++++++++++++
>>   libvirt.spec.in             | 11 ++++++++++
>>   src/Makefile.am             |  4 ++--
>>   src/network/bridge_driver.c | 49
>> ++++++++++++++++++++++++++++++++++++++++++
>>   src/util/ebtables.c         | 52
>> ++++++++++++++++++++++++++++++++++++++++++++-
>>   src/util/iptables.c         | 49
>> +++++++++++++++++++++++++++++++++++++++---
>>   7 files changed, 177 insertions(+), 7 deletions(-)
>>
>> diff --git a/AUTHORS b/AUTHORS
>> index 8581aea..5dec3a2 100644
>> --- a/AUTHORS
>> +++ b/AUTHORS
>> @@ -257,7 +257,7 @@ Patches have also been contributed by:
>>     Frido Roose          <frido roose gmail com>
>>     Asad Saeed           <asad saeed acidseed com>
>>     Sukadev Bhattiprolu  <sukadev linux vnet ibm com>
>> -
>> +  Thomas Woerner       <twoerner redhat com>
>>     [....send patches to get your name here....]
>>
>>   The libvirt Logo was designed by Diana Fong
>> diff --git a/configure.ac b/configure.ac
>> index ba5a3cd..0150f99 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1321,6 +1321,22 @@ AM_CONDITIONAL([HAVE_POLKIT1], [test
>> "x$with_polkit1" = "xyes"])
>>   AC_SUBST([POLKIT_CFLAGS])
>>   AC_SUBST([POLKIT_LIBS])
>>
>> +dnl firewalld
>> +AC_ARG_WITH([firewalld],
>> +  AC_HELP_STRING([--with-firewalld], [enable firewalld support
>> @<:@default=check@:>@]),
>> +  [],
>> +  [with_firewalld=check])
>> +if test "x$with_firewalld" = "xcheck" ; then
>> +   with_firewalld=$with_dbus
>> +fi
>> +if test "x$with_firewalld" == "xyes" ; then
>> +  if test "x$with_dbus" != "xyes" ; then
>> +     AC_MSG_ERROR([You must have dbus enabled for firewalld support])
>> +  fi
>> +  AC_DEFINE_UNQUOTED([HAVE_FIREWALLD], [1], [whether firewalld
>> support is enabled])
>> +fi
>> +AM_CONDITIONAL([HAVE_FIREWALLD], [test "x$with_firewalld" != "xno"])
>> +
>>   dnl Avahi library
>>   AC_ARG_WITH([avahi],
>>     AC_HELP_STRING([--with-avahi], [use avahi to advertise remote
>> daemon @<:@default=check@:>@]),
>> @@ -3028,6 +3044,7 @@ AC_MSG_NOTICE([ sanlock: $SANLOCK_CFLAGS
>> $SANLOCK_LIBS])
>>   else
>>   AC_MSG_NOTICE([ sanlock: no])
>>   fi
>> +AC_MSG_NOTICE([firewalld: $with_firewalld])
>>   if test "$with_avahi" = "yes" ; then
>>   AC_MSG_NOTICE([   avahi: $AVAHI_CFLAGS $AVAHI_LIBS])
>>   else
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index 67b955a..ea2fd88 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -106,6 +106,7 @@
>>   %define with_sanlock       0%{!?_without_sanlock:0}
>>   %define with_systemd       0%{!?_without_systemd:0}
>>   %define with_numad         0%{!?_without_numad:0}
>> +%define with_firewalld     0%{!?_without_firewalld:0}
>>
>>   # Non-server/HV driver defaults which are always enabled
>>   %define with_python        0%{!?_without_python:1}
>> @@ -146,6 +147,11 @@
>>   %define with_systemd 1
>>   %endif
>>
>> +# Fedora 18 / RHEL-7 are first where firewalld support is enabled
>> +%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7
>> +%define with_firewalld 1
>> +%endif
>> +
>>   # RHEL-5 has restricted QEMU to x86_64 only and is too old for LXC
>>   %if 0%{?rhel} == 5
>>   %define with_qemu_tcg 0
>> @@ -1182,6 +1188,10 @@ of recent versions of Linux (and other OSes).
>>   %define _without_driver_modules --without-driver-modules
>>   %endif
>>
>> +%if %{with_firewalld}
>> +%define _with_firewalld --with-firewalld
>> +%endif
>> +
>>   %define when  %(date +"%%F-%%T")
>>   %define where %(hostname)
>>   %define who   %{?packager}%{!?packager:Unknown}
>> @@ -1240,6 +1250,7 @@ autoreconf -if
>>              %{?_without_audit} \
>>              %{?_without_dtrace} \
>>              %{?_without_driver_modules} \
>> +           %{?_with_firewalld} \
>>              %{with_packager} \
>>              %{with_packager_version} \
>>              --with-qemu-user=%{qemu_user} \
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index b5f8056..6a94ecc 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -988,7 +988,7 @@ libvirt_driver_network_la_SOURCES =
>>   libvirt_driver_network_la_LIBADD = libvirt_driver_network_impl.la
>>   if WITH_DRIVER_MODULES
>>   mod_LTLIBRARIES += libvirt_driver_network.la
>> -libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la
>> $(LIBNL_LIBS)
>> +libvirt_driver_network_la_LIBADD += ../gnulib/lib/libgnu.la
>> $(LIBNL_LIBS) $(DBUS_LIBS)
>>   libvirt_driver_network_la_LDFLAGS = -module -avoid-version
>> $(AM_LDFLAGS)
>>   else
>>   noinst_LTLIBRARIES += libvirt_driver_network.la
>> @@ -998,7 +998,7 @@ endif
>>
>>   libvirt_driver_network_impl_la_CFLAGS = \
>>           $(LIBNL_CFLAGS) \
>> -        -I$(top_srcdir)/src/conf $(AM_CFLAGS)
>> +        -I$(top_srcdir)/src/conf $(AM_CFLAGS) $(DBUS_CFLAGS)
>>   libvirt_driver_network_impl_la_SOURCES = $(NETWORK_DRIVER_SOURCES)
>>   endif
>>   EXTRA_DIST += network/default.xml
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 474bbfa..e3f0c1c 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -62,6 +62,7 @@
>>   #include "virnetdevbridge.h"
>>   #include "virnetdevtap.h"
>>   #include "virnetdevvportprofile.h"
>> +#include "virdbus.h"
>>
>>   #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
>>   #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
>> @@ -249,6 +250,25 @@ networkAutostartConfigs(struct network_driver
>> *driver) {
>>       }
>>   }
>>
>> +#if HAVE_FIREWALLD
>> +static DBusHandlerResult
>> +firewalld_dbus_filter_bridge(DBusConnection *connection
>> ATTRIBUTE_UNUSED,
>> +                             DBusMessage *message, void *user_data) {
>> +    struct network_driver *_driverState = user_data;
>> +
>> +    if (dbus_message_is_signal(message, DBUS_INTERFACE_DBUS,
>> +                               "NameOwnerChanged") ||
>> +        dbus_message_is_signal(message, "org.fedoraproject.FirewallD1",
>> +                               "Reloaded"))
>> +    {
>> +        VIR_DEBUG("Reload in bridge_driver because of firewalld.");
>> +        networkReloadIptablesRules(_driverState);
>> +    }
>> +
>> +    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
>> +}
>> +#endif
>> +
>>   /**
>>    * networkStartup:
>>    *
>> @@ -257,6 +277,9 @@ networkAutostartConfigs(struct network_driver
>> *driver) {
>>   static int
>>   networkStartup(int privileged) {
>>       char *base = NULL;
>> +#ifdef HAVE_FIREWALLD
>> +    DBusConnection *sysbus = NULL;
>> +#endif
>>
>>       if (VIR_ALLOC(driverState) < 0)
>>           goto error;
>> @@ -323,6 +346,32 @@ networkStartup(int privileged) {
>>
>>       networkDriverUnlock(driverState);
>>
>> +#ifdef HAVE_FIREWALLD
>> +    if (!(sysbus = virDBusGetSystemBus())) {
>> +        virErrorPtr err = virGetLastError();
>> +        VIR_WARN("DBus not available, disabling firewalld support "
>> +                 "in bridge_driver: %s", err->message);
>> +    } else {
>> +        /* add matches for
>> +         * NameOwnerChanged on org.freedesktop.DBus for firewalld
>> start/stop
>> +         * Reloaded on org.fedoraproject.FirewallD1 for firewalld
>> reload
>> +         */
>> +        dbus_bus_add_match(sysbus,
>> +                           "type='signal'"
>> +                           ",interface='"DBUS_INTERFACE_DBUS"'"
>> +                           ",member='NameOwnerChanged'"
>> +                           ",arg0='org.fedoraproject.FirewallD1'",
>> +                           NULL);
>> +        dbus_bus_add_match(sysbus,
>> +                           "type='signal'"
>> +                           ",interface='org.fedoraproject.FirewallD1'"
>> +                           ",member='Reloaded'",
>> +                           NULL);
>> +        dbus_connection_add_filter(sysbus,
>> firewalld_dbus_filter_bridge,
>> +                                   driverState, NULL);
>> +    }
>> +#endif
>> +
>>       return 0;
>>
>>   out_of_memory:
>> diff --git a/src/util/ebtables.c b/src/util/ebtables.c
>> index ca056b1..1a78f89 100644
>> --- a/src/util/ebtables.c
>> +++ b/src/util/ebtables.c
>> @@ -1,5 +1,5 @@
>>   /*
>> - * Copyright (C) 2007-2010 Red Hat, Inc.
>> + * Copyright (C) 2007-2012 Red Hat, Inc.
>>    * Copyright (C) 2009 IBM Corp.
>>    *
>>    * This library is free software; you can redistribute it and/or
>> @@ -45,6 +45,38 @@
>>   #include "memory.h"
>>   #include "virterror_internal.h"
>>   #include "logging.h"
>> +#include "threads.h"
>> +
>> +#if HAVE_FIREWALLD
>> +static char *firewall_cmd_path = NULL;
>> +
>> +static int
>> +virEbTablesOnceInit(void)
>> +{
>> +    firewall_cmd_path = virFindFileInPath("firewall-cmd");
>> +    if (!firewall_cmd_path) {
>> +        VIR_WARN("firewall-cmd not found on system. "
>> +                 "firewalld support disabled for ebtables.");
>> +    } else {
>> +        virCommandPtr cmd = virCommandNew(firewall_cmd_path);
>> +        int status;
>> +
>> +        virCommandAddArgList(cmd, "--state", NULL);
>> +        if (virCommandRun(cmd, &status) < 0 || status != 0) {
>> +            VIR_WARN("firewall-cmd found but disabled for ebtables");
>> +            VIR_FREE(firewall_cmd_path);
>> +            firewall_cmd_path = NULL;
>> +        } else {
>> +            VIR_WARN("using firewalld for ebtables commands");
>> +        }
>> +        virCommandFree(cmd);
>> +    }
>> +    return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virEbTables)
>> +
>> +#endif
>>
>>   struct _ebtablesContext
>>   {
>> @@ -181,6 +213,12 @@ ebtablesAddRemoveRule(ebtRules *rules, int
>> action, const char *arg, ...)
>>           2 + /*   --insert bar  */
>>           1;  /*   arg           */
>>
>> +#if HAVE_FIREWALLD
>> +    virEbTablesInitialize();
>> +    if (firewall_cmd_path)
>> +        n += 3; /* --direct --passthrough eb */
>> +#endif
>> +
>>       va_start(args, arg);
>>       while (va_arg(args, const char *))
>>           n++;
>> @@ -192,6 +230,18 @@ ebtablesAddRemoveRule(ebtRules *rules, int
>> action, const char *arg, ...)
>>
>>       n = 0;
>>
>> +#if HAVE_FIREWALLD
>> +    if (firewall_cmd_path) {
>> +        if (!(argv[n++] = strdup(firewall_cmd_path)))
>> +            goto error;
>> +        if (!(argv[n++] = strdup("--direct")))
>> +            goto error;
>> +        if (!(argv[n++] = strdup("--passthrough")))
>> +            goto error;
>> +        if (!(argv[n++] = strdup("eb")))
>> +            goto error;
>> +    } else
>> +#endif
>>       if (!(argv[n++] = strdup(EBTABLES_PATH)))
>>           goto error;
>>
>> diff --git a/src/util/iptables.c b/src/util/iptables.c
>> index b23aca9..d8fdd3b 100644
>> --- a/src/util/iptables.c
>> +++ b/src/util/iptables.c
>> @@ -1,5 +1,5 @@
>>   /*
>> - * Copyright (C) 2007-2011 Red Hat, Inc.
>> + * Copyright (C) 2007-2012 Red Hat, Inc.
>>    *
>>    * This library is free software; you can redistribute it and/or
>>    * modify it under the terms of the GNU Lesser General Public
>> @@ -43,6 +43,38 @@
>>   #include "memory.h"
>>   #include "virterror_internal.h"
>>   #include "logging.h"
>> +#include "threads.h"
>> +
>> +#if HAVE_FIREWALLD
>> +static char *firewall_cmd_path = NULL;
>> +
>> +static int
>> +virIpTablesOnceInit(void)
>> +{
>> +    firewall_cmd_path = virFindFileInPath("firewall-cmd");
>> +    if (!firewall_cmd_path) {
>> +        VIR_WARN("firewall-cmd not found on system. "
>> +                 "firewalld support disabled for iptables.");
>> +    } else {
>> +        virCommandPtr cmd = virCommandNew(firewall_cmd_path);
>> +        int status;
>> +
>> +        virCommandAddArgList(cmd, "--state", NULL);
>> +        if (virCommandRun(cmd, &status) < 0 || status != 0) {
>> +            VIR_WARN("firewall-cmd found but disabled for iptables");
>> +            VIR_FREE(firewall_cmd_path);
>> +            firewall_cmd_path = NULL;
>> +        } else {
>> +            VIR_WARN("using firewalld for iptables commands");
>> +        }
>> +        virCommandFree(cmd);
>> +    }
>> +    return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virIpTables)
>> +
>> +#endif
>>
>>   #define VIR_FROM_THIS VIR_FROM_NONE
>>
>> @@ -101,11 +133,22 @@ iptablesAddRemoveRule(iptRules *rules, int
>> family, int action,
>>   {
>>       va_list args;
>>       int ret;
>> -    virCommandPtr cmd;
>> +    virCommandPtr cmd = NULL;
>>       const char *s;
>>
>> -    cmd = virCommandNew((family == AF_INET6)
>> +#if HAVE_FIREWALLD
>> +    virIpTablesInitialize();
>> +    if (firewall_cmd_path) {
>> +        cmd = virCommandNew(firewall_cmd_path);
>> +        virCommandAddArgList(cmd, "--direct", "--passthrough",
>> +                             (family == AF_INET6) ? "ipv6" : "ipv4",
>> NULL);
>> +    }
>> +#endif
>> +
>> +    if (cmd == NULL) {
>> +        cmd = virCommandNew((family == AF_INET6)
>>                           ? IP6TABLES_PATH : IPTABLES_PATH);
>> +    }
>>
>>       virCommandAddArgList(cmd, "--table", rules->table,
>>                            action == ADD ? "--insert" : "--delete",
>>
>
> here is my ACK for the changes.

Thanks. I just pushed it (along with a slightly tweaked version of
Stefan's nwfilter+firewalld patch).



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]