network: use previously saved list of firewall removal commands

When destroying a network, the network driver has always assumed that
it knew what firewall rules had been added as the network was
started. This was usually correct - I only recall one time in the past
that the firewall rules added by libvirt were changed. But if the
exact rules used for a network *were* ever changed from one
build/version of libvirt to another, then we would end up attempting
to remove rules that hadn't been added, and could possibly *not*
remove rules that had been added.

The solution to this to not make such brash assumptions about the
past, but instead to save (in the network status object at network
start time) a list of all the rules needed to remove the rules that
were added for the network, and then use that saved list during
network destroy to remove exactly what was previous added.

Beyond making net-destroy more precise, there are other benefits:

1) We can change the details of the rules we add for networks from one
build/release of libvirt to another and painlessly upgrade.

2) The user can switch from one firewall backend to another by simply
changing the setting in network.conf and restarting
libvirtd/virtnetworkd.

In both cases, the restarted libvirtd/virtnetworkd will remove all the
rules that had been previously added (based on the network status),
and then add new rules (saving the new removal commands back into the
network status)

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d14853cfd00483b285ce67ce3a0387f39837d234..3c175786fe72da57707d033bd4a26807f58e3f17 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1685,6 +1685,7 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj,
     g_autoptr(virNetworkDriverConfig) cfg = virNetworkDriverGetConfig(networkGetDriver());
     VIR_LOCK_GUARD lock = virObjectLockGuard(obj);
     virNetworkDef *def = virNetworkObjGetDef(obj);
+    virFirewall *fwRemoval = NULL;
 
     if (virNetworkObjIsActive(obj)) {
         switch ((virNetworkForwardType) def->forward.type) {
@@ -1696,8 +1697,9 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj,
              * network type, forward='open', doesn't need this because it
              * has no iptables rules.
              */
-            networkRemoveFirewallRules(def, cfg->firewallBackend);
-            ignore_value(networkAddFirewallRules(def, cfg->firewallBackend));
+            networkRemoveFirewallRules(obj);
+            ignore_value(networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval));
+            virNetworkObjSetFwRemoval(obj, fwRemoval);
             break;
 
         case VIR_NETWORK_FORWARD_OPEN:
@@ -1904,6 +1906,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
     bool devOnline = false;
     bool firewalRulesAdded = false;
     virSocketAddr *dnsServer = NULL;
+    virFirewall *fwRemoval = NULL;
 
     /* Check to see if any network IP collides with an existing route */
     if (networkCheckRouteCollision(def) < 0)
@@ -1949,9 +1952,11 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
 
     /* Add "once per network" rules */
     if (def->forward.type != VIR_NETWORK_FORWARD_OPEN &&
-        networkAddFirewallRules(def, cfg->firewallBackend) < 0)
+        networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) {
         goto error;
+    }
 
