Libguestfs - Jan 2020 - Re: [PATCH v2v v2 1/2] rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)

My stab v2 at fixing this: https://bugzilla.redhat.com/show_bug.cgi?id=1789279

It took me quite some time to go through the whole rfc 4122 just to realize we
do not need to do anything with the versions.

v2:
 - Use EEXIST instead of EINVAL
 - Put the colliding UUID into the error
 - Do not evaluate the PCRE needlessly multiple times
v1:
 https://www.redhat.com/archives/libguestfs/2020-January/msg00184.html


Martin Kletzander (2):
  rhv-upload: Validate UUIDs passed to -oo rhv-disk-uuid (RHBZ#1789279)
  rhv-upload: Check that rhv-disk-uuid is not already taken
    (RHBZ#1789279)

 v2v/output_rhv_upload.ml | 12 ++++++++++++
 v2v/rhv-upload-plugin.py | 12 ++++++++++++
 2 files changed, 24 insertions(+)

-- 
2.25.0

The validation helps us fail early and with a sensible error message.  The NIL
UUID is not valid for oVirt, but other than that there is no other logic in
there merely because the UUID types are a matter of the generator and they are
just forwarded in this partucular case.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 v2v/output_rhv_upload.ml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
index 14153db36897..6482460f8de8 100644
--- a/v2v/output_rhv_upload.ml
+++ b/v2v/output_rhv_upload.ml
@@ -49,6 +49,16 @@ after their uploads (if you do, you must supply one for each
disk):
   -oo rhv-disk-uuid=UUID          Disk UUID
 ")
 
+let is_nonnil_uuid uuid +  let nil_uuid =
"00000000-0000-0000-0000-000000000000" in
+  let rex_uuid = lazy (
+    let hex = "[a-fA-F0-9]" in
+    let str = sprintf "^%s{8}-%s{4}-%s{4}-%s{4}-%s{12}$" hex hex hex
hex hex in
+    PCRE.compile str
+  ) in
+  if uuid = nil_uuid then false
+  else PCRE.matches rex_uuid uuid
+
 let parse_output_options options    let rhv_cafile = ref None in
   let rhv_cluster = ref None in
@@ -71,6 +81,8 @@ let parse_output_options options      |
"rhv-verifypeer", "" -> rhv_verifypeer := true
     | "rhv-verifypeer", v -> rhv_verifypeer := bool_of_string v
     | "rhv-disk-uuid", v ->
+       if not (is_nonnil_uuid v) then
+         error (f_"-o rhv-upload: invalid UUID for -oo
rhv-disk-uuid");
        rhv_disk_uuids := Some (v :: (Option.default [] !rhv_disk_uuids))
     | k, _ ->
        error (f_"-o rhv-upload: unknown output option ‘-oo %s’") k
-- 
2.25.0

This makes the code fail with a sensible error message instead of cryptic error
from ovirtsdk.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 v2v/rhv-upload-plugin.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
index d3e6260e97f4..413ad53b05ab 100644
--- a/v2v/rhv-upload-plugin.py
+++ b/v2v/rhv-upload-plugin.py
@@ -26,6 +26,9 @@ import ssl
 import sys
 import time
 
+import nbdkit
+import errno
+
 from http.client import HTTPSConnection, HTTPConnection
 from urllib.parse import urlparse
 
@@ -461,6 +464,15 @@ def create_disk(connection):
     system_service = connection.system_service()
     disks_service = system_service.disks_service()
 
+    uuid = params.get('rhv_disk_uuid')
+    if uuid is not None:
+        try:
+            disks_service.disk_service(uuid).get()
+            nbdkit.set_error(errno.EEXIST)
+            raise ValueError("Disk with the UUID '%s' already
exists" % uuid)
+        except sdk.NotFoundError:
+            pass
+
     if params['disk_format'] == "raw":
         disk_format = types.DiskFormat.RAW
     else:
-- 
2.25.0

On Wed, Jan 29, 2020 at 03:34:48PM +0100, Martin Kletzander
wrote:
>The validation helps us fail early and with a sensible error message.  The
NIL
>UUID is not valid for oVirt, but other than that there is no other logic in
>there merely because the UUID types are a matter of the generator and they
are
>just forwarded in this partucular case.
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> v2v/output_rhv_upload.ml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/v2v/output_rhv_upload.ml b/v2v/output_rhv_upload.ml
>index 14153db36897..6482460f8de8 100644
>--- a/v2v/output_rhv_upload.ml
>+++ b/v2v/output_rhv_upload.ml
>@@ -49,6 +49,16 @@ after their uploads (if you do, you must supply one for
each disk):
>   -oo rhv-disk-uuid=UUID          Disk UUID
> ")
>
>+let is_nonnil_uuid uuid >+  let nil_uuid =
"00000000-0000-0000-0000-000000000000" in
>+  let rex_uuid = lazy (
>+    let hex = "[a-fA-F0-9]" in
>+    let str = sprintf "^%s{8}-%s{4}-%s{4}-%s{4}-%s{12}$" hex hex
hex hex hex in
>+    PCRE.compile str
>+  ) in
>+  if uuid = nil_uuid then false
>+  else PCRE.matches rex_uuid uuid
>+
> let parse_output_options options >   let rhv_cafile = ref None in
>   let rhv_cluster = ref None in
>@@ -71,6 +81,8 @@ let parse_output_options options >     |
"rhv-verifypeer", "" -> rhv_verifypeer := true
>     | "rhv-verifypeer", v -> rhv_verifypeer := bool_of_string
v
>     | "rhv-disk-uuid", v ->
>+       if not (is_nonnil_uuid v) then
>+         error (f_"-o rhv-upload: invalid UUID for -oo
rhv-disk-uuid");
>        rhv_disk_uuids := Some (v :: (Option.default [] !rhv_disk_uuids))
>     | k, _ ->
>        error (f_"-o rhv-upload: unknown output option ‘-oo %s’")
k
>-- 
I was too fast with the sending, consider this squashed in, (the fixed version
is in my github repo [1]):

diff --git i/v2v/output_rhv_upload.ml w/v2v/output_rhv_upload.ml
index 6482460f8de8..c06c4c1844de 100644
--- i/v2v/output_rhv_upload.ml
+++ w/v2v/output_rhv_upload.ml
@@ -57,7 +57,7 @@ let is_nonnil_uuid uuid       PCRE.compile str
    ) in
    if uuid = nil_uuid then false
-  else PCRE.matches rex_uuid uuid
+  else PCRE.matches (Lazy.force rex_uuid) uuid

  let parse_output_options options     let rhv_cafile = ref None in
--

[1] https://github.com/nertpinx/virt-v2v/tree/uuids
>2.25.0
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

On Wednesday, 29 January 2020 15:34:49 CET Martin Kletzander
wrote:
> This makes the code fail with a sensible error message instead of cryptic
error
> from ovirtsdk.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  v2v/rhv-upload-plugin.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/v2v/rhv-upload-plugin.py b/v2v/rhv-upload-plugin.py
> index d3e6260e97f4..413ad53b05ab 100644
> --- a/v2v/rhv-upload-plugin.py
> +++ b/v2v/rhv-upload-plugin.py
> @@ -26,6 +26,9 @@ import ssl
>  import sys
>  import time
>  
> +import nbdkit
> +import errno
> +
>  from http.client import HTTPSConnection, HTTPConnection
>  from urllib.parse import urlparse
>  
> @@ -461,6 +464,15 @@ def create_disk(connection):
>      system_service = connection.system_service()
>      disks_service = system_service.disks_service()
>  
> +    uuid = params.get('rhv_disk_uuid')
> +    if uuid is not None:
> +        try:
> +            disks_service.disk_service(uuid).get()
> +            nbdkit.set_error(errno.EEXIST)
> +            raise ValueError("Disk with the UUID '%s' already
exists" % uuid)
> +        except sdk.NotFoundError:
> +            pass
This check seems correct to me, although it is done too late: IMHO this
is something to do in the precheck script, so we do not even attempt to
connect to the source if any of the specified UUIDs already exists in
oVirt.

-- 
Pino Toscano