Possibly bad uuid generator in dependencies

[aarondl]

gouuid is being used by this package but there's an open issue against it claiming that it doesn't correctly generate uuids.

See: nu7hatch/gouuid#28

Apart from that it seems like it's a generally unmaintained piece of software with the latest commit being 2 years ago.

The issue suggests moving to: https://github.com/satori/go.uuid

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/108581258.

[rosenhouse]

hey @aarondl I'm looking at the V4 implementation in satori/go.uuid and the one here. I don't see any meaningful differences. Could you explain what about the V4 code path is different between the two packages?

I'm also cross-posting this question to nu7hatch/gouuid#28

Thanks!

[aarondl]

@rosenhouse I'll admit I hadn't looked into the code directly. Now I have and there seems to be little difference in the v4 code.

Having said that the other reasons I had opened this issue is that there are still a number of issues opened against that library as well as several pull requests (some dealing with spec compliance). The last commit was Dec 21, 2013 while the other package was updated as recently as 28 days ago.

Some of the more serious open issues include:

• v3 uuid format doesn't match python library
• NewV4() should check number of bytes returned from rand.Read
• Valid UUID not parsing correctly

This library doesn't use gofmt or go lint:
http://goreportcard.com/report/nu7hatch/gouuid
While the other library has a perfect score:
http://goreportcard.com/report/satori/go.uuid

I think there's a pretty strong case to discard this library (and I don't think smoke-tests is the only thing in CF to be using it, fairly sure I saw it elsewhere too). Both are licensed MIT so there shouldn't be any trouble switching.

[medvedzver]

@aarondl Can you please submit a pull request with the library that fits the best?

[aarondl]

Can do. I probably should have done so from the start, but as a new contributor to CF I've been favoring opening discussions before PR's to make sure I'm not doing anything people would object to :) Expect PRs on gorouter, hm9000, statsd-injector and smoke-tests in the near future to remove this dependency from cloudfoundry.

[Amit-PivotalLabs]

Hey @aarondl

@ematpl brings up a good point, namely that the satori library has a different signature; it doesn't return an error and in fact can panic instead. This is quite undesirable, as developers using it would have to know to match all calls with a defer-recover to handle the error.

I believe your motivating concern here is licensing issues (the nu7hatch is missing some info). Seems like nu7hatch is abandoned so moving to something like satori makes sense, but we'd want it to return an error instead of panic during normal usage.

This PR: satori/go.uuid#12 introduced code to make it panic during init, which is reasonable, but it should not panic when calling the normal library functions. Would you be willing to submit a PR to their repo, and then submitting the PR to this repo to switch to satori after they merge?

[aarondl]

Hi @Amit-PivotalLabs,

After perusing the Go source code, it's clear that the only way crypto/rand.Read() really fails is when the source of entropy fails. This means that there's no real way to recover except for using an alternative source of randomness or to retry, and we all know that math/rand is an insufficient source of entropy.

Since we can't proceed in our program when we're unable to create a UUID in almost any situation the only solution then becomes to retry. And it feels so rare that /dev/urandom would fail, or that it could recover after a period of time if it were to fail. I think the library does the correct thing in using panic here. Sometimes you don't want your program to be able to continue, when /dev/urandom fails is probably one of them.

In addition there are many other users of this library and performing a breaking change on their API so we could use it here doesn't seem appropriate.

I'd prefer to use the library as is, as a panic is certainly better than a silent error that will occur in the current library being used in these CF components.

[aarondl]

Argh. I have to retract the previous comment as I had read the wrong chunk of code, there is no silent error.

Even so I can't really break the API of the other library, so we can go ahead and close this issue. Sorry for the noise!

[Amit-PivotalLabs]

nu7hatch returns an error instead of panicking. I would always rather handle a returned error (and then choose as the client whether or not panic is appropriate), rather than call out to some library which panics unbeknownst to me. It is a breaking API change, but you could see whether they would go for it.

I would argue to satori that the PR they pulled in: satori/go.uuid#12, makes its case for panicking in init by referencing: https://golang.org/doc/effective_go.html#panic. But that very document itself says:

... real library functions should avoid panic. If the problem can be masked or worked around, it's always better to let things continue to run rather than taking down the whole program. One possible counterexample is during initialization: if the library truly cannot set itself up, it might be reasonable to panic, so to speak.

If you don't feel comfortable suggesting that change to satori, that's cool too.

[aarondl]

I somewhat agree. But I definitely won't be the one to suggest breaking API changes in order to get it in. Once I realized my mistake and that there's no silent error, I'm much less concerned about getting the library switched out. As unmaintained and potentially bad as gouuid is, it looks like for the small chunk of what CF uses it for, it might be good enough!