+    virNetworkObjSetFwRemoval(obj, fwRemoval);
     firewalRulesAdded = true;
 
     for (i = 0; (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i)); i++) {
@@ -2067,7 +2072,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
 
     if (firewalRulesAdded &&
         def->forward.type != VIR_NETWORK_FORWARD_OPEN)
-        networkRemoveFirewallRules(def, cfg->firewallBackend);
+        networkRemoveFirewallRules(obj);
 
     virNetworkObjUnrefMacMap(obj);
 
@@ -2079,8 +2084,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
 
 
 static int
-networkShutdownNetworkVirtual(virNetworkObj *obj,
-                              virNetworkDriverConfig *cfg)
+networkShutdownNetworkVirtual(virNetworkObj *obj)
 {
     virNetworkDef *def = virNetworkObjGetDef(obj);
     pid_t dnsmasqPid;
@@ -2106,7 +2110,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj,
     ignore_value(virNetDevSetOnline(def->bridge, false));
 
     if (def->forward.type != VIR_NETWORK_FORWARD_OPEN)
-        networkRemoveFirewallRules(def, cfg->firewallBackend);
+        networkRemoveFirewallRules(obj);
 
     ignore_value(virNetDevBridgeDelete(def->bridge));
 
@@ -2410,7 +2414,7 @@ networkShutdownNetwork(virNetworkDriverState *driver,
     case VIR_NETWORK_FORWARD_NAT:
     case VIR_NETWORK_FORWARD_ROUTE:
     case VIR_NETWORK_FORWARD_OPEN:
-        ret = networkShutdownNetworkVirtual(obj, cfg);
+        ret = networkShutdownNetworkVirtual(obj);
         break;
 
     case VIR_NETWORK_FORWARD_BRIDGE:
@@ -3215,6 +3219,8 @@ networkUpdate(virNetworkPtr net,
     virNetworkIPDef *ipdef;
     bool oldDhcpActive = false;
     bool needFirewallRefresh = false;
+    virFirewall *fwRemoval = NULL;
+
 
 
     virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE |
@@ -3261,7 +3267,7 @@ networkUpdate(virNetworkPtr net,
                  * old rules (and remember to load new ones after the
                  * update).
                  */
-                networkRemoveFirewallRules(def, cfg->firewallBackend);
+                networkRemoveFirewallRules(obj);
                 needFirewallRefresh = true;
                 break;
             default:
@@ -3288,16 +3294,21 @@ networkUpdate(virNetworkPtr net,
     if (virNetworkObjUpdate(obj, command, section,
                             parentIndex, xml,
                             network_driver->xmlopt, flags) < 0) {
-        if (needFirewallRefresh)
-            ignore_value(networkAddFirewallRules(def, cfg->firewallBackend));
+        if (needFirewallRefresh) {
+            ignore_value(networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval));
+            virNetworkObjSetFwRemoval(obj, fwRemoval);
+        }
         goto cleanup;
     }
 
     /* @def is replaced */
     def = virNetworkObjGetDef(obj);
 
-    if (needFirewallRefresh && networkAddFirewallRules(def, cfg->firewallBackend) < 0)
+    if (needFirewallRefresh &&
+        networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0) {
         goto cleanup;
+    }
+    virNetworkObjSetFwRemoval(obj, fwRemoval);
 
     if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) {
         /* save updated persistent config to disk */

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 988362abef7f13a83f73dd963b5c3424e62e3b60..b488600989eaea453393f8c08f5e2e11e304e06b 100644 (file)
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -325,7 +325,8 @@ int networkCheckRouteCollision(virNetworkDef *def)
 
 int
 networkAddFirewallRules(virNetworkDef *def,
-                        virFirewallBackend firewallBackend)
+                        virFirewallBackend firewallBackend,
+                        virFirewall **fwRemoval)
 {
 
     networkSetupPrivateChains(firewallBackend, false);
@@ -411,13 +412,28 @@ networkAddFirewallRules(virNetworkDef *def,
         }
     }
 
-    return iptablesAddFirewallRules(def);
+    return iptablesAddFirewallRules(def, fwRemoval);
 }
 
 
 void
-networkRemoveFirewallRules(virNetworkDef *def,
-                           virFirewallBackend firewallBackend G_GNUC_UNUSED)
+networkRemoveFirewallRules(virNetworkObj *obj)
 {
-    iptablesRemoveFirewallRules(def);
+    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.
+         */
+        VIR_DEBUG("No firewall info in network status, assuming old-style iptables");
+        iptablesRemoveFirewallRules(virNetworkObjGetDef(obj));
+        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");
+    virFirewallApply(fw);
 }

diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index 7d9a061e50757854c70cf05ca1b0a3554bf55690..537b9234f81a6cab582e90870afaf3fc16b64cf9 100644 (file)
--- a/src/network/bridge_driver_nop.c
+++ b/src/network/bridge_driver_nop.c
@@ -37,12 +37,12 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNUC_UNUSED)
 }
 
 int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
-                            virFirewallBackend firewallBackend G_GNUC_UNUSED)
+                            virFirewallBackend firewallBackend G_GNUC_UNUSED,
+                            virFirewall **fwRemoval G_GNUC_UNUSED)
 {
     return 0;
 }
 
-void networkRemoveFirewallRules(virNetworkDef *def G_GNUC_UNUSED,
-                               virFirewallBackend firewallBackend G_GNUC_UNUSED)
+void networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED)
 {
 }

diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h
index 7443c3129ff0e7c3d60282e1314ce543fadd496c..cd2e3fa7b52303e6393b97b537cc9ccd49a5db47 100644 (file)
--- a/src/network/bridge_driver_platform.h
+++ b/src/network/bridge_driver_platform.h
@@ -33,7 +33,7 @@ void networkPostReloadFirewallRules(bool startup);
 int networkCheckRouteCollision(virNetworkDef *def);
 
 int networkAddFirewallRules(virNetworkDef *def,
-                            virFirewallBackend firewallBackend);
+                            virFirewallBackend firewallBackend,
+                            virFirewall **fwRemoval);
 
-void networkRemoveFirewallRules(virNetworkDef *def,
-                                virFirewallBackend firewallBackend);
+void networkRemoveFirewallRules(virNetworkObj *obj);

diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c
index 467d43c1e9441d180263c714699ddbd9382377a8..f774176b3d05fa2956fef82dfacc61cee3a83f34 100644 (file)
--- a/src/network/network_iptables.c
+++ b/src/network/network_iptables.c
@@ -1591,9 +1591,19 @@ iptablesRemoveIPSpecificFirewallRules(virFirewall *fw,
 }
 
 
-/* Add all rules for all ip addresses (and general rules) on a network */
+/* iptablesAddFirewallrules:
+ *
+ * @def - the network that needs an iptables firewall added
+ * @fwRemoval - if this is not NULL, it points to a pointer
+ *    that should be filled in with a virFirewall object containing
+ *    all the commands needed to remove this firewall at a later time.
+ *
+ * Add all rules for all ip addresses (and general rules) on a
+ * network, and optionally return a virFirewall object containing all
+ * the rules needed to later remove the firewall that has been added.
+*/
 int
-iptablesAddFirewallRules(virNetworkDef *def)
+iptablesAddFirewallRules(virNetworkDef *def, virFirewall **fwRemoval)
 {
     size_t i;
     virNetworkIPDef *ipdef;
@@ -1614,10 +1624,35 @@ iptablesAddFirewallRules(virNetworkDef *def)
                                      VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK));
     iptablesAddChecksumFirewallRules(fw, def);
 
-    return virFirewallApply(fw);
+    if (virFirewallApply(fw) < 0)
+        return -1;
+
+    if (fwRemoval) {
+        /* caller wants us to create a virFirewall object that can be
+         * applied to undo everything that was just done by * virFirewallApply()
+         */
+
+        if (virFirewallNewFromRollback(fw, fwRemoval) < 0)
+            return -1;
+    }
+
+    return 0;
 }
 
-/* Remove all rules for all ip addresses (and general rules) on a network */
+/* iptablesRemoveFirewallRules:
+ *
+ * @def - the network that needs its iptables firewall rules removed
+ *
+ * Remove all rules for all ip addresses (and general rules) on a
+ * network that is being shut down.
+ *
+ * This function assumes the set of iptables rules that were added by
+ * all versions of libvirt prior to 10.4.0; any libvirt of that
+ * release or newer may or may not have this same set of rules, and
+ * should be using the list of commands saved in NetworkObj::fwRemoval
+ * (<fwRemoval> element in the network status XML) to remove the
+ * network's firewall rules.
+ */
 void
 iptablesRemoveFirewallRules(virNetworkDef *def)
 {

diff --git a/src/network/network_iptables.h b/src/network/network_iptables.h
index cdc143f154a8e789a7a93443a5f76b22ba2b5c08..d6ffc15bb0602a54735884c2cc64f8f5daf2b3ac 100644 (file)
--- a/src/network/network_iptables.h
+++ b/src/network/network_iptables.h
@@ -23,7 +23,7 @@
 #include "virfirewall.h"
 #include "network_conf.h"
 
-int iptablesAddFirewallRules(virNetworkDef *def);
+int iptablesAddFirewallRules(virNetworkDef *def, virFirewall **fwRemoval);
 
 void iptablesRemoveFirewallRules(virNetworkDef *def);
 

diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 3a9f409e2a1d200df39a60061c0e710255d51054..082979e5dcefb6352bdc30d556a8702a5b8f35bd 100644 (file)
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -98,7 +98,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
     if (!(def = virNetworkDefParse(NULL, xml, NULL, false)))
         return -1;
 
-    if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES) < 0)
+    if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES, NULL) < 0)
         return -1;
 
     actual = actualargv = virBufferContentAndReset(&buf);