9:53 p.m.
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(a)redhat.com>
==
NB: the current implementation saves only the commands necessary to
remove the network's firewall, and names this <firewall> in the status
XML. It would be simple to instead save the *entire* virFirewall
object (thus also including the commands that were used to add the
firewall, as well as the commands needed to remove it) - although very
verbose, it's possible it could be useful when debugging a firewall
issue (since it's not obvious which rules were added for which network
when just looking at the output of "nft list ruleset". Alternately, we
could continue to store only the removal commands, but maybe change
the name of the element in XML from <firewall> to <fwRemoval> (which
would leave the door open to expanding what is saved in the
future). Any opinions on this?
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 31 ++++++++++++--------
src/network/bridge_driver_linux.c | 26 +++++++++++++----
src/network/bridge_driver_nop.c | 6 ++--
src/network/bridge_driver_platform.h | 6 ++--
src/network/network_iptables.c | 43 +++++++++++++++++++++++++---
src/network/network_iptables.h | 2 +-
tests/networkxml2firewalltest.c | 2 +-
7 files changed, 87 insertions(+), 29 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 38e4ab84ad..c54a595d4d 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1696,8 +1696,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,
+ virNetworkObjGetFwRemovalPtr(obj)));
break;
case VIR_NETWORK_FORWARD_OPEN:
@@ -1949,8 +1950,10 @@ 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,
+ virNetworkObjGetFwRemovalPtr(obj)) < 0) {
goto error;
+ }
firewalRulesAdded = true;
@@ -2065,7 +2068,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
if (firewalRulesAdded &&
def->forward.type != VIR_NETWORK_FORWARD_OPEN)
- networkRemoveFirewallRules(def, cfg->firewallBackend);
+ networkRemoveFirewallRules(obj);
virNetworkObjUnrefMacMap(obj);
@@ -2077,8 +2080,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
static int
-networkShutdownNetworkVirtual(virNetworkObj *obj,
- virNetworkDriverConfig *cfg)
+networkShutdownNetworkVirtual(virNetworkObj *obj)
{
virNetworkDef *def = virNetworkObjGetDef(obj);
pid_t dnsmasqPid;
@@ -2104,7 +2106,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));
@@ -2408,7 +2410,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:
@@ -3259,7 +3261,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:
@@ -3286,16 +3288,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,
+ virNetworkObjGetFwRemovalPtr(obj)));
+ }
goto cleanup;
}
/* @def is replaced */
def = virNetworkObjGetDef(obj);
- if (needFirewallRefresh && networkAddFirewallRules(def,
cfg->firewallBackend) < 0)
+ if (needFirewallRefresh &&
+ networkAddFirewallRules(def, cfg->firewallBackend,
+ virNetworkObjGetFwRemovalPtr(obj)) < 0) {
goto cleanup;
+ }
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 20671e3ec5..61a4a6cdc1 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -333,7 +333,8 @@ int networkCheckRouteCollision(virNetworkDef *def)
int
networkAddFirewallRules(virNetworkDef *def,
- virFirewallBackend firewallBackend)
+ virFirewallBackend firewallBackend,
+ virFirewall **fwRemoval)
{
networkSetupPrivateChains(firewallBackend, false);
@@ -419,13 +420,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 7d9a061e50..537b9234f8 100644
--- 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 7443c3129f..cd2e3fa7b5 100644
--- 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 467d43c1e9..f774176b3d 100644
--- 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 cdc143f154..d6ffc15bb0 100644
--- 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 e61787daec..93f693a8d7 100644
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -105,7 +105,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);
--
2.44.0