Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CP-34028: implement Uuid with Uuidm #4532

Merged
merged 10 commits into from
Apr 27, 2022
Merged

Conversation

psafont
Copy link
Member

@psafont psafont commented Sep 22, 2021

The Uuid module is problematic because it doesn't parse input at all. This PR tries to do a better job at that and replaces its base type from string to Uuidm, which should makes the representation more compact when used at the expense of conversion roundtrips when reading the database

In order to guard from invalid values it's necessary to change the interface to use Option: using exceptions would be dangerous and some uses might be missed and lead to unhandled exceptions.

Draft PR as it has a breaking change so clients need to be modified first; I would like some feedback before doing that.

TODO: The functions that generate random UUID do not use the ones provided by Uuidm. I don't think the Option.get is worrying because previously a different exception would have been raised, but there's a chance to make it more robust.

Notes:

  • While having the phantom type makes sense to avoid comparing uuid of different types of resources, xapi needs to have a dedicated type to model uuids instead of using strings.

  • On top of this, using Uuid.t means that for every roundtrip we're paying the cost of converting from the string representation to the raw byte value and viceversa which may impact performance, having the string representation with the new interface may be more desirable.

Original PR can be found in xapi-project/xen-api-libs-transitional#103

@psafont psafont force-pushed the reuuid branch 2 times, most recently from 74e4df6 to 0979a35 Compare September 24, 2021 16:02
@psafont psafont force-pushed the reuuid branch 2 times, most recently from a7a6860 to c76f9f9 Compare November 1, 2021 14:21
@psafont psafont force-pushed the reuuid branch 3 times, most recently from 954f3d5 to 90c9c7d Compare February 23, 2022 10:55
@psafont psafont force-pushed the reuuid branch 2 times, most recently from fea7521 to 068d274 Compare March 16, 2022 13:41

val make_uuid_prng : unit -> 'a t
val make : unit -> 'a t
(** Create a fresh UUID *)

val make_uuid_urnd : unit -> 'a t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two functions to create a UUID. It's not obvious when to use which.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the make_uuid_urnd and document the random source for make.

@lindig
Copy link
Contributor

lindig commented Mar 17, 2022

I think this is a good change but it touches a lot of code. Random thoughts:

  • The old code supported UUIDs of different length and we always use the V4 variant. I assume we don't need the flexibility to change this and Uuid as a module does not need to be as general.

  • Most use cases need a UUID in string representation. This could be supported directly with a make_string or similar function to avoid Uuid.to_string (Uuid.make ()) everywhere.

  • Given that we mostly use strings, where is the compact representation used?

@psafont
Copy link
Member Author

psafont commented Mar 17, 2022

The old code supported UUIDs of different length and we always use the V4 variant. I assume we don't need the flexibility to change this and Uuid as a module does not need to be as general.

This is the first time I hear UUIDs can have variable length, RFC 4122 is clear on this:

The UUID format is 16 octets; some bits of the eight octet variant field specified below determine finer structure.

Is there a non-standard source of UUIDs that we have to support? I'll test the changes before undrafting anyway to try to discover any unpleasant surprises

Most use cases need a UUID in string representation. This could be supported directly with a make_string or similar function to avoid Uuid.to_string (Uuid.make ()) everywhere.

Yeah, this makes sense

Given that we mostly use strings, where is the compact representation used?

I would like to use the compact representation more often and push strings only for storing to the database, it has the added benefit of specializing the type to a class of objects, e.g. [ `Vm] Uuid.t so they cannot be mixed up.

@psafont psafont force-pushed the reuuid branch 2 times, most recently from 18e51fb to 5983d6e Compare March 25, 2022 10:36
@edwintorok
Copy link
Contributor

We have to be careful here to keep using a secure way of generating UUIDs, because the session UUID is used as an access token in API calls once a password login has succeeded.
The code here uses /dev/urandom which should be safe enough (except perhaps during early boot).

Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the confclits? What remains to be done to get this ready?

@psafont psafont force-pushed the reuuid branch 2 times, most recently from 39013d6 to 49ba4d7 Compare April 13, 2022 15:46
@psafont
Copy link
Member Author

psafont commented Apr 13, 2022

I think now it's much better than before, it needs a build for end-to-end testing
SR: 167637 ring3 BST + BVT is all green

@psafont psafont marked this pull request as ready for review April 13, 2022 15:46
Also adds a pretty_printer and an equal function

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Change signature functions that generate UUIDs from inputs to return a
UUID as an option. They now transform the input to make sure the
resulting string is a valid UUID.

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Also reorganized the functions and signatures to group similar
functionality together.

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
It's useful to extract uuid from hashed bytes

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Compared to /dev/random uses the same CSPRNG (since Linux 4.8) and it
doesn't block

OCaml's PRNG is not a CSPRNG as it is predictable

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Now the only modules where Uuidm is used is inside Uuid, and in
varstoredguard's RPC. This is to avoid using GADTs for little benefit

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Now all alcotest binaries are executed in compact mode when running make
test. This allows the developers to run the tests with dune runtest
--profile=debug to see all the output of test if they wish to.

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@psafont psafont merged commit 2c19fa7 into xapi-project:master Apr 27, 2022
@psafont psafont deleted the reuuid branch April 27, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants