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>

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index fe053f423ab5dc49ce11cbf04a0bbf597889ad72..f604b2695c5ae9f95aa55a8b0c501f49fd5ffec7 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/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
-             * libvirt need to have iptables rules reloaded. The 4th L3
-             * network type, forward='open', doesn't need this because it
-             * has no iptables rules.
+        case VIR_NETWORK_FORWARD_OPEN:
+            /* even 'open' forward type networks need to call
+             * networkAdd/RemoveFirewallRules() in spite of the fact
+             * 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 &&
-        networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) {
+    if (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 &&
-        def->forward.type != VIR_NETWORK_FORWARD_OPEN)
+    if (firewalRulesAdded)
         networkRemoveFirewallRules(obj);
 
     virNetworkObjUnrefMacMap(obj);
@@ -2158,8 +2159,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj)
 
     ignore_value(virNetDevSetOnline(def->bridge, false));
 
-    if (def->forward.type != VIR_NETWORK_FORWARD_OPEN)
-        networkRemoveFirewallRules(obj);
+    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:

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 5981e3bd193c3cdeecccc46dd25a5d10946be5f5..31feec9c9fc7576c62c64d879b8536e17a5873a9 100644 (file)
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -337,90 +337,101 @@ networkAddFirewallRules(virNetworkDef *def,
                         virFirewallBackend firewallBackend,
                         virFirewall **fwRemoval)
 {
+    if (def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
 
-    networkSetupPrivateChains(firewallBackend, false);
+        VIR_DEBUG("No firewall rules to add for mode='open' network '%s'", def->name);
 
-    if (errInitV4 &&
-        (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
-         virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
-        virSetError(errInitV4);
-        return -1;
-    }
+    } else {
 
-    if (errInitV6 &&
-        (virNetworkDefGetIPByIndex(def, AF_INET6, 0) ||
-         virNetworkDefGetRouteByIndex(def, AF_INET6, 0) ||
-         def->ipv6nogw)) {
-        virSetError(errInitV6);
-        return -1;
-    }
+        VIR_DEBUG("Adding firewall rules for mode='%s' network '%s' using %s",
+                  virNetworkForwardTypeToString(def->forward.type),
+                  def->name,
+                  virFirewallBackendTypeToString(firewallBackend));
 
-    if (def->bridgeZone) {
+        networkSetupPrivateChains(firewallBackend, false);
 
-        /* if a firewalld zone has been specified, fail/log an error
-         * if we can't honor it
-         */
-        if (virFirewallDIsRegistered() < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("zone %1$s requested for network %2$s but firewalld is not active"),
-                           def->bridgeZone, def->name);
+        if (errInitV4 &&
+            (virNetworkDefGetIPByIndex(def, AF_INET, 0) ||
+             virNetworkDefGetRouteByIndex(def, AF_INET, 0))) {
+            virSetError(errInitV4);
             return -1;
         }
 
-        if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0)
+        if (errInitV6 &&
+            (virNetworkDefGetIPByIndex(def, AF_INET6, 0) ||
+             virNetworkDefGetRouteByIndex(def, AF_INET6, 0) ||
+             def->ipv6nogw)) {
+            virSetError(errInitV6);
             return -1;
+        }
 
-    } else {
+        if (def->bridgeZone) {
 
-        /* if firewalld is active, try to set the "libvirt" zone. This is
-         * desirable (for consistency) if firewalld is using the iptables
-         * backend, but is necessary (for basic network connectivity) if
-         * firewalld is using the nftables backend
-         */
-        if (virFirewallDIsRegistered() == 0) {
-
-            /* if the "libvirt" zone exists, then set it. If not, and
-             * if firewalld is using the nftables backend, then we
-             * need to log an error because the combination of
-             * nftables + default zone means that traffic cannot be
-             * forwarded (and even DHCP and DNS from guest to host
-             * will probably no be permitted by the default zone
-             *
-             * Routed networks use a different zone and policy which we also
-             * need to verify exist. Probing for the policy guarantees the
-             * running firewalld has support for policies (firewalld >= 0.9.0).
+            /* if a firewalld zone has been specified, fail/log an error
+             * if we can't honor it
              */
-            if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE &&
-                virFirewallDPolicyExists("libvirt-routed-out") &&
-                virFirewallDZoneExists("libvirt-routed")) {
-                if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0)
-                    return -1;
-            } else if (virFirewallDZoneExists("libvirt")) {
-                if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0)
-                    return -1;
-            } else {
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt"));
+            if (virFirewallDIsRegistered() < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("zone %1$s requested for network %2$s but firewalld is not active"),
+                               def->bridgeZone, def->name);
                 return -1;
             }
