Currently when we build with nbdkit support, libvirt will always try to
use nbdkit to access remote disk sources when it is available. But
without an up-to-date selinux policy allowing this, it will fail.
Because the required selinux policies are not yet widely available, we
have disabled nbdkit support on rpm builds for all distributions before
Fedora 40.

Unfortunately, this makes it more difficult to test nbdkit support.
After someone updates to the necessary selinux policies, they would also
need to rebuild libvirt to enable nbdkit support. By introducing a
configure option (storage_use_nbdkit), we can build packages with nbdkit
support but have it disabled by default.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---

Suggested as an option for making testing easier by Andrea Bolognani

 libvirt.spec.in            | 10 +---------
 src/qemu/libvirtd_qemu.aug |  3 +++
 src/qemu/qemu.conf.in      | 10 ++++++++++
 src/qemu/qemu_conf.c       | 14 ++++++++++++++
 src/qemu/qemu_conf.h       |  2 ++
 src/qemu/qemu_domain.c     |  3 +++
 tests/qemuxml2argvtest.c   | 15 +++++++++------
 7 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f50c451e73..e2ba245ade 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -174,16 +174,8 @@
     %endif
 %endif
 
-# We should only enable nbdkit support if the OS ships a SELinux policy that
-# allows libvirt to launch it. Right now that's not the case anywhere, but
-# things should be fine by the time Fedora 40 is released.
-#
-# TODO: add RHEL 9 once a minor release that contains the necessary SELinux
-#       bits exists (we only support the most recent minor release)
 %if %{with_qemu}
-    %if 0%{?fedora} >= 40
-        %define with_nbdkit 0%{!?_without_nbdkit:1}
-    %endif
+    %define with_nbdkit 0%{!?_without_nbdkit:1}
 %endif
 
 %ifarch %{arches_dmidecode}
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ed097ea3d9..43485b43fb 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -147,6 +147,8 @@ module Libvirtd_qemu =
 
    let capability_filters_entry = str_array_entry "capability_filters"
 
+   let storage_entry = bool_entry "storage_use_nbdkit"
+
    (* Each entry in the config is one of the following ... *)
    let entry = default_tls_entry
              | vnc_entry
@@ -170,6 +172,7 @@ module Libvirtd_qemu =
              | nbd_entry
              | swtpm_entry
              | capability_filters_entry
+             | storage_entry
              | obsolete_entry
 
    let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index 6897e0f760..1017edefa5 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -974,3 +974,13 @@
 # "full"    -  both QEMU and its helper processes are placed into separate
 #              scheduling group
 #sched_core = "none"
+
+# Using nbdkit to access remote disk sources
+#
+# If this is set then libvirt will use nbdkit to access remote disk sources
+# when available. nbdkit will export an NBD share to qemu rather than having
+# qemu attempt access the remote server directly.
+#
+# Possible values are 0 or 1. Disabled by default.
+#
+# storage_use_nbdkit = 1
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 513b5ebb1e..b5c0ca10b4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1065,6 +1065,17 @@ virQEMUDriverConfigLoadCapsFiltersEntry(virQEMUDriverConfig *cfg,
 }
 
 
+static int
+virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
+                                    virConf *conf)
+{
+    if (virConfGetValueBool(conf, "storage_use_nbdkit", &cfg->storageUseNbdkit) < 0)
+        return -1;
+
+    return 0;
+}
+
+
 int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
                                 const char *filename,
                                 bool privileged)
@@ -1136,6 +1147,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
     if (virQEMUDriverConfigLoadCapsFiltersEntry(cfg, conf) < 0)
         return -1;
 
