Issue 90 Further Clarifications

[Peter Krempa]

On Mon, Nov 23, 2020 at 17:17:15 -0600, Dustan B Helm wrote:
> On Mon, Nov 23, 2020 at 2:33 AM Peter Krempa <pkrempa at redhat.com> wrote:
> 
> > On Sat, Nov 21, 2020 at 11:20:57 -0600, Dustan B Helm wrote:
> > > We plan to support NFS protocol according to the example XML from Issue
> > 90
> > > <http://gitlab.com/libvirt/libvirt/-/issues/90>. Since there is already
> > > support for network disks of different protocol types and host
> > information,
> > > we think that the only new XML information we will add is an <nfs>
> > element
> > > which will be a subelement of <source>, with attributes “user” and
> > “group”
> > > (both strings). This element will only be generated if the source
> > protocol
> > > is “nfs” and we assume that both “user” and “group” will be required.
> > >
> > > Here is the XML example given in the issue for reference:
> > >
> > > <disk type='network' device='disk'>
> > >
> > >   <driver name='qemu' type='raw'/>
> > >
> > >   <source protocol='nfs' name='PATH'>
> > >
> > >     <host name='example.com' port='2049'/
> > >
> > >     <nfs user='USER' group='GROUP'/>
> > >
> > >   </source>
> > >
> > >   <target dev='vda' bus='virtio'/>
> > >
> > > </disk>
> >
> > Sounds reasonable to me. We tend to name elements equivalent to <nfs>
> > you propose by their purpose (such as <auth> <initiator> <cookies> for
> > other protocols) but in this case I don't have a better suggestion so
> > going with <nfs> is reasonable.
> >
> > Since you are proposing 'user' and 'group' to be strings while qemu
> > accepts only numeric UID/GID, please use the same conversion code we
> > have for the <inituser> and <initgroup> values in regards to forcing
> > numeric value to skip being interpreded:
> >
> > https://www.libvirt.org/formatdomain.html#container-boot
> >
> > The linked documentation
> <https://www.libvirt.org/formatdomain.html#container-boot> suggests that
> providing a “+” prefix will force attribute data to be interpreted
> numerically. Since QEMU requires that the group and user attributes be
> numeric id values, would we want to simply insert a “+” prefix to these
> attributes when they are provided explicitly (instead of using the
> hypervisor default values, which we assume will just be numeric data)?

No, not at all. QEMU requires you to provide an integer according to the
schema. That means that for any '+'-prefixed value, you strip the + and
convert it to int. For a username you look it up in the system to get
the uid and use that. We have helpers for this, but I can't recall the
name. You'll have to do some digging.

> > What do you think of these proposed changes? Should either of the <nfs>
> > > tag's string attributes be optional?
> >
> > In this case qemu doesn't mandate the use of the user/group field so you
> > can make the nfs element and both user and group optional especially
> > since it's only a workaround for the broken-by design NFS "security".
> >
> > You can claim that a hypervisor-default uid/gid is used when the fields
> > are not present.
> >
> > We have assumed that because “group” and “user” are optional, they are not
> mutually required- that is, one or the other may be provided explicitly, in
> which case the other is assumed to be the hypervisor-default. Is this
> correct?

Yes.

> Additionally, when both default values for the hypervisor are to be used
> (that is, there is no explicitly given nfs-user or nfs-group attribute),
> would we simply remove the entire <nfs> tag as it would be empty? Or is it
> still necessary in order to bypass the “broken by design” NFS security?

Yes, you want to format the element only when any of the two attributes
are present. This is simple to achieve, our XML formatting helper
(virXMLFormatElement) does the correct thing if no attributes and no
subelements are passed for formatting.

> >  You also probably want to mention in the documentation that in most
> > cases qemu is running as non-root and thus doesn't have access to
> > privileged ports. Thus the export has to use the 'insecure' option to
> > allow non-privileged ports.
> >
> > One further thing possibly worth mentioning is that the name=''
> > attribute starts with the NFS export name.
> >
> > Does the NFS export requiring the “insecure” option in most cases have
> implications on the code we add to support the protocol beyond
> documentation of the usage semantics? Also, what specifically is referred

No. I just wanted that you point out in the docs that the NFS client
used in qemu has no access to privileged ports in the general use case
and thus may have implications on what is used. We tend to use a vague
terminology such as. "Note that hypervisors may not be able to use
privileged ports to access the NFS export and thus may require to use
the ``insecure`` option for the export to allow access"

> to by the “NFS export”- is this simply the directory containing all the
> files/folders to be shared across the NFS (i.e. etc/exports/), or some
> representation thereof in memory?

NFS export is one exported directory, thus maps to one line in
/etc/exports. Since there are multiple exports possible on a host you
have to use it when addressing the specific file.

Example:

Assume we have the following exports (leftmost column of /etc/exports)

/some/export *:...
/foo/bar/baz *:...

And 'ls /foo/bar/baz' on the NFS server:

image.qcow2
image2.qcow2

So in your proposal, if you want to point to 'image2.qcow2' you'd use:

<disk type='network' device='disk'>
  <driver name='qemu' type='qcow2'/>
  <source protocol='nfs' name='/foo/bar/baz/image.qcow2'>
    <host name='example.com' port='2049'/
    <nfs user='+123' group='wheel'/>
  </source>
  <target dev='vda' bus='virtio'/>
</disk>

Here the export name is part of the "path" in the `name` attribute.

An alternative is to expose 'export' as a separate attribute:

<disk type='network' device='disk'>
  <driver name='qemu' type='qcow2'/>
  <source protocol='nfs' name='image.qcow2'>
    <host name='example.com' port='2049'/
    <nfs export='/foo/bar/baz' user='+123' group='wheel'/>
  </source>
  <target dev='vda' bus='virtio'/>
</disk>

Here export is stored explicitly. Note that qemu takes the export
concatenated with the filename as argument so you'd have to concatenate
them when formatting the JSON struct representing the backend of the
block device.

I don't have a preference for either of them, but I'd like you to
document that bit. Specifically we have very similar documentation for
other protocols such as iSCSI, etc. which also store specifics in the
name filed.