+
+            if (virFirewallDInterfaceSetZone(def->bridge, def->bridgeZone) < 0)
+                return -1;
+
+        } else {
+
+            /* if firewalld is active, try to set the "libvirt" zone. This is
+             * desirable (for consistency) if firewalld is using the iptables
+             * backend, but is necessary (for basic network connectivity) if
+             * firewalld is using the nftables backend
+             */
+            if (virFirewallDIsRegistered() == 0) {
+
+                /* if the "libvirt" zone exists, then set it. If not, and
+                 * if firewalld is using the nftables backend, then we
+                 * need to log an error because the combination of
+                 * nftables + default zone means that traffic cannot be
+                 * forwarded (and even DHCP and DNS from guest to host
+                 * will probably no be permitted by the default zone
+                 *
+                 * Routed networks use a different zone and policy which we also
+                 * need to verify exist. Probing for the policy guarantees the
+                 * running firewalld has support for policies (firewalld >= 0.9.0).
+                 */
+                if (def->forward.type == VIR_NETWORK_FORWARD_ROUTE &&
+                    virFirewallDPolicyExists("libvirt-routed-out") &&
+                    virFirewallDZoneExists("libvirt-routed")) {
+                    if (virFirewallDInterfaceSetZone(def->bridge, "libvirt-routed") < 0)
+                        return -1;
+                } else if (virFirewallDZoneExists("libvirt")) {
+                    if (virFirewallDInterfaceSetZone(def->bridge, "libvirt") < 0)
+                        return -1;
+                } else {
+                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                   _("firewalld can't find the 'libvirt' zone that should have been installed with libvirt"));
+                    return -1;
+                }
+            }
         }
-    }
 
-    switch (firewallBackend) {
-    case VIR_FIREWALL_BACKEND_NONE:
-        virReportError(VIR_ERR_NO_SUPPORT, "%s",
-                       _("No firewall backend is available"));
-        return -1;
+        switch (firewallBackend) {
+        case VIR_FIREWALL_BACKEND_NONE:
+            virReportError(VIR_ERR_NO_SUPPORT, "%s",
+                           _("No firewall backend is available"));
+            return -1;
 
-    case VIR_FIREWALL_BACKEND_IPTABLES:
-        return iptablesAddFirewallRules(def, fwRemoval);
+        case VIR_FIREWALL_BACKEND_IPTABLES:
+            return iptablesAddFirewallRules(def, fwRemoval);
 
-    case VIR_FIREWALL_BACKEND_NFTABLES:
-        return nftablesAddFirewallRules(def, fwRemoval);
+        case VIR_FIREWALL_BACKEND_NFTABLES:
+            return nftablesAddFirewallRules(def, fwRemoval);
 
-    case VIR_FIREWALL_BACKEND_LAST:
-        virReportEnumRangeError(virFirewallBackend, firewallBackend);
-        return -1;
+        case VIR_FIREWALL_BACKEND_LAST:
+            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 ((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.
+    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 status of network '%s', assuming old-style iptables", def->name);
+            iptablesRemoveFirewallRules(def);
+            return;
+        }
+
+        /* fwRemoval info was stored in the network status, so use that to
+         * remove the firewall
          */
-        VIR_DEBUG("No firewall info in network status, assuming old-style iptables");
-        iptablesRemoveFirewallRules(virNetworkObjGetDef(obj));
-        return;
+        VIR_DEBUG("Removing firewall rules of network '%s' using commands saved in status", def->name);
+        virFirewallApply(fw);
     }
-
-    /* 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");
-    virFirewallApply(fw);
 }