[PATCH 0/3] meson: Improve default firewall backend configuration
Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1308917840 See 1/3 for details. Andrea Bolognani (3): meson: Improve default firewall backend configuration meson: Include firewall backend selection in summary rpm: Configure firewall backends explicitly libvirt.spec.in | 1 + meson.build | 21 ++++++++++++++------- meson_options.txt | 3 +-- src/network/bridge_driver_conf.c | 6 +++++- src/network/meson.build | 6 ++++-- src/network/network.conf.in | 13 +++++++------ 6 files changed, 32 insertions(+), 18 deletions(-) -- 2.45.1
The current implementation requires users to configure the preference as such: -Dfirewall_backend_default_1=iptables -Dfirewall_backend_default_2=nftables In addition to being more verbose than one would hope, there are several things that could go wrong. First of all, meson performs no validation on the provided values, so mistakes will only be caught by the compiler. Additionally, it's entirely possible to provide nonsensical combinations, such as repeating the same value twice. Change things so that the preference can now be configured as such: -Dfirewall_backend_priority=iptables,nftables Checks have been added to prevent invalid values from being accepted. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 16 +++++++++------- meson_options.txt | 3 +-- src/network/bridge_driver_conf.c | 6 +++++- src/network/meson.build | 6 ++++-- src/network/network.conf.in | 13 +++++++------ 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/meson.build b/meson.build index f67c3d2724..ed0e9686f8 100644 --- a/meson.build +++ b/meson.build @@ -1635,15 +1635,17 @@ endif if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD') conf.set('WITH_NETWORK', 1) - firewall_backend_default_1 = get_option('firewall_backend_default_1') - firewall_backend_default_conf = firewall_backend_default_1 - firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_1.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1) - firewall_backend_default_2 = get_option('firewall_backend_default_2') - firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_2.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2) + firewall_backend_priority = get_option('firewall_backend_priority') + if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2) + error('invalid value for firewall_backend_priority option') + endif + conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[0].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[1].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length()) elif get_option('driver_network').enabled() error('libvirtd must be enabled to build the network driver') endif diff --git a/meson_options.txt b/meson_options.txt index ad354a8668..8723d13231 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for st option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support') # dep:firewalld option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone') -option('firewall_backend_default_1', type: 'string', value: 'nftables', description: 'first firewall backend to try when none is specified') -option('firewall_backend_default_2', type: 'string', value: 'iptables', description: 'second firewall backend to try when none is specified (and first is unavailable)') +option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first') option('host_validate', type: 'feature', value: 'auto', description: 'build virt-host-validate') option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check', 'none'], value: 'check', description: 'Style of init script to install') option('loader_nvram', type: 'string', value: '', description: 'Pass list of pairs of <loader>:<nvram> paths. Both pairs and list items are separated by a colon.') diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c index 8f4956dace..e2f3613a41 100644 --- a/src/network/bridge_driver_conf.c +++ b/src/network/bridge_driver_conf.c @@ -67,8 +67,12 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, g_autofree char *fwBackendStr = NULL; bool fwBackendSelected = false; size_t i; - int fwBackends[] = { FIREWALL_BACKEND_DEFAULT_1, FIREWALL_BACKEND_DEFAULT_2 }; + int fwBackends[] = { + FIREWALL_BACKEND_PRIORITY_0, + FIREWALL_BACKEND_PRIORITY_1, + }; G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == VIR_FIREWALL_BACKEND_LAST); + G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == FIREWALL_BACKEND_PRIORITY_NUM); int nFwBackends = G_N_ELEMENTS(fwBackends); if (access(filename, R_OK) == 0) { diff --git a/src/network/meson.build b/src/network/meson.build index bf2893accc..07cd5cda55 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -51,7 +51,8 @@ if conf.has('WITH_NETWORK') } network_options_conf = configuration_data({ - 'FIREWALL_BACKEND': firewall_backend_default_conf, + 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), + 'FIREWALL_BACKEND': firewall_backend_priority[0], }) network_conf = configure_file( @@ -61,7 +62,8 @@ if conf.has('WITH_NETWORK') ) network_options_hack_conf = configuration_data({ - 'FIREWALL_BACKEND': firewall_backend_default_conf, + 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), + 'FIREWALL_BACKEND': firewall_backend_priority[0], # This hack is necessary because the output file is going to be # used as input for another configure_file() call later, which # will take care of substituting @CONFIG@ with useful data diff --git a/src/network/network.conf.in b/src/network/network.conf.in index f579f39fcd..5ed64a04a5 100644 --- a/src/network/network.conf.in +++ b/src/network/network.conf.in @@ -12,12 +12,13 @@ # iptables - use iptables commands to construct the firewall # nftables - use nft commands to construct the firewall # -# If firewall_backend isn't set in this file, libvirt will -# prefer the @FIREWALL_BACKEND@ backend *if the necessary package. -# binary is installed*, otherwise it will look for the package/binary -# needed for the other backend and use that if available. If neither -# is available on the host, then the network driver will fail to -# start, and an error will be logged. +# If firewall_backend isn't configured, libvirt will choose the +# first available backend from the following list: +# +# [@FIREWALL_BACKEND_PRIORITY@] +# +# If no backend is available on the host, then the network driver +# will fail to start, and an error will be logged. # # (NB: switching from one backend to another while there are active # virtual networks *is* supported. The change will take place the -- 2.45.1
On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
The current implementation requires users to configure the preference as such:
-Dfirewall_backend_default_1=iptables -Dfirewall_backend_default_2=nftables
In addition to being more verbose than one would hope, there are several things that could go wrong.
First of all, meson performs no validation on the provided values, so mistakes will only be caught by the compiler. Additionally, it's entirely possible to provide nonsensical combinations, such as repeating the same value twice.
Change things so that the preference can now be configured as such:
-Dfirewall_backend_priority=iptables,nftables
Checks have been added to prevent invalid values from being accepted.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 16 +++++++++------- meson_options.txt | 3 +-- src/network/bridge_driver_conf.c | 6 +++++- src/network/meson.build | 6 ++++-- src/network/network.conf.in | 13 +++++++------ 5 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/meson.build b/meson.build index f67c3d2724..ed0e9686f8 100644 --- a/meson.build +++ b/meson.build @@ -1635,15 +1635,17 @@ endif
if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD') conf.set('WITH_NETWORK', 1) - firewall_backend_default_1 = get_option('firewall_backend_default_1') - firewall_backend_default_conf = firewall_backend_default_1 - firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_1.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1)
- firewall_backend_default_2 = get_option('firewall_backend_default_2') - firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_2.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2) + firewall_backend_priority = get_option('firewall_backend_priority') + if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2)
No need to have a check for specific values. Meson will already check if they are from the list of choices defined in meson_options.txt .
+ error('invalid value for firewall_backend_priority option') + endif
+ conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[0].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[1].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length()) elif get_option('driver_network').enabled() error('libvirtd must be enabled to build the network driver') endif diff --git a/meson_options.txt b/meson_options.txt index ad354a8668..8723d13231 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for st option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support') # dep:firewalld option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone') -option('firewall_backend_default_1', type: 'string', value: 'nftables', description: 'first firewall backend to try when none is specified') -option('firewall_backend_default_2', type: 'string', value: 'iptables', description: 'second firewall backend to try when none is specified (and first is unavailable)') +option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first')
What about "order of firewall backends to try"? The part "preferred ones first" sounds strange to me. Otherwise looks good. Pavel
On 5/28/24 12:31 PM, Pavel Hrdina wrote:
On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
The current implementation requires users to configure the preference as such:
-Dfirewall_backend_default_1=iptables -Dfirewall_backend_default_2=nftables
In addition to being more verbose than one would hope, there are several things that could go wrong.
First of all, meson performs no validation on the provided values, so mistakes will only be caught by the compiler. Additionally, it's entirely possible to provide nonsensical combinations, such as repeating the same value twice.
Change things so that the preference can now be configured as such:
-Dfirewall_backend_priority=iptables,nftables
Checks have been added to prevent invalid values from being accepted.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 16 +++++++++------- meson_options.txt | 3 +-- src/network/bridge_driver_conf.c | 6 +++++- src/network/meson.build | 6 ++++-- src/network/network.conf.in | 13 +++++++------ 5 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/meson.build b/meson.build index f67c3d2724..ed0e9686f8 100644 --- a/meson.build +++ b/meson.build @@ -1635,15 +1635,17 @@ endif
if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD') conf.set('WITH_NETWORK', 1) - firewall_backend_default_1 = get_option('firewall_backend_default_1') - firewall_backend_default_conf = firewall_backend_default_1 - firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_1.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1)
- firewall_backend_default_2 = get_option('firewall_backend_default_2') - firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_2.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2) + firewall_backend_priority = get_option('firewall_backend_priority') + if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2)
No need to have a check for specific values. Meson will already check if they are from the list of choices defined in meson_options.txt .
But we don't just need to check that the values in the list are all valid options; we also want to make sure that nobody has entered the same value multiple times (which this ends up doing by making sure that there is at least one entry for each valid value, and that the list is the same length as the number of valid values). Or do we not care if someone repeats the same value? Maybe somebody wants to include iptables support in the build, but not look for it automatically (instead only use it if it's explicitly requested in network.conf). One way of doing that would be to sent firewall_backend_priority = nftables,nftables (that does seem a bit obtuse; perhaps it would be better to allow limiting the length of the option list to 1) Anyway, I'm fine with it either way. Both are better than what I had before.
+ error('invalid value for firewall_backend_priority option') + endif
+ conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[0].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[1].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length()) elif get_option('driver_network').enabled() error('libvirtd must be enabled to build the network driver') endif diff --git a/meson_options.txt b/meson_options.txt index ad354a8668..8723d13231 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for st option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support') # dep:firewalld option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone') -option('firewall_backend_default_1', type: 'string', value: 'nftables', description: 'first firewall backend to try when none is specified') -option('firewall_backend_default_2', type: 'string', value: 'iptables', description: 'second firewall backend to try when none is specified (and first is unavailable)') +option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first')
What about "order of firewall backends to try"? The part "preferred ones first" sounds strange to me.
Otherwise looks good.
Pavel
On Tue, May 28, 2024 at 12:50:51PM GMT, Laine Stump wrote:
On 5/28/24 12:31 PM, Pavel Hrdina wrote:
On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
+ if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2)
No need to have a check for specific values. Meson will already check if they are from the list of choices defined in meson_options.txt .
But we don't just need to check that the values in the list are all valid options; we also want to make sure that nobody has entered the same value multiple times (which this ends up doing by making sure that there is at least one entry for each valid value, and that the list is the same length as the number of valid values).
Yes, that was exactly the idea.
Or do we not care if someone repeats the same value? Maybe somebody wants to include iptables support in the build, but not look for it automatically (instead only use it if it's explicitly requested in network.conf). One way of doing that would be to sent firewall_backend_priority = nftables,nftables
(that does seem a bit obtuse; perhaps it would be better to allow limiting the length of the option list to 1)
If that's something that we want to allow, then we should include explicit support for it rather than make it possible through obscure runes :) I'm not sure we really need to bother, but I don't feel strongly either way so I could be persuaded to look into it. Perhaps as an after-release follow up, though?
+option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first')
What about "order of firewall backends to try"? The part "preferred ones first" sounds strange to me.
Sure, that works too. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, May 28, 2024 at 09:59:17AM -0700, Andrea Bolognani wrote:
On Tue, May 28, 2024 at 12:50:51PM GMT, Laine Stump wrote:
On 5/28/24 12:31 PM, Pavel Hrdina wrote:
On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
+ if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2)
No need to have a check for specific values. Meson will already check if they are from the list of choices defined in meson_options.txt .
But we don't just need to check that the values in the list are all valid options; we also want to make sure that nobody has entered the same value multiple times (which this ends up doing by making sure that there is at least one entry for each valid value, and that the list is the same length as the number of valid values).
Yes, that was exactly the idea.
True, that is not checked so we still need to duplicate the list here that I wanted to avoid.
Or do we not care if someone repeats the same value? Maybe somebody wants to include iptables support in the build, but not look for it automatically (instead only use it if it's explicitly requested in network.conf). One way of doing that would be to sent firewall_backend_priority = nftables,nftables
(that does seem a bit obtuse; perhaps it would be better to allow limiting the length of the option list to 1)
If that's something that we want to allow, then we should include explicit support for it rather than make it possible through obscure runes :)
I'm not sure we really need to bother, but I don't feel strongly either way so I could be persuaded to look into it. Perhaps as an after-release follow up, though?
+option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first')
What about "order of firewall backends to try"? The part "preferred ones first" sounds strange to me.
Sure, that works too.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On 5/28/24 12:59 PM, Andrea Bolognani wrote:
On Tue, May 28, 2024 at 12:50:51PM GMT, Laine Stump wrote:
On 5/28/24 12:31 PM, Pavel Hrdina wrote:
On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
+ if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2)
No need to have a check for specific values. Meson will already check if they are from the list of choices defined in meson_options.txt .
But we don't just need to check that the values in the list are all valid options; we also want to make sure that nobody has entered the same value multiple times (which this ends up doing by making sure that there is at least one entry for each valid value, and that the list is the same length as the number of valid values).
Yes, that was exactly the idea.
Or do we not care if someone repeats the same value? Maybe somebody wants to include iptables support in the build, but not look for it automatically (instead only use it if it's explicitly requested in network.conf). One way of doing that would be to sent firewall_backend_priority = nftables,nftables
(that does seem a bit obtuse; perhaps it would be better to allow limiting the length of the option list to 1)
If that's something that we want to allow, then we should include explicit support for it rather than make it possible through obscure runes :)
Agreed. Just trying to come up with a valid advantage for not checking that all values are present :-)
I'm not sure we really need to bother, but I don't feel strongly either way so I could be persuaded to look into it. Perhaps as an after-release follow up, though?
Seeing that we could all change our opinion tomorrow, my choice would be to push what you currently have and then discuss tweaks later. The important thing that needs to be gotten correct from the beginning is the basic public "API", and I think the way you have it is solid, and none of these details will affect that.
+option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first')
What about "order of firewall backends to try"? The part "preferred ones first" sounds strange to me.
Sure, that works too.
On 5/28/24 11:49 AM, Andrea Bolognani wrote:
The current implementation requires users to configure the preference as such:
-Dfirewall_backend_default_1=iptables -Dfirewall_backend_default_2=nftables
In addition to being more verbose than one would hope, there are several things that could go wrong.
First of all, meson performs no validation on the provided values, so mistakes will only be caught by the compiler. Additionally, it's entirely possible to provide nonsensical combinations, such as repeating the same value twice.
Change things so that the preference can now be configured as such:
-Dfirewall_backend_priority=iptables,nftables
Checks have been added to prevent invalid values from being accepted.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 16 +++++++++------- meson_options.txt | 3 +-- src/network/bridge_driver_conf.c | 6 +++++- src/network/meson.build | 6 ++++-- src/network/network.conf.in | 13 +++++++------ 5 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/meson.build b/meson.build index f67c3d2724..ed0e9686f8 100644 --- a/meson.build +++ b/meson.build @@ -1635,15 +1635,17 @@ endif
if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD') conf.set('WITH_NETWORK', 1) - firewall_backend_default_1 = get_option('firewall_backend_default_1') - firewall_backend_default_conf = firewall_backend_default_1 - firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_1.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1)
- firewall_backend_default_2 = get_option('firewall_backend_default_2') - firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' + firewall_backend_default_2.to_upper() - conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2) + firewall_backend_priority = get_option('firewall_backend_priority') + if (not firewall_backend_priority.contains('nftables') or + not firewall_backend_priority.contains('iptables') or + firewall_backend_priority.length() != 2)
Okay, yeah. That does actually make sure that both possibilities are in the list, and (indirectly) that none is duplicated.
+ error('invalid value for firewall_backend_priority option') + endif
+ conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[0].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + firewall_backend_priority[1].to_upper()) + conf.set('FIREWALL_BACKEND_PRIORITY_NUM', firewall_backend_priority.length()) elif get_option('driver_network').enabled() error('libvirtd must be enabled to build the network driver') endif diff --git a/meson_options.txt b/meson_options.txt index ad354a8668..8723d13231 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value: 'auto', description: 'use dtrace for st option('firewalld', type: 'feature', value: 'auto', description: 'firewalld support') # dep:firewalld option('firewalld_zone', type: 'feature', value: 'auto', description: 'whether to install firewalld libvirt zone') -option('firewall_backend_default_1', type: 'string', value: 'nftables', description: 'first firewall backend to try when none is specified') -option('firewall_backend_default_2', type: 'string', value: 'iptables', description: 'second firewall backend to try when none is specified (and first is unavailable)') +option('firewall_backend_priority', type: 'array', choices: ['nftables', 'iptables'], description: 'firewall backends to try, preferred ones first')
Yes! This is the way I initially wanted to do it, but didn't have enough meson-foo to make it work.
option('host_validate', type: 'feature', value: 'auto', description: 'build virt-host-validate') option('init_script', type: 'combo', choices: ['systemd', 'openrc', 'check', 'none'], value: 'check', description: 'Style of init script to install') option('loader_nvram', type: 'string', value: '', description: 'Pass list of pairs of <loader>:<nvram> paths. Both pairs and list items are separated by a colon.') diff --git a/src/network/bridge_driver_conf.c b/src/network/bridge_driver_conf.c index 8f4956dace..e2f3613a41 100644 --- a/src/network/bridge_driver_conf.c +++ b/src/network/bridge_driver_conf.c @@ -67,8 +67,12 @@ virNetworkLoadDriverConfig(virNetworkDriverConfig *cfg G_GNUC_UNUSED, g_autofree char *fwBackendStr = NULL; bool fwBackendSelected = false; size_t i; - int fwBackends[] = { FIREWALL_BACKEND_DEFAULT_1, FIREWALL_BACKEND_DEFAULT_2 }; + int fwBackends[] = { + FIREWALL_BACKEND_PRIORITY_0, + FIREWALL_BACKEND_PRIORITY_1, + }; G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == VIR_FIREWALL_BACKEND_LAST); + G_STATIC_ASSERT(G_N_ELEMENTS(fwBackends) == FIREWALL_BACKEND_PRIORITY_NUM);
I had to think about this for a minute, since at first glance those both seem kind of the same. you want to continue checking that the backends priority list has an entry for every item, and also make sure that there wasn't an extra item in the priorities list given to the build.
int nFwBackends = G_N_ELEMENTS(fwBackends);
if (access(filename, R_OK) == 0) { diff --git a/src/network/meson.build b/src/network/meson.build index bf2893accc..07cd5cda55 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -51,7 +51,8 @@ if conf.has('WITH_NETWORK') }
network_options_conf = configuration_data({ - 'FIREWALL_BACKEND': firewall_backend_default_conf, + 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), + 'FIREWALL_BACKEND': firewall_backend_priority[0], })
network_conf = configure_file( @@ -61,7 +62,8 @@ if conf.has('WITH_NETWORK') )
network_options_hack_conf = configuration_data({ - 'FIREWALL_BACKEND': firewall_backend_default_conf, + 'FIREWALL_BACKEND_PRIORITY': ', '.join(firewall_backend_priority), + 'FIREWALL_BACKEND': firewall_backend_priority[0], # This hack is necessary because the output file is going to be # used as input for another configure_file() call later, which # will take care of substituting @CONFIG@ with useful data diff --git a/src/network/network.conf.in b/src/network/network.conf.in index f579f39fcd..5ed64a04a5 100644 --- a/src/network/network.conf.in +++ b/src/network/network.conf.in @@ -12,12 +12,13 @@ # iptables - use iptables commands to construct the firewall # nftables - use nft commands to construct the firewall # -# If firewall_backend isn't set in this file, libvirt will -# prefer the @FIREWALL_BACKEND@ backend *if the necessary package. -# binary is installed*, otherwise it will look for the package/binary -# needed for the other backend and use that if available. If neither -# is available on the host, then the network driver will fail to -# start, and an error will be logged. +# If firewall_backend isn't configured, libvirt will choose the +# first available backend from the following list: +# +# [@FIREWALL_BACKEND_PRIORITY@] +# +# If no backend is available on the host, then the network driver +# will fail to start, and an error will be logged. # # (NB: switching from one backend to another while there are active # virtual networks *is* supported. The change will take place the
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/meson.build b/meson.build index ed0e9686f8..e98ab0d5ac 100644 --- a/meson.build +++ b/meson.build @@ -2382,6 +2382,11 @@ misc_summary = { 'sysctl config': conf.has('WITH_SYSCTL'), 'userfaultfd sysctl': conf.has('WITH_USERFAULTFD_SYSCTL'), } +if conf.has('WITH_NETWORK') + misc_summary += { + 'firewall backends': firewall_backend_priority, + } +endif summary(misc_summary, section: 'Miscellaneous', bool_yn: true, list_sep: ' ') devtools_summary = { -- 2.45.1
On Tue, May 28, 2024 at 05:49:20PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 1 + 1 file changed, 1 insertion(+) diff --git a/libvirt.spec.in b/libvirt.spec.in index 6fb223c74a..4381dbe30c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1387,6 +1387,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) %{?enable_werror} \ -Dexpensive_tests=enabled \ -Dinit_script=systemd \ + -Dfirewall_backend_priority=nftables,iptables \ -Ddocs=enabled \ -Dtests=enabled \ -Drpath=disabled \ -- 2.45.1
On Tue, May 28, 2024 at 05:49:21PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Tue, May 28, 2024 at 05:49:21PM +0200, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- libvirt.spec.in | 1 + 1 file changed, 1 insertion(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 6fb223c74a..4381dbe30c 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1387,6 +1387,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) %{?enable_werror} \ -Dexpensive_tests=enabled \ -Dinit_script=systemd \ + -Dfirewall_backend_priority=nftables,iptables \
This change isn't right as it doesn't match earlier logic in the spec which is version specific %if 0%{?rhel} >= 10 || 0%{?fedora} >= 41 Requires: nftables %else Requires: iptables %endif ie, we should *not* be preferring nftables on existing RHEL and Fedora, only future, otherwise users will get a functional behaviour change on their existing distro. With 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 :|
On Mon, Jun 03, 2024 at 10:57:15AM GMT, Daniel P. Berrangé wrote:
On Tue, May 28, 2024 at 05:49:21PM +0200, Andrea Bolognani wrote:
+++ b/libvirt.spec.in @@ -1387,6 +1387,7 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/libvirt.spec) %{?enable_werror} \ -Dexpensive_tests=enabled \ -Dinit_script=systemd \ + -Dfirewall_backend_priority=nftables,iptables \
This change isn't right as it doesn't match earlier logic in the spec which is version specific
%if 0%{?rhel} >= 10 || 0%{?fedora} >= 41 Requires: nftables %else Requires: iptables %endif
ie, we should *not* be preferring nftables on existing RHEL and Fedora, only future, otherwise users will get a functional behaviour change on their existing distro.
You're right. I'll fix it. Unfortunately 10.4.0 has already been tagged :( -- Andrea Bolognani / Red Hat / Virtualization
On 5/28/24 11:49 AM, Andrea Bolognani wrote:
Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1308917840
See 1/3 for details.
Andrea Bolognani (3): meson: Improve default firewall backend configuration meson: Include firewall backend selection in summary rpm: Configure firewall backends explicitly
Reviewed-by: Laine Stump <laine@redhat.com> Thanks for doing this - I had put "make the backend default selection better" on a *mental* list, but not a physical list, and then I forgot (easy to forget because doing things in a meson build file is for me an exercise in poking at it until it works correctly). I've tried it out and it works as advertised. I would prefer to categorize this as a bugfix and push it before the release (so that we don't have the build-time config different in this release than in later releases)
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Laine Stump -
Pavel Hrdina