On 4/19/22 9:05 AM, Daniel P. Berrangé wrote:
On Thu, Apr 14, 2022 at 05:02:46PM -0500, Jonathon Jongsma wrote:
> As mentioned in
https://bugzilla.redhat.com/show_bug.cgi?id=2016527, RHEL is
> planning to remove dependencies on the qemu-block-curl and qemu-block-ssh
> plugins from the main qemu package. This creates issues for libvirt for
> supporting network disk sources. So I've been looking into using nbdkit from
> libvirt to proxy these network disks to qemu as NBD disks.
>
> The basic idea is that libvirt will spin up an nbdkit instance for e.g. an
> https network disk source, and will provide the resulting unix socket to
> qemu as an nbd disk. This allows libvirt to continue supporting http/ftp/ssh
> disk sources regardless of whether the qemu block plugins are installed or
> not.
>
> However, there are a couple of issues and feature gaps that I've run into
> that I'd like to discuss.
>
> 1. secrets
>
> There is some code in libvirt[1] which seems to expect that it is possible
> for http(s) disk sources to have a username and password specified. However,
> I can't find any valid xml schema for specifying an http username and
> password, and my reading of the code suggests that there shouldn't be any
> way for these to be set for http(s)/ftp(s) [disk sources either since auth is
> only supported for ISCSI and RBD protocols [2]. Am I missing something?
>
> [1]
https://gitlab.com/libvirt/libvirt/-/blob/6be7beb3bdb9ad611a5598dad7edfbd...
>
> [2]
https://gitlab.com/libvirt/libvirt/-/blob/6be7beb3bdb9ad611a5598dad7edfbd...
I'm unclear if this is a regression, or a long term bug. Clearly the
intention of the code is to support auth, so if it doesn't work right
now we need to fix that, regardless of adding the nbdkit support.
I do notice we don't have any XML in tests/qemuxml2argvdata/ that
exercises auth with https, which is likely why we have this bug
lurking.
Well, As far as I can tell, there is no valid XML for exercising http
auth. The schema for http(s) sources does not include any <auth> element
[1]. And the schema for the <auth> element [2] requires a <secret>
element with a required 'type' attribute, and the only choices for
'type' are 'ceph' or 'iscsi', neither of which apply to http
authentication.
So it looks to me like http auth was never supported, rather than a
regression. It also seems that it would require some additional schema
changes to support this.
[1]
https://gitlab.com/libvirt/libvirt/-/blob/136b821f183deb0b58c571211f69179...
[
2]https://gitlab.com/libvirt/libvirt/-/blob/136b821f183deb0b58c571211f691...
> If it *is* possible for the username/password to be set for these disks,
> then we have the issue that these sensitive pieces of data have so far been
> passed as encrypted data to qemu using qemu secrets. But if nbdkit is
> handling the http requests, we need to pass this data to nbdkit rather than
> qemu and so we can no longer use qemu secrets. The same issue applies to
> http cookies, which could potentially include security-sensitive data such
> as login credentials, etc.
>
> Fortunately, nbdkit provides a method for reading cookies and passwords from
> a file, which should be secure if the file has permissions set properly. So
> I'm currently planning to write a file containing the cookies and pass them
> to nbdkit by specifying the filename. But I'm still confused about the
> username/password possibility.
>
> 2. readahead
>
> The libvirt xml format allows to specify a readahead size for disks that are
> handled by the qemu-block-curl plugin. Unfortunately, nbdkit doesn't
> currently support any readahead configuration. In nbdkit, readahead is
> handled by an nbkit "filter" that takes no configuration options (`nbdkit
> --filter=readahead ...`). In theory, this filter tries to adaptively read
> ahead. But when I discussed it with Rich, he suggested that he had stopped
> using it in virt-v2v because it was causing more trouble than it was worth.
> He also suggested that this readahead filter might need a complete rewrite,
> and presumably the rewrite could include the ability to configure a
> readahead buffer size. But I'm not sure what the timeframe might be for
> that.
In terms of libvirt upstream policy, there is no requirement for a given
hypervisor implementation to support all the features possible in the XML.
If readahead isn't supported then just report VIR_ERR_CONFIG_UNSUPPORTED
if the user provides it in the XML.
If there is a choice of backends though, we should prefer the impl that
avoids regressions.
IOW, if both qemu-block-curl and nbdkit are installed, we must prefer
qemu-block-curl.
It is upto RHEL downstream if they are happy to ship only nbdkit given
the feature differences.
> 3. blockdev-create
>
> There is support in libvirt[3] for creating ssh network disks by sending a
> 'blockdev-create' command to qemu. If qemu is no longer handling ssh network
> disk sources directly, this feature becomes significantly more complicated.
> I don't yet know enough about this part of the libvirt code to know what
> further complications might pop up here. From my reading of the code, this
> is mostly triggered by things like `virsh blockcopy` `virsh backup-begin`,
> etc. But nbdkit cannot currently do this. Rich pointed me to a recent
> commit[4] where he added disk creation to the nbdkit vddk plugin, and
> suggested that something similar could be added for the nbdkit ssh plugin.
>
> [3]
https://gitlab.com/libvirt/libvirt/-/blob/6be7beb3bdb9ad611a5598dad7edfbd...
>
> [4]
https://gitlab.com/nbdkit/nbdkit/-/commit/a39d5773afc3ebab7e5768118a2bccb...
>
>
> It seems to me that it's essential that we resolve #3 before we can move
> forward with nbdkit support in libvirt. (Although I admit that I have no
> idea how common it is for people to use ssh disks so I suppose there's a
> slim possibility that we could just disable the 'create disk' feature for
> ssh disks without any practical loss of functionality?) But it's less
> obvious to me whether we could move ahead despite missing readahead size
> configuration, etc.
Again, from an upstream POV #3 isn't a blocker, as long as we prefer
qemu-block-curl when available. It is upto RHEL to decide whether
#3 is a blocker for their downstream dropping of qemu-block-curl.
And now I notice that we do not actually have support for 'ssh' network
disks in our xml schema either [3]. The VIR_STORAGE_NET_PROTOCOL_SSH
enum value was originally added with a comment that it allows using the
ssh protocol in backing chains. I've also seen some commits indicating
that some of the support for ssh disk sources may be related to
libguestfs usage in some way. cc'ing Rich and Peter in case they can add
any historical context here.
[3]
https://gitlab.com/libvirt/libvirt/-/blob/136b821f183deb0b58c571211f69179...