10:19 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, 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.
As a result of doing this, not only can we change the details of the
rules we add for networks from one build/release of libvirt to another
and painlessly upgrade, but the user can also switch from one firewall
backend to another by simply changing the setting in network.conf and
restarting libvirtd/virtnetworkd.
Signed-off-by: Laine Stump <laine(a)redhat.com>
---
src/network/bridge_driver.c | 34 +++++++++++------
src/network/bridge_driver_linux.c | 56 +++++++++++++++++++++-------
src/network/bridge_driver_nop.c | 4 +-
src/network/bridge_driver_platform.h | 4 +-
tests/networkxml2firewalltest.c | 2 +-
5 files changed, 70 insertions(+), 30 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index fb353e449a..9f876d7418 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1698,8 +1698,10 @@ 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,
+ virNetworkObjGetFwRemovalPtr(obj),
+ cfg->firewallBackend));
break;
case VIR_NETWORK_FORWARD_OPEN:
@@ -1950,8 +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,
+ virNetworkObjGetFwRemovalPtr(obj),
+ cfg->firewallBackend) < 0) {
goto error;
+ }
firewalRulesAdded = true;
@@ -2037,7 +2042,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
if (firewalRulesAdded &&
def->forward.type != VIR_NETWORK_FORWARD_OPEN)
- networkRemoveFirewallRules(def, cfg->firewallBackend);
+ networkRemoveFirewallRules(obj);
virNetworkObjUnrefMacMap(obj);
@@ -2049,8 +2054,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
static int
-networkShutdownNetworkVirtual(virNetworkObj *obj,
- virNetworkDriverConfig *cfg)
+networkShutdownNetworkVirtual(virNetworkObj *obj)
{
virNetworkDef *def = virNetworkObjGetDef(obj);
pid_t dnsmasqPid;
@@ -2076,7 +2080,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));
@@ -2380,7 +2384,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:
@@ -3243,7 +3247,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:
@@ -3270,16 +3274,22 @@ 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,
+ virNetworkObjGetFwRemovalPtr(obj),
+ cfg->firewallBackend));
+ }
goto cleanup;
}
/* @def is replaced */
def = virNetworkObjGetDef(obj);
- if (needFirewallRefresh && networkAddFirewallRules(def,
cfg->firewallBackend) < 0)
+ if (needFirewallRefresh && networkAddFirewallRules(def,
+
virNetworkObjGetFwRemovalPtr(obj),
+ cfg->firewallBackend) < 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 f6bae334aa..9adf05c05d 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -818,6 +818,7 @@ networkRemoveIPSpecificFirewallRules(virFirewall *fw,
/* Add all rules for all ip addresses (and general rules) on a network */
int
networkAddFirewallRules(virNetworkDef *def,
+ virFirewall **fwRemoval,
virFirewallBackend firewallBackend)
{
size_t i;
@@ -930,30 +931,59 @@ networkAddFirewallRules(virNetworkDef *def,
| VIR_FIREWALL_TRANSACTION_AUTO_ROLLBACK));
networkAddChecksumFirewallRules(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 */
void
-networkRemoveFirewallRules(virNetworkDef *def,
- virFirewallBackend firewallBackend)
+networkRemoveFirewallRules(virNetworkObj *obj)
{
size_t i;
+ virNetworkDef *def = virNetworkObjGetDef(obj);
virNetworkIPDef *ipdef;
- g_autoptr(virFirewall) fw = virFirewallNew(firewallBackend);
+ g_autoptr(virFirewall) fw = NULL;
+
+ if ((fw = virNetworkObjGetFwRemoval(obj))) {
+ /* exact list of removal rules was saved
+ * when the firewall rules were originally added
+ */
+ VIR_DEBUG("Removing exact firewall rules previously saved");
+ virNetworkObjSetFwRemoval(obj, NULL);
- virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
- networkRemoveChecksumFirewallRules(fw, def);
+ } else {
+ /* The firewall rules were added by an older libvirt that
+ * didn't automatically save removal rules, so we guess
+ * at what rules were added (NB: any libvirt old enough
+ * to require this only supported the iptables backend)
+ */
+ VIR_DEBUG("Removing a guess at what firewall rules were previously
saved");
+ fw = virFirewallNew(VIR_FIREWALL_BACKEND_IPTABLES);
- virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
+ virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
+ networkRemoveChecksumFirewallRules(fw, def);
- for (i = 0;
- (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i));
- i++) {
- if (networkRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0)
- return;
+ virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
+
+ for (i = 0;
+ (ipdef = virNetworkDefGetIPByIndex(def, AF_UNSPEC, i));
+ i++) {
+ if (networkRemoveIPSpecificFirewallRules(fw, def, ipdef) < 0)
+ return;
+ }
+ networkRemoveGeneralFirewallRules(fw, def);
}
- networkRemoveGeneralFirewallRules(fw, def);
virFirewallApply(fw);
}
diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c
index 7d9a061e50..e73831ccc6 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,
+ virFirewall **fwRemoval G_GNUC_UNUSED,
virFirewallBackend firewallBackend 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..81305f7a0d 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,
+ virFirewall **fwRemoval,
virFirewallBackend firewallBackend);
-void networkRemoveFirewallRules(virNetworkDef *def,
- virFirewallBackend firewallBackend);
+void networkRemoveFirewallRules(virNetworkObj *obj);
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c
index 6e9eca0832..8254fde94e 100644
--- a/tests/networkxml2firewalltest.c
+++ b/tests/networkxml2firewalltest.c
@@ -106,7 +106,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(def = virNetworkDefParse(NULL, xml, NULL, false)))
return -1;
- if (networkAddFirewallRules(def, backend) < 0)
+ if (networkAddFirewallRules(def, NULL, backend) < 0)
return -1;
actual = actualargv = virBufferContentAndReset(&buf);
--
2.39.2