network: add private chains only if there are networks adding iptables rules

Juan Quintela noticed that when he restarted libvirt he was getting
extra iptables rules added by libvirt even though he didn't have any
libvirt networks that used iptables rules. It turns out this also
happens if the firewalld service is restarted. The extra rules are
just the private chains, and they're sometimes being added
unnecessarily because they are added separately in a global
networkPreReloadFirewallRules() that does the init if there are any
active networks, regardless of whether or not any of those networks
will actually add rules to the host firewall.

The fix is to change the check for "any active networks" to instead
check for "any active networks that add firewall rules".

(NB: although the timing seems suspicious, this isn't a new regression
caused by the recently pushed f5418b427 (which forces recreation of
private chains when firewalld is restarted); it was an existing bug
since iptables rules were first put into private chains, even after
commit c6cbe18771 delayed creation of the private chains. The
implication is that any downstream based on v5.1.0 or later that cares
about these extraneous (but harmless) private chains would want to
backport this patch (along with the other two if they aren't already
there))

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index b0bd2072508eac52d523a497f77c284bc60fad11..4145411b4b3654ec3623193ffcb2a22f2ac5892c 100644 (file)
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -91,28 +91,55 @@ static void networkSetupPrivateChains(void)
 
 
 static int
-networkHasRunningNetworksHelper(virNetworkObjPtr obj,
+networkHasRunningNetworksWithFWHelper(virNetworkObjPtr obj,
                                 void *opaque)
 {
-    bool *running = opaque;
+    bool *activeWithFW = opaque;
 
     virObjectLock(obj);
-    if (virNetworkObjIsActive(obj))
-        *running = true;
+    if (virNetworkObjIsActive(obj)) {
+        virNetworkDefPtr def = virNetworkObjGetDef(obj);
+
+        switch ((virNetworkForwardType) def->forward.type) {
+        case VIR_NETWORK_FORWARD_NONE:
+        case VIR_NETWORK_FORWARD_NAT:
+        case VIR_NETWORK_FORWARD_ROUTE:
+            *activeWithFW = true;
+            break;
+
+        case VIR_NETWORK_FORWARD_OPEN:
+        case VIR_NETWORK_FORWARD_BRIDGE:
+        case VIR_NETWORK_FORWARD_PRIVATE:
+        case VIR_NETWORK_FORWARD_VEPA:
+        case VIR_NETWORK_FORWARD_PASSTHROUGH:
+        case VIR_NETWORK_FORWARD_HOSTDEV:
+        case VIR_NETWORK_FORWARD_LAST:
+            break;
+        }
+    }
+
     virObjectUnlock(obj);
 
+    /*
+     * terminate ForEach early once we find an active network that
+     * adds Firewall rules (return status is ignored)
+     */
+    if (*activeWithFW)
+        return -1;
+
     return 0;
 }
 
 
 static bool
-networkHasRunningNetworks(virNetworkDriverStatePtr driver)
+networkHasRunningNetworksWithFW(virNetworkDriverStatePtr driver)
 {
-    bool running = false;
+    bool activeWithFW = false;
+
     virNetworkObjListForEach(driver->networks,
-                             networkHasRunningNetworksHelper,
-                             &running);
-    return running;
+                             networkHasRunningNetworksWithFWHelper,
+                             &activeWithFW);
+    return activeWithFW;
 }
 
 
@@ -150,8 +177,8 @@ networkPreReloadFirewallRules(virNetworkDriverStatePtr driver,
         networkSetupPrivateChains();
 
     } else {
-        if (!networkHasRunningNetworks(driver)) {
-            VIR_DEBUG("Delayed global rule setup as no networks are running");
+        if (!networkHasRunningNetworksWithFW(driver)) {
+            VIR_DEBUG("Delayed global rule setup as no networks with firewall rules are running");
             return;
         }