[libvirt] [PATCH 0/2] network: improve firewall rule creation error handling
This is a different approach to solving the problem describd in: https://www.redhat.com/archives/libvir-list/2019-March/msg00584.html That patch would treat each chain creation attempt as non-fatal. This means ipv4 chains still get created if ipv6 is missing, or if a subset of ip[6]tables modules are missing (eg "mangle" chain). This series takes a different approach of splitting IPv4 and IPv6 chain creation. Setup for either address family can succeed/fail independently, however, within an address family everything must still succeed. Improved error reporting means that users will see the root cause error when trying to start an error. So with this series, 'mangle' support is still compulsory for any address family, but if IPv6 lacks mangle support, this won't break IPv4 support. This is good for the default network which only does IPv4 out of the box. Daniel P. Berrangé (2): network: improve error report when firewall chain creation fails network: split setup of ipv4 and ipv6 top level chains src/network/bridge_driver.c | 3 +- src/network/bridge_driver_linux.c | 51 ++++++++++++++++++++++++---- src/network/bridge_driver_nop.c | 3 +- src/network/bridge_driver_platform.h | 2 +- src/util/viriptables.c | 14 +++----- src/util/viriptables.h | 2 +- 6 files changed, 53 insertions(+), 22 deletions(-) -- 2.20.1
During startup we create some top level chains in which all virtual network firewall rules will be placed. The upfront creation is done to avoid slowing down creation of individual virtual networks by checking for chain existance every time. There are some factors which can cause this upfront creation to fail and while a message will get into the libvirtd log this won't be seen by users who later try to start a virtual network. Instead they'll just get a message saying that the libvirt top level chain does not exist. This message is accurate, but unhelpful for solving the root cause. This patch thus saves any error during daemon startup and reports it when trying to create a virtual network later. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/bridge_driver.c | 3 +-- src/network/bridge_driver_linux.c | 29 ++++++++++++++++++++++------ src/network/bridge_driver_nop.c | 3 +-- src/network/bridge_driver_platform.h | 2 +- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6789dafd15..27d7d072ce 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2095,8 +2095,7 @@ static void networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup) { VIR_INFO("Reloading iptables rules"); - if (networkPreReloadFirewallRules(startup) < 0) - return; + networkPreReloadFirewallRules(startup); virNetworkObjListForEach(driver->networks, networkReloadFirewallRulesHelper, NULL); diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index b10d0a6c4d..04b9c079ff 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -35,11 +35,25 @@ VIR_LOG_INIT("network.bridge_driver_linux"); #define PROC_NET_ROUTE "/proc/net/route" -int networkPreReloadFirewallRules(bool startup) +static virErrorPtr errInit; + +void networkPreReloadFirewallRules(bool startup) { - int ret = iptablesSetupPrivateChains(); - if (ret < 0) - return -1; + int ret; + + /* We create global rules upfront as we don't want + * the perf hit of conditionally figuring out whether + * to create them each time a network is started. + * + * Any errors here are saved to be reported at time + * of starting the network though as that makes them + * more likely to be seen by a human + */ + ret = iptablesSetupPrivateChains(); + if (ret < 0) { + errInit = virSaveLastError(); + virResetLastError(); + } /* * If this is initial startup, and we just created the @@ -56,8 +70,6 @@ int networkPreReloadFirewallRules(bool startup) */ if (startup && ret == 1) iptablesSetDeletePrivate(false); - - return 0; } @@ -671,6 +683,11 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallPtr fw = NULL; int ret = -1; + if (errInit) { + virSetError(errInit); + return -1; + } + if (def->bridgeZone) { /* if a firewalld zone has been specified, fail/log an error diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index a0e57012f9..ea9db338cb 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -19,9 +19,8 @@ #include <config.h> -int networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED) +void networkPreReloadFirewallRules(bool startup ATTRIBUTE_UNUSED) { - return 0; } diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index baeb22bc3e..95fd64bdc7 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -58,7 +58,7 @@ struct _virNetworkDriverState { typedef struct _virNetworkDriverState virNetworkDriverState; typedef virNetworkDriverState *virNetworkDriverStatePtr; -int networkPreReloadFirewallRules(bool startup); +void networkPreReloadFirewallRules(bool startup); void networkPostReloadFirewallRules(bool startup); int networkCheckRouteCollision(virNetworkDefPtr def); -- 2.20.1
On 3/18/19 6:47 PM, Daniel P. Berrangé wrote:
During startup we create some top level chains in which all virtual network firewall rules will be placed. The upfront creation is done to avoid slowing down creation of individual virtual networks by checking for chain existance every time.
There are some factors which can cause this upfront creation to fail and while a message will get into the libvirtd log this won't be seen by users who later try to start a virtual network. Instead they'll just get a message saying that the libvirt top level chain does not exist. This message is accurate, but unhelpful for solving the root cause.
This patch thus saves any error during daemon startup and reports it when trying to create a virtual network later.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/bridge_driver.c | 3 +-- src/network/bridge_driver_linux.c | 29 ++++++++++++++++++++++------ src/network/bridge_driver_nop.c | 3 +-- src/network/bridge_driver_platform.h | 2 +- 4 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6789dafd15..27d7d072ce 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2095,8 +2095,7 @@ static void networkReloadFirewallRules(virNetworkDriverStatePtr driver, bool startup) { VIR_INFO("Reloading iptables rules"); - if (networkPreReloadFirewallRules(startup) < 0) - return; + networkPreReloadFirewallRules(startup); virNetworkObjListForEach(driver->networks, networkReloadFirewallRulesHelper, NULL); diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index b10d0a6c4d..04b9c079ff 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -35,11 +35,25 @@ VIR_LOG_INIT("network.bridge_driver_linux");
#define PROC_NET_ROUTE "/proc/net/route"
-int networkPreReloadFirewallRules(bool startup) +static virErrorPtr errInit; + +void networkPreReloadFirewallRules(bool startup) { - int ret = iptablesSetupPrivateChains(); - if (ret < 0) - return -1; + int ret;
Nitpick, this should be 'rc' or 'rv' or something like that. I view 'ret' as a variable that holds the retval of a function and this is not such case.
+ + /* We create global rules upfront as we don't want + * the perf hit of conditionally figuring out whether + * to create them each time a network is started. + * + * Any errors here are saved to be reported at time + * of starting the network though as that makes them + * more likely to be seen by a human + */ + ret = iptablesSetupPrivateChains(); + if (ret < 0) { + errInit = virSaveLastError(); + virResetLastError(); + }
ACK Michal
During startup libvirtd creates top level chains for both ipv4 and ipv6 protocols. If this fails for any reason then startup of virtual networks is blocked. The default virtual network, however, only requires use of ipv4 and some servers have ipv6 disabled so it is expected that ipv6 chain creation will fail. There could equally be servers with no ipv4, only ipv6. This patch thus makes error reporting a little more fine grained so that it works more sensibly when either ipv4 or ipv6 is disabled on the server. Only the protocols that are actually used by the virtual network have errors reported. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/bridge_driver_linux.c | 36 +++++++++++++++++++++++++------ src/util/viriptables.c | 14 ++++-------- src/util/viriptables.h | 2 +- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 04b9c079ff..4e2320ea0a 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -35,10 +35,12 @@ VIR_LOG_INIT("network.bridge_driver_linux"); #define PROC_NET_ROUTE "/proc/net/route" -static virErrorPtr errInit; +static virErrorPtr errInitV4; +static virErrorPtr errInitV6; -void networkPreReloadFirewallRules(bool startup) +int networkPreReloadFirewallRules(bool startup) { + bool created = false; int ret; /* We create global rules upfront as we don't want @@ -49,11 +51,21 @@ void networkPreReloadFirewallRules(bool startup) * of starting the network though as that makes them * more likely to be seen by a human */ - ret = iptablesSetupPrivateChains(); + ret = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV4); if (ret < 0) { - errInit = virSaveLastError(); + errInitV4 = virSaveLastError(); virResetLastError(); } + if (ret) + created = true; + + ret = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6); + if (ret < 0) { + errInitV6 = virSaveLastError(); + virResetLastError(); + } + if (ret) + created = true; /* * If this is initial startup, and we just created the @@ -68,7 +80,7 @@ void networkPreReloadFirewallRules(bool startup) * rules will be present. Thus we can safely just tell it * to always delete from the builin chain */ - if (startup && ret == 1) + if (startup && created) iptablesSetDeletePrivate(false); } @@ -683,8 +695,18 @@ int networkAddFirewallRules(virNetworkDefPtr def) virFirewallPtr fw = NULL; int ret = -1; - if (errInit) { - virSetError(errInit); + if (errInitV4 && + (virNetworkDefGetIPByIndex(def, AF_INET, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET, 0))) { + virSetError(errInitV4); + return -1; + } + + if (errInitV6 && + (virNetworkDefGetIPByIndex(def, AF_INET6, 0) || + virNetworkDefGetRouteByIndex(def, AF_INET6, 0) || + def->ipv6nogw)) { + virSetError(errInitV6); return -1; } diff --git a/src/util/viriptables.c b/src/util/viriptables.c index d67b640a3b..0e3c0ad73a 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -127,7 +127,7 @@ iptablesPrivateChainCreate(virFirewallPtr fw, int -iptablesSetupPrivateChains(void) +iptablesSetupPrivateChains(virFirewallLayer layer) { virFirewallPtr fw = NULL; int ret = -1; @@ -143,17 +143,11 @@ iptablesSetupPrivateChains(void) }; bool changed = false; iptablesGlobalChainData data[] = { - { VIR_FIREWALL_LAYER_IPV4, "filter", + { layer, "filter", filter_chains, ARRAY_CARDINALITY(filter_chains), &changed }, - { VIR_FIREWALL_LAYER_IPV4, "nat", + { layer, "nat", natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, - { VIR_FIREWALL_LAYER_IPV4, "mangle", - natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, - { VIR_FIREWALL_LAYER_IPV6, "filter", - filter_chains, ARRAY_CARDINALITY(filter_chains), &changed }, - { VIR_FIREWALL_LAYER_IPV6, "nat", - natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, - { VIR_FIREWALL_LAYER_IPV6, "mangle", + { layer, "mangle", natmangle_chains, ARRAY_CARDINALITY(natmangle_chains), &changed }, }; size_t i; diff --git a/src/util/viriptables.h b/src/util/viriptables.h index 903f390f89..e680407ec8 100644 --- a/src/util/viriptables.h +++ b/src/util/viriptables.h @@ -24,7 +24,7 @@ # include "virsocketaddr.h" # include "virfirewall.h" -int iptablesSetupPrivateChains (void); +int iptablesSetupPrivateChains (virFirewallLayer layer); void iptablesSetDeletePrivate (bool pvt); -- 2.20.1
On 3/18/19 6:47 PM, Daniel P. Berrangé wrote:
During startup libvirtd creates top level chains for both ipv4 and ipv6 protocols. If this fails for any reason then startup of virtual networks is blocked.
The default virtual network, however, only requires use of ipv4 and some servers have ipv6 disabled so it is expected that ipv6 chain creation will fail. There could equally be servers with no ipv4, only ipv6.
This patch thus makes error reporting a little more fine grained so that it works more sensibly when either ipv4 or ipv6 is disabled on the server. Only the protocols that are actually used by the virtual network have errors reported.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/network/bridge_driver_linux.c | 36 +++++++++++++++++++++++++------ src/util/viriptables.c | 14 ++++-------- src/util/viriptables.h | 2 +- 3 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index 04b9c079ff..4e2320ea0a 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -35,10 +35,12 @@ VIR_LOG_INIT("network.bridge_driver_linux");
#define PROC_NET_ROUTE "/proc/net/route"
-static virErrorPtr errInit; +static virErrorPtr errInitV4; +static virErrorPtr errInitV6;
-void networkPreReloadFirewallRules(bool startup) +int networkPreReloadFirewallRules(bool startup)
I guess you didn't mean to do this change.
{ + bool created = false; int ret;
/* We create global rules upfront as we don't want @@ -49,11 +51,21 @@ void networkPreReloadFirewallRules(bool startup) * of starting the network though as that makes them * more likely to be seen by a human */ - ret = iptablesSetupPrivateChains(); + ret = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV4); if (ret < 0) { - errInit = virSaveLastError(); + errInitV4 = virSaveLastError(); virResetLastError(); } + if (ret)
Again, small nitpick, if (ret > 0).
+ created = true; + + ret = iptablesSetupPrivateChains(VIR_FIREWALL_LAYER_IPV6); + if (ret < 0) { + errInitV6 = virSaveLastError(); + virResetLastError(); + } + if (ret) + created = true;
This fixes my usecase, so ACK Michal
On Mon, 2019-03-18 at 17:47 +0000, Daniel P. Berrangé wrote:
This is a different approach to solving the problem describd in:
https://www.redhat.com/archives/libvir-list/2019-March/msg00584.html
That patch would treat each chain creation attempt as non-fatal. This means ipv4 chains still get created if ipv6 is missing, or if a subset of ip[6]tables modules are missing (eg "mangle" chain).
This series takes a different approach of splitting IPv4 and IPv6 chain creation. Setup for either address family can succeed/fail independently, however, within an address family everything must still succeed. Improved error reporting means that users will see the root cause error when trying to start an error.
So with this series, 'mangle' support is still compulsory for any address family, but if IPv6 lacks mangle support, this won't break IPv4 support. This is good for the default network which only does IPv4 out of the box.
Daniel P. Berrangé (2): network: improve error report when firewall chain creation fails network: split setup of ipv4 and ipv6 top level chains
src/network/bridge_driver.c | 3 +- src/network/bridge_driver_linux.c | 51 ++++++++++++++++++++++++---- src/network/bridge_driver_nop.c | 3 +- src/network/bridge_driver_platform.h | 2 +- src/util/viriptables.c | 14 +++----- src/util/viriptables.h | 2 +- 6 files changed, 53 insertions(+), 22 deletions(-)
The changes make sense and they make the issue I was encountering on my machine go away, so with the tweaks Michal already pointed out Reviewed-by: Andrea Bolognani <abologna@redhat.com> Is this worth backporting to the stable 5.1.0 branch? -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik