The error message comes from QEMU:

qemu-kvm: -blockdev {"driver":"rbd","pool":"libvirt-pool","image":"rhel8.6-secret.img","server":[{"host":"10.66.144.57","port":"6789"}],"node-name":"libvirt-4-storage","cache":{"direct":false,"no-flush":true},"auto-read-only":true,"discard":"unmap"}: error connecting: Operation not supported [code=1 int1=-1]

Regarding the fact that it works with the direct backend: I think the QEMU command lines must be slightly different.

The QEMU command line should be in the libguestfs log when you use the direct backend, can you please upload that?

And when you use the libvirt backend, only the temporary domain XML is in the libguestfs log; the corresponding QEMU cmdline should be in the libvirtd log. Can you please upload that too? For comparison. Thanks.

Thanks for the logs.

With both the libvirt backend (comment#0) and the direct backend (comment#2), libguestfs invokes qemu-img to create an overlay qcow2, setting the "backing file" field in the qcow2 image to "rbd:libvirt-pool/rhel8.6-secret.img...". The properties are:

libvirt-pool/rhel8.6-secret.img
mon_host=10.66.144.57:6789
id=libvirt
auth_supported=cephx;none
key=AQDv2LpheIW2OhAASKiQ3x0Hz1Bpc3tdASjvcw==

Then, in the direct backend case (comment#2), libguestfs passes only one related option to QEMU, namely:

    -drive file=/tmp/libguestfssrNDCl/overlay1.qcow2,format=qcow2,cache=unsafe,id=hd0,if=none \

Consequently, the backing file on rbd is handled entirely internally to QEMU.

In the libvirt backend case, libguestfs again passes only one related XML element to libvirtd, namely:

    <disk device="disk" type="file">
      <source file="/tmp/libguestfsgIpdNX/overlay1.qcow2"/>
      <target dev="sda" bus="scsi"/>
      <driver name="qemu" type="qcow2" cache="unsafe"/>
      <address type="drive" controller="0" bus="0" target="0" unit="0"/>
    </disk>

(overlay2 is for the appliance root). *However*, libvirtd turns this into a multitude of -blockdev options for QEMU. First layer:

-blockdev '{"node-name":"libvirt-2-format","read-only":false,"cache":{"direct":false,"no-flush":true},"driver":"qcow2","file":"libvirt-2-storage","backing":"libvirt-4-format"}' \
-blockdev '{"driver":"file","filename":"/tmp/libguestfsJHYFSp/overlay1.qcow2","node-name":"libvirt-2-storage","cache":{"direct":false,"no-flush":true},"auto-read-only":true,"discard":"unmap"}' \

Second layer:

-blockdev '{"node-name":"libvirt-4-format","read-only":true,"cache":{"direct":false,"no-flush":true},"driver":"raw","file":"libvirt-4-storage"}' \
-blockdev '{"driver":"rbd","pool":"libvirt-pool","image":"rhel8.6-secret.img","server":[{"host":"10.66.144.57","port":"6789"}],"node-name":"libvirt-4-storage","cache":{"direct":false,"no-flush":true},"auto-read-only":true,"discard":"unmap"}' \

Thus (I think) libvirtd itself collects the backing file information first, and then passes that info, possibly with some modifications, to QEMU, explicitly. The rbd connection is thus not handled internally to QEMU.

Minimally, the "discard:unmap" part is unique to the libvirtd backend -- the "-drive" option from the direct backend does not have that!

I think this BZ should be further triaged by libvirt / qemu / rbd storage folks. Something fails in qemu_rbd_connect(), I think.

IIRC the problem will be that in cases where a secret is required to access a storage volume, it can't be passed in via teh backing storage string, but at that point you must enter it into the XML, and define the secret.

We

I wanted to add that it's also generally not recomended, and actually dangerous, to pass secrets on the commandline.

Correct me if I'm wrong, but I guess this is a new use case (using encrypted RBD volumes?) and not a regression? And in that case, does this really qualifies as an RFE to have the secret in the XML then (as opposed to the commnand-line)? Are there existing use-cases where the secret was passes through command-line arguments that we would need to revisit as well?

(In reply to Klaus Heinrich Kiwi from comment #11)
> Correct me if I'm wrong, but I guess this is a new use case (using encrypted
> RBD volumes?) and not a regression? And in that case, does this really
> qualifies as an RFE to have the secret in the XML then (as opposed to the
> commnand-line)? Are there existing use-cases where the secret was passes
> through command-line arguments that we would need to revisit as well?

It should be a regression. It's a manual test case for libguestfs component. But I don't know whether there are users or customers to use libguestfs to detect encrypted rbd images in this way (specify the secret in the command-line like comment 0). In general, we test the non-encrypted rbd image as high priority.

(In reply to Peter Krempa from comment #9)
> IIRC the problem will be that in cases where a secret is required to
> access a storage volume, it can't be passed in via teh backing storage
> string, but at that point you must enter it into the XML, and define
> the secret.

After hours of analysis, I must confirm that this is indeed the correct
explanation.

As of current upstream libvirt (@7b0e2e4a558d), the
virStorageSourceParseRBDColonString() function in file
"src/storage_file/storage_source_backingstore.c" is responsible for
parsing the RBD URL details from the backing chain. As a reminder from
comment 4, in the specific case, we have the following key-value list,
for "libvirt-pool/rhel8.6-secret.img":

> mon_host=10.66.144.57:6789
> id=libvirt
> auth_supported=cephx;none
> key=AQDv2LpheIW2OhAASKiQ3x0Hz1Bpc3tdASjvcw==

The function in question does *not* parse the "key" key at all. The only
key, related to authentication, that the function parses, is "id":

>         if (STRPREFIX(p, "id=")) {
>             /* formulate authdef for src->auth */
>             if (src->auth) {
>                 virReportError(VIR_ERR_INTERNAL_ERROR,
>                                _("duplicate 'id' found in '%s'"), src->path);
>                 return -1;
>             }
>
>             authdef = g_new0(virStorageAuthDef, 1);
>
>             authdef->username = g_strdup(p + strlen("id="));
>
>             authdef->secrettype = g_strdup(virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH));
>             src->auth = g_steal_pointer(&authdef);
>
>             /* Cannot formulate a secretType (eg, usage or uuid) given
>              * what is provided.
>              */
>         }

Compare this with the domain XML documentation
<https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms>:

>   <disk type='network'>
>     <driver name="qemu" type="raw"/>
>     <source protocol="rbd" name="image_name2">
>       <host name="hostname" port="7000"/>
>       <snapshot name="snapname"/>
>       <config file="/path/to/file"/>
>       <auth username='myuser'>
>         <secret type='ceph' usage='mypassid'/>
>       </auth>
>     </source>
>     <target dev="hdc" bus="ide"/>
>   </disk>

> auth
>
>     [Since libvirt 3.9.0] the auth element is supported for a disk
>     type "network" that is using a source element with the protocol
>     attributes "rbd" or "iscsi". If present, the auth element provides
>     the authentication credentials needed to access the source. It
>     includes a mandatory attribute username, which identifies the
>     username to use during authentication, as well as a sub-element
>     secret with mandatory attribute type, to tie back to a libvirt
>     secret object that holds the actual password or other credentials
>     (the domain XML intentionally does not expose the password, only
>     the reference to the object that does manage the password). Known
>     secret types are "ceph" for Ceph RBD network sources and "iscsi"
>     for CHAP authentication of iSCSI targets. Both will require either
>     a uuid attribute with the UUID of the secret object or a usage
>     attribute matching the key that was specified in the secret
>     object.

That is, libvirt represents the (userid, secret) pair via the <auth>
element:

[src/conf/storage_source_conf.h]

> typedef struct _virStorageAuthDef virStorageAuthDef;
> struct _virStorageAuthDef {
>     char *username;
>     char *secrettype; /* <secret type='%s' for disk source */
>     int authType;     /* virStorageAuthType */
>     virSecretLookupTypeDef seclookupdef;
> };

[src/conf/storage_source_conf.h]

> typedef enum {
>     VIR_STORAGE_AUTH_TYPE_NONE,
>     VIR_STORAGE_AUTH_TYPE_CHAP,
>     VIR_STORAGE_AUTH_TYPE_CEPHX,
>
>     VIR_STORAGE_AUTH_TYPE_LAST,
> } virStorageAuthType;

[src/util/virsecret.h]

> typedef struct _virSecretLookupTypeDef virSecretLookupTypeDef;
> struct _virSecretLookupTypeDef {
>     int type;   /* virSecretLookupType */
>     union {
>         unsigned char uuid[VIR_UUID_BUFLEN];
>         char *usage;
>     } u;
>
> };

and the virStorageSourceParseRBDColonString() can only fill in the
"username" and "secrettype" fields from the RBD key-value string, it
*cannot* fill in the "authType" and "seclookupdef" fields from the
key-value string. For that, it would have to create a separate secret
object somehow.

(The comment quoted higher up refers to the field name incorrectly -- it
says "Cannot formulate a secretType (eg, usage or uuid) given what is
provided", but what it means is "seclookupdef".)

Now, looking a bit at the libvirt git history, a very relevant commit is
6887af392cbb ("Utilize virDomainDiskAuth for domain disk", 2014-07-03).
That's when the current "virStorageAuthDef" structure was first
introduced -- except back then it was called "virDomainDiskAuth". That's
when the RBD key-value string parsing was also modified to produce this
central authentication structure, and that's when the comment in
question was added, too:

> @@ -2619,9 +2620,24 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
>              *e = '\0';
>          }
>
> -        if (STRPREFIX(p, "id=") &&
> -            VIR_STRDUP(disk->src->auth.username, p + strlen("id=")) < 0)
> -            goto error;
> +        if (STRPREFIX(p, "id=")) {
> +            const char *secrettype;
> +            /* formulate authdef for disk->src->auth */
> +            if (VIR_ALLOC(authdef) < 0)
> +                goto error;
> +
> +            if (VIR_STRDUP(authdef->username, p + strlen("id=")) < 0)
> +                goto error;
> +            secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH);
> +            if (VIR_STRDUP(authdef->secrettype, secrettype) < 0)
> +                goto error;
> +            disk->src->auth = authdef;
> +            authdef = NULL;
> +
> +            /* Cannot formulate a secretType (eg, usage or uuid) given
> +             * what is provided.
> +             */
> +        }
>          if (STRPREFIX(p, "mon_host=")) {
>              char *h, *sep;

In other words, when parsing RBD key-value strings from the backing
chain, libvirt has *never* parsed the "key" entry. Over time it has
formalized and changed the authentication structure, and moved the
comment in question around (quite many times in fact), but this use case
was never supported by it.

So here's what I think:

- We prepare a valid qcow2 overlay with a backing file specification,
  but that specification is unsupported by libvirtd. As long as we use
  an overlay at all, the problem is unfixable in libguestfs.

- Not a regression in libvirtd -- this never worked in libvirtd, as much
  as I can tell.

- If it is *supposedly* regression in libguestfs, then please precisely
  identify the version in which this use case (with the libvirtd
  backend) last *worked* [NEEDINFO]. Perhaps, at that time, we did not
  prepare an overlay at all.

  Note: the guestfish command in comment 6 works even with the libvirtd
  backend because no overlay is created there! But if you retry it with
  the "--ro" flag, an overlay will be created, and I expect that it will
  show the same symptom.

  https://libguestfs.org/guestfish.1.html#add-drive-opts

> readonly
>
>     If true then the image is treated as read-only. Writes are still
>     allowed, but they are stored in a temporary snapshot overlay which
>     is discarded at the end. The disk that you add is not modified.

I think the best we can do here is update the documentation. Example:

  https://libguestfs.org/guestfs.3.html#network-block-device

This section already has Notes, which list various backend-specific
limitations:

> * The libvirt backend requires that you set the format parameter of
>   "guestfs_add_drive_opts" accurately when you use writable NBD disks.
>
> * The libvirt backend has a bug that stops Unix domain socket
>   connections from working:
>   https://bugzilla.redhat.com/show_bug.cgi?id=922888
>
> * The direct backend does not support readonly connections because of
>   a bug in qemu: https://bugs.launchpad.net/qemu/+bug/1155677

A sibling section there is about Ceph:

  https://libguestfs.org/guestfs.3.html#ceph

We should add a similar Note there, explaining that an encrypted RBD
disk does not work with the libvirt backend *IF* it is specified as the
backing file of a QCOW2 image. With this particular scope (and this
scope only), i.e. for updating the documentation, I'm taking the bug.

libguestfs commit 6d6644d52d80 ("launch: libvirt: Implement drive secrets (RHBZ#1159016).", 2014-10-31) is what makes the guestfish command in comment 6 work -- but, again, that's entirely different. In that case, we don't have an overlay, and libguestfs explicitly prepares secrets for libvirtd in advance.

In the problematic case, libvirtd itself parses the RBD params from the "backing file" field of the QCOW2 image, and it does not create a (temporary) secret for itself, from the "key" RBD param.

(We create overlays ultimately with disk_create_qcow2() in libguestfs [lib/create.c], which invokes qemu-img.)

[libguestfs PATCH] guestfs.pod: document encrypted RBD disk limitation
Message-Id: <20220518083014.9890-1-lersek>
https://listman.redhat.com/archives/libguestfs/2022-May/028903.html

Also, since there's been no answer to my NEEDINFO request in comment 13, I'm removing the Regression keyword.

(In reply to Laszlo Ersek from comment #16)
> [libguestfs PATCH] guestfs.pod: document encrypted RBD disk limitation
> Message-Id: <20220518083014.9890-1-lersek>
> https://listman.redhat.com/archives/libguestfs/2022-May/028903.html
> 
> Also, since there's been no answer to my NEEDINFO request in comment 13, I'm
> removing the Regression keyword.

Sorry for the late response. Documenting the encrypted RBD disk limitation in the man page is ok for me.

(In reply to Laszlo Ersek from comment #16)
> [libguestfs PATCH] guestfs.pod: document encrypted RBD disk limitation
> Message-Id: <20220518083014.9890-1-lersek>
> https://listman.redhat.com/archives/libguestfs/2022-May/028903.html

Upstream commit 544bb0ff5079 ("guestfs.pod: document encrypted RBD disk limitation", 2022-05-19).

Verified with libguestfs-1.48.3-1.el9.x86_64:

Steps:

1. On rhel9.1 host

$ man 3 guestfs 

An encrypted RBD disk -- directly opening which would require the "username" and "secret" parameters -- cannot be accessed if the following conditions all hold:
•   the backend is libvirt,

•   the image specified by the "filename" parameter is different from the encrypted RBD disk,

•   the image specified by the "filename" parameter has qcow2 format,

•   the encrypted RBD disk is specified as a backing file at some level in the qcow2 backing chain.

This limitation is due to libvirt's (justified) separate handling of disks vs. secrets.  When the RBD username and secret are provided inside a qcow2 backing file specification, libvirt does not construct an ephemeral secret object from those, for Ceph authentication.  Refer to https://bugzilla.redhat.com/2033247.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Low: libguestfs security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2022:7958