This patch adds some previously missing test cases that test for
proper firewall rule creation when the following are included in the
network definition:
* <forward dev='blah'>
* no forward element (an "isolated" network)
* nat port range when only ipv4 is nat-ed
* nat port range when both ipv4 & ipv6 are nated
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
Run all the networkxml2firewall tests twice - once with iptables
backend, and once with the nftables backend.
The results files for the existing iptables tests were previously
named *.args. That has been changed to *.iptables, and the results
files for the new nftables tests are named *.nftables.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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>
It still can have only one useful value ("iptables"), but once a 2nd
value is supported, it will be selectable by setting
"firewall_backend=nftables" in /etc/libvirt/network.conf.
If firewall_backend isn't set in network.conf, then libvirt will check
to see if FIREWALL_BACKEND_DEFAULT_1 is available and, if so, set
that. (Since FIREWALL_BACKEND_DEFAULT_1 is currently "iptables", this
means checking to see it the iptables binary is present on the
system). If the default backend isn't available, that is considered a
fatal error (since no networks can be started anyway), so an error is
logged and startup of the network driver fails.
NB: network.conf is itself created from network.conf.in at build time,
and the advertised default setting of firewall_backend (in a commented
out line) is set from the meson_options.txt setting
"firewall_backend_default_1". This way the conf file will have correct
information no matter what ordering is chosen for default backend at
build time (as more backends are added, settings will be added for
"firewall_backend_default_n", and those will be settable in
meson_options.txt and on the meson commandline to change the ordering
of the auto-detection when no backend is set in network.conf).
virNetworkLoadDriverConfig() may look more complicated than necessary,
but as additional backends are added, it will be easier to add checks
for those backends (and to re-order the checks based on builders'
preferences).
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Replace virNetworkDefParseString/File by direct calls to
virNetworkDefParse.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
After converting virNetworkDef * to g_autoptr(virNetworkDef) the
cleanup codepath was empty, so it has been removed.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since the currentBackend (direct vs. firewalld) setting is no longer
used for anything, we don't need to set it (either explicitly from
tests, or implicitly during init), and can completely remove it.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
It isn't intuitive (to me) that a test just converting xml text into
iptables commands should need to call dbus, so rather than forcing the
next person to look through the commit logs and/or run the test under
gdb to understand why this is needed, just add a short comment in the
source.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Instead of checking for specific error that the binaries are not
available mock the virFindFileInPath function. This way we don't have
to skip these tests on host where the binaries are missing.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Historically, we declared pointer type to our types:
typedef struct _virXXX virXXX;
typedef virXXX *virXXXPtr;
But usefulness of such declaration is questionable, at best.
Unfortunately, we can't drop every such declaration - we have to
carry some over, because they are part of public API (e.g.
virDomainPtr). But for internal types - we can do drop them and
use what every other C project uses 'virXXX *'.
This change was generated by a very ugly shell script that
generated sed script which was then called over each file in the
repository. For the shell script refer to the cover letter:
https://listman.redhat.com/archives/libvir-list/2021-March/msg00537.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
virCommandSetDryRun allows to invoke virCommandToString so that the
command string is already wrapped.
We now also need to load the base arguments file without unwrapping the
arguments.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Enable the internal path clearing instead of using
virTestClearCommandPath.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
virCommandToStringFull used internally when virCommandSetDryRun is
requested allows to strip command path and wrap lines nicely. Expose
these via virCommandSetDryRun so that tests can use those features
instead of local hacks.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
While virCommandSetDryRun is used in tests only, there were some cases
when error paths would not call the function with NULL arguments to
reset the dry run infrastructure.
Introduce virCommandDryRunToken type which must be allocated via
virCommandDryRunTokenNew and passed to virCommandSetDryRun.
This way we can use automatic variable cleaning to trigger the cleanup
of virCommandSetDryRun parameters and also the use of the token variable
ensures that all callers of virCommandSetDryRun clean up after
themselves and also that the token isn't left unused in the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
iptables and ip6tables have had a "-w" commandline option to grab a
systemwide lock that prevents two iptables invocations from modifying
the iptables chains since 2013 (upstream commit 93587a04 in
iptables-1.4.20). Similarly, ebtables has had a "--concurrent"
commandline option for the same purpose since 2011 (in the upstream
ebtables commit f9b4bcb93, which was present in ebtables-2.0.10.4).
Libvirt added code to conditionally use the commandline option for
iptables/ip6tables in upstream commit ba95426d6f (libvirt-1.2.0,
November 2013), and for ebtables in upstream commit dc33e6e4a5
(libvirt-1.2.11, November 2014) (the latter actually *re*-added the
locking for iptables/ip6tables, as it had accidentally been removed
during a refactor of firewall code in the interim).
I say "conditionally" because a check was made during firewall module
initialization that tried executing a test command with the
-w/--concurrent option, and only continued using it for actual
commands if that test command completed successfully. At the time the
code was added this was a reasonable thing to do, as it had been less
than a year since introduction of -w to iptables, so many distros
supported by libvirt were still using iptables (and possibly even
ebtables) versions too old to have the new commandline options.
It is now 2020, and as far as I can discern from repology.org (and
manually examining a RHEL7.9 system), every version of every distro
that is supported by libvirt now uses new enough versions of both
iptables and ebtables that they all have support for -w/--concurrent.
That means we can finally remove the conditional code and simply
always use them.
Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
GLib implementation of g_dbus_connection_call_sync() calls
g_variant_ref_sink() on the passed @parameters to make sure they have
proper reference. If the original reference is floating the
g_dbus_connection_call_sync() consumes it, but if it's normal reference
it will just add another one.
Our mock functions were only freeing the @parameters which is incorrect
and doesn't reflect how the real implementation works.
Reported-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This test calls into src/util/virfirewalld.c where it uses DBus to
figure out if firewalld is registered. Without the mock it luckily
fails and the test works correctly.
To isolate the tests from host environment we should mock the DBus
calls.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Now that we have support for IPv6 in the iptables helpers, and a new
option in the XML schema, we can wire up support for it in the network
driver.
Reviewed-by: Laine Stump <laine@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Compilers are not very good at detecting this problem. Fixed by manual
inspection of compilation warnings after replacing 'VIR_FREE' with an
empty macro.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com
The function now does not return an error so we can drop it fully.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Replace all the occurrences of
ignore_value(VIR_STRDUP_QUIET(a, b));
with
a = g_strdup(b);
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Since commit 44e7f02915
util: rewrite auto cleanup macros to use glib's equivalent
VIR_AUTOFREE is just an alias for g_autofree. Use the GLib macros
directly instead of our custom aliases.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use G_GNUC_UNUSED from GLib instead of ATTRIBUTE_UNUSED.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Pass an xmlopt argument through all the needed network conf
functions, like is done for domain XML handling. No functional
change for now
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Creating firewall rules for the virtual networks causes the kernel to
load the conntrack module. This imposes a significant performance
penalty on Linux network traffic. Thus we want to only take that hit if
we actually have virtual networks running.
We need to create global firewall rules during startup in order to
"upgrade" rules for any running networks created by older libvirt.
If no running networks are present though, we can safely delay setup
until the time we actually start a network.
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This code snippet has clearly been cargo-culted, and all its
instances can be safely dropped seeing as 1) a much better
way to handle the scenario in C programs would be to pass the
value via the preprocessor, and 2) the value is actually not
used anywhere after being defined.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Require that all headers are guarded by a symbol named
LIBVIRT_$FILENAME
where $FILENAME is the uppercased filename, with all characters
outside a-z changed into '_'.
Note we do not use a leading __ because that is technically a
namespace reserved for the toolchain.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The networkxml2firewalltest sets virCommand to dry run mode but doesn't
provide a callback to fill in stdout/stderr. As a result when the
firewall code queries rules it gets a NULL output and so never triggers
the callback to process output.
This trivial change just returns an empty string for the command output
in order to ensure the callback gets triggered. It has no effect right
now, but in future patches this will trigger greater test coverage.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Right-aligning backslashes when defining macros or using complex
commands in Makefiles looks cute, but as soon as any changes is
required to the code you end up with either distractingly broken
alignment or unnecessarily big diffs where most of the changes
are just pushing all backslashes a few characters to one side.
Generated using
$ git grep -El '[[:blank:]][[:blank:]]\\$' | \
grep -E '*\.([chx]|am|mk)$$' | \
while read f; do \
sed -Ei 's/[[:blank:]]*[[:blank:]]\\$/ \\/g' "$f"; \
done
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
When running tests in a restricted container (as opposed to a full
OS install), we can't assume ebtables/iptbles/ip6tables are going
to be installed. We must check this and mark the tests as skipped.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
There are two more places after commit 3865941b that need to be adapted
in order to get rid of some test failures when building as root.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Using the virCommand dry run capability, capture iptables rules
created by various network XML documents.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
