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

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

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.

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):
+    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

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>