[libvirt] [PATCH v4] network: use firewalld instead of iptables, when available
Thomas Woerner
twoerner at redhat.com
Tue Aug 21 15:24:42 UTC 2012
Hello Laine,
On 08/16/2012 08:18 AM, Laine Stump wrote:
> From: Thomas Woerner <twoerner at 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 at gmail.com>
> Asad Saeed <asad.saeed at acidseed.com>
> Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>
> -
> + Thomas Woerner <twoerner at 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,
Thomas
More information about the libvir-list
mailing list