+    if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0)
+        return -1;
+
     return 0;
 }
 
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 1a3ba3a0fb..36049b4bfa 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -230,6 +230,8 @@ struct _virQEMUDriverConfig {
 
     char *deprecationBehavior;
 
+    bool storageUseNbdkit;
+
     virQEMUSchedCore schedCore;
 };
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ae19ce884b..f8dda6c898 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10333,6 +10333,9 @@ qemuDomainPrepareStorageSourceNbdkit(virStorageSource *src,
 {
     g_autoptr(qemuNbdkitCaps) nbdkit = NULL;
 
+    if (!cfg->storageUseNbdkit)
+        return false;
+
     if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK)
         return false;
 
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 4fda68a4ce..3c64fcc7eb 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1115,7 +1115,6 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-cdrom-empty-network-invalid");
     DO_TEST_CAPS_LATEST("disk-cdrom-bus-other");
     DO_TEST_CAPS_LATEST("disk-cdrom-network");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-cdrom-network-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     DO_TEST_CAPS_LATEST("disk-cdrom-tray");
     DO_TEST_CAPS_LATEST("disk-floppy");
     DO_TEST_CAPS_LATEST("disk-floppy-q35");
@@ -1161,8 +1160,6 @@ mymain(void)
     DO_TEST_CAPS_VER("disk-network-sheepdog", "6.0.0");
     DO_TEST_CAPS_LATEST("disk-network-source-auth");
     DO_TEST_CAPS_LATEST("disk-network-source-curl");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit-backing", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     DO_TEST_CAPS_LATEST("disk-network-nfs");
     driver.config->vxhsTLS = 1;
     driver.config->nbdTLSx509secretUUID = g_strdup("6fd3f62d-9fe7-4a4e-a869-7acd6376d8ea");
@@ -1173,13 +1170,10 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-network-tlsx509-nbd-hostname");
     DO_TEST_CAPS_VER("disk-network-tlsx509-vxhs", "5.0.0");
     DO_TEST_CAPS_LATEST("disk-network-http");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-http-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
     VIR_FREE(driver.config->nbdTLSx509secretUUID);
     VIR_FREE(driver.config->vxhsTLSx509secretUUID);
     driver.config->vxhsTLS = 0;
     DO_TEST_CAPS_LATEST("disk-network-ssh");
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
-    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
     DO_TEST_CAPS_LATEST("disk-no-boot");
     DO_TEST_CAPS_LATEST("disk-nvme");
     DO_TEST_CAPS_VER("disk-vhostuser-numa", "4.2.0");
@@ -1249,6 +1243,15 @@ mymain(void)
     DO_TEST_CAPS_LATEST("disk-geometry");
     DO_TEST_CAPS_LATEST("disk-blockio");
 
+    driver.config->storageUseNbdkit = 1;
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-cdrom-network-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-source-curl-nbdkit-backing", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-http-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_CURL);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-nbdkit", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
+    DO_TEST_CAPS_LATEST_NBDKIT("disk-network-ssh-password", QEMU_NBDKIT_CAPS_PLUGIN_SSH);
+    driver.config->storageUseNbdkit = 0;
+
     DO_TEST_CAPS_VER("disk-virtio-scsi-reservations", "5.2.0");
     DO_TEST_CAPS_LATEST("disk-virtio-scsi-reservations");
 
-- 
2.41.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

On Wed, Nov 08, 2023 at 03:39:22PM -0600, Jonathon Jongsma wrote:
> Currently when we build with nbdkit support, libvirt will always try to
> use nbdkit to access remote disk sources when it is available. But
> without an up-to-date selinux policy allowing this, it will fail.
> Because the required selinux policies are not yet widely available, we
> have disabled nbdkit support on rpm builds for all distributions before
> Fedora 40.
>
> Unfortunately, this makes it more difficult to test nbdkit support.
> After someone updates to the necessary selinux policies, they would also
> need to rebuild libvirt to enable nbdkit support. By introducing a
> configure option (storage_use_nbdkit), we can build packages with nbdkit
> support but have it disabled by default.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>
> Suggested as an option for making testing easier by Andrea Bolognani

This is what the Suggested-by tag exists for ;)

Anyway, this looks reasonable. In addition to making life easier for
those testing the SELinux (and AppArmor!) policy changes during this
transitional period, making it possible for the local admin to opt
out of nbdkit usage sounds like it could be useful in at least some
scenarios.

The main concern I have is how this will be handled for upgrades. At
some point we want to make nbdkit the default, right? But that would
mean changing how existing installations behave. I guess that is fine
in this case? Performing the switch transparently was always the plan
after all... I still feel a tiny bit uneasy about that though.

> +++ b/libvirt.spec.in
> -# We should only enable nbdkit support if the OS ships a SELinux policy that
> -# allows libvirt to launch it. Right now that's not the case anywhere, but
> -# things should be fine by the time Fedora 40 is released.
> -#
> -# TODO: add RHEL 9 once a minor release that contains the necessary SELinux
> -#       bits exists (we only support the most recent minor release)
>  %if %{with_qemu}
> -    %if 0%{?fedora} >= 40
> -        %define with_nbdkit 0%{!?_without_nbdkit:1}
> -    %endif
> +    %define with_nbdkit 0%{!?_without_nbdkit:1}
>  %endif

Once the updated SELinux policy hits Fedora 40 (if it hasn't already)
we're going to want nbdkit support to be enabled by default (not just
available) there, no?

So I think we need something along the lines of

  %if %{with_qemu}
    %define with_nbdkit 0%{!?_without_nbdkit:1}
    %if 0%{?fedora} >= 40 # and later RHEL 9 too
      %define with_nbdkit_enabled 0%{!?_without_nbdkit_enabled:1}
    %endif
  %endif

> +++ b/src/qemu/qemu.conf.in
> +# Using nbdkit to access remote disk sources
> +#
> +# If this is set then libvirt will use nbdkit to access remote disk sources
> +# when available. nbdkit will export an NBD share to qemu rather than having
> +# qemu attempt access the remote server directly.
> +#
> +# Possible values are 0 or 1. Disabled by default.
> +#
> +# storage_use_nbdkit = 1

... and then to reflect the actual compile-time default here. There's
precedent for that, see @QEMU_USER@ and @QEMU_GROUP@.

Also s/qemu/QEMU/g in the comment :)

> +++ b/src/qemu/qemu_conf.h
> @@ -230,6 +230,8 @@ struct _virQEMUDriverConfig {
> +    bool storageUseNbdkit;

This works fine as long as the default is false, but in order to make
it possible to change it via a compile-time knob
virQEMUDriverConfigNew() will need to be updated too.

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org