On Sun, Jan 02, 2022 at 09:41:37PM -0500, Laine Stump wrote:
> I'm currently working on switching the backend of the network driver from
> using iptables to using nftables. Due to some functionality that is not
> available with nftables (the rule that fixes up the checksum of DHCP packets
> which, btw, is only relevant for *very* old guests, e.g. RHEL5), this needs
> to be opt-in via a config file setting. In the meantime, in order to make
> this doable in a reasonable amount of time, I am *not* converting the
> nwfilter driver right away, and when I do it will need its own config file
> setting for opt-in.
>
> I've never before looked at the code for the .conf file settings at all. I
> had assumed there would be some sort of "pull" API, where code in the
> drivers could call, e.g. virConfGetString("filter_backend") and it would
> return the config setting to the caller. But when I look at it, I see that
> all daemons use the same daemonConfigLoadFile() called from
> remote_daemon.c:main() (which is shared by all the daemons) and the
> daemonConfig object that is created to hold the config settings that are
> read is only visible within main() - the only way that a config setting is
> used is by main() "pushing" it out to a static variable somewhere else where
> it is later retrieved by the interested party, e.g. the way that main()
> calls daemonSetupNetDevOpenvswitch(config), which then sets the static
> virNetDevOpenvswitchTimeout in util/virnetdevopenvswitch.c.
I'd consider the OVS approach to be a bad example
Global state needing configurable parameters for stuff in util/ should
generally be considered a design flaw. Global state should be exclusively
in the drivers, and then the desired values passed into the util APIs
explicitly.
ie ovs_timeout should have been in qemu.conf (any other drivers' config
files if appropriate).
> (NB: util/virnetdevopenvswitch.c is linked into every deamon, so even for
> the daemons that don't use it, calls to virnetdevopenvswitch.c functions
> still compile properly (and calling them is harmless), so
> virNetDevOpenvswitchTimeout is set even for daemons that never call
> openvswitch APIs).
This is another bit of technical debt. We've been lazy with putting
stuff into util that really ought not to be there.
This stuff even gets into the libvirt.so that's used client side,
so the argument that we had a single monolithic libvirtd didn't
apply either.
> If I could count on all builds using split daemons (i.e. separate
> virtnetworkd and virtnwfilterd) then I could add a similar API in
> virfirewall.c that remote_daemon.c:main() could use to set "filter_backend"
> into a static in virfirewall.c (which is used by both drivers) and
> everything would just happily work:
>
> virtnetworkd.conf:
> filter_backend = nftables
>
> virtnwfilterd.conf
> filter_backend = iptables
Putting these settings into virtnetworkd.conf and virtnwfilterd.conf
certainly makes conceptual sense.
The problem you mention is avoided by not having global state in
virtfirewall.c. Just pass the setting into every API call whuere it
is relevant.
> However, I need to also deal with the possibility that the nwfilter and
> network drivers are in the same unified libvirtd binary, and in that case
> both drivers would have the same virfirewall.c:filter_backend setting, thus
> making it impossible to use the iptables backend for the nwfilter driver and
> nftables backend for the network driver. For that case I would need separate
> settings in the config for each driver, e.g.
>
> libvirtd.conf:
> network_filter_backend = nftables
> nwfilter_backend = iptables
Definitely don't want this, as its just follwing thue mistake we did
with ovs.
> So should I perhaps declare the nftables backend for nwfilter to be a lost
> cause until everyone moves to split daemons, add a "filter_backend" setting
> that is directly set in virfirewall.c (by remote_daemon.c:main()), and then
> provide some sort of override in virFirewallNew so calls from the nwfilter
> driver can say "ignore the filter_backend setting and use iptables"?
I'm wondering how you're integrating nftables into virfirewall in
general ?
Currently we just have
VIR_FIREWALL_LAYER_ETHERNET,
VIR_FIREWALL_LAYER_IPV4,
VIR_FIREWALL_LAYER_IPV6,
which get mapped to ebtables, iptables and ip6tables internally.
Previously they could also get mapped to firewalld but we removed
that. This worked because both firewalld passthrough and the
native commands took the same syntax, so the choice of backends
was transparent to the caller.
Now with use of nftables, we have completely different syntax
for adding rules. IOW, the caller needs to decide which backend
to use, in order to decide what syntax to use with
virFirewallAddRule.
IIUC, with nftables there is no split between ethernet, ipv4
and ipv6 filtering. This makes the VIR_FIREWALL_LAYER_*
enum somewhat redundant/inappropriate as a high level
conceptual thing.
Since the arguments to virFirewallAddRule are inherantly
tied to the specific firewall command, we shoudl probably
just admit this in the API. IOW, rename
typedef enum {
VIR_FIREWALL_LAYER_ETHERNET,
VIR_FIREWALL_LAYER_IPV4,
VIR_FIREWALL_LAYER_IPV6,
VIR_FIREWALL_LAYER_LAST,
} virFirewallLayer;
to
typedef enum {
VIR_FIREWALL_TOOL_EBTABLES,
VIR_FIREWALL_TOOL_IPTABLES,
VIR_FIREWALL_TOOL_IP6TABLES,
VIR_FIREWALL_TOOL_LAST,
} virFirewallTool;
at which point we can just add
VIR_FIREWALL_TOOL_NFTABLES
Now we don't need any global config in firewall.c to select between
nftables and traditional xtables commands - it is always explicitly
given by the caller
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
