[Libguestfs] [PATCH v2v v2] -o rhv-upload: Improve error message for invalid or missing -os parameter

[Nir Soffer]

On Fri, Jan 27, 2023 at 2:26 PM Richard W.M. Jones <rjones at redhat.com> wrote:
>
> For -o rhv-upload, the -os parameter specifies the storage domain.
> Because the RHV API allows globs when searching for a domain, if you
> used a parameter like -os 'data*' then this would confuse the Python
> code, since it can glob to the name of a storage domain, but then
> later fail when we try to exact match the storage domain we found.
> The result of this was a confusing error in the precheck script:
>
>   IndexError: list index out of range
>
> This fix validates the output storage parameter before trying to use
> it.  Since valid storage domain names cannot contain glob characters
> or spaces, it avoids the problems above and improves the error message
> that the user sees:
>
>   $ virt-v2v [...] -o rhv-upload -os ''
>   ...
>   RuntimeError: The storage domain (-os) parameter ‘’ is not valid
>   virt-v2v: error: failed server prechecks, see earlier errors
>
>   $ virt-v2v [...] -o rhv-upload -os 'data*'
>   ...
>   RuntimeError: The storage domain (-os) parameter ‘data*’ is not valid
>   virt-v2v: error: failed server prechecks, see earlier errors
>

Makes sense, the new errors are very helpful.

> Although the IndexError should no longer happen, I also added a
> try...catch around that code to improve the error in case it still
> happens.

Theoretically it can happen if the admin changes the storage domain
name or detaches the domain from the data center in the window
after the precheck completes and before the transfer starts.

>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1986386
> Reported-by: Junqin Zhou
> Thanks: Nir Soffer
> ---
>  output/rhv-upload-precheck.py | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/output/rhv-upload-precheck.py b/output/rhv-upload-precheck.py
> index 1dc1b8498a..ba125611ba 100644
> --- a/output/rhv-upload-precheck.py
> +++ b/output/rhv-upload-precheck.py
> @@ -18,6 +18,7 @@
>
>  import json
>  import logging
> +import re
>  import sys
>
>  from urllib.parse import urlparse
> @@ -46,6 +47,15 @@ output_password = output_password.rstrip()
>  parsed = urlparse(params['output_conn'])
>  username = parsed.username or "admin at internal"
>
> +# Check the storage domain name is valid
> +# (https://bugzilla.redhat.com/show_bug.cgi?id=1986386#c1)
> +# Also this means it cannot contain spaces or glob symbols, so
> +# the search below is valid.
> +output_storage = params['output_storage']
> +if not re.match('^[-a-zA-Z0-9_]+$', output_storage):

The comment in the bug does not point to the docs or code enforcing
the domain name restrictions, but I validated with ovirt 4.5. Trying to
create a domain name with a space or using Hebrew characters is blocked
in the UI, displaying an error. See attached screenshots.

I think it is highly unlikely that this limit will change in the
future since nobody
is working on oVirt now, but if it does change this may prevent uploading to an
existing storage domain.

> +    raise RuntimeError("The storage domain (-os) parameter ‘%s’ is not valid" %
> +                       output_storage)
> +
>  # Connect to the server.
>  connection = sdk.Connection(
>      url=params['output_conn'],
> @@ -60,28 +70,33 @@ system_service = connection.system_service()
>
>  # Check whether there is a datacenter for the specified storage.
>  data_centers = system_service.data_centers_service().list(
> -    search='storage.name=%s' % params['output_storage'],
> +    search='storage.name=%s' % output_storage,
>      case_sensitive=True,
>  )
>  if len(data_centers) == 0:
>      storage_domains = system_service.storage_domains_service().list(
> -        search='name=%s' % params['output_storage'],
> +        search='name=%s' % output_storage,
>          case_sensitive=True,
>      )
>      if len(storage_domains) == 0:
>          # The storage domain does not even exist.
>          raise RuntimeError("The storage domain ‘%s’ does not exist" %
> -                           (params['output_storage']))
> +                           output_storage)
>
>      # The storage domain is not attached to a datacenter
>      # (shouldn't happen, would fail on disk creation).
>      raise RuntimeError("The storage domain ‘%s’ is not attached to a DC" %
> -                       (params['output_storage']))
> +                       output_storage)
>  datacenter = data_centers[0]
>
>  # Get the storage domain.
>  storage_domains = connection.follow_link(datacenter.storage_domains)
> -storage_domain = [sd for sd in storage_domains if sd.name == params['output_storage']][0]
> +try:
> +    storage_domain = [sd for sd in storage_domains
> +                          if sd.name == output_storage][0]
> +except IndexError:
> +    raise RuntimeError("The storage domain ‘%s’ does not exist" %
> +                       output_storage)
>
>  # Get the cluster.
>  clusters = connection.follow_link(datacenter.clusters)
> @@ -90,7 +105,7 @@ if len(clusters) == 0:
>      raise RuntimeError("The cluster ‘%s’ is not part of the DC ‘%s’, "
>                         "where the storage domain ‘%s’ is" %
>                         (params['rhv_cluster'], datacenter.name,
> -                        params['output_storage']))
> +                        output_storage))
>  cluster = clusters[0]
>  cpu = cluster.cpu
>  if cpu.architecture == types.Architecture.UNDEFINED:
> --
> 2.39.0
>

Reviewed-by: Nir Soffer <nsoffer at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Screenshot from 2023-01-28 18-07-52.png
Type: image/png
Size: 12594 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20230128/6ee789e2/attachment.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Screenshot from 2023-01-28 18-07-24.png
Type: image/png
Size: 13496 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20230128/6ee789e2/attachment-0001.png>