network: delay global firewall setup if no networks are running

Creating firewall rules for the virtual networks causes the kernel to
load the conntrack module. This imposes a significant performance
penalty on Linux network traffic. Thus we want to only take that hit if
we actually have virtual networks running.

We need to create global firewall rules during startup in order to
"upgrade" rules for any running networks created by older libvirt.
If no running networks are present though, we can safely delay setup
until the time we actually start a network.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0e9bb78c32a4a2ae1b7bee61a191eeecf32a7a5a..c0c026e24248a092e6a796876fe640f35c557990 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2122,7 +2122,7 @@ networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
      * but until we untangle the virt driver that's not viable */
     if (!driver->privileged)
         return;
-    networkPreReloadFirewallRules(startup);
+    networkPreReloadFirewallRules(driver, startup);
     virNetworkObjListForEach(driver->networks,
                              networkReloadFirewallRulesHelper,
                              NULL);

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 75b34fc3172990d8fe76be52b2f9451b80f94e42..35459c10d1a885a60176e71e25a88f7cfea162a5 100644 (file)
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -84,16 +84,57 @@ static void networkSetupPrivateChains(void)
     }
 }
 
-void networkPreReloadFirewallRules(bool startup)
+
+static int
+networkHasRunningNetworksHelper(virNetworkObjPtr obj,
+                                void *opaque)
+{
+    bool *running = opaque;
+
+    virObjectLock(obj);
+    if (virNetworkObjIsActive(obj))
+        *running = true;
+    virObjectUnlock(obj);
+
+    return 0;
+}
+
+
+static bool
+networkHasRunningNetworks(virNetworkDriverStatePtr driver)
+{
+    bool running = false;
+    virNetworkObjListForEach(driver->networks,
+                             networkHasRunningNetworksHelper,
+                             &running);
+    return running;
+}
+
+
+void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup)
 {
-    /* We create global rules upfront as we don't want
-     * the perf hit of conditionally figuring out whether
-     * to create them each time a network is started.
+    /*
+     * If there are any running networks, we need to
+     * create the global rules upfront. This allows us
+     * convert rules created by old libvirt into the new
+     * format.
+     *
+     * If there are not any running networks, then we
+     * must not create rules, because the rules will
+     * cause the conntrack kernel module to be loaded.
+     * This imposes a significant performance hit on
+     * the networking stack. Thus we will only create
+     * rules if a network is later startup.
      *
      * Any errors here are saved to be reported at time
      * of starting the network though as that makes them
      * more likely to be seen by a human
      */
+    if (!networkHasRunningNetworks(driver)) {
+        VIR_DEBUG("Delayed global rule setup as no networks are running");
+        return;
+    }
+
     ignore_value(virOnce(&createdOnce, networkSetupPrivateChains));
 
     /*
@@ -726,6 +767,9 @@ int networkAddFirewallRules(virNetworkDefPtr def)
     virFirewallPtr fw = NULL;
     int ret = -1;
 
+    if (virOnce(&createdOnce, networkSetupPrivateChains) < 0)
+        return -1;
+
     if (errInitV4 &&
         (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
          virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {

diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index ea9db338cb3c51fcdb7a17c19458967c66aa06ed..ec7b1ed8b70c86195f9dc3708688431ff28ebd98 100644 (file)
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -19,7 +19,8 @@
 
 #include <config.h>
 
-void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED)
+void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver ATTRIBUTE_UNUSED,
+                                   bool startup ATTRIBUTE_UNUSED)
 {
 }
 

diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
index 95fd64bdc7c50b97202a593b01f28d64195f40cb..5f534fc132b9a0165ef9f18428e0ecf91ca045df 100644 (file)
--- a/src/network/bridge_driver_platform.h
+++ b/src/network/bridge_driver_platform.h
@@ -58,7 +58,7 @@ struct _virNetworkDriverState {
 typedef struct _virNetworkDriverState virNetworkDriverState;
 typedef virNetworkDriverState *virNetworkDriverStatePtr;
 
-void networkPreReloadFirewallRules(bool startup);
+void networkPreReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup);
 void networkPostReloadFirewallRules(bool startup);
 
 int networkCheckRouteCollision(virNetworkDefPtr def);

diff --git a/tests/networkxml2firewalldata/base.args b/tests/networkxml2firewalldata/base.args
new file mode 100644 (file)
index 0000000..0e71bf3
--- /dev/null
+++ b/tests/networkxml2firewalldata/base.args
@@ -0,0 +1,116 @@
+iptables \
+--table filter \
+--list-rules
+iptables \
+--table nat \
+--list-rules
+iptables \
+--table mangle \
+--list-rules
+iptables \
+--table filter \
+--new-chain LIBVIRT_INP
+iptables \
+--table filter \
+--insert INPUT \
+--jump LIBVIRT_INP
+iptables \
+--table filter \
+--new-chain LIBVIRT_OUT
+iptables \
+--table filter \
+--insert OUTPUT \
+--jump LIBVIRT_OUT
+iptables \
+--table filter \
+--new-chain LIBVIRT_FWO
+iptables \
+--table filter \
+--insert FORWARD \
+--jump LIBVIRT_FWO
+iptables \
+--table filter \
+--new-chain LIBVIRT_FWI
+iptables \
+--table filter \
+--insert FORWARD \
+--jump LIBVIRT_FWI
+iptables \
+--table filter \
+--new-chain LIBVIRT_FWX
+iptables \
+--table filter \
+--insert FORWARD \
+--jump LIBVIRT_FWX
+iptables \
+--table nat \
+--new-chain LIBVIRT_PRT
+iptables \
+--table nat \
+--insert POSTROUTING \
+--jump LIBVIRT_PRT
+iptables \
+--table mangle \
+--new-chain LIBVIRT_PRT
+iptables \
+--table mangle \
+--insert POSTROUTING \
+--jump LIBVIRT_PRT
+ip6tables \
+--table filter \
+--list-rules
+ip6tables \
+--table nat \
+--list-rules
+ip6tables \
+--table mangle \
+--list-rules
+ip6tables \
+--table filter \
+--new-chain LIBVIRT_INP
+ip6tables \
+--table filter \
+--insert INPUT \
+--jump LIBVIRT_INP
+ip6tables \
+--table filter \
+--new-chain LIBVIRT_OUT
+ip6tables \
+--table filter \
+--insert OUTPUT \
+--jump LIBVIRT_OUT
+ip6tables \
+--table filter \
+--new-chain LIBVIRT_FWO
+ip6tables \
+--table filter \
+--insert FORWARD \
+--jump LIBVIRT_FWO
+ip6tables \
+--table filter \
+--new-chain LIBVIRT_FWI
+ip6tables \
+--table filter \
+--insert FORWARD \
+--jump LIBVIRT_FWI
+ip6tables \
+--table filter \
+--new-chain LIBVIRT_FWX
+ip6tables \
+--table filter \
+--insert FORWARD \
+--jump LIBVIRT_FWX
+ip6tables \
+--table nat \
+--new-chain LIBVIRT_PRT
+ip6tables \
+--table nat \
+--insert POSTROUTING \
+--jump LIBVIRT_PRT
+ip6tables \
+--table mangle \
+--new-chain LIBVIRT_PRT
+ip6tables \
+--table mangle \
+--insert POSTROUTING \
+--jump LIBVIRT_PRT

diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 575b68379af0a53f32d33b1dcb0773f7ca0bc916..c25282ebb18f75e9d7953330fb8a24b096cae02a 100644 (file)
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -22,6 +22,7 @@
 #include <config.h>
 
 #include "testutils.h"
+#include "viralloc.h"
 
 #if defined (__linux__)
 
@@ -57,13 +58,15 @@ testCommandDryRun(const char *const*args ATTRIBUTE_UNUSED,
 }
 
 static int testCompareXMLToArgvFiles(const char *xml,
-                                     const char *cmdline)
+                                     const char *cmdline,
+                                     const char *baseargs)
 {
     char *expectargv = NULL;
     char *actualargv = NULL;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     virNetworkDefPtr def = NULL;
     int ret = -1;
+    char *actual;
 
     virCommandSetDryRun(&buf, testCommandDryRun, NULL);
 
@@ -76,11 +79,18 @@ static int testCompareXMLToArgvFiles(const char *xml,
     if (virBufferError(&buf))
         goto cleanup;
 
-    actualargv = virBufferContentAndReset(&buf);
+    actual = actualargv = virBufferContentAndReset(&buf);
     virTestClearCommandPath(actualargv);
     virCommandSetDryRun(NULL, NULL, NULL);
 
-    if (virTestCompareToFile(actualargv, cmdline) < 0)
+    /* The first network to be created populates the
+     * libvirt global chains. We must skip args for
+     * that if present
+     */
+    if (STRPREFIX(actual, baseargs))
+        actual += strlen(baseargs);
+
+    if (virTestCompareToFile(actual, cmdline) < 0)
         goto cleanup;
 
     ret = 0;
@@ -95,6 +105,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
 
 struct testInfo {
     const char *name;
+    const char *baseargs;
 };
 
 
@@ -112,7 +123,7 @@ testCompareXMLToIPTablesHelper(const void *data)
                     abs_srcdir, info->name, RULESTYPE) < 0)
         goto cleanup;
 
-    result = testCompareXMLToArgvFiles(xml, args);
+    result = testCompareXMLToArgvFiles(xml, args, info->baseargs);
 
  cleanup:
     VIR_FREE(xml);
@@ -133,11 +144,13 @@ static int
 mymain(void)
 {
     int ret = 0;
+    VIR_AUTOFREE(char *)basefile = NULL;
+    VIR_AUTOFREE(char *)baseargs = NULL;
 
 # define DO_TEST(name) \
     do { \
-        static struct testInfo info = { \
-            name, \
+        struct testInfo info = { \
+            name, baseargs, \
         }; \
         if (virTestRun("Network XML-2-iptables " name, \
                        testCompareXMLToIPTablesHelper, &info) < 0) \
@@ -156,6 +169,17 @@ mymain(void)
         goto cleanup;
     }
 
+    if (virAsprintf(&basefile, "%s/networkxml2firewalldata/base.args",
+                    abs_srcdir) < 0) {
+        ret = -1;
+        goto cleanup;
+    }
+
+    if (virTestLoadFile(basefile, &baseargs) < 0) {
+        ret = -1;
+        goto cleanup;
+    }
+
     DO_TEST("nat-default");
     DO_TEST("nat-tftp");
     DO_TEST("nat-many-ips");