network: call network(Add|Remove)FirewallRules() for forward mode='open'

Previously networkAddFirewallRules() and networkRemoveFirewallRules()
were only called if the forward mode was none, 'route', or 'nat', so
those functions didn't check the forward mode. Although their current
contents shouldn't be executed for forward mode='open', soon they will
have extra functionality that should be executed for all the current
forward modes and also mode='open'.

This patch modifies all places either of the functions are called to
make sure they are called for mode='open' in addition to current modes
(by either adding 'case ..._OPEN:' to the case of a switch statement,
or just removing an 'if (mode != ...OPEN)' around the calls; to
balance out for that, it puts the entirety of the contents of both
functions inside if (mode != ...OPEN) to retain current behavior. (an
upcoming patch will add code outside that if clause).

debug log messages were also added to make it easier to test that the
right thing is being done in all cases.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

src/network/bridge_driver.c

@@ -1735,10 +1735,15 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj,
         case VIR_NETWORK_FORWARD_NONE:
         case VIR_NETWORK_FORWARD_NAT:
         case VIR_NETWORK_FORWARD_ROUTE:
-            /* Only three of the L3 network types that are configured by
+        case VIR_NETWORK_FORWARD_OPEN:
-             * libvirt need to have iptables rules reloaded. The 4th L3
+            /* even 'open' forward type networks need to call
-             * network type, forward='open', doesn't need this because it
+             * networkAdd/RemoveFirewallRules() in spite of the fact
-             * has no iptables rules.
+             * that, by definition, libvirt doesn't add any firewall
+             * rules for those networks.. This is because libvirt
+             * *does* support explicitly naming (in the config) a
+             * firewalld zone the network's bridge should be added to,
+             * and this functionality is also handled by
+             * networkAdd/RemoveFirewallRules()
              */
             networkRemoveFirewallRules(obj);
             ignore_value(networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval));
@@ -1746,7 +1751,6 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj,
             saveStatus = true;
             break;
 
-        case VIR_NETWORK_FORWARD_OPEN:
         case VIR_NETWORK_FORWARD_BRIDGE:
         case VIR_NETWORK_FORWARD_PRIVATE:
         case VIR_NETWORK_FORWARD_VEPA:
@@ -2000,10 +2004,8 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
         goto error;
 
     /* Add "once per network" rules */
-    if (def->forward.type != VIR_NETWORK_FORWARD_OPEN &&
+    if (networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0)
-        networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) {
         goto error;
-    }
 
     virNetworkObjSetFwRemoval(obj, fwRemoval);
     firewalRulesAdded = true;
@@ -2119,8 +2121,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
     if (devOnline)
         ignore_value(virNetDevSetOnline(def->bridge, false));
 
-    if (firewalRulesAdded &&
+    if (firewalRulesAdded)
-        def->forward.type != VIR_NETWORK_FORWARD_OPEN)
         networkRemoveFirewallRules(obj);
 
     virNetworkObjUnrefMacMap(obj);
@@ -2158,7 +2159,6 @@ networkShutdownNetworkVirtual(virNetworkObj *obj)
 
     ignore_value(virNetDevSetOnline(def->bridge, false));
 
-    if (def->forward.type != VIR_NETWORK_FORWARD_OPEN)
     networkRemoveFirewallRules(obj);
 
     ignore_value(virNetDevBridgeDelete(def->bridge));
@@ -3307,6 +3307,7 @@ networkUpdate(virNetworkPtr net,
         case VIR_NETWORK_FORWARD_NONE:
         case VIR_NETWORK_FORWARD_NAT:
         case VIR_NETWORK_FORWARD_ROUTE:
+        case VIR_NETWORK_FORWARD_OPEN:
             switch (section) {
             case VIR_NETWORK_SECTION_FORWARD:
             case VIR_NETWORK_SECTION_FORWARD_INTERFACE:
@@ -3325,7 +3326,6 @@ networkUpdate(virNetworkPtr net,
             }
             break;
 
-        case VIR_NETWORK_FORWARD_OPEN:
         case VIR_NETWORK_FORWARD_BRIDGE:
         case VIR_NETWORK_FORWARD_PRIVATE:
         case VIR_NETWORK_FORWARD_VEPA:

src/network/bridge_driver_linux.c

@@ -337,6 +337,16 @@ networkAddFirewallRules(virNetworkDef *def,
                         virFirewallBackend firewallBackend,
                         virFirewall **fwRemoval)
 {
+    if (def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
+
+        VIR_DEBUG("No firewall rules to add for mode='open' network '%s'", def->name);
+
+    } else {
+
+        VIR_DEBUG("Adding firewall rules for mode='%s' network '%s' using %s",
+                  virNetworkForwardTypeToString(def->forward.type),
+                  def->name,
+                  virFirewallBackendTypeToString(firewallBackend));
 
         networkSetupPrivateChains(firewallBackend, false);
 
@@ -422,6 +432,7 @@ networkAddFirewallRules(virNetworkDef *def,
             virReportEnumRangeError(virFirewallBackend, firewallBackend);
             return -1;
         }
+    }
     return 0;
 }
 
@@ -429,21 +440,29 @@ networkAddFirewallRules(virNetworkDef *def,
 void
 networkRemoveFirewallRules(virNetworkObj *obj)
 {
+    virNetworkDef *def = virNetworkObjGetDef(obj);
     virFirewall *fw;
 
+    if (def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
+
+        VIR_DEBUG("No firewall rules to remove for mode='open' network '%s'", def->name);
+
+    } else {
+
         if ((fw = virNetworkObjGetFwRemoval(obj)) == NULL) {
             /* No information about firewall rules in the network status,
              * so we assume the old iptables-based rules from 10.2.0 and
              * earlier.
              */
-        VIR_DEBUG("No firewall info in network status, assuming old-style iptables");
+            VIR_DEBUG("No firewall info in status of network '%s', assuming old-style iptables", def->name);
-        iptablesRemoveFirewallRules(virNetworkObjGetDef(obj));
+            iptablesRemoveFirewallRules(def);
             return;
         }
 
         /* fwRemoval info was stored in the network status, so use that to
          * remove the firewall
          */
-    VIR_DEBUG("Removing firewall rules with commands saved in network status");
+        VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name);
         virFirewallApply(fw);
     }